Re: [PATCH v2 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.cooper3@xxxxxxxxxx>
> Again technically:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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