Re: [PATCH v3 0/3] amd/msr: implement MSR_VIRT_SPEC_CTRL for HVM guests

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

On Fri, Apr 22, 2022 at 06:49:57PM +0000, Andrew Cooper wrote:
> On 31/03/2022 10:27, Roger Pau Monne wrote:
> > Hello,
> >
> > The following series implements support for MSR_VIRT_SPEC_CTRL
> > (VIRT_SSBD) on different AMD CPU families.
> >
> > Note that the support is added backwards, starting with the newer CPUs
> > that support MSR_SPEC_CTRL and moving to the older ones either using
> > MSR_VIRT_SPEC_CTRL or the SSBD bit in LS_CFG.
> >
> > Xen is still free to use it's own SSBD setting, as the selection is
> > context switched on vm{entry,exit}.
> >
> > On Zen2 and later, SPEC_CTRL.SSBD exists and should be used in
> > preference to VIRT_SPEC_CTRL.SSBD.  However, for migration
> > compatibility, Xen offers VIRT_SSBD to guests (in the max CPUID policy,
> > not default) implemented in terms of SPEC_CTRL.SSBD.
> >
> > On Fam15h thru Zen1, Xen exposes VIRT_SSBD to guests by default to
> > abstract away the model and/or hypervisor specific differences in
> > MSR_LS_CFG/MSR_VIRT_SPEC_CTRL.
> >
> > Note that if the hardware itself does offer VIRT_SSBD (ie: very likely
> > when running virtualized on < Zen2 hardware) and not AMD_SSBD Xen will
> > allow untrapped access to MSR_VIRT_SPEC_CTRL for HVM guests.
> >
> > So the implementation of VIRT_SSBD exposed to HVM guests will use one of
> > the following underlying mechanisms, in the preference order listed
> > below:
> >
> >  * SPEC_CTRL.SSBD. (patch 1)
> >  * VIRT_SPEC_CTRL.SSBD (untrapped). (patch 2).
> >  * Non-architectural way using LS_CFG. (patch 3)
> >
> > This has survived a XenRT basic set of tests on AMD machines.
> 
> Sorry for the mixed feedback, but some is applicable across multiple
> patches.
> 
> First, it is important to know why MSR_VIRT_SPEC_CTRL exists, because
> that informs what is, and is not, sensible to do with it.
> 
> It exists to be a FMS-invariant abstraction of the DE_CFG interface,
> emulated by the hypervisor.  At the time, we experimented with emulating
> MSR_SPEC_CTRL directly, but the results were unusable slow (legacy IBRS
> causing a vmexit on every syscall/interrupt entry&exit) so
> MSR_VIRT_SPEC_CTRL is also an explicit statement that it is an expensive
> operation and shouldn't be used frequently.
> 
> In practice, this means "only for very very important processes, and not
> to be used more frequently than process context switch".  Also, there is
> no hardware which implements MSR_VIRT_SPEC_CTRL, nor will there be.
> 
> Patch 2 has added an extra two vmexits around each vmexit, in an effort

No, it adds just one extra unconditional vmexit, the wrmsr is
conditional to the value in the guest and Xen differing, and it's not
unconditionally executed.

> to let L2 vmexit to L0 rather than L1 for what is likely to be 0 times
> in an L1 timeslice.  It's not a credible optimisation, for something
> which isn't a production usecase.  Yes - nested virt does exist, and is
> useful for dev, but noone runs a fully fat server virt hypervisor at L1
> in production if they care in the slightest about performance.  Either
> way, patch 2 is premature optimisation with a massive complexity cost.

OK, so your recommendation is to trap writes to VIRT_SPEC_CTRL so the
unconditional rdmsr on vmexit is avoided at the cost of taking a
vmexit on all writes to VIRT_SPEC_CTRL, that's indeed fine.

> 
> Furthermore, writes to LS_CFG are also incredibly expensive, even if
> you're not changing any bits.  The AMD recommended algorithm
> specifically avoids rewriting it with the same value as before.

Writes to LS_CFG will only happen if the guest and Xen selection of
SSBD differ, as the guest value is cached.

> Another thing is that Xen shouldn't touch LS_CFG like this if there is
> any hint of a hypervisor on the system.  If there is a hypervisor and it
> doesn't offer VIRT_SPEC_CTRL, trying to play with LS_CFG isn't going to
> make the situation any better.

I would assume that no hypervisor will offer LS_CFG support for
setting SSBD if the guest shouldn't use it, and hence set_legacy_ssbd
will already return false.

> As to the CPUID bit handling, on consideration of the whole series, it
> wants to be "!" only.  ! is there to indicate "something complicated is
> going on with this bit", and life is too short to try and get the
> derivation logic right with both implicit and explicit conditions. 
> Leave it without an s/S (so no auto propagation from the host policy),
> and set it in the max policy for LS_CFG || VIRT_SPEC_CTRL || SPEC_CTRL,
> and set it in the default policy for LS_CFG || VIRT_SPEC_CTRL, which
> will be far clearer to follow.

OK, as both Jan and you agree that using just '!' is fine I can work
with that.

> For `struct ssbd_core`, the name isn't great.  It's more
> ssbd_ls_cfg/state.  Also, each array element wants 64 byte alignment,
> because that's the only way to avoid atomic cacheline pingpong from the
> spinlocks.  Also, the accessors need to be raw, because GIF=0 context is
> weird and working around checklock with irqsave variants is not a clever
> move.  It is not safe to printk()/bug/etc from GIF=0 context, so logic
> needs to be kept to an absolute bare minimum.

Sorry I'm a bit dense today, but I don't seem to be able to find any
raw accessors for our spinlock implementation. _spin_lock_cb will
unconditionally call check_lock and there doesn't seem to be a way to
bypass the checks if lock debugging is enabled.  Am I missing
something?

Thanks, Roger.