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.
