[PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Yazen Ghannam]

The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
commonly used error response from hardware. This may occur due to a real
hardware issue or when running in a VM.

The user will see all reset reasons reported in this case.

Check for an error response value and return early to avoid decoding
invalid data.

Also, adjust the data variable type to match the hardware register size.

Fixes: ab8131028710 ("x86/CPU/AMD: Print the reason for the last reset")
Reported-by: Libing He <libhe@redhat.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: David Arcari <darcari@redhat.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/cpu/amd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 50f88fe51816..db0f9e0d181a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1274,8 +1274,8 @@ static const char * const s5_reset_reason_txt[] = {
 
 static __init int print_s5_reset_status_mmio(void)
 {
-	unsigned long value;
 	void __iomem *addr;
+	u32 value;
 	int i;
 
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
@@ -1288,12 +1288,16 @@ static __init int print_s5_reset_status_mmio(void)
 	value = ioread32(addr);
 	iounmap(addr);
 
+	/* Value with "all bits set" is an error response and should be ignored. */
+	if (value == U32_MAX)
+		return 0;
+
 	for (i = 0; i < ARRAY_SIZE(s5_reset_reason_txt); i++) {
 		if (!(value & BIT(i)))
 			continue;
 
 		if (s5_reset_reason_txt[i]) {
-			pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n",
+			pr_info("x86/amd: Previous system reset reason [0x%08x]: %s\n",
 				value, s5_reset_reason_txt[i]);
 		}
 	}

base-commit: 65f55a30176662ee37fe18b47430ee30b57bfc98
-- 
2.50.1

[PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Yazen Ghannam]

The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
commonly used error response from hardware. This may occur due to a real
hardware issue or when running in a VM.

The user will see all reset reasons reported in this case.

Check for an error response value and return early to avoid decoding
invalid data.

Also, adjust the data variable type to match the hardware register size.

Fixes: ab8131028710 ("x86/CPU/AMD: Print the reason for the last reset")
Reported-by: Libing He <libhe@redhat.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: David Arcari <darcari@redhat.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/cpu/amd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 50f88fe51816..db0f9e0d181a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1274,8 +1274,8 @@ static const char * const s5_reset_reason_txt[] = {
 
 static __init int print_s5_reset_status_mmio(void)
 {
-	unsigned long value;
 	void __iomem *addr;
+	u32 value;
 	int i;
 
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
@@ -1288,12 +1288,16 @@ static __init int print_s5_reset_status_mmio(void)
 	value = ioread32(addr);
 	iounmap(addr);
 
+	/* Value with "all bits set" is an error response and should be ignored. */
+	if (value == U32_MAX)
+		return 0;
+
 	for (i = 0; i < ARRAY_SIZE(s5_reset_reason_txt); i++) {
 		if (!(value & BIT(i)))
 			continue;
 
 		if (s5_reset_reason_txt[i]) {
-			pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n",
+			pr_info("x86/amd: Previous system reset reason [0x%08x]: %s\n",
 				value, s5_reset_reason_txt[i]);
 		}
 	}

base-commit: 65f55a30176662ee37fe18b47430ee30b57bfc98
-- 
2.50.1

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Mario Limonciello]

On 7/21/25 1:11 PM, Yazen Ghannam wrote:
> The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
> commonly used error response from hardware. This may occur due to a real
> hardware issue or when running in a VM.
> 
> The user will see all reset reasons reported in this case.
> 
> Check for an error response value and return early to avoid decoding
> invalid data.
> 
> Also, adjust the data variable type to match the hardware register size.
> 
> Fixes: ab8131028710 ("x86/CPU/AMD: Print the reason for the last reset")
> Reported-by: Libing He <libhe@redhat.com>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: David Arcari <darcari@redhat.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: stable@vger.kernel.org
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   arch/x86/kernel/cpu/amd.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 50f88fe51816..db0f9e0d181a 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1274,8 +1274,8 @@ static const char * const s5_reset_reason_txt[] = {
>   
>   static __init int print_s5_reset_status_mmio(void)
>   {
> -	unsigned long value;
>   	void __iomem *addr;
> +	u32 value;
>   	int i;
>   
>   	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
> @@ -1288,12 +1288,16 @@ static __init int print_s5_reset_status_mmio(void)
>   	value = ioread32(addr);
>   	iounmap(addr);
>   
> +	/* Value with "all bits set" is an error response and should be ignored. */
> +	if (value == U32_MAX)
> +		return 0;
> +
>   	for (i = 0; i < ARRAY_SIZE(s5_reset_reason_txt); i++) {
>   		if (!(value & BIT(i)))
>   			continue;
>   
>   		if (s5_reset_reason_txt[i]) {
> -			pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n",
> +			pr_info("x86/amd: Previous system reset reason [0x%08x]: %s\n",
>   				value, s5_reset_reason_txt[i]);
>   		}
>   	}
> 
> base-commit: 65f55a30176662ee37fe18b47430ee30b57bfc98

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Borislav Petkov]

On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
> The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
> commonly used error response from hardware. This may occur due to a real
> hardware issue or when running in a VM.

Well, which is it Libing is reporting? VM or a real hw issue?

If it is a VM, is that -1 the only thing a VMM returns when reading that
MMIO address or can it be anything?

If latter, you need to check X86_FEATURE_HYPERVISOR.

Same for a real hw issue.

IOW, is -1 the *only* invalid data we can read here or are we playing
whack-a-mole with it?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Yazen Ghannam]

On Tue, Jul 22, 2025 at 06:56:15PM +0200, Borislav Petkov wrote:
> On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
> > The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
> > commonly used error response from hardware. This may occur due to a real
> > hardware issue or when running in a VM.
> 
> Well, which is it Libing is reporting? VM or a real hw issue?
> 

In this case, it was a VM.

> If it is a VM, is that -1 the only thing a VMM returns when reading that
> MMIO address or can it be anything?
> 
> If latter, you need to check X86_FEATURE_HYPERVISOR.
> 
> Same for a real hw issue.
> 
> IOW, is -1 the *only* invalid data we can read here or are we playing
> whack-a-mole with it?
> 

I see you're point, but I don't think we can know for sure all possible
cases. There are some reserved bits that shouldn't be set. But these
definitions could change in the future.

And it'd be a pain to try and verify combinations of bits and configs.
Like can bit A and B be set together, or can bit C be set while running
in a VM, or can bit D ever be set on Model Z? 

The -1 (all bits set) is the only "applies to all cases" invalid data,
since this is a common hardware error response. So we can at least check
for this.

Thanks,
Yazen

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Borislav Petkov]

On July 23, 2025 9:34:26 PM GMT+03:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>On Tue, Jul 22, 2025 at 06:56:15PM +0200, Borislav Petkov wrote:
>> On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
>> > The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
>> > commonly used error response from hardware. This may occur due to a real
>> > hardware issue or when running in a VM.
>> 
>> Well, which is it Libing is reporting? VM or a real hw issue?
>> 
>
>In this case, it was a VM.
>
>> If it is a VM, is that -1 the only thing a VMM returns when reading that
>> MMIO address or can it be anything?
>> 
>> If latter, you need to check X86_FEATURE_HYPERVISOR.
>> 
>> Same for a real hw issue.
>> 
>> IOW, is -1 the *only* invalid data we can read here or are we playing
>> whack-a-mole with it?
>> 
>
>I see you're point, but I don't think we can know for sure all possible
>cases. There are some reserved bits that shouldn't be set. But these
>definitions could change in the future.
>
>And it'd be a pain to try and verify combinations of bits and configs.
>Like can bit A and B be set together, or can bit C be set while running
>in a VM, or can bit D ever be set on Model Z? 
>
>The -1 (all bits set) is the only "applies to all cases" invalid data,
>since this is a common hardware error response. So we can at least check
>for this.
>
>Thanks,
>Yazen

I think you should check both: HV or -1.

HV covers the VM angle as they don't emulate this and we simply should disable this functionality when running as a guest.

-1 covers the known-bad hw value.

Thx.

-- 
Sent from a small device: formatting sucks and brevity is inevitable.

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Yazen Ghannam]

On Wed, Jul 23, 2025 at 10:35:31PM +0300, Borislav Petkov wrote:
> On July 23, 2025 9:34:26 PM GMT+03:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >On Tue, Jul 22, 2025 at 06:56:15PM +0200, Borislav Petkov wrote:
> >> On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
> >> > The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
> >> > commonly used error response from hardware. This may occur due to a real
> >> > hardware issue or when running in a VM.
> >> 
> >> Well, which is it Libing is reporting? VM or a real hw issue?
> >> 
> >
> >In this case, it was a VM.
> >
> >> If it is a VM, is that -1 the only thing a VMM returns when reading that
> >> MMIO address or can it be anything?
> >> 
> >> If latter, you need to check X86_FEATURE_HYPERVISOR.
> >> 
> >> Same for a real hw issue.
> >> 
> >> IOW, is -1 the *only* invalid data we can read here or are we playing
> >> whack-a-mole with it?
> >> 
> >
> >I see you're point, but I don't think we can know for sure all possible
> >cases. There are some reserved bits that shouldn't be set. But these
> >definitions could change in the future.
> >
> >And it'd be a pain to try and verify combinations of bits and configs.
> >Like can bit A and B be set together, or can bit C be set while running
> >in a VM, or can bit D ever be set on Model Z? 
> >
> >The -1 (all bits set) is the only "applies to all cases" invalid data,
> >since this is a common hardware error response. So we can at least check
> >for this.
> >
> >Thanks,
> >Yazen
> 
> I think you should check both: HV or -1.
> 
> HV covers the VM angle as they don't emulate this and we simply should disable this functionality when running as a guest.
> 
> -1 covers the known-bad hw value.
> 

Okay, will do.

Thanks,
Yazen

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Sean Christopherson]

On Wed, Jul 23, 2025, Borislav Petkov wrote:
> On July 23, 2025 9:34:26 PM GMT+03:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> >On Tue, Jul 22, 2025 at 06:56:15PM +0200, Borislav Petkov wrote:
> >> On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
> >> > The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
> >> > commonly used error response from hardware. This may occur due to a real
> >> > hardware issue or when running in a VM.
> >> 
> >> Well, which is it Libing is reporting? VM or a real hw issue?
> >> 
> >
> >In this case, it was a VM.
> >
> >> If it is a VM, is that -1 the only thing a VMM returns when reading that
> >> MMIO address or can it be anything?
> >> 
> >> If latter, you need to check X86_FEATURE_HYPERVISOR.
> >> 
> >> Same for a real hw issue.
> >> 
> >> IOW, is -1 the *only* invalid data we can read here or are we playing
> >> whack-a-mole with it?
> >> 
> >
> >I see you're point, but I don't think we can know for sure all possible
> >cases. There are some reserved bits that shouldn't be set. But these
> >definitions could change in the future.
> >
> >And it'd be a pain to try and verify combinations of bits and configs.
> >Like can bit A and B be set together, or can bit C be set while running
> >in a VM, or can bit D ever be set on Model Z? 
> >
> >The -1 (all bits set) is the only "applies to all cases" invalid data,
> >since this is a common hardware error response. So we can at least check
> >for this.
> >
> >Thanks,
> >Yazen
> 
> I think you should check both: HV or -1.
> 
> HV covers the VM angle as they don't emulate this

You can't possibly know that.  If there exists a hardware spec of any kind, it's
fair game for emulation.

> and we simply should disable this functionality when running as a guest.
> 
> -1 covers the known-bad hw value.

And in a guest, -1, i.e. 0xffffffff is all but guaranteed to come from the VMM
providing PCI master abort semantics for reads to MMIO where no device exists.
That's about as "architectural" of behavior as you're going to get, so I don't
see any reason to assume no VMM will every emulate whatever this feature is.

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Mario Limonciello]

On 7/24/2025 3:58 PM, Sean Christopherson wrote:
> On Wed, Jul 23, 2025, Borislav Petkov wrote:
>> On July 23, 2025 9:34:26 PM GMT+03:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
>>> On Tue, Jul 22, 2025 at 06:56:15PM +0200, Borislav Petkov wrote:
>>>> On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
>>>>> The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
>>>>> commonly used error response from hardware. This may occur due to a real
>>>>> hardware issue or when running in a VM.
>>>>
>>>> Well, which is it Libing is reporting? VM or a real hw issue?
>>>>
>>>
>>> In this case, it was a VM.
>>>
>>>> If it is a VM, is that -1 the only thing a VMM returns when reading that
>>>> MMIO address or can it be anything?
>>>>
>>>> If latter, you need to check X86_FEATURE_HYPERVISOR.
>>>>
>>>> Same for a real hw issue.
>>>>
>>>> IOW, is -1 the *only* invalid data we can read here or are we playing
>>>> whack-a-mole with it?
>>>>
>>>
>>> I see you're point, but I don't think we can know for sure all possible
>>> cases. There are some reserved bits that shouldn't be set. But these
>>> definitions could change in the future.
>>>
>>> And it'd be a pain to try and verify combinations of bits and configs.
>>> Like can bit A and B be set together, or can bit C be set while running
>>> in a VM, or can bit D ever be set on Model Z?
>>>
>>> The -1 (all bits set) is the only "applies to all cases" invalid data,
>>> since this is a common hardware error response. So we can at least check
>>> for this.
>>>
>>> Thanks,
>>> Yazen
>>
>> I think you should check both: HV or -1.
>>
>> HV covers the VM angle as they don't emulate this
> 
> You can't possibly know that.  If there exists a hardware spec of any kind, it's
> fair game for emulation.
> 
>> and we simply should disable this functionality when running as a guest.
>>
>> -1 covers the known-bad hw value.
> 
> And in a guest, -1, i.e. 0xffffffff is all but guaranteed to come from the VMM
> providing PCI master abort semantics for reads to MMIO where no device exists.
> That's about as "architectural" of behavior as you're going to get, so I don't
> see any reason to assume no VMM will every emulate whatever this feature is.

I don't really understand why there would be any value in a VMM 
emulating this feature.  It's specifically about the reason the hardware 
saw for the last reboot.  Those reasons are *hardware reasons*.  IE, 
you're never going to see a thermal event as the reason a guest was 
rebooted.

CF9 reset or ACPI power state transition are about all I can envision 
for guest reboot reasons.  And even then do you *want* the to really 
have the VMM track the reasons for a guest reboot?

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Sean Christopherson]

On Thu, Jul 24, 2025, Mario Limonciello wrote:
> On 7/24/2025 3:58 PM, Sean Christopherson wrote:
> > On Wed, Jul 23, 2025, Borislav Petkov wrote:
> > > On July 23, 2025 9:34:26 PM GMT+03:00, Yazen Ghannam <yazen.ghannam@amd.com> wrote:
> > > > On Tue, Jul 22, 2025 at 06:56:15PM +0200, Borislav Petkov wrote:
> > > > > On Mon, Jul 21, 2025 at 06:11:54PM +0000, Yazen Ghannam wrote:
> > > > > > The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
> > > > > > commonly used error response from hardware. This may occur due to a real
> > > > > > hardware issue or when running in a VM.
> > > > > 
> > > > > Well, which is it Libing is reporting? VM or a real hw issue?
> > > > > 
> > > > 
> > > > In this case, it was a VM.
> > > > 
> > > > > If it is a VM, is that -1 the only thing a VMM returns when reading that
> > > > > MMIO address or can it be anything?
> > > > > 
> > > > > If latter, you need to check X86_FEATURE_HYPERVISOR.
> > > > > 
> > > > > Same for a real hw issue.
> > > > > 
> > > > > IOW, is -1 the *only* invalid data we can read here or are we playing
> > > > > whack-a-mole with it?
> > > > > 
> > > > 
> > > > I see you're point, but I don't think we can know for sure all possible
> > > > cases. There are some reserved bits that shouldn't be set. But these
> > > > definitions could change in the future.
> > > > 
> > > > And it'd be a pain to try and verify combinations of bits and configs.
> > > > Like can bit A and B be set together, or can bit C be set while running
> > > > in a VM, or can bit D ever be set on Model Z?
> > > > 
> > > > The -1 (all bits set) is the only "applies to all cases" invalid data,
> > > > since this is a common hardware error response. So we can at least check
> > > > for this.
> > > > 
> > > > Thanks,
> > > > Yazen
> > > 
> > > I think you should check both: HV or -1.
> > > 
> > > HV covers the VM angle as they don't emulate this
> > 
> > You can't possibly know that.  If there exists a hardware spec of any kind, it's
> > fair game for emulation.
> > 
> > > and we simply should disable this functionality when running as a guest.
> > > 
> > > -1 covers the known-bad hw value.
> > 
> > And in a guest, -1, i.e. 0xffffffff is all but guaranteed to come from the VMM
> > providing PCI master abort semantics for reads to MMIO where no device exists.
> > That's about as "architectural" of behavior as you're going to get, so I don't
> > see any reason to assume no VMM will every emulate whatever this feature is.
> 
> I don't really understand why there would be any value in a VMM emulating
> this feature.  It's specifically about the reason the hardware saw for the
> last reboot.  Those reasons are *hardware reasons*.  IE, you're never going
> to see a thermal event as the reason a guest was rebooted.

Not necessarily.  There are a variety of use cases for doing nearly-full passthrough
of bare metal state into a VM, e.g. to deprivilege the "main" OS, interpose and/or
isolate select resources, etc.  I don't think it's too far fetched to imagine one
or more such use cases exposing this range of MMIO to the guest _and_ also setting
the HYPERVISOR bit in CPUID.

But whether or not there's a legitimate use case is beside the point.  I'm not
arguing this is at all useful for VMs.  I'm arguing _against_ splattering
X86_FEATURE_HYPERVISOR checks all over the place just because an error was first
(or only) observed while running in a VM.

If a VMM is operating out of spec, then fix the VMM.  If the behavior is legal
but unexpected, then address it in the guest.  Neither of those requires gating
code on X86_FEATURE_HYPERVISOR.

> CF9 reset or ACPI power state transition are about all I can envision for
> guest reboot reasons.  And even then do you *want* the to really have the
> VMM track the reasons for a guest reboot?

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Borislav Petkov]

On Thu, Jul 24, 2025 at 02:32:41PM -0700, Sean Christopherson wrote:
> Not necessarily.  There are a variety of use cases for doing nearly-full passthrough
> of bare metal state into a VM, e.g. to deprivilege the "main" OS, interpose and/or
> isolate select resources, etc.  I don't think it's too far fetched to imagine one
> or more such use cases exposing this range of MMIO to the guest _and_ also setting
> the HYPERVISOR bit in CPUID.

What would be that use case?

To tell the guest why the hypervisor rebooted?

Sounds weird to me but virt does weird things sometimes. :-P

> But whether or not there's a legitimate use case is beside the point.  I'm not
> arguing this is at all useful for VMs.  I'm arguing _against_ splattering
> X86_FEATURE_HYPERVISOR checks all over the place just because an error was first
> (or only) observed while running in a VM.

We use X86_FEATURE_HYPERVISOR to gate purely-hw-only features behind it.
Because they don't make any sense for guests. Just like this one.

And even if this one makes sense for some virt scenario, we want those
folks to *actually* explain why removing the HV check for that feature
makes sense. And why the kernel needs to support it.

Just like loading microcode in a guest, for example. There's a reason we
don't do that. And when some day, someone appears and wants to do that,
I would like there to be an explicit patch removing that HV check in the
loader and explaining *why* it is doing so.

And until that day, that feature is hw-only. Just like this one.

And yeah, I know you don't like X86_FEATURE_HYPERVISOR but I would like
to save some of my sanity when looking at a bug report which says that
the reboot reason reporting is talking crap, only to find out that a HV
underneath is doing something silly, muddying up the waters.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Sean Christopherson]

On Fri, Jul 25, 2025, Borislav Petkov wrote:
> On Thu, Jul 24, 2025 at 02:32:41PM -0700, Sean Christopherson wrote:
> > Not necessarily.  There are a variety of use cases for doing nearly-full passthrough
> > of bare metal state into a VM, e.g. to deprivilege the "main" OS, interpose and/or
> > isolate select resources, etc.  I don't think it's too far fetched to imagine one
> > or more such use cases exposing this range of MMIO to the guest _and_ also setting
> > the HYPERVISOR bit in CPUID.
> 
> What would be that use case?
> 
> To tell the guest why the hypervisor rebooted?
> 
> Sounds weird to me but virt does weird things sometimes. :-P
> 
> > But whether or not there's a legitimate use case is beside the point.  I'm not
> > arguing this is at all useful for VMs.  I'm arguing _against_ splattering
> > X86_FEATURE_HYPERVISOR checks all over the place just because an error was first
> > (or only) observed while running in a VM.
> 
> We use X86_FEATURE_HYPERVISOR to gate purely-hw-only features behind it.

And I'm advocating that we stop doing that (with maybe a few exceptions), because
there are very, very few features that are truly bare metal only.  Some things,
e.g. SEV-SNP, might require proxying certain actions, but even SEV-SNP can be
exposed to a virtual machine.

> Because they don't make any sense for guests. Just like this one.

You're conflating running under (on?) a hypervisor with being a guest in the
traditional model of virtualization.  Running a privileged, host-owned software
stack in a virtual machine is beneficial/desirable for a variety of use cases,
and I expect the number of such use cases to grow in the near future.

E.g. as mentioned earlier, pKVM uses virtualization to de-privilege the kernel,
and Hyper-V's VBS uses virtualization to monitor accesses to sensitive state.

Future use cases could use virtualization:

 - to improve system stability/uptime, e.g. restart the VM if the kernel crashes
   as opposed to rebooting the entire host
 - to isolate different software components, e.g. to run post-boot functionality
   in a separate VM to limit the impact of an escape/flaw/bug
 - to limit the amount of code that has direct access to system physical resources
 - to partion a system into multiple machines (each running their own kernel)
   for performance reason; e.g. AIUI, kernel-wide locks are becomming more and
   more problematic as the number of cores continues to go up

Not all of those use cases will want to access "bare metal" features, but my
point is that inferring anything about the system setup based solely on the
HYPERVISOR flag is all but guaranteed to break sooner or later.

> And even if this one makes sense for some virt scenario, we want those
> folks to *actually* explain why removing the HV check for that feature
> makes sense. And why the kernel needs to support it.

That seems like a gigantic waste of time/effort.  E.g. imagine the pain/churn if
MONITOR/MWAIT had been limited to !HYPERVISOR simply because early VM setups
didn't support executing MONITOR/MWAIT natively.

> Just like loading microcode in a guest, for example. There's a reason we
> don't do that.  And when some day, someone appears and wants to do that, I
> would like there to be an explicit patch removing that HV check in the loader
> and explaining *why* it is doing so.

Patch loading is exceptional because, AFAIK, neither Intel nor AMD officially
support ucode updates while running as a guest.  Though FWIW, I do expect that
sooner or later someone will indeed want to do patch loads in a VM.

And even if Intel/AMD never support doing the actual patch load from guest code,
I can definitely see a trap-and-execute scheme, e.g. where the bare metal kernel
doesn't have network or file system access, and so the ucode patch needs to be
routed through a highly privileged VM.

> And until that day, that feature is hw-only. Just like this one.
> 
> And yeah, I know you don't like X86_FEATURE_HYPERVISOR 

I don't mind HYPERVISOR per se, what I don't like is disabling code (or going
down different paths) based solely on historical use cases, i.e. based on the
assumption that "no one will ever do xyz!".  I have no objection to pivoting
on HYPERVISOR if there is an explicitly documented limitation, or for cases where
there's a very high probability of making a bad decision, e.g. using FMS to
enumerate features.

The odds of _this_ particular code patch being problematic/interesting are low,
but I really don't want to set a precedent that it's ok to gate code on !HYPERVISOR
without a strong, documented technical reason for doing so.

> but I would like to save some of my sanity when looking at a bug report which
> says that the reboot reason reporting is talking crap, only to find out that
> a HV underneath is doing something silly, muddying up the waters.

That cuts both ways though, e.g. I don't want to debug issues due to Linux not
behaving as expected because someone incorrectly assumed a feature would never
be exposed to a virtual machine.

IMO, if you're regularly spending more than a few seconds sussing out the
environment from a bug report, then we need to improve reporting processes and/or
infrastructure.

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Borislav Petkov]

On Fri, Aug 15, 2025 at 02:39:50PM -0700, Sean Christopherson wrote:
> You're conflating running under (on?) a hypervisor with being a guest in the
> traditional model of virtualization.  Running a privileged, host-owned software
> stack in a virtual machine is beneficial/desirable for a variety of use cases,
> and I expect the number of such use cases to grow in the near future.
> 
> E.g. as mentioned earlier, pKVM uses virtualization to de-privilege the kernel,
> and Hyper-V's VBS uses virtualization to monitor accesses to sensitive state.
> 
> Future use cases could use virtualization:
> 
>  - to improve system stability/uptime, e.g. restart the VM if the kernel crashes
>    as opposed to rebooting the entire host
>  - to isolate different software components, e.g. to run post-boot functionality
>    in a separate VM to limit the impact of an escape/flaw/bug
>  - to limit the amount of code that has direct access to system physical resources
>  - to partion a system into multiple machines (each running their own kernel)
>    for performance reason; e.g. AIUI, kernel-wide locks are becomming more and
>    more problematic as the number of cores continues to go up
> 
> Not all of those use cases will want to access "bare metal" features, but my
> point is that inferring anything about the system setup based solely on the
> HYPERVISOR flag is all but guaranteed to break sooner or later.

No, that's *exactly* why I will want to check HYPERVISOR. Because I don't know
what any of that virt zoo of paravisors, smaller layers, this and that layers
are hiding from me.

That's exactly why I cannot trust that register anymore.

Or do you have a better suggestion how am I to know in the kernel whether the
values I read from that register actually come from the hardware the feature
support was added for? Or not some funky layer in-between.

Hell, I can't even trust HYPERVISOR. Because some sneaky layer might not even
set it.

Which makes this even worse.

> That seems like a gigantic waste of time/effort.  E.g. imagine the pain/churn if
> MONITOR/MWAIT had been limited to !HYPERVISOR simply because early VM setups
> didn't support executing MONITOR/MWAIT natively.

No, that's called enablement. You test the kernel on your new hw or HV env and
then you send patches and claim support. And we support it.

> Patch loading is exceptional because, AFAIK, neither Intel nor AMD officially
> support ucode updates while running as a guest.  Though FWIW, I do expect that
> sooner or later someone will indeed want to do patch loads in a VM.

Oh, ofc. If you give the non-rebooting clouders a finger, they'll bite off the
whole hand if they can.

> I don't mind HYPERVISOR per se, what I don't like is disabling code (or going
> down different paths) based solely on historical use cases, i.e. based on the
> assumption that "no one will ever do xyz!".  I have no objection to pivoting
> on HYPERVISOR if there is an explicitly documented limitation, or for cases where
> there's a very high probability of making a bad decision, e.g. using FMS to
> enumerate features.

I don't think you're reading me here: it is not "no one will ever do". It is
"off until someone really says she will do it. And do it according to the hw
spec."

Because if she does it differently, then also crap.

So until she does it right, it is off.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Sean Christopherson]

On Sat, Aug 16, 2025, Borislav Petkov wrote:
> On Fri, Aug 15, 2025 at 02:39:50PM -0700, Sean Christopherson wrote:
> > Not all of those use cases will want to access "bare metal" features, but my
> > point is that inferring anything about the system setup based solely on the
> > HYPERVISOR flag is all but guaranteed to break sooner or later.
> 
> No, that's *exactly* why I will want to check HYPERVISOR. Because I don't know
> what any of that virt zoo of paravisors, smaller layers, this and that layers
> are hiding from me.
> 
> That's exactly why I cannot trust that register anymore.
> 
> Or do you have a better suggestion how am I to know in the kernel whether the
> values I read from that register actually come from the hardware the feature
> support was added for? Or not some funky layer in-between.

I'm saying the kernel shouldn't care.  This patch fixed a bug/flaw where the
kernel didn't check for valid data.  Here's another in-progress bug where the
kernel isn't sufficiently paranoid/hardened and runs afoul of a VMM that is
operating within spec, but differently from bare metal systems:

https://lore.kernel.org/all/ZjEfVNwRnE1GUd1T@google.com

In both cases, the kernel can avoid badness without needing to know/care where
an unexpected value/result came from.

For things like zen2_zenbleed_check() where the kernel needs to check a ucode
revision and toggle a uarch specific chicken bit, yeah, go ahead and query
HYPERVISOR because there's no sane alternative.  But I want HYPERVISOR to be
viewed as an absolute last resort, not as a way to paper over kernel flaws.

> Hell, I can't even trust HYPERVISOR. Because some sneaky layer might not even
> set it.
> 
> Which makes this even worse.

That's pretty much my point: HYPERVISOR says that there is a hypervisor underneath,
and that's about it.

> > That seems like a gigantic waste of time/effort.  E.g. imagine the pain/churn if
> > MONITOR/MWAIT had been limited to !HYPERVISOR simply because early VM setups
> > didn't support executing MONITOR/MWAIT natively.
> 
> No, that's called enablement. You test the kernel on your new hw or HV env and
> then you send patches and claim support. And we support it.

That simply can't scale.  If you really want to make everything off-by-default
for VMs, then the kernel would need hundreds of HYPERVISOR checks.

How would you draw the line, i.e. how would you choose which features get to be
automatically used when running as a guest and which features need explicit
enablement from someone?  I think the only reasonable answer is that features
with any kind of detection mechanism should just use that and not check HYPERVISOR,
because checking HYPERVISOR would add an extra layer of "enablement" with no
meaningful benefit.

Which is what I'm suggesting here.  If an MMIO load reads back -1u, then it's a
darn good signal that FCH_PM_S5_RESET_STATUS is either unsupported or malfunctioning.
I don't see any reason to drag HYPERVISOR into the mess.

> > I don't mind HYPERVISOR per se, what I don't like is disabling code (or going
> > down different paths) based solely on historical use cases, i.e. based on the
> > assumption that "no one will ever do xyz!".  I have no objection to pivoting
> > on HYPERVISOR if there is an explicitly documented limitation, or for cases where
> > there's a very high probability of making a bad decision, e.g. using FMS to
> > enumerate features.
> 
> I don't think you're reading me here: it is not "no one will ever do". It is
> "off until someone really says she will do it. And do it according to the hw spec."

I am 100% in favor in holding VMMs to hardware specs, but IMO this isn't a case
where the VMM is operating out of spec.

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Borislav Petkov]

On Fri, Aug 15, 2025 at 03:49:00PM -0700, Sean Christopherson wrote:
> Which is what I'm suggesting here.  If an MMIO load reads back -1u, then
> it's a darn good signal that FCH_PM_S5_RESET_STATUS is either unsupported or
> malfunctioning.  I don't see any reason to drag HYPERVISOR into the mess.

Ok, let's play through how you suggest it:

We do not check HYPERVISOR.

The machine boots on *some* guest and it says:

"x86/amd: Previous system reset reason [0x1]: "power button was pressed for 4 seconds".

If this were:

* a normal KVM guest: the machine is straight lying here. There's no power
button.

* "to improve system stability/uptime, e.g. restart the VM if the kernel
crashes as opposed to rebooting the entire host" - this is basically telling
the guest owner that the *host* got rebooted. I'm not sure you want to tell
that to guest owners if you're a cloud vendor.

* If this guest type is lying: same issue as above.

So, what guarantees that hypervisors will adhere to the spec and DTRT here?

Hell, hypervisors don't even care about that probably because they don't know
yet that this thing exists or if they do, they *definitely* want to return an
error here and not disclose to guest owners why they got rebooted.

So the only thing that is workable here is if all hypervisors either return an
error value we do handle or they implement this properly.

And because I don't trust hypervisors to do this right because, as I said,
there needs to be a concentrated effort to support it - otherwise no one
cares, the *least* we can do here is:

                if (s5_reset_reason_txt[i]) {
                        pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s%s\n",
                                value, s5_reset_reason_txt[i], 
                                (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ? " - unreliable: running on a HV" : ""));
                }

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Sean Christopherson]

On Sat, Aug 16, 2025, Borislav Petkov wrote:
> On Fri, Aug 15, 2025 at 03:49:00PM -0700, Sean Christopherson wrote:
> > Which is what I'm suggesting here.  If an MMIO load reads back -1u, then
> > it's a darn good signal that FCH_PM_S5_RESET_STATUS is either unsupported or
> > malfunctioning.  I don't see any reason to drag HYPERVISOR into the mess.
> 
> Ok, let's play through how you suggest it:
> 
> We do not check HYPERVISOR.
> 
> The machine boots on *some* guest and it says:
> 
> "x86/amd: Previous system reset reason [0x1]: "power button was pressed for 4 seconds".
> 
> If this were:
> 
> * a normal KVM guest: the machine is straight lying here. There's no power
> button.
> 
> * "to improve system stability/uptime, e.g. restart the VM if the kernel
> crashes as opposed to rebooting the entire host" - this is basically telling
> the guest owner that the *host* got rebooted. I'm not sure you want to tell
> that to guest owners if you're a cloud vendor.

Most definitely not if the guest owner and host owner are not one and the same.
The example use case is where the platform owner is running one of _their_ kernels
in a VM, in which case that kernel probably does want to know why the platform
reboot.

> * If this guest type is lying: same issue as above.
> 
> So, what guarantees that hypervisors will adhere to the spec and DTRT here?

The same thing that guarantees hardware vendors adhere to specs: the desire to
get paid.

> Hell, hypervisors don't even care about that probably because they don't know
> yet that this thing exists or if they do, they *definitely* want to return an
> error here and not disclose to guest owners why they got rebooted.
> 
> So the only thing that is workable here is if all hypervisors either return an
> error value we do handle or they implement this properly.

And QEMU did return an error value, 0xffffffff, a.k.a. PCI Master Abort / PCIe
Unsupported Request.  I would be amazed if any real world, general purpose VMM
did anything else for an MMIO access to an unknown/unsupported range.

> And because I don't trust hypervisors to do this right because, as I said,
> there needs to be a concentrated effort to support it - otherwise no one
> cares, the *least* we can do here is:
> 
>                 if (s5_reset_reason_txt[i]) {
>                         pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s%s\n",
>                                 value, s5_reset_reason_txt[i], 
>                                 (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ? " - unreliable: running on a HV" : ""));
>                 }

Huh?  Handle a read of all 0xffs as proposed in this patch, and this is unnecessary.

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Borislav Petkov]

On Mon, Aug 18, 2025 at 07:24:26AM -0700, Sean Christopherson wrote:
> Most definitely not if the guest owner and host owner are not one and the same.
> The example use case is where the platform owner is running one of _their_ kernels
> in a VM, in which case that kernel probably does want to know why the platform
> reboot.

Except that can you control who uses that feature? If it ends up being used by
a VM stack where the guest owner should not know the reboot reason, you've
lost.

> The same thing that guarantees hardware vendors adhere to specs: the desire to
> get paid.

So you're basically saying all HV vendors return -1 for an unimplemented
register and we should be fine there?

> And QEMU did return an error value, 0xffffffff, a.k.a. PCI Master Abort / PCIe
> Unsupported Request.  I would be amazed if any real world, general purpose VMM
> did anything else for an MMIO access to an unknown/unsupported range.

Ok, I guess we will know soon enough. :-)

> Huh?  Handle a read of all 0xffs as proposed in this patch, and this is unnecessary.

I don't trust that all HVs will DTRT. But ok, I'll take your word for it. We
can run with this and we'll know soon enough whether fishing out -1 is good
enough. And then we'll have more fun.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/CPU/AMD: Ignore invalid reset reason value

[Sean Christopherson]

On Mon, Aug 18, 2025, Borislav Petkov wrote:
> On Mon, Aug 18, 2025 at 07:24:26AM -0700, Sean Christopherson wrote:
> > Most definitely not if the guest owner and host owner are not one and the same.
> > The example use case is where the platform owner is running one of _their_ kernels
> > in a VM, in which case that kernel probably does want to know why the platform
> > reboot.
> 
> Except that can you control who uses that feature? If it ends up being used by
> a VM stack where the guest owner should not know the reboot reason, you've
> lost.

Yeah, but "expose/advertise XYZ to the wrong VM and you've lost" holds true for
so many things.  In all honesty, of the many ways a hypervisor/CSP can screw up,
this one doesn't scare me at all.

> > The same thing that guarantees hardware vendors adhere to specs: the desire to
> > get paid.
> 
> So you're basically saying all HV vendors return -1 for an unimplemented
> register and we should be fine there?

For this type of register, yes, they should.

> > And QEMU did return an error value, 0xffffffff, a.k.a. PCI Master Abort / PCIe
> > Unsupported Request.  I would be amazed if any real world, general purpose VMM
> > did anything else for an MMIO access to an unknown/unsupported range.
> 
> Ok, I guess we will know soon enough. :-)
> 
> > Huh?  Handle a read of all 0xffs as proposed in this patch, and this is unnecessary.
> 
> I don't trust that all HVs will DTRT. But ok, I'll take your word for it.

Heh, I don't I trust hypervisors/VMMs either, but if they don't behave, then we
yell at them and/or send patches.

[tip: x86/urgent] x86/CPU/AMD: Ignore invalid reset reason value

[tip-bot2 for Yazen Ghannam]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     e9576e078220c50ace9e9087355423de23e25fa5
Gitweb:        https://git.kernel.org/tip/e9576e078220c50ace9e9087355423de23e25fa5
Author:        Yazen Ghannam <yazen.ghannam@amd.com>
AuthorDate:    Mon, 21 Jul 2025 18:11:54 
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 18 Aug 2025 16:36:59 +02:00

x86/CPU/AMD: Ignore invalid reset reason value

The reset reason value may be "all bits set", e.g. 0xFFFFFFFF. This is a
commonly used error response from hardware. This may occur due to a real
hardware issue or when running in a VM.

The user will see all reset reasons reported in this case.

Check for an error response value and return early to avoid decoding
invalid data.

Also, adjust the data variable type to match the hardware register size.

Fixes: ab8131028710 ("x86/CPU/AMD: Print the reason for the last reset")
Reported-by: Libing He <libhe@redhat.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/20250721181155.3536023-1-yazen.ghannam@amd.com
---
 arch/x86/kernel/cpu/amd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index a5ece6e..a6f88ca 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1326,8 +1326,8 @@ static const char * const s5_reset_reason_txt[] = {
 
 static __init int print_s5_reset_status_mmio(void)
 {
-	unsigned long value;
 	void __iomem *addr;
+	u32 value;
 	int i;
 
 	if (!cpu_feature_enabled(X86_FEATURE_ZEN))
@@ -1340,12 +1340,16 @@ static __init int print_s5_reset_status_mmio(void)
 	value = ioread32(addr);
 	iounmap(addr);
 
+	/* Value with "all bits set" is an error response and should be ignored. */
+	if (value == U32_MAX)
+		return 0;
+
 	for (i = 0; i < ARRAY_SIZE(s5_reset_reason_txt); i++) {
 		if (!(value & BIT(i)))
 			continue;
 
 		if (s5_reset_reason_txt[i]) {
-			pr_info("x86/amd: Previous system reset reason [0x%08lx]: %s\n",
+			pr_info("x86/amd: Previous system reset reason [0x%08x]: %s\n",
 				value, s5_reset_reason_txt[i]);
 		}
 	}