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.
