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

Sohil Mehta posted 9 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH v5 4/9] x86/nmi: Assign and register NMI-source vectors
Posted by Sohil Mehta 7 months, 1 week ago
Prior to NMI-source support, the vector information was ignored by the
hardware while delivering NMIs. With NMI-source, the architecture
currently supports a 16-bit source bitmap to identify the source of the
NMI. Upon receiving an NMI, this bitmap is delivered as part of the FRED
event delivery mechanism to the kernel.

Assign a vector space of 0-15 that is specific to NMI-source and
independent of the IDT vector space of 0-255. Being a bitmap, the
NMI-source vectors do not have any inherent priority associated with
them. The order of executing the NMI handlers is up to the kernel.

Existing NMI handling already has a priority mechanism for the NMI
handlers, with CPU-specific (NMI_LOCAL) handlers executed first,
followed by platform NMI handlers and unknown NMI (NMI_UNKNOWN) handlers
being last. Within each of these NMI types, the handlers registered with
NMI_FLAG_FIRST are given priority.

NMI-source follows the same priority scheme to avoid unnecessary
complexity. Therefore, the NMI-source vectors are assigned arbitrarily,
except for vectors 0 and 2.

Vector 0 is set by the hardware whenever a source vector was not used
while generating an NMI or the originator could not be reliably
identified. Do not assign it to any handler.

Vector 2 is reserved for external NMIs corresponding to Local APIC -
LINT1. Some third-party chipsets may send NMI messages with a hardcoded
vector of 2, which would result in vector 2 being set in the NMI-source
bitmap. To avoid confusion, do not assign vector 2 to any handler.

NMI-source vectors are only assigned for NMI_LOCAL type handlers.
Platform NMI handlers have a single handler registered per type. They
don't need additional source information to differentiate among them.

Use the assigned vectors to register the respective NMI handlers. A
couple of NMI handlers, such as the microcode rendezvous and the crash
reboot, do not use the typical NMI registration interface. Leave them
as-is for now.

Originally-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v5: Move the vector defines to nmi.h.
    Combine vector allocation and registration into one patch.
    Simplify NMI vector names.
    Describe usage of vector 2 for external NMIs.
    Get rid of vector priorities.
---
 arch/x86/events/core.c           |  2 +-
 arch/x86/include/asm/nmi.h       | 32 ++++++++++++++++++++++++++++++++
 arch/x86/kernel/apic/hw_nmi.c    |  2 +-
 arch/x86/kernel/cpu/mce/inject.c |  2 +-
 arch/x86/kernel/kgdb.c           |  2 +-
 arch/x86/kernel/nmi.c            |  5 +++++
 arch/x86/kernel/nmi_selftest.c   |  2 +-
 arch/x86/kernel/smp.c            |  2 +-
 8 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index b84b8be1f075..031e908f0d61 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2115,7 +2115,7 @@ static int __init init_hw_perf_events(void)
 		x86_pmu.config_mask = X86_RAW_EVENT_MASK;
 
 	perf_events_lapic_init();
-	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI", 0);
+	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI", NMIS_VECTOR_PMI);
 
 	unconstrained = (struct event_constraint)
 		__EVENT_CONSTRAINT(0, x86_pmu.cntr_mask64,
diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
index f0a577bf7bba..b9beb216f2d0 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -57,6 +57,38 @@ struct nmiaction {
 	u8			source_vector;
 };
 
+/**
+ * NMI-source vectors are used to identify the origin of an NMI and to
+ * route the NMI directly to the appropriate handler.
+ *
+ * On CPUs that support NMI-source reporting with FRED, receiving an NMI
+ * with a valid vector sets the corresponding bit in the NMI-source
+ * bitmap. The bitmap is delivered as FRED event data on the stack.
+ * Multiple NMIs are coalesced in the NMI-source bitmap until the next
+ * NMI delivery.
+ *
+ * If an NMI is received without a vector or beyond the defined range,
+ * the CPU sets bit 0 of the NMI-source bitmap.
+ *
+ * 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.
+ *
+ * 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 */
+
 /**
  * register_nmi_handler - Register a handler for a specific NMI type
  * @t:    NMI type (e.g. NMI_LOCAL)
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index 612b77660d05..4e04f13d2de9 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -53,7 +53,7 @@ NOKPROBE_SYMBOL(nmi_cpu_backtrace_handler);
 
 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;
 }
 early_initcall(register_nmi_cpu_backtrace_handler);
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 17804ba0b02f..a3c753dfce91 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -774,7 +774,7 @@ static int __init inject_init(void)
 
 	debugfs_init();
 
-	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify", 0);
+	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify", NMIS_VECTOR_MCE);
 	mce_register_injector_chain(&inject_nb);
 
 	setup_inj_struct(&i_mce);
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index ab2d1b79b79e..9ca4b141da0c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -602,7 +602,7 @@ int kgdb_arch_init(void)
 	if (retval)
 		goto out;
 
-	retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler, 0, "kgdb", 0);
+	retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler, 0, "kgdb", NMIS_VECTOR_KGDB);
 	if (retval)
 		goto out1;
 
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;
+
 	/*
 	 * Indicate if there are multiple registrations on the
 	 * internal NMI handler call chains (SERR and IO_CHECK).
diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index b203e4371816..5196023b31dc 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -63,7 +63,7 @@ static void __init test_nmi_ipi(struct cpumask *mask)
 	unsigned long timeout;
 
 	if (register_nmi_handler(NMI_LOCAL, test_nmi_ipi_callback, NMI_FLAG_FIRST,
-				 "nmi_selftest", 0, __initdata)) {
+				 "nmi_selftest", NMIS_VECTOR_TEST, __initdata)) {
 		nmi_fail = FAILURE;
 		return;
 	}
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index b80812aa06c3..5be1c0bdf901 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -143,7 +143,7 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_reboot)
 static int register_stop_handler(void)
 {
 	return register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback, NMI_FLAG_FIRST, "smp_stop",
-				    0);
+				    NMIS_VECTOR_SMP_STOP);
 }
 
 static void native_stop_other_cpus(int wait)
-- 
2.43.0
Re: [PATCH v5 4/9] x86/nmi: Assign and register NMI-source vectors
Posted by Peter Zijlstra 7 months, 1 week ago
On Tue, May 06, 2025 at 06:21:40PM -0700, Sohil Mehta wrote:

> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index f0a577bf7bba..b9beb216f2d0 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -57,6 +57,38 @@ struct nmiaction {
>  	u8			source_vector;
>  };
>  
> +/**
> + * NMI-source vectors are used to identify the origin of an NMI and to
> + * route the NMI directly to the appropriate handler.
> + *
> + * On CPUs that support NMI-source reporting with FRED, receiving an NMI
> + * with a valid vector sets the corresponding bit in the NMI-source
> + * bitmap. The bitmap is delivered as FRED event data on the stack.
> + * Multiple NMIs are coalesced in the NMI-source bitmap until the next
> + * NMI delivery.
> + *
> + * If an NMI is received without a vector or beyond the defined range,
> + * the CPU sets bit 0 of the NMI-source bitmap.
> + *
> + * 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.
> + *
> + * 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 */

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?

> 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);

instead?
Re: [PATCH v5 4/9] x86/nmi: Assign and register NMI-source vectors
Posted by Sohil Mehta 7 months, 1 week ago
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.