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

From: Yazen Ghannam <yazen.ghannam@amd.com>

The following register contains bits that indicate the cause for the
previous reset.

        PMx000000C0 (FCH::PM::S5_RESET_STATUS)

This is useful for debug. The reasons for reset are broken into 6 high
level categories. Decode it by category and print during boot.

Specifics within a category are split off into debugging documentation.

The register is accessed indirectly through a "PM" port in the FCH. Use
MMIO access in order to avoid restrictions with legacy port access.

Use a late_initcall() to ensure that MMIO has been set up before trying
to access the register.

This register was introduced with AMD Family 17h, so avoid access on
older families. There is no CPUID feature bit for this register.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3:
 * Align strings in the CSV and code.
 * Switch to an array of strings
 * Switch to looking up bit of first value
 * Re-order message to have number first (makes grepping easier)
 * Add x86/amd prefix to message
v2:
 * Add string for each reason, but still include value in case multiple
   values are set.
---
 Documentation/admin-guide/amd/index.rst | 42 ++++++++++++++++++++
 arch/x86/include/asm/amd_node.h         |  1 +
 arch/x86/kernel/cpu/amd.c               | 51 +++++++++++++++++++++++++
 3 files changed, 94 insertions(+)

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?

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

What's guaranteeing that s5_reset_reason_txt[bit] is still set here?

I'd suggest you check it again and never trust the hw because we'll be fixing
a null ptr here at some point otherwise...

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.

> 
>> +	pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n",
>> +		value, s5_reset_reason_txt[bit]);
> 
> What's guaranteeing that s5_reset_reason_txt[bit] is still set here?
> 
> I'd suggest you check it again and never trust the hw because we'll be fixing
> a null ptr here at some point otherwise...
> 

Right; I was worried about that too but find_next_bit() will return the 
size argument when it doesn't find anything.

So that should be s5_reset_reason_txt[32] which has the "Unknown" string.

On Fri, Apr 11, 2025 at 07:12:24AM -0500, Mario Limonciello wrote:
> 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.

Uff, that needs a comment at least.

But you can write it a lot simpler instead:

	for (i = 0; i <= ARRAY_SIZE(s5_reset_reason_txt); i++) {
		if (!(value & BIT(i)))
			continue;

		if (s5_reset_reason_txt[i])
			break;
	}

Simple loop, simple statements and all easy. :-)

> Right; I was worried about that too but find_next_bit() will return the size
> argument when it doesn't find anything.
> 
> So that should be s5_reset_reason_txt[32] which has the "Unknown" string.

Yeah, that definitely needs a comment above it.

On 4/11/25 07:50, Borislav Petkov wrote:
> On Fri, Apr 11, 2025 at 07:12:24AM -0500, Mario Limonciello wrote:
>> 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.
> 
> Uff, that needs a comment at least.
> 
> But you can write it a lot simpler instead:
> 
> 	for (i = 0; i <= ARRAY_SIZE(s5_reset_reason_txt); i++) {
> 		if (!(value & BIT(i)))
> 			continue;
> 
> 		if (s5_reset_reason_txt[i])
> 			break;
> 	}
> 
> Simple loop, simple statements and all easy. :-)
> 
>> Right; I was worried about that too but find_next_bit() will return the size
>> argument when it doesn't find anything.
>>
>> So that should be s5_reset_reason_txt[32] which has the "Unknown" string.
> 
> Yeah, that definitely needs a comment above it.
> 

Thanks; I'll take your simpler solution and leave a comment above the 
!(value & BIT(i)) check about skipping reserved bits.

* Mario Limonciello <superm1@kernel.org> 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