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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.