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

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

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

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