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

[Sohil Mehta <sohil.mehta@intel.com>]

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