[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 26 Jan 2022 17:50:50 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=cVoBSauL43wjcVPt8t8sL/oARtzHvIs2psLXuiupbu8=; b=dKkhF35sLmXFGsfq0n/c61R1Ab0fNg5yWMOvqjhhxy6SoOUc7aHC0Crm2PemjHKvBis/8U1VF7zZ0RuRA0nu5/tusB2gnA8VeVmK+EOMMxLb9ubJQM79FckKc9RY2tHRolIQBeC1nQhTG9yUboMRrGGBwuOwkkmHcD7ZQcFFEhEYAevg+/Z/DNtdaGNUhSjL2h6xXxL4XOwj9n0y8QlrdLbCV9M4I8EmynfrBawT6qMpIn/9UDJI8J0BG8IUgLBZJbYBgzFswmf4+ZvvYcZLHhyJaxHYqruLvOT3QjFWVAWL0stSrGwWDSohP62TFz7j7Pdr6IHV2Et58Y/n6WRhQg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=awZ+EhHz3MAQDHeUIQlavzxPioldpL8gWEHCiQJnc2065PalY6wpmlB1tjefY9cNS04W60PR849Cq3fUjG+wesgDwUh4p14I11HM2kgjHxnxOJ1LQpQd578PnUaxn4Fx8dIoLVEcHnn/k2AFqYatoGeCEEspwjtqDWXEye60tACv6PSAEAY0W/naZJpPcuMGRUx0p/orfHl+p+Cx6itbDaDYmCAbI8W8wlhyM9+M4JR/8AK1RpDSb94DiqVsU4IybvWDDgah5S9LNWiNrPbN+3m9Xb0C7iwprxTBQBwd1/yM+rZvYk+ISKd/cVNVcHdT2SNgDOe3H63BFZrASasOQQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 26 Jan 2022 16:51:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.