| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <77266075-e90c-4620-a95c-f302be8d0267@intel.com>
Date: Thu, 19 Jun 2025 15:54:39 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Xin Li <xin@...or.com>, "H . Peter Anvin" <hpa@...or.com>, Andy Lutomirski
<luto@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
<mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
<dave.hansen@...ux.intel.com>, Peter Zijlstra <peterz@...radead.org>, "Adrian
Hunter" <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>,
"Tony Luck" <tony.luck@...el.com>, Zhang Rui <rui.zhang@...el.com>, Steven
Rostedt <rostedt@...dmis.org>, Andrew Cooper <andrew.cooper3@...rix.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>, Jacob Pan
<jacob.pan@...ux.microsoft.com>, Andi Kleen <ak@...ux.intel.com>, Kai Huang
<kai.huang@...el.com>, Sandipan Das <sandipan.das@....com>,
<linux-perf-users@...r.kernel.org>, <linux-edac@...r.kernel.org>,
<kvm@...r.kernel.org>, <linux-pm@...r.kernel.org>,
<linux-trace-kernel@...r.kernel.org>, <x86@...nel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 07/10] x86/nmi: Prepare for the new NMI-source vector
encoding
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@...gle.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@...or.com>
> Signed-off-by: Xin Li (Intel) <xin@...or.com>
> Signed-off-by: Sohil Mehta <sohil.mehta@...el.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);
>
>
Powered by blists - more mailing lists