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

[Nikolay Borisov <nik.borisov@suse.com>]

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",