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 hardcoded 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 be zero.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
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 | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index a1d672dcb6f0..183e3e717326 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -127,9 +127,25 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration)
 			    action->handler, duration, decimal_msecs);
 }
 
+/*
+ * Match the NMI-source vector saved during registration with the source
+ * bitmap.
+ *
+ * Always return true when bit 0 of the source bitmap is set.
+ */
+static inline bool match_nmi_source(unsigned long source_bitmap, struct nmiaction *action)
+{
+	unsigned long match_vector;
+
+	match_vector = BIT(NMIS_VECTOR_NONE) | BIT(action->source_vector);
+
+	return (source_bitmap & match_vector);
+}
+
 static int nmi_handle(unsigned int type, struct pt_regs *regs)
 {
 	struct nmi_desc *desc = nmi_to_desc(type);
+	unsigned long source_bitmap = 0;
 	nmi_handler_t ehandler;
 	struct nmiaction *a;
 	int handled=0;
@@ -148,16 +164,40 @@ 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);
+
 	/*
 	 * 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-source information is available, 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 && !match_nmi_source(source_bitmap, a))
+			continue;
+
 		delta = sched_clock();
 		thishandled = a->handler(type, regs);
 		handled += thishandled;
-- 
2.43.0

On Tue, May 06, 2025 at 06:21:41PM -0700, Sohil Mehta wrote:
> 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 hardcoded 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 be zero.
> 
> Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
> ---
> 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 | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index a1d672dcb6f0..183e3e717326 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c

>  static int nmi_handle(unsigned int type, struct pt_regs *regs)
>  {
>  	struct nmi_desc *desc = nmi_to_desc(type);
> +	unsigned long source_bitmap = 0;

	unsigned long source = ~0UL;

>  	nmi_handler_t ehandler;
>  	struct nmiaction *a;
>  	int handled=0;
> @@ -148,16 +164,40 @@ 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);

	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
		source = fred_event_data(regs);
		if (source & BIT(0))
			source = ~0UL;
	}

>  	/*
>  	 * 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-source information is available, 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 && !match_nmi_source(source_bitmap, a))
> +			continue;

		if (!(souce & BIT(a->source_vector)))
			continue;

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

On 5/7/2025 2:14 AM, Peter Zijlstra wrote:
> On Tue, May 06, 2025 at 06:21:41PM -0700, Sohil Mehta wrote:
>>
>> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
>> index a1d672dcb6f0..183e3e717326 100644
>> --- a/arch/x86/kernel/nmi.c
>> +++ b/arch/x86/kernel/nmi.c
> 
>>  static int nmi_handle(unsigned int type, struct pt_regs *regs)
>>  {
>>  	struct nmi_desc *desc = nmi_to_desc(type);
>> +	unsigned long source_bitmap = 0;
> 
> 	unsigned long source = ~0UL;
> 

Thanks! This makes the logic even simpler by getting rid of
match_nmi_source(). A minor change described further down.

Also, do you prefer "source" over "source_bitmap"? I had it as such to
avoid confusion between source_vector and source_bitmap.

>>  	nmi_handler_t ehandler;
>>  	struct nmiaction *a;
>>  	int handled=0;
>> @@ -148,16 +164,40 @@ 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);
> 
> 	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
> 		source = fred_event_data(regs);
> 		if (source & BIT(0))
> 			source = ~0UL;
> 	}
> 

Looks good, except when fred_event_data() returns 0. I don't expect it
to happen in practice. But, maybe with new hardware and eventually
different hypervisors being involved, it is a possibility.

We can either call it a bug that an NMI happened without source
information. Or be extra nice and do this:

if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
	source = fred_event_data(regs);
	if (!source || (source & BIT(0)))
		source = ~0UL;
}

>>  	/*
>>  	 * 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-source information is available, 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 && !match_nmi_source(source_bitmap, a))
>> +			continue;
> 
> 		if (!(souce & BIT(a->source_vector)))
> 			continue;
> 
>>  		delta = sched_clock();
>>  		thishandled = a->handler(type, regs);
>>  		handled += thishandled;

On Wed, May 07, 2025 at 02:48:34PM -0700, Sohil Mehta wrote:
> On 5/7/2025 2:14 AM, Peter Zijlstra wrote:
> > On Tue, May 06, 2025 at 06:21:41PM -0700, Sohil Mehta wrote:
> >>
> >> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> >> index a1d672dcb6f0..183e3e717326 100644
> >> --- a/arch/x86/kernel/nmi.c
> >> +++ b/arch/x86/kernel/nmi.c
> > 
> >>  static int nmi_handle(unsigned int type, struct pt_regs *regs)
> >>  {
> >>  	struct nmi_desc *desc = nmi_to_desc(type);
> >> +	unsigned long source_bitmap = 0;
> > 
> > 	unsigned long source = ~0UL;
> > 
> 
> Thanks! This makes the logic even simpler by getting rid of
> match_nmi_source(). A minor change described further down.
> 
> Also, do you prefer "source" over "source_bitmap"? I had it as such to
> avoid confusion between source_vector and source_bitmap.

Yeah, I was lazy typing. Perhaps just call it bitmap then?

> >>  	nmi_handler_t ehandler;
> >>  	struct nmiaction *a;
> >>  	int handled=0;
> >> @@ -148,16 +164,40 @@ 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);
> > 
> > 	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
> > 		source = fred_event_data(regs);
> > 		if (source & BIT(0))
> > 			source = ~0UL;
> > 	}
> > 
> 
> Looks good, except when fred_event_data() returns 0. I don't expect it
> to happen in practice. But, maybe with new hardware and eventually
> different hypervisors being involved, it is a possibility.
> 
> We can either call it a bug that an NMI happened without source
> information. Or be extra nice and do this:
> 
> if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
> 	source = fred_event_data(regs);
> 	if (!source || (source & BIT(0)))
> 		source = ~0UL;
> }

Perhaps also WARN about the !source case?

On May 8, 2025 5:15:44 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Wed, May 07, 2025 at 02:48:34PM -0700, Sohil Mehta wrote:
>> On 5/7/2025 2:14 AM, Peter Zijlstra wrote:
>> > On Tue, May 06, 2025 at 06:21:41PM -0700, Sohil Mehta wrote:
>> >>
>> >> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
>> >> index a1d672dcb6f0..183e3e717326 100644
>> >> --- a/arch/x86/kernel/nmi.c
>> >> +++ b/arch/x86/kernel/nmi.c
>> > 
>> >>  static int nmi_handle(unsigned int type, struct pt_regs *regs)
>> >>  {
>> >>  	struct nmi_desc *desc = nmi_to_desc(type);
>> >> +	unsigned long source_bitmap = 0;
>> > 
>> > 	unsigned long source = ~0UL;
>> > 
>> 
>> Thanks! This makes the logic even simpler by getting rid of
>> match_nmi_source(). A minor change described further down.
>> 
>> Also, do you prefer "source" over "source_bitmap"? I had it as such to
>> avoid confusion between source_vector and source_bitmap.
>
>Yeah, I was lazy typing. Perhaps just call it bitmap then?
>
>> >>  	nmi_handler_t ehandler;
>> >>  	struct nmiaction *a;
>> >>  	int handled=0;
>> >> @@ -148,16 +164,40 @@ 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);
>> > 
>> > 	if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
>> > 		source = fred_event_data(regs);
>> > 		if (source & BIT(0))
>> > 			source = ~0UL;
>> > 	}
>> > 
>> 
>> Looks good, except when fred_event_data() returns 0. I don't expect it
>> to happen in practice. But, maybe with new hardware and eventually
>> different hypervisors being involved, it is a possibility.
>> 
>> We can either call it a bug that an NMI happened without source
>> information. Or be extra nice and do this:
>> 
>> if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
>> 	source = fred_event_data(regs);
>> 	if (!source || (source & BIT(0)))
>> 		source = ~0UL;
>> }
>
>Perhaps also WARN about the !source case?

A 0 should be interpreted such that NMI source is not available, e.g. due to a broken hypervisor or similar.

On Thu, May 08, 2025 at 01:23:04PM -0700, H. Peter Anvin wrote:
> On May 8, 2025 5:15:44 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:

> >> Looks good, except when fred_event_data() returns 0. I don't expect it
> >> to happen in practice. But, maybe with new hardware and eventually
> >> different hypervisors being involved, it is a possibility.
> >> 
> >> We can either call it a bug that an NMI happened without source
> >> information. Or be extra nice and do this:
> >> 
> >> if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
> >> 	source = fred_event_data(regs);
> >> 	if (!source || (source & BIT(0)))
> >> 		source = ~0UL;
> >> }
> >
> >Perhaps also WARN about the !source case?
> 
> A 0 should be interpreted such that NMI source is not available, e.g.
> due to a broken hypervisor or similar.

I'm reading that as an agreement for WARN-ing on 0. We should definitely
WARN on broken hypervisors and similar.

On 5/8/2025 1:49 PM, Peter Zijlstra wrote:
> On Thu, May 08, 2025 at 01:23:04PM -0700, H. Peter Anvin wrote:
>> On May 8, 2025 5:15:44 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
> 
>>>> Looks good, except when fred_event_data() returns 0. I don't expect it
>>>> to happen in practice. But, maybe with new hardware and eventually
>>>> different hypervisors being involved, it is a possibility.
>>>>
>>>> We can either call it a bug that an NMI happened without source
>>>> information. Or be extra nice and do this:
>>>>
>>>> if (cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) && type == NMI_LOCAL) {
>>>> 	source = fred_event_data(regs);
>>>> 	if (!source || (source & BIT(0)))
>>>> 		source = ~0UL;
>>>> }
>>>
>>> Perhaps also WARN about the !source case?
>>
>> A 0 should be interpreted such that NMI source is not available, e.g.
>> due to a broken hypervisor or similar.
> 
> I'm reading that as an agreement for WARN-ing on 0. We should definitely
> WARN on broken hypervisors and similar.

Yup, will do.