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

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests
Date: Fri, 11 Mar 2022 08:31:23 +0100	[thread overview]
Message-ID: <d54a2875-b170-3d38-b025-1654d8c61203@suse.com> (raw)
In-Reply-To: <YioqL+CcQHiwyRwi@Air-de-Roger>

On 10.03.2022 17:41, Roger Pau Monné wrote:
> 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.

Yes, setting SSBD with SPEC_CTRL already / still set ought to be fine.

>> 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.

Hmm, I can't see the change to init_speculation_mitigations()
guaranteeing that at most one of the two would be enabled.

> 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.

I don't think this matters for this particular aspect of my reply.
It was possibly causing some confusion to me, but elsewhere.

Jan