[PATCH 0/5] x86/nmi: Watchdog fixes/improvement Part 1

[PATCH 0/5] x86/nmi: Watchdog fixes/improvement Part 1

[Andrew Cooper]

This is the start of a very long rabbit hole to address the
mis-classification of some watchdog NMIs as non-watchdog NMIs.  For
now, just some simple and hopefully non-controvertial changes.

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2733861049

Andrew Cooper (5):
  x86/nmi: Drop {reserve,release}_lapic_nmi()
  x86/nmi: Drop K7_NMI_EVENT
  x86/nmi: Misc style fixes
  x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms
  x86/nmi: Don't configure EvtSel repeatedly

 xen/arch/x86/include/asm/apic.h |   2 -
 xen/arch/x86/nmi.c              | 153 ++++++++++----------------------
 2 files changed, 47 insertions(+), 108 deletions(-)

-- 
2.34.1

[PATCH 1/5] x86/nmi: Drop {reserve,release}_lapic_nmi()

[Andrew Cooper]

With Oprofile support dropped, there are no more users of these.  Drop them.

No functional change.

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>
---
 xen/arch/x86/include/asm/apic.h |  2 --
 xen/arch/x86/nmi.c              | 50 ---------------------------------
 2 files changed, 52 deletions(-)

diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index 918f1cee3567..f30d57ad22c5 100644
--- a/xen/arch/x86/include/asm/apic.h
+++ b/xen/arch/x86/include/asm/apic.h
@@ -174,8 +174,6 @@ extern void setup_boot_APIC_clock (void);
 extern void setup_secondary_APIC_clock (void);
 extern void setup_apic_nmi_watchdog (void);
 extern void disable_lapic_nmi_watchdog(void);
-extern int reserve_lapic_nmi(void);
-extern void release_lapic_nmi(void);
 extern void self_nmi(void);
 extern void disable_timer_nmi_watchdog(void);
 extern void enable_timer_nmi_watchdog(void);
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 91f95fe6d080..616bfdbc9029 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -89,20 +89,6 @@ static int __init cf_check parse_watchdog_timeout(const char *s)
 }
 custom_param("watchdog_timeout", parse_watchdog_timeout);
 
-/*
- * lapic_nmi_owner tracks the ownership of the lapic NMI hardware:
- * - it may be reserved by some other driver, or not
- * - when not reserved by some other driver, it may be used for
- *   the NMI watchdog, or not
- *
- * This is maintained separately from nmi_active because the NMI
- * watchdog may also be driven from the I/O APIC timer.
- */
-static DEFINE_SPINLOCK(lapic_nmi_owner_lock);
-static unsigned int lapic_nmi_owner;
-#define LAPIC_NMI_WATCHDOG	(1<<0)
-#define LAPIC_NMI_RESERVED	(1<<1)
-
 /* nmi_active:
  * +1: the lapic NMI watchdog is active, but can be disabled
  *  0: the lapic NMI watchdog has not been set up, and cannot
@@ -239,41 +225,6 @@ void disable_lapic_nmi_watchdog(void)
     nmi_watchdog = NMI_NONE;
 }
 
-static void enable_lapic_nmi_watchdog(void)
-{
-    if (nmi_active < 0) {
-        nmi_watchdog = NMI_LOCAL_APIC;
-        setup_apic_nmi_watchdog();
-    }
-}
-
-int reserve_lapic_nmi(void)
-{
-    unsigned int old_owner;
-
-    spin_lock(&lapic_nmi_owner_lock);
-    old_owner = lapic_nmi_owner;
-    lapic_nmi_owner |= LAPIC_NMI_RESERVED;
-    spin_unlock(&lapic_nmi_owner_lock);
-    if (old_owner & LAPIC_NMI_RESERVED)
-        return -EBUSY;
-    if (old_owner & LAPIC_NMI_WATCHDOG)
-        disable_lapic_nmi_watchdog();
-    return 0;
-}
-
-void release_lapic_nmi(void)
-{
-    unsigned int new_owner;
-
-    spin_lock(&lapic_nmi_owner_lock);
-    new_owner = lapic_nmi_owner & ~LAPIC_NMI_RESERVED;
-    lapic_nmi_owner = new_owner;
-    spin_unlock(&lapic_nmi_owner_lock);
-    if (new_owner & LAPIC_NMI_WATCHDOG)
-        enable_lapic_nmi_watchdog();
-}
-
 /*
  * Activate the NMI watchdog via the local APIC.
  * Original code written by Keith Owens.
@@ -417,7 +368,6 @@ void setup_apic_nmi_watchdog(void)
         return;
     }
 
-    lapic_nmi_owner = LAPIC_NMI_WATCHDOG;
     nmi_active = 1;
 }
 
-- 
2.34.1

[PATCH 2/5] x86/nmi: Drop K7_NMI_EVENT

[Andrew Cooper]

This name is misleading.

It's not possible to configure NMI or not from the event select register; that
comes from the APIC configuration for performance events.

This name is "the thing we want to count for the NMI watchdog", but that's
clearer to follow when it simply names the event.  Drop the indirection.

No functional change.

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>
---
 xen/arch/x86/nmi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 616bfdbc9029..113e672c4f15 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -102,7 +102,6 @@ static int nmi_active;
 #define K7_EVNTSEL_OS		(1 << 17)
 #define K7_EVNTSEL_USR		(1 << 16)
 #define K7_EVENT_CYCLES_PROCESSOR_IS_RUNNING	0x76
-#define K7_NMI_EVENT		K7_EVENT_CYCLES_PROCESSOR_IS_RUNNING
 #define K7_EVENT_WIDTH          32
 
 #define P6_EVNTSEL0_ENABLE	(1 << 22)
@@ -259,7 +258,7 @@ static void setup_k7_watchdog(void)
     evntsel = K7_EVNTSEL_INT
         | K7_EVNTSEL_OS
         | K7_EVNTSEL_USR
-        | K7_NMI_EVENT;
+        | K7_EVENT_CYCLES_PROCESSOR_IS_RUNNING;
 
     wrmsrns(MSR_K7_EVNTSEL0, evntsel);
     write_watchdog_counter("K7_PERFCTR0");
-- 
2.34.1

[PATCH 3/5] x86/nmi: Misc style fixes

[Andrew Cooper]

 * Drop trailing whitespace
 * Sort includes, dropping asm/mc146818rtc.h and asm/div64.h as unused
 * Brace position, and types

No functional change.

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>
---
 xen/arch/x86/nmi.c | 54 ++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index 113e672c4f15..d9d07870a333 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -13,26 +13,25 @@
  *  Mikael Pettersson : PM converted to driver model. Disable/enable API.
  */
 
+#include <xen/console.h>
+#include <xen/cpu.h>
+#include <xen/delay.h>
 #include <xen/init.h>
+#include <xen/irq.h>
+#include <xen/keyhandler.h>
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/param.h>
-#include <xen/irq.h>
-#include <xen/delay.h>
-#include <xen/time.h>
 #include <xen/sched.h>
-#include <xen/console.h>
 #include <xen/smp.h>
-#include <xen/keyhandler.h>
+#include <xen/time.h>
 #include <xen/watchdog.h>
-#include <xen/cpu.h>
+
+#include <asm/apic.h>
 #include <asm/current.h>
-#include <asm/mc146818rtc.h>
-#include <asm/msr.h>
 #include <asm/mpspec.h>
+#include <asm/msr.h>
 #include <asm/nmi.h>
-#include <asm/div64.h>
-#include <asm/apic.h>
 
 unsigned int nmi_watchdog = NMI_NONE;
 static unsigned int nmi_hz = HZ;
@@ -124,10 +123,10 @@ static int nmi_active;
 #define P4_CCCR_REQUIRED	(3<<16)
 #define P4_CCCR_ESCR_SELECT(N)	((N)<<13)
 #define P4_CCCR_ENABLE		(1<<12)
-/* 
+/*
  * Set up IQ_PERFCTR0 to behave like a clock, by having IQ_CCCR0 filter
  * CRU_ESCR0 (with any non-null event selector) through a complemented
- * max threshold. [IA32-Vol3, Section 14.9.9] 
+ * max threshold. [IA32-Vol3, Section 14.9.9]
  */
 #define P4_NMI_CRU_ESCR0	P4_ESCR_EVENT_SELECT(0x3F)
 #define P4_NMI_IQ_CCCR0	\
@@ -182,7 +181,7 @@ void __init check_nmi_watchdog(void)
      * There's a limit to how slow we can go because writing the perfctr
      * MSRs only sets the low 32 bits, with the top 8 bits sign-extended
      * from those, so it's not possible to set up a delay larger than
-     * 2^31 cycles and smaller than (2^40 - 2^31) cycles. 
+     * 2^31 cycles and smaller than (2^40 - 2^31) cycles.
      * (Intel SDM, section 18.22.2)
      */
     if ( nmi_watchdog == NMI_LOCAL_APIC )
@@ -199,8 +198,9 @@ static void cf_check nmi_timer_fn(void *unused)
 
 void disable_lapic_nmi_watchdog(void)
 {
-    if (nmi_active <= 0)
+    if ( nmi_active <= 0 )
         return;
+
     switch ( boot_cpu_data.vendor )
     {
     case X86_VENDOR_AMD:
@@ -231,9 +231,7 @@ void disable_lapic_nmi_watchdog(void)
 
 static void clear_msr_range(unsigned int base, unsigned int n)
 {
-    unsigned int i;
-
-    for (i = 0; i < n; i++)
+    for ( unsigned int i = 0; i < n; i++ )
         wrmsrns(base + i, 0);
 }
 
@@ -302,7 +300,7 @@ static void setup_p4_watchdog(void)
     uint64_t misc_enable;
 
     rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
-    if (!(misc_enable & MSR_IA32_MISC_ENABLE_PERF_AVAIL))
+    if ( !(misc_enable & MSR_IA32_MISC_ENABLE_PERF_AVAIL) )
         return;
 
     nmi_perfctr_msr = MSR_P4_IQ_PERFCTR0;
@@ -310,15 +308,18 @@ static void setup_p4_watchdog(void)
     if ( boot_cpu_data.x86_num_siblings == 2 )
         nmi_p4_cccr_val |= P4_CCCR_OVF_PMI1;
 
-    if (!(misc_enable & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL))
+    if ( !(misc_enable & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL) )
         clear_msr_range(0x3F1, 2);
     /* MSR 0x3F0 seems to have a default value of 0xFC00, but current
        docs doesn't fully define it, so leave it alone for now. */
-    if (boot_cpu_data.model >= 0x3) {
+    if ( boot_cpu_data.model >= 0x3 )
+    {
         /* MSR_P4_IQ_ESCR0/1 (0x3ba/0x3bb) removed */
         clear_msr_range(0x3A0, 26);
         clear_msr_range(0x3BC, 3);
-    } else {
+    }
+    else
+    {
         clear_msr_range(0x3A0, 31);
     }
     clear_msr_range(0x3C0, 6);
@@ -393,7 +394,7 @@ static int cf_check cpu_nmi_callback(
 }
 
 static struct notifier_block cpu_nmi_nfb = {
-    .notifier_call = cpu_nmi_callback
+    .notifier_call = cpu_nmi_callback,
 };
 
 static DEFINE_PER_CPU(unsigned int, last_irq_sums);
@@ -444,15 +445,15 @@ bool nmi_watchdog_tick(const struct cpu_user_regs *regs)
          * before doing the oops ...
          */
         this_cpu(alert_counter)++;
-        if ( this_cpu(alert_counter) == opt_watchdog_timeout*nmi_hz )
+        if ( this_cpu(alert_counter) == opt_watchdog_timeout * nmi_hz )
         {
             console_force_unlock();
             printk("Watchdog timer detects that CPU%d is stuck!\n",
                    smp_processor_id());
             fatal_trap(regs, 1);
         }
-    } 
-    else 
+    }
+    else
     {
         this_cpu(last_irq_sums) = sum;
         this_cpu(alert_counter) = 0;
@@ -512,7 +513,8 @@ bool nmi_watchdog_tick(const struct cpu_user_regs *regs)
 void self_nmi(void)
 {
     unsigned long flags;
-    u32 id = get_apic_id();
+    uint32_t id = get_apic_id();
+
     local_irq_save(flags);
     apic_wait_icr_idle();
     apic_icr_write(APIC_DM_NMI | APIC_DEST_PHYSICAL, id);
-- 
2.34.1

[PATCH 4/5] x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms

[Andrew Cooper]

Right now it's only checked in setup_p4_watchdog(), and not in
setup_p6_watchdog().

Perform the check in the common Intel path in
setup_apic_nmi_watchdog(), and pass misc_enable as a parameter into
setup_p4_watchdog() to aoid reading it twice.

Fixes: 0dfba864fbff ("NMI watchdog support in Xen.")
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>

I presume this bug went unnoticed because watchdog is off-by-default.
---
 xen/arch/x86/nmi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index d9d07870a333..a8b0d79c7cf2 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -295,14 +295,8 @@ static void setup_p6_watchdog(unsigned counter)
     wrmsrns(MSR_P6_EVNTSEL(0), evntsel);
 }
 
-static void setup_p4_watchdog(void)
+static void setup_p4_watchdog(uint64_t misc_enable)
 {
-    uint64_t misc_enable;
-
-    rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
-    if ( !(misc_enable & MSR_IA32_MISC_ENABLE_PERF_AVAIL) )
-        return;
-
     nmi_perfctr_msr = MSR_P4_IQ_PERFCTR0;
     nmi_p4_cccr_val = P4_NMI_IQ_CCCR0;
     if ( boot_cpu_data.x86_num_siblings == 2 )
@@ -337,6 +331,8 @@ static void setup_p4_watchdog(void)
 
 void setup_apic_nmi_watchdog(void)
 {
+    uint64_t misc;
+
     if ( nmi_watchdog == NMI_NONE )
         return;
 
@@ -347,6 +343,14 @@ void setup_apic_nmi_watchdog(void)
         break;
 
     case X86_VENDOR_INTEL:
+        misc = rdmsr(MSR_IA32_MISC_ENABLE);
+
+        if ( !(misc & MSR_IA32_MISC_ENABLE_PERF_AVAIL) )
+        {
+            printk(XENLOG_WARNING "Intel Perfmon unavailable\n");
+            goto disable;
+        }
+
         switch ( boot_cpu_data.family )
         {
         case 6:
@@ -355,7 +359,7 @@ void setup_apic_nmi_watchdog(void)
                               : CORE_EVENT_CPU_CLOCKS_NOT_HALTED);
             break;
         case 15:
-            setup_p4_watchdog();
+            setup_p4_watchdog(misc);
             break;
         }
         break;
@@ -363,6 +367,7 @@ void setup_apic_nmi_watchdog(void)
 
     if ( nmi_perfctr_msr == 0 )
     {
+    disable:
         printk(XENLOG_WARNING "Failed to configure NMI watchdog\n");
         nmi_watchdog = NMI_NONE;
         return;
-- 
2.34.1

[PATCH 5/5] x86/nmi: Don't configure EvtSel repeatedly

[Andrew Cooper]

In both setup_{k7,p6}_watchdog(), EvtSel0 is first zeroed, then written with
everything but the enable bit, then written with the enable bit.

setup_p4_watchdog() is slightly more complicated, owing to what
appears to be a bug introduced by commit 2a2bd8de16b6 ("Clean up NMI
watchdog handler."), which causes a second bit to be temporarily
different too.

The middle of the three writes is useless in all cases.  Drop it.

While doing this, rename the 'counter' parameter for
setup_p6_watchdog().  It is the event which is passed in; the counter
is always counter 0.

No functional change.

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>
---
 xen/arch/x86/nmi.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index a8b0d79c7cf2..a697486b834d 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -246,29 +246,20 @@ static inline void write_watchdog_counter(const char *descr)
 
 static void setup_k7_watchdog(void)
 {
-    unsigned int evntsel;
-
     nmi_perfctr_msr = MSR_K7_PERFCTR0;
 
     clear_msr_range(MSR_K7_EVNTSEL0, 4);
     clear_msr_range(MSR_K7_PERFCTR0, 4);
 
-    evntsel = K7_EVNTSEL_INT
-        | K7_EVNTSEL_OS
-        | K7_EVNTSEL_USR
-        | K7_EVENT_CYCLES_PROCESSOR_IS_RUNNING;
-
-    wrmsrns(MSR_K7_EVNTSEL0, evntsel);
     write_watchdog_counter("K7_PERFCTR0");
     apic_write(APIC_LVTPC, APIC_DM_NMI);
-    evntsel |= K7_EVNTSEL_ENABLE;
-    wrmsrns(MSR_K7_EVNTSEL0, evntsel);
+    wrmsrns(MSR_K7_EVNTSEL0,
+            K7_EVNTSEL_ENABLE | K7_EVNTSEL_INT | K7_EVNTSEL_OS |
+            K7_EVNTSEL_USR | K7_EVENT_CYCLES_PROCESSOR_IS_RUNNING);
 }
 
-static void setup_p6_watchdog(unsigned counter)
+static void setup_p6_watchdog(unsigned int event)
 {
-    unsigned int evntsel;
-
     if ( !nmi_p6_event_width && current_cpu_data.cpuid_level >= 0xa )
         nmi_p6_event_width = MASK_EXTR(cpuid_eax(0xa), P6_EVENT_WIDTH_MASK);
     if ( !nmi_p6_event_width )
@@ -283,16 +274,11 @@ static void setup_p6_watchdog(unsigned counter)
     clear_msr_range(MSR_P6_EVNTSEL(0), 2);
     clear_msr_range(MSR_P6_PERFCTR(0), 2);
 
-    evntsel = P6_EVNTSEL_INT
-        | P6_EVNTSEL_OS
-        | P6_EVNTSEL_USR
-        | counter;
-
-    wrmsrns(MSR_P6_EVNTSEL(0), evntsel);
     write_watchdog_counter("P6_PERFCTR0");
     apic_write(APIC_LVTPC, APIC_DM_NMI);
-    evntsel |= P6_EVNTSEL0_ENABLE;
-    wrmsrns(MSR_P6_EVNTSEL(0), evntsel);
+    wrmsrns(MSR_P6_EVNTSEL(0),
+            P6_EVNTSEL0_ENABLE | P6_EVNTSEL_INT | P6_EVNTSEL_OS |
+            P6_EVNTSEL_USR | event);
 }
 
 static void setup_p4_watchdog(uint64_t misc_enable)
@@ -323,7 +309,6 @@ static void setup_p4_watchdog(uint64_t misc_enable)
     clear_msr_range(MSR_P4_BPU_PERFCTR0, 18);
 
     wrmsrl(MSR_P4_CRU_ESCR0, P4_NMI_CRU_ESCR0);
-    wrmsrl(MSR_P4_IQ_CCCR0, P4_NMI_IQ_CCCR0 & ~P4_CCCR_ENABLE);
     write_watchdog_counter("P4_IQ_COUNTER0");
     apic_write(APIC_LVTPC, APIC_DM_NMI);
     wrmsrl(MSR_P4_IQ_CCCR0, nmi_p4_cccr_val);
-- 
2.34.1

Re: [PATCH 0/5] x86/nmi: Watchdog fixes/improvement Part 1

[Jan Beulich]

On 05.08.2026 14:45, Andrew Cooper wrote:
> This is the start of a very long rabbit hole to address the
> mis-classification of some watchdog NMIs as non-watchdog NMIs.  For
> now, just some simple and hopefully non-controvertial changes.
> 
> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2733861049
> 
> Andrew Cooper (5):
>   x86/nmi: Drop {reserve,release}_lapic_nmi()
>   x86/nmi: Drop K7_NMI_EVENT
>   x86/nmi: Misc style fixes
>   x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms
>   x86/nmi: Don't configure EvtSel repeatedly
> 
>  xen/arch/x86/include/asm/apic.h |   2 -
>  xen/arch/x86/nmi.c              | 153 ++++++++++----------------------
>  2 files changed, 47 insertions(+), 108 deletions(-)

This series, once again, is putting me in a difficult position: Should I look
at it, or should I let it sit for two years or more, just like my earlier
fixes in this area [1], [2] are? (Of course, as always so far, I will look at
the patches, and I will likely also accept them going in ahead of mine. But I
cannot exclude that at some point I might actually stop doing so, seeing how
many of my patches are in that state. While at the same time none of yours
are, afaict, i.e. as per the track record that I keep of what still needs
responding to.)

Yes, you did respond to [1], but is not being comfortable with a change really
a reason to block it, when it _is_ an improvement, and when the alternative
hasn't materialized in all the time?

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2024-01/msg01365.html
[2] https://lists.xen.org/archives/html/xen-devel/2024-04/msg00194.html

Re: [PATCH 1/5] x86/nmi: Drop {reserve,release}_lapic_nmi()

[Jan Beulich]

On 05.08.2026 14:45, Andrew Cooper wrote:
> With Oprofile support dropped, there are no more users of these.  Drop them.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 2/5] x86/nmi: Drop K7_NMI_EVENT

[Jan Beulich]

On 05.08.2026 14:45, Andrew Cooper wrote:
> This name is misleading.
> 
> It's not possible to configure NMI or not from the event select register; that
> comes from the APIC configuration for performance events.
> 
> This name is "the thing we want to count for the NMI watchdog", but that's
> clearer to follow when it simply names the event.  Drop the indirection.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 3/5] x86/nmi: Misc style fixes

[Jan Beulich]

On 05.08.2026 14:45, Andrew Cooper wrote:
>  * Drop trailing whitespace
>  * Sort includes, dropping asm/mc146818rtc.h and asm/div64.h as unused
>  * Brace position, and types
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
albeit I would have suggested ...

> @@ -310,15 +308,18 @@ static void setup_p4_watchdog(void)
>      if ( boot_cpu_data.x86_num_siblings == 2 )
>          nmi_p4_cccr_val |= P4_CCCR_OVF_PMI1;
>  
> -    if (!(misc_enable & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL))
> +    if ( !(misc_enable & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL) )
>          clear_msr_range(0x3F1, 2);
>      /* MSR 0x3F0 seems to have a default value of 0xFC00, but current
>         docs doesn't fully define it, so leave it alone for now. */
> -    if (boot_cpu_data.model >= 0x3) {
> +    if ( boot_cpu_data.model >= 0x3 )
> +    {
>          /* MSR_P4_IQ_ESCR0/1 (0x3ba/0x3bb) removed */
>          clear_msr_range(0x3A0, 26);
>          clear_msr_range(0x3BC, 3);
> -    } else {
> +    }
> +    else
> +    {
>          clear_msr_range(0x3A0, 31);
>      }

... to instead drop the figure braces here.

Jan

Re: [PATCH 4/5] x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms

[Jan Beulich]

On 05.08.2026 14:45, Andrew Cooper wrote:
> @@ -347,6 +343,14 @@ void setup_apic_nmi_watchdog(void)
>          break;
>  
>      case X86_VENDOR_INTEL:
> +        misc = rdmsr(MSR_IA32_MISC_ENABLE);
> +
> +        if ( !(misc & MSR_IA32_MISC_ENABLE_PERF_AVAIL) )
> +        {
> +            printk(XENLOG_WARNING "Intel Perfmon unavailable\n");
> +            goto disable;
> +        }

Please can we avoid "goto" when that's easily possible? You can use
"break" here instead, and ...

>          switch ( boot_cpu_data.family )
>          {
>          case 6:
> @@ -355,7 +359,7 @@ void setup_apic_nmi_watchdog(void)
>                                : CORE_EVENT_CPU_CLOCKS_NOT_HALTED);
>              break;
>          case 15:
> -            setup_p4_watchdog();
> +            setup_p4_watchdog(misc);
>              break;
>          }
>          break;
> @@ -363,6 +367,7 @@ void setup_apic_nmi_watchdog(void)
>  
>      if ( nmi_perfctr_msr == 0 )
>      {
> +    disable:
>          printk(XENLOG_WARNING "Failed to configure NMI watchdog\n");
>          nmi_watchdog = NMI_NONE;
>          return;

... we'll still end up here, as nmi_perfctr_msr won't be written.
Preferably with that change:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 5/5] x86/nmi: Don't configure EvtSel repeatedly

[Jan Beulich]

On 05.08.2026 14:45, Andrew Cooper wrote:
> In both setup_{k7,p6}_watchdog(), EvtSel0 is first zeroed, then written with
> everything but the enable bit, then written with the enable bit.
> 
> setup_p4_watchdog() is slightly more complicated, owing to what
> appears to be a bug introduced by commit 2a2bd8de16b6 ("Clean up NMI
> watchdog handler."), which causes a second bit to be temporarily
> different too.
> 
> The middle of the three writes is useless in all cases.  Drop it.

Spotting the 1st write in setup_p4_watchdog() wasn't quite as easy, as
MSR_P4_BPU_CCCR0 (as passed to clear_msr_range()) has nothing to do with
MSR_P4_IQ_CCCR0. Using unrelated MSR names there is as unhelpful as using
raw hex numbers.

> While doing this, rename the 'counter' parameter for
> setup_p6_watchdog().  It is the event which is passed in; the counter
> is always counter 0.
> 
> No functional change.

These sequences of writes almost look as if they were trying to cover for
errata. Are you sufficiently sure there are none anywhere, for this to
truly be no functional change? If so, ...

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Jan

Re: [PATCH 5/5] x86/nmi: Don't configure EvtSel repeatedly

[Andrew Cooper]

On 05/08/2026 3:20 pm, Jan Beulich wrote:
> On 05.08.2026 14:45, Andrew Cooper wrote:
>> In both setup_{k7,p6}_watchdog(), EvtSel0 is first zeroed, then written with
>> everything but the enable bit, then written with the enable bit.
>>
>> setup_p4_watchdog() is slightly more complicated, owing to what
>> appears to be a bug introduced by commit 2a2bd8de16b6 ("Clean up NMI
>> watchdog handler."), which causes a second bit to be temporarily
>> different too.
>>
>> The middle of the three writes is useless in all cases.  Drop it.
> Spotting the 1st write in setup_p4_watchdog() wasn't quite as easy, as
> MSR_P4_BPU_CCCR0 (as passed to clear_msr_range()) has nothing to do with
> MSR_P4_IQ_CCCR0. Using unrelated MSR names there is as unhelpful as using
> raw hex numbers.

Perf counters on the P4 are utterly insane, but our local logic really
doesn't help matters.

Another option would be to remove P4 watchdog support, in the basis that
we really can't test it.

>
>> While doing this, rename the 'counter' parameter for
>> setup_p6_watchdog().  It is the event which is passed in; the counter
>> is always counter 0.
>>
>> No functional change.
> These sequences of writes almost look as if they were trying to cover for
> errata. Are you sufficiently sure there are none anywhere, for this to
> truly be no functional change?

There is a reason to write logic in this form; it's just not applicable
to us.

The original P6 (besides being 32bit only) had two counters, but the
enable bit was in counter 0 only and controlled both.  I.e. you needed
to write logic to program EvtSel1 without the enable bit (it is strictly
reserved), and then EvtSel0 with the enable bit, in that order.  This is
in the SDM, just well hidden.

I spoke to various people, including PeterZ who wrote the perf
infrastructure in Linux (starting with Westmere, so it never ran on P6),
and Linux has absolutely no logic of this form at all.

I am reasonably confident that it's just copy&paste without due care and
attention which has left us with the code in this form.

>  If so, ...
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

Re: [PATCH 0/5] x86/nmi: Watchdog fixes/improvement Part 1

[Andrew Cooper]

On 05/08/2026 2:42 pm, Jan Beulich wrote:
> On 05.08.2026 14:45, Andrew Cooper wrote:
>> This is the start of a very long rabbit hole to address the
>> mis-classification of some watchdog NMIs as non-watchdog NMIs.  For
>> now, just some simple and hopefully non-controvertial changes.
>>
>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2733861049
>>
>> Andrew Cooper (5):
>>   x86/nmi: Drop {reserve,release}_lapic_nmi()
>>   x86/nmi: Drop K7_NMI_EVENT
>>   x86/nmi: Misc style fixes
>>   x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms
>>   x86/nmi: Don't configure EvtSel repeatedly
>>
>>  xen/arch/x86/include/asm/apic.h |   2 -
>>  xen/arch/x86/nmi.c              | 153 ++++++++++----------------------
>>  2 files changed, 47 insertions(+), 108 deletions(-)
> This series, once again, is putting me in a difficult position: Should I look
> at it, or should I let it sit for two years or more, just like my earlier
> fixes in this area [1], [2] are? (Of course, as always so far, I will look at
> the patches, and I will likely also accept them going in ahead of mine. But I
> cannot exclude that at some point I might actually stop doing so, seeing how
> many of my patches are in that state. While at the same time none of yours
> are, afaict, i.e. as per the track record that I keep of what still needs
> responding to.)
>
> Yes, you did respond to [1], but is not being comfortable with a change really
> a reason to block it, when it _is_ an improvement, and when the alternative
> hasn't materialized in all the time?
>
> Jan
>
> [1] https://lists.xen.org/archives/html/xen-devel/2024-01/msg01365.html
> [2] https://lists.xen.org/archives/html/xen-devel/2024-04/msg00194.html

I'd forgotten about these.

Patch 1, I'm (still) distinctly uneasy about, but I dispute your claim
that it is an improvement.  You are adding complexity and not fixing
anything AFAICT.

The watchdog counts NMIs (and counts incorrectly; this is the root issue
I'm needing to fix).  A timeout is declared when a fixed number of NMIs
(10, in default configuration) pass without the timer softirq having run.

The rate of NMIs varies with P states, including lower than cpu_khz, and
differs between cores.  In some but not all hardware, we could switch
from Unhalted Cycles to Unhalted Reference Cycles, but even that has a
bit caveat saying that the definition changed in 12th Generation.

You are making the rate of the timer softirq dynamic, but it is an
arbitrary fixed rate still unconnected to the rate of NMIs.

The only fix is to make it safe for the NMI handler to read real time. 
Until that time, in a choice between your patch and saying "well don't
set watchdog_timeout=1 then", I'd firmly favour the latter because at
least it means there's less to revert when a real fix does come along.


For patch 2, I had figured that bug out independently though inspection,
and yes I do agree it's an issue.  I was debating removing
watchdog_timeout=, and agree with that aspect of the patch.  However,
watchdog_force needs deleting to fix the incorrect counting, and with
your /* reset to defaults */ you're breaking the incremental property we
have of command line parsing elsewhere; specifically "watchdog=force
watchdog=10s" now sets force to false.

I will make sure to address this bug in my series, but I think it will
be a fairly different patch when the other dust has settled.

~Andrew

Re: [PATCH 0/5] x86/nmi: Watchdog fixes/improvement Part 1

[Jan Beulich]

On 05.08.2026 19:56, Andrew Cooper wrote:
> On 05/08/2026 2:42 pm, Jan Beulich wrote:
>> On 05.08.2026 14:45, Andrew Cooper wrote:
>>> This is the start of a very long rabbit hole to address the
>>> mis-classification of some watchdog NMIs as non-watchdog NMIs.  For
>>> now, just some simple and hopefully non-controvertial changes.
>>>
>>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2733861049
>>>
>>> Andrew Cooper (5):
>>>   x86/nmi: Drop {reserve,release}_lapic_nmi()
>>>   x86/nmi: Drop K7_NMI_EVENT
>>>   x86/nmi: Misc style fixes
>>>   x86/nmi: Check MSR_MISC_ENABLE for all Intel platforms
>>>   x86/nmi: Don't configure EvtSel repeatedly
>>>
>>>  xen/arch/x86/include/asm/apic.h |   2 -
>>>  xen/arch/x86/nmi.c              | 153 ++++++++++----------------------
>>>  2 files changed, 47 insertions(+), 108 deletions(-)
>> This series, once again, is putting me in a difficult position: Should I look
>> at it, or should I let it sit for two years or more, just like my earlier
>> fixes in this area [1], [2] are? (Of course, as always so far, I will look at
>> the patches, and I will likely also accept them going in ahead of mine. But I
>> cannot exclude that at some point I might actually stop doing so, seeing how
>> many of my patches are in that state. While at the same time none of yours
>> are, afaict, i.e. as per the track record that I keep of what still needs
>> responding to.)
>>
>> Yes, you did respond to [1], but is not being comfortable with a change really
>> a reason to block it, when it _is_ an improvement, and when the alternative
>> hasn't materialized in all the time?
>>
>> Jan
>>
>> [1] https://lists.xen.org/archives/html/xen-devel/2024-01/msg01365.html
>> [2] https://lists.xen.org/archives/html/xen-devel/2024-04/msg00194.html
> 
> I'd forgotten about these.
> 
> Patch 1, I'm (still) distinctly uneasy about, but I dispute your claim
> that it is an improvement.  You are adding complexity and not fixing
> anything AFAICT.
> 
> The watchdog counts NMIs (and counts incorrectly; this is the root issue
> I'm needing to fix).  A timeout is declared when a fixed number of NMIs
> (10, in default configuration) pass without the timer softirq having run.
> 
> The rate of NMIs varies with P states, including lower than cpu_khz, and
> differs between cores.  In some but not all hardware, we could switch
> from Unhalted Cycles to Unhalted Reference Cycles, but even that has a
> bit caveat saying that the definition changed in 12th Generation.
> 
> You are making the rate of the timer softirq dynamic, but it is an
> arbitrary fixed rate still unconnected to the rate of NMIs.

And I'm not claiming to address that (independent) issue. What the patch
does fix is a watchdog timeout occurring too early when a CPU runs in
turbo mode for perhaps an extended period of time.

> The only fix is to make it safe for the NMI handler to read real time. 
> Until that time, in a choice between your patch and saying "well don't
> set watchdog_timeout=1 then", I'd firmly favour the latter because at
> least it means there's less to revert when a real fix does come along.

As said in the description, if the ratio between max and normal is high
enough, even the default of 5 could be a problem.

> For patch 2, I had figured that bug out independently though inspection,
> and yes I do agree it's an issue.  I was debating removing
> watchdog_timeout=, and agree with that aspect of the patch.  However,
> watchdog_force needs deleting to fix the incorrect counting, and with
> your /* reset to defaults */ you're breaking the incremental property we
> have of command line parsing elsewhere; specifically "watchdog=force
> watchdog=10s" now sets force to false.
> 
> I will make sure to address this bug in my series, but I think it will
> be a fairly different patch when the other dust has settled.

Okay, we'll see if and when that arrives. With your intent to address
this differently, I don't see a reason then to try and adjust the cmdline
behavior. FTR, with watchdog= in particular I'm rather uncertain whether
the common (but unwritten) "incremental" policy is appropriate.

Jan

Re: [PATCH 5/5] x86/nmi: Don't configure EvtSel repeatedly

[Jan Beulich]

On 05.08.2026 17:37, Andrew Cooper wrote:
> On 05/08/2026 3:20 pm, Jan Beulich wrote:
>> On 05.08.2026 14:45, Andrew Cooper wrote:
>>> In both setup_{k7,p6}_watchdog(), EvtSel0 is first zeroed, then written with
>>> everything but the enable bit, then written with the enable bit.
>>>
>>> setup_p4_watchdog() is slightly more complicated, owing to what
>>> appears to be a bug introduced by commit 2a2bd8de16b6 ("Clean up NMI
>>> watchdog handler."), which causes a second bit to be temporarily
>>> different too.
>>>
>>> The middle of the three writes is useless in all cases.  Drop it.
>> Spotting the 1st write in setup_p4_watchdog() wasn't quite as easy, as
>> MSR_P4_BPU_CCCR0 (as passed to clear_msr_range()) has nothing to do with
>> MSR_P4_IQ_CCCR0. Using unrelated MSR names there is as unhelpful as using
>> raw hex numbers.
> 
> Perf counters on the P4 are utterly insane, but our local logic really
> doesn't help matters.
> 
> Another option would be to remove P4 watchdog support, in the basis that
> we really can't test it.

Well, my Tulsa system is still alive, and the watchdog looks to be working
there.

Jan

Re: [PATCH 5/5] x86/nmi: Don't configure EvtSel repeatedly

[Andrew Cooper]

On 06/08/2026 7:57 am, Jan Beulich wrote:
> On 05.08.2026 17:37, Andrew Cooper wrote:
>> On 05/08/2026 3:20 pm, Jan Beulich wrote:
>>> On 05.08.2026 14:45, Andrew Cooper wrote:
>>>> In both setup_{k7,p6}_watchdog(), EvtSel0 is first zeroed, then written with
>>>> everything but the enable bit, then written with the enable bit.
>>>>
>>>> setup_p4_watchdog() is slightly more complicated, owing to what
>>>> appears to be a bug introduced by commit 2a2bd8de16b6 ("Clean up NMI
>>>> watchdog handler."), which causes a second bit to be temporarily
>>>> different too.
>>>>
>>>> The middle of the three writes is useless in all cases.  Drop it.
>>> Spotting the 1st write in setup_p4_watchdog() wasn't quite as easy, as
>>> MSR_P4_BPU_CCCR0 (as passed to clear_msr_range()) has nothing to do with
>>> MSR_P4_IQ_CCCR0. Using unrelated MSR names there is as unhelpful as using
>>> raw hex numbers.
>> Perf counters on the P4 are utterly insane, but our local logic really
>> doesn't help matters.
>>
>> Another option would be to remove P4 watchdog support, in the basis that
>> we really can't test it.
> Well, my Tulsa system is still alive, and the watchdog looks to be working
> there.

Oh, if you're still able to test, then that's even better.

~Andrew