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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Hmm, so maybe the wording in the description is merely a little
unfortunate.

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

Well, if the next patch needs to make adjustments here, then that's
fine but different from what's needed at this point. However, ...

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

... yes, I see.

> Hope all this makes sense,

It does, and ...

> I find it quite complex due to all the interactions.

... yes, I definitely agree.

Jan