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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_rBFMp34zIYkRwh@gmail.com>
Date: Sat, 12 Apr 2025 21:37:56 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Mario Limonciello <superm1@...nel.org>
Cc: Borislav Petkov <bp@...en8.de>, Jean Delvare <jdelvare@...e.com>,
	Andi Shyti <andi.shyti@...nel.org>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Jonathan Corbet <corbet@....net>,
	Mario Limonciello <mario.limonciello@....com>,
	Yazen Ghannam <yazen.ghannam@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>,
	Shyam Sundar S K <Shyam-sundar.S-k@....com>,
	Hans de Goede <hdegoede@...hat.com>,
	"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:I2C/SMBUS CONTROLLER DRIVERS FOR PC" <linux-i2c@...r.kernel.org>,
	"open list:AMD PMC DRIVER" <platform-driver-x86@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] x86/CPU/AMD: Print the reason for the last reset


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