Re: [PATCH v6 4/9] x86/nmi: Assign and register NMI-source vectors

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

On 6/3/2025 10:23 AM, Dave Hansen wrote:
> On 5/13/25 13:37, Sohil Mehta wrote:
> ...
>> + * Vector 2 is reserved for external NMIs related to the Local APIC -
>> + * LINT1. Some third-party chipsets may send NMI messages with a
>> + * hardcoded vector of 2, which would result in bit 2 being set in the
>> + * NMI-source bitmap.
> 
> This doesn't actually say what problem this causes. Is this better?
> 
> 	Third-party chipsets send NMI messages with a fixed vector of 2.
> 	Using vector 2 for some other purpose would cause confusion
> 	between those Local APIC messages and the other purpose. Avoid
> 	using it.
> 

Yes, that is better. Though, the behavior of these external NMIs isn't
expected to consistent across all third-parties.

Third-party chipsets may send NMI messages with a fixed vector of 2.
Using vector 2 for some other purpose would cause confusion between
those external NMI messages and the other purpose. Avoid using it.


>> + * The vectors are in no particular priority order. Add new vector
>> + * assignments sequentially in the list below.
>> + */
>> +#define NMIS_VECTOR_NONE	0	/* Reserved - Set for all unidentified sources */
>> +#define NMIS_VECTOR_TEST	1	/* NMI selftest */
>> +#define NMIS_VECTOR_EXTERNAL	2	/* Reserved - Match External NMI vector 2 */
>> +#define NMIS_VECTOR_SMP_STOP	3	/* Panic stop CPU */
>> +#define NMIS_VECTOR_BT		4	/* CPU backtrace */
>> +#define NMIS_VECTOR_KGDB	5	/* Kernel debugger */
>> +#define NMIS_VECTOR_MCE		6	/* MCE injection */
>> +#define NMIS_VECTOR_PMI		7	/* PerfMon counters */
>> +
>> +#define NMIS_VECTORS_MAX	16	/* Maximum number of NMI-source vectors */
> 
> Would an enum fit here?
> 

I had experimented using an enum, but I avoided that approach because
the source bitmap would also be visible to users/developers. For
example, patch 9 includes it in the "unknown NMI" print. I am planning
to add it to trace_nmi_handler() as well.

With an enum, it's harder to figure out the exact sources when let's say
the source bitmap is printed as 0x0090.

enum nmi_source_vector {
    NMIS_VECTOR_NONE,
    NMIS_VECTOR_TEST,
    NMIS_VECTOR_EXTERNAL,
    NMIS_VECTOR_SMP_STOP,
    NMIS_VECTOR_BT,
    NMIS_VECTOR_KGDB,
    NMIS_VECTOR_MCE,
    NMIS_VECTOR_PMI,
    ...,
    NMIS_VECTOR_COUNT
};

Users would have to convert the enum back to numbers to make sense of
the bitmap. It isn't bad but feels inconvenient.

> You could also add a:
> 
> 	NMIS_VECTOR_COUNT
> 
> as the last entry and then just:
> 
> 	BUILD_BUG_ON(NMIS_VECTOR_COUNT >= 16);
> 
> somewhere.
> 
> I guess it's a little annoying that you need NMIS_VECTOR_EXTERNAL to
> have a fixed value of 2, but I do like way the enum makes the type explicit.
> 

Yeah, fixing the external vector makes the enum annoying to use.

We could probably define an enum in this unusual way to keep the table
consistent and help users quickly refer bit offsets. But I am not sure
if this is any better than the current macros.

enum nmi_source_vector {
    NMIS_VECTOR_NONE        = 0,
    NMIS_VECTOR_TEST        = 1,
    NMIS_VECTOR_EXTERNAL    = 2,
    NMIS_VECTOR_SMP_STOP    = 3,
    NMIS_VECTOR_BT          = 4,
    NMIS_VECTOR_KGDB        = 5,
    NMIS_VECTOR_MCE         = 6,
    NMIS_VECTOR_PMI         = 7
};

Any suggestions?

> 
>>  static int __init register_nmi_cpu_backtrace_handler(void)
>>  {
>> -	register_nmi_handler(NMI_LOCAL, nmi_cpu_backtrace_handler, 0, "arch_bt", 0);
>> +	register_nmi_handler(NMI_LOCAL, nmi_cpu_backtrace_handler, 0, "arch_bt", NMIS_VECTOR_BT);
>>  	return 0;
>>  }
> 
> ... Oh you replaced _most_ of the random 0's in this patch. That helps
> for sure.

There are still quite a few places left with an extra 0 at the end.
Explicitly using NMIS_VECTOR_NONE for them should be useful.


> ret = register_nmi_handler(NMI_LOCAL, perf_ibs_nmi_handler, 0, "perf_ibs", 0);
> register_nmi_handler(NMI_UNKNOWN, hv_nmi_unknown, NMI_FLAG_FIRST, "hv_nmi_unknown", 0);
> retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler, 0, "kgdb", 0);
> if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv", 0))
> if (register_nmi_handler(NMI_LOCAL, uv_handle_nmi_ping, 0, "uvping", 0))
> register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes", 0);
> rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0, "ipmi", 0);
> rc = register_nmi_handler(NMI_SERR, ecclog_nmi_handler, 0, IGEN6_NMI_NAME, 0);
> retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, 0, "hpwdt", 0);
> retval = register_nmi_handler(NMI_SERR, hpwdt_pretimeout, 0, "hpwdt", 0);
> retval = register_nmi_handler(NMI_IO_CHECK, hpwdt_pretimeout, 0, "hpwdt", 0);