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

Re: [PATCH v2 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 28 Mar 2022 17:19:58 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; 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=lM8Q8VdPi7mV2bL9P5TFzN1kXs7rH6U8+nD1vY3+orc=; b=Vp53GmiW2ooqJpDU16w+FgWnJhRsUsUhda0DD3qlCG+OQShFu7hviH8ls5vmi9N6Fj2CPu/9VZYntFLAh58ZNbyPXHozegBuHi087Vu8dNvej0k/zBvjtgdD7EFPm2jDosPY7NuvfFtvibxK7Ka4zMT3xWPlmZilvvTileeA1oWJQ18XZCfQAjKsV5a4LTP25GU9Uwq3OPO4rul7m10aqKZdJv3upOVmIfvHD6B9XskDCkmeX6SqEhQ/5y/No20Vxj94hBh9NdRCp10XK3R0rri8M6mwOXrwBh9kYv1OGGA/McFW0C4//ktsC+GXaqfbgPXCmjPZicD7K8BlDnyMtA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=e0IV10VqGBfPaY79kloG3wvKLLLqwBBpyCsNTTMMKZ8cnuHsZo/Ekn71A9+/5Ui0V452yDmvGwJaJaxHrdJBbt27Nv2LfYQgxfhzEKTxl7dCF2lcWepmVxnu8Wgdg3ynlCZ3Rjg0mDMgle8xkTwRjuyVz0oWBVRMbieaUtC3fm7d8GsbRBXKsKsyWtiMhRRaRlOPyHIIUD7Bxud8QACpoWxMVyEAQR/kCnYZZwDrRZMm0BmGEVB8qIq6hQ1qnzBCIy9K2rpC7AKWE655Ycnai3kaummHF55vi0+3V4LA4Wl7MpuVpH1S0QpqN/hPRjuvjQt2vs670nAKUm1OEVdK6g==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 28 Mar 2022 15:20:18 +0000
  • Ironport-data: A9a23:4jA7568SVSvFk5GO6rPTDrUDlH6TJUtcMsCJ2f8bNWPcYEJGY0x3n WUaWT+COvqOZ2rwKdkjOtzl/E8D65GAytFjGQpvpCE8E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si+Fa+Sn9T8mvU2xbuKU5NTsY0idfic5DnZ54f5fs7Rh2NQw2oLpW1rlV e7a+KUzBnf0g1aYDUpMg06zgEsHUCPa4W5wUvQWPJinjXeG/5UnJMt3yZKZdhMUdrJ8DO+iL 9sv+Znilo/vE7XBPfv++lrzWhVirrc/pmFigFIOM0SpqkAqSiDfTs/XnRfTAKtao2zhojx/9 DlCnZa9eB8RGerpobwEWEBhOARYMZdo1ZaSdBBTseTLp6HHW37lwvEoB0AqJ4wIvO1wBAmi9 9RBdmpLNErawbvrnvTrEYGAhex6RCXvFJkYtXx6iynQEN4tQIzZQrWM7thdtNs1rp4RR6uFN 5BGAdZpRC7mfxcQYQxKMshgu+eqt3LPahcbkU3A8MLb5ECMlVcsgdABKuH9atGMAMlYgEucj mbH5HjiRAEXMsSFzjiI+W7qgfXA9QvkXKoCGbv+8eRl6HWRzGEODBwdVXOgvOK0zEW5Xrpix 1c8o3R06/JorQryE4e7D0bQTGO4UgA0UddhC9UdryS0krPfuj+yI04IdBBsQYlz3CMpfgAC2 liMltLvIDVgtryJVH6QnoupQSOO1Ts9djFbO3JdJecRy5y6+dxo0EqTJjp2OPTt5uAZDw0c1 NxjQMIWo7wIxfAG2Kyglbwsq2L9/8OZJuLZC+i+Y45E0u+bTNP+D2BLwQKChRqlEGp/ZgPb1 JTjs5LChN3i9bnXyESwrBwlRdlFHcqtPjzGmkJIFJI87Tmr8HPLVdkOvGEifBgzaJhfJ2+Bj KrvVeV5vsU70JyCN/IfXm5MI55ykfiI+SrNCJg4keaikrAuLVTarUmClGab3nz3kVhErE3ME czzTCpYNl5DUf4P5GPvH481iOZ3rghjlTK7bc2qlHyPjOvBDEN5vJ9YaTNimMhit/jayOgUm v4CX/a3J+J3DbSmO3KNoNdJRb3IRFBiba3LRwVsXrfrCiJtGX07Cu+XxrUkeod/mL9SmPuO9 Xa4MnK0AnKl7ZEbAW1mskxeVY4=
  • Ironport-hdrordr: A9a23:to2mma4cpEPnnhgm2APXwPvXdLJyesId70hD6qkmc20tTiX4rb HKoBx4vSWftN91YhwdcL+7VJVoLUmyyXcX2/hyAV7BZmnbUTCTXedfBOLZqlWKJ8SZzIBgPM xbAstD4bPLbGSTIqzBkXGF+3pL+qjizEgI792uqEtQcQ==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Mar 28, 2022 at 04:02:40PM +0200, Jan Beulich wrote:
> On 15.03.2022 15:18, Roger Pau Monne wrote:
> > Allow HVM guests untrapped access to MSR_VIRT_SPEC_CTRL if the
> > hardware has support for it. This requires adding logic in the
> > vm{entry,exit} paths for SVM in order to context switch between the
> > hypervisor value and the guest one. The added handlers for context
> > switch will also be used for the legacy SSBD support.
> > 
> > Introduce a new synthetic feature leaf (X86_FEATURE_VIRT_SC_MSR_HVM)
> > to signal whether VIRT_SPEC_CTRL needs to be handled on guest
> > vm{entry,exit}.
> > 
> > Note the change in the handling of VIRT_SSBD in the featureset
> > description. The change from 's' to 'S' is due to the fact that now if
> > VIRT_SSBD is exposed by the hardware it can be passed through to HVM
> > guests.
> 
> But lower vs upper case mean "(do not) expose by default", not whether
> underlying hardware exposes the feature. In patch 1 you actually used
> absence in underlying hardware to justify !, not s.

Maybe I'm getting lost with all this !, lower case and upper case
stuff.

Patch 1 uses '!s' to account for:
 * '!': the feature might be exposed to guests even when not present
   on the host hardware.
 * 's': the feature won't be exposed by default.

Which I think matches what is implemented in patch 1 where VIRT_SSBD
is possibly exposed to guest when running on hardware that don't
necessarily have VIRT_SSBD (ie: because we use AMD_SSBD in order to
implement VIRT_SSBD).

Patch 2 changes the 's' to 'S' because this patch introduces support
to expose VIRT_SSBD to guests by default when the host (virtual)
hardware also supports it.

Maybe my understanding of the annotations is incorrect.

> > @@ -610,6 +611,14 @@ static void cf_check svm_cpuid_policy_changed(struct 
> > vcpu *v)
> >      svm_intercept_msr(v, MSR_SPEC_CTRL,
> >                        cp->extd.ibrs ? MSR_INTERCEPT_NONE : 
> > MSR_INTERCEPT_RW);
> >  
> > +    /*
> > +     * Give access to MSR_VIRT_SPEC_CTRL if the guest has been told about 
> > it
> > +     * and the hardware implements it.
> > +     */
> > +    svm_intercept_msr(v, MSR_VIRT_SPEC_CTRL,
> > +                      cp->extd.virt_ssbd && cpu_has_virt_ssbd ?
> 
> Despite giving the guest direct access guest_{rd,wr}msr() can be hit
> for such guests. Don't you need to update what patch 1 added there?

Indeed, I should add the chunk that's added in the next patch.

> Also, is there a reason the qualifier here is not in sync with ...
> 
> > @@ -3105,6 +3114,36 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> >      vmcb_set_vintr(vmcb, intr);
> >  }
> >  
> > +/* Called with GIF=0. */
> > +void vmexit_virt_spec_ctrl(void)
> > +{
> > +    unsigned int val = opt_ssbd ? SPEC_CTRL_SSBD : 0;
> > +
> > +    if ( cpu_has_virt_ssbd )
> 
> ... this one? Since the patching is keyed to VIRT_SC_MSR_HVM, which in
> turn is enabled only when cpu_has_virt_ssbd, it would seem to me that
> if any asymmetry was okay here, then using cp->extd.virt_ssbd without
> cpu_has_virt_ssbd.

Using just cp->extd.virt_ssbd will be wrong when next patch also
introduces support for exposing VIRT_SSBD by setting SSBD using the
non-architectural method.

We need to context switch just based on cpu_has_virt_ssbd because the
running guest might not get VIRT_SSBD offered (cp->extd.virt_ssbd ==
false) but Xen might be using SSBD itself so it needs to context
switch in order to activate it. Ie: if !cp->extd.virt_ssbd then the
guest will always run with SSBD disabled, but Xen might not.

> > @@ -1069,6 +1072,10 @@ void __init init_speculation_mitigations(void)
> >              setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
> >      }
> >  
> > +    /* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
> > +    if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd && cpu_has_virt_ssbd )
> > +        setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);
> 
> In cpuid.c the comment (matching the code there) talks about exposing
> by default. I can't bring this in line with the use of !cpu_has_amd_ssbd
> here.

Exposing by default if !AMD_SSBD. Otherwise VIRT_SSBD is only in the
max policy, and the default policy will instead contain AMD_SSBD.

If AMD_SSBD is available it implies that X86_FEATURE_SC_MSR_HVM is
already set (or otherwise opt_msr_sc_hvm is disabled), and hence the
way to implement VIRT_SSBD is by using SPEC_CTRL.

I think I need to fix the intercept in that case, so it's:

    svm_intercept_msr(v, MSR_VIRT_SPEC_CTRL,
                      cp->extd.virt_ssbd && cpu_has_virt_ssbd &&
                      !cpu_has_amd_ssbd ?
                      MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);

Because it AMD_SSBD is available VIRT_SSBD will be implemented using
SPEC_CTRL, regardless of whether VIRT_SSBD is also available natively.

Hope all this makes sense, I find it quite complex due to all the
interactions.

Thanks, Roger.



 


Rackspace

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