Re: [PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2022 09:44, Andrew Cooper wrote:
> 1) It would be slightly more efficient to pass curr and cpu_info into
>    vm{entry,exit}_spec_ctrl(), but setup of such state can't be in the
>    ALTERNATIVE block because then the call displacement won't get fixed up.
>    All the additional accesses are hot off the stack, so almost certainly
>    negligible compared to the WRMSR.

What's wrong with using two instances of ALTERNATIVE, one to setup the
call arguments and the 2nd for just the CALL?

> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -55,11 +55,11 @@ __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 "", __stringify(call vmentry_spec_ctrl), 
> X86_FEATURE_SC_MSR_HVM

I guess the new upper case C after Clob: stands for "all call-clobbered
registers"? In which case ...

> @@ -86,8 +85,9 @@ __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: 
> ac,C */
>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
> +        ALTERNATIVE "", __stringify(call vmexit_spec_ctrl), 
> X86_FEATURE_SC_MSR_HVM

... why the explicit further "ac" here? Is the intention to annotate
every individual ALTERNATIVE this way?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -3086,6 +3086,36 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>      vmcb_set_vintr(vmcb, intr);
>  }
>  
> +/* Called with GIF=0. */
> +void vmexit_spec_ctrl(void)
> +{
> +    struct cpu_info *info = get_cpu_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(void)
> +{
> +    struct cpu_info *info = get_cpu_info();
> +    const struct vcpu *curr = current;
> +    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;
> +    }

Is this correct for the very first use on a CPU? last_spec_ctrl
starts out as zero afaict, and hence this very first write would be
skipped if the guest value is also zero (which it will be for a
vCPU first launched), even if we have a non-zero value in the MSR
at that point.

Jan