* 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
