On 31/01/2022 10:33, Jan Beulich wrote: > On 28.01.2022 14:29, Andrew Cooper wrote: >> Hardware maintains both host and guest versions of MSR_SPEC_CTRL, but guests >> run with the logical OR of both values. Therefore, in principle we want to >> clear Xen's value before entering the guest. However, for migration >> compatibility, and for performance reasons with SEV-SNP guests, we want the >> ability to use a nonzero value behind the guest's back. Use vcpu_msrs to >> hold >> this value, with the guest value in the VMCB. >> >> On the VMEntry path, adjusting MSR_SPEC_CTRL must be done after CLGI so as to >> be atomic with respect to NMIs/etc. The loading of spec_ctrl_raw into %eax >> was also stale from the unused old code, so can be dropped too. >> >> Implement both pieces of logic as small pieces of C, and alternative the call >> to get there based on X86_FEATURE_SC_MSR_HVM. The use of double alternative >> blocks is due to a quirk of the current infrastructure, where call >> displacements only get fixed up for the first replacement instruction. While >> adjusting the clobber lists, drop the stale requirements on the VMExit side. >> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> > Again technically: > Reviewed-by: Jan Beulich <jbeul...@suse.com>
Thanks. > But ... > >> --- a/xen/arch/x86/hvm/svm/entry.S >> +++ b/xen/arch/x86/hvm/svm/entry.S >> @@ -55,11 +55,12 @@ __UNLIKELY_END(nsvm_hap) >> mov %rsp, %rdi >> call svm_vmenter_helper >> >> - mov VCPU_arch_msrs(%rbx), %rax >> - mov VCPUMSR_spec_ctrl_raw(%rax), %eax >> + clgi >> >> /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ >> - /* SPEC_CTRL_EXIT_TO_SVM (nothing currently) */ >> + /* SPEC_CTRL_EXIT_TO_SVM Req: Clob: >> C */ >> + ALTERNATIVE "", STR(mov %rbx, %rdi; mov %rsp, %rsi), >> X86_FEATURE_SC_MSR_HVM >> + ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM > Is there a reason to use a macro for converting to a string here at > all? There are no "inner" macros here which might need expanding. And > "brevity" (as you have in the rev log) would call for > > ALTERNATIVE "", "mov %rbx, %rdi; mov %rsp, %rsi", > X86_FEATURE_SC_MSR_HVM > ALTERNATIVE "", "call vmentry_spec_ctrl", X86_FEATURE_SC_MSR_HVM Good point. I'll switch to plain strings. > > >> @@ -86,8 +86,10 @@ __UNLIKELY_END(nsvm_hap) >> >> GET_CURRENT(bx) >> >> - /* SPEC_CTRL_ENTRY_FROM_SVM Req: b=curr %rsp=regs/cpuinfo, Clob: >> ac */ >> + /* SPEC_CTRL_ENTRY_FROM_SVM Req: Clob: >> C */ >> ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM >> + ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM >> + ALTERNATIVE "", STR(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM > Same here then, obviously. > >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -3086,6 +3086,33 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) >> vmcb_set_vintr(vmcb, intr); >> } >> >> +/* Called with GIF=0. */ >> +void vmexit_spec_ctrl(struct cpu_info *info) >> +{ >> + unsigned int val = info->xen_spec_ctrl; >> + >> + /* >> + * Write to MSR_SPEC_CTRL unconditionally, for the RAS[:32] flushing >> side >> + * effect. >> + */ >> + wrmsr(MSR_SPEC_CTRL, val, 0); >> + info->last_spec_ctrl = val; >> +} >> + >> +/* Called with GIF=0. */ >> +void vmentry_spec_ctrl(const struct vcpu *curr, struct cpu_info *info) >> +{ >> + unsigned int val = curr->arch.msrs->spec_ctrl.raw; >> + >> + if ( val != info->last_spec_ctrl ) >> + { >> + wrmsr(MSR_SPEC_CTRL, val, 0); >> + info->last_spec_ctrl = val; >> + } >> + >> + /* No Spectre v1 concerns. Execution is going to hit VMRUN imminently. >> */ >> +} > These living in SVM code I think their names want to avoid suggesting > they could also be used for VMX (irrespective of us not meaning to use > them there). Or else they want to move to common code, with comments > slightly adjusted. I'll add svm_ prefixes. I can't see these being useful elsewhere. ~Andrew