| Message ID | 20250331082251.3171276-11-xin@zytor.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MSR refactor with new MSR instructions support | expand |
On Mon, Mar 31, 2025 at 01:22:46AM -0700, Xin Li (Intel) wrote: > Signed-off-by: Xin Li (Intel) <xin@zytor.com> > --- > arch/x86/include/asm/msr-index.h | 6 ++++++ > arch/x86/kvm/vmx/vmenter.S | 28 ++++++++++++++++++++++++---- > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > index e6134ef2263d..04244c3ba374 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -1226,4 +1226,10 @@ > * a #GP > */ > > +/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */ > +#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6) > + > +/* Instruction opcode for the immediate form RDMSR/WRMSRNS */ > +#define ASM_WRMSRNS_RAX _ASM_BYTES(0xc4,0xe7,0x7a,0xf6,0xc0) > + > #endif /* _ASM_X86_MSR_INDEX_H */ > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S > index f6986dee6f8c..9fae43723c44 100644 > --- a/arch/x86/kvm/vmx/vmenter.S > +++ b/arch/x86/kvm/vmx/vmenter.S > @@ -64,6 +64,29 @@ > RET > .endm > > +/* > + * Write EAX to MSR_IA32_SPEC_CTRL. > + * > + * Choose the best WRMSR instruction based on availability. > + * > + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. > + */ > +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL > + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ > + xor %edx, %edx; \ > + mov %edi, %eax; \ > + ds wrmsr), \ > + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ > + xor %edx, %edx; \ > + mov %edi, %eax; \ > + ASM_WRMSRNS), \ > + X86_FEATURE_WRMSRNS, \ > + __stringify(xor %_ASM_AX, %_ASM_AX; \ > + mov %edi, %eax; \ > + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ > + X86_FEATURE_MSR_IMM > +.endm > + > .section .noinstr.text, "ax" > > /** > @@ -123,10 +146,7 @@ SYM_FUNC_START(__vmx_vcpu_run) > movl PER_CPU_VAR(x86_spec_ctrl_current), %esi > cmp %edi, %esi > je .Lspec_ctrl_done > - mov $MSR_IA32_SPEC_CTRL, %ecx > - xor %edx, %edx > - mov %edi, %eax > - wrmsr Is that the right path forward? That is replace the MSR write to disable speculative execution with a non-serialized WRMSR? Doesn't that mean the WRMSRNS is speculative? > + WRITE_EAX_TO_MSR_IA32_SPEC_CTRL > > .Lspec_ctrl_done: > > -- > 2.49.0 > >
On Mon, Mar 31, 2025 at 04:27:23PM -0400, Konrad Rzeszutek Wilk wrote: > Is that the right path forward? > > That is replace the MSR write to disable speculative execution with a > non-serialized WRMSR? Doesn't that mean the WRMSRNS is speculative? Ha, interesting question. If the WRMSR is non-serializing, when do speculative things like indirect branches and the like get *actually* cleared and can such a speculation window be used to leak branch data even if IBRS is actually enabled for example... Fun. This change needs to be run by hw folks and I guess until then WRMSRNS should not get anywhere near mitigation MSRs like SPEC_CTRL or PRED_CMD... Thx.
On 31/03/2025 9:27 pm, Konrad Rzeszutek Wilk wrote: > On Mon, Mar 31, 2025 at 01:22:46AM -0700, Xin Li (Intel) wrote: >> Signed-off-by: Xin Li (Intel) <xin@zytor.com> >> --- >> arch/x86/include/asm/msr-index.h | 6 ++++++ >> arch/x86/kvm/vmx/vmenter.S | 28 ++++++++++++++++++++++++---- >> 2 files changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h >> index e6134ef2263d..04244c3ba374 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -1226,4 +1226,10 @@ >> * a #GP >> */ >> >> +/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */ >> +#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6) >> + >> +/* Instruction opcode for the immediate form RDMSR/WRMSRNS */ >> +#define ASM_WRMSRNS_RAX _ASM_BYTES(0xc4,0xe7,0x7a,0xf6,0xc0) >> + >> #endif /* _ASM_X86_MSR_INDEX_H */ >> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S >> index f6986dee6f8c..9fae43723c44 100644 >> --- a/arch/x86/kvm/vmx/vmenter.S >> +++ b/arch/x86/kvm/vmx/vmenter.S >> @@ -64,6 +64,29 @@ >> RET >> .endm >> >> +/* >> + * Write EAX to MSR_IA32_SPEC_CTRL. >> + * >> + * Choose the best WRMSR instruction based on availability. >> + * >> + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. >> + */ >> +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL >> + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >> + xor %edx, %edx; \ >> + mov %edi, %eax; \ >> + ds wrmsr), \ >> + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >> + xor %edx, %edx; \ >> + mov %edi, %eax; \ >> + ASM_WRMSRNS), \ >> + X86_FEATURE_WRMSRNS, \ >> + __stringify(xor %_ASM_AX, %_ASM_AX; \ >> + mov %edi, %eax; \ >> + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ >> + X86_FEATURE_MSR_IMM >> +.endm >> + >> .section .noinstr.text, "ax" >> >> /** >> @@ -123,10 +146,7 @@ SYM_FUNC_START(__vmx_vcpu_run) >> movl PER_CPU_VAR(x86_spec_ctrl_current), %esi >> cmp %edi, %esi >> je .Lspec_ctrl_done >> - mov $MSR_IA32_SPEC_CTRL, %ecx >> - xor %edx, %edx >> - mov %edi, %eax >> - wrmsr > Is that the right path forward? > > That is replace the MSR write to disable speculative execution with a > non-serialized WRMSR? Doesn't that mean the WRMSRNS is speculative? MSR_SPEC_CTRL is explicitly non-serialising, even with a plain WRMSR. non-serialising != non-speculative. Although WRMSRNS's precise statement on the matter of non-speculativeness is woolly, given an intent to optimise it some more in the future. ~Andrew
On March 31, 2025 1:27:23 PM PDT, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >On Mon, Mar 31, 2025 at 01:22:46AM -0700, Xin Li (Intel) wrote: >> Signed-off-by: Xin Li (Intel) <xin@zytor.com> >> --- >> arch/x86/include/asm/msr-index.h | 6 ++++++ >> arch/x86/kvm/vmx/vmenter.S | 28 ++++++++++++++++++++++++---- >> 2 files changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h >> index e6134ef2263d..04244c3ba374 100644 >> --- a/arch/x86/include/asm/msr-index.h >> +++ b/arch/x86/include/asm/msr-index.h >> @@ -1226,4 +1226,10 @@ >> * a #GP >> */ >> >> +/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */ >> +#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6) >> + >> +/* Instruction opcode for the immediate form RDMSR/WRMSRNS */ >> +#define ASM_WRMSRNS_RAX _ASM_BYTES(0xc4,0xe7,0x7a,0xf6,0xc0) >> + >> #endif /* _ASM_X86_MSR_INDEX_H */ >> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S >> index f6986dee6f8c..9fae43723c44 100644 >> --- a/arch/x86/kvm/vmx/vmenter.S >> +++ b/arch/x86/kvm/vmx/vmenter.S >> @@ -64,6 +64,29 @@ >> RET >> .endm >> >> +/* >> + * Write EAX to MSR_IA32_SPEC_CTRL. >> + * >> + * Choose the best WRMSR instruction based on availability. >> + * >> + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. >> + */ >> +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL >> + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >> + xor %edx, %edx; \ >> + mov %edi, %eax; \ >> + ds wrmsr), \ >> + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >> + xor %edx, %edx; \ >> + mov %edi, %eax; \ >> + ASM_WRMSRNS), \ >> + X86_FEATURE_WRMSRNS, \ >> + __stringify(xor %_ASM_AX, %_ASM_AX; \ >> + mov %edi, %eax; \ >> + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ >> + X86_FEATURE_MSR_IMM >> +.endm >> + >> .section .noinstr.text, "ax" >> >> /** >> @@ -123,10 +146,7 @@ SYM_FUNC_START(__vmx_vcpu_run) >> movl PER_CPU_VAR(x86_spec_ctrl_current), %esi >> cmp %edi, %esi >> je .Lspec_ctrl_done >> - mov $MSR_IA32_SPEC_CTRL, %ecx >> - xor %edx, %edx >> - mov %edi, %eax >> - wrmsr > >Is that the right path forward? > >That is replace the MSR write to disable speculative execution with a >non-serialized WRMSR? Doesn't that mean the WRMSRNS is speculative? > > >> + WRITE_EAX_TO_MSR_IA32_SPEC_CTRL >> >> .Lspec_ctrl_done: >> >> -- >> 2.49.0 >> >> So to clarify the semantics of WRMSRNS: it is an opt-in capability for the OS to allow the hardware to choose the amount of serialization needed on an MSR- or implementation-specific basis. It also allows the OS to set multiple MSRs followed by a SERIALIZE instruction if full hard serialization is still desired, rather than having each individual MSR write do a full hard serialization (killing the full pipe and starting over from instruction fetch.) This will replace the – architecturally questionable, in my opinion – practice of introducing non-serializing MSRs which after all are retroactive changes to the semantics WRMSR instruction with no opt-out (although the existence of SERIALIZE improves the situation somewhat.) I agree that we need better documentation as to the semantics of WRMSRNS on critical MSRs like SPEC_CTRL, and especially in that specific case, when post-batch SERIALIZE would be called for.
On 3/31/25 13:41, Andrew Cooper wrote: >> >> That is replace the MSR write to disable speculative execution with a >> non-serialized WRMSR? Doesn't that mean the WRMSRNS is speculative? > > MSR_SPEC_CTRL is explicitly non-serialising, even with a plain WRMSR. > > non-serialising != non-speculative. > > Although WRMSRNS's precise statement on the matter of > non-speculativeness is woolly, given an intent to optimise it some more > in the future. > To be specific, "serializing" is a much harder statement than "non-speculative." For architecturally non-serializing MSRs, WRMSRNS and WRMSR are equivalent (or to put it differently, WRMSR acts like WRMSRNS.) The advantage with making them explicitly WRMSRNS is that it allows for the substitution of the upcoming immediate forms. -hpa
On Mon, Mar 31, 2025, Xin Li (Intel) wrote: > Signed-off-by: Xin Li (Intel) <xin@zytor.com> > --- > arch/x86/include/asm/msr-index.h | 6 ++++++ > arch/x86/kvm/vmx/vmenter.S | 28 ++++++++++++++++++++++++---- > 2 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > index e6134ef2263d..04244c3ba374 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -1226,4 +1226,10 @@ > * a #GP > */ > > +/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */ > +#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6) > + > +/* Instruction opcode for the immediate form RDMSR/WRMSRNS */ > +#define ASM_WRMSRNS_RAX _ASM_BYTES(0xc4,0xe7,0x7a,0xf6,0xc0) > + > #endif /* _ASM_X86_MSR_INDEX_H */ > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S > index f6986dee6f8c..9fae43723c44 100644 > --- a/arch/x86/kvm/vmx/vmenter.S > +++ b/arch/x86/kvm/vmx/vmenter.S > @@ -64,6 +64,29 @@ > RET > .endm > > +/* > + * Write EAX to MSR_IA32_SPEC_CTRL. > + * > + * Choose the best WRMSR instruction based on availability. > + * > + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. > + */ > +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL > + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ > + xor %edx, %edx; \ > + mov %edi, %eax; \ > + ds wrmsr), \ > + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ > + xor %edx, %edx; \ > + mov %edi, %eax; \ > + ASM_WRMSRNS), \ > + X86_FEATURE_WRMSRNS, \ > + __stringify(xor %_ASM_AX, %_ASM_AX; \ > + mov %edi, %eax; \ > + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ > + X86_FEATURE_MSR_IMM > +.endm This is quite hideous. I have no objection to optimizing __vmx_vcpu_run(), but I would much prefer that a macro like this live in generic code, and that it be generic. It should be easy enough to provide an assembly friendly equivalent to __native_wrmsr_constant(). > + > .section .noinstr.text, "ax" > > /** > @@ -123,10 +146,7 @@ SYM_FUNC_START(__vmx_vcpu_run) > movl PER_CPU_VAR(x86_spec_ctrl_current), %esi > cmp %edi, %esi > je .Lspec_ctrl_done > - mov $MSR_IA32_SPEC_CTRL, %ecx > - xor %edx, %edx > - mov %edi, %eax > - wrmsr > + WRITE_EAX_TO_MSR_IA32_SPEC_CTRL > > .Lspec_ctrl_done: > > -- > 2.49.0 >
On 4/10/2025 4:24 PM, Sean Christopherson wrote: >> +/* >> + * Write EAX to MSR_IA32_SPEC_CTRL. >> + * >> + * Choose the best WRMSR instruction based on availability. >> + * >> + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. >> + */ >> +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL >> + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >> + xor %edx, %edx; \ >> + mov %edi, %eax; \ >> + ds wrmsr), \ >> + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >> + xor %edx, %edx; \ >> + mov %edi, %eax; \ >> + ASM_WRMSRNS), \ >> + X86_FEATURE_WRMSRNS, \ >> + __stringify(xor %_ASM_AX, %_ASM_AX; \ >> + mov %edi, %eax; \ >> + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ >> + X86_FEATURE_MSR_IMM >> +.endm > This is quite hideous. I have no objection to optimizing __vmx_vcpu_run(), but > I would much prefer that a macro like this live in generic code, and that it be > generic. It should be easy enough to provide an assembly friendly equivalent to > __native_wrmsr_constant(). Will do.
On April 11, 2025 9:18:08 AM PDT, Xin Li <xin@zytor.com> wrote: >On 4/10/2025 4:24 PM, Sean Christopherson wrote: >>> +/* >>> + * Write EAX to MSR_IA32_SPEC_CTRL. >>> + * >>> + * Choose the best WRMSR instruction based on availability. >>> + * >>> + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. >>> + */ >>> +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL >>> + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >>> + xor %edx, %edx; \ >>> + mov %edi, %eax; \ >>> + ds wrmsr), \ >>> + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ >>> + xor %edx, %edx; \ >>> + mov %edi, %eax; \ >>> + ASM_WRMSRNS), \ >>> + X86_FEATURE_WRMSRNS, \ >>> + __stringify(xor %_ASM_AX, %_ASM_AX; \ >>> + mov %edi, %eax; \ >>> + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ >>> + X86_FEATURE_MSR_IMM >>> +.endm >> This is quite hideous. I have no objection to optimizing __vmx_vcpu_run(), but >> I would much prefer that a macro like this live in generic code, and that it be >> generic. It should be easy enough to provide an assembly friendly equivalent to >> __native_wrmsr_constant(). > >Will do. This should be coming anyway, right?
On Thu, Apr 10, 2025 at 4:24 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Mar 31, 2025, Xin Li (Intel) wrote: > > Signed-off-by: Xin Li (Intel) <xin@zytor.com> > > --- > > arch/x86/include/asm/msr-index.h | 6 ++++++ > > arch/x86/kvm/vmx/vmenter.S | 28 ++++++++++++++++++++++++---- > > 2 files changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h > > index e6134ef2263d..04244c3ba374 100644 > > --- a/arch/x86/include/asm/msr-index.h > > +++ b/arch/x86/include/asm/msr-index.h > > @@ -1226,4 +1226,10 @@ > > * a #GP > > */ > > > > +/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */ > > +#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6) > > + > > +/* Instruction opcode for the immediate form RDMSR/WRMSRNS */ > > +#define ASM_WRMSRNS_RAX _ASM_BYTES(0xc4,0xe7,0x7a,0xf6,0xc0) > > + > > #endif /* _ASM_X86_MSR_INDEX_H */ > > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S > > index f6986dee6f8c..9fae43723c44 100644 > > --- a/arch/x86/kvm/vmx/vmenter.S > > +++ b/arch/x86/kvm/vmx/vmenter.S > > @@ -64,6 +64,29 @@ > > RET > > .endm > > > > +/* > > + * Write EAX to MSR_IA32_SPEC_CTRL. > > + * > > + * Choose the best WRMSR instruction based on availability. > > + * > > + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. > > + */ > > +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL > > + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ > > + xor %edx, %edx; \ > > + mov %edi, %eax; \ > > + ds wrmsr), \ > > + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ > > + xor %edx, %edx; \ > > + mov %edi, %eax; \ > > + ASM_WRMSRNS), \ > > + X86_FEATURE_WRMSRNS, \ > > + __stringify(xor %_ASM_AX, %_ASM_AX; \ > > + mov %edi, %eax; \ > > + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ > > + X86_FEATURE_MSR_IMM > > +.endm > > This is quite hideous. I have no objection to optimizing __vmx_vcpu_run(), but > I would much prefer that a macro like this live in generic code, and that it be > generic. It should be easy enough to provide an assembly friendly equivalent to > __native_wrmsr_constant(). Surely, any CPU that has WRMSRNS also supports "Virtualize IA32_SPEC_CTRL," right? Shouldn't we be using that feature rather than swapping host and guest values with some form of WRMSR? > > + > > .section .noinstr.text, "ax" > > > > /** > > @@ -123,10 +146,7 @@ SYM_FUNC_START(__vmx_vcpu_run) > > movl PER_CPU_VAR(x86_spec_ctrl_current), %esi > > cmp %edi, %esi > > je .Lspec_ctrl_done > > - mov $MSR_IA32_SPEC_CTRL, %ecx > > - xor %edx, %edx > > - mov %edi, %eax > > - wrmsr > > + WRITE_EAX_TO_MSR_IA32_SPEC_CTRL > > > > .Lspec_ctrl_done: > > > > -- > > 2.49.0 > > >
On 4/11/2025 1:50 PM, H. Peter Anvin wrote: >>> This is quite hideous. I have no objection to optimizing __vmx_vcpu_run(), but >>> I would much prefer that a macro like this live in generic code, and that it be >>> generic. It should be easy enough to provide an assembly friendly equivalent to >>> __native_wrmsr_constant(). >> Will do. > This should be coming anyway, right? Absolutely. Totally stupid me: we have it ready to use here, but ...
On 4/11/2025 2:12 PM, Jim Mattson wrote: > Surely, any CPU that has WRMSRNS also supports "Virtualize > IA32_SPEC_CTRL," right? Shouldn't we be using that feature rather than > swapping host and guest values with some form of WRMSR? Good question, the simple answer is that they are irrelevant.
On April 11, 2025 9:32:32 PM PDT, Xin Li <xin@zytor.com> wrote: >On 4/11/2025 2:12 PM, Jim Mattson wrote: >> Surely, any CPU that has WRMSRNS also supports "Virtualize >> IA32_SPEC_CTRL," right? Shouldn't we be using that feature rather than >> swapping host and guest values with some form of WRMSR? > >Good question, the simple answer is that they are irrelevant. Also, *in this specific case* IA32_SPEC_CTRL is architecturally nonserializing, i.e. WRMSR executes as WRMSRNS anyway.
On 4/12/2025 4:10 PM, H. Peter Anvin wrote:
> Also,*in this specific case* IA32_SPEC_CTRL is architecturally nonserializing, i.e. WRMSR executes as WRMSRNS anyway.
While the immediate form WRMSRNS could be faster because the MSR index
is available *much* earlier in the pipeline, right?
On April 14, 2025 10:48:47 AM PDT, Xin Li <xin@zytor.com> wrote: >On 4/12/2025 4:10 PM, H. Peter Anvin wrote: >> Also,*in this specific case* IA32_SPEC_CTRL is architecturally nonserializing, i.e. WRMSR executes as WRMSRNS anyway. > >While the immediate form WRMSRNS could be faster because the MSR index >is available *much* earlier in the pipeline, right? Yes, but then it would be redundant with the virtualization support.
On 4/14/2025 11:56 PM, H. Peter Anvin wrote: >> arlier in the pipeline, right? > Yes, but then it would be redundant with the virtualization support. > So better to drop this patch then.
On April 15, 2025 10:06:01 AM PDT, Xin Li <xin@zytor.com> wrote: >On 4/14/2025 11:56 PM, H. Peter Anvin wrote: >>> arlier in the pipeline, right? >> Yes, but then it would be redundant with the virtualization support. >> > >So better to drop this patch then. Yeah, if it gets pulled in as a consequence of a global change that is OK but the local change makes no sense.
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index e6134ef2263d..04244c3ba374 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -1226,4 +1226,10 @@ * a #GP */ +/* Instruction opcode for WRMSRNS supported in binutils >= 2.40 */ +#define ASM_WRMSRNS _ASM_BYTES(0x0f,0x01,0xc6) + +/* Instruction opcode for the immediate form RDMSR/WRMSRNS */ +#define ASM_WRMSRNS_RAX _ASM_BYTES(0xc4,0xe7,0x7a,0xf6,0xc0) + #endif /* _ASM_X86_MSR_INDEX_H */ diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S index f6986dee6f8c..9fae43723c44 100644 --- a/arch/x86/kvm/vmx/vmenter.S +++ b/arch/x86/kvm/vmx/vmenter.S @@ -64,6 +64,29 @@ RET .endm +/* + * Write EAX to MSR_IA32_SPEC_CTRL. + * + * Choose the best WRMSR instruction based on availability. + * + * Replace with 'wrmsrns' and 'wrmsrns %rax, $MSR_IA32_SPEC_CTRL' once binutils support them. + */ +.macro WRITE_EAX_TO_MSR_IA32_SPEC_CTRL + ALTERNATIVE_2 __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ + xor %edx, %edx; \ + mov %edi, %eax; \ + ds wrmsr), \ + __stringify(mov $MSR_IA32_SPEC_CTRL, %ecx; \ + xor %edx, %edx; \ + mov %edi, %eax; \ + ASM_WRMSRNS), \ + X86_FEATURE_WRMSRNS, \ + __stringify(xor %_ASM_AX, %_ASM_AX; \ + mov %edi, %eax; \ + ASM_WRMSRNS_RAX; .long MSR_IA32_SPEC_CTRL), \ + X86_FEATURE_MSR_IMM +.endm + .section .noinstr.text, "ax" /** @@ -123,10 +146,7 @@ SYM_FUNC_START(__vmx_vcpu_run) movl PER_CPU_VAR(x86_spec_ctrl_current), %esi cmp %edi, %esi je .Lspec_ctrl_done - mov $MSR_IA32_SPEC_CTRL, %ecx - xor %edx, %edx - mov %edi, %eax - wrmsr + WRITE_EAX_TO_MSR_IA32_SPEC_CTRL .Lspec_ctrl_done:
Signed-off-by: Xin Li (Intel) <xin@zytor.com> --- arch/x86/include/asm/msr-index.h | 6 ++++++ arch/x86/kvm/vmx/vmenter.S | 28 ++++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-)