Re: [PATCH v1 2/3] x86/msr: Switch between WRMSRNS and WRMSR with the alternatives mechanism

From: Xin Li
Date: Fri Aug 16 2024 - 13:53:42 EST


On 8/9/2024 4:07 PM, Andrew Cooper wrote:
On 07/08/2024 6:47 am, Xin Li (Intel) wrote:
From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
+/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */
+#define 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)
{
- __wrmsrns(msr, val, val >> 32);
+ /*
+ * 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",

This isn't the version I presented, and there's no discussion of the
alteration.

I'm trying to implement wrmsr() as

static __always_inline void wrmsr(u32 msr, u64 val)
{
asm volatile("1: " ALTERNATIVE_2("wrmsr", WRMSRNS, X86_FEATURE_WRMSRNS,
"call asm_xen_write_msr", X86_FEATURE_XENPV)
"2: " _ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_WRMSR)
: : "c" (msr), "a" (val), "d" ((u32)(val >> 32)),
"D" (msr), "S" (val));
}


As the CALL instruction is 5-byte long, and we need to pad nop for both
WRMSR and WRMSRNS, what about not using segment prefix at all?


The choice of CS over DS was deliberate, and came from Intel:

https://www.intel.com/content/dam/support/us/en/documents/processors/mitigations-jump-conditional-code-erratum.pdf

So unless Intel want to retract that whitepaper, and all the binutils
work with it, I'd suggest keeping it as CS like we use elsewhere, and as
explicitly instructed by Intel.