Re: [PATCH v3 4/4] x86/CPU/AMD: Print the reason for the last reset

[Ingo Molnar]

* Mario Limonciello <superm1@xxxxxxxxxx> wrote:

> 
> 
> On 4/11/25 07:06, Borislav Petkov wrote:
> > On Thu, Apr 10, 2025 at 03:02:02PM -0500, Mario Limonciello wrote:
> > > +static __init int print_s5_reset_status_mmio(void)
> > > +{
> > > +	void __iomem *addr;
> > > +	unsigned long value;
> > > +	int bit = -1;
> > > +
> > > +	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
> > > +		return 0;
> > > +
> > > +	addr = ioremap(FCH_PM_BASE + FCH_PM_S5_RESET_STATUS, sizeof(value));
> > > +	if (!addr)
> > > +		return 0;
> > 
> > newline.
> > 
> > > +	value = ioread32(addr);
> > > +	iounmap(addr);
> > > +
> > > +	do {
> > > +		bit = find_next_bit(&value, BITS_PER_LONG, bit + 1);
> > > +	} while (!s5_reset_reason_txt[bit]);
> > 
> > What's the idea here? The highest bit is the most fitting one?
> > 
> > So why don't you do fls() or so?
> 
> The idea was to walk all the bits and pick the first one that has a string
> associated with it.  I was finding that sometimes the reserved bits are set
> which would get you a NULL pointer deref.

Would it be possible for firmware to set multiple bits with a text 
behind it?

BTW:

+       [32] = "unknown",

but BITS_PER_LONG is 64 on x86-64, not 32. How is that supposed to 
work?

Anyway, in terms of robustness, it would be best to assume nothing 
about the structure of the bitmask, and do something straightforward 
like this:

	unsigned long value;
	int nr_reasons = 0;
	int bit = -1;

	...

	/* Iterate on each bit in the 'value' mask: */

	for (;;) {
		bit = find_next_bit(&value, BITS_PER_LONG, bit + 1);

		/* Reached the end of the word, no more bits: */
		if (bit >= BITS_PER_LONG) {
			if (!nr_reasons)
				pr_info("x86/amd: Previous system reset reason [0x%08lx]: Unknown\n", value);
			break;
		}

		nr_reasons++;
		pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n",
			value, s5_reset_reason_txt[bit]);
	}

which prints out multiple bits as well, and does the right thing if no 
bit is found, without having to encode BITS_PER_LONG in the 
s5_reset_reason_txt[] array.

And BTW: thanks for implementing this. :-)

Thanks,

	Ingo