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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> CC: Teddy Astie <teddy.astie@xxxxxxxxxx>
>
> 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;