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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>
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

.

> @@ -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.

Jan