[PATCH] x86/nmi: Fix mis-classification of watchdog NMIs

[PATCH] x86/nmi: Fix mis-classification of watchdog NMIs

[Andrew Cooper]

It used to be the case that cpu_data[] inherited the BSP's cpuid_level until
the AP had calculated it itself.  Following the rework, cpuid_level has a
placeholder 1 until it is caluclated propely.

setup_apic_nmi_watchdog() happens to be called on the BSP after SMP bringup,
meaning that the first call is on CPU1.  It is also positioned in the window
where cpu_data[] is garbage.

As a result, setup_p6_watchdog()'s one-time calculation of the performance
counter width falls back into Pentium compatibility mode assuming 32bit
counters.  This causes a watchdog NMI which is delayed a little (e.g. from an
SMI), to appear as if it hadn't overflowed, and therefore be (mis)classifed as
not a watchdog NMI.  On systems where unknown NMIs are treated as fatal, this
results in a spurious crash.

Switch setup_p6_watchdog() to use boot_cpu_data.cpuid_level, which is how this
is checked almost everywhere else.

core2_vpmu_init() used the same pattern to look at leaf 0xa.  Despite being
init code and only running on the BSP, {boot,current}_cpu_data are different
objects, so switch it over to checking boot_cpu_data.cpuid_level too.

Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger@xenproject.org>
CC: Teddy Astie <teddy.astie@vates.tech>

Found on a system where:

   [root@box ~]# time xen-ucode ./blob

   real    0m9.166s
   user    0m0.001s
   sys     0m9.165s

is changing several expectations, and spurious crashes from mis-classified
watchdog NMIs is just one part of the problem.

I hate this fix, but it's the only thing which I consider remotely safe to
backport.  Recent attempts to alter CPUID ordering have 0 success at being
bug-free.

I have not investigated what else was broken by the cpu_data[] change, owing
to a lack of time on my part.  I would be amazed if this is the only thing.
---
 xen/arch/x86/cpu/vpmu_intel.c | 2 +-
 xen/arch/x86/nmi.c            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index ed9f62b9366d..af6cb0a85cdd 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -896,7 +896,7 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void)
     unsigned int version = 0;
     unsigned int i;
 
-    if ( current_cpu_data.cpuid_level >= 0xa )
+    if ( bsp_cpu_data.cpuid_level >= 0xa )
         version = MASK_EXTR(cpuid_eax(0xa), PMU_VERSION_MASK);
 
     switch ( version )
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 91f95fe6d080..ec85516609b0 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -321,7 +321,7 @@ static void setup_p6_watchdog(unsigned counter)
 {
     unsigned int evntsel;
 
-    if ( !nmi_p6_event_width && current_cpu_data.cpuid_level >= 0xa )
+    if ( !nmi_p6_event_width && boot_cpu_data.cpuid_level >= 0xa )
         nmi_p6_event_width = MASK_EXTR(cpuid_eax(0xa), P6_EVENT_WIDTH_MASK);
     if ( !nmi_p6_event_width )
         nmi_p6_event_width = P6_EVENT_WIDTH_MIN;
-- 
2.39.5

Re: [PATCH] x86/nmi: Fix mis-classification of watchdog NMIs

[Andrew Cooper]

On 13/08/2026 6:43 pm, Andrew Cooper wrote:
> It used to be the case that cpu_data[] inherited the BSP's cpuid_level until
> the AP had calculated it itself.  Following the rework, cpuid_level has a
> placeholder 1 until it is caluclated propely.
>
> setup_apic_nmi_watchdog() happens to be called on the BSP after SMP bringup,
> meaning that the first call is on CPU1.  It is also positioned in the window
> where cpu_data[] is garbage.
>
> As a result, setup_p6_watchdog()'s one-time calculation of the performance
> counter width falls back into Pentium compatibility mode assuming 32bit
> counters.  This causes a watchdog NMI which is delayed a little (e.g. from an
> SMI), to appear as if it hadn't overflowed, and therefore be (mis)classifed as
> not a watchdog NMI.  On systems where unknown NMIs are treated as fatal, this
> results in a spurious crash.
>
> Switch setup_p6_watchdog() to use boot_cpu_data.cpuid_level, which is how this
> is checked almost everywhere else.
>
> core2_vpmu_init() used the same pattern to look at leaf 0xa.  Despite being
> init code and only running on the BSP, {boot,current}_cpu_data are different
> objects, so switch it over to checking boot_cpu_data.cpuid_level too.
>
> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Roger Pau Monné <roger@xenproject.org>
> CC: Teddy Astie <teddy.astie@vates.tech>
>
> Found on a system where:
>
>    [root@box ~]# time xen-ucode ./blob
>
>    real    0m9.166s
>    user    0m0.001s
>    sys     0m9.165s
>
> is changing several expectations, and spurious crashes from mis-classified
> watchdog NMIs is just one part of the problem.
>
> I hate this fix, but it's the only thing which I consider remotely safe to
> backport.  Recent attempts to alter CPUID ordering have 0 success at being
> bug-free.
>
> I have not investigated what else was broken by the cpu_data[] change, owing
> to a lack of time on my part.  I would be amazed if this is the only thing.
> ---
>  xen/arch/x86/cpu/vpmu_intel.c | 2 +-
>  xen/arch/x86/nmi.c            | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index ed9f62b9366d..af6cb0a85cdd 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -896,7 +896,7 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void)
>      unsigned int version = 0;
>      unsigned int i;
>  
> -    if ( current_cpu_data.cpuid_level >= 0xa )
> +    if ( bsp_cpu_data.cpuid_level >= 0xa )
>          version = MASK_EXTR(cpuid_eax(0xa), PMU_VERSION_MASK);

Hmm, this is a stale copy of the patch.  Fixed locally.

~Andrew

>  
>      switch ( version )
> diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
> index 91f95fe6d080..ec85516609b0 100644
> --- a/xen/arch/x86/nmi.c
> +++ b/xen/arch/x86/nmi.c
> @@ -321,7 +321,7 @@ static void setup_p6_watchdog(unsigned counter)
>  {
>      unsigned int evntsel;
>  
> -    if ( !nmi_p6_event_width && current_cpu_data.cpuid_level >= 0xa )
> +    if ( !nmi_p6_event_width && boot_cpu_data.cpuid_level >= 0xa )
>          nmi_p6_event_width = MASK_EXTR(cpuid_eax(0xa), P6_EVENT_WIDTH_MASK);
>      if ( !nmi_p6_event_width )
>          nmi_p6_event_width = P6_EVENT_WIDTH_MIN;

Re: [PATCH] x86/nmi: Fix mis-classification of watchdog NMIs

[Jan Beulich]

On 13.08.2026 19:43, Andrew Cooper wrote:
> It used to be the case that cpu_data[] inherited the BSP's cpuid_level until
> the AP had calculated it itself.  Following the rework, cpuid_level has a
> placeholder 1 until it is caluclated propely.
> 
> setup_apic_nmi_watchdog() happens to be called on the BSP after SMP bringup,
> meaning that the first call is on CPU1.  It is also positioned in the window
> where cpu_data[] is garbage.
> 
> As a result, setup_p6_watchdog()'s one-time calculation of the performance
> counter width falls back into Pentium compatibility mode assuming 32bit
> counters.  This causes a watchdog NMI which is delayed a little (e.g. from an
> SMI), to appear as if it hadn't overflowed, and therefore be (mis)classifed as
> not a watchdog NMI.  On systems where unknown NMIs are treated as fatal, this
> results in a spurious crash.
> 
> Switch setup_p6_watchdog() to use boot_cpu_data.cpuid_level, which is how this
> is checked almost everywhere else.
> 
> core2_vpmu_init() used the same pattern to look at leaf 0xa.  Despite being
> init code and only running on the BSP, {boot,current}_cpu_data are different
> objects, so switch it over to checking boot_cpu_data.cpuid_level too.
> 
> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> I hate this fix, but it's the only thing which I consider remotely safe to
> backport.  Recent attempts to alter CPUID ordering have 0 success at being
> bug-free.

How that? Collecting CPUID output should be doable almost first thing. There
are no (or in case of doubt: there should not be any) dependencies on about
anything else. Of course re-collecting may still be necessary after ucode
loading. Yet from what you say I must be missing something crucial.

With that in mind, I'm also questioning the Fixes: tag (without this being a
request to drop or change it): Using another CPU's data isn't much better
than using partially unset data. Unless we assumed full symmetry, at which
point re-obtaining of most data on the APs would be entirely useless. Hence
the issue was pre-existing, with one bug there hiding the issue addressed
here.

Jan

Re: [PATCH] x86/nmi: Fix mis-classification of watchdog NMIs

[Andrew Cooper]

On 17/08/2026 9:30 am, Jan Beulich wrote:
> On 13.08.2026 19:43, Andrew Cooper wrote:
>> It used to be the case that cpu_data[] inherited the BSP's cpuid_level until
>> the AP had calculated it itself.  Following the rework, cpuid_level has a
>> placeholder 1 until it is caluclated propely.
>>
>> setup_apic_nmi_watchdog() happens to be called on the BSP after SMP bringup,
>> meaning that the first call is on CPU1.  It is also positioned in the window
>> where cpu_data[] is garbage.
>>
>> As a result, setup_p6_watchdog()'s one-time calculation of the performance
>> counter width falls back into Pentium compatibility mode assuming 32bit
>> counters.  This causes a watchdog NMI which is delayed a little (e.g. from an
>> SMI), to appear as if it hadn't overflowed, and therefore be (mis)classifed as
>> not a watchdog NMI.  On systems where unknown NMIs are treated as fatal, this
>> results in a spurious crash.
>>
>> Switch setup_p6_watchdog() to use boot_cpu_data.cpuid_level, which is how this
>> is checked almost everywhere else.
>>
>> core2_vpmu_init() used the same pattern to look at leaf 0xa.  Despite being
>> init code and only running on the BSP, {boot,current}_cpu_data are different
>> objects, so switch it over to checking boot_cpu_data.cpuid_level too.
>>
>> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> I hate this fix, but it's the only thing which I consider remotely safe to
>> backport.  Recent attempts to alter CPUID ordering have 0 success at being
>> bug-free.
> How that? Collecting CPUID output should be doable almost first thing. There
> are no (or in case of doubt: there should not be any) dependencies on about
> anything else. Of course re-collecting may still be necessary after ucode
> loading. Yet from what you say I must be missing something crucial.

I was referring to commits in Xen rearranging the boot sequence with
respect to feature handling.  There have been many failures recently.

> With that in mind, I'm also questioning the Fixes: tag (without this being a
> request to drop or change it): Using another CPU's data isn't much better
> than using partially unset data. Unless we assumed full symmetry, at which
> point re-obtaining of most data on the APs would be entirely useless. Hence
> the issue was pre-existing, with one bug there hiding the issue addressed
> here.

Copying the BSP is less bad than the current behaviour.  It's not
necessarily ideal, but it's a damsight better default than the arbitrary
1 that this patch puts in place.

Even with the very old 64bit systems where mixing steppings was
commonplace, I'm not aware of a vendor supported combination where
max_leaf was different.


The issue was not prexisting.  Prior to your rearrangement, the NMI
watchdog setup found the counter width in CPUID and used it, without
falling back into original Pentium compatibility mode.

~Andrew

Re: [PATCH] x86/nmi: Fix mis-classification of watchdog NMIs

[Jan Beulich]

On 17.08.2026 17:55, Andrew Cooper wrote:
> On 17/08/2026 9:30 am, Jan Beulich wrote:
>> On 13.08.2026 19:43, Andrew Cooper wrote:
>>> It used to be the case that cpu_data[] inherited the BSP's cpuid_level until
>>> the AP had calculated it itself.  Following the rework, cpuid_level has a
>>> placeholder 1 until it is caluclated propely.
>>>
>>> setup_apic_nmi_watchdog() happens to be called on the BSP after SMP bringup,
>>> meaning that the first call is on CPU1.  It is also positioned in the window
>>> where cpu_data[] is garbage.
>>>
>>> As a result, setup_p6_watchdog()'s one-time calculation of the performance
>>> counter width falls back into Pentium compatibility mode assuming 32bit
>>> counters.  This causes a watchdog NMI which is delayed a little (e.g. from an
>>> SMI), to appear as if it hadn't overflowed, and therefore be (mis)classifed as
>>> not a watchdog NMI.  On systems where unknown NMIs are treated as fatal, this
>>> results in a spurious crash.
>>>
>>> Switch setup_p6_watchdog() to use boot_cpu_data.cpuid_level, which is how this
>>> is checked almost everywhere else.
>>>
>>> core2_vpmu_init() used the same pattern to look at leaf 0xa.  Despite being
>>> init code and only running on the BSP, {boot,current}_cpu_data are different
>>> objects, so switch it over to checking boot_cpu_data.cpuid_level too.
>>>
>>> Fixes: 7126b7f806d5 ("x86/CPU: re-work populating of cpu_data[]")
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.
> 
>>
>>> I hate this fix, but it's the only thing which I consider remotely safe to
>>> backport.  Recent attempts to alter CPUID ordering have 0 success at being
>>> bug-free.
>> How that? Collecting CPUID output should be doable almost first thing. There
>> are no (or in case of doubt: there should not be any) dependencies on about
>> anything else. Of course re-collecting may still be necessary after ucode
>> loading. Yet from what you say I must be missing something crucial.
> 
> I was referring to commits in Xen rearranging the boot sequence with
> respect to feature handling.  There have been many failures recently.
> 
>> With that in mind, I'm also questioning the Fixes: tag (without this being a
>> request to drop or change it): Using another CPU's data isn't much better
>> than using partially unset data. Unless we assumed full symmetry, at which
>> point re-obtaining of most data on the APs would be entirely useless. Hence
>> the issue was pre-existing, with one bug there hiding the issue addressed
>> here.
> 
> Copying the BSP is less bad than the current behaviour.  It's not
> necessarily ideal, but it's a damsight better default than the arbitrary
> 1 that this patch puts in place.
> 
> Even with the very old 64bit systems where mixing steppings was
> commonplace, I'm not aware of a vendor supported combination where
> max_leaf was different.
> 
> 
> The issue was not prexisting.  Prior to your rearrangement, the NMI
> watchdog setup found the counter width in CPUID and used it, without
> falling back into original Pentium compatibility mode.

FTAOD - with "pre-existing" I meant the using of the (possibly) wrong data,
not the particular issue of the NMI watchdog being affected.

Jan