[PATCH 0/9] x86: Cleanup NMI handling

[PATCH 0/9] x86: Cleanup NMI handling

[Sohil Mehta]

While reworking the NMI-source patches[1], I spent quite a few days
understanding the existing NMI handling code. The intention of this series is
to fix the inconsistencies and redundancies that I encountered.

This series also includes improved documentation to make the code easier to
follow. It does not introduce any significant functional changes.

I have tried to split the patches logically to make them easier to review. Let
me know if some of them should be combined. The patches are in no particular
order and can be picked up independently. 

The documentation updates in patches (5, 6, 7) are to the best of my ability.
However, I would really appreciate extra eyes to ensure it is precise.

The updated NMI-source patches will follow this series sometime later.

@Ingo, I tried to format the commit references the way you prefer:

  Commit:
    feeaf5512947 ("x86: Move sysctls into arch/x86")

However, checkpatch seems to dislike it, so I reformatted them as below:

  Commit feeaf5512947 ("x86: Move sysctls into arch/x86")

Is there a specific guideline for x86?

[1]: https://lore.kernel.org/lkml/20240709143906.1040477-1-jacob.jun.pan@linux.intel.com/

Sohil Mehta (9):
  x86/nmi: Simplify unknown NMI panic handling
  x86/nmi: Consolidate NMI panic variables
  x86/nmi: Use a macro to initialize NMI descriptors
  x86/nmi: Remove export of local_touch_nmi()
  x86/nmi: Fix comment in unknown NMI handling
  x86/nmi: Improve and relocate NMI handler comments
  x86/nmi: Improve NMI header documentation
  x86/nmi: Clean up NMI selftest
  x86/nmi: Improve NMI duration console print

 arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
 arch/x86/include/asm/x86_init.h |  1 +
 arch/x86/kernel/dumpstack.c     |  2 -
 arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
 arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
 arch/x86/kernel/setup.c         | 37 ++++++--------
 include/linux/panic.h           |  2 -
 7 files changed, 122 insertions(+), 108 deletions(-)

-- 
2.43.0

[PATCH 1/9] x86/nmi: Simplify unknown NMI panic handling

[Sohil Mehta]

The unknown_nmi_panic variable is used to control whether the kernel
should panic on unknown NMIs. There is a sysctl entry for the same, which
can be used to change the behavior at runtime.

However, it seems that in some places, the option unnecessarily depends
on CONFIG_X86_LOCAL_APIC. Other code in nmi.c uses unknown_nmi_panic
without such a dependency. This results in a few messy #ifdefs
splattered across the code. The dependency was likely introduce due to a
potential compile issue [1] reported a long time ago. Such an issue no
longer exists.

Also, similar NMI panic options, such as panic_on_unrecovered_nmi and
panic_on_io_nmi, do not have an explicit dependency on the local APIC.
Though, it's hard to imagine a production system without the local APIC
configuration, making a specific NMI sysctl option dependent on it
doesn't make sense.

Remove the explicit dependency between unknown NMI handling and the
local APIC to make the code cleaner and more consistent.

While at it, reorder the header includes to maintain alphabetical order.

[1]: https://lore.kernel.org/lkml/40BC67F9.3000609@myrealbox.com/

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/include/asm/nmi.h |  4 ++--
 arch/x86/kernel/setup.c    | 37 ++++++++++++++++---------------------
 2 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index f677382093f3..9cf96cce02fc 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -14,10 +14,10 @@ extern void release_perfctr_nmi(unsigned int);
 extern int reserve_evntsel_nmi(unsigned int);
 extern void release_evntsel_nmi(unsigned int);
 
-extern int unknown_nmi_panic;
-
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+extern int unknown_nmi_panic;
+
 #define NMI_FLAG_FIRST	1
 
 enum {
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c7164a8de983..c3e1ae7373e9 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -7,10 +7,11 @@
  */
 #include <linux/acpi.h>
 #include <linux/console.h>
-#include <linux/cpu.h>
 #include <linux/crash_dump.h>
+#include <linux/cpu.h>
 #include <linux/dma-map-ops.h>
 #include <linux/efi.h>
+#include <linux/hugetlb.h>
 #include <linux/ima.h>
 #include <linux/init_ohci1394_dma.h>
 #include <linux/initrd.h>
@@ -18,21 +19,19 @@
 #include <linux/memblock.h>
 #include <linux/panic_notifier.h>
 #include <linux/pci.h>
+#include <linux/random.h>
 #include <linux/root_dev.h>
-#include <linux/hugetlb.h>
-#include <linux/tboot.h>
-#include <linux/usb/xhci-dbgp.h>
 #include <linux/static_call.h>
 #include <linux/swiotlb.h>
-#include <linux/random.h>
+#include <linux/tboot.h>
+#include <linux/usb/xhci-dbgp.h>
+#include <linux/vmalloc.h>
 
 #include <uapi/linux/mount.h>
 
 #include <xen/xen.h>
 
 #include <asm/apic.h>
-#include <asm/efi.h>
-#include <asm/numa.h>
 #include <asm/bios_ebda.h>
 #include <asm/bugs.h>
 #include <asm/cacheinfo.h>
@@ -47,18 +46,16 @@
 #include <asm/mce.h>
 #include <asm/memtype.h>
 #include <asm/mtrr.h>
-#include <asm/realmode.h>
+#include <asm/nmi.h>
+#include <asm/numa.h>
 #include <asm/olpc_ofw.h>
 #include <asm/pci-direct.h>
 #include <asm/prom.h>
 #include <asm/proto.h>
+#include <asm/realmode.h>
 #include <asm/thermal.h>
 #include <asm/unwind.h>
 #include <asm/vsyscall.h>
-#include <linux/vmalloc.h>
-#if defined(CONFIG_X86_LOCAL_APIC)
-#include <asm/nmi.h>
-#endif
 
 /*
  * max_low_pfn_mapped: highest directly mapped pfn < 4 GB
@@ -150,6 +147,13 @@ static size_t ima_kexec_buffer_size;
 int bootloader_type, bootloader_version;
 
 static const struct ctl_table x86_sysctl_table[] = {
+	{
+		.procname       = "unknown_nmi_panic",
+		.data           = &unknown_nmi_panic,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec,
+	},
 	{
 		.procname	= "panic_on_unrecovered_nmi",
 		.data		= &panic_on_unrecovered_nmi,
@@ -185,15 +189,6 @@ static const struct ctl_table x86_sysctl_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
-#if defined(CONFIG_X86_LOCAL_APIC)
-	{
-		.procname       = "unknown_nmi_panic",
-		.data           = &unknown_nmi_panic,
-		.maxlen         = sizeof(int),
-		.mode           = 0644,
-		.proc_handler   = proc_dointvec,
-	},
-#endif
 #if defined(CONFIG_ACPI_SLEEP)
 	{
 		.procname	= "acpi_video_flags",
-- 
2.43.0

Re: [PATCH 1/9] x86/nmi: Simplify unknown NMI panic handling

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> The unknown_nmi_panic variable is used to control whether the kernel
> should panic on unknown NMIs. There is a sysctl entry for the same, which
> can be used to change the behavior at runtime.
> 
> However, it seems that in some places, the option unnecessarily depends
> on CONFIG_X86_LOCAL_APIC. Other code in nmi.c uses unknown_nmi_panic
> without such a dependency. This results in a few messy #ifdefs
> splattered across the code. The dependency was likely introduce due to a
							^
							introduced


[...]

> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index c7164a8de983..c3e1ae7373e9 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -7,10 +7,11 @@
>   */
>  #include <linux/acpi.h>
>  #include <linux/console.h>
> -#include <linux/cpu.h>
>  #include <linux/crash_dump.h>
> +#include <linux/cpu.h>

This code change doesn't seem to be able to make the headers in "alphabetical
order".

>  #include <linux/dma-map-ops.h>
>  #include <linux/efi.h>
> +#include <linux/hugetlb.h>
>  #include <linux/ima.h>
>  #include <linux/init_ohci1394_dma.h>
>  #include <linux/initrd.h>
> @@ -18,21 +19,19 @@
>  #include <linux/memblock.h>
>  #include <linux/panic_notifier.h>
>  #include <linux/pci.h>
> +#include <linux/random.h>
>  #include <linux/root_dev.h>
> -#include <linux/hugetlb.h>
> -#include <linux/tboot.h>
> -#include <linux/usb/xhci-dbgp.h>
>  #include <linux/static_call.h>
>  #include <linux/swiotlb.h>
> -#include <linux/random.h>
> +#include <linux/tboot.h>
> +#include <linux/usb/xhci-dbgp.h>
> +#include <linux/vmalloc.h>
>  
>  #include <uapi/linux/mount.h>
>  
>  #include <xen/xen.h>
>  
>  #include <asm/apic.h>
> -#include <asm/efi.h>

Seems this header just got removed here but was never added back.

> -#include <asm/numa.h>
>  #include <asm/bios_ebda.h>
>  #include <asm/bugs.h>
>  #include <asm/cacheinfo.h>
> @@ -47,18 +46,16 @@
>  #include <asm/mce.h>
>  #include <asm/memtype.h>
>  #include <asm/mtrr.h>
> -#include <asm/realmode.h>
> +#include <asm/nmi.h>
> +#include <asm/numa.h>
>  #include <asm/olpc_ofw.h>
>  #include <asm/pci-direct.h>
>  #include <asm/prom.h>
>  #include <asm/proto.h>
> +#include <asm/realmode.h>
>  #include <asm/thermal.h>
>  #include <asm/unwind.h>
>  #include <asm/vsyscall.h>
> -#include <linux/vmalloc.h>
> -#if defined(CONFIG_X86_LOCAL_APIC)
> -#include <asm/nmi.h>
> -#endif
>  

[...]

Re: [PATCH 1/9] x86/nmi: Simplify unknown NMI panic handling

[Sohil Mehta]

On 3/31/2025 3:52 PM, Huang, Kai wrote:

>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index c7164a8de983..c3e1ae7373e9 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -7,10 +7,11 @@
>>   */
>>  #include <linux/acpi.h>
>>  #include <linux/console.h>
>> -#include <linux/cpu.h>
>>  #include <linux/crash_dump.h>
>> +#include <linux/cpu.h>
> 
> This code change doesn't seem to be able to make the headers in "alphabetical
> order".
> 

Ah! Thanks for catching it.


>>  
>>  #include <asm/apic.h>
>> -#include <asm/efi.h>
> 
> Seems this header just got removed here but was never added back.
> 

<asm/efi.h> is included twice. Therefore, the diff only shows it being
removed.

>> -#include <asm/numa.h>
>>  #include <asm/bios_ebda.h>
>>  #include <asm/bugs.h>
>>  #include <asm/cacheinfo.h>

Thanks,
Sohil

Re: [PATCH 1/9] x86/nmi: Simplify unknown NMI panic handling

[Nikolay Borisov]

On 28.03.25 г. 1:46 ч., Sohil Mehta wrote:
> The unknown_nmi_panic variable is used to control whether the kernel
> should panic on unknown NMIs. There is a sysctl entry for the same, which
> can be used to change the behavior at runtime.
> 
> However, it seems that in some places, the option unnecessarily depends
> on CONFIG_X86_LOCAL_APIC. Other code in nmi.c uses unknown_nmi_panic
> without such a dependency. This results in a few messy #ifdefs
> splattered across the code. The dependency was likely introduce due to a
> potential compile issue [1] reported a long time ago. Such an issue no
> longer exists.
> 
> Also, similar NMI panic options, such as panic_on_unrecovered_nmi and
> panic_on_io_nmi, do not have an explicit dependency on the local APIC.
> Though, it's hard to imagine a production system without the local APIC
> configuration, making a specific NMI sysctl option dependent on it
> doesn't make sense.
> 
> Remove the explicit dependency between unknown NMI handling and the
> local APIC to make the code cleaner and more consistent.
> 
> While at it, reorder the header includes to maintain alphabetical order.
 > > [1]: https://lore.kernel.org/lkml/40BC67F9.3000609@myrealbox.com/
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
>   arch/x86/include/asm/nmi.h |  4 ++--
>   arch/x86/kernel/setup.c    | 37 ++++++++++++++++---------------------
>   2 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index f677382093f3..9cf96cce02fc 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -14,10 +14,10 @@ extern void release_perfctr_nmi(unsigned int);
>   extern int reserve_evntsel_nmi(unsigned int);
>   extern void release_evntsel_nmi(unsigned int);
>   
> -extern int unknown_nmi_panic;
> -
>   #endif /* CONFIG_X86_LOCAL_APIC */
>   
> +extern int unknown_nmi_panic;
> +
>   #define NMI_FLAG_FIRST	1
>   
>   enum {
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index c7164a8de983..c3e1ae7373e9 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -7,10 +7,11 @@
>    */
>   #include <linux/acpi.h>
>   #include <linux/console.h>
> -#include <linux/cpu.h>
>   #include <linux/crash_dump.h>
> +#include <linux/cpu.h>
>   #include <linux/dma-map-ops.h>
>   #include <linux/efi.h>
> +#include <linux/hugetlb.h>
>   #include <linux/ima.h>
>   #include <linux/init_ohci1394_dma.h>
>   #include <linux/initrd.h>
> @@ -18,21 +19,19 @@
>   #include <linux/memblock.h>
>   #include <linux/panic_notifier.h>
>   #include <linux/pci.h>
> +#include <linux/random.h>
>   #include <linux/root_dev.h>
> -#include <linux/hugetlb.h>
> -#include <linux/tboot.h>
> -#include <linux/usb/xhci-dbgp.h>
>   #include <linux/static_call.h>
>   #include <linux/swiotlb.h>
> -#include <linux/random.h>
> +#include <linux/tboot.h>
> +#include <linux/usb/xhci-dbgp.h>
> +#include <linux/vmalloc.h>
>   
>   #include <uapi/linux/mount.h>
>   
>   #include <xen/xen.h>
>   
>   #include <asm/apic.h>
> -#include <asm/efi.h>
> -#include <asm/numa.h>
>   #include <asm/bios_ebda.h>
>   #include <asm/bugs.h>
>   #include <asm/cacheinfo.h>
> @@ -47,18 +46,16 @@
>   #include <asm/mce.h>
>   #include <asm/memtype.h>
>   #include <asm/mtrr.h>
> -#include <asm/realmode.h>
> +#include <asm/nmi.h>
> +#include <asm/numa.h>
>   #include <asm/olpc_ofw.h>
>   #include <asm/pci-direct.h>
>   #include <asm/prom.h>
>   #include <asm/proto.h>
> +#include <asm/realmode.h>
>   #include <asm/thermal.h>
>   #include <asm/unwind.h>
>   #include <asm/vsyscall.h>
> -#include <linux/vmalloc.h>
> -#if defined(CONFIG_X86_LOCAL_APIC)
> -#include <asm/nmi.h>
> -#endif

As far as headers are concerned only this change falls under the 
"simplify nmi handling code" the others while nice cleanups should 
ideally be in a separate patch.

>   
>   /*
>    * max_low_pfn_mapped: highest directly mapped pfn < 4 GB
> @@ -150,6 +147,13 @@ static size_t ima_kexec_buffer_size;
>   int bootloader_type, bootloader_version;
>   
>   static const struct ctl_table x86_sysctl_table[] = {
> +	{
> +		.procname       = "unknown_nmi_panic",
> +		.data           = &unknown_nmi_panic,
> +		.maxlen         = sizeof(int),
> +		.mode           = 0644,
> +		.proc_handler   = proc_dointvec,
> +	},
>   	{
>   		.procname	= "panic_on_unrecovered_nmi",
>   		.data		= &panic_on_unrecovered_nmi,
> @@ -185,15 +189,6 @@ static const struct ctl_table x86_sysctl_table[] = {
>   		.mode		= 0644,
>   		.proc_handler	= proc_dointvec,
>   	},
> -#if defined(CONFIG_X86_LOCAL_APIC)
> -	{
> -		.procname       = "unknown_nmi_panic",
> -		.data           = &unknown_nmi_panic,
> -		.maxlen         = sizeof(int),
> -		.mode           = 0644,
> -		.proc_handler   = proc_dointvec,
> -	},
> -#endif

Why move the definition and not just delete the #ifdef ?

>   #if defined(CONFIG_ACPI_SLEEP)
>   	{
>   		.procname	= "acpi_video_flags",

Re: [PATCH 1/9] x86/nmi: Simplify unknown NMI panic handling

[Sohil Mehta]

On 4/1/2025 8:00 AM, Nikolay Borisov wrote:

>> +#include <asm/realmode.h>
>>   #include <asm/thermal.h>
>>   #include <asm/unwind.h>
>>   #include <asm/vsyscall.h>
>> -#include <linux/vmalloc.h>
>> -#if defined(CONFIG_X86_LOCAL_APIC)
>> -#include <asm/nmi.h>
>> -#endif
> 
> As far as headers are concerned only this change falls under the 
> "simplify nmi handling code" the others while nice cleanups should 
> ideally be in a separate patch.
> 

You are right. The simplification is only for the #ifdef. I could have
just removed the CONFIG check and left the rest of the headers as-is.

But the <linux/vmalloc.h> include there was completely out of place and
needed to be moved. This eventually led to the reshuffle.

I feel header reordering by itself may not be considered worthwhile as a
patch. Since this series is focussed on cleanups, I tried to keep
cleanups in the same vicinity together. It's mainly to avoid bloating
the patch count but still keep the reviewing of these changes manageable.


>>   
>>   /*
>>    * max_low_pfn_mapped: highest directly mapped pfn < 4 GB
>> @@ -150,6 +147,13 @@ static size_t ima_kexec_buffer_size;
>>   int bootloader_type, bootloader_version;
>>   
>>   static const struct ctl_table x86_sysctl_table[] = {
>> +	{
>> +		.procname       = "unknown_nmi_panic",
>> +		.data           = &unknown_nmi_panic,
>> +		.maxlen         = sizeof(int),
>> +		.mode           = 0644,
>> +		.proc_handler   = proc_dointvec,
>> +	},
>>   	{
>>   		.procname	= "panic_on_unrecovered_nmi",
>>   		.data		= &panic_on_unrecovered_nmi,
>> @@ -185,15 +189,6 @@ static const struct ctl_table x86_sysctl_table[] = {
>>   		.mode		= 0644,
>>   		.proc_handler	= proc_dointvec,
>>   	},
>> -#if defined(CONFIG_X86_LOCAL_APIC)
>> -	{
>> -		.procname       = "unknown_nmi_panic",
>> -		.data           = &unknown_nmi_panic,
>> -		.maxlen         = sizeof(int),
>> -		.mode           = 0644,
>> -		.proc_handler   = proc_dointvec,
>> -	},
>> -#endif
> 
> Why move the definition and not just delete the #ifdef ?
> 

Mainly for consistency. This keeps all the NMI panic options together in
x86_sysctl_table[].

>>   #if defined(CONFIG_ACPI_SLEEP)
>>   	{
>>   		.procname	= "acpi_video_flags",
> 

[tip: x86/nmi] x86/nmi: Simplify unknown NMI panic handling

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     2e016da1cbbd013368095270c040e065678c38f7
Gitweb:        https://git.kernel.org/tip/2e016da1cbbd013368095270c040e065678c38f7
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:21 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:25:41 +02:00

x86/nmi: Simplify unknown NMI panic handling

The unknown_nmi_panic variable is used to control whether the kernel
should panic on unknown NMIs. There is a sysctl entry under:

  /proc/sys/kernel/unknown_nmi_panic

which can be used to change the behavior at runtime.

However, it seems that in some places, the option unnecessarily depends
on CONFIG_X86_LOCAL_APIC. Other code in nmi.c uses unknown_nmi_panic
without such a dependency. This results in a few messy #ifdefs
splattered across the code. The dependency was likely introduce due to a
potential build bug reported a long time ago:

  https://lore.kernel.org/lkml/40BC67F9.3000609@myrealbox.com/

This build bug no longer exists.

Also, similar NMI panic options, such as panic_on_unrecovered_nmi and
panic_on_io_nmi, do not have an explicit dependency on the local APIC
either.

Though, it's hard to imagine a production system without the local APIC
configuration, making a specific NMI sysctl option dependent on it
doesn't make sense.

Remove the explicit dependency between unknown NMI handling and the
local APIC to make the code cleaner and more consistent.

While at it, reorder the header includes to maintain alphabetical order.

[ mingo: Cleaned up the changelog a bit, truly ordered the headers ... ]

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-2-sohil.mehta@intel.com
---
 arch/x86/include/asm/nmi.h |  4 ++--
 arch/x86/kernel/setup.c    | 35 +++++++++++++++--------------------
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index f677382..9cf96cc 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -14,10 +14,10 @@ extern void release_perfctr_nmi(unsigned int);
 extern int reserve_evntsel_nmi(unsigned int);
 extern void release_evntsel_nmi(unsigned int);
 
-extern int unknown_nmi_panic;
-
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+extern int unknown_nmi_panic;
+
 #define NMI_FLAG_FIRST	1
 
 enum {
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c7164a8..ecf4c13 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -11,6 +11,7 @@
 #include <linux/crash_dump.h>
 #include <linux/dma-map-ops.h>
 #include <linux/efi.h>
+#include <linux/hugetlb.h>
 #include <linux/ima.h>
 #include <linux/init_ohci1394_dma.h>
 #include <linux/initrd.h>
@@ -18,21 +19,19 @@
 #include <linux/memblock.h>
 #include <linux/panic_notifier.h>
 #include <linux/pci.h>
+#include <linux/random.h>
 #include <linux/root_dev.h>
-#include <linux/hugetlb.h>
-#include <linux/tboot.h>
-#include <linux/usb/xhci-dbgp.h>
 #include <linux/static_call.h>
 #include <linux/swiotlb.h>
-#include <linux/random.h>
+#include <linux/tboot.h>
+#include <linux/usb/xhci-dbgp.h>
+#include <linux/vmalloc.h>
 
 #include <uapi/linux/mount.h>
 
 #include <xen/xen.h>
 
 #include <asm/apic.h>
-#include <asm/efi.h>
-#include <asm/numa.h>
 #include <asm/bios_ebda.h>
 #include <asm/bugs.h>
 #include <asm/cacheinfo.h>
@@ -47,18 +46,16 @@
 #include <asm/mce.h>
 #include <asm/memtype.h>
 #include <asm/mtrr.h>
-#include <asm/realmode.h>
+#include <asm/nmi.h>
+#include <asm/numa.h>
 #include <asm/olpc_ofw.h>
 #include <asm/pci-direct.h>
 #include <asm/prom.h>
 #include <asm/proto.h>
+#include <asm/realmode.h>
 #include <asm/thermal.h>
 #include <asm/unwind.h>
 #include <asm/vsyscall.h>
-#include <linux/vmalloc.h>
-#if defined(CONFIG_X86_LOCAL_APIC)
-#include <asm/nmi.h>
-#endif
 
 /*
  * max_low_pfn_mapped: highest directly mapped pfn < 4 GB
@@ -151,6 +148,13 @@ int bootloader_type, bootloader_version;
 
 static const struct ctl_table x86_sysctl_table[] = {
 	{
+		.procname       = "unknown_nmi_panic",
+		.data           = &unknown_nmi_panic,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec,
+	},
+	{
 		.procname	= "panic_on_unrecovered_nmi",
 		.data		= &panic_on_unrecovered_nmi,
 		.maxlen		= sizeof(int),
@@ -185,15 +189,6 @@ static const struct ctl_table x86_sysctl_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
-#if defined(CONFIG_X86_LOCAL_APIC)
-	{
-		.procname       = "unknown_nmi_panic",
-		.data           = &unknown_nmi_panic,
-		.maxlen         = sizeof(int),
-		.mode           = 0644,
-		.proc_handler   = proc_dointvec,
-	},
-#endif
 #if defined(CONFIG_ACPI_SLEEP)
 	{
 		.procname	= "acpi_video_flags",

[PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Sohil Mehta]

Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
recently moved the sysctl handling of panic_on_unrecovered_nmi and
panic_on_io_nmi to x86-specific code. These variables no longer need to
be declared in the generic header file.

Relocate the variable definitions and declarations closer to where they
are used. This makes all the NMI panic options consistent and easier to
track.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/include/asm/nmi.h  | 2 ++
 arch/x86/kernel/dumpstack.c | 2 --
 arch/x86/kernel/nmi.c       | 3 +++
 include/linux/panic.h       | 2 --
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index 9cf96cce02fc..f85aea7bf7f1 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -17,6 +17,8 @@ extern void release_evntsel_nmi(unsigned int);
 #endif /* CONFIG_X86_LOCAL_APIC */
 
 extern int unknown_nmi_panic;
+extern int panic_on_unrecovered_nmi;
+extern int panic_on_io_nmi;
 
 #define NMI_FLAG_FIRST	1
 
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 91639d1e4ec2..4abc9153e8a4 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -23,8 +23,6 @@
 #include <asm/stacktrace.h>
 #include <asm/unwind.h>
 
-int panic_on_unrecovered_nmi;
-int panic_on_io_nmi;
 static int die_counter;
 
 static struct pt_regs exec_summary_regs;
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 9a95d00f1423..671d846ed620 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -91,6 +91,9 @@ static DEFINE_PER_CPU(struct nmi_stats, nmi_stats);
 static int ignore_nmis __read_mostly;
 
 int unknown_nmi_panic;
+int panic_on_unrecovered_nmi;
+int panic_on_io_nmi;
+
 /*
  * Prevent NMI reason port (0x61) being accessed simultaneously, can
  * only be used in NMI handler.
diff --git a/include/linux/panic.h b/include/linux/panic.h
index 54d90b6c5f47..b0ec89a9a966 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -20,8 +20,6 @@ extern bool panic_triggering_all_cpu_backtrace;
 extern int panic_timeout;
 extern unsigned long panic_print;
 extern int panic_on_oops;
-extern int panic_on_unrecovered_nmi;
-extern int panic_on_io_nmi;
 extern int panic_on_warn;
 
 extern unsigned long panic_on_taint;
-- 
2.43.0

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> Commit feeaf5512947 ("x86: Move sysctls into arch/x86")

AFAICT the commit hash is wrong.  It should be c305a4e98378.

> recently moved the sysctl handling of panic_on_unrecovered_nmi and
> panic_on_io_nmi to x86-specific code. These variables no longer need to
> be declared in the generic header file.
> 
> Relocate the variable definitions and declarations closer to where they
> are used. This makes all the NMI panic options consistent and easier to
> track.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Sohil Mehta]

On 3/31/2025 3:43 PM, Huang, Kai wrote:
> On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
>> Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
> 
> AFAICT the commit hash is wrong.  It should be c305a4e98378.
> 

Yes, sorry about that. I ended up referencing a pre-merge duplicate
commit in my repo.

Thankfully, Ingo fixed it when he applied the patch:
https://web.git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/nmi&id=1a5b15f6b4d18507dc3b2958ca01877cfc8808fd


>> recently moved the sysctl handling of panic_on_unrecovered_nmi and
>> panic_on_io_nmi to x86-specific code. These variables no longer need to
>> be declared in the generic header file.
>>
>> Relocate the variable definitions and declarations closer to where they
>> are used. This makes all the NMI panic options consistent and easier to
>> track.
>>
>> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> 

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Huang, Kai]

On Mon, 2025-03-31 at 15:50 -0700, Mehta, Sohil wrote:
> On 3/31/2025 3:43 PM, Huang, Kai wrote:
> > On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> > > Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
> > 
> > AFAICT the commit hash is wrong.  It should be c305a4e98378.
> > 
> 
> Yes, sorry about that. I ended up referencing a pre-merge duplicate
> commit in my repo.
> 
> Thankfully, Ingo fixed it when he applied the patch:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/nmi&id=1a5b15f6b4d18507dc3b2958ca01877cfc8808fd
> 
> 

Ah I didn't know this series was merged.  I think I'll stop looking at it.  :-)

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Sohil Mehta]

On 3/31/2025 4:05 PM, Huang, Kai wrote:
> On Mon, 2025-03-31 at 15:50 -0700, Mehta, Sohil wrote:
>> On 3/31/2025 3:43 PM, Huang, Kai wrote:
>>> On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
>>>> Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
>>>
>>> AFAICT the commit hash is wrong.  It should be c305a4e98378.
>>>
>>
>> Yes, sorry about that. I ended up referencing a pre-merge duplicate
>> commit in my repo.
>>
>> Thankfully, Ingo fixed it when he applied the patch:
>> https://web.git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/nmi&id=1a5b15f6b4d18507dc3b2958ca01877cfc8808fd
>>
>>
> 
> Ah I didn't know this series was merged.  I think I'll stop looking at it.  :-)

Your review comments have been useful. Please continue reviewing if you
can. You found an issue in patch 1 that both of us missed.

Also, I am not sure if the series is truly merged. I haven't seen the
tip tree tip-bot emails yet. Maybe Ingo can answer this better.

Sohil

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Huang, Kai]

On Mon, 2025-03-31 at 16:20 -0700, Mehta, Sohil wrote:
> On 3/31/2025 4:05 PM, Huang, Kai wrote:
> > On Mon, 2025-03-31 at 15:50 -0700, Mehta, Sohil wrote:
> > > On 3/31/2025 3:43 PM, Huang, Kai wrote:
> > > > On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> > > > > Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
> > > > 
> > > > AFAICT the commit hash is wrong.  It should be c305a4e98378.
> > > > 
> > > 
> > > Yes, sorry about that. I ended up referencing a pre-merge duplicate
> > > commit in my repo.
> > > 
> > > Thankfully, Ingo fixed it when he applied the patch:
> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/nmi&id=1a5b15f6b4d18507dc3b2958ca01877cfc8808fd
> > > 
> > > 
> > 
> > Ah I didn't know this series was merged.  I think I'll stop looking at it.  :-)
> 
> Your review comments have been useful. Please continue reviewing if you
> can. You found an issue in patch 1 that both of us missed.
> 

I just did.  I didn't find anything apart from one thing that I _think_ it might
be helpful to mention another commit in the changelog of patch 5 "x86/nmi: Fix
comment in unknown NMI handling".

Thanks.

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Sohil Mehta]

On 3/31/2025 6:06 PM, Huang, Kai wrote:

>>> Ah I didn't know this series was merged.  I think I'll stop looking at it.  :-)
>>
>> Your review comments have been useful. Please continue reviewing if you
>> can. You found an issue in patch 1 that both of us missed.
>>
> 
> I just did.  I didn't find anything apart from one thing that I _think_ it might
> be helpful to mention another commit in the changelog of patch 5 "x86/nmi: Fix
> comment in unknown NMI handling".
> 

Kai, I feel the additional commit is indirectly implied. I am inclined
to leave the changelog as-is unless you feel strongly about it.

Thank you for the reviews. Really appreciate it!

Ingo, there are 2 main changes from the patches in tip:x86/nmi.

1) As pointed out by Kai, there is a minor oversight in patch 1 on my
part. The header files aren't strictly ordered. There needs to be a
single line change to keep them alphabetically ordered.

2) As discussed in patch 7, the title for split-off patch is incorrect.

If you prefer, I can generate a new series based on the patches you have
in tip to save you the extra effort again. Or I can generate a single
additional patch on top if you want.

OTOH, feel free to make the changes yourself and include my signoffs.

Re: [PATCH 2/9] x86/nmi: Consolidate NMI panic variables

[Huang, Kai]

On Mon, 2025-03-31 at 22:46 -0700, Sohil Mehta wrote:
> On 3/31/2025 6:06 PM, Huang, Kai wrote:
> 
> > > > Ah I didn't know this series was merged.  I think I'll stop looking at it.  :-)
> > > 
> > > Your review comments have been useful. Please continue reviewing if you
> > > can. You found an issue in patch 1 that both of us missed.
> > > 
> > 
> > I just did.  I didn't find anything apart from one thing that I _think_ it might
> > be helpful to mention another commit in the changelog of patch 5 "x86/nmi: Fix
> > comment in unknown NMI handling".
> > 
> 
> Kai, I feel the additional commit is indirectly implied. I am inclined
> to leave the changelog as-is unless you feel strongly about it.

Sure np to me.

[tip: x86/nmi] x86/nmi: Consolidate NMI panic variables

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     78a0323506f01e8017a5826cd7e91951c13184fa
Gitweb:        https://git.kernel.org/tip/78a0323506f01e8017a5826cd7e91951c13184fa
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:22 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:25:56 +02:00

x86/nmi: Consolidate NMI panic variables

Commit:

  c305a4e98378 ("x86: Move sysctls into arch/x86")

recently moved the sysctl handling of panic_on_unrecovered_nmi and
panic_on_io_nmi to x86-specific code. These variables no longer need to
be declared in the generic header file.

Relocate the variable definitions and declarations closer to where they
are used. This makes all the NMI panic options consistent and easier to
track.

[ mingo: Fixed up the SHA1 of the commit reference. ]

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Cc: Joel Granados <joel.granados@kernel.org>
Link: https://lore.kernel.org/r/20250327234629.3953536-3-sohil.mehta@intel.com
---
 arch/x86/include/asm/nmi.h  | 2 ++
 arch/x86/kernel/dumpstack.c | 2 --
 arch/x86/kernel/nmi.c       | 3 +++
 include/linux/panic.h       | 2 --
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index 9cf96cc..f85aea7 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -17,6 +17,8 @@ extern void release_evntsel_nmi(unsigned int);
 #endif /* CONFIG_X86_LOCAL_APIC */
 
 extern int unknown_nmi_panic;
+extern int panic_on_unrecovered_nmi;
+extern int panic_on_io_nmi;
 
 #define NMI_FLAG_FIRST	1
 
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index c6fefd4..71ee201 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -23,8 +23,6 @@
 #include <asm/stacktrace.h>
 #include <asm/unwind.h>
 
-int panic_on_unrecovered_nmi;
-int panic_on_io_nmi;
 static int die_counter;
 
 static struct pt_regs exec_summary_regs;
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 9a95d00..671d846 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -91,6 +91,9 @@ static DEFINE_PER_CPU(struct nmi_stats, nmi_stats);
 static int ignore_nmis __read_mostly;
 
 int unknown_nmi_panic;
+int panic_on_unrecovered_nmi;
+int panic_on_io_nmi;
+
 /*
  * Prevent NMI reason port (0x61) being accessed simultaneously, can
  * only be used in NMI handler.
diff --git a/include/linux/panic.h b/include/linux/panic.h
index 2494d51..4adc657 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -20,8 +20,6 @@ extern bool panic_triggering_all_cpu_backtrace;
 extern int panic_timeout;
 extern unsigned long panic_print;
 extern int panic_on_oops;
-extern int panic_on_unrecovered_nmi;
-extern int panic_on_io_nmi;
 extern int panic_on_warn;
 
 extern unsigned long panic_on_taint;

[PATCH 3/9] x86/nmi: Use a macro to initialize NMI descriptors

[Sohil Mehta]

The NMI descriptors for each NMI type are stored in an array. However,
they are currently initialized using raw numbers, which makes it
difficult to understand the code.

Introduce a macro to initialize the NMI descriptors using the NMI type
enum values to make the code more readable.

No functional change.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/nmi.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 671d846ed620..6a5dc35522c8 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -49,27 +49,20 @@ struct nmi_desc {
 	struct list_head head;
 };
 
-static struct nmi_desc nmi_desc[NMI_MAX] = 
-{
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[0].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[0].head),
-	},
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[1].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[1].head),
-	},
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[2].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[2].head),
-	},
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[3].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[3].head),
-	},
+#define NMI_DESC_INIT(type) { \
+	.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[type].lock), \
+	.head = LIST_HEAD_INIT(nmi_desc[type].head), \
+}
 
+static struct nmi_desc nmi_desc[NMI_MAX] = {
+	NMI_DESC_INIT(NMI_LOCAL),
+	NMI_DESC_INIT(NMI_UNKNOWN),
+	NMI_DESC_INIT(NMI_SERR),
+	NMI_DESC_INIT(NMI_IO_CHECK),
 };
 
+#define nmi_to_desc(type) (&nmi_desc[type])
+
 struct nmi_stats {
 	unsigned int normal;
 	unsigned int unknown;
@@ -107,8 +100,6 @@ static int __init setup_unknown_nmi_panic(char *str)
 }
 __setup("unknown_nmi_panic", setup_unknown_nmi_panic);
 
-#define nmi_to_desc(type) (&nmi_desc[type])
-
 static u64 nmi_longest_ns = 1 * NSEC_PER_MSEC;
 
 static int __init nmi_warning_debugfs(void)
-- 
2.43.0

Re: [PATCH 3/9] x86/nmi: Use a macro to initialize NMI descriptors

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> The NMI descriptors for each NMI type are stored in an array. However,
> they are currently initialized using raw numbers, which makes it
> difficult to understand the code.
> 
> Introduce a macro to initialize the NMI descriptors using the NMI type
> enum values to make the code more readable.
> 
> No functional change.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

[tip: x86/nmi] x86/nmi: Use a macro to initialize NMI descriptors

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     4a8fba4be879251fa010d248c179efbd42ec667d
Gitweb:        https://git.kernel.org/tip/4a8fba4be879251fa010d248c179efbd42ec667d
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:23 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:01 +02:00

x86/nmi: Use a macro to initialize NMI descriptors

The NMI descriptors for each NMI type are stored in an array. However,
they are currently initialized using raw numbers, which makes it
difficult to understand the code.

Introduce a macro to initialize the NMI descriptors using the NMI type
enum values to make the code more readable.

No functional change intended.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-4-sohil.mehta@intel.com
---
 arch/x86/kernel/nmi.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 671d846..6a5dc35 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -49,27 +49,20 @@ struct nmi_desc {
 	struct list_head head;
 };
 
-static struct nmi_desc nmi_desc[NMI_MAX] = 
-{
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[0].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[0].head),
-	},
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[1].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[1].head),
-	},
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[2].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[2].head),
-	},
-	{
-		.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[3].lock),
-		.head = LIST_HEAD_INIT(nmi_desc[3].head),
-	},
+#define NMI_DESC_INIT(type) { \
+	.lock = __RAW_SPIN_LOCK_UNLOCKED(&nmi_desc[type].lock), \
+	.head = LIST_HEAD_INIT(nmi_desc[type].head), \
+}
 
+static struct nmi_desc nmi_desc[NMI_MAX] = {
+	NMI_DESC_INIT(NMI_LOCAL),
+	NMI_DESC_INIT(NMI_UNKNOWN),
+	NMI_DESC_INIT(NMI_SERR),
+	NMI_DESC_INIT(NMI_IO_CHECK),
 };
 
+#define nmi_to_desc(type) (&nmi_desc[type])
+
 struct nmi_stats {
 	unsigned int normal;
 	unsigned int unknown;
@@ -107,8 +100,6 @@ static int __init setup_unknown_nmi_panic(char *str)
 }
 __setup("unknown_nmi_panic", setup_unknown_nmi_panic);
 
-#define nmi_to_desc(type) (&nmi_desc[type])
-
 static u64 nmi_longest_ns = 1 * NSEC_PER_MSEC;
 
 static int __init nmi_warning_debugfs(void)

[PATCH 4/9] x86/nmi: Remove export of local_touch_nmi()

[Sohil Mehta]

Commit feb6cd6a0f9f ("thermal/intel_powerclamp: stop sched tick in
forced idle")

got rid of the last exported user of local_touch_nmi() a while back.

Remove the unnecessary export.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/nmi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 6a5dc35522c8..cdfb3864d59a 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -745,4 +745,3 @@ void local_touch_nmi(void)
 {
 	__this_cpu_write(last_nmi_rip, 0);
 }
-EXPORT_SYMBOL_GPL(local_touch_nmi);
-- 
2.43.0

Re: [PATCH 4/9] x86/nmi: Remove export of local_touch_nmi()

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> Commit feb6cd6a0f9f ("thermal/intel_powerclamp: stop sched tick in
> forced idle")
> 
> got rid of the last exported user of local_touch_nmi() a while back.
> 
> Remove the unnecessary export.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

> ---
>  arch/x86/kernel/nmi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 6a5dc35522c8..cdfb3864d59a 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -745,4 +745,3 @@ void local_touch_nmi(void)
>  {
>  	__this_cpu_write(last_nmi_rip, 0);
>  }
> -EXPORT_SYMBOL_GPL(local_touch_nmi);

[tip: x86/nmi] x86/nmi: Remove export of local_touch_nmi()

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     6325f947014644ff39e5902afa48238af8f92e0e
Gitweb:        https://git.kernel.org/tip/6325f947014644ff39e5902afa48238af8f92e0e
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:24 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:06 +02:00

x86/nmi: Remove export of local_touch_nmi()

Commit:

  feb6cd6a0f9f ("thermal/intel_powerclamp: stop sched tick in forced idle")

got rid of the last exported user of local_touch_nmi() a while back.

Remove the unnecessary export.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-5-sohil.mehta@intel.com
---
 arch/x86/kernel/nmi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 6a5dc35..cdfb386 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -745,4 +745,3 @@ void local_touch_nmi(void)
 {
 	__this_cpu_write(last_nmi_rip, 0);
 }
-EXPORT_SYMBOL_GPL(local_touch_nmi);

[PATCH 5/9] x86/nmi: Fix comment in unknown NMI handling

[Sohil Mehta]

The comment in unknown NMI handling is incorrect and misleading. There
is no longer a restriction on having a single Unknown NMI handler. Also,
nmi_handle() does not use the 'b2b' parameter anymore.

The changes that made the comment outdated are:

commit 0d443b70cc92 ("x86/platform: Remove warning message for duplicate
NMI handlers")

commit bf9f2ee28d47 ("x86/nmi: Remove the 'b2b' parameter from
nmi_handle()")

Remove the old comment and update it to reflect the current intention.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/nmi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index cdfb3864d59a..2a07c9adc6a6 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -327,10 +327,9 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 	int handled;
 
 	/*
-	 * Use 'false' as back-to-back NMIs are dealt with one level up.
-	 * Of course this makes having multiple 'unknown' handlers useless
-	 * as only the first one is ever run (unless it can actually determine
-	 * if it caused the NMI)
+	 * As a last resort, let the "unknown" handlers make a
+	 * best-effort attempt to figure out if they can claim
+	 * responsibility for this Unknown NMI.
 	 */
 	handled = nmi_handle(NMI_UNKNOWN, regs);
 	if (handled) {
-- 
2.43.0

Re: [PATCH 5/9] x86/nmi: Fix comment in unknown NMI handling

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> The comment in unknown NMI handling is incorrect and misleading. There
> is no longer a restriction on having a single Unknown NMI handler. Also,
> nmi_handle() does not use the 'b2b' parameter anymore.
> 
> The changes that made the comment outdated are:
> 
> commit 0d443b70cc92 ("x86/platform: Remove warning message for duplicate
> NMI handlers")
> 
> commit bf9f2ee28d47 ("x86/nmi: Remove the 'b2b' parameter from
> nmi_handle()")

After some digging, IIUC, the 'b2b' parameter of the nmi_handle() was actually
never used when it was originally added in the 

  commit b227e23399dc ("x86, nmi: Add in logic to handle multiple events and
unknown NMIs")

.., so IIUC the comment was wrong when it was firstly added.  

The above commit to remove the 'b2b' seems just a fixup patch but it didn't fix
the comment.

Not sure whether it is worth to mention in the changelog.

> 
> Remove the old comment and update it to reflect the current intention.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> 

Anyway:

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 5/9] x86/nmi: Fix comment in unknown NMI handling

[Sohil Mehta]

On 3/31/2025 5:17 PM, Huang, Kai wrote:
> On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
>> The comment in unknown NMI handling is incorrect and misleading. There
>> is no longer a restriction on having a single Unknown NMI handler. Also,
>> nmi_handle() does not use the 'b2b' parameter anymore.
>>
>> The changes that made the comment outdated are:
>>
>> commit 0d443b70cc92 ("x86/platform: Remove warning message for duplicate
>> NMI handlers")
>>
>> commit bf9f2ee28d47 ("x86/nmi: Remove the 'b2b' parameter from
>> nmi_handle()")
> 
> After some digging, IIUC, the 'b2b' parameter of the nmi_handle() was actually
> never used when it was originally added in the 
> 

You are right. The b2b parameter was never really used. The fixup patch
says the same thing as well.

	"x86/nmi: Remove the 'b2b' parameter from nmi_handle()

	It has never had any effect. Remove it for comprehensibility."



>   commit b227e23399dc ("x86, nmi: Add in logic to handle multiple events and
> unknown NMIs")
> 

It's probably okay even if we don't mention this directly in the
changelog. It is indirectly implied through the above fixup patch.

> .., so IIUC the comment was wrong when it was firstly added.  
> > The above commit to remove the 'b2b' seems just a fixup patch but it
didn't fix
> the comment.
> 

Yup, that's what I meant. The fixup patch should have fixed the comment
as well.

> Not sure whether it is worth to mention in the changelog.
> 

> 
> Anyway:
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>

Thanks!

Re: [PATCH 5/9] x86/nmi: Fix comment in unknown NMI handling

[H. Peter Anvin]

On March 31, 2025 5:17:55 PM PDT, "Huang, Kai" <kai.huang@intel.com> wrote:
>On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
>> The comment in unknown NMI handling is incorrect and misleading. There
>> is no longer a restriction on having a single Unknown NMI handler. Also,
>> nmi_handle() does not use the 'b2b' parameter anymore.
>> 
>> The changes that made the comment outdated are:
>> 
>> commit 0d443b70cc92 ("x86/platform: Remove warning message for duplicate
>> NMI handlers")
>> 
>> commit bf9f2ee28d47 ("x86/nmi: Remove the 'b2b' parameter from
>> nmi_handle()")
>
>After some digging, IIUC, the 'b2b' parameter of the nmi_handle() was actually
>never used when it was originally added in the 
>
>  commit b227e23399dc ("x86, nmi: Add in logic to handle multiple events and
>unknown NMIs")
>
>.., so IIUC the comment was wrong when it was firstly added.  
>
>The above commit to remove the 'b2b' seems just a fixup patch but it didn't fix
>the comment.
>
>Not sure whether it is worth to mention in the changelog.
>
>> 
>> Remove the old comment and update it to reflect the current intention.
>> 
>> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
>> 
>
>Anyway:
>
>Reviewed-by: Kai Huang <kai.huang@intel.com>
>

Ha! Any idea what it was *supposed* to do? I'm guessing it stood for "back to back" or some such?

Re: [PATCH 5/9] x86/nmi: Fix comment in unknown NMI handling

[Sohil Mehta]

On 3/31/2025 10:45 PM, H. Peter Anvin wrote:

> Ha! Any idea what it was *supposed* to do? I'm guessing it stood for "back to back" or some such?

Yes, it stands for back to back NMIs. I added additional comments to
clarify how they are detected.
https://lore.kernel.org/lkml/20250327234629.3953536-7-sohil.mehta@intel.com/

nmi_handle() still has special handling for back to back NMIs. But at
some point, there was a b2b parameter that was passed to nmi_handle().
AFAICT, that parameter was never really used.

Sohil

[tip: x86/nmi] x86/nmi: Fix comment in unknown_nmi_error()

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     b4bc3144c1eca9107f45018000a1e68bfd308d8c
Gitweb:        https://git.kernel.org/tip/b4bc3144c1eca9107f45018000a1e68bfd308d8c
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:25 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:11 +02:00

x86/nmi: Fix comment in unknown_nmi_error()

The comment in unknown_nmi_error() is incorrect and misleading. There
is no longer a restriction on having a single Unknown NMI handler. Also,
nmi_handle() never used the 'b2b' parameter.

The commits that made the comment outdated are:

  0d443b70cc92 ("x86/platform: Remove warning message for duplicate NMI handlers")
  bf9f2ee28d47 ("x86/nmi: Remove the 'b2b' parameter from nmi_handle()")

Remove the old comment and update it to reflect the current logic.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-6-sohil.mehta@intel.com
---
 arch/x86/kernel/nmi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index cdfb386..2a07c9a 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -327,10 +327,9 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 	int handled;
 
 	/*
-	 * Use 'false' as back-to-back NMIs are dealt with one level up.
-	 * Of course this makes having multiple 'unknown' handlers useless
-	 * as only the first one is ever run (unless it can actually determine
-	 * if it caused the NMI)
+	 * As a last resort, let the "unknown" handlers make a
+	 * best-effort attempt to figure out if they can claim
+	 * responsibility for this Unknown NMI.
 	 */
 	handled = nmi_handle(NMI_UNKNOWN, regs);
 	if (handled) {

[PATCH 6/9] x86/nmi: Improve and relocate NMI handler comments

[Sohil Mehta]

Some of the comments in the default NMI handling code are out of place
or inadequate. Move them to the appropriate locations and update them as
needed.

Move the comment related to CPU-specific NMIs closer to the actual code.
Also, add more details about how back-to-back NMIs are detected since
that isn't immediately obvious.

Opportunistically, replace an #ifdef section in the vicinity with an
IS_ENABLED() check to make the code easier to read.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/nmi.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 2a07c9adc6a6..59ed74ec010e 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -359,17 +359,18 @@ static noinstr void default_do_nmi(struct pt_regs *regs)
 	bool b2b = false;
 
 	/*
-	 * CPU-specific NMI must be processed before non-CPU-specific
-	 * NMI, otherwise we may lose it, because the CPU-specific
-	 * NMI can not be detected/processed on other CPUs.
-	 */
-
-	/*
-	 * Back-to-back NMIs are interesting because they can either
-	 * be two NMI or more than two NMIs (any thing over two is dropped
-	 * due to NMI being edge-triggered).  If this is the second half
-	 * of the back-to-back NMI, assume we dropped things and process
-	 * more handlers.  Otherwise reset the 'swallow' NMI behaviour
+	 * Back-to-back NMIs are detected by comparing the RIP of the
+	 * current NMI with that of the previous NMI. If it is the same,
+	 * it is assumed that the CPU did not have a chance to jump back
+	 * into a non-NMI context and execute code in between the two
+	 * NMIs.
+	 *
+	 * They are interesting because even if there are more than two,
+	 * only a maximum of two can be detected (anything over two is
+	 * dropped due to NMI being edge-triggered). If this is the
+	 * second half of the back-to-back NMI, assume we dropped things
+	 * and process more handlers. Otherwise, reset the 'swallow' NMI
+	 * behavior.
 	 */
 	if (regs->ip == __this_cpu_read(last_nmi_rip))
 		b2b = true;
@@ -383,6 +384,11 @@ static noinstr void default_do_nmi(struct pt_regs *regs)
 	if (microcode_nmi_handler_enabled() && microcode_nmi_handler())
 		goto out;
 
+	/*
+	 * CPU-specific NMI must be processed before non-CPU-specific
+	 * NMI, otherwise we may lose it, because the CPU-specific
+	 * NMI can not be detected/processed on other CPUs.
+	 */
 	handled = nmi_handle(NMI_LOCAL, regs);
 	__this_cpu_add(nmi_stats.normal, handled);
 	if (handled) {
@@ -419,13 +425,14 @@ static noinstr void default_do_nmi(struct pt_regs *regs)
 			pci_serr_error(reason, regs);
 		else if (reason & NMI_REASON_IOCHK)
 			io_check_error(reason, regs);
-#ifdef CONFIG_X86_32
+
 		/*
 		 * Reassert NMI in case it became active
 		 * meanwhile as it's edge-triggered:
 		 */
-		reassert_nmi();
-#endif
+		if (IS_ENABLED(CONFIG_X86_32))
+			reassert_nmi();
+
 		__this_cpu_add(nmi_stats.external, 1);
 		raw_spin_unlock(&nmi_reason_lock);
 		goto out;
-- 
2.43.0

Re: [PATCH 6/9] x86/nmi: Improve and relocate NMI handler comments

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> Some of the comments in the default NMI handling code are out of place
> or inadequate. Move them to the appropriate locations and update them as
> needed.
> 
> Move the comment related to CPU-specific NMIs closer to the actual code.
> Also, add more details about how back-to-back NMIs are detected since
> that isn't immediately obvious.
> 
> Opportunistically, replace an #ifdef section in the vicinity with an
> IS_ENABLED() check to make the code easier to read.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Acked-by: Kai Huang <kai.huang@intel.com>

[tip: x86/nmi] x86/nmi: Improve and relocate NMI handler comments

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     59cddd397accfa92fc36c223e3d532b47fdab841
Gitweb:        https://git.kernel.org/tip/59cddd397accfa92fc36c223e3d532b47fdab841
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:26 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:16 +02:00

x86/nmi: Improve and relocate NMI handler comments

Some of the comments in the default NMI handling code are out of place
or inadequate. Move them to the appropriate locations and update them as
needed.

Move the comment related to CPU-specific NMIs closer to the actual code.
Also, add more details about how back-to-back NMIs are detected since
that isn't immediately obvious.

Opportunistically, replace an #ifdef section in the vicinity with an
IS_ENABLED() check to make the code easier to read.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Kai Huang <kai.huang@intel.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-7-sohil.mehta@intel.com
---
 arch/x86/kernel/nmi.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 2a07c9a..59ed74e 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -359,17 +359,18 @@ static noinstr void default_do_nmi(struct pt_regs *regs)
 	bool b2b = false;
 
 	/*
-	 * CPU-specific NMI must be processed before non-CPU-specific
-	 * NMI, otherwise we may lose it, because the CPU-specific
-	 * NMI can not be detected/processed on other CPUs.
-	 */
-
-	/*
-	 * Back-to-back NMIs are interesting because they can either
-	 * be two NMI or more than two NMIs (any thing over two is dropped
-	 * due to NMI being edge-triggered).  If this is the second half
-	 * of the back-to-back NMI, assume we dropped things and process
-	 * more handlers.  Otherwise reset the 'swallow' NMI behaviour
+	 * Back-to-back NMIs are detected by comparing the RIP of the
+	 * current NMI with that of the previous NMI. If it is the same,
+	 * it is assumed that the CPU did not have a chance to jump back
+	 * into a non-NMI context and execute code in between the two
+	 * NMIs.
+	 *
+	 * They are interesting because even if there are more than two,
+	 * only a maximum of two can be detected (anything over two is
+	 * dropped due to NMI being edge-triggered). If this is the
+	 * second half of the back-to-back NMI, assume we dropped things
+	 * and process more handlers. Otherwise, reset the 'swallow' NMI
+	 * behavior.
 	 */
 	if (regs->ip == __this_cpu_read(last_nmi_rip))
 		b2b = true;
@@ -383,6 +384,11 @@ static noinstr void default_do_nmi(struct pt_regs *regs)
 	if (microcode_nmi_handler_enabled() && microcode_nmi_handler())
 		goto out;
 
+	/*
+	 * CPU-specific NMI must be processed before non-CPU-specific
+	 * NMI, otherwise we may lose it, because the CPU-specific
+	 * NMI can not be detected/processed on other CPUs.
+	 */
 	handled = nmi_handle(NMI_LOCAL, regs);
 	__this_cpu_add(nmi_stats.normal, handled);
 	if (handled) {
@@ -419,13 +425,14 @@ static noinstr void default_do_nmi(struct pt_regs *regs)
 			pci_serr_error(reason, regs);
 		else if (reason & NMI_REASON_IOCHK)
 			io_check_error(reason, regs);
-#ifdef CONFIG_X86_32
+
 		/*
 		 * Reassert NMI in case it became active
 		 * meanwhile as it's edge-triggered:
 		 */
-		reassert_nmi();
-#endif
+		if (IS_ENABLED(CONFIG_X86_32))
+			reassert_nmi();
+
 		__this_cpu_add(nmi_stats.external, 1);
 		raw_spin_unlock(&nmi_reason_lock);
 		goto out;

[PATCH 7/9] x86/nmi: Improve NMI header documentation

[Sohil Mehta]

NMI handlers can be registered by various subsystems, including drivers.
However, the interface for registering and unregistering such handlers
is not clearly documented. In the future, the interface may need to be
extended to identify the source of the NMI.

Add documentation to make the current API more understandable and easier
to use.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/include/asm/nmi.h      | 43 ++++++++++++++++++++++++++++++++-
 arch/x86/include/asm/x86_init.h |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index f85aea7bf7f1..79d88d12c8fb 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -20,8 +20,20 @@ extern int unknown_nmi_panic;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
 
+/* NMI handler flags */
 #define NMI_FLAG_FIRST	1
 
+/**
+ * enum - NMI types.
+ * @NMI_LOCAL:    Local NMI, CPU-specific NMI generated by the Local APIC.
+ * @NMI_UNKNOWN:  Unknown NMI, the source of the NMI may not be identified.
+ * @NMI_SERR:     System Error NMI, typically triggered by PCI errors.
+ * @NMI_IO_CHECK: I/O Check NMI, related to I/O errors.
+ * @NMI_MAX:      Maximum value for NMI types.
+ *
+ * NMI types are used to categorize NMIs and to dispatch them to the
+ * appropriate handler.
+ */
 enum {
 	NMI_LOCAL=0,
 	NMI_UNKNOWN,
@@ -30,6 +42,7 @@ enum {
 	NMI_MAX
 };
 
+/* NMI handler return values */
 #define NMI_DONE	0
 #define NMI_HANDLED	1
 
@@ -43,6 +56,25 @@ struct nmiaction {
 	const char		*name;
 };
 
+/**
+ * register_nmi_handler - Register a handler for a specific NMI type
+ * @t:    NMI type (e.g. NMI_LOCAL)
+ * @fn:   The NMI handler
+ * @fg:   Flags associated with the NMI handler
+ * @n:    Name of the NMI handler
+ * @init: Optional __init* attributes for struct nmiaction
+ *
+ * Adds the provided handler to the list of handlers for the specified
+ * NMI type. Handlers flagged with NMI_FLAG_FIRST would be executed first.
+ *
+ * Sometimes the source of an NMI can't be reliably determined which
+ * results in an NMI being tagged as "unknown". Register an additional
+ * handler using the NMI type - NMI_UNKNOWN to handle such cases. The
+ * caller would get one last chance to assume responsibility for the
+ * NMI.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
 #define register_nmi_handler(t, fn, fg, n, init...)	\
 ({							\
 	static struct nmiaction init fn##_na = {	\
@@ -56,7 +88,16 @@ struct nmiaction {
 
 int __register_nmi_handler(unsigned int, struct nmiaction *);
 
-void unregister_nmi_handler(unsigned int, const char *);
+/**
+ * unregister_nmi_handler - Unregister a handler for a specific NMI type
+ * @type: NMI type (e.g. NMI_LOCAL)
+ * @name: Name of the NMI handler used during registration
+ *
+ * Removes the handler associated with the specified NMI type from the
+ * NMI handler list. The "name" is used as a lookup key to identify the
+ * handler.
+ */
+void unregister_nmi_handler(unsigned int type, const char *name);
 
 void set_emergency_nmi_handler(unsigned int type, nmi_handler_t handler);
 
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 213cf5379a5a..36698cc9fb44 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -292,6 +292,7 @@ struct x86_hyper_runtime {
  * @set_wallclock:		set time back to HW clock
  * @is_untracked_pat_range	exclude from PAT logic
  * @nmi_init			enable NMI on cpus
+ * @get_nmi_reason		get the reason an NMI was received
  * @save_sched_clock_state:	save state for sched_clock() on suspend
  * @restore_sched_clock_state:	restore state for sched_clock() on resume
  * @apic_post_init:		adjust apic if needed
-- 
2.43.0

Re: [PATCH 7/9] x86/nmi: Improve NMI header documentation

[Ingo Molnar]

* Sohil Mehta <sohil.mehta@intel.com> wrote:

> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 213cf5379a5a..36698cc9fb44 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -292,6 +292,7 @@ struct x86_hyper_runtime {
>   * @set_wallclock:		set time back to HW clock
>   * @is_untracked_pat_range	exclude from PAT logic
>   * @nmi_init			enable NMI on cpus
> + * @get_nmi_reason		get the reason an NMI was received
>   * @save_sched_clock_state:	save state for sched_clock() on suspend
>   * @restore_sched_clock_state:	restore state for sched_clock() on resume
>   * @apic_post_init:		adjust apic if needed

Note that I've split off this second half into a separate commit:

  x86/nmi: Add missing description x86_hyper_runtime::get_nmi_reason to <asm/x86_init.h>

as it's basically an independent KernelDoc bugfix AFAICS.

Thanks,

	Ingo

Re: [PATCH 7/9] x86/nmi: Improve NMI header documentation

[Sohil Mehta]

On 3/31/2025 3:47 AM, Ingo Molnar wrote:
> 
> * Sohil Mehta <sohil.mehta@intel.com> wrote:
> 
>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
>> index 213cf5379a5a..36698cc9fb44 100644
>> --- a/arch/x86/include/asm/x86_init.h
>> +++ b/arch/x86/include/asm/x86_init.h
>> @@ -292,6 +292,7 @@ struct x86_hyper_runtime {
>>   * @set_wallclock:		set time back to HW clock
>>   * @is_untracked_pat_range	exclude from PAT logic
>>   * @nmi_init			enable NMI on cpus
>> + * @get_nmi_reason		get the reason an NMI was received
>>   * @save_sched_clock_state:	save state for sched_clock() on suspend
>>   * @restore_sched_clock_state:	restore state for sched_clock() on resume
>>   * @apic_post_init:		adjust apic if needed
> 
> Note that I've split off this second half into a separate commit:
> 
>   x86/nmi: Add missing description x86_hyper_runtime::get_nmi_reason to <asm/x86_init.h>
> 
> as it's basically an independent KernelDoc bugfix AFAICS.
> 

That's fine with me. I did consider it for a moment but choose otherwise
to avoid micro-patches. Here is my sign-off if you need one.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Re: [PATCH 7/9] x86/nmi: Improve NMI header documentation

[Sohil Mehta]

On 3/31/2025 9:04 AM, Sohil Mehta wrote:
> On 3/31/2025 3:47 AM, Ingo Molnar wrote:
>>
>> * Sohil Mehta <sohil.mehta@intel.com> wrote:
>>
>>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
>>> index 213cf5379a5a..36698cc9fb44 100644
>>> --- a/arch/x86/include/asm/x86_init.h
>>> +++ b/arch/x86/include/asm/x86_init.h
>>> @@ -292,6 +292,7 @@ struct x86_hyper_runtime {
>>>   * @set_wallclock:		set time back to HW clock
>>>   * @is_untracked_pat_range	exclude from PAT logic
>>>   * @nmi_init			enable NMI on cpus
>>> + * @get_nmi_reason		get the reason an NMI was received
>>>   * @save_sched_clock_state:	save state for sched_clock() on suspend
>>>   * @restore_sched_clock_state:	restore state for sched_clock() on resume
>>>   * @apic_post_init:		adjust apic if needed
>>
>> Note that I've split off this second half into a separate commit:
>>
>>   x86/nmi: Add missing description x86_hyper_runtime::get_nmi_reason to <asm/x86_init.h>
>>

Upon closer inspection, I realized that the commit title for the new
patch is incorrect.
s/x86_hyper_runtime/x86_platform_ops

The git diff context shown above is misleading. We are modifying the
struct x86_platform_ops documentation and not struct x86_hyper_runtime.

/**
 * struct x86_platform_ops - platform specific runtime functions
 * @calibrate_cpu:		calibrate CPU
 * @calibrate_tsc:		calibrate TSC, if different from CPU
 * @get_wallclock:		get time from HW clock like RTC etc.
 * @set_wallclock:		set time back to HW clock
 * @is_untracked_pat_range	exclude from PAT logic
 * @nmi_init			enable NMI on cpus
 * @get_nmi_reason		get the reason an NMI was received
 * @save_sched_clock_state:	save state for sched_clock() on suspend
 * @restore_sched_clock_state:	restore state for sched_clock() on resu

>> as it's basically an independent KernelDoc bugfix AFAICS.
>>
> 
> That's fine with me. I did consider it for a moment but choose otherwise
> to avoid micro-patches. Here is my sign-off if you need one.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Sohil

Re: [PATCH 7/9] x86/nmi: Improve NMI header documentation

[Ingo Molnar]

* Sohil Mehta <sohil.mehta@intel.com> wrote:

> On 3/31/2025 9:04 AM, Sohil Mehta wrote:
> > On 3/31/2025 3:47 AM, Ingo Molnar wrote:
> >>
> >> * Sohil Mehta <sohil.mehta@intel.com> wrote:
> >>
> >>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> >>> index 213cf5379a5a..36698cc9fb44 100644
> >>> --- a/arch/x86/include/asm/x86_init.h
> >>> +++ b/arch/x86/include/asm/x86_init.h
> >>> @@ -292,6 +292,7 @@ struct x86_hyper_runtime {
> >>>   * @set_wallclock:		set time back to HW clock
> >>>   * @is_untracked_pat_range	exclude from PAT logic
> >>>   * @nmi_init			enable NMI on cpus
> >>> + * @get_nmi_reason		get the reason an NMI was received
> >>>   * @save_sched_clock_state:	save state for sched_clock() on suspend
> >>>   * @restore_sched_clock_state:	restore state for sched_clock() on resume
> >>>   * @apic_post_init:		adjust apic if needed
> >>
> >> Note that I've split off this second half into a separate commit:
> >>
> >>   x86/nmi: Add missing description x86_hyper_runtime::get_nmi_reason to <asm/x86_init.h>
> >>
> 
> Upon closer inspection, I realized that the commit title for the new
> patch is incorrect.
> s/x86_hyper_runtime/x86_platform_ops

Yeah, the git patch context threw me off: I fixed the title, have 
included the other review feedback, and added the Reviewed-by tags of 
Kai Huang as well.

Thanks!

	Ingo

Re: [PATCH 7/9] x86/nmi: Improve NMI header documentation

[Sohil Mehta]

On 4/1/2025 1:08 AM, Ingo Molnar wrote:

>> Upon closer inspection, I realized that the commit title for the new
>> patch is incorrect.
>> s/x86_hyper_runtime/x86_platform_ops
> 
> Yeah, the git patch context threw me off: I fixed the title, have 
> included the other review feedback, and added the Reviewed-by tags of 
> Kai Huang as well.
> 

Great, that's awesome! We now have ACKs from PeterZ and Nikolay as well.
Thank you for the reviews.

I haven't seen the tip-bot emails yet. Just out of curiosity, when do
the tip-bot emails get sent? Does it happen automatically when you push
the patches to upstream tip:x86/nmi or do you trigger them manually at a
later point?

Thanks,
Sohil

[tip: x86/nmi] x86/nmi: Add missing description x86_platform_ops::get_nmi_reason to <asm/x86_init.h>

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     7324d7de7740fdbb29c79f5e2a001524370fa732
Gitweb:        https://git.kernel.org/tip/7324d7de7740fdbb29c79f5e2a001524370fa732
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:27 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:27 +02:00

x86/nmi: Add missing description x86_platform_ops::get_nmi_reason to <asm/x86_init.h>

[ mingo: Split off from another patch. ]

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-8-sohil.mehta@intel.com
---
 arch/x86/include/asm/x86_init.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 213cf53..36698cc 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -292,6 +292,7 @@ struct x86_hyper_runtime {
  * @set_wallclock:		set time back to HW clock
  * @is_untracked_pat_range	exclude from PAT logic
  * @nmi_init			enable NMI on cpus
+ * @get_nmi_reason		get the reason an NMI was received
  * @save_sched_clock_state:	save state for sched_clock() on suspend
  * @restore_sched_clock_state:	restore state for sched_clock() on resume
  * @apic_post_init:		adjust apic if needed

[tip: x86/nmi] x86/nmi: Improve <asm/nmi.h> documentation

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     3b1292706305e5949d2aa739eb0ae009a9e6f824
Gitweb:        https://git.kernel.org/tip/3b1292706305e5949d2aa739eb0ae009a9e6f824
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:27 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:21 +02:00

x86/nmi: Improve <asm/nmi.h> documentation

NMI handlers can be registered by various subsystems, including drivers.

However, the interface for registering and unregistering such handlers
is not clearly documented. In the future, the interface may need to be
extended to identify the source of the NMI.

Add documentation to make the current API more understandable and easier
to use.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-8-sohil.mehta@intel.com
---
 arch/x86/include/asm/nmi.h | 43 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index f85aea7..79d88d1 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -20,8 +20,20 @@ extern int unknown_nmi_panic;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
 
+/* NMI handler flags */
 #define NMI_FLAG_FIRST	1
 
+/**
+ * enum - NMI types.
+ * @NMI_LOCAL:    Local NMI, CPU-specific NMI generated by the Local APIC.
+ * @NMI_UNKNOWN:  Unknown NMI, the source of the NMI may not be identified.
+ * @NMI_SERR:     System Error NMI, typically triggered by PCI errors.
+ * @NMI_IO_CHECK: I/O Check NMI, related to I/O errors.
+ * @NMI_MAX:      Maximum value for NMI types.
+ *
+ * NMI types are used to categorize NMIs and to dispatch them to the
+ * appropriate handler.
+ */
 enum {
 	NMI_LOCAL=0,
 	NMI_UNKNOWN,
@@ -30,6 +42,7 @@ enum {
 	NMI_MAX
 };
 
+/* NMI handler return values */
 #define NMI_DONE	0
 #define NMI_HANDLED	1
 
@@ -43,6 +56,25 @@ struct nmiaction {
 	const char		*name;
 };
 
+/**
+ * register_nmi_handler - Register a handler for a specific NMI type
+ * @t:    NMI type (e.g. NMI_LOCAL)
+ * @fn:   The NMI handler
+ * @fg:   Flags associated with the NMI handler
+ * @n:    Name of the NMI handler
+ * @init: Optional __init* attributes for struct nmiaction
+ *
+ * Adds the provided handler to the list of handlers for the specified
+ * NMI type. Handlers flagged with NMI_FLAG_FIRST would be executed first.
+ *
+ * Sometimes the source of an NMI can't be reliably determined which
+ * results in an NMI being tagged as "unknown". Register an additional
+ * handler using the NMI type - NMI_UNKNOWN to handle such cases. The
+ * caller would get one last chance to assume responsibility for the
+ * NMI.
+ *
+ * Return: 0 on success, or an error code on failure.
+ */
 #define register_nmi_handler(t, fn, fg, n, init...)	\
 ({							\
 	static struct nmiaction init fn##_na = {	\
@@ -56,7 +88,16 @@ struct nmiaction {
 
 int __register_nmi_handler(unsigned int, struct nmiaction *);
 
-void unregister_nmi_handler(unsigned int, const char *);
+/**
+ * unregister_nmi_handler - Unregister a handler for a specific NMI type
+ * @type: NMI type (e.g. NMI_LOCAL)
+ * @name: Name of the NMI handler used during registration
+ *
+ * Removes the handler associated with the specified NMI type from the
+ * NMI handler list. The "name" is used as a lookup key to identify the
+ * handler.
+ */
+void unregister_nmi_handler(unsigned int type, const char *name);
 
 void set_emergency_nmi_handler(unsigned int type, nmi_handler_t handler);
 

[PATCH 8/9] x86/nmi: Clean up NMI selftest

[Sohil Mehta]

The expected_testcase_failures variable in the NMI selftest has never
been set since its introduction. Remove this unused variable along with
the related checks to simplify the code.

While at it, replace printk() with the corresponding pr_{cont,info}()
calls. Also, get rid of the superfluous testname wrapper and the
redundant file path comment.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/nmi_selftest.c | 52 +++++++++++-----------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index e93a8545c74d..a010e9d062bf 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -1,7 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * arch/x86/kernel/nmi-selftest.c
- *
  * Testsuite for NMI: IPIs
  *
  * Started by Don Zickus:
@@ -30,7 +28,6 @@ static DECLARE_BITMAP(nmi_ipi_mask, NR_CPUS) __initdata;
 
 static int __initdata testcase_total;
 static int __initdata testcase_successes;
-static int __initdata expected_testcase_failures;
 static int __initdata unexpected_testcase_failures;
 static int __initdata unexpected_testcase_unknowns;
 
@@ -120,26 +117,22 @@ static void __init dotest(void (*testcase_fn)(void), int expected)
 		unexpected_testcase_failures++;
 
 		if (nmi_fail == FAILURE)
-			printk(KERN_CONT "FAILED |");
+			pr_cont("FAILED |");
 		else if (nmi_fail == TIMEOUT)
-			printk(KERN_CONT "TIMEOUT|");
+			pr_cont("TIMEOUT|");
 		else
-			printk(KERN_CONT "ERROR  |");
+			pr_cont("ERROR  |");
 		dump_stack();
 	} else {
 		testcase_successes++;
-		printk(KERN_CONT "  ok  |");
+		pr_cont("  ok  |");
 	}
-	testcase_total++;
+	pr_cont("\n");
 
+	testcase_total++;
 	reset_nmi();
 }
 
-static inline void __init print_testname(const char *testname)
-{
-	printk("%12s:", testname);
-}
-
 void __init nmi_selftest(void)
 {
 	init_nmi_testsuite();
@@ -147,38 +140,25 @@ void __init nmi_selftest(void)
         /*
 	 * Run the testsuite:
 	 */
-	printk("----------------\n");
-	printk("| NMI testsuite:\n");
-	printk("--------------------\n");
+	pr_info("----------------\n");
+	pr_info("| NMI testsuite:\n");
+	pr_info("--------------------\n");
 
-	print_testname("remote IPI");
+	pr_info("%12s:", "remote IPI");
 	dotest(remote_ipi, SUCCESS);
-	printk(KERN_CONT "\n");
-	print_testname("local IPI");
+
+	pr_info("%12s:", "local IPI");
 	dotest(local_ipi, SUCCESS);
-	printk(KERN_CONT "\n");
 
 	cleanup_nmi_testsuite();
 
+	pr_info("--------------------\n");
 	if (unexpected_testcase_failures) {
-		printk("--------------------\n");
-		printk("BUG: %3d unexpected failures (out of %3d) - debugging disabled! |\n",
+		pr_info("BUG: %3d unexpected failures (out of %3d) - debugging disabled! |\n",
 			unexpected_testcase_failures, testcase_total);
-		printk("-----------------------------------------------------------------\n");
-	} else if (expected_testcase_failures && testcase_successes) {
-		printk("--------------------\n");
-		printk("%3d out of %3d testcases failed, as expected. |\n",
-			expected_testcase_failures, testcase_total);
-		printk("----------------------------------------------------\n");
-	} else if (expected_testcase_failures && !testcase_successes) {
-		printk("--------------------\n");
-		printk("All %3d testcases failed, as expected. |\n",
-			expected_testcase_failures);
-		printk("----------------------------------------\n");
 	} else {
-		printk("--------------------\n");
-		printk("Good, all %3d testcases passed! |\n",
+		pr_info("Good, all %3d testcases passed! |\n",
 			testcase_successes);
-		printk("---------------------------------\n");
 	}
+	pr_info("-----------------------------------------------------------------\n");
 }
-- 
2.43.0

[tip: x86/nmi] x86/nmi: Clean up NMI selftest

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     05279a2863ddba6ed993aad655b9f3e57d9a94ce
Gitweb:        https://git.kernel.org/tip/05279a2863ddba6ed993aad655b9f3e57d9a94ce
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:28 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:32 +02:00

x86/nmi: Clean up NMI selftest

The expected_testcase_failures variable in the NMI selftest has never
been set since its introduction. Remove this unused variable along with
the related checks to simplify the code.

While at it, replace printk() with the corresponding pr_{cont,info}()
calls. Also, get rid of the superfluous testname wrapper and the
redundant file path comment.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-9-sohil.mehta@intel.com
---
 arch/x86/kernel/nmi_selftest.c | 52 ++++++++++-----------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index e93a854..a010e9d 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -1,7 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * arch/x86/kernel/nmi-selftest.c
- *
  * Testsuite for NMI: IPIs
  *
  * Started by Don Zickus:
@@ -30,7 +28,6 @@ static DECLARE_BITMAP(nmi_ipi_mask, NR_CPUS) __initdata;
 
 static int __initdata testcase_total;
 static int __initdata testcase_successes;
-static int __initdata expected_testcase_failures;
 static int __initdata unexpected_testcase_failures;
 static int __initdata unexpected_testcase_unknowns;
 
@@ -120,26 +117,22 @@ static void __init dotest(void (*testcase_fn)(void), int expected)
 		unexpected_testcase_failures++;
 
 		if (nmi_fail == FAILURE)
-			printk(KERN_CONT "FAILED |");
+			pr_cont("FAILED |");
 		else if (nmi_fail == TIMEOUT)
-			printk(KERN_CONT "TIMEOUT|");
+			pr_cont("TIMEOUT|");
 		else
-			printk(KERN_CONT "ERROR  |");
+			pr_cont("ERROR  |");
 		dump_stack();
 	} else {
 		testcase_successes++;
-		printk(KERN_CONT "  ok  |");
+		pr_cont("  ok  |");
 	}
-	testcase_total++;
+	pr_cont("\n");
 
+	testcase_total++;
 	reset_nmi();
 }
 
-static inline void __init print_testname(const char *testname)
-{
-	printk("%12s:", testname);
-}
-
 void __init nmi_selftest(void)
 {
 	init_nmi_testsuite();
@@ -147,38 +140,25 @@ void __init nmi_selftest(void)
         /*
 	 * Run the testsuite:
 	 */
-	printk("----------------\n");
-	printk("| NMI testsuite:\n");
-	printk("--------------------\n");
+	pr_info("----------------\n");
+	pr_info("| NMI testsuite:\n");
+	pr_info("--------------------\n");
 
-	print_testname("remote IPI");
+	pr_info("%12s:", "remote IPI");
 	dotest(remote_ipi, SUCCESS);
-	printk(KERN_CONT "\n");
-	print_testname("local IPI");
+
+	pr_info("%12s:", "local IPI");
 	dotest(local_ipi, SUCCESS);
-	printk(KERN_CONT "\n");
 
 	cleanup_nmi_testsuite();
 
+	pr_info("--------------------\n");
 	if (unexpected_testcase_failures) {
-		printk("--------------------\n");
-		printk("BUG: %3d unexpected failures (out of %3d) - debugging disabled! |\n",
+		pr_info("BUG: %3d unexpected failures (out of %3d) - debugging disabled! |\n",
 			unexpected_testcase_failures, testcase_total);
-		printk("-----------------------------------------------------------------\n");
-	} else if (expected_testcase_failures && testcase_successes) {
-		printk("--------------------\n");
-		printk("%3d out of %3d testcases failed, as expected. |\n",
-			expected_testcase_failures, testcase_total);
-		printk("----------------------------------------------------\n");
-	} else if (expected_testcase_failures && !testcase_successes) {
-		printk("--------------------\n");
-		printk("All %3d testcases failed, as expected. |\n",
-			expected_testcase_failures);
-		printk("----------------------------------------\n");
 	} else {
-		printk("--------------------\n");
-		printk("Good, all %3d testcases passed! |\n",
+		pr_info("Good, all %3d testcases passed! |\n",
 			testcase_successes);
-		printk("---------------------------------\n");
 	}
+	pr_info("-----------------------------------------------------------------\n");
 }

[PATCH 9/9] x86/nmi: Improve NMI duration console print

[Sohil Mehta]

Convert the last remaining printk() in nmi.c to pr_info(). Along with
it, use timespec macros to calculate the NMI handler duration.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/kernel/nmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 59ed74ec010e..be93ec7255bf 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -119,12 +119,12 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration)
 
 	action->max_duration = duration;
 
-	remainder_ns = do_div(duration, (1000 * 1000));
-	decimal_msecs = remainder_ns / 1000;
+	/* Convert duration from nsec to msec */
+	remainder_ns = do_div(duration, NSEC_PER_MSEC);
+	decimal_msecs = remainder_ns / NSEC_PER_USEC;
 
-	printk_ratelimited(KERN_INFO
-		"INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n",
-		action->handler, duration, decimal_msecs);
+	pr_info_ratelimited("INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n",
+			    action->handler, duration, decimal_msecs);
 }
 
 static int nmi_handle(unsigned int type, struct pt_regs *regs)
-- 
2.43.0

Re: [PATCH 9/9] x86/nmi: Improve NMI duration console print

[Huang, Kai]

On Thu, 2025-03-27 at 23:46 +0000, Mehta, Sohil wrote:
> Convert the last remaining printk() in nmi.c to pr_info(). Along with
> it, use timespec macros to calculate the NMI handler duration.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

I eventually figured out the reasons to use NSEC_PER_MSEC and NSEC_PER_USEC:

Reviewed-by: Kai Huang <kai.huang@intel.com>

> ---
>  arch/x86/kernel/nmi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 59ed74ec010e..be93ec7255bf 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -119,12 +119,12 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration)
>  
>  	action->max_duration = duration;
>  
> -	remainder_ns = do_div(duration, (1000 * 1000));
> -	decimal_msecs = remainder_ns / 1000;
> +	/* Convert duration from nsec to msec */
> +	remainder_ns = do_div(duration, NSEC_PER_MSEC);
> +	decimal_msecs = remainder_ns / NSEC_PER_USEC;
>  
> -	printk_ratelimited(KERN_INFO
> -		"INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n",
> -		action->handler, duration, decimal_msecs);
> +	pr_info_ratelimited("INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n",
> +			    action->handler, duration, decimal_msecs);
>  }
>  
>  static int nmi_handle(unsigned int type, struct pt_regs *regs)

[tip: x86/nmi] x86/nmi: Improve NMI duration console printouts

[tip-bot2 for Sohil Mehta]

The following commit has been merged into the x86/nmi branch of tip:

Commit-ID:     f2e01dcf6df2d12e86c363ea9c37d53994d89dd6
Gitweb:        https://git.kernel.org/tip/f2e01dcf6df2d12e86c363ea9c37d53994d89dd6
Author:        Sohil Mehta <sohil.mehta@intel.com>
AuthorDate:    Thu, 27 Mar 2025 23:46:29 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 01 Apr 2025 22:26:38 +02:00

x86/nmi: Improve NMI duration console printouts

Convert the last remaining printk() in nmi.c to pr_info(). Along with
it, use timespec macros to calculate the NMI handler duration.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Link: https://lore.kernel.org/r/20250327234629.3953536-10-sohil.mehta@intel.com
---
 arch/x86/kernel/nmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 59ed74e..be93ec7 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -119,12 +119,12 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration)
 
 	action->max_duration = duration;
 
-	remainder_ns = do_div(duration, (1000 * 1000));
-	decimal_msecs = remainder_ns / 1000;
+	/* Convert duration from nsec to msec */
+	remainder_ns = do_div(duration, NSEC_PER_MSEC);
+	decimal_msecs = remainder_ns / NSEC_PER_USEC;
 
-	printk_ratelimited(KERN_INFO
-		"INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n",
-		action->handler, duration, decimal_msecs);
+	pr_info_ratelimited("INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n",
+			    action->handler, duration, decimal_msecs);
 }
 
 static int nmi_handle(unsigned int type, struct pt_regs *regs)

Re: [PATCH 0/9] x86: Cleanup NMI handling

[Peter Zijlstra]

On Thu, Mar 27, 2025 at 11:46:20PM +0000, Sohil Mehta wrote:

> Sohil Mehta (9):
>   x86/nmi: Simplify unknown NMI panic handling
>   x86/nmi: Consolidate NMI panic variables
>   x86/nmi: Use a macro to initialize NMI descriptors
>   x86/nmi: Remove export of local_touch_nmi()
>   x86/nmi: Fix comment in unknown NMI handling
>   x86/nmi: Improve and relocate NMI handler comments
>   x86/nmi: Improve NMI header documentation
>   x86/nmi: Clean up NMI selftest
>   x86/nmi: Improve NMI duration console print
> 
>  arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
>  arch/x86/include/asm/x86_init.h |  1 +
>  arch/x86/kernel/dumpstack.c     |  2 -
>  arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
>  arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
>  arch/x86/kernel/setup.c         | 37 ++++++--------
>  include/linux/panic.h           |  2 -
>  7 files changed, 122 insertions(+), 108 deletions(-)

These seem sane,

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH 0/9] x86: Cleanup NMI handling

[Nikolay Borisov]

On 28.03.25 г. 1:46 ч., Sohil Mehta wrote:
> While reworking the NMI-source patches[1], I spent quite a few days
> understanding the existing NMI handling code. The intention of this series is
> to fix the inconsistencies and redundancies that I encountered.
> 
> This series also includes improved documentation to make the code easier to
> follow. It does not introduce any significant functional changes.
> 
> I have tried to split the patches logically to make them easier to review. Let
> me know if some of them should be combined. The patches are in no particular
> order and can be picked up independently.
> 
> The documentation updates in patches (5, 6, 7) are to the best of my ability.
> However, I would really appreciate extra eyes to ensure it is precise.
> 
> The updated NMI-source patches will follow this series sometime later.
> 
> @Ingo, I tried to format the commit references the way you prefer:
> 
>    Commit:
>      feeaf5512947 ("x86: Move sysctls into arch/x86")
> 
> However, checkpatch seems to dislike it, so I reformatted them as below:
> 
>    Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
> 
> Is there a specific guideline for x86?
> 
> [1]: https://lore.kernel.org/lkml/20240709143906.1040477-1-jacob.jun.pan@linux.intel.com/
> 
> Sohil Mehta (9):
>    x86/nmi: Simplify unknown NMI panic handling
>    x86/nmi: Consolidate NMI panic variables
>    x86/nmi: Use a macro to initialize NMI descriptors
>    x86/nmi: Remove export of local_touch_nmi()
>    x86/nmi: Fix comment in unknown NMI handling
>    x86/nmi: Improve and relocate NMI handler comments
>    x86/nmi: Improve NMI header documentation
>    x86/nmi: Clean up NMI selftest
>    x86/nmi: Improve NMI duration console print
> 
>   arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
>   arch/x86/include/asm/x86_init.h |  1 +
>   arch/x86/kernel/dumpstack.c     |  2 -
>   arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
>   arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
>   arch/x86/kernel/setup.c         | 37 ++++++--------
>   include/linux/panic.h           |  2 -
>   7 files changed, 122 insertions(+), 108 deletions(-)
> 

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

Re: [PATCH 0/9] x86: Cleanup NMI handling

[H. Peter Anvin]

On March 27, 2025 4:46:20 PM PDT, Sohil Mehta <sohil.mehta@intel.com> wrote:
>While reworking the NMI-source patches[1], I spent quite a few days
>understanding the existing NMI handling code. The intention of this series is
>to fix the inconsistencies and redundancies that I encountered.
>
>This series also includes improved documentation to make the code easier to
>follow. It does not introduce any significant functional changes.
>
>I have tried to split the patches logically to make them easier to review. Let
>me know if some of them should be combined. The patches are in no particular
>order and can be picked up independently. 
>
>The documentation updates in patches (5, 6, 7) are to the best of my ability.
>However, I would really appreciate extra eyes to ensure it is precise.
>
>The updated NMI-source patches will follow this series sometime later.
>
>@Ingo, I tried to format the commit references the way you prefer:
>
>  Commit:
>    feeaf5512947 ("x86: Move sysctls into arch/x86")
>
>However, checkpatch seems to dislike it, so I reformatted them as below:
>
>  Commit feeaf5512947 ("x86: Move sysctls into arch/x86")
>
>Is there a specific guideline for x86?
>
>[1]: https://lore.kernel.org/lkml/20240709143906.1040477-1-jacob.jun.pan@linux.intel.com/
>
>Sohil Mehta (9):
>  x86/nmi: Simplify unknown NMI panic handling
>  x86/nmi: Consolidate NMI panic variables
>  x86/nmi: Use a macro to initialize NMI descriptors
>  x86/nmi: Remove export of local_touch_nmi()
>  x86/nmi: Fix comment in unknown NMI handling
>  x86/nmi: Improve and relocate NMI handler comments
>  x86/nmi: Improve NMI header documentation
>  x86/nmi: Clean up NMI selftest
>  x86/nmi: Improve NMI duration console print
>
> arch/x86/include/asm/nmi.h      | 49 +++++++++++++++++--
> arch/x86/include/asm/x86_init.h |  1 +
> arch/x86/kernel/dumpstack.c     |  2 -
> arch/x86/kernel/nmi.c           | 87 ++++++++++++++++-----------------
> arch/x86/kernel/nmi_selftest.c  | 52 ++++++--------------
> arch/x86/kernel/setup.c         | 37 ++++++--------
> include/linux/panic.h           |  2 -
> 7 files changed, 122 insertions(+), 108 deletions(-)
>

Talking about NMI, one thing I would like to see done would be to explicitly enable NMI at the point where we are ready to take them. 

If we enter the kernel with NMIs disabled, using IDT they will get spuriously reenabled the first time we do IRET, but using FRED they could stay disabled until we enter user space.

It also seems "cleaner".

We ought to already be doing something with the RTC NMI gate register, so that ought to be in the same logical spot, too.