[PATCH v7 00/10] x86: Add support for NMI-source reporting with FRED

[PATCH v7 00/10] x86: Add support for NMI-source reporting with FRED

[Sohil Mehta]

Changes since v6
================
This series is a minor update from v6, with the following changes:

 * Includes a pre-patch to provide separate KVM IRQ and NMI entry
   wrappers for FRED (Sean).
 * Uses an enum to allocate NMI-source vectors (DaveH).
 * Picks up review tags from Xin.
 * Rebased on top of 6.16-rc1.

The patchset seems to be maturing, so while reviewing this version, please 
consider providing Acks and Review tags if the patches look okay.

v6: https://lore.kernel.org/lkml/20250513203803.2636561-1-sohil.mehta@intel.com/

Background
==========
NMI-source reporting with FRED [1] provides a new mechanism for
identifying the source of NMIs. As part of the FRED event delivery
framework, a 16-bit vector bitmap is provided that identifies one or
more sources that caused the NMI.

Using the source bitmap, the kernel can precisely run the relevant NMI
handlers instead of polling the entire NMI handler list. Additionally,
the source information would be invaluable for debugging misbehaving
handlers and unknown NMIs.

Overview of NMI-source usage
============================
Code snippets:

// Allocate a static source vector at compile time
#define NMIS_VECTOR_TEST	1

// Register an NMI handler with the vector
register_nmi_handler(NMI_LOCAL, test_handler, 0, "nmi_test", NMIS_VECTOR_TEST);

// Generate an NMI with the source vector using NMI encoded delivery
__apic_send_IPI_mask(cpumask, APIC_DM_NMI | NMIS_VECTOR_TEST);

// Handle an NMI with or without the source information (oversimplified)
source_bitmap = fred_event_data(regs);
if (!source_bitmap || (source_bitmap & BIT(NMIS_VECTOR_TEST)))
	test_handler();

// Unregister handler along with the vector
unregister_nmi_handler(NMI_LOCAL, "nmi_test");

Patch structure
===============
Patch 1-3: Prepare FRED/KVM and enumerate NMI-source reporting
Patch 4-6: Register and handle NMI-source vectors 
Patch 7-9: APIC changes to generate NMIs with vectors
Patch  10: Improve debug print with NMI-source information

Many thanks to Sean Christopherson, Xin Li, H. Peter Anvin, Andi Kleen,
Tony Luck, Kan Liang, Jacob Pan Jun, Zeng Guang, Peter Zijlstra,
Sandipan Das, Steven Rostedt, Dave Hansen and others for their
contributions, reviews and feedback.

Future work
===========
I am considering a few additional changes related to debugging and
tracing, as well as KVM support, that would be valuable for enhancing
NMI handling in the kernel.

Refer the v6 cover letter for more details:
v6: https://lore.kernel.org/lkml/20250513203803.2636561-1-sohil.mehta@intel.com/

Links
=====
[1]: Chapter 9, https://www.intel.com/content/www/us/en/content-details/819481/flexible-return-and-event-delivery-fred-specification.html

Jacob Pan (1):
  perf/x86: Enable NMI-source reporting for perfmon

Sean Christopherson (1):
  x86/fred: Provide separate IRQ vs. NMI wrappers for entry from KVM

Sohil Mehta (8):
  x86/fred: Pass event data to the NMI entry point from KVM
  x86/cpufeatures: Add the CPUID feature bit for NMI-source reporting
  x86/nmi: Extend the registration interface to include the NMI-source
    vector
  x86/nmi: Assign and register NMI-source vectors
  x86/nmi: Add support to handle NMIs with source information
  x86/nmi: Prepare for the new NMI-source vector encoding
  x86/nmi: Enable NMI-source for IPIs delivered as NMIs
  x86/nmi: Print source information with the unknown NMI console message

 arch/x86/entry/entry_64_fred.S      |  2 +-
 arch/x86/events/amd/ibs.c           |  2 +-
 arch/x86/events/core.c              |  6 +--
 arch/x86/events/intel/core.c        |  6 +--
 arch/x86/include/asm/apic.h         | 39 ++++++++++++++++++
 arch/x86/include/asm/apicdef.h      |  2 +-
 arch/x86/include/asm/cpufeatures.h  |  1 +
 arch/x86/include/asm/fred.h         | 35 +++++++++++-----
 arch/x86/include/asm/nmi.h          | 62 ++++++++++++++++++++++++++++-
 arch/x86/kernel/apic/hw_nmi.c       |  5 +--
 arch/x86/kernel/apic/ipi.c          |  4 +-
 arch/x86/kernel/apic/local.h        | 24 ++++++-----
 arch/x86/kernel/cpu/cpuid-deps.c    |  1 +
 arch/x86/kernel/cpu/mce/inject.c    |  4 +-
 arch/x86/kernel/cpu/mshyperv.c      |  2 +-
 arch/x86/kernel/kgdb.c              |  8 ++--
 arch/x86/kernel/kvm.c               |  9 +----
 arch/x86/kernel/nmi.c               | 40 +++++++++++++++++++
 arch/x86/kernel/nmi_selftest.c      |  8 ++--
 arch/x86/kernel/smp.c               |  6 +--
 arch/x86/kvm/vmx/vmx.c              |  4 +-
 arch/x86/platform/uv/uv_nmi.c       |  4 +-
 drivers/acpi/apei/ghes.c            |  2 +-
 drivers/char/ipmi/ipmi_watchdog.c   |  3 +-
 drivers/edac/igen6_edac.c           |  3 +-
 drivers/thermal/intel/therm_throt.c |  2 +-
 drivers/watchdog/hpwdt.c            |  6 +--
 27 files changed, 218 insertions(+), 72 deletions(-)


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.43.0

[PATCH v7 01/10] x86/fred: Provide separate IRQ vs. NMI wrappers for entry from KVM

[Sohil Mehta]

From: Sean Christopherson <seanjc@google.com>

Provide separate wrappers for forwarding IRQs vs NMIs from KVM in
anticipation of adding support for NMI source reporting, which will add
an NMI-only parameter, i.e. will further pollute the current API with a
param that is a hardcoded for one of the two call sites.

Opportunistically tag the non-FRED NMI wrapper __always_inline, as the
compiler could theoretically generate a function call and trigger and a
(completely benign) "leaving noinstr" warning.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: New patch
---
 arch/x86/include/asm/fred.h | 30 ++++++++++++++++++++++--------
 arch/x86/kvm/vmx/vmx.c      |  4 ++--
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
index 12b34d5b2953..552332ca060c 100644
--- a/arch/x86/include/asm/fred.h
+++ b/arch/x86/include/asm/fred.h
@@ -9,6 +9,7 @@
 #include <linux/const.h>
 
 #include <asm/asm.h>
+#include <asm/irq_vectors.h>
 #include <asm/msr.h>
 #include <asm/trapnr.h>
 
@@ -71,15 +72,27 @@ __visible void fred_entry_from_user(struct pt_regs *regs);
 __visible void fred_entry_from_kernel(struct pt_regs *regs);
 __visible void __fred_entry_from_kvm(struct pt_regs *regs);
 
-/* Can be called from noinstr code, thus __always_inline */
-static __always_inline void fred_entry_from_kvm(unsigned int type, unsigned int vector)
+/* Must be called from noinstr code, thus __always_inline */
+static __always_inline void fred_nmi_from_kvm(void)
 {
 	struct fred_ss ss = {
-		.ss     =__KERNEL_DS,
-		.type   = type,
-		.vector = vector,
-		.nmi    = type == EVENT_TYPE_NMI,
-		.lm     = 1,
+		.ss	= __KERNEL_DS,
+		.type	= EVENT_TYPE_NMI,
+		.vector	= NMI_VECTOR,
+		.nmi	= true,
+		.lm	= 1,
+	};
+
+	asm_fred_entry_from_kvm(ss);
+}
+
+static inline void fred_irq_from_kvm(unsigned int vector)
+{
+	struct fred_ss ss = {
+		.ss	= __KERNEL_DS,
+		.type	= EVENT_TYPE_EXTINT,
+		.vector	= vector,
+		.lm	= 1,
 	};
 
 	asm_fred_entry_from_kvm(ss);
@@ -110,7 +123,8 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs) { ret
 static inline void cpu_init_fred_exceptions(void) { }
 static inline void cpu_init_fred_rsps(void) { }
 static inline void fred_complete_exception_setup(void) { }
-static inline void fred_entry_from_kvm(unsigned int type, unsigned int vector) { }
+static __always_inline void fred_nmi_from_kvm(void) { }
+static inline void fred_irq_from_kvm(unsigned int vector) { }
 static inline void fred_sync_rsp0(unsigned long rsp0) { }
 static inline void fred_update_rsp0(void) { }
 #endif /* CONFIG_X86_FRED */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4953846cb30d..8183886eda3d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6991,7 +6991,7 @@ static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu,
 
 	kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
 	if (cpu_feature_enabled(X86_FEATURE_FRED))
-		fred_entry_from_kvm(EVENT_TYPE_EXTINT, vector);
+		fred_irq_from_kvm(vector);
 	else
 		vmx_do_interrupt_irqoff(gate_offset((gate_desc *)host_idt_base + vector));
 	kvm_after_interrupt(vcpu);
@@ -7264,7 +7264,7 @@ noinstr void vmx_handle_nmi(struct kvm_vcpu *vcpu)
 
 	kvm_before_interrupt(vcpu, KVM_HANDLING_NMI);
 	if (cpu_feature_enabled(X86_FEATURE_FRED))
-		fred_entry_from_kvm(EVENT_TYPE_NMI, NMI_VECTOR);
+		fred_nmi_from_kvm();
 	else
 		vmx_do_nmi_irqoff();
 	kvm_after_interrupt(vcpu);
-- 
2.43.0

[PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sohil Mehta]

Extend the FRED NMI entry point from KVM to take an extra argument to
allow KVM to invoke the FRED event dispatch framework with event data.

This API is used to pass the NMI-source bitmap for NMI-induced VM exits.
Read the VMCS exit qualification field to get the NMI-source information
and store it as event data precisely in the format expected by the FRED
event framework.

Read the VMCS exit qualification unconditionally since almost all
upcoming CPUs are expected to enable FRED and NMI-source together. In
the rare case that NMI-source isn't enabled, the extra VMREAD would be
harmless since the exit qualification is expected to be zero.

Suggested-by: Sean Christopherson <seanjc@google.com>
Originally-by: Zeng Guang <guang.zeng@intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: Pass the event data from KVM only for NMI. (Sean)

v6: No change

v5: Read the VMCS exit qualification unconditionally. (Sean)
    Combine related patches into one.
---
 arch/x86/entry/entry_64_fred.S |  2 +-
 arch/x86/include/asm/fred.h    | 11 ++++++-----
 arch/x86/kvm/vmx/vmx.c         |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index 29c5c32c16c3..1c9c6e036233 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
 	 * +--------+-----------------+
 	 */
 	push $0				/* Reserved, must be 0 */
-	push $0				/* Event data, 0 for IRQ/NMI */
+	push %rsi			/* Event data for NMI */
 	push %rdi			/* fred_ss handed in by the caller */
 	push %rbp
 	pushf
diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
index 552332ca060c..bccf4a3c06b8 100644
--- a/arch/x86/include/asm/fred.h
+++ b/arch/x86/include/asm/fred.h
@@ -66,14 +66,14 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs)
 
 void asm_fred_entrypoint_user(void);
 void asm_fred_entrypoint_kernel(void);
-void asm_fred_entry_from_kvm(struct fred_ss);
+void asm_fred_entry_from_kvm(struct fred_ss ss, unsigned long edata);
 
 __visible void fred_entry_from_user(struct pt_regs *regs);
 __visible void fred_entry_from_kernel(struct pt_regs *regs);
 __visible void __fred_entry_from_kvm(struct pt_regs *regs);
 
 /* Must be called from noinstr code, thus __always_inline */
-static __always_inline void fred_nmi_from_kvm(void)
+static __always_inline void fred_nmi_from_kvm(unsigned long edata)
 {
 	struct fred_ss ss = {
 		.ss	= __KERNEL_DS,
@@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
 		.lm	= 1,
 	};
 
-	asm_fred_entry_from_kvm(ss);
+	asm_fred_entry_from_kvm(ss, edata);
 }
 
 static inline void fred_irq_from_kvm(unsigned int vector)
@@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
 		.lm	= 1,
 	};
 
-	asm_fred_entry_from_kvm(ss);
+	/* Event data is always zero for IRQ */
+	asm_fred_entry_from_kvm(ss, 0);
 }
 
 void cpu_init_fred_exceptions(void);
@@ -123,7 +124,7 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs) { ret
 static inline void cpu_init_fred_exceptions(void) { }
 static inline void cpu_init_fred_rsps(void) { }
 static inline void fred_complete_exception_setup(void) { }
-static __always_inline void fred_nmi_from_kvm(void) { }
+static __always_inline void fred_nmi_from_kvm(unsigned long edata) { }
 static inline void fred_irq_from_kvm(unsigned int vector) { }
 static inline void fred_sync_rsp0(unsigned long rsp0) { }
 static inline void fred_update_rsp0(void) { }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 8183886eda3d..69832ca31e5b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7264,7 +7264,7 @@ noinstr void vmx_handle_nmi(struct kvm_vcpu *vcpu)
 
 	kvm_before_interrupt(vcpu, KVM_HANDLING_NMI);
 	if (cpu_feature_enabled(X86_FEATURE_FRED))
-		fred_nmi_from_kvm();
+		fred_nmi_from_kvm(vmx_get_exit_qual(vcpu));
 	else
 		vmx_do_nmi_irqoff();
 	kvm_after_interrupt(vcpu);
-- 
2.43.0

[PATCH v7 03/10] x86/cpufeatures: Add the CPUID feature bit for NMI-source reporting

[Sohil Mehta]

NMI-source reporting is introduced to report the sources of NMIs with
FRED event delivery based on vectors in NMI interrupt messages or the
local APIC. This enables the kernel to avoid the latency incurred by
going over the entire NMI handler list and reduces ambiguity about the
source of an NMI.

Enumerate NMI-source reporting in cpufeatures.h. Also, since NMI-source
reporting uses the FRED event dispatch framework, make it dependent on
FRED in the CPUID dependency table. This ensures that NMI-source
reporting gets disabled when FRED is disabled.

NMI-source reporting is intended as a kernel feature and does not need
userspace enumeration or configuration. There is no need to expose it to
userspace through /proc/cpuinfo.

Originally-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: No change.

v6: No change.

v5: Add NMI-source to the CPUID dependency table.
    Do not expose NMI-source feature through /proc/cpuinfo.
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index ee176236c2be..b6c907666d5f 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -321,6 +321,7 @@
 #define X86_FEATURE_FRED		(12*32+17) /* "fred" Flexible Return and Event Delivery */
 #define X86_FEATURE_LKGS		(12*32+18) /* Load "kernel" (userspace) GS */
 #define X86_FEATURE_WRMSRNS		(12*32+19) /* Non-serializing WRMSR */
+#define X86_FEATURE_NMI_SOURCE		(12*32+20) /* NMI-Source reporting with FRED */
 #define X86_FEATURE_AMX_FP16		(12*32+21) /* AMX fp16 Support */
 #define X86_FEATURE_AVX_IFMA            (12*32+23) /* Support for VPMADD52[H,L]UQ */
 #define X86_FEATURE_LAM			(12*32+26) /* "lam" Linear Address Masking */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 46efcbd6afa4..87a334f639d5 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -88,6 +88,7 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_AMX_INT8,			X86_FEATURE_AMX_TILE  },
 	{ X86_FEATURE_SHSTK,			X86_FEATURE_XSAVES    },
 	{ X86_FEATURE_FRED,			X86_FEATURE_LKGS      },
+	{ X86_FEATURE_NMI_SOURCE,		X86_FEATURE_FRED      },
 	{ X86_FEATURE_SPEC_CTRL_SSBD,		X86_FEATURE_SPEC_CTRL },
 	{}
 };
-- 
2.43.0

[PATCH v7 04/10] x86/nmi: Extend the registration interface to include the NMI-source vector

[Sohil Mehta]

To prepare for NMI-source reporting, add a source vector argument to the
NMI handler registration interface. Later, this will be used to
register NMI handlers with a unique source vector that can be used to
identify the originator of the NMI.

Vector 0 is reserved by the hardware for situations when a source vector
is not used while generating an NMI or the originator could not be
reliably identified. Registering an NMI handler with vector 0 is
equivalent to not using NMI-source reporting.

For now, just extend the interface with no source information (vector 0)
for all the handlers. No functional change intended.

Originally-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Xin Li (Intel) <xin@zytor.com>
---
v7: Use an enum for defining source vectors (DaveH).
    Use NMIS_NO_SOURCE instead of zero (DaveH).
    Add review tag (Xin).

v6: No change.

v5: Split the patch into two parts. This one only extends the interface.
---
 arch/x86/events/amd/ibs.c         |  2 +-
 arch/x86/events/core.c            |  2 +-
 arch/x86/include/asm/nmi.h        | 16 +++++++++++++++-
 arch/x86/kernel/apic/hw_nmi.c     |  3 +--
 arch/x86/kernel/cpu/mce/inject.c  |  2 +-
 arch/x86/kernel/cpu/mshyperv.c    |  2 +-
 arch/x86/kernel/kgdb.c            |  6 ++----
 arch/x86/kernel/nmi_selftest.c    |  6 +++---
 arch/x86/kernel/smp.c             |  4 ++--
 arch/x86/platform/uv/uv_nmi.c     |  4 ++--
 drivers/acpi/apei/ghes.c          |  2 +-
 drivers/char/ipmi/ipmi_watchdog.c |  3 +--
 drivers/edac/igen6_edac.c         |  3 +--
 drivers/watchdog/hpwdt.c          |  6 +++---
 14 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index 112f43b23ebf..6f8f0d663f2f 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -1485,7 +1485,7 @@ static __init int perf_event_ibs_init(void)
 	if (ret)
 		goto err_op;
 
-	ret = register_nmi_handler(NMI_LOCAL, perf_ibs_nmi_handler, 0, "perf_ibs");
+	ret = register_nmi_handler(NMI_LOCAL, perf_ibs_nmi_handler, 0, "perf_ibs", NMIS_NO_SOURCE);
 	if (ret)
 		goto err_nmi;
 
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 7610f26dfbd9..ec59837d10d1 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2129,7 +2129,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");
+	register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI", NMIS_NO_SOURCE);
 
 	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 79d88d12c8fb..42820c4f59b9 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -48,12 +48,24 @@ enum {
 
 typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *);
 
+/**
+ * enum nmi_source_vectors - NMI-source vectors are used to identify the
+ * origin of an NMI and to route the NMI directly to the appropriate
+ * handler.
+ *
+ * @NMIS_NO_SOURCE:        Reserved for undefined or unidentified sources.
+ */
+enum nmi_source_vectors {
+	NMIS_NO_SOURCE		= 0,
+};
+
 struct nmiaction {
 	struct list_head	list;
 	nmi_handler_t		handler;
 	u64			max_duration;
 	unsigned long		flags;
 	const char		*name;
+	enum nmi_source_vectors	source_vector;
 };
 
 /**
@@ -62,6 +74,7 @@ struct nmiaction {
  * @fn:   The NMI handler
  * @fg:   Flags associated with the NMI handler
  * @n:    Name of the NMI handler
+ * @src:  NMI-source vector for the NMI handler
  * @init: Optional __init* attributes for struct nmiaction
  *
  * Adds the provided handler to the list of handlers for the specified
@@ -75,13 +88,14 @@ struct nmiaction {
  *
  * Return: 0 on success, or an error code on failure.
  */
-#define register_nmi_handler(t, fn, fg, n, init...)	\
+#define register_nmi_handler(t, fn, fg, n, src, init...)\
 ({							\
 	static struct nmiaction init fn##_na = {	\
 		.list = LIST_HEAD_INIT(fn##_na.list),	\
 		.handler = (fn),			\
 		.name = (n),				\
 		.flags = (fg),				\
+		.source_vector = (src),			\
 	};						\
 	__register_nmi_handler((t), &fn##_na);		\
 })
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index 45af535c44a0..d09e771723ed 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -53,8 +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");
+	register_nmi_handler(NMI_LOCAL, nmi_cpu_backtrace_handler, 0, "arch_bt", NMIS_NO_SOURCE);
 	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 d02c4f556cd0..ba70ef8a1964 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -775,7 +775,7 @@ static int __init inject_init(void)
 
 	debugfs_init();
 
-	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify");
+	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify", NMIS_NO_SOURCE);
 	mce_register_injector_chain(&inject_nb);
 
 	setup_inj_struct(&i_mce);
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index c78f860419d6..c093f7baab6a 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -550,7 +550,7 @@ static void __init ms_hyperv_init_platform(void)
 	}
 
 	register_nmi_handler(NMI_UNKNOWN, hv_nmi_unknown, NMI_FLAG_FIRST,
-			     "hv_nmi_unknown");
+			     "hv_nmi_unknown", NMIS_NO_SOURCE);
 #endif
 
 #ifdef CONFIG_X86_IO_APIC
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 102641fd2172..ae28fdeefc7f 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -602,13 +602,11 @@ int kgdb_arch_init(void)
 	if (retval)
 		goto out;
 
-	retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler,
-					0, "kgdb");
+	retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler, 0, "kgdb", NMIS_NO_SOURCE);
 	if (retval)
 		goto out1;
 
-	retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler,
-					0, "kgdb");
+	retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler, 0, "kgdb", NMIS_NO_SOURCE);
 
 	if (retval)
 		goto out2;
diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index a010e9d062bf..c4fffa868160 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -41,7 +41,7 @@ static void __init init_nmi_testsuite(void)
 {
 	/* trap all the unknown NMIs we may generate */
 	register_nmi_handler(NMI_UNKNOWN, nmi_unk_cb, 0, "nmi_selftest_unk",
-			__initdata);
+			     NMIS_NO_SOURCE, __initdata);
 }
 
 static void __init cleanup_nmi_testsuite(void)
@@ -63,8 +63,8 @@ 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", __initdata)) {
+	if (register_nmi_handler(NMI_LOCAL, test_nmi_ipi_callback, NMI_FLAG_FIRST,
+				 "nmi_selftest", NMIS_NO_SOURCE, __initdata)) {
 		nmi_fail = FAILURE;
 		return;
 	}
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 18266cc3d98c..c9435730dfb0 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -142,8 +142,8 @@ 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");
+	return register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback, NMI_FLAG_FIRST,
+				    "smp_stop", NMIS_NO_SOURCE);
 }
 
 static void native_stop_other_cpus(int wait)
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index 5c50e550ab63..5af368710f69 100644
--- a/arch/x86/platform/uv/uv_nmi.c
+++ b/arch/x86/platform/uv/uv_nmi.c
@@ -1029,10 +1029,10 @@ static int uv_handle_nmi_ping(unsigned int reason, struct pt_regs *regs)
 
 static void uv_register_nmi_notifier(void)
 {
-	if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv"))
+	if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv", NMIS_NO_SOURCE))
 		pr_warn("UV: NMI handler failed to register\n");
 
-	if (register_nmi_handler(NMI_LOCAL, uv_handle_nmi_ping, 0, "uvping"))
+	if (register_nmi_handler(NMI_LOCAL, uv_handle_nmi_ping, 0, "uvping", NMIS_NO_SOURCE))
 		pr_warn("UV: PING NMI handler failed to register\n");
 }
 
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index f0584ccad451..5e8632f34769 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -1421,7 +1421,7 @@ static void ghes_nmi_add(struct ghes *ghes)
 {
 	mutex_lock(&ghes_list_mutex);
 	if (list_empty(&ghes_nmi))
-		register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes");
+		register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes", NMIS_NO_SOURCE);
 	list_add_rcu(&ghes->list, &ghes_nmi);
 	mutex_unlock(&ghes_list_mutex);
 }
diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
index ab759b492fdd..944caef3dd31 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -1227,8 +1227,7 @@ static void check_parms(void)
 		}
 	}
 	if (do_nmi && !nmi_handler_registered) {
-		rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0,
-						"ipmi");
+		rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0, "ipmi", NMIS_NO_SOURCE);
 		if (rv) {
 			pr_warn("Can't register nmi handler\n");
 			return;
diff --git a/drivers/edac/igen6_edac.c b/drivers/edac/igen6_edac.c
index 1930dc00c791..9c7913e5bd3e 100644
--- a/drivers/edac/igen6_edac.c
+++ b/drivers/edac/igen6_edac.c
@@ -1419,8 +1419,7 @@ static int register_err_handler(void)
 		return 0;
 	}
 
-	rc = register_nmi_handler(NMI_SERR, ecclog_nmi_handler,
-				  0, IGEN6_NMI_NAME);
+	rc = register_nmi_handler(NMI_SERR, ecclog_nmi_handler, 0, IGEN6_NMI_NAME, NMIS_NO_SOURCE);
 	if (rc) {
 		igen6_printk(KERN_ERR, "Failed to register NMI handler\n");
 		return rc;
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index ae30e394d176..90ae59a09270 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -242,13 +242,13 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
 	/*
 	 * Only one function can register for NMI_UNKNOWN
 	 */
-	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, 0, "hpwdt");
+	retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, 0, "hpwdt", NMIS_NO_SOURCE);
 	if (retval)
 		goto error;
-	retval = register_nmi_handler(NMI_SERR, hpwdt_pretimeout, 0, "hpwdt");
+	retval = register_nmi_handler(NMI_SERR, hpwdt_pretimeout, 0, "hpwdt", NMIS_NO_SOURCE);
 	if (retval)
 		goto error1;
-	retval = register_nmi_handler(NMI_IO_CHECK, hpwdt_pretimeout, 0, "hpwdt");
+	retval = register_nmi_handler(NMI_IO_CHECK, hpwdt_pretimeout, 0, "hpwdt", NMIS_NO_SOURCE);
 	if (retval)
 		goto error2;
 
-- 
2.43.0

[PATCH v7 05/10] x86/nmi: Assign and register NMI-source vectors

[Sohil Mehta]

Prior to NMI-source support, the vector information was ignored by the
hardware while delivering NMIs. With NMI-source reporting, the initial
architecture supports a 16-bit source bitmap to identify the source of
the NMI. Upon receiving an NMI, this bitmap is delivered to the kernel
as part of the FRED event delivery mechanism.

Assign a vector space of 1-15 that is specific to NMI-source and
independent of the IDT vector space. Though unlikely, the hardware may
extend the NMI-source bitmap in the future. Add a code comment to
clarify how the kernel support can be easily extended.

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 vector 2.

Vector 2 is reserved for external NMIs corresponding to the Local APIC -
LINT1 pin. Some third-party chipsets may send NMI messages with a fixed
vector value of 2. Using vector 2 for something else would lead to
confusion about the exact source. Do not assign it 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. Warn
if the vector values are unexpected.

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>
Reviewed-by: Xin Li (Intel) <xin@zytor.com>
---
v7: Use an enum to define the NMI-source vectors. (DaveH)
    Add a BUILD_BUG_ON to validate the allocated vector count. (DaveH)
    Add review tag from Xin.

v6: Store source vector unconditionally. (PeterZ)
    Add a warning for unexpected source vector values. (PeterZ)

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       | 46 ++++++++++++++++++++++++++++++++
 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            |  7 +++++
 arch/x86/kernel/nmi_selftest.c   |  2 +-
 arch/x86/kernel/smp.c            |  2 +-
 8 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index ec59837d10d1..dd42fe7bce9c 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2129,7 +2129,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", NMIS_NO_SOURCE);
+	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 42820c4f59b9..a48958a236fd 100644
--- a/arch/x86/include/asm/nmi.h
+++ b/arch/x86/include/asm/nmi.h
@@ -53,12 +53,58 @@ typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *);
  * 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.
+ *
+ * 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 before the COUNT.
+ *
  * @NMIS_NO_SOURCE:        Reserved for undefined or unidentified sources.
+ * @NMIS_VECTOR_TEST:      NMI selftest.
+ * @NMIS_VECTOR_EXTERNAL:  Reserved to match external NMI vector 2.
+ * @NMIS_VECTOR_SMP_STOP:  Panic stop CPU.
+ * @NMIS_VECTOR_BT:        CPU backtrace.
+ * @NMIS_VECTOR_KGDB:      Kernel debugger.
+ * @NMIS_VECTOR_MCE:       MCE injection.
+ * @NMIS_VECTOR_PMI:       PerfMon counters.
+ *
+ * @NMIS_VECTOR_COUNT:     Count of the defined vectors.
  */
 enum nmi_source_vectors {
 	NMIS_NO_SOURCE		= 0,
+	NMIS_VECTOR_TEST,
+	NMIS_VECTOR_EXTERNAL	= 2,
+	NMIS_VECTOR_SMP_STOP,
+	NMIS_VECTOR_BT,
+	NMIS_VECTOR_KGDB,
+	NMIS_VECTOR_MCE,
+	NMIS_VECTOR_PMI,
+
+	NMIS_VECTOR_COUNT
 };
 
+/*
+ * The early (and likely all future) hardware implementations of
+ * NMI-source reporting would only support a 16-bit source bitmap, with
+ * 1-15 being valid source vectors.
+ *
+ * If the hardware ever supports a larger bitmap, the kernel support can
+ * easily be extended to 64 bits by modifying the MAX below. However,
+ * care must be taken to reallocate the latency sensitive NMI sources
+ * within the first 15 vectors. Any source vector beyond the supported
+ * maximum on prior systems would set bit 0 in the NMI-source bitmap.
+ */
+#define NMIS_VECTORS_MAX	16
+
 struct nmiaction {
 	struct list_head	list;
 	nmi_handler_t		handler;
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index d09e771723ed..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", NMIS_NO_SOURCE);
+	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 ba70ef8a1964..320068e01c22 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -775,7 +775,7 @@ static int __init inject_init(void)
 
 	debugfs_init();
 
-	register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify", NMIS_NO_SOURCE);
+	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 ae28fdeefc7f..dfde434d2992 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", NMIS_NO_SOURCE);
+	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..8071ad32aa11 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -182,6 +182,13 @@ int __register_nmi_handler(unsigned int type, struct nmiaction *action)
 	if (WARN_ON_ONCE(!action->handler || !list_empty(&action->list)))
 		return -EINVAL;
 
+	/* NMI-source reporting should only be used for NMI_LOCAL */
+	WARN_ON_ONCE((type != NMI_LOCAL) && (action->source_vector != NMIS_NO_SOURCE));
+
+	/* Check for valid vector values. See comment above NMIS_VECTORS_MAX */
+	BUILD_BUG_ON(NMIS_VECTOR_COUNT > NMIS_VECTORS_MAX);
+	WARN_ON_ONCE(action->source_vector >= NMIS_VECTOR_COUNT);
+
 	raw_spin_lock_irqsave(&desc->lock, flags);
 
 	/*
diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index c4fffa868160..f3918888e494 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -64,7 +64,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", NMIS_NO_SOURCE, __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 c9435730dfb0..1ed065b78467 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", NMIS_NO_SOURCE);
+				    "smp_stop", NMIS_VECTOR_SMP_STOP);
 }
 
 static void native_stop_other_cpus(int wait)
-- 
2.43.0

[PATCH v7 06/10] x86/nmi: Add support to handle NMIs with source information

[Sohil Mehta]

The NMI-source bitmap is delivered as FRED event data to the kernel.
When available, use NMI-source based filtering to determine the exact
handlers to run.

Activate NMI-source based filtering only for Local NMIs. While handling
platform NMI types (such as SERR and IOCHK), do not use the source
bitmap. They have only one handler registered per type, so there is no
need to disambiguate between multiple handlers.

Some third-party chipsets may send NMI messages with a fixed vector of
2, which would result in bit 2 being set in the NMI-source bitmap. Skip
the local NMI handlers in this situation.

Bit 0 of the source bitmap is set by the hardware whenever a source
vector was not used while generating an NMI, or the originator could not
be reliably identified. Poll all the registered handlers in that case.

When multiple handlers need to be executed, adhere to the existing
priority scheme and execute the handlers registered with NMI_FLAG_FIRST
before others.

The logic for handling legacy NMIs is unaffected since the source bitmap
would always have all bits set.

Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Xin Li (Intel) <xin@zytor.com>
---
v7: Add review tag from Xin.

v6: Get rid of a separate NMI source matching function (PeterZ)
    Set source_bitmap to ULONG_MAX to match all sources by default

v5: Significantly simplify NMI-source handling logic.
    Get rid of a separate lookup table for NMI-source vectors.
    Adhere to existing priority scheme for handling NMIs.
---
 arch/x86/kernel/nmi.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 8071ad32aa11..3d2b636e9379 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -130,6 +130,7 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration)
 static int nmi_handle(unsigned int type, struct pt_regs *regs)
 {
 	struct nmi_desc *desc = nmi_to_desc(type);
+	unsigned long source_bitmap = ULONG_MAX;
 	nmi_handler_t ehandler;
 	struct nmiaction *a;
 	int handled=0;
@@ -148,16 +149,45 @@ static int nmi_handle(unsigned int type, struct pt_regs *regs)
 
 	rcu_read_lock();
 
+	/*
+	 * Activate NMI source-based filtering only for Local NMIs.
+	 *
+	 * Platform NMI types (such as SERR and IOCHK) have only one
+	 * handler registered per type, so there is no need to
+	 * disambiguate between multiple handlers.
+	 *
+	 * Also, if a platform source ends up setting bit 2 in the
+	 * source bitmap, the local NMI handlers would be skipped since
+	 * none of them use this reserved vector.
+	 *
+	 * For Unknown NMIs, avoid using the source bitmap to ensure all
+	 * potential handlers have a chance to claim responsibility.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
+		source_bitmap = fred_event_data(regs);
+
+		/* Reset the bitmap if a valid source could not be identified */
+		if (WARN_ON_ONCE(!source_bitmap) || (source_bitmap & BIT(NMIS_NO_SOURCE)))
+			source_bitmap = ULONG_MAX;
+	}
+
 	/*
 	 * NMIs are edge-triggered, which means if you have enough
 	 * of them concurrently, you can lose some because only one
 	 * can be latched at any given time.  Walk the whole list
 	 * to handle those situations.
+	 *
+	 * However, NMI-source reporting does not have this limitation.
+	 * When NMI sources have been identified, only run the handlers
+	 * that match the reported vectors.
 	 */
 	list_for_each_entry_rcu(a, &desc->head, list) {
 		int thishandled;
 		u64 delta;
 
+		if (!(source_bitmap & BIT(a->source_vector)))
+			continue;
+
 		delta = sched_clock();
 		thishandled = a->handler(type, regs);
 		handled += thishandled;
-- 
2.43.0

[PATCH v7 07/10] x86/nmi: Prepare for the new NMI-source vector encoding

[Sohil Mehta]

When using the send_IPI_* APIC calls, callers typically use NMI vector
0x2 to trigger NMIs. The APIC APIs convert the NMI vector to the NMI
delivery mode, which is eventually used to program the APIC.

Before FRED, the hardware would ignore the vector used with NMI delivery
mode. However, with NMI-source reporting, the vector information is
relayed to the destination CPU, which sets the corresponding bit in the
NMI-source bitmap. Unfortunately, the kernel now needs to maintain a new
set of NMI vectors and differentiate them from the IDT vectors.

Instead of creating a parallel set of send_NMI_* APIs to handle
NMI-source vectors, enhance the existing send_IPI_* APIs with a new
encoding scheme to handle the NMI delivery mode along with the
NMI-source vector.

NMI-source vectors would be encoded as:
    APIC_DM_NMI (0x400) | NMI_SOURCE_VECTOR (0x1-0xF)

Also, introduce a helper to prepare the ICR value with the encoded
delivery mode and vector. Update the guest paravirtual APIC code to use
the new helper as well.

While at it, rename APIC_DM_FIXED_MASK to the more appropriate
APIC_DM_MASK.

Suggested-by: Sean Christopherson <seanjc@google.com>
Co-developed-by: Xin Li (Intel) <xin@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: No change.

v6: Remove a redundant else statement. (PeterZ)

v5: Use a simiplified encoding scheme for NMI-source vectors.
---
 arch/x86/include/asm/apic.h         | 30 +++++++++++++++++++++++++++++
 arch/x86/include/asm/apicdef.h      |  2 +-
 arch/x86/kernel/apic/ipi.c          |  4 ++--
 arch/x86/kernel/apic/local.h        | 24 ++++++++++++-----------
 arch/x86/kernel/kvm.c               |  9 +--------
 drivers/thermal/intel/therm_throt.c |  2 +-
 6 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 23d86c9750b9..32cdd81e5e45 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -470,6 +470,36 @@ static __always_inline bool apic_id_valid(u32 apic_id)
 	return apic_id <= apic->max_apic_id;
 }
 
+/*
+ * Prepare the delivery mode and vector for the ICR.
+ *
+ * NMI-source vectors have the NMI delivery mode encoded within them to
+ * differentiate them from the IDT vectors. IDT vector 0x2 (NMI_VECTOR)
+ * is treated as an NMI request but without any NMI-source information.
+ */
+static inline u16 __prepare_ICR_DM_vector(u16 dm_vector)
+{
+	u16 vector = dm_vector & APIC_VECTOR_MASK;
+	u16 dm = dm_vector & APIC_DM_MASK;
+
+	if (dm == APIC_DM_NMI) {
+		/*
+		 * Pre-FRED, the actual vector is ignored for NMIs, but
+		 * zero it if NMI-source reporting is not supported to
+		 * avoid breakage on misbehaving hardware or hypervisors.
+		 */
+		if (!cpu_feature_enabled(X86_FEATURE_NMI_SOURCE))
+			vector = 0;
+
+		return dm | vector;
+	}
+
+	if (vector == NMI_VECTOR)
+		return APIC_DM_NMI;
+
+	return APIC_DM_FIXED | vector;
+}
+
 #else /* CONFIG_X86_LOCAL_APIC */
 
 static inline u32 apic_read(u32 reg) { return 0; }
diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 094106b6a538..3fb8fa73f6aa 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -87,8 +87,8 @@
 #define		APIC_ICR_BUSY		0x01000
 #define		APIC_DEST_LOGICAL	0x00800
 #define		APIC_DEST_PHYSICAL	0x00000
+#define		APIC_DM_MASK		0x00700
 #define		APIC_DM_FIXED		0x00000
-#define		APIC_DM_FIXED_MASK	0x00700
 #define		APIC_DM_LOWEST		0x00100
 #define		APIC_DM_SMI		0x00200
 #define		APIC_DM_REMRD		0x00300
diff --git a/arch/x86/kernel/apic/ipi.c b/arch/x86/kernel/apic/ipi.c
index 98a57cb4aa86..4e8bc42f3bd5 100644
--- a/arch/x86/kernel/apic/ipi.c
+++ b/arch/x86/kernel/apic/ipi.c
@@ -158,7 +158,7 @@ static void __default_send_IPI_shortcut(unsigned int shortcut, int vector)
 	 * issues where otherwise the system hangs when the panic CPU tries
 	 * to stop the others before launching the kdump kernel.
 	 */
-	if (unlikely(vector == NMI_VECTOR))
+	if (unlikely(is_nmi_vector(vector)))
 		apic_mem_wait_icr_idle_timeout();
 	else
 		apic_mem_wait_icr_idle();
@@ -175,7 +175,7 @@ void __default_send_IPI_dest_field(unsigned int dest_mask, int vector,
 				   unsigned int dest_mode)
 {
 	/* See comment in __default_send_IPI_shortcut() */
-	if (unlikely(vector == NMI_VECTOR))
+	if (unlikely(is_nmi_vector(vector)))
 		apic_mem_wait_icr_idle_timeout();
 	else
 		apic_mem_wait_icr_idle();
diff --git a/arch/x86/kernel/apic/local.h b/arch/x86/kernel/apic/local.h
index bdcf609eb283..9a54c589a4bf 100644
--- a/arch/x86/kernel/apic/local.h
+++ b/arch/x86/kernel/apic/local.h
@@ -24,22 +24,24 @@ extern u32 x2apic_max_apicid;
 
 /* IPI */
 
+u16 __prepare_ICR_DM_vector(u16 vector);
+
 DECLARE_STATIC_KEY_FALSE(apic_use_ipi_shorthand);
 
+/* NMI-source vectors have the delivery mode encoded within them */
+static inline bool is_nmi_vector(u16 vector)
+{
+	if ((vector & APIC_DM_MASK) == APIC_DM_NMI)
+		return true;
+	if ((vector & APIC_VECTOR_MASK) == NMI_VECTOR)
+		return true;
+	return false;
+}
+
 static inline unsigned int __prepare_ICR(unsigned int shortcut, int vector,
 					 unsigned int dest)
 {
-	unsigned int icr = shortcut | dest;
-
-	switch (vector) {
-	default:
-		icr |= APIC_DM_FIXED | vector;
-		break;
-	case NMI_VECTOR:
-		icr |= APIC_DM_NMI;
-		break;
-	}
-	return icr;
+	return shortcut | dest | __prepare_ICR_DM_vector(vector);
 }
 
 void default_init_apic_ldr(void);
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 921c1c783bc1..317d585ff3d0 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -518,14 +518,7 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
 
 	local_irq_save(flags);
 
-	switch (vector) {
-	default:
-		icr = APIC_DM_FIXED | vector;
-		break;
-	case NMI_VECTOR:
-		icr = APIC_DM_NMI;
-		break;
-	}
+	icr = __prepare_ICR_DM_vector(vector);
 
 	for_each_cpu(cpu, mask) {
 		apic_id = per_cpu(x86_cpu_to_apicid, cpu);
diff --git a/drivers/thermal/intel/therm_throt.c b/drivers/thermal/intel/therm_throt.c
index debc94e2dc16..5c0d2de2986e 100644
--- a/drivers/thermal/intel/therm_throt.c
+++ b/drivers/thermal/intel/therm_throt.c
@@ -740,7 +740,7 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
 	 * BIOS has programmed on AP based on BSP's info we saved since BIOS
 	 * is always setting the same value for all threads/cores.
 	 */
-	if ((h & APIC_DM_FIXED_MASK) != APIC_DM_FIXED)
+	if ((h & APIC_DM_MASK) != APIC_DM_FIXED)
 		apic_write(APIC_LVTTHMR, lvtthmr_init);
 
 
-- 
2.43.0

[PATCH v7 08/10] x86/nmi: Enable NMI-source for IPIs delivered as NMIs

[Sohil Mehta]

With the IPI handling APIs ready to support the new NMI encoding, encode
the NMI delivery mode directly with the NMI-source vectors to trigger
NMIs.

Move most of the existing NMI-based IPIs to use the new NMI-source
vectors, except for the microcode rendezvous NMI and the crash reboot
NMI. NMI handling for them is special-cased in exc_nmi() and does not
need NMI-source reporting.

However, in the future, it might be useful to assign a source vector to
all NMI sources to improve isolation and debuggability.

Originally-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Suggested-by: Sean Christopherson <seanjc@google.com>
Co-developed-by: Xin Li (Intel) <xin@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: No change.

v6: Include asm/nmi.h to avoid compile errors. (LKP)

v5: Encode APIC_DM_NMI directly with the NMI-source vector.
---
 arch/x86/include/asm/apic.h      | 8 ++++++++
 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_selftest.c   | 2 +-
 arch/x86/kernel/smp.c            | 2 +-
 6 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 32cdd81e5e45..5789df1708bd 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -14,6 +14,7 @@
 #include <asm/msr.h>
 #include <asm/hardirq.h>
 #include <asm/io.h>
+#include <asm/nmi.h>
 #include <asm/posted_intr.h>
 
 #define ARCH_APICTIMER_STOPS_ON_C3	1
@@ -23,6 +24,13 @@
 #define APIC_EXTNMI_ALL		1
 #define APIC_EXTNMI_NONE	2
 
+/* Trigger NMIs with source information */
+#define TEST_NMI		(APIC_DM_NMI | NMIS_VECTOR_TEST)
+#define SMP_STOP_NMI		(APIC_DM_NMI | NMIS_VECTOR_SMP_STOP)
+#define BT_NMI			(APIC_DM_NMI | NMIS_VECTOR_BT)
+#define KGDB_NMI		(APIC_DM_NMI | NMIS_VECTOR_KGDB)
+#define MCE_NMI			(APIC_DM_NMI | NMIS_VECTOR_MCE)
+
 /*
  * Debugging macros
  */
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index 4e04f13d2de9..586f4b25feae 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -33,7 +33,7 @@ u64 hw_nmi_get_sample_period(int watchdog_thresh)
 #ifdef arch_trigger_cpumask_backtrace
 static void nmi_raise_cpu_backtrace(cpumask_t *mask)
 {
-	__apic_send_IPI_mask(mask, NMI_VECTOR);
+	__apic_send_IPI_mask(mask, BT_NMI);
 }
 
 void arch_trigger_cpumask_backtrace(const cpumask_t *mask, int exclude_cpu)
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 320068e01c22..81a04836ac74 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -270,7 +270,7 @@ static void __maybe_unused raise_mce(struct mce *m)
 					mce_irq_ipi, NULL, 0);
 				preempt_enable();
 			} else if (m->inject_flags & MCJ_NMI_BROADCAST)
-				__apic_send_IPI_mask(mce_inject_cpumask, NMI_VECTOR);
+				__apic_send_IPI_mask(mce_inject_cpumask, MCE_NMI);
 		}
 		start = jiffies;
 		while (!cpumask_empty(mce_inject_cpumask)) {
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index dfde434d2992..013feeb6a9ef 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -416,7 +416,7 @@ static void kgdb_disable_hw_debug(struct pt_regs *regs)
  */
 void kgdb_roundup_cpus(void)
 {
-	apic_send_IPI_allbutself(NMI_VECTOR);
+	apic_send_IPI_allbutself(KGDB_NMI);
 }
 #endif
 
diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c
index f3918888e494..d5578370b47f 100644
--- a/arch/x86/kernel/nmi_selftest.c
+++ b/arch/x86/kernel/nmi_selftest.c
@@ -72,7 +72,7 @@ static void __init test_nmi_ipi(struct cpumask *mask)
 	/* sync above data before sending NMI */
 	wmb();
 
-	__apic_send_IPI_mask(mask, NMI_VECTOR);
+	__apic_send_IPI_mask(mask, TEST_NMI);
 
 	/* Don't wait longer than a second */
 	timeout = USEC_PER_SEC;
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 1ed065b78467..239e9bf97abb 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -217,7 +217,7 @@ static void native_stop_other_cpus(int wait)
 			pr_emerg("Shutting down cpus with NMI\n");
 
 			for_each_cpu(cpu, &cpus_stop_mask)
-				__apic_send_IPI(cpu, NMI_VECTOR);
+				__apic_send_IPI(cpu, SMP_STOP_NMI);
 		}
 		/*
 		 * Don't wait longer than 10 ms if the caller didn't
-- 
2.43.0

[PATCH v7 09/10] perf/x86: Enable NMI-source reporting for perfmon

[Sohil Mehta]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

Program the designated PMI NMI-source vector into the local vector table
for the PMU. An NMI for the PMU would directly invoke the PMI handler
without polling other NMI handlers, resulting in reduced PMI delivery
latency.

Co-developed-by: Zeng Guang <guang.zeng@intel.com>
Signed-off-by: Zeng Guang <guang.zeng@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Tested-by: Sandipan Das <sandipan.das@amd.com> # AMD overlapping bits
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Reviewed-by: Xin Li (Intel) <xin@zytor.com>
---
v7: Pick up a review tag (Xin).

v6: Pick up a tested-by tag (Sandipan).

v5: No significant change.
---
 arch/x86/events/core.c       | 4 ++--
 arch/x86/events/intel/core.c | 6 +++---
 arch/x86/include/asm/apic.h  | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index dd42fe7bce9c..3336609288b0 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1704,7 +1704,7 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
 	 * This generic handler doesn't seem to have any issues where the
 	 * unmasking occurs so it was left at the top.
 	 */
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
+	apic_write(APIC_LVTPC, PERF_NMI);
 
 	for_each_set_bit(idx, x86_pmu.cntr_mask, X86_PMC_IDX_MAX) {
 		if (!test_bit(idx, cpuc->active_mask))
@@ -1746,7 +1746,7 @@ void perf_events_lapic_init(void)
 	/*
 	 * Always use NMI for PMU
 	 */
-	apic_write(APIC_LVTPC, APIC_DM_NMI);
+	apic_write(APIC_LVTPC, PERF_NMI);
 }
 
 static int
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 741b229f0718..000d3ab72bd2 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3318,7 +3318,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 	 * NMI handler.
 	 */
 	if (!late_ack && !mid_ack)
-		apic_write(APIC_LVTPC, APIC_DM_NMI);
+		apic_write(APIC_LVTPC, PERF_NMI);
 	intel_bts_disable_local();
 	cpuc->enabled = 0;
 	__intel_pmu_disable_all(true);
@@ -3355,7 +3355,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 
 done:
 	if (mid_ack)
-		apic_write(APIC_LVTPC, APIC_DM_NMI);
+		apic_write(APIC_LVTPC, PERF_NMI);
 	/* Only restore PMU state when it's active. See x86_pmu_disable(). */
 	cpuc->enabled = pmu_enabled;
 	if (pmu_enabled)
@@ -3368,7 +3368,7 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 	 * Haswell CPUs.
 	 */
 	if (late_ack)
-		apic_write(APIC_LVTPC, APIC_DM_NMI);
+		apic_write(APIC_LVTPC, PERF_NMI);
 	return handled;
 }
 
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 5789df1708bd..7287005f05a6 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -30,6 +30,7 @@
 #define BT_NMI			(APIC_DM_NMI | NMIS_VECTOR_BT)
 #define KGDB_NMI		(APIC_DM_NMI | NMIS_VECTOR_KGDB)
 #define MCE_NMI			(APIC_DM_NMI | NMIS_VECTOR_MCE)
+#define PERF_NMI		(APIC_DM_NMI | NMIS_VECTOR_PMI)
 
 /*
  * Debugging macros
-- 
2.43.0

[PATCH v7 10/10] x86/nmi: Print source information with the unknown NMI console message

[Sohil Mehta]

The NMI-source bitmap is a useful piece of information provided by the
NMI-source reporting feature. It is very helpful for debugging unknown
NMIs, as it can pinpoint the exact source that caused the NMI.

Print the complete source bitmap along with the "unknown NMI" kernel log
message, since unexpected sources might have triggered the NMI.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
v7: No change.

v6: Drop the tracepoint modification part for now.

v5: New patch
---
 arch/x86/kernel/nmi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 3d2b636e9379..0b5bb20c5eb7 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -379,6 +379,9 @@ unknown_nmi_error(unsigned char reason, struct pt_regs *regs)
 	pr_emerg_ratelimited("Uhhuh. NMI received for unknown reason %02x on CPU %d.\n",
 			     reason, smp_processor_id());
 
+	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE))
+		pr_emerg_ratelimited("NMI-source bitmap is 0x%lx\n", fred_event_data(regs));
+
 	if (unknown_nmi_panic || panic_on_unrecovered_nmi)
 		nmi_panic(regs, "NMI: Not continuing");
 
-- 
2.43.0

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sean Christopherson]

On Thu, Jun 12, 2025, Sohil Mehta wrote:
> Extend the FRED NMI entry point from KVM to take an extra argument to
> allow KVM to invoke the FRED event dispatch framework with event data.
> 
> This API is used to pass the NMI-source bitmap for NMI-induced VM exits.
> Read the VMCS exit qualification field to get the NMI-source information
> and store it as event data precisely in the format expected by the FRED
> event framework.
> 
> Read the VMCS exit qualification unconditionally since almost all
> upcoming CPUs are expected to enable FRED and NMI-source together. In
> the rare case that NMI-source isn't enabled, the extra VMREAD would be
> harmless since the exit qualification is expected to be zero.

Nit, instead of "is expected to be zero", something like this 

  harmless since the exit qualification is architecturally guaranteed to be
  zero on CPUs that don't support NMI-source reporting.  Per the SDM's
  "Exit qualification" subsection of "Basic VM-Exit Information":

      For all other VM exits, this field is cleared.
--

to make it very explicit that reading the exit qualification on older CPUs is 100%
safe, e.g. even on non-FRED CPUs (see https://lore.kernel.org/all/aBUiwLV4ZY2HdRbz@google.com).

> Suggested-by: Sean Christopherson <seanjc@google.com>
> Originally-by: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---

Acked-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v7 00/10] x86: Add support for NMI-source reporting with FRED

[Peter Zijlstra]

On Thu, Jun 12, 2025 at 02:48:39PM -0700, Sohil Mehta wrote:

> Jacob Pan (1):
>   perf/x86: Enable NMI-source reporting for perfmon
> 
> Sean Christopherson (1):
>   x86/fred: Provide separate IRQ vs. NMI wrappers for entry from KVM
> 
> Sohil Mehta (8):
>   x86/fred: Pass event data to the NMI entry point from KVM
>   x86/cpufeatures: Add the CPUID feature bit for NMI-source reporting
>   x86/nmi: Extend the registration interface to include the NMI-source
>     vector
>   x86/nmi: Assign and register NMI-source vectors
>   x86/nmi: Add support to handle NMIs with source information
>   x86/nmi: Prepare for the new NMI-source vector encoding
>   x86/nmi: Enable NMI-source for IPIs delivered as NMIs
>   x86/nmi: Print source information with the unknown NMI console message

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sohil Mehta]

On 6/12/2025 5:18 PM, Sean Christopherson wrote:

>> Read the VMCS exit qualification unconditionally since almost all
>> upcoming CPUs are expected to enable FRED and NMI-source together. In
>> the rare case that NMI-source isn't enabled, the extra VMREAD would be
>> harmless since the exit qualification is expected to be zero.
> 
> Nit, instead of "is expected to be zero", something like this 
> 
>   harmless since the exit qualification is architecturally guaranteed to be
>   zero on CPUs that don't support NMI-source reporting.  Per the SDM's
>   "Exit qualification" subsection of "Basic VM-Exit Information":
> 
>       For all other VM exits, this field is cleared.
> --
> 

Looks good. Clarifying it explicitly reduces ambiguity.

If this patchset gets applied directly, I am hoping the clarification
can be included while applying. If we end up doing another version, I'll
add it to the log.

> to make it very explicit that reading the exit qualification on older CPUs is 100%
> safe, e.g. even on non-FRED CPUs (see https://lore.kernel.org/all/aBUiwLV4ZY2HdRbz@google.com).
> 

That's interesting. Thanks for the link.

>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Originally-by: Zeng Guang <guang.zeng@intel.com>
>> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
>> ---
> 
> Acked-by: Sean Christopherson <seanjc@google.com>

Thank you!

Re: [PATCH v7 00/10] x86: Add support for NMI-source reporting with FRED

[Sohil Mehta]

On 6/13/2025 12:06 AM, Peter Zijlstra wrote:

> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 

Thank you!

Re: [PATCH v7 01/10] x86/fred: Provide separate IRQ vs. NMI wrappers for entry from KVM

[Xin Li]

On 6/12/2025 2:48 PM, Sohil Mehta wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Provide separate wrappers for forwarding IRQs vs NMIs from KVM in
> anticipation of adding support for NMI source reporting, which will add
> an NMI-only parameter, i.e. will further pollute the current API with a
> param that is a hardcoded for one of the two call sites.
> 
> Opportunistically tag the non-FRED NMI wrapper __always_inline, as the
> compiler could theoretically generate a function call and trigger and a
                                                                     ^
Nit, looks to me that this is an extra "and".

> (completely benign) "leaving noinstr" warning.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Reviewed-by: Xin Li (Intel) <xin@zytor.com>

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Xin Li]

On 6/12/2025 2:48 PM, Sohil Mehta wrote:
> Extend the FRED NMI entry point from KVM to take an extra argument to
> allow KVM to invoke the FRED event dispatch framework with event data.
> 
> This API is used to pass the NMI-source bitmap for NMI-induced VM exits.
> Read the VMCS exit qualification field to get the NMI-source information
> and store it as event data precisely in the format expected by the FRED
> event framework.
> 
> Read the VMCS exit qualification unconditionally since almost all
> upcoming CPUs are expected to enable FRED and NMI-source together. In
> the rare case that NMI-source isn't enabled, the extra VMREAD would be
> harmless since the exit qualification is expected to be zero.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Originally-by: Zeng Guang <guang.zeng@intel.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

A couple of nits below, otherwise:

Reviewed-by: Xin Li (Intel) <xin@zytor.com>

> ---
> v7: Pass the event data from KVM only for NMI. (Sean)
> 
> v6: No change
> 
> v5: Read the VMCS exit qualification unconditionally. (Sean)
>      Combine related patches into one.
> ---
>   arch/x86/entry/entry_64_fred.S |  2 +-
>   arch/x86/include/asm/fred.h    | 11 ++++++-----
>   arch/x86/kvm/vmx/vmx.c         |  2 +-
>   3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
> index 29c5c32c16c3..1c9c6e036233 100644
> --- a/arch/x86/entry/entry_64_fred.S
> +++ b/arch/x86/entry/entry_64_fred.S
> @@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
>   	 * +--------+-----------------+
>   	 */
>   	push $0				/* Reserved, must be 0 */
> -	push $0				/* Event data, 0 for IRQ/NMI */
> +	push %rsi			/* Event data for NMI */

Maybe a bit more accurate?

/* Event data, NMI-source bitmap only so far */

>   	push %rdi			/* fred_ss handed in by the caller */
>   	push %rbp
>   	pushf
> diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
> index 552332ca060c..bccf4a3c06b8 100644
> --- a/arch/x86/include/asm/fred.h
> +++ b/arch/x86/include/asm/fred.h
> @@ -66,14 +66,14 @@ static __always_inline unsigned long fred_event_data(struct pt_regs *regs)
>   
>   void asm_fred_entrypoint_user(void);
>   void asm_fred_entrypoint_kernel(void);
> -void asm_fred_entry_from_kvm(struct fred_ss);
> +void asm_fred_entry_from_kvm(struct fred_ss ss, unsigned long edata);
>   
>   __visible void fred_entry_from_user(struct pt_regs *regs);
>   __visible void fred_entry_from_kernel(struct pt_regs *regs);
>   __visible void __fred_entry_from_kvm(struct pt_regs *regs);
>   
>   /* Must be called from noinstr code, thus __always_inline */
> -static __always_inline void fred_nmi_from_kvm(void)
> +static __always_inline void fred_nmi_from_kvm(unsigned long edata)
>   {
>   	struct fred_ss ss = {
>   		.ss	= __KERNEL_DS,
> @@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
>   		.lm	= 1,
>   	};
>   
> -	asm_fred_entry_from_kvm(ss);
> +	asm_fred_entry_from_kvm(ss, edata);
>   }
>   
>   static inline void fred_irq_from_kvm(unsigned int vector)
> @@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
>   		.lm	= 1,
>   	};
>   
> -	asm_fred_entry_from_kvm(ss);
> +	/* Event data is always zero for IRQ */

/* Event data not used for IRQ thus 0 */

Re: [PATCH v7 03/10] x86/cpufeatures: Add the CPUID feature bit for NMI-source reporting

[Xin Li]

On 6/12/2025 2:48 PM, Sohil Mehta wrote:
> NMI-source reporting is introduced to report the sources of NMIs with
> FRED event delivery based on vectors in NMI interrupt messages or the
> local APIC. This enables the kernel to avoid the latency incurred by
> going over the entire NMI handler list and reduces ambiguity about the
> source of an NMI.
> 
> Enumerate NMI-source reporting in cpufeatures.h. Also, since NMI-source
> reporting uses the FRED event dispatch framework, make it dependent on
> FRED in the CPUID dependency table. This ensures that NMI-source
> reporting gets disabled when FRED is disabled.
> 
> NMI-source reporting is intended as a kernel feature and does not need
> userspace enumeration or configuration. There is no need to expose it to
> userspace through /proc/cpuinfo.
> 
> Originally-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
> Signed-off-by: Sohil Mehta<sohil.mehta@intel.com>

Reviewed-by: Xin Li (Intel) <xin@zytor.com>

Re: [PATCH v7 07/10] x86/nmi: Prepare for the new NMI-source vector encoding

[Chao Gao]

On Thu, Jun 12, 2025 at 02:48:46PM -0700, Sohil Mehta wrote:
>When using the send_IPI_* APIC calls, callers typically use NMI vector
>0x2 to trigger NMIs. The APIC APIs convert the NMI vector to the NMI
>delivery mode, which is eventually used to program the APIC.
>
>Before FRED, the hardware would ignore the vector used with NMI delivery
>mode. However, with NMI-source reporting, the vector information is
>relayed to the destination CPU, which sets the corresponding bit in the
>NMI-source bitmap. Unfortunately, the kernel now needs to maintain a new
>set of NMI vectors and differentiate them from the IDT vectors.
>
>Instead of creating a parallel set of send_NMI_* APIs to handle
>NMI-source vectors, enhance the existing send_IPI_* APIs with a new
>encoding scheme to handle the NMI delivery mode along with the
>NMI-source vector.
>
>NMI-source vectors would be encoded as:
>    APIC_DM_NMI (0x400) | NMI_SOURCE_VECTOR (0x1-0xF)
>
>Also, introduce a helper to prepare the ICR value with the encoded
>delivery mode and vector. Update the guest paravirtual APIC code to use
>the new helper as well.
>
>While at it, rename APIC_DM_FIXED_MASK to the more appropriate
>APIC_DM_MASK.
>
>Suggested-by: Sean Christopherson <seanjc@google.com>
>Co-developed-by: Xin Li (Intel) <xin@zytor.com>
>Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

One nit below,

>--- a/arch/x86/kernel/apic/local.h
>+++ b/arch/x86/kernel/apic/local.h
>@@ -24,22 +24,24 @@ extern u32 x2apic_max_apicid;
> 
> /* IPI */
> 
>+u16 __prepare_ICR_DM_vector(u16 vector);
>+

This seems unnecessary. local.h already includes asm/apic.h, where the function
is defined.

Re: [PATCH v7 01/10] x86/fred: Provide separate IRQ vs. NMI wrappers for entry from KVM

[Sohil Mehta]

On 6/18/2025 8:53 PM, Xin Li wrote:
> On 6/12/2025 2:48 PM, Sohil Mehta wrote:
>> From: Sean Christopherson <seanjc@google.com>
>>
>> Provide separate wrappers for forwarding IRQs vs NMIs from KVM in
>> anticipation of adding support for NMI source reporting, which will add
>> an NMI-only parameter, i.e. will further pollute the current API with a
>> param that is a hardcoded for one of the two call sites.
>>
>> Opportunistically tag the non-FRED NMI wrapper __always_inline, as the
>> compiler could theoretically generate a function call and trigger and a
>                                                                      ^
> Nit, looks to me that this is an extra "and".
> 

Ah! Will remove.

>> (completely benign) "leaving noinstr" warning.
>>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> 
> Reviewed-by: Xin Li (Intel) <xin@zytor.com>

Thanks

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sohil Mehta]

On 6/18/2025 10:02 PM, Xin Li wrote:
>> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
>> index 29c5c32c16c3..1c9c6e036233 100644
>> --- a/arch/x86/entry/entry_64_fred.S
>> +++ b/arch/x86/entry/entry_64_fred.S
>> @@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
>>   	 * +--------+-----------------+
>>   	 */
>>   	push $0				/* Reserved, must be 0 */
>> -	push $0				/* Event data, 0 for IRQ/NMI */
>> +	push %rsi			/* Event data for NMI */
> 
> Maybe a bit more accurate?
> 

Actually, I am wondering if it might be better to make it less precise
than it is right now. How about

/* Event data passed in by the caller */

The problem with having precise comments for a generic implementation is
that the caller might get updated, but we would forget to update this
comment since nothing in this function needs to change.

> /* Event data, NMI-source bitmap only so far */
> 
>>   	push %rdi			/* fred_ss handed in by the caller */
>>   	push %rbp
>>   	pushf

...

>>   /* Must be called from noinstr code, thus __always_inline */
>> -static __always_inline void fred_nmi_from_kvm(void)
>> +static __always_inline void fred_nmi_from_kvm(unsigned long edata)
>>   {
>>   	struct fred_ss ss = {
>>   		.ss	= __KERNEL_DS,
>> @@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
>>   		.lm	= 1,
>>   	};
>>   
>> -	asm_fred_entry_from_kvm(ss);
>> +	asm_fred_entry_from_kvm(ss, edata);
>>   }
>>   
>>   static inline void fred_irq_from_kvm(unsigned int vector)
>> @@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
>>   		.lm	= 1,
>>   	};
>>   
>> -	asm_fred_entry_from_kvm(ss);
>> +	/* Event data is always zero for IRQ */
> 
> /* Event data not used for IRQ thus 0 */

Event data "not used" might imply that the architecture provides
something, but the kernel is choosing to not use it. There is no event
data for IRQ, right?

I want to say that the event data for IRQ has to be zero until the
architecture changes — Similar to the /* Reserved, must be 0 */ comment
in asm_fred_entry_from_kvm().

Re: [PATCH v7 07/10] x86/nmi: Prepare for the new NMI-source vector encoding

[Sohil Mehta]

On 6/19/2025 12:43 AM, Chao Gao wrote:

>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Co-developed-by: Xin Li (Intel) <xin@zytor.com>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> 
> Reviewed-by: Chao Gao <chao.gao@intel.com>
> 

Thanks!

> One nit below,
> 
>> --- a/arch/x86/kernel/apic/local.h
>> +++ b/arch/x86/kernel/apic/local.h
>> @@ -24,22 +24,24 @@ extern u32 x2apic_max_apicid;
>>
>> /* IPI */
>>
>> +u16 __prepare_ICR_DM_vector(u16 vector);
>> +
> 
> This seems unnecessary. local.h already includes asm/apic.h, where the function
> is defined.

I added this because 0day was flagging an issue with i386 allnoconfig.
But, thinking about it more, I'll instead move the definition of
__prepare_ICR_DM_vector() outside the CONFIG_X86_LOCAL_APIC section in
asm/apic.h.

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Xin Li]

On 6/19/2025 3:15 PM, Sohil Mehta wrote:
> On 6/18/2025 10:02 PM, Xin Li wrote:
>>> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
>>> index 29c5c32c16c3..1c9c6e036233 100644
>>> --- a/arch/x86/entry/entry_64_fred.S
>>> +++ b/arch/x86/entry/entry_64_fred.S
>>> @@ -93,7 +93,7 @@ SYM_FUNC_START(asm_fred_entry_from_kvm)
>>>    	 * +--------+-----------------+
>>>    	 */
>>>    	push $0				/* Reserved, must be 0 */
>>> -	push $0				/* Event data, 0 for IRQ/NMI */
>>> +	push %rsi			/* Event data for NMI */
>>
>> Maybe a bit more accurate?
>>
> 
> Actually, I am wondering if it might be better to make it less precise
> than it is right now. How about
> 
> /* Event data passed in by the caller */
> 
> The problem with having precise comments for a generic implementation is
> that the caller might get updated, but we would forget to update this
> comment since nothing in this function needs to change.

No strong preference, I'm okay if you take this approach.

> 
>> /* Event data, NMI-source bitmap only so far */
>>
>>>    	push %rdi			/* fred_ss handed in by the caller */
>>>    	push %rbp
>>>    	pushf
> 
> ...
> 
>>>    /* Must be called from noinstr code, thus __always_inline */
>>> -static __always_inline void fred_nmi_from_kvm(void)
>>> +static __always_inline void fred_nmi_from_kvm(unsigned long edata)
>>>    {
>>>    	struct fred_ss ss = {
>>>    		.ss	= __KERNEL_DS,
>>> @@ -83,7 +83,7 @@ static __always_inline void fred_nmi_from_kvm(void)
>>>    		.lm	= 1,
>>>    	};
>>>    
>>> -	asm_fred_entry_from_kvm(ss);
>>> +	asm_fred_entry_from_kvm(ss, edata);
>>>    }
>>>    
>>>    static inline void fred_irq_from_kvm(unsigned int vector)
>>> @@ -95,7 +95,8 @@ static inline void fred_irq_from_kvm(unsigned int vector)
>>>    		.lm	= 1,
>>>    	};
>>>    
>>> -	asm_fred_entry_from_kvm(ss);
>>> +	/* Event data is always zero for IRQ */
>>
>> /* Event data not used for IRQ thus 0 */
> 
> Event data "not used" might imply that the architecture provides
> something, but the kernel is choosing to not use it. There is no event
> data for IRQ, right?
> 
> I want to say that the event data for IRQ has to be zero until the
> architecture changes — Similar to the /* Reserved, must be 0 */ comment
> in asm_fred_entry_from_kvm().
> 

FRED spec says:

For any other event, the event data are not currently defined and will 
be zero until they are.

So "Event data not defined for IRQ thus 0."

Re: [PATCH v7 07/10] x86/nmi: Prepare for the new NMI-source vector encoding

[Sohil Mehta]

Hi Sean,

Thank you for helping shape this series. Please see the request below.

On 6/12/2025 2:48 PM, Sohil Mehta wrote:
> When using the send_IPI_* APIC calls, callers typically use NMI vector
> 0x2 to trigger NMIs. The APIC APIs convert the NMI vector to the NMI
> delivery mode, which is eventually used to program the APIC.
> 
> Before FRED, the hardware would ignore the vector used with NMI delivery
> mode. However, with NMI-source reporting, the vector information is
> relayed to the destination CPU, which sets the corresponding bit in the
> NMI-source bitmap. Unfortunately, the kernel now needs to maintain a new
> set of NMI vectors and differentiate them from the IDT vectors.
> 
> Instead of creating a parallel set of send_NMI_* APIs to handle
> NMI-source vectors, enhance the existing send_IPI_* APIs with a new
> encoding scheme to handle the NMI delivery mode along with the
> NMI-source vector.
> 
> NMI-source vectors would be encoded as:
>     APIC_DM_NMI (0x400) | NMI_SOURCE_VECTOR (0x1-0xF)
> 
> Also, introduce a helper to prepare the ICR value with the encoded
> delivery mode and vector. Update the guest paravirtual APIC code to use
> the new helper as well.
> 
> While at it, rename APIC_DM_FIXED_MASK to the more appropriate
> APIC_DM_MASK.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>

The changes in patches 7 and 8 are based on suggestions you made almost
a year back. https://lore.kernel.org/lkml/Zr9X-08zsOKFlvkB@google.com/

I am wondering if the implementation seems appropriate? Also, there is a
minor KVM change in this patch. Eventually, I am hoping to convert the
suggested-bys into acked-bys in both the patches.

Thanks!


> Co-developed-by: Xin Li (Intel) <xin@zytor.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
> v7: No change.
> 
> v6: Remove a redundant else statement. (PeterZ)
> 
> v5: Use a simiplified encoding scheme for NMI-source vectors.
> ---
>  arch/x86/include/asm/apic.h         | 30 +++++++++++++++++++++++++++++
>  arch/x86/include/asm/apicdef.h      |  2 +-
>  arch/x86/kernel/apic/ipi.c          |  4 ++--
>  arch/x86/kernel/apic/local.h        | 24 ++++++++++++-----------
>  arch/x86/kernel/kvm.c               |  9 +--------
>  drivers/thermal/intel/therm_throt.c |  2 +-
>  6 files changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 23d86c9750b9..32cdd81e5e45 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -470,6 +470,36 @@ static __always_inline bool apic_id_valid(u32 apic_id)
>  	return apic_id <= apic->max_apic_id;
>  }
>  
> +/*
> + * Prepare the delivery mode and vector for the ICR.
> + *
> + * NMI-source vectors have the NMI delivery mode encoded within them to
> + * differentiate them from the IDT vectors. IDT vector 0x2 (NMI_VECTOR)
> + * is treated as an NMI request but without any NMI-source information.
> + */
> +static inline u16 __prepare_ICR_DM_vector(u16 dm_vector)
> +{
> +	u16 vector = dm_vector & APIC_VECTOR_MASK;
> +	u16 dm = dm_vector & APIC_DM_MASK;
> +
> +	if (dm == APIC_DM_NMI) {
> +		/*
> +		 * Pre-FRED, the actual vector is ignored for NMIs, but
> +		 * zero it if NMI-source reporting is not supported to
> +		 * avoid breakage on misbehaving hardware or hypervisors.
> +		 */
> +		if (!cpu_feature_enabled(X86_FEATURE_NMI_SOURCE))
> +			vector = 0;
> +
> +		return dm | vector;
> +	}
> +
> +	if (vector == NMI_VECTOR)
> +		return APIC_DM_NMI;
> +
> +	return APIC_DM_FIXED | vector;
> +}
> +
>  #else /* CONFIG_X86_LOCAL_APIC */
>  
>  static inline u32 apic_read(u32 reg) { return 0; }
> diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
> index 094106b6a538..3fb8fa73f6aa 100644
> --- a/arch/x86/include/asm/apicdef.h
> +++ b/arch/x86/include/asm/apicdef.h
> @@ -87,8 +87,8 @@
>  #define		APIC_ICR_BUSY		0x01000
>  #define		APIC_DEST_LOGICAL	0x00800
>  #define		APIC_DEST_PHYSICAL	0x00000
> +#define		APIC_DM_MASK		0x00700
>  #define		APIC_DM_FIXED		0x00000
> -#define		APIC_DM_FIXED_MASK	0x00700
>  #define		APIC_DM_LOWEST		0x00100
>  #define		APIC_DM_SMI		0x00200
>  #define		APIC_DM_REMRD		0x00300
> diff --git a/arch/x86/kernel/apic/ipi.c b/arch/x86/kernel/apic/ipi.c
> index 98a57cb4aa86..4e8bc42f3bd5 100644
> --- a/arch/x86/kernel/apic/ipi.c
> +++ b/arch/x86/kernel/apic/ipi.c
> @@ -158,7 +158,7 @@ static void __default_send_IPI_shortcut(unsigned int shortcut, int vector)
>  	 * issues where otherwise the system hangs when the panic CPU tries
>  	 * to stop the others before launching the kdump kernel.
>  	 */
> -	if (unlikely(vector == NMI_VECTOR))
> +	if (unlikely(is_nmi_vector(vector)))
>  		apic_mem_wait_icr_idle_timeout();
>  	else
>  		apic_mem_wait_icr_idle();
> @@ -175,7 +175,7 @@ void __default_send_IPI_dest_field(unsigned int dest_mask, int vector,
>  				   unsigned int dest_mode)
>  {
>  	/* See comment in __default_send_IPI_shortcut() */
> -	if (unlikely(vector == NMI_VECTOR))
> +	if (unlikely(is_nmi_vector(vector)))
>  		apic_mem_wait_icr_idle_timeout();
>  	else
>  		apic_mem_wait_icr_idle();
> diff --git a/arch/x86/kernel/apic/local.h b/arch/x86/kernel/apic/local.h
> index bdcf609eb283..9a54c589a4bf 100644
> --- a/arch/x86/kernel/apic/local.h
> +++ b/arch/x86/kernel/apic/local.h
> @@ -24,22 +24,24 @@ extern u32 x2apic_max_apicid;
>  
>  /* IPI */
>  
> +u16 __prepare_ICR_DM_vector(u16 vector);
> +
>  DECLARE_STATIC_KEY_FALSE(apic_use_ipi_shorthand);
>  
> +/* NMI-source vectors have the delivery mode encoded within them */
> +static inline bool is_nmi_vector(u16 vector)
> +{
> +	if ((vector & APIC_DM_MASK) == APIC_DM_NMI)
> +		return true;
> +	if ((vector & APIC_VECTOR_MASK) == NMI_VECTOR)
> +		return true;
> +	return false;
> +}
> +
>  static inline unsigned int __prepare_ICR(unsigned int shortcut, int vector,
>  					 unsigned int dest)
>  {
> -	unsigned int icr = shortcut | dest;
> -
> -	switch (vector) {
> -	default:
> -		icr |= APIC_DM_FIXED | vector;
> -		break;
> -	case NMI_VECTOR:
> -		icr |= APIC_DM_NMI;
> -		break;
> -	}
> -	return icr;
> +	return shortcut | dest | __prepare_ICR_DM_vector(vector);
>  }
>  
>  void default_init_apic_ldr(void);
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 921c1c783bc1..317d585ff3d0 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -518,14 +518,7 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
>  
>  	local_irq_save(flags);
>  
> -	switch (vector) {
> -	default:
> -		icr = APIC_DM_FIXED | vector;
> -		break;
> -	case NMI_VECTOR:
> -		icr = APIC_DM_NMI;
> -		break;
> -	}
> +	icr = __prepare_ICR_DM_vector(vector);
>  
>  	for_each_cpu(cpu, mask) {
>  		apic_id = per_cpu(x86_cpu_to_apicid, cpu);
> diff --git a/drivers/thermal/intel/therm_throt.c b/drivers/thermal/intel/therm_throt.c
> index debc94e2dc16..5c0d2de2986e 100644
> --- a/drivers/thermal/intel/therm_throt.c
> +++ b/drivers/thermal/intel/therm_throt.c
> @@ -740,7 +740,7 @@ void intel_init_thermal(struct cpuinfo_x86 *c)
>  	 * BIOS has programmed on AP based on BSP's info we saved since BIOS
>  	 * is always setting the same value for all threads/cores.
>  	 */
> -	if ((h & APIC_DM_FIXED_MASK) != APIC_DM_FIXED)
> +	if ((h & APIC_DM_MASK) != APIC_DM_FIXED)
>  		apic_write(APIC_LVTTHMR, lvtthmr_init);
>  
>  

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sohil Mehta]

On 6/19/2025 3:45 PM, Xin Li wrote:
> On 6/19/2025 3:15 PM, Sohil Mehta wrote:
>>
>> I want to say that the event data for IRQ has to be zero until the
>> architecture changes — Similar to the /* Reserved, must be 0 */ comment
>> in asm_fred_entry_from_kvm().
>>
> 
> FRED spec says:
> 
> For any other event, the event data are not currently defined and will 
> be zero until they are.
> 
> So "Event data not defined for IRQ thus 0."

I am fine with this. Not *defined* removes the ambiguity.

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[H. Peter Anvin]

On 2025-06-19 15:57, Sohil Mehta wrote:
> On 6/19/2025 3:45 PM, Xin Li wrote:
>> On 6/19/2025 3:15 PM, Sohil Mehta wrote:
>>>
>>> I want to say that the event data for IRQ has to be zero until the
>>> architecture changes — Similar to the /* Reserved, must be 0 */ comment
>>> in asm_fred_entry_from_kvm().
>>>
>>
>> FRED spec says:
>>
>> For any other event, the event data are not currently defined and will
>> be zero until they are.
>>
>> So "Event data not defined for IRQ thus 0."
> 
> I am fine with this. Not *defined* removes the ambiguity.
> 

So I was thinking about this, and wonder: how expensive is it to get the 
event data exit information out of VMX? If it is not very expensive, it 
would arguably be a good thing to future-proof by fetching that 
information, even if it is currently always zero.

	-hpa

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sean Christopherson]

On Fri, Jun 20, 2025, H. Peter Anvin wrote:
> On 2025-06-19 15:57, Sohil Mehta wrote:
> > On 6/19/2025 3:45 PM, Xin Li wrote:
> > > On 6/19/2025 3:15 PM, Sohil Mehta wrote:
> > > > 
> > > > I want to say that the event data for IRQ has to be zero until the
> > > > architecture changes — Similar to the /* Reserved, must be 0 */ comment
> > > > in asm_fred_entry_from_kvm().
> > > > 
> > > 
> > > FRED spec says:
> > > 
> > > For any other event, the event data are not currently defined and will
> > > be zero until they are.
> > > 
> > > So "Event data not defined for IRQ thus 0."
> > 
> > I am fine with this. Not *defined* removes the ambiguity.
> > 
> 
> So I was thinking about this, and wonder: how expensive is it to get the
> event data exit information out of VMX? If it is not very expensive, it
> would arguably be a good thing to future-proof by fetching that information,
> even if it is currently always zero.

It's trivially easy to do in KVM, and the cost of the VMREAD should be less than
20 cycles.  So quite cheap in the grand scheme.  If VMREAD is more costly than
that, then we have bigger problems :-)

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[H. Peter Anvin]

On 2025-06-20 16:18, Sean Christopherson wrote:
>>
>> So I was thinking about this, and wonder: how expensive is it to get the
>> event data exit information out of VMX? If it is not very expensive, it
>> would arguably be a good thing to future-proof by fetching that information,
>> even if it is currently always zero.
> 
> It's trivially easy to do in KVM, and the cost of the VMREAD should be less than
> 20 cycles.  So quite cheap in the grand scheme.  If VMREAD is more costly than
> that, then we have bigger problems :-)
> 

LOL. Since it is up to you, Paulo, etc. to decide how to do the 
tradeoffs formaintainability, debuggability and performance in KVM I am 
guessing this is a vote in favor? (You can always take it out if it is a 
performance problem, until such time that the kernel itself starts 
consuming this information for reasons currently unknown.)

	-hpa

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[Sean Christopherson]

On Fri, Jun 20, 2025, H. Peter Anvin wrote:
> On 2025-06-20 16:18, Sean Christopherson wrote:
> > > 
> > > So I was thinking about this, and wonder: how expensive is it to get the
> > > event data exit information out of VMX? If it is not very expensive, it
> > > would arguably be a good thing to future-proof by fetching that information,
> > > even if it is currently always zero.
> > 
> > It's trivially easy to do in KVM, and the cost of the VMREAD should be less than
> > 20 cycles.  So quite cheap in the grand scheme.  If VMREAD is more costly than
> > that, then we have bigger problems :-)
> > 
> 
> LOL. Since it is up to you, Paulo, etc. to decide how to do the tradeoffs
> formaintainability, debuggability and performance in KVM I am guessing this
> is a vote in favor? (You can always take it out if it is a performance
> problem, until such time that the kernel itself starts consuming this
> information for reasons currently unknown.)

Unless you can pinky swear that vmcs.EXIT_QUALIFICATION will provide event data
for IRQ exits, then I'd prefer to pass '0' unconditionally.  '0' will always be
safe, if potentially suboptimal.  But passing what could in theory be something
other than FRED-formatted event data could lead to buggy behavior.  Per the FRED
spec, Revision 7.0, exit-qualification doesn't hold event data for IRQ exits.

  For some events for which event data is defined (see Section 5.2.1), the event
  data is saved in the exit-qualification field. (This is done for #DB, #PF, and NMI.)

Re: [PATCH v7 02/10] x86/fred: Pass event data to the NMI entry point from KVM

[H. Peter Anvin]

On June 23, 2025 8:39:41 AM PDT, Sean Christopherson <seanjc@google.com> wrote:
>On Fri, Jun 20, 2025, H. Peter Anvin wrote:
>> On 2025-06-20 16:18, Sean Christopherson wrote:
>> > > 
>> > > So I was thinking about this, and wonder: how expensive is it to get the
>> > > event data exit information out of VMX? If it is not very expensive, it
>> > > would arguably be a good thing to future-proof by fetching that information,
>> > > even if it is currently always zero.
>> > 
>> > It's trivially easy to do in KVM, and the cost of the VMREAD should be less than
>> > 20 cycles.  So quite cheap in the grand scheme.  If VMREAD is more costly than
>> > that, then we have bigger problems :-)
>> > 
>> 
>> LOL. Since it is up to you, Paulo, etc. to decide how to do the tradeoffs
>> formaintainability, debuggability and performance in KVM I am guessing this
>> is a vote in favor? (You can always take it out if it is a performance
>> problem, until such time that the kernel itself starts consuming this
>> information for reasons currently unknown.)
>
>Unless you can pinky swear that vmcs.EXIT_QUALIFICATION will provide event data
>for IRQ exits, then I'd prefer to pass '0' unconditionally.  '0' will always be
>safe, if potentially suboptimal.  But passing what could in theory be something
>other than FRED-formatted event data could lead to buggy behavior.  Per the FRED
>spec, Revision 7.0, exit-qualification doesn't hold event data for IRQ exits.
>
>  For some events for which event data is defined (see Section 5.2.1), the event
>  data is saved in the exit-qualification field. (This is done for #DB, #PF, and NMI.)

I agree, let's stick to that for now, since this is a kernel internal interface and nothing consumes it. After all, it will save a handful of cycles.

I will still check into the "pinky swear", though.

RE: [PATCH v7 05/10] x86/nmi: Assign and register NMI-source vectors

[Zhuo, Qiuxu]

> From: Sohil Mehta <sohil.mehta@intel.com>
> [...]
> Vector 2 is reserved for external NMIs corresponding to the Local APIC -
> LINT1 pin. Some third-party chipsets may send NMI messages with a fixed
> vector value of 2. Using vector 2 for something else would lead to confusion
> about the exact source. Do not assign it 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

From the current NMI code point of view [1] or [2],
a type of platform NMI handler may have multiple handlers registered. 

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/nmi.c#n199
[2] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/nmi.c#n201

> need additional source information to differentiate among them.
> 
> [...]

RE: [PATCH v7 06/10] x86/nmi: Add support to handle NMIs with source information

[Zhuo, Qiuxu]

> From: Sohil Mehta <sohil.mehta@intel.com>
> [...]
> Activate NMI-source based filtering only for Local NMIs. While handling
> platform NMI types (such as SERR and IOCHK), do not use the source bitmap.
> They have only one handler registered per type, so there is no need to

Same as the comments for patch 6. 
Platform NMI types may have multiple handlers registered per type.

> [...]

> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -130,6 +130,7 @@ static void nmi_check_duration(struct nmiaction
> *action, u64 duration)  static int nmi_handle(unsigned int type, struct pt_regs
> *regs)  {
>  	struct nmi_desc *desc = nmi_to_desc(type);
> +	unsigned long source_bitmap = ULONG_MAX;
>  	nmi_handler_t ehandler;
>  	struct nmiaction *a;
>  	int handled=0;
> @@ -148,16 +149,45 @@ static int nmi_handle(unsigned int type, struct
> pt_regs *regs)
> 
>  	rcu_read_lock();
> 
> +	/*
> +	 * Activate NMI source-based filtering only for Local NMIs.
> +	 *
> +	 * Platform NMI types (such as SERR and IOCHK) have only one
> +	 * handler registered per type, so there is no need to

Ditto.

> +	 * disambiguate between multiple handlers.
> +	 *
> +	 * Also, if a platform source ends up setting bit 2 in the
> +	 * source bitmap, the local NMI handlers would be skipped since
> +	 * none of them use this reserved vector.
> +	 *
> +	 * For Unknown NMIs, avoid using the source bitmap to ensure all
> +	 * potential handlers have a chance to claim responsibility.
> +	 */
> +	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type ==
> NMI_LOCAL) {
> +		source_bitmap = fred_event_data(regs);
> +
> +		/* Reset the bitmap if a valid source could not be identified */
> +		if (WARN_ON_ONCE(!source_bitmap) || (source_bitmap &
> BIT(NMIS_NO_SOURCE)))
> +			source_bitmap = ULONG_MAX;
> +	}
> +
>  	/*
>  	 * NMIs are edge-triggered, which means if you have enough
>  	 * of them concurrently, you can lose some because only one
>  	 * can be latched at any given time.  Walk the whole list
>  	 * to handle those situations.
> +	 *
> +	 * However, NMI-source reporting does not have this limitation.
> +	 * When NMI sources have been identified, only run the handlers
> +	 * that match the reported vectors.
>  	 */
>  	list_for_each_entry_rcu(a, &desc->head, list) {
>  		int thishandled;
>  		u64 delta;
> 
> +		if (!(source_bitmap & BIT(a->source_vector)))
> +			continue;
> +

Is it possible for the "source_bitmap" to have some non-NMIS_NO_SOURCE bit set 
while the user registers their NMI handler with the NMIS_NO_SOURCE type? 

If so, we may need to allow the NMI handler with the NMIS_NO_SOURCE type to be 
invoked unconditionally to ensure no NMIs are lost.

>  		delta = sched_clock();
>  		thishandled = a->handler(type, regs);
>  		handled += thishandled;
> --
> 2.43.0
> 

RE: [PATCH v7 00/10] x86: Add support for NMI-source reporting with FRED

[Zhuo, Qiuxu]

> From: Sohil Mehta <sohil.mehta@intel.com>
> [...]
> Subject: [PATCH v7 00/10] x86: Add support for NMI-source reporting with
> FRED

Aside from the comments for patches 5 & 6, the others LGTM.

     Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Re: [PATCH v7 05/10] x86/nmi: Assign and register NMI-source vectors

[Sohil Mehta]

On 7/7/2025 6:21 AM, Zhuo, Qiuxu wrote:
>> From: Sohil Mehta <sohil.mehta@intel.com>
>> [...]
>> Vector 2 is reserved for external NMIs corresponding to the Local APIC -
>> LINT1 pin. Some third-party chipsets may send NMI messages with a fixed
>> vector value of 2. Using vector 2 for something else would lead to confusion
>> about the exact source. Do not assign it 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
> 
> From the current NMI code point of view [1] or [2],
> a type of platform NMI handler may have multiple handlers registered. 
> 
> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/nmi.c#n199
> [2] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/nmi.c#n201
> 

We have warnings that should flag any time multiple handlers are
registered for SERR or IO_CHK:

	/*
	 * Indicate if there are multiple registrations on the
	 * internal NMI handler call chains (SERR and IO_CHECK).
	 */
	WARN_ON_ONCE(type == NMI_SERR && !list_empty(&desc->head));
	WARN_ON_ONCE(type == NMI_IO_CHECK && !list_empty(&desc->head));

>> need additional source information to differentiate among them.
>>
>> [...]

Re: [PATCH v7 06/10] x86/nmi: Add support to handle NMIs with source information

[Sohil Mehta]

On 7/7/2025 6:50 AM, Zhuo, Qiuxu wrote:

> Is it possible for the "source_bitmap" to have some non-NMIS_NO_SOURCE bit set 
> while the user registers their NMI handler with the NMIS_NO_SOURCE type? 
> 

IIUC, this is what you are asking:

Someone registers a handler without any source information.
For example, GHES does this.

register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, "ghes", NMIS_NO_SOURCE);

Now, when a GHES NMI shows up, can it have anything other than bit 0
(NMIS_NO_SOURCE) set in the source bitmap?

I believe the answer is no. Unless the GHES implementation or the
hardware has a bug, this should not happen. If this happens, it would
get logged as an unknown NMI in the kernel log.


> If so, we may need to allow the NMI handler with the NMIS_NO_SOURCE type to be 
> invoked unconditionally to ensure no NMIs are lost.
> 

If the kernel can't rely on the accuracy of the source_bitmap, then
NMI-source as a feature starts losing value.

Sohil

Re: [PATCH v7 00/10] x86: Add support for NMI-source reporting with FRED

[Sohil Mehta]

On 7/7/2025 6:56 AM, Zhuo, Qiuxu wrote:
>> From: Sohil Mehta <sohil.mehta@intel.com>
>> [...]
>> Subject: [PATCH v7 00/10] x86: Add support for NMI-source reporting with
>> FRED
> 
> Aside from the comments for patches 5 & 6, the others LGTM.
> 
>      Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>


Thank you! Appreciate the review.

RE: [PATCH v7 05/10] x86/nmi: Assign and register NMI-source vectors

[Zhuo, Qiuxu]

> From: Mehta, Sohil <sohil.mehta@intel.com>
> [...]
> >> Platform NMI handlers have a single handler registered per type. They
> >> don't
> >
> > From the current NMI code point of view [1] or [2], a type of platform
> > NMI handler may have multiple handlers registered.
> >
> > [1]
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > /tree/arch/x86/kernel/nmi.c#n199 [2]
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > /tree/arch/x86/kernel/nmi.c#n201
> >
> 
> We have warnings that should flag any time multiple handlers are registered
> for SERR or IO_CHK:
> 
> 	/*
> 	 * Indicate if there are multiple registrations on the
> 	 * internal NMI handler call chains (SERR and IO_CHECK).
> 	 */
> 	WARN_ON_ONCE(type == NMI_SERR && !list_empty(&desc->head));
> 	WARN_ON_ONCE(type == NMI_IO_CHECK && !list_empty(&desc-
> >head));
> 

The warning doesn't imply that it must be a single NMI handler. 
Otherwise, why not use a single NMI callback pointer per platform NMI 
type for registration in the current NMI code? 

I just don't want your comments or commit messages to 
mismatch the current code as it stands.

-Qiuxu

Re: [PATCH v7 08/10] x86/nmi: Enable NMI-source for IPIs delivered as NMIs

[Sean Christopherson]

On Thu, Jun 12, 2025, Sohil Mehta wrote:
> With the IPI handling APIs ready to support the new NMI encoding, encode
> the NMI delivery mode directly with the NMI-source vectors to trigger
> NMIs.
> 
> Move most of the existing NMI-based IPIs to use the new NMI-source
> vectors, except for the microcode rendezvous NMI and the crash reboot
> NMI. NMI handling for them is special-cased in exc_nmi() and does not
> need NMI-source reporting.
> 
> However, in the future, it might be useful to assign a source vector to
> all NMI sources to improve isolation and debuggability.
> 
> Originally-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Co-developed-by: Xin Li (Intel) <xin@zytor.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
> v7: No change.
> 
> v6: Include asm/nmi.h to avoid compile errors. (LKP)
> 
> v5: Encode APIC_DM_NMI directly with the NMI-source vector.
> ---
>  arch/x86/include/asm/apic.h      | 8 ++++++++
>  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_selftest.c   | 2 +-
>  arch/x86/kernel/smp.c            | 2 +-
>  6 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 32cdd81e5e45..5789df1708bd 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -14,6 +14,7 @@
>  #include <asm/msr.h>
>  #include <asm/hardirq.h>
>  #include <asm/io.h>
> +#include <asm/nmi.h>
>  #include <asm/posted_intr.h>
>  
>  #define ARCH_APICTIMER_STOPS_ON_C3	1
> @@ -23,6 +24,13 @@
>  #define APIC_EXTNMI_ALL		1
>  #define APIC_EXTNMI_NONE	2
>  
> +/* Trigger NMIs with source information */
> +#define TEST_NMI		(APIC_DM_NMI | NMIS_VECTOR_TEST)
> +#define SMP_STOP_NMI		(APIC_DM_NMI | NMIS_VECTOR_SMP_STOP)
> +#define BT_NMI			(APIC_DM_NMI | NMIS_VECTOR_BT)

s/BT/BACKTRACE?

> +#define KGDB_NMI		(APIC_DM_NMI | NMIS_VECTOR_KGDB)
> +#define MCE_NMI			(APIC_DM_NMI | NMIS_VECTOR_MCE)

IMO, NMI_xxx reads better, e.g. it's easier to see that code is sending an NMI
at the call sites.

> +
>  /*
>   * Debugging macros
>   */
> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> index 4e04f13d2de9..586f4b25feae 100644
> --- a/arch/x86/kernel/apic/hw_nmi.c
> +++ b/arch/x86/kernel/apic/hw_nmi.c
> @@ -33,7 +33,7 @@ u64 hw_nmi_get_sample_period(int watchdog_thresh)
>  #ifdef arch_trigger_cpumask_backtrace
>  static void nmi_raise_cpu_backtrace(cpumask_t *mask)
>  {
> -	__apic_send_IPI_mask(mask, NMI_VECTOR);
> +	__apic_send_IPI_mask(mask, BT_NMI);

This patch is buggy.  There are at least two implementations of ->send_IPI_mask()
that this breaks:

  uv_send_IPI_mask() = > uv_send_IPI_one():

	if (vector == NMI_VECTOR)
		dmode = APIC_DELIVERY_MODE_NMI;
	else
		dmode = APIC_DELIVERY_MODE_FIXED;


and xen_send_IPI_mask() => xen_map_vector():

	switch (vector) {
	case RESCHEDULE_VECTOR:
		xen_vector = XEN_RESCHEDULE_VECTOR;
		break;
	case CALL_FUNCTION_VECTOR:
		xen_vector = XEN_CALL_FUNCTION_VECTOR;
		break;
	case CALL_FUNCTION_SINGLE_VECTOR:
		xen_vector = XEN_CALL_FUNCTION_SINGLE_VECTOR;
		break;
	case IRQ_WORK_VECTOR:
		xen_vector = XEN_IRQ_WORK_VECTOR;
		break;
#ifdef CONFIG_X86_64
	case NMI_VECTOR:
	case APIC_DM_NMI: /* Some use that instead of NMI_VECTOR */
		xen_vector = XEN_NMI_VECTOR;
		break;
#endif
	default:
		xen_vector = -1;
		printk(KERN_ERR "xen: vector 0x%x is not implemented\n",
			vector);
	}

	return xen_vector;

Looking at all of this again, shoving the NMI source information into the @vector
is quite brittle.  Nothing forces implementations to handle embedded delivery
mode information.

One thought would be to pass a small struct (by value), and then provide macros
to generate the structure for a specific vector.  That provides some amount of
type safety and should make it a bit harder to pass in garbage, without making
the callers any less readable.

struct apic_ipi {
	u8 vector;
	u8 type;
};

#define APIC_IPI(v, t) ({ struct apic_ipi i = { .vector = v, .type = t }; i; })
#define APIC_IPI_IRQ(vector) APIC_IPI(vector, APIC_DELIVERY_MODE_FIXED)
#define APIC_IPI_NMI(vector) APIC_IPI(vector, APIC_DELIVERY_MODE_NMI)

#define IPI_IRQ_WORK		APIC_IPI_IRQ(IRQ_WORK_VECTOR)
#define IPI_POSTED_INTR_WAKEUP	APIC_IPI_IRQ(POSTED_INTR_WAKEUP_VECTOR)

#define IPI_NMI_BACKTRACE	APIC_IPI_NMI(NMI_BACKTRACE_VECTOR)

static __always_inline void __apic_send_IPI_self(struct apic_ipi ipi)

Re: [PATCH v7 08/10] x86/nmi: Enable NMI-source for IPIs delivered as NMIs

[Sohil Mehta]

On 7/8/2025 11:37 AM, Sean Christopherson wrote:

> This patch is buggy.  There are at least two implementations of ->send_IPI_mask()
> that this breaks:
> 

Thank you for point this out. I should have been more diligent.


> Looking at all of this again, shoving the NMI source information into the @vector
> is quite brittle.  Nothing forces implementations to handle embedded delivery
> mode information.
> 

I agree. There is already some confusion with NMI_VECTOR and APIC_DM_NMI
used interchangeably sometimes. Adding the new NMI-source vectors with
the encoded delivery mode makes it worse.


> One thought would be to pass a small struct (by value), and then provide macros
> to generate the structure for a specific vector.  That provides some amount of
> type safety and should make it a bit harder to pass in garbage, without making
> the callers any less readable.
>
> struct apic_ipi {
> 	u8 vector;
> 	u8 type;
> };
>  

I am fine with this approach. Though, the changes would be massive since
we have quite a few interfaces and a lot of "struct apic".

	.send_IPI
	.send_IPI_mask
	.send_IPI_mask_allbutself
	.send_IPI_allbutself
	.send_IPI_all
	.send_IPI_self


An option I was considering was whether we should avoid exposing the raw
delivery mode to the callers since it is mainly an APIC internal thing.
The callers should only have to say NMI or IRQ along with the vector and
let the APIC code figure out how to generate it.

One option is to add a separate set of send_IPI_NMI APIs parallel to
send_IPI ones that we have. But then we would end with >10 ways to
generate IPIs.

Another way would be to assign the NMI vectors in a different range and
use the range to differentiate between IRQ and NMI.

For example:
	IRQ => 0x0-0xFF
	NMI => 0x10000-0x1000F.

However, this would still be fragile and probably have similar issues to
the one you pointed out.

> 
> static __always_inline void __apic_send_IPI_self(struct apic_ipi ipi)

Taking a step back:

Since we are considering changing the interface, would it be worth
consolidating the multiple send_IPI APIs into one or two? Mainly, by
moving the destination information from the function name to the
function parameter.

  apic_send_IPI(DEST, MASK, TYPE, VECTOR)

  DEST   => self, all, allbutself, mask, maskbutself

  MASK   => cpumask

  TYPE   => IRQ, NMI

  VECTOR => Vector number specific to the type.

I like the single line IPI invocation. All of this can still be passed
in a neat "struct apic_ipi" with a macro helping the callers fill the
struct.

These interfaces are decades old. So, maybe I am being too ambitious and
this isn't practically feasible. Thoughts/Suggestions?


Note: Another part of me says there are only a handful of NMI IPI usages
and the heavy lifting isn't worth it. We should fix the bugs, improve
testing and use the existing approach since it is the least invasive :)

Re: [PATCH v7 08/10] x86/nmi: Enable NMI-source for IPIs delivered as NMIs

[Sean Christopherson]

On Thu, Jul 10, 2025, Sohil Mehta wrote:
> On 7/8/2025 11:37 AM, Sean Christopherson wrote:
> 
> > This patch is buggy.  There are at least two implementations of ->send_IPI_mask()
> > that this breaks:
> > 
> 
> Thank you for point this out. I should have been more diligent.
> 
> 
> > Looking at all of this again, shoving the NMI source information into the @vector
> > is quite brittle.  Nothing forces implementations to handle embedded delivery
> > mode information.
> > 
> 
> I agree. There is already some confusion with NMI_VECTOR and APIC_DM_NMI
> used interchangeably sometimes. Adding the new NMI-source vectors with
> the encoded delivery mode makes it worse.
> 
> 
> > One thought would be to pass a small struct (by value), and then provide macros
> > to generate the structure for a specific vector.  That provides some amount of
> > type safety and should make it a bit harder to pass in garbage, without making
> > the callers any less readable.
> >
> > struct apic_ipi {
> > 	u8 vector;
> > 	u8 type;
> > };
> >  
> 
> I am fine with this approach. Though, the changes would be massive since
> we have quite a few interfaces and a lot of "struct apic".

It'd definitely be big, but it doesn't seem like it'd be overwhelmingly painful.
Though it's certainly enough churn that I wouldn't do anything until there's a
consensus one way or the other :-)

> 	.send_IPI
> 	.send_IPI_mask
> 	.send_IPI_mask_allbutself
> 	.send_IPI_allbutself
> 	.send_IPI_all
> 	.send_IPI_self
> 
> 
> An option I was considering was whether we should avoid exposing the raw
> delivery mode to the callers since it is mainly an APIC internal thing.
> The callers should only have to say NMI or IRQ along with the vector and
> let the APIC code figure out how to generate it.
> 
> One option is to add a separate set of send_IPI_NMI APIs parallel to
> send_IPI ones that we have. But then we would end with >10 ways to
> generate IPIs.

Yeah, that idea crossed my mind too, and I came to the same conclusion.

> Another way would be to assign the NMI vectors in a different range and
> use the range to differentiate between IRQ and NMI.
> 
> For example:
> 	IRQ => 0x0-0xFF
> 	NMI => 0x10000-0x1000F.
> 
> However, this would still be fragile and probably have similar issues to
> the one you pointed out.
> 
> > 
> > static __always_inline void __apic_send_IPI_self(struct apic_ipi ipi)
> 
> Taking a step back:
> 
> Since we are considering changing the interface, would it be worth
> consolidating the multiple send_IPI APIs into one or two? Mainly, by
> moving the destination information from the function name to the
> function parameter.
> 
>   apic_send_IPI(DEST, MASK, TYPE, VECTOR)
> 
>   DEST   => self, all, allbutself, mask, maskbutself
> 
>   MASK   => cpumask
> 
>   TYPE   => IRQ, NMI
> 
>   VECTOR => Vector number specific to the type.
> 
> I like the single line IPI invocation. All of this can still be passed
> in a neat "struct apic_ipi" with a macro helping the callers fill the
> struct.
> 
> These interfaces are decades old. So, maybe I am being too ambitious and
> this isn't practically feasible. Thoughts/Suggestions?

I suspect making DEST a parameter will be a net negative.  Many (most?) implementations
will likely de-multiplex the DEST on the back end, i.e. the amount of churn will
be roughly the same, and we might end up with *more* code due to multiple
implemenations having to do the fan out.

I think we'd also end up with slightly less readable code in the callers.

> Note: Another part of me says there are only a handful of NMI IPI usages
> and the heavy lifting isn't worth it. We should fix the bugs, improve
> testing and use the existing approach since it is the least invasive :)

FWIW, I think the churn would be worthwhile in the long run.  But I'm also not
volunteering to do said work...

Re: [PATCH v7 05/10] x86/nmi: Assign and register NMI-source vectors

[Sohil Mehta]

On 7/8/2025 12:30 AM, Zhuo, Qiuxu wrote:
>> From: Mehta, Sohil <sohil.mehta@intel.com>
>>
>> We have warnings that should flag any time multiple handlers are registered
>> for SERR or IO_CHK:
>>
>> 	/*
>> 	 * Indicate if there are multiple registrations on the
>> 	 * internal NMI handler call chains (SERR and IO_CHECK).
>> 	 */
>> 	WARN_ON_ONCE(type == NMI_SERR && !list_empty(&desc->head));
>> 	WARN_ON_ONCE(type == NMI_IO_CHECK && !list_empty(&desc-
>>> head));
>>
> 
> The warning doesn't imply that it must be a single NMI handler. 
> Otherwise, why not use a single NMI callback pointer per platform NMI 
> type for registration in the current NMI code? 
> 

You are right, the warning doesn't prevent multiple handlers from being
registered. But, it is also impractical that someone ignores a kernel
warning in production.

> I just don't want your comments or commit messages to 
> mismatch the current code as it stands.
> 

Having precise comments is definitely better. I will reword the comment
to say why NMI-source reporting isn't needed in practice for platform NMIs.

Re: [PATCH v7 08/10] x86/nmi: Enable NMI-source for IPIs delivered as NMIs

[Sohil Mehta]

Hi Thomas, Dave,

Seeking your inputs on the below direction.

On 7/10/2025 3:40 PM, Sean Christopherson wrote:
>>> One thought would be to pass a small struct (by value), and then provide macros
>>> to generate the structure for a specific vector.  That provides some amount of
>>> type safety and should make it a bit harder to pass in garbage, without making
>>> the callers any less readable.
>>>
>>> struct apic_ipi {
>>> 	u8 vector;
>>> 	u8 type;
>>> };
>>>  
>>> #define APIC_IPI(v, t) ({ struct apic_ipi i = { .vector = v, .type = t }; i; })
>>> #define APIC_IPI_IRQ(vector) APIC_IPI(vector, APIC_DELIVERY_MODE_FIXED)
>>> #define APIC_IPI_NMI(vector) APIC_IPI(vector, APIC_DELIVERY_MODE_NMI)
>>> 
>>> #define IPI_IRQ_WORK		APIC_IPI_IRQ(IRQ_WORK_VECTOR)
>>> #define IPI_POSTED_INTR_WAKEUP	APIC_IPI_IRQ(POSTED_INTR_WAKEUP_VECTOR)
>>> 
>>> #define IPI_NMI_BACKTRACE	APIC_IPI_NMI(NMI_BACKTRACE_VECTOR)
>>> 
>>> static __always_inline void __apic_send_IPI_self(struct apic_ipi ipi)
>>
>> I am fine with this approach. Though, the changes would be massive since
>> we have quite a few interfaces and a lot of "struct apic".
> 
> It'd definitely be big, but it doesn't seem like it'd be overwhelmingly painful.
> Though it's certainly enough churn that I wouldn't do anything until there's a
> consensus one way or the other :-)
> 

In order to accommodate NMI source vectors, updating the send_IPI_*()
APIs seems like the right thing to do for the long run. But it would
introduce some churn. Is this the optimal path forward? Any other
options we should consider?