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

Andrew Cooper <[email protected]>
Newsgroups org.xenproject.lists.xen-devel
Message-ID <[email protected]>
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 <[email protected]>
---
CC: Jan Beulich <[email protected]>
CC: Roger Pau Monné <[email protected]>
CC: Teddy Astie <[email protected]>

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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.