Re: [PATCH v4 09/11] x86/irq: Enable NMI source on IPIs delivered as NMI

[Sean Christopherson <seanjc@google.com>]

On Tue, Jul 09, 2024, Jacob Pan wrote:
> Program designated NMI source vectors for all NMI delivered IPIs
> such that their handlers can be selectively invoked.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
> v4: Enhance comments, no functional changes (Li Xin)
> ---
>  arch/x86/include/asm/irq_vectors.h | 10 ++++++++++
>  arch/x86/kernel/apic/hw_nmi.c      |  3 ++-
>  arch/x86/kernel/apic/ipi.c         |  4 ++--
>  arch/x86/kernel/apic/local.h       | 18 ++++++++++++------
>  arch/x86/kernel/cpu/mce/inject.c   |  2 +-
>  arch/x86/kernel/kgdb.c             |  2 +-
>  arch/x86/kernel/nmi_selftest.c     |  2 +-
>  arch/x86/kernel/reboot.c           |  2 +-
>  arch/x86/kernel/smp.c              |  2 +-
>  9 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
> index 4f767c3940d6..9b7241e7faa3 100644
> --- a/arch/x86/include/asm/irq_vectors.h
> +++ b/arch/x86/include/asm/irq_vectors.h
> @@ -135,6 +135,16 @@
>  #define NMI_SOURCE_VEC_IPI_TEST		8	/* For remote and local IPIs */
>  #define NR_NMI_SOURCE_VECTORS		9
>  
> +/*
> + * When programming the local APIC, IDT NMI vector and NMI-source vector
> + * are encoded in a single 32 bit variable. The top 16 bits contain
> + * the NMI-source vector and the bottom 16 bits contain NMI_VECTOR (2)
> + * The top 16 bits are always zero when NMI-source reporting feature
> + * is not enabled or the caller does not use NMI-source reporting.

This is *extremely* misleading, bordering on being an outright lie.  The vectors
are not encoded in a single 32-bit variable when programming the _local APIC_,
that is 100% an arbitrary software construct.  The actually write to APIC.ICR
morphs bits 15:0 into the TYPE, and the bits 31:16 into the VECTOR.

> + */
> +#define NMI_VECTOR_WITH_SOURCE(src)	(NMI_VECTOR | (src << 16))

Why require callers to use NMI_VECTOR_WITH_SOURCE instead of having macros to
directly encode each source NMI, a la APIC_PERF_NMI?

To me, this:

				__apic_send_IPI(cpu, NMI_VECTOR_WITH_SOURCE(NMI_SOURCE_VEC_IPI_SMP_STOP));

is *way* harder to read/parse than:

				__apic_send_IPI(cpu, NMI_VECTOR_SMP_STOP);

especially since that first one blasts way past 80 chars (yeah, I know 80 is now
a soft limit, but it's still nice to keep line lengths short when possible).

> +#define NMI_SOURCE_VEC_MASK		GENMASK(15, 0)

IMO, this is an absolutely awful encoding scheme.  Vectors are 8-bit values, so
why on earth use 16 bits?  And @vector is passed along as a _signed_ integer,
which means 32-bit kernels could end up observing negative values, which probably
isn't problematic in practice, but it's unnecessarily confusing.

All this FRED stuff is hard enough to follow given the specs have been rolled out
piecemeal (someone at Intel must get paid based on how many specs they publish),
using a software-defined scheme when FRED is already overloading a decades old
hardware-defined encoding is just mean.

Why not encode APIC_DM_NMI straightaway?  You're already making it a hard
requirement that the backend (__prepare_ICR()) be able to handle a @vector that
has bits 31:8!=0.  I don't see how the above scheme provides any value.

Side topic, APIC_DM_FIXED_MASK should really be APIC_DM_MASK, that "FIXED" part
is completely wrong.

E.g.

static inline unsigned int __prepare_ICR(unsigned int shortcut, int vector,
					 unsigned int dest)
{
	unsigned int icr = shortcut | dest;

	/*
	 * Callers are allowed to encode the NMI delivery mode directly, which
	 * allows using the vector field to provide the NMI source (FRED only).
	 */
	if ((vector & APIC_DM_MASK) == APIC_DM_NMI) {
		icr |= vector;

		/*
		 * Pre-FRED, the actual vector is ignored for NMIs, but zero it
		 * if NMI source reporting is unsupported so as not to risk
		 * breakage on misbehaving hardware/hypervisors.
		 */
		if (!cpu_feature_enabled(X86_FEATURE_NMI_SOURCE))
			icr &= ~APIC_VECTOR_MASK;
	} else if (vector == NMI_VECTOR) {
		icr |= APIC_DM_NMI;
	} else {
		icr |= APIC_DM_FIXED | vector;
	}

	return icr;
}

and then NMI_VECTOR_SMP_STOP would be (APIC_DM_NMI | NMI_SOURCE_VEC_IPI_SMP_STOP).