diff mbox series

[v2,9/9] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

Message ID 20220128132927.14997-10-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86: MSR_SPEC_CTRL support for SVM guests | expand

Commit Message

Andrew Cooper Jan. 28, 2022, 1:29 p.m. UTC
With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM guests.

Update the CPUID derivation logic (both PV and HVM to avoid losing subtle
changes), drop the MSR intercept, and explicitly enable the CPUID bits for HVM
guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * Drop the MSR intercept too
 * Rework the comment block in gen-cpuid.py
 * Fix typo in comment
---
 xen/arch/x86/cpuid.c                        | 16 ++++++++++++----
 xen/arch/x86/hvm/svm/svm.c                  |  4 ++++
 xen/include/public/arch-x86/cpufeatureset.h | 16 ++++++++--------
 xen/tools/gen-cpuid.py                      | 14 +++++++++-----
 4 files changed, 33 insertions(+), 17 deletions(-)

Comments

Jan Beulich Jan. 31, 2022, 10:39 a.m. UTC | #1
On 28.01.2022 14:29, Andrew Cooper wrote:
> With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM guests.
> 
> Update the CPUID derivation logic (both PV and HVM to avoid losing subtle
> changes), drop the MSR intercept, and explicitly enable the CPUID bits for HVM
> guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Oneremark:

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -606,6 +606,10 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
>  
>      vmcb_set_exception_intercepts(vmcb, bitmap);
>  
> +    /* Give access to MSR_SPEC_CTRL if the guest has been told about it. */
> +    svm_intercept_msr(v, MSR_SPEC_CTRL,
> +                      cp->extd.ibrs ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);

Technically I suppose the intercept would also be unneeded if the MSR
doesn't exist at all, as then the CPU would raise #GP(0) for any guest
attempt to access it.

Jan
Andrew Cooper Jan. 31, 2022, 11:54 a.m. UTC | #2
On 31/01/2022 10:39, Jan Beulich wrote:
> On 28.01.2022 14:29, Andrew Cooper wrote:
>> With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM guests.
>>
>> Update the CPUID derivation logic (both PV and HVM to avoid losing subtle
>> changes), drop the MSR intercept, and explicitly enable the CPUID bits for HVM
>> guests.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> Oneremark:
>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -606,6 +606,10 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
>>  
>>      vmcb_set_exception_intercepts(vmcb, bitmap);
>>  
>> +    /* Give access to MSR_SPEC_CTRL if the guest has been told about it. */
>> +    svm_intercept_msr(v, MSR_SPEC_CTRL,
>> +                      cp->extd.ibrs ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
> Technically I suppose the intercept would also be unneeded if the MSR
> doesn't exist at all, as then the CPU would raise #GP(0) for any guest
> attempt to access it.

Yes, but that is very dangerous.  There are known examples of real model
specific registers in the place where architectural ones also exist. 
The Haswell uarch has two non-faulting MSRs in the x2APIC range.

A guest poking MSR_SPEC_CTRL when it isn't enumerated is not a path
which needs optimising, and taking a vmexit is more robust against
model-specific behaviour.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index b5af48324aef..e24dd283e761 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -433,6 +433,8 @@  static void __init guest_common_feature_adjustments(uint32_t *fs)
      */
     if ( test_bit(X86_FEATURE_IBRSB, fs) )
         __set_bit(X86_FEATURE_STIBP, fs);
+    if ( test_bit(X86_FEATURE_IBRS, fs) )
+        __set_bit(X86_FEATURE_AMD_STIBP, fs);
 
     /*
      * On hardware which supports IBRS/IBPB, we can offer IBPB independently
@@ -456,11 +458,14 @@  static void __init calculate_pv_max_policy(void)
         pv_featureset[i] &= pv_max_featuremask[i];
 
     /*
-     * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of
-     * administrator choice, hide the feature.
+     * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests (functional
+     * availability, or admin choice), hide the feature.
      */
     if ( !boot_cpu_has(X86_FEATURE_SC_MSR_PV) )
+    {
         __clear_bit(X86_FEATURE_IBRSB, pv_featureset);
+        __clear_bit(X86_FEATURE_IBRS, pv_featureset);
+    }
 
     guest_common_feature_adjustments(pv_featureset);
 
@@ -530,11 +535,14 @@  static void __init calculate_hvm_max_policy(void)
         __set_bit(X86_FEATURE_SEP, hvm_featureset);
 
     /*
-     * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests because of
-     * administrator choice, hide the feature.
+     * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
+     * availability, or admin choice), hide the feature.
      */
     if ( !boot_cpu_has(X86_FEATURE_SC_MSR_HVM) )
+    {
         __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
+        __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
+    }
 
     /*
      * With VT-x, some features are only supported by Xen if dedicated
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index aa82fe29befb..01ce6c71b5f8 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -606,6 +606,10 @@  static void svm_cpuid_policy_changed(struct vcpu *v)
 
     vmcb_set_exception_intercepts(vmcb, bitmap);
 
+    /* Give access to MSR_SPEC_CTRL if the guest has been told about it. */
+    svm_intercept_msr(v, MSR_SPEC_CTRL,
+                      cp->extd.ibrs ? 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);
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index fd8ab2572304..957df23b65f2 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -256,18 +256,18 @@  XEN_CPUFEATURE(CLZERO,        8*32+ 0) /*A  CLZERO instruction */
 XEN_CPUFEATURE(RSTR_FP_ERR_PTRS, 8*32+ 2) /*A  (F)X{SAVE,RSTOR} always saves/restores FPU Error pointers */
 XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*   WBNOINVD instruction */
 XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support only (no IBRS, used by AMD) */
-XEN_CPUFEATURE(IBRS,          8*32+14) /*   MSR_SPEC_CTRL.IBRS */
-XEN_CPUFEATURE(AMD_STIBP,     8*32+15) /*   MSR_SPEC_CTRL.STIBP */
-XEN_CPUFEATURE(IBRS_ALWAYS,   8*32+16) /*   IBRS preferred always on */
-XEN_CPUFEATURE(STIBP_ALWAYS,  8*32+17) /*   STIBP preferred always on */
-XEN_CPUFEATURE(IBRS_FAST,     8*32+18) /*   IBRS preferred over software options */
-XEN_CPUFEATURE(IBRS_SAME_MODE, 8*32+19) /*   IBRS provides same-mode protection */
+XEN_CPUFEATURE(IBRS,          8*32+14) /*S  MSR_SPEC_CTRL.IBRS */
+XEN_CPUFEATURE(AMD_STIBP,     8*32+15) /*S  MSR_SPEC_CTRL.STIBP */
+XEN_CPUFEATURE(IBRS_ALWAYS,   8*32+16) /*S  IBRS preferred always on */
+XEN_CPUFEATURE(STIBP_ALWAYS,  8*32+17) /*S  STIBP preferred always on */
+XEN_CPUFEATURE(IBRS_FAST,     8*32+18) /*S  IBRS preferred over software options */
+XEN_CPUFEATURE(IBRS_SAME_MODE, 8*32+19) /*S  IBRS provides same-mode protection */
 XEN_CPUFEATURE(NO_LMSL,       8*32+20) /*S  EFER.LMSLE no longer supported. */
 XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*   Protected Processor Inventory Number */
-XEN_CPUFEATURE(AMD_SSBD,      8*32+24) /*   MSR_SPEC_CTRL.SSBD available */
+XEN_CPUFEATURE(AMD_SSBD,      8*32+24) /*S  MSR_SPEC_CTRL.SSBD available */
 XEN_CPUFEATURE(VIRT_SSBD,     8*32+25) /*   MSR_VIRT_SPEC_CTRL.SSBD */
 XEN_CPUFEATURE(SSB_NO,        8*32+26) /*A  Hardware not vulnerable to SSB */
-XEN_CPUFEATURE(PSFD,          8*32+28) /*   MSR_SPEC_CTRL.PSFD */
+XEN_CPUFEATURE(PSFD,          8*32+28) /*S  MSR_SPEC_CTRL.PSFD */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0.edx, word 9 */
 XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A  AVX512 Neural Network Instructions */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 470cd76d1c52..39c8b0c77465 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -277,16 +277,20 @@  def crunch_numbers(state):
         # The features:
         #   * Single Thread Indirect Branch Predictors
         #   * Speculative Store Bypass Disable
+        #   * Predictive Store Forward Disable
         #
-        # enumerate new bits in MSR_SPEC_CTRL, which is enumerated by Indirect
-        # Branch Restricted Speculation/Indirect Branch Prediction Barrier.
+        # enumerate new bits in MSR_SPEC_CTRL, and technically enumerate
+        # MSR_SPEC_CTRL itself.  AMD further enumerates hints to guide OS
+        # behaviour.
         #
-        # In practice, these features also enumerate the presense of
-        # MSR_SPEC_CTRL.  However, no real hardware will exist with SSBD but
-        # not IBRSB, and we pass this MSR directly to guests.  Treating them
+        # However, no real hardware will exist with e.g. SSBD but not
+        # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
         # as dependent features simplifies Xen's logic, and prevents the guest
         # from seeing implausible configurations.
         IBRSB: [STIBP, SSBD],
+        IBRS: [AMD_STIBP, AMD_SSBD, PSFD,
+               IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
+        AMD_STIBP: [STIBP_ALWAYS],
 
         # In principle the TSXLDTRK insns could also be considered independent.
         RTM: [TSXLDTRK],