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

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

On Mon, Feb 14, 2022 at 05:02:52PM +0100, Jan Beulich wrote:
> On 01.02.2022 17:46, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/svm/entry.S
> > +++ b/xen/arch/x86/hvm/svm/entry.S
> > @@ -71,7 +71,9 @@ __UNLIKELY_END(nsvm_hap)
> >              mov    %al, CPUINFO_last_spec_ctrl(%rsp)
> >  1:          /* No Spectre v1 concerns.  Execution will hit VMRUN 
> > imminently. */
> >          .endm
> > -        ALTERNATIVE "", svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> > +        ALTERNATIVE_2 "", STR(call vmentry_virt_spec_ctrl), \
> 
> I'm afraid this violates the "ret" part of the warning a few lines up,
> while ...
> 
> > +                          X86_FEATURE_VIRT_SC_MSR_HVM, \
> > +                      svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> >  
> >          pop  %r15
> >          pop  %r14
> > @@ -111,7 +113,9 @@ __UNLIKELY_END(nsvm_hap)
> >              wrmsr
> >              mov    %al, CPUINFO_last_spec_ctrl(%rsp)
> >          .endm
> > -        ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> > +        ALTERNATIVE_2 "", STR(call vmexit_virt_spec_ctrl), \
> 
> ... this violates ...
> 
> > +                          X86_FEATURE_VIRT_SC_MSR_HVM, \
> > +                      svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> >          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> 
> ... the "ret" part of this warning.

Hm, so while I could load VIRT_SPEC_CTRL easily from assembly, loading
of the legacy non-architectural setting of SSBD for Fam18h and earlier
it's likely not doable from assembly.

Since those helpers would only set SSBD, isn't it fine to execute a
`ret` after either having set or clear SSBD?

AFAICT the requirement would be to either have loaded SPEC_CTRL first
(if present) in the VM exit path, or to set SSBD before setting
SPEC_CTRL in the VM entry path.

> Furthermore, opposite to what the change to amd_init_ssbd() suggests,
> the ordering of the alternatives here means you prefer SPEC_CTRL over
> VIRT_SPEC_CTRL; see the comment near the top of _apply_alternatives().
> Unless I've missed logic guaranteeing that both of the keyed to
> features can't be active at the same time.

Xen itself will only use a single one (either SPEC_CTRL.SSBD or
VIRT_SPEC_CTRL.SSBD) in order to implement support on behalf of the
guest. amd_init_ssbd already prefer to use SPEC_CTRL.SSBD over
VIRT_SPEC_CTRL.SSBD when both are available, so we aim to do the same
here.

I think part of the confusion steams from using info->{last_spec_ctrl,
xen_spec_ctrl} even when SPEC_CTRL MSR is not used by Xen, I need to
clarify this somehow, maybe by not using those fields in the first
place.

Thanks, Roger.