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
