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

2022-04-29 Thread Jan Beulich
On 27.04.2022 12:47, Roger Pau Monne wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -541,6 +541,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.
> @@ -597,6 +601,13 @@ static void __init calculate_hvm_def_policy(void)
>  guest_common_feature_adjustments(hvm_featureset);
>  guest_common_default_feature_adjustments(hvm_featureset);
>  
> +/*
> + * Only expose VIRT_SSBD if AMD_SSBD is not available, and thus
> + * VIRT_SC_MSR_HVM is set.
> + */
> +if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
> +__set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);

The earlier patch sets the bit in "max" when SC_MSR_HVM && AMD_SSBD.
This patch doesn't set the bit in "max" at all (it only clears it in
one case as per the earlier hunk), thus leading to "def" holding a
set bit which supposedly cannot be set. Irrespective of the feature's
'!' annotation I think we'd better not violate "max" >= "def".

Everything else in this patch looks good to me.

Jan




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

2022-04-27 Thread Roger Pau Monne
Allow HVM guests access to MSR_VIRT_SPEC_CTRL if the platform Xen is
running on 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}.

Suggested-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
---
Changes since v3:
 - Always trap write accesses to VIRT_SPEC_CTRL in order to cache the
   guest setting.
 - Do not use the 'S' annotation for the VIRT_SSBD feature.

Changes since v2:
 - Reword part of the commit message regarding annotation change.
 - Fix MSR intercept.
 - Add handling of VIRT_SPEC_CTRL to guest_{rd,wr}msr when using
   VIRT_SSBD also.

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   |  8 ++
 xen/arch/x86/hvm/svm/svm.c | 35 ++
 xen/arch/x86/include/asm/cpufeatures.h |  1 +
 xen/arch/x86/include/asm/msr.h | 10 
 xen/arch/x86/msr.c | 16 +---
 xen/arch/x86/spec_ctrl.c   |  9 ++-
 7 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 979dcf8164..9a8c73f067 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -541,6 +541,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.
@@ -597,6 +601,13 @@ static void __init calculate_hvm_def_policy(void)
 guest_common_feature_adjustments(hvm_featureset);
 guest_common_default_feature_adjustments(hvm_featureset);
 
+/*
+ * Only expose VIRT_SSBD if AMD_SSBD is not available, and thus
+ * VIRT_SC_MSR_HVM is set.
+ */
+if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
+__set_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..2f63a2e3c6 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -19,6 +19,8 @@
 
 .file "svm/entry.S"
 
+#include 
+
 #include 
 #include 
 
@@ -57,6 +59,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 +119,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 2455835eda..e15c9754d7 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,16 @@ 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);
 
+/*
+ * Always trap write accesses to VIRT_SPEC_CTRL in order to cache the guest
+ * setting and avoid having to perform a rdmsr on vmexit to get the guest
+ * setting even if VIRT_SSBD is offered to Xen itself.
+ */
+svm_intercept_msr(v, MSR_VIRT_SPEC_CTRL,
+  cp->extd.virt_ssbd && cpu_has_virt_ssbd &&
+  !cpu_has_amd_ssbd ?
+  MSR_INTERCEPT_WRITE : 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 +3116,30 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 vmcb_set_vintr(vmcb, intr);
 }
 
+/* Called with GIF=0. */
+void vmexit_virt_spec_ctrl(void)
+{
+