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
