[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: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Wed, 26 Jan 2022 20:16:13 +0000
  • Accept-language: en-GB, en-US
  • 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=TjbYf3qr0mB4cdZhoonTYB0XlIapDqxwV8LmDv+frN8=; b=RVMMhNLlrFqfDXkfpQ4T6wMg/JSb7vM3LfL/s4QmD813LkDsSlxnRV5zhIGdFSO7NRh/mfgLEdVPTG4udmBK2vHwNvazDzwrxZXGCnrLPyKf/BY6VbOoEoXkYLGRQOVNuwsoazbKadPgnDD0/dGLQHfwNOSYn2tay/lgW437w/p3aUAqOfhj+Di+zaXFOoI9/YNU1t1gptBnnZyrtDCCz4LGU/dBD4nIVXIDWCm7zJ2iLQSIBUgKAL8NEUYw1c7cPEKGz2XWv9aH2V8L0FpKn7IQUZ2jrp2/jzHuE7cqNQj53jMn9kXhsUHcPKXH0AwVTawvO0tE3Yghv7rI1s/QEw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KPfv3Ja/wBMP+hKf2Axcy49PqN17Z5upuzYVhu3/RGvxdgKvUga+dPOY9dDA/kmNiEMoUd+9vt2vWMtT+oSuZuZpJAn2iUJ53uh3IoCuJaWgdpkqli4iAAvgxJ5yTE864cdbIW77I9F3ohy1dlW0fRJizW7Bw1uYzBn1ms2qm3LC0NLfz2UjCrYE1nPwTQLvGa9EfQ9jxGb68Pn3+xPgxvO7IhoOREqjhwJgq+aVx2diDLFNVc1cmofHu+uyfqIKt+cuAx887B9egrcTgtdIp9BKntpOSJJy9qhV0XDYsrqLDpFx5cvJD2YVlDAd4cx4pvTxuaHSMhmtu5TgQLVe6A==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 26 Jan 2022 20:16:33 +0000
  • Ironport-data: A9a23:Ni4Nw6pRZVHvj6sdSYy6igfg1OFeBmL0YhIvgKrLsJaIsI4StFCzt garIBmCOqyKY2T3fYgjO4ix/BkA65TTmtBhQQc5qS8xQSITpZuZCYyVIHmrMnLJJKUvbq7GA +byyDXkBJppJpMJjk71atANlZT4vE2xbuKU5NTsY0idfic5Dndx4f5fs7Rh2NQw2ILnW1jlV e7a+KUzBnf0g1aYDUpMg06zgEsHUCPa4W5wUvQWPJinjXeG/5UnJMt3yZKZdhMUdrJ8DO+iL 9sv+Znilo/vE7XBPfv++lrzWhVirrc/pmFigFIOM0SpqkAqSiDfTs/XnRfTAKtao2zhojx/9 DlCnaafZh80I7HRoeUMdyEGKhp5P45t6qCSdBBTseTLp6HHW37lwvEoB0AqJ4wIvO1wBAmi9 9RBdmpLNErawbvrnvTrEYGAhex6RCXvFKoZtmtt0nfyCvE+TIqYa67L+cVZzHE7gcUm8fP2O ZBJMmc0MUqojxtnGw5NF7A/lcGU22jFUhMBpny8o/YZyj2GpOB2+Oe0a4eEEjCQfu1Fk0Ddq m/Y8mDRBhABKMfZ2TeD6mirhOLEgWX8Qo16PKK83u5nhhuU3GN7IB8cWEa/oPK5olWjQN8ZI EsRkhfCtoBrqhbtFIOkGUTl/jjU5XbwRua8DcUAxy6V17fI/j+YF0w8EAd6b94Fn90pEGlCO kCyo/vlAjlmsbuwQH2b96uJoT7aBRX5PVPudgdfE1JbvoCLTJUby0uWE409SPLdYsjdRGmoq w1muhTSkFn6YSQj86ygtW7KjDu3znQiZl5kv16HNo5JA+4QWWJEW2BKwQSKhRqjBNzAJrVkg JTis5LPhAzpJcrV/BFhuM1XQNmUCw+taVUwe2JHEZg77CiK8HW+Z41W6zwWDB43bp1dImK3P BOO6Vw5CHpv0J2CN/4fj2WZUJxC8EQdPY69CqC8giRmP/CdizNrDAkxPBXNjggBYWAnkL0lO IfzTCpfJS1yNEiT9xLvH711+eZynkgWnDqPLbimkUjP+efANRa9FOdUWHPTP7tRxP7V/23oH yN3apHiJ+N3CrOuO0E6MOc7cDg3EJTMLcmm8pMMLr/afFMO9aNII6a5/I7NsrdNxsx9vuzJ4 mu8Sglfzl/+jmfAMgKEdjZob7aHYHq1hShnVcD1FVr3iXUlf6i166ITK8k+cbU9rbQxxv9oV fgVPc6HB60XGDjA/j0ca7j7rZBjK0v31V7fYXL9bWhtZYNkSizI5sTgIlnl+h4RA3flrsA5u bChiF/WGMJRWwR4Ac/KQ/uz1Fft72MFked/UhKQcNlecUnh6qZwLCn1gqNlKs0AM0yblDCby xyXEVETouyU+90599zAhKalqYa1ErQhQhoGTjeDtbvvbHvU5Guux4NEQd2kRzGFWTOm4rima MVU0+r4bK8NkmFVvtcuCL1s168/uYfi/ucI0gR+EXzXRF23Ebc8cGKe1MxCu6ARlL9UvQy6B hCG9tVAYOjbPcrkFBgaJRY/b/TF3vYRw2GA4fMwKUT8xSl24LvYDhkCY0jS0HRQfOlvLYco4 eY9o8pHuQWwhy0jPsuCki0JpX+HKWYNUvl/u5wXaGMxZtHHFr2WjUTgNxLL
  • Ironport-hdrordr: A9a23:cPogaaAmH6XcOZTlHenH55DYdb4zR+YMi2TDtnoQdfU4SKGlfq OV/cjztyWatN95YhhJ8rq90cK7L080m6QY3bUs
  • Ironport-sdr: JJYnYkD4kUzyEG5qn0egiiGJAx9a2rSGOudfEja5LuPCy5tUytR8jwVGw1Y4gZr3B3baS0YoMU CoyhOK32VIqqjCfPYLWX9JUVrcCJfXYbUFeXYuXb/pQwjRAgl5zMVg1R2YGTltWml/q3XpJzfK sKx2sfsbjgsC+a6E6AI0PqFKft2OuGsiYwkfIa0KV3QIuhRgPlugfjaf1qPZ4PE903wZmCozJn pzivMn0q/+0x8mC14MDRBUaUorMppviIjTsi2dkL9rmyU4lFIT19YPARA1fbhmdJb58l5frPaY EpXFGSKu9PYIQ/SDuWtC4/bQ
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYEpEQO22I/y1Di0S4u/YrCMvx5qx1hJwAgAA5Y4A=
  • Thread-topic: [PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

On 26/01/2022 16:50, Jan Beulich wrote:
> 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?

Hmm

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index c718328ac4cf..1d4be7e97ae2 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -59,6 +59,7 @@ __UNLIKELY_END(nsvm_hap)
 
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
         /* SPEC_CTRL_EXIT_TO_SVM       Req:                          
Clob: C   */
+        ALTERNATIVE "", __stringify(mov %rbx, %rdi; mov %rsp, %rsi),
X86_FEATURE_SC_MSR_HVM
         ALTERNATIVE "", __stringify(call vmentry_spec_ctrl),
X86_FEATURE_SC_MSR_HVM
 
         pop  %r15

is somewhat of a long line, but isn't too terrible.

I'm tempted to switch back to using STR() seeing as we have both and it
is much more concise.
>> --- 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"?

That was the intention, yes.

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

Fair point.  I'll switch to just C.

The clobbers are rather more important for the PV side where the logic
has multiple live variables and it's not totally obvious that all GPRs
are available.

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

Ish.

We intentionally write MSR_SPEC_CTRL once on each CPU to clear out any
previous-environment settings, but those boot paths need to latch
last_spec_ctrl too for this to work correctly.

Making this safe is slightly nasty.  I think the best option would be to
reorder this patch to be after the patch 6, and tweak the wording in
patch 6's commit message.  That way, we're not adding latching to
later-dropped codepaths.

~Andrew

 


Rackspace

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