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

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

On 5/7/2025 1:22 AM, Peter Zijlstra wrote:
> On Tue, May 06, 2025 at 06:21:40PM -0700, Sohil Mehta wrote:

>> + */
>> +#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 */
> 
> Are these really independent NMI vectors, or simply NMI source reporting bits?
> 
> Because if they are not NMI vectors, naming them such is confusing.
> 
> Specifically, is there a latch per source?
> 

Yes, they are truly vectors, confirmed with HPA that there is one latch
per source. Also, while generating the NMIs these values are used in the
APIC code to program the ICR vector field as shown.

ICR[7:0]  — Vector         -> NMIS_VECTOR_BT (4)
ICR[10:8] — Delivery Mode  -> APIC_DM_NMI    (100)

>> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
>> index be93ec7255bf..a1d672dcb6f0 100644
>> --- a/arch/x86/kernel/nmi.c
>> +++ b/arch/x86/kernel/nmi.c
>> @@ -184,6 +184,11 @@ int __register_nmi_handler(unsigned int type, struct nmiaction *action)
>>  
>>  	raw_spin_lock_irqsave(&desc->lock, flags);
>>  
>> +	WARN_ON_ONCE(action->source_vector >= NMIS_VECTORS_MAX);
>> +
>> +	if (!cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) || type != NMI_LOCAL)
>> +		action->source_vector = 0;
> 
> How about:
> 
> 	WARN_ON_ONCE(type != NMI_LOCAL && action->source_vector);
> 

This should work fine as well.

I don't see any harm in storing the source_vector unconditionally even
if X86_FEATURE_NMI_SOURCE is disabled.