Re: [PATCH v2 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests
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
Re: [PATCH v2 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests
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.
Maybe my understanding of the annotations is incorrect.
> > @@ -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.
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.
> > @@ -1069,6 +1072,10 @@ void __init init_speculation_mitigations(void)
> > setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
> > }
> >
> > +/* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
> > +if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd && cpu_has_virt_ssbd )
> > +setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);
>
> In cpuid.c the comment (matching the code there) talks about exposing
> by default. I can't bring this in line with the use of !cpu_has_amd_ssbd
> here.
Exposing by default if !AMD_SSBD. Otherwise VIRT_SSBD is only in the
max policy, and the default policy will instead contain AMD_SSBD.
If AMD_SSBD is available it implies that X86_FEATURE_SC_MSR_HVM is
already set (or otherwise opt_msr_sc_hvm is disabled), and hence the
way to implement VIRT_SSBD is by using SPEC_CTRL.
I think I need to fix the intercept in that case, so it's:
svm_intercept_msr(v, MSR_VIRT_SPEC_CTRL,
cp->extd.virt_ssbd && cpu_has_virt_ssbd &&
!cpu_has_amd_ssbd ?
MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
Because it AMD_SSBD is available VIRT_SSBD will be implemented using
SPEC_CTRL, regardless of whether VIRT_SSBD is also available natively.
Hope all this makes sense, I find it quite complex due to all the
interactions.
Thanks, Roger.
Re: [PATCH v2 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests
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.
> @@ -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?
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.
> @@ -1069,6 +1072,10 @@ void __init init_speculation_mitigations(void)
> setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
> }
>
> +/* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
> +if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd && cpu_has_virt_ssbd )
> +setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);
In cpuid.c the comment (matching the code there) talks about exposing
by default. I can't bring this in line with the use of !cpu_has_amd_ssbd
here.
Jan
[PATCH v2 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests
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.
Suggested-by: Andrew Cooper
Signed-off-by: Roger Pau Monné
---
Changes since v1:
- Introduce virt_spec_ctrl vCPU field.
- Context switch VIRT_SPEC_CTRL on vmentry/vmexit separately from
SPEC_CTRL.
---
xen/arch/x86/cpuid.c| 11 ++
xen/arch/x86/hvm/svm/entry.S| 6
xen/arch/x86/hvm/svm/svm.c | 39 +
xen/arch/x86/include/asm/cpufeatures.h | 1 +
xen/arch/x86/include/asm/msr.h | 10 ++
xen/arch/x86/spec_ctrl.c| 9 -
xen/include/public/arch-x86/cpufeatureset.h | 2 +-
7 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 4ca77ea870..91e53284fc 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -534,6 +534,10 @@ static void __init calculate_hvm_max_policy(void)
raw_cpuid_policy.basic.sep )
__set_bit(X86_FEATURE_SEP, hvm_featureset);
+if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
+/* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
+__clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
+
/*
* If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
* availability, or admin choice), hide the feature.
@@ -590,6 +594,13 @@ static void __init calculate_hvm_def_policy(void)
guest_common_feature_adjustments(hvm_featureset);
guest_common_default_feature_adjustments(hvm_featureset);
+/*
+ * AMD_SSBD if preferred over VIRT_SSBD, so don't expose the later by
+ * default if the former is available.
+ */
+if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
+__clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
+
sanitise_featureset(hvm_featureset);
cpuid_featureset_to_policy(hvm_featureset, p);
recalculate_xstate(p);
diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index 4ae55a2ef6..e2c104bb1e 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -57,6 +57,9 @@ __UNLIKELY_END(nsvm_hap)
clgi
+ALTERNATIVE "", STR(call vmentry_virt_spec_ctrl), \
+X86_FEATURE_VIRT_SC_MSR_HVM
+
/* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
/* SPEC_CTRL_EXIT_TO_SVM Req: b=curr %rsp=regs/cpuinfo, Clob:
acd */
.macro svm_vmentry_spec_ctrl
@@ -114,6 +117,9 @@ __UNLIKELY_END(nsvm_hap)
ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+ALTERNATIVE "", STR(call vmexit_virt_spec_ctrl), \
+X86_FEATURE_VIRT_SC_MSR_HVM
+
stgi
GLOBAL(svm_stgi_label)
mov %rsp,%rdi
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 64a45045da..73a0af599b 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -52,6 +52,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -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 ?
+ MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
+
/* Give access to MSR_PRED_CMD if the guest has been told about it. */
svm_intercept_msr(v, MSR_PRED_CMD,
cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
@@ -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 )
+{
+unsigned int lo, hi;
+
+/*
+ * Need to read from the hardware because VIRT_SPEC_CTRL is not context
+ * switched by the hardware, and we allow the guest untrapped access to
+ * the register.
+ */
+rdmsr(MSR_VIRT_SPEC_CTRL,
