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

[Sean Christopherson <seanjc@google.com>]

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)