[PATCH v2 00/12] x86/msr: Inline rdmsr/wrmsr instructions

[PATCH v2 00/12] x86/msr: Inline rdmsr/wrmsr instructions

[Juergen Gross]

When building a kernel with CONFIG_PARAVIRT_XXL the paravirt
infrastructure will always use functions for reading or writing MSRs,
even when running on bare metal.

Switch to inline RDMSR/WRMSR instructions in this case, reducing the
paravirt overhead.

In order to make this less intrusive, some further reorganization of
the MSR access helpers is done in the first 5 patches.

The next 5 patches are converting the non-paravirt case to use direct
inlining of the MSR access instructions, including the WRMSRNS
instruction and the immediate variants of RDMSR and WRMSR if possible.

Patch 11 removes the PV hooks for MSR accesses and implements the
Xen PV cases via calls depending on X86_FEATURE_XENPV, which results
in runtime patching those calls away for the non-XenPV case.

Patch 12 is a final little cleanup patch.

This series has been tested to work with Xen PV and on bare metal.

This series is inspired by Xin Li, who used a similar approach, but
(in my opinion) with some flaws. Originally I thought it should be
possible to use the paravirt infrastructure, but this turned out to be
rather complicated, especially for the Xen PV case in the *_safe()
variants of the MSR access functions.

Changes since V1:
- Use Xin Li's approach for inlining
- Several new patches

Juergen Gross (9):
  coco/tdx: Rename MSR access helpers
  x86/sev: replace call of native_wrmsr() with native_wrmsrq()
  x86/kvm: Remove the KVM private read_msr() function
  x86/msr: minimize usage of native_*() msr access functions
  x86/msr: Move MSR trace calls one function level up
  x86/msr: Use the alternatives mechanism for WRMSR
  x86/msr: Use the alternatives mechanism for RDMSR
  x86/paravirt: Don't use pv_ops vector for MSR access functions
  x86/msr: Reduce number of low level MSR access helpers

Xin Li (Intel) (3):
  x86/cpufeatures: Add a CPU feature bit for MSR immediate form
    instructions
  x86/opcode: Add immediate form MSR instructions
  x86/extable: Add support for immediate form MSR instructions

 arch/x86/coco/tdx/tdx.c               |   8 +-
 arch/x86/hyperv/ivm.c                 |   2 +-
 arch/x86/include/asm/cpufeatures.h    |   1 +
 arch/x86/include/asm/fred.h           |   2 +-
 arch/x86/include/asm/kvm_host.h       |  10 -
 arch/x86/include/asm/msr.h            | 409 +++++++++++++++++++-------
 arch/x86/include/asm/paravirt.h       |  67 -----
 arch/x86/include/asm/paravirt_types.h |  13 -
 arch/x86/include/asm/sev-internal.h   |   7 +-
 arch/x86/kernel/cpu/scattered.c       |   1 +
 arch/x86/kernel/kvmclock.c            |   2 +-
 arch/x86/kernel/paravirt.c            |   5 -
 arch/x86/kvm/svm/svm.c                |  16 +-
 arch/x86/kvm/vmx/vmx.c                |   4 +-
 arch/x86/lib/x86-opcode-map.txt       |   5 +-
 arch/x86/mm/extable.c                 |  39 ++-
 arch/x86/xen/enlighten_pv.c           |  24 +-
 arch/x86/xen/pmu.c                    |   5 +-
 tools/arch/x86/lib/x86-opcode-map.txt |   5 +-
 19 files changed, 383 insertions(+), 242 deletions(-)

-- 
2.51.0

[PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Juergen Gross]

When available use one of the non-serializing WRMSR variants (WRMSRNS
with or without an immediate operand specifying the MSR register) in
__wrmsrq().

For the safe/unsafe variants make __wrmsrq() to be a common base
function instead of duplicating the ALTERNATIVE*() macros. This
requires to let native_wrmsr() use native_wrmsrq() instead of
__wrmsrq(). While changing this, convert native_wrmsr() into an inline
function.

Replace the only call of wsrmsrns() with the now equivalent call to
native_wrmsrq() and remove wsrmsrns().

The paravirt case will be handled later.

Originally-by: Xin Li (Intel) <xin@zytor.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch, partially taken from "[RFC PATCH v2 21/34] x86/msr: Utilize
  the alternatives mechanism to write MSR" by Xin Li.
---
 arch/x86/include/asm/fred.h |   2 +-
 arch/x86/include/asm/msr.h  | 144 +++++++++++++++++++++++++++---------
 2 files changed, 110 insertions(+), 36 deletions(-)

diff --git a/arch/x86/include/asm/fred.h b/arch/x86/include/asm/fred.h
index 12b34d5b2953..8ae4429e5401 100644
--- a/arch/x86/include/asm/fred.h
+++ b/arch/x86/include/asm/fred.h
@@ -101,7 +101,7 @@ static __always_inline void fred_update_rsp0(void)
 	unsigned long rsp0 = (unsigned long) task_stack_page(current) + THREAD_SIZE;
 
 	if (cpu_feature_enabled(X86_FEATURE_FRED) && (__this_cpu_read(fred_rsp0) != rsp0)) {
-		wrmsrns(MSR_IA32_FRED_RSP0, rsp0);
+		native_wrmsrq(MSR_IA32_FRED_RSP0, rsp0);
 		__this_cpu_write(fred_rsp0, rsp0);
 	}
 }
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index cf5205300266..19ed780c2a09 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -7,11 +7,11 @@
 #ifndef __ASSEMBLER__
 
 #include <asm/asm.h>
-#include <asm/errno.h>
 #include <asm/cpumask.h>
 #include <uapi/asm/msr.h>
 #include <asm/shared/msr.h>
 
+#include <linux/errno.h>
 #include <linux/types.h>
 #include <linux/percpu.h>
 
@@ -56,6 +56,36 @@ static inline void do_trace_read_msr(u32 msr, u64 val, int failed) {}
 static inline void do_trace_rdpmc(u32 msr, u64 val, int failed) {}
 #endif
 
+/* The GNU Assembler (Gas) with Binutils 2.40 adds WRMSRNS support */
+#if defined(CONFIG_AS_IS_GNU) && CONFIG_AS_VERSION >= 24000
+#define ASM_WRMSRNS		"wrmsrns"
+#else
+#define ASM_WRMSRNS		_ASM_BYTES(0x0f,0x01,0xc6)
+#endif
+
+/* The GNU Assembler (Gas) with Binutils 2.41 adds the .insn directive support */
+#if defined(CONFIG_AS_IS_GNU) && CONFIG_AS_VERSION >= 24100
+#define ASM_WRMSRNS_IMM			\
+	" .insn VEX.128.F3.M7.W0 0xf6 /0, %[val], %[msr]%{:u32}\n\t"
+#else
+/*
+ * Note, clang also doesn't support the .insn directive.
+ *
+ * The register operand is encoded as %rax because all uses of the immediate
+ * form MSR access instructions reference %rax as the register operand.
+ */
+#define ASM_WRMSRNS_IMM			\
+	" .byte 0xc4,0xe7,0x7a,0xf6,0xc0; .long %c[msr]"
+#endif
+
+#define PREPARE_RDX_FOR_WRMSR		\
+	"mov %%rax, %%rdx\n\t"		\
+	"shr $0x20, %%rdx\n\t"
+
+#define PREPARE_RCX_RDX_FOR_WRMSR	\
+	"mov %[msr], %%ecx\n\t"		\
+	PREPARE_RDX_FOR_WRMSR
+
 /*
  * Called only from an MSR fault handler, the instruction pointer points to
  * the MSR access instruction that caused the fault.
@@ -93,12 +123,76 @@ static __always_inline u64 __rdmsr(u32 msr)
 	return EAX_EDX_VAL(val, low, high);
 }
 
-static __always_inline void __wrmsrq(u32 msr, u64 val)
+static __always_inline bool __wrmsrq_variable(u32 msr, u64 val, int type)
 {
-	asm volatile("1: wrmsr\n"
-		     "2:\n"
-		     _ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_WRMSR)
-		     : : "c" (msr), "a" ((u32)val), "d" ((u32)(val >> 32)) : "memory");
+#ifdef CONFIG_X86_64
+	BUILD_BUG_ON(__builtin_constant_p(msr));
+#endif
+
+	/*
+	 * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant
+	 * DS prefix to avoid a trailing NOP.
+	 */
+	asm_inline volatile goto(
+		"1:\n"
+		ALTERNATIVE("ds wrmsr",
+			    ASM_WRMSRNS,
+			    X86_FEATURE_WRMSRNS)
+		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])
+
+		:
+		: "c" (msr), "a" ((u32)val), "d" ((u32)(val >> 32)), [type] "i" (type)
+		: "memory"
+		: badmsr);
+
+	return false;
+
+badmsr:
+	return true;
+}
+
+#ifdef CONFIG_X86_64
+/*
+ * Non-serializing WRMSR or its immediate form, when available.
+ *
+ * Otherwise, it falls back to a serializing WRMSR.
+ */
+static __always_inline bool __wrmsrq_constant(u32 msr, u64 val, int type)
+{
+	BUILD_BUG_ON(!__builtin_constant_p(msr));
+
+	asm_inline volatile goto(
+		"1:\n"
+		ALTERNATIVE_2(PREPARE_RCX_RDX_FOR_WRMSR
+			      "2: ds wrmsr",
+			      PREPARE_RCX_RDX_FOR_WRMSR
+			      ASM_WRMSRNS,
+			      X86_FEATURE_WRMSRNS,
+			      ASM_WRMSRNS_IMM,
+			      X86_FEATURE_MSR_IMM)
+		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
+		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
+
+		:
+		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
+		: "memory", "ecx", "rdx"
+		: badmsr);
+
+	return false;
+
+badmsr:
+	return true;
+}
+#endif
+
+static __always_inline bool __wrmsrq(u32 msr, u64 val, int type)
+{
+#ifdef CONFIG_X86_64
+	if (__builtin_constant_p(msr))
+		return __wrmsrq_constant(msr, val, type);
+#endif
+
+	return __wrmsrq_variable(msr, val, type);
 }
 
 #define native_rdmsr(msr, val1, val2)			\
@@ -113,11 +207,15 @@ static __always_inline u64 native_rdmsrq(u32 msr)
 	return __rdmsr(msr);
 }
 
-#define native_wrmsr(msr, low, high)			\
-	__wrmsrq((msr), (u64)(high) << 32 | (low))
+static __always_inline void native_wrmsrq(u32 msr, u64 val)
+{
+	__wrmsrq(msr, val, EX_TYPE_WRMSR);
+}
 
-#define native_wrmsrq(msr, val)				\
-	__wrmsrq((msr), (val))
+static __always_inline void native_wrmsr(u32 msr, u32 low, u32 high)
+{
+	native_wrmsrq(msr, (u64)high << 32 | low);
+}
 
 static inline u64 native_read_msr(u32 msr)
 {
@@ -149,15 +247,7 @@ static inline void notrace native_write_msr(u32 msr, u64 val)
 /* Can be uninlined because referenced by paravirt */
 static inline int notrace native_write_msr_safe(u32 msr, u64 val)
 {
-	int err;
-
-	asm volatile("1: wrmsr ; xor %[err],%[err]\n"
-		     "2:\n\t"
-		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_WRMSR_SAFE, %[err])
-		     : [err] "=a" (err)
-		     : "c" (msr), "0" ((u32)val), "d" ((u32)(val >> 32))
-		     : "memory");
-	return err;
+	return __wrmsrq(msr, val, EX_TYPE_WRMSR_SAFE) ? -EIO : 0;
 }
 
 extern int rdmsr_safe_regs(u32 regs[8]);
@@ -176,7 +266,6 @@ static inline u64 native_read_pmc(int counter)
 #ifdef CONFIG_PARAVIRT_XXL
 #include <asm/paravirt.h>
 #else
-#include <linux/errno.h>
 static __always_inline u64 read_msr(u32 msr)
 {
 	return native_read_msr(msr);
@@ -268,21 +357,6 @@ static inline int wrmsrq_safe(u32 msr, u64 val)
 	return err;
 }
 
-/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
-#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
-
-/* Non-serializing WRMSR, when available.  Falls back to a serializing WRMSR. */
-static __always_inline void wrmsrns(u32 msr, u64 val)
-{
-	/*
-	 * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant
-	 * DS prefix to avoid a trailing NOP.
-	 */
-	asm volatile("1: " ALTERNATIVE("ds wrmsr", ASM_WRMSRNS, X86_FEATURE_WRMSRNS)
-		     "2: " _ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_WRMSR)
-		     : : "c" (msr), "a" ((u32)val), "d" ((u32)(val >> 32)));
-}
-
 static inline void wrmsr(u32 msr, u32 low, u32 high)
 {
 	wrmsrq(msr, (u64)high << 32 | low);
-- 
2.51.0

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Peter Zijlstra]

On Tue, Sep 30, 2025 at 09:03:53AM +0200, Juergen Gross wrote:

> +static __always_inline bool __wrmsrq_constant(u32 msr, u64 val, int type)
> +{
> +	BUILD_BUG_ON(!__builtin_constant_p(msr));
> +
> +	asm_inline volatile goto(
> +		"1:\n"
> +		ALTERNATIVE_2(PREPARE_RCX_RDX_FOR_WRMSR
> +			      "2: ds wrmsr",
> +			      PREPARE_RCX_RDX_FOR_WRMSR
> +			      ASM_WRMSRNS,
> +			      X86_FEATURE_WRMSRNS,
> +			      ASM_WRMSRNS_IMM,
> +			      X86_FEATURE_MSR_IMM)
> +		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
> +		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
> +
> +		:
> +		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
> +		: "memory", "ecx", "rdx"
> +		: badmsr);
> +
> +	return false;
> +
> +badmsr:
> +	return true;
> +}

Just wondering, would something this work?

	asm_inline volatile goto(
		"1:\n"
		ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
			    "2:\n"
			    ALTERNATIVE("ds wrmsr",
			                ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
			    ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */

		:
		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
		: "memory", "ecx", "rdx"
		: badmsr);

Its a bit weird because the nested alternative isn't for the exact same
position I suppose. But I find it a more readable form.

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2113 bytes --]

On 30.09.25 10:31, Peter Zijlstra wrote:
> On Tue, Sep 30, 2025 at 09:03:53AM +0200, Juergen Gross wrote:
> 
>> +static __always_inline bool __wrmsrq_constant(u32 msr, u64 val, int type)
>> +{
>> +	BUILD_BUG_ON(!__builtin_constant_p(msr));
>> +
>> +	asm_inline volatile goto(
>> +		"1:\n"
>> +		ALTERNATIVE_2(PREPARE_RCX_RDX_FOR_WRMSR
>> +			      "2: ds wrmsr",
>> +			      PREPARE_RCX_RDX_FOR_WRMSR
>> +			      ASM_WRMSRNS,
>> +			      X86_FEATURE_WRMSRNS,
>> +			      ASM_WRMSRNS_IMM,
>> +			      X86_FEATURE_MSR_IMM)
>> +		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
>> +		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
>> +
>> +		:
>> +		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
>> +		: "memory", "ecx", "rdx"
>> +		: badmsr);
>> +
>> +	return false;
>> +
>> +badmsr:
>> +	return true;
>> +}
> 
> Just wondering, would something this work?
> 
> 	asm_inline volatile goto(
> 		"1:\n"
> 		ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
> 			    "2:\n"
> 			    ALTERNATIVE("ds wrmsr",
> 			                ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
> 			    ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
> 		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
> 		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
> 
> 		:
> 		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
> 		: "memory", "ecx", "rdx"
> 		: badmsr);
> 
> Its a bit weird because the nested alternative isn't for the exact same
> position I suppose. But I find it a more readable form.

I don't think it would work. Nested ALTERNATIVE()s do work only with
all of them starting at the same location. Have a look at the
ALTERNATIVE() macro, which is defining the label "771" via OLDINSTR()
and then referring to this label via ALTINSTR_ENTRY(). In your case
the ALTINSTR_ENTRY() of the outer ALTERNATIVE() invocation would find
the wrong "771" label (the one of the inner ALTERNATIVE()).

Allowing such constructs would probably require switching from preprocessor
macros to assembler macros.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Peter Zijlstra]

On Tue, Sep 30, 2025 at 10:46:23AM +0200, Jürgen Groß wrote:
> On 30.09.25 10:31, Peter Zijlstra wrote:
> > On Tue, Sep 30, 2025 at 09:03:53AM +0200, Juergen Gross wrote:
> > 
> > > +static __always_inline bool __wrmsrq_constant(u32 msr, u64 val, int type)
> > > +{
> > > +	BUILD_BUG_ON(!__builtin_constant_p(msr));
> > > +
> > > +	asm_inline volatile goto(
> > > +		"1:\n"
> > > +		ALTERNATIVE_2(PREPARE_RCX_RDX_FOR_WRMSR
> > > +			      "2: ds wrmsr",
> > > +			      PREPARE_RCX_RDX_FOR_WRMSR
> > > +			      ASM_WRMSRNS,
> > > +			      X86_FEATURE_WRMSRNS,
> > > +			      ASM_WRMSRNS_IMM,
> > > +			      X86_FEATURE_MSR_IMM)
> > > +		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
> > > +		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
> > > +
> > > +		:
> > > +		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
> > > +		: "memory", "ecx", "rdx"
> > > +		: badmsr);
> > > +
> > > +	return false;
> > > +
> > > +badmsr:
> > > +	return true;
> > > +}
> > 
> > Just wondering, would something this work?
> > 
> > 	asm_inline volatile goto(
> > 		"1:\n"
> > 		ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
> > 			    "2:\n"
> > 			    ALTERNATIVE("ds wrmsr",
> > 			                ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
> > 			    ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
> > 		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
> > 		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
> > 
> > 		:
> > 		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
> > 		: "memory", "ecx", "rdx"
> > 		: badmsr);
> > 
> > Its a bit weird because the nested alternative isn't for the exact same
> > position I suppose. But I find it a more readable form.
> 
> I don't think it would work. Nested ALTERNATIVE()s do work only with
> all of them starting at the same location. Have a look at the
> ALTERNATIVE() macro, which is defining the label "771" via OLDINSTR()
> and then referring to this label via ALTINSTR_ENTRY(). In your case
> the ALTINSTR_ENTRY() of the outer ALTERNATIVE() invocation would find
> the wrong "771" label (the one of the inner ALTERNATIVE()).
> 
> Allowing such constructs would probably require switching from preprocessor
> macros to assembler macros.

Right, I was looking at the asm macros. 

As long as the inner comes first in the apply list it should all just
work, except you get the double patching back.

Using the asm macros isn't going to make it more readable though. 
Oh well, lets forget about this :-)

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Peter Zijlstra]

On Tue, Sep 30, 2025 at 10:50:44AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 30, 2025 at 10:46:23AM +0200, Jürgen Groß wrote:

> > > 	asm_inline volatile goto(
> > > 		"1:\n"
> > > 		ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
> > > 			    "2:\n"
> > > 			    ALTERNATIVE("ds wrmsr",
> > > 			                ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
> > > 			    ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
> > > 		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
> > > 		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
> > > 
> > > 		:
> > > 		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
> > > 		: "memory", "ecx", "rdx"
> > > 		: badmsr);
> > > 

> Oh well, lets forget about this :-)

So I couldn't. I tried the below, which when building a .i generates the
following:


static inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((no_instrument_function)) void clear_page(void *page)
{

 kmsan_unpoison_memory(page, ((1UL) << 12));
 asm __inline volatile(
 	"# ALT: oldinstr\n"
	"__UNIQUE_ID_altinstr_9" "_begin:\n\t" 

		"# ALT: oldinstr\n"
		"__UNIQUE_ID_altinstr_8" "_begin:\n\t"
		"call %c[old]" "\n"
		"__UNIQUE_ID_altinstr_8" "_pad:\n"
		"# ALT: padding\n"
		".skip -(((" "__UNIQUE_ID_altinstr_8" "_alt_end - " "__UNIQUE_ID_altinstr_8" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_8" "_pad - " "__UNIQUE_ID_altinstr_8" "_begin" ")) > 0) * " 
		"((" "__UNIQUE_ID_altinstr_8" "_alt_end - " "__UNIQUE_ID_altinstr_8" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_8" "_pad - " "__UNIQUE_ID_altinstr_8" "_begin" ")),0x90\n" 
		"__UNIQUE_ID_altinstr_8" "_end:\n"
		".pushsection .altinstructions,\"a\"\n"
		" .long " "__UNIQUE_ID_altinstr_8" "_begin - .\n"
		" .long " "__UNIQUE_ID_altinstr_8" "_alt_begin - .\n" 
		" .4byte " "( 3*32+16)" "\n" 
		" .byte " "__UNIQUE_ID_altinstr_8" "_end - " "__UNIQUE_ID_altinstr_8" "_begin" "\n" 
		" .byte " "__UNIQUE_ID_altinstr_8" "_alt_end - " "__UNIQUE_ID_altinstr_8" "_alt_begin" "\n" 
		".popsection\n" 
		".pushsection .altinstr_replacement, \"ax\"\n" 
		"# ALT: replacement\n" 
		"__UNIQUE_ID_altinstr_8" "_alt_begin:\n\t" 
		"call %c[new1]" "\n" 
		"__UNIQUE_ID_altinstr_8" "_alt_end:\n" 
		".popsection\n"
		"\n" 

	"__UNIQUE_ID_altinstr_9" "_pad:\n" 
	"# ALT: padding\n" 
	".skip -(((" "__UNIQUE_ID_altinstr_9" "_alt_end - " "__UNIQUE_ID_altinstr_9" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_9" "_pad - " "__UNIQUE_ID_altinstr_9" "_begin" ")) > 0) * "
	"((" "__UNIQUE_ID_altinstr_9" "_alt_end - " "__UNIQUE_ID_altinstr_9" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_9" "_pad - " "__UNIQUE_ID_altinstr_9" "_begin" ")),0x90\n" 
	"__UNIQUE_ID_altinstr_9" "_end:\n" 
	".pushsection .altinstructions,\"a\"\n" 
	" .long " "__UNIQUE_ID_altinstr_9" "_begin - .\n"
	" .long " "__UNIQUE_ID_altinstr_9" "_alt_begin - .\n" 
	" .4byte " "( 9*32+ 9)" "\n" 
	" .byte " "__UNIQUE_ID_altinstr_9" "_end - " "__UNIQUE_ID_altinstr_9" "_begin" "\n"
	" .byte " "__UNIQUE_ID_altinstr_9" "_alt_end - " "__UNIQUE_ID_altinstr_9" "_alt_begin" "\n"
	".popsection\n" 
	".pushsection .altinstr_replacement, \"ax\"\n" 
	"# ALT: replacement\n" 
	"__UNIQUE_ID_altinstr_9" "_alt_begin:\n\t" 
	"call %c[new2]" "\n" 
	"__UNIQUE_ID_altinstr_9" "_alt_end:\n" 
	".popsection\n" 
	: "+r" (current_stack_pointer), "=D" (page) 
	: [old] "i" (clear_page_orig), [new1] "i" (clear_page_rep), [new2] "i" (clear_page_erms) , "D" (page) 
	: "cc", "memory", "rax", "rcx")

                                   ;
}

Which looks right, but utterly fails to build :(


diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 15bc07a5ebb3..12a93982457a 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -5,6 +5,7 @@
 #include <linux/types.h>
 #include <linux/stringify.h>
 #include <linux/objtool.h>
+#include <linux/compiler.h>
 #include <asm/asm.h>
 #include <asm/bug.h>
 
@@ -184,38 +185,41 @@ static inline int alternatives_text_reserved(void *start, void *end)
 
 #define ALT_CALL_INSTR		"call BUG_func"
 
-#define alt_slen		"772b-771b"
-#define alt_total_slen		"773b-771b"
-#define alt_rlen		"775f-774f"
+#define alt_slen(pfx)		#pfx "_pad - " #pfx "_begin"
+#define alt_total_slen(pfx)	#pfx "_end - " #pfx "_begin"
+#define alt_rlen(pfx)		#pfx "_alt_end - " #pfx "_alt_begin"
 
-#define OLDINSTR(oldinstr)						\
+#define OLDINSTR(oldinstr, pfx)						\
 	"# ALT: oldinstr\n"						\
-	"771:\n\t" oldinstr "\n772:\n"					\
+	#pfx "_begin:\n\t" oldinstr "\n" #pfx "_pad:\n"			\
 	"# ALT: padding\n"						\
-	".skip -(((" alt_rlen ")-(" alt_slen ")) > 0) * "		\
-		"((" alt_rlen ")-(" alt_slen ")),0x90\n"		\
-	"773:\n"
+	".skip -(((" alt_rlen(pfx) ")-(" alt_slen(pfx) ")) > 0) * "	\
+		"((" alt_rlen(pfx) ")-(" alt_slen(pfx) ")),0x90\n"	\
+	#pfx "_end:\n"
 
-#define ALTINSTR_ENTRY(ft_flags)					      \
+#define ALTINSTR_ENTRY(ft_flags, pfx)					      \
 	".pushsection .altinstructions,\"a\"\n"				      \
-	" .long 771b - .\n"				/* label           */ \
-	" .long 774f - .\n"				/* new instruction */ \
+	" .long " #pfx "_begin - .\n"			/* label           */ \
+	" .long " #pfx "_alt_begin - .\n"			/* new instruction */ \
 	" .4byte " __stringify(ft_flags) "\n"		/* feature + flags */ \
-	" .byte " alt_total_slen "\n"			/* source len      */ \
-	" .byte " alt_rlen "\n"				/* replacement len */ \
+	" .byte " alt_total_slen(pfx) "\n"		/* source len      */ \
+	" .byte " alt_rlen(pfx) "\n"			/* replacement len */ \
 	".popsection\n"
 
-#define ALTINSTR_REPLACEMENT(newinstr)		/* replacement */	\
+#define ALTINSTR_REPLACEMENT(newinstr, pfx)	/* replacement */	\
 	".pushsection .altinstr_replacement, \"ax\"\n"			\
 	"# ALT: replacement\n"						\
-	"774:\n\t" newinstr "\n775:\n"					\
+	#pfx "_alt_begin:\n\t" newinstr "\n" #pfx "_alt_end:\n"			\
 	".popsection\n"
 
 /* alternative assembly primitive: */
-#define ALTERNATIVE(oldinstr, newinstr, ft_flags)			\
-	OLDINSTR(oldinstr)						\
-	ALTINSTR_ENTRY(ft_flags)					\
-	ALTINSTR_REPLACEMENT(newinstr)
+#define __ALTERNATIVE(oldinstr, newinstr, ft_flags, pfx)		\
+	OLDINSTR(oldinstr, pfx)						\
+	ALTINSTR_ENTRY(ft_flags, pfx)					\
+	ALTINSTR_REPLACEMENT(newinstr, pfx)
+
+#define ALTERNATIVE(oldinstr, newinstr, feat)				\
+	__ALTERNATIVE(oldinstr, newinstr, feat, __UNIQUE_ID(altinstr))
 
 #define ALTERNATIVE_2(oldinstr, newinstr1, ft_flags1, newinstr2, ft_flags2) \
 	ALTERNATIVE(ALTERNATIVE(oldinstr, newinstr1, ft_flags1), newinstr2, ft_flags2)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 64ff73c533e5..d79552a61fda 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -163,7 +163,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	__asm__ ("" : "=r" (var) : "0" (var))
 #endif
 
-#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
+#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__PASTE(__UNIQUE_ID_, prefix), _), __COUNTER__)
 
 /**
  * data_race - mark an expression as containing intentional data races

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3829 bytes --]

On 30.09.25 14:51, Peter Zijlstra wrote:
> On Tue, Sep 30, 2025 at 10:50:44AM +0200, Peter Zijlstra wrote:
>> On Tue, Sep 30, 2025 at 10:46:23AM +0200, Jürgen Groß wrote:
> 
>>>> 	asm_inline volatile goto(
>>>> 		"1:\n"
>>>> 		ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
>>>> 			    "2:\n"
>>>> 			    ALTERNATIVE("ds wrmsr",
>>>> 			                ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
>>>> 			    ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
>>>> 		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
>>>> 		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
>>>>
>>>> 		:
>>>> 		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
>>>> 		: "memory", "ecx", "rdx"
>>>> 		: badmsr);
>>>>
> 
>> Oh well, lets forget about this :-)
> 
> So I couldn't. I tried the below, which when building a .i generates the
> following:
> 
> 
> static inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((no_instrument_function)) void clear_page(void *page)
> {
> 
>   kmsan_unpoison_memory(page, ((1UL) << 12));
>   asm __inline volatile(
>   	"# ALT: oldinstr\n"
> 	"__UNIQUE_ID_altinstr_9" "_begin:\n\t"
> 
> 		"# ALT: oldinstr\n"
> 		"__UNIQUE_ID_altinstr_8" "_begin:\n\t"
> 		"call %c[old]" "\n"
> 		"__UNIQUE_ID_altinstr_8" "_pad:\n"
> 		"# ALT: padding\n"
> 		".skip -(((" "__UNIQUE_ID_altinstr_8" "_alt_end - " "__UNIQUE_ID_altinstr_8" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_8" "_pad - " "__UNIQUE_ID_altinstr_8" "_begin" ")) > 0) * "
> 		"((" "__UNIQUE_ID_altinstr_8" "_alt_end - " "__UNIQUE_ID_altinstr_8" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_8" "_pad - " "__UNIQUE_ID_altinstr_8" "_begin" ")),0x90\n"
> 		"__UNIQUE_ID_altinstr_8" "_end:\n"
> 		".pushsection .altinstructions,\"a\"\n"
> 		" .long " "__UNIQUE_ID_altinstr_8" "_begin - .\n"
> 		" .long " "__UNIQUE_ID_altinstr_8" "_alt_begin - .\n"
> 		" .4byte " "( 3*32+16)" "\n"
> 		" .byte " "__UNIQUE_ID_altinstr_8" "_end - " "__UNIQUE_ID_altinstr_8" "_begin" "\n"
> 		" .byte " "__UNIQUE_ID_altinstr_8" "_alt_end - " "__UNIQUE_ID_altinstr_8" "_alt_begin" "\n"
> 		".popsection\n"
> 		".pushsection .altinstr_replacement, \"ax\"\n"
> 		"# ALT: replacement\n"
> 		"__UNIQUE_ID_altinstr_8" "_alt_begin:\n\t"
> 		"call %c[new1]" "\n"
> 		"__UNIQUE_ID_altinstr_8" "_alt_end:\n"
> 		".popsection\n"
> 		"\n"
> 
> 	"__UNIQUE_ID_altinstr_9" "_pad:\n"
> 	"# ALT: padding\n"
> 	".skip -(((" "__UNIQUE_ID_altinstr_9" "_alt_end - " "__UNIQUE_ID_altinstr_9" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_9" "_pad - " "__UNIQUE_ID_altinstr_9" "_begin" ")) > 0) * "
> 	"((" "__UNIQUE_ID_altinstr_9" "_alt_end - " "__UNIQUE_ID_altinstr_9" "_alt_begin" ")-(" "__UNIQUE_ID_altinstr_9" "_pad - " "__UNIQUE_ID_altinstr_9" "_begin" ")),0x90\n"
> 	"__UNIQUE_ID_altinstr_9" "_end:\n"
> 	".pushsection .altinstructions,\"a\"\n"
> 	" .long " "__UNIQUE_ID_altinstr_9" "_begin - .\n"
> 	" .long " "__UNIQUE_ID_altinstr_9" "_alt_begin - .\n"
> 	" .4byte " "( 9*32+ 9)" "\n"
> 	" .byte " "__UNIQUE_ID_altinstr_9" "_end - " "__UNIQUE_ID_altinstr_9" "_begin" "\n"
> 	" .byte " "__UNIQUE_ID_altinstr_9" "_alt_end - " "__UNIQUE_ID_altinstr_9" "_alt_begin" "\n"
> 	".popsection\n"
> 	".pushsection .altinstr_replacement, \"ax\"\n"
> 	"# ALT: replacement\n"
> 	"__UNIQUE_ID_altinstr_9" "_alt_begin:\n\t"
> 	"call %c[new2]" "\n"
> 	"__UNIQUE_ID_altinstr_9" "_alt_end:\n"
> 	".popsection\n"
> 	: "+r" (current_stack_pointer), "=D" (page)
> 	: [old] "i" (clear_page_orig), [new1] "i" (clear_page_rep), [new2] "i" (clear_page_erms) , "D" (page)
> 	: "cc", "memory", "rax", "rcx")
> 
>                                     ;
> }
> 
> Which looks right, but utterly fails to build :(

What does the failure look like?

Could it be that the labels should be local ones?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Peter Zijlstra]

On Tue, Sep 30, 2025 at 05:42:13PM +0200, Jürgen Groß wrote:

> Could it be that the labels should be local ones?

I already tried 's/#pfx/".L" #pfx/g' and that made no difference.

> What does the failure look like?

Its complaining about label re-definitions.

$ make O=defconfig-build/ arch/x86/kernel/alternative.o
../arch/x86/include/asm/cpufeature.h: Assembler messages:
../arch/x86/include/asm/cpufeature.h:102: Error: symbol `.L__UNIQUE_ID_altinstr_67_begin' is already defined
../arch/x86/include/asm/cpufeature.h:104: Error: symbol `.L__UNIQUE_ID_altinstr_66_begin' is already defined
../arch/x86/include/asm/cpufeature.h:106: Error: symbol `.L__UNIQUE_ID_altinstr_66_pad' is already defined
../arch/x86/include/asm/cpufeature.h:109: Error: symbol `.L__UNIQUE_ID_altinstr_66_end' is already defined
../arch/x86/include/asm/cpufeature.h:119: Error: symbol `.L__UNIQUE_ID_altinstr_66_alt_begin' is already defined
../arch/x86/include/asm/cpufeature.h:121: Error: symbol `.L__UNIQUE_ID_altinstr_66_alt_end' is already defined
../arch/x86/include/asm/cpufeature.h:124: Error: symbol `.L__UNIQUE_ID_altinstr_67_pad' is already defined
../arch/x86/include/asm/cpufeature.h:127: Error: symbol `.L__UNIQUE_ID_altinstr_67_end' is already defined
../arch/x86/include/asm/cpufeature.h:137: Error: symbol `.L__UNIQUE_ID_altinstr_67_alt_begin' is already defined
../arch/x86/include/asm/cpufeature.h:139: Error: symbol `.L__UNIQUE_ID_altinstr_67_alt_end' is already defined
../arch/x86/include/asm/cpufeature.h:102: Error: symbol `.L__UNIQUE_ID_altinstr_67_begin' is already defined
../arch/x86/include/asm/cpufeature.h:104: Error: symbol `.L__UNIQUE_ID_altinstr_66_begin' is already defined
../arch/x86/include/asm/cpufeature.h:106: Error: symbol `.L__UNIQUE_ID_altinstr_66_pad' is already defined
../arch/x86/include/asm/cpufeature.h:109: Error: symbol `.L__UNIQUE_ID_altinstr_66_end' is already defined
../arch/x86/include/asm/cpufeature.h:119: Error: symbol `.L__UNIQUE_ID_altinstr_66_alt_begin' is already defined
../arch/x86/include/asm/cpufeature.h:121: Error: symbol `.L__UNIQUE_ID_altinstr_66_alt_end' is already defined
../arch/x86/include/asm/cpufeature.h:124: Error: symbol `.L__UNIQUE_ID_altinstr_67_pad' is already defined
../arch/x86/include/asm/cpufeature.h:127: Error: symbol `.L__UNIQUE_ID_altinstr_67_end' is already defined
../arch/x86/include/asm/cpufeature.h:137: Error: symbol `.L__UNIQUE_ID_altinstr_67_alt_begin' is already defined
../arch/x86/include/asm/cpufeature.h:139: Error: symbol `.L__UNIQUE_ID_altinstr_67_alt_end' is already defined
../arch/x86/include/asm/cpufeature.h:102: Error: symbol `.L__UNIQUE_ID_altinstr_67_begin' is already defined
../arch/x86/include/asm/cpufeature.h:104: Error: symbol `.L__UNIQUE_ID_altinstr_66_begin' is already defined
../arch/x86/include/asm/cpufeature.h:106: Error: symbol `.L__UNIQUE_ID_altinstr_66_pad' is already defined
../arch/x86/include/asm/cpufeature.h:109: Error: symbol `.L__UNIQUE_ID_altinstr_66_end' is already defined
../arch/x86/include/asm/cpufeature.h:119: Error: symbol `.L__UNIQUE_ID_altinstr_66_alt_begin' is already defined
../arch/x86/include/asm/cpufeature.h:121: Error: symbol `.L__UNIQUE_ID_altinstr_66_alt_end' is already defined
../arch/x86/include/asm/cpufeature.h:124: Error: symbol `.L__UNIQUE_ID_altinstr_67_pad' is already defined
../arch/x86/include/asm/cpufeature.h:127: Error: symbol `.L__UNIQUE_ID_altinstr_67_end' is already defined
...

That specific one is this:

  static inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((no_instrument_function)) __attribute__((__always_inline__)) bool _static_cpu_has(u16 bit)
  {
   asm goto("# ALT: oldinstr\n" ".L" "__UNIQUE_ID_altinstr_67" "_begin:\n\t" "# ALT: oldinstr\n" ".L" "__UNIQUE_ID_altinstr_66" "_begin:\n\t" "jmp 6f" "\n" ".L" "__UNIQUE_ID_altinstr_66" "_pad:\n" "# ALT: padding\n" ".skip -(((" ".L" "_   _UNIQUE_ID_altinstr_66" "_alt_end - " ".L" "__UNIQUE_ID_altinstr_66" "_alt_begin" ")-(" ".L" "__UNIQUE_ID_altinstr_66" "_pad - " ".L" "__UNIQUE_ID_altinstr_66" "_begin" ")) > 0) * " "((" ".L" "__UNIQUE_ID_altinstr_66" "_alt_end - " "   .L" "__UNIQUE_ID_altinstr_66" "_alt_begin" ")-(" ".L" "__UNIQUE_ID_altinstr_66" "_pad - " ".L" "__UNIQUE_ID_altinstr_66" "_begin" ")),0x90\n" ".L" "__UNIQUE_ID_altinstr_66" "_end:\n" ".pushsection .altinstructions,\"a\"\n" " .long "    ".L" "__UNIQUE_ID_altinstr_66" "_begin - .\n" " .long " ".L" "__UNIQUE_ID_altinstr_66" "_alt_begin - .\n" " .4byte " "( 3*32+21)" "\n" " .byte " ".L" "__UNIQUE_ID_altinstr_66" "_end - " ".L" "__UNIQUE_ID_altinstr_66" "_begin" "\n" "    .byte " ".L" "__UNIQUE_ID_altinstr_66" "_alt_end - " ".L" "__UNIQUE_ID_altinstr_66" "_alt_begin" "\n" ".popsection\n" ".pushsection .altinstr_replacement, \"ax\"\n" "# ALT: replacement\n" ".L" "__UNIQUE_ID_altinstr_66" "_alt_begin:\n   \t" "jmp %l[t_no]" "\n" ".L" "__UNIQUE_ID_altinstr_66" "_alt_end:\n" ".popsection\n" "\n" ".L" "__UNIQUE_ID_altinstr_67" "_pad:\n" "# ALT: padding\n" ".skip -(((" ".L" "__UNIQUE_ID_altinstr_67" "_alt_end - " ".L" "__UNIQUE_ID_altinst   r_67" "_alt_begin" ")-(" ".L" "__UNIQUE_ID_altinstr_67" "_pad - " ".L" "__UNIQUE_ID_altinstr_67" "_begin" ")) > 0) * " "((" ".L" "__UNIQUE_ID_altinstr_67" "_alt_end - " ".L" "__UNIQUE_ID_altinstr_67" "_alt_begin" ")-(" ".L" "__UNIQUE   _ID_altinstr_67" "_pad - " ".L" "__UNIQUE_ID_altinstr_67" "_begin" ")),0x90\n" ".L" "__UNIQUE_ID_altinstr_67" "_end:\n" ".pushsection .altinstructions,\"a\"\n" " .long " ".L" "__UNIQUE_ID_altinstr_67" "_begin - .\n" " .long " ".L" "_   _UNIQUE_ID_altinstr_67" "_alt_begin - .\n" " .4byte " "%c[feature]" "\n" " .byte " ".L" "__UNIQUE_ID_altinstr_67" "_end - " ".L" "__UNIQUE_ID_altinstr_67" "_begin" "\n" " .byte " ".L" "__UNIQUE_ID_altinstr_67" "_alt_end - " ".L" "__U   NIQUE_ID_altinstr_67" "_alt_begin" "\n" ".popsection\n" ".pushsection .altinstr_replacement, \"ax\"\n" "# ALT: replacement\n" ".L" "__UNIQUE_ID_altinstr_67" "_alt_begin:\n\t" "" "\n" ".L" "__UNIQUE_ID_altinstr_67" "_alt_end:\n" ".pop   section\n"
    ".pushsection .altinstr_aux,\"ax\"\n"
    "6:\n"
    " testb %[bitnum], %a[cap_byte]\n"
    " jnz %l[t_yes]\n"
    " jmp %l[t_no]\n"
    ".popsection\n"
     : : [feature] "i" (bit),
         [bitnum] "i" (1 << (bit & 7)),
         [cap_byte] "i" (&((const char *)boot_cpu_data.x86_capability)[bit >> 3])
     : : t_yes, t_no);
  t_yes:
   return true;
  t_no:
   return false;
  }

What I'm thinking is happening is that the __always_inline__ is causing
multiple exact copies of the thing.

Let me see how terrible it all ends up when using as macros

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Peter Zijlstra]

On Wed, Oct 01, 2025 at 08:43:39AM +0200, Peter Zijlstra wrote:

> Let me see how terrible it all ends up when using as macros

Argh, as macros are differently painful. I hate computers :/

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Dave Hansen]

On 10/1/25 00:23, Peter Zijlstra wrote:
> On Wed, Oct 01, 2025 at 08:43:39AM +0200, Peter Zijlstra wrote:
>> Let me see how terrible it all ends up when using as macros
> Argh, as macros are differently painful. I hate computers :/

ALTERNATIVES are fun and all, but is there a good reason we're pulling
out our hair to use them here?

Normal WRMSR is slooooooooooow. The ones that aren't slow don't need
WRMSRNS in the first place.

Would an out-of-line wrmsr() with an if() in it be so bad? Or a
static_call()? Having WRMSR be inlined in a laudable goal, but I'm
really asking if it's worth it.

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[H. Peter Anvin]

On October 3, 2025 7:23:29 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>On 10/1/25 00:23, Peter Zijlstra wrote:
>> On Wed, Oct 01, 2025 at 08:43:39AM +0200, Peter Zijlstra wrote:
>>> Let me see how terrible it all ends up when using as macros
>> Argh, as macros are differently painful. I hate computers :/
>
>ALTERNATIVES are fun and all, but is there a good reason we're pulling
>out our hair to use them here?
>
>Normal WRMSR is slooooooooooow. The ones that aren't slow don't need
>WRMSRNS in the first place.
>
>Would an out-of-line wrmsr() with an if() in it be so bad? Or a
>static_call()? Having WRMSR be inlined in a laudable goal, but I'm
>really asking if it's worth it.

We need them to use wrmsrns immediate.

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3918 bytes --]

On 30.09.25 10:46, Jürgen Groß wrote:
> On 30.09.25 10:31, Peter Zijlstra wrote:
>> On Tue, Sep 30, 2025 at 09:03:53AM +0200, Juergen Gross wrote:
>>
>>> +static __always_inline bool __wrmsrq_constant(u32 msr, u64 val, int type)
>>> +{
>>> +    BUILD_BUG_ON(!__builtin_constant_p(msr));
>>> +
>>> +    asm_inline volatile goto(
>>> +        "1:\n"
>>> +        ALTERNATIVE_2(PREPARE_RCX_RDX_FOR_WRMSR
>>> +                  "2: ds wrmsr",
>>> +                  PREPARE_RCX_RDX_FOR_WRMSR
>>> +                  ASM_WRMSRNS,
>>> +                  X86_FEATURE_WRMSRNS,
>>> +                  ASM_WRMSRNS_IMM,
>>> +                  X86_FEATURE_MSR_IMM)
>>> +        _ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])    /* For WRMSRNS 
>>> immediate */
>>> +        _ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])    /* For WRMSR(NS) */
>>> +
>>> +        :
>>> +        : [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
>>> +        : "memory", "ecx", "rdx"
>>> +        : badmsr);
>>> +
>>> +    return false;
>>> +
>>> +badmsr:
>>> +    return true;
>>> +}
>>
>> Just wondering, would something this work?
>>
>>     asm_inline volatile goto(
>>         "1:\n"
>>         ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
>>                 "2:\n"
>>                 ALTERNATIVE("ds wrmsr",
>>                             ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
>>                 ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
>>         _ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])    /* For WRMSRNS 
>> immediate */
>>         _ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])    /* For WRMSR(NS) */
>>
>>         :
>>         : [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
>>         : "memory", "ecx", "rdx"
>>         : badmsr);
>>
>> Its a bit weird because the nested alternative isn't for the exact same
>> position I suppose. But I find it a more readable form.
> 
> I don't think it would work. Nested ALTERNATIVE()s do work only with
> all of them starting at the same location. Have a look at the
> ALTERNATIVE() macro, which is defining the label "771" via OLDINSTR()
> and then referring to this label via ALTINSTR_ENTRY(). In your case
> the ALTINSTR_ENTRY() of the outer ALTERNATIVE() invocation would find
> the wrong "771" label (the one of the inner ALTERNATIVE()).
> 
> Allowing such constructs would probably require switching from preprocessor
> macros to assembler macros.

Thinking more about that I believe there are additional problems:

Having overlapping alternatives not starting at the same address will result
in problems with length padding of the outer alternative in case the inner
one starting later is extending past the end of the outer one. This might be
possible to handle, but it will be tedious.

A similar problem occurs with my recent series for merging nested alternative
patching into a temporary buffer. Currently the code relies on all nested
alternatives to start at the same location.

Using your idea with pv_ops could result in the inner alternative not being
at the start of the outer alternative AND being not the initial code. This
would result in patching in the .altinstructions area instead of the normal
.text site, resulting in possible loss of a patching action if the patched
area would have been used as a replacement before.

So in my opinion allowing alternatives to nest without all inner levels
starting at the same location as the outermost level would be a receipt for
failure.

I think I'll write another patch to BUG() in case such a situation is being
detected.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Peter Zijlstra]

On Wed, Oct 01, 2025 at 10:49:31AM +0200, Juergen Gross wrote:

> Thinking more about that I believe there are additional problems:
> 
> Having overlapping alternatives not starting at the same address will result
> in problems with length padding of the outer alternative in case the inner
> one starting later is extending past the end of the outer one. This might be
> possible to handle, but it will be tedious.

Yes, this must not happen.

> Using your idea with pv_ops could result in the inner alternative not being
> at the start of the outer alternative AND being not the initial code. This
> would result in patching in the .altinstructions area instead of the normal
> .text site, resulting in possible loss of a patching action if the patched
> area would have been used as a replacement before.

Not quite, the nested alternative was in the orig_insn part. So it would
result in patching the orig text twice, once from the inner (which comes
first in the patch list) and then once again from the outer (which comes
later).

> So in my opinion allowing alternatives to nest without all inner levels
> starting at the same location as the outermost level would be a receipt for
> failure.

Certainly tricky, no argument there.

> I think I'll write another patch to BUG() in case such a situation is being
> detected.

Fair enough; we should not currently have any such cases. And going by
my attempt to make it work, its going to be really tricky in any case.

But please put on a comment on why, which explains the constraints.

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1886 bytes --]

On 01.10.25 12:50, Peter Zijlstra wrote:
> On Wed, Oct 01, 2025 at 10:49:31AM +0200, Juergen Gross wrote:
> 
>> Thinking more about that I believe there are additional problems:
>>
>> Having overlapping alternatives not starting at the same address will result
>> in problems with length padding of the outer alternative in case the inner
>> one starting later is extending past the end of the outer one. This might be
>> possible to handle, but it will be tedious.
> 
> Yes, this must not happen.
> 
>> Using your idea with pv_ops could result in the inner alternative not being
>> at the start of the outer alternative AND being not the initial code. This
>> would result in patching in the .altinstructions area instead of the normal
>> .text site, resulting in possible loss of a patching action if the patched
>> area would have been used as a replacement before.
> 
> Not quite, the nested alternative was in the orig_insn part. So it would
> result in patching the orig text twice, once from the inner (which comes
> first in the patch list) and then once again from the outer (which comes
> later).

Yes, but that was the native case only.

With pv_ops this would mean the original instruction would be the
paravirt-call, resulting in your construct becoming an inner nesting level.

> 
>> So in my opinion allowing alternatives to nest without all inner levels
>> starting at the same location as the outermost level would be a receipt for
>> failure.
> 
> Certainly tricky, no argument there.
> 
>> I think I'll write another patch to BUG() in case such a situation is being
>> detected.
> 
> Fair enough; we should not currently have any such cases. And going by
> my attempt to make it work, its going to be really tricky in any case.
> 
> But please put on a comment on why, which explains the constraints.

Agreed.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Sean Christopherson]

On Tue, Sep 30, 2025, Juergen Gross wrote:
> When available use one of the non-serializing WRMSR variants (WRMSRNS
> with or without an immediate operand specifying the MSR register) in
> __wrmsrq().
> 
> For the safe/unsafe variants make __wrmsrq() to be a common base
> function instead of duplicating the ALTERNATIVE*() macros. This
> requires to let native_wrmsr() use native_wrmsrq() instead of
> __wrmsrq(). While changing this, convert native_wrmsr() into an inline
> function.
> 
> Replace the only call of wsrmsrns() with the now equivalent call to
> native_wrmsrq() and remove wsrmsrns().
> 
> The paravirt case will be handled later.

...

> @@ -268,21 +357,6 @@ static inline int wrmsrq_safe(u32 msr, u64 val)
>  	return err;
>  }
>  
> -/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
> -#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
> -
> -/* Non-serializing WRMSR, when available.  Falls back to a serializing WRMSR. */
> -static __always_inline void wrmsrns(u32 msr, u64 val)

FYI, a use of wrmsrns() is likely coming in through the KVM (x86) tree, commit
65391feb042b ("KVM: VMX: Add host MSR read/write helpers to consolidate preemption
handling").

Probably makes sense to spin v3 after the merge window?  Or on linux-next? (I
can't tell what was used as the base, and I double-checked that the above commit
is in linux-next).

> -{
> -	/*
> -	 * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant
> -	 * DS prefix to avoid a trailing NOP.
> -	 */
> -	asm volatile("1: " ALTERNATIVE("ds wrmsr", ASM_WRMSRNS, X86_FEATURE_WRMSRNS)
> -		     "2: " _ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_WRMSR)
> -		     : : "c" (msr), "a" ((u32)val), "d" ((u32)(val >> 32)));
> -}
> -
>  static inline void wrmsr(u32 msr, u32 low, u32 high)
>  {
>  	wrmsrq(msr, (u64)high << 32 | low);
> -- 
> 2.51.0
> 

Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1574 bytes --]

On 30.09.25 18:00, Sean Christopherson wrote:
> On Tue, Sep 30, 2025, Juergen Gross wrote:
>> When available use one of the non-serializing WRMSR variants (WRMSRNS
>> with or without an immediate operand specifying the MSR register) in
>> __wrmsrq().
>>
>> For the safe/unsafe variants make __wrmsrq() to be a common base
>> function instead of duplicating the ALTERNATIVE*() macros. This
>> requires to let native_wrmsr() use native_wrmsrq() instead of
>> __wrmsrq(). While changing this, convert native_wrmsr() into an inline
>> function.
>>
>> Replace the only call of wsrmsrns() with the now equivalent call to
>> native_wrmsrq() and remove wsrmsrns().
>>
>> The paravirt case will be handled later.
> 
> ...
> 
>> @@ -268,21 +357,6 @@ static inline int wrmsrq_safe(u32 msr, u64 val)
>>   	return err;
>>   }
>>   
>> -/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
>> -#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6)
>> -
>> -/* Non-serializing WRMSR, when available.  Falls back to a serializing WRMSR. */
>> -static __always_inline void wrmsrns(u32 msr, u64 val)
> 
> FYI, a use of wrmsrns() is likely coming in through the KVM (x86) tree, commit
> 65391feb042b ("KVM: VMX: Add host MSR read/write helpers to consolidate preemption
> handling").

Thanks for the heads up!

> 
> Probably makes sense to spin v3 after the merge window?  Or on linux-next? (I
> can't tell what was used as the base, and I double-checked that the above commit
> is in linux-next).

I'll find a proper solution. :-)


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 00/12] x86/msr: Inline rdmsr/wrmsr instructions

[H. Peter Anvin]

On 2025-09-30 00:03, Juergen Gross wrote:
> When building a kernel with CONFIG_PARAVIRT_XXL the paravirt
> infrastructure will always use functions for reading or writing MSRs,
> even when running on bare metal.
> 
> Switch to inline RDMSR/WRMSR instructions in this case, reducing the
> paravirt overhead.
> 
> In order to make this less intrusive, some further reorganization of
> the MSR access helpers is done in the first 5 patches.
> 
> The next 5 patches are converting the non-paravirt case to use direct
> inlining of the MSR access instructions, including the WRMSRNS
> instruction and the immediate variants of RDMSR and WRMSR if possible.
> 
> Patch 11 removes the PV hooks for MSR accesses and implements the
> Xen PV cases via calls depending on X86_FEATURE_XENPV, which results
> in runtime patching those calls away for the non-XenPV case.
> 
> Patch 12 is a final little cleanup patch.
> 
> This series has been tested to work with Xen PV and on bare metal.
> 
> This series is inspired by Xin Li, who used a similar approach, but
> (in my opinion) with some flaws. Originally I thought it should be
> possible to use the paravirt infrastructure, but this turned out to be
> rather complicated, especially for the Xen PV case in the *_safe()
> variants of the MSR access functions.
> 

Looks good to me.

(I'm not at all surprised that paravirt_ops didn't do the job. Both I and Xin
had come to the same conclusion.)


Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>