[PATCH 0/8] x86: MSR_SPEC_CTRL support for SVM guests

[PATCH 0/8] x86: MSR_SPEC_CTRL support for SVM guests

[Andrew Cooper]

Fixes/extensions to allow HVM guests to use AMD hardware MSR_SPEC_CTRL
facilities.

No PV support yet - that will require some substantially more careful
unpicking of the PV entry/exit asm.

Andrew Cooper (8):
  x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL
  x86/boot: Collect AMD speculative features earlier during boot
  x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL
  x86/spec-ctrl: Drop use_spec_ctrl boolean
  x86/spec-ctrl: Introduce new has_spec_ctrl boolean
  x86/spec-ctrl: Use common MSR_SPEC_CTRL logic for AMD
  x86/msr: AMD MSR_SPEC_CTRL infrastructure
  x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

 xen/arch/x86/cpu/amd.c                      |  2 +-
 xen/arch/x86/cpu/common.c                   | 15 +++++---
 xen/arch/x86/cpuid.c                        | 16 ++++++---
 xen/arch/x86/hvm/hvm.c                      | 25 +++++++++++--
 xen/arch/x86/hvm/svm/entry.S                | 10 +++---
 xen/arch/x86/hvm/svm/svm.c                  | 39 ++++++++++++++++++++
 xen/arch/x86/include/asm/current.h          |  2 +-
 xen/arch/x86/include/asm/msr.h              | 11 ++++++
 xen/arch/x86/include/asm/spec_ctrl_asm.h    |  7 ++++
 xen/arch/x86/msr.c                          | 37 ++++++++++++-------
 xen/arch/x86/spec_ctrl.c                    | 56 ++++++++++++++++++++---------
 xen/include/public/arch-x86/cpufeatureset.h | 18 +++++-----
 xen/tools/gen-cpuid.py                      |  5 +++
 13 files changed, 187 insertions(+), 56 deletions(-)

-- 
2.11.0

[PATCH 1/8] x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL

[Andrew Cooper]

This bug existed in early in 2018 between MSR_SPEC_CTRL arriving in microcode,
and SSBD arriving a few months later.  It went unnoticed presumably because
everyone was busy rebooting everything.

The same bug will reappear when adding PSFD support.

Clamp the guest MSR_SPEC_CTRL value to that permitted by CPUID on migrate.
The guest is already playing with reserved bits at this point, and clamping
the value will prevent a migration to a less capable host from failing.

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>
---
 xen/arch/x86/hvm/hvm.c         | 25 +++++++++++++++++++++++--
 xen/arch/x86/include/asm/msr.h |  2 ++
 xen/arch/x86/msr.c             | 33 +++++++++++++++++++++------------
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d7d3299b431e..c4ddb8607d9c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1340,6 +1340,7 @@ static const uint32_t msrs_to_send[] = {
 
 static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
 {
+    const struct domain *d = v->domain;
     struct hvm_save_descriptor *desc = _p(&h->data[h->cur]);
     struct hvm_msr *ctxt;
     unsigned int i;
@@ -1355,7 +1356,8 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
     for ( i = 0; i < ARRAY_SIZE(msrs_to_send); ++i )
     {
         uint64_t val;
-        int rc = guest_rdmsr(v, msrs_to_send[i], &val);
+        unsigned int msr = msrs_to_send[i];
+        int rc = guest_rdmsr(v, msr, &val);
 
         /*
          * It is the programmers responsibility to ensure that
@@ -1375,7 +1377,26 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
         if ( !val )
             continue; /* Skip empty MSRs. */
 
-        ctxt->msr[ctxt->count].index = msrs_to_send[i];
+        /*
+         * Guests are given full access to certain MSRs for performance
+         * reasons.  A consequence is that Xen is unable to enforce that all
+         * bits disallowed by the CPUID policy yield #GP, and an enterprising
+         * guest may be able to set and use a bit it ought to leave alone.
+         *
+         * When migrating from a more capable host to a less capable one, such
+         * bits may be rejected by the destination, and the migration failed.
+         *
+         * Discard such bits here on the source side.  Such bits have reserved
+         * behaviour, and the guest has only itself to blame.
+         */
+        switch ( msr )
+        {
+        case MSR_SPEC_CTRL:
+            val &= msr_spec_ctrl_valid_bits(d->arch.cpuid);
+            break;
+        }
+
+        ctxt->msr[ctxt->count].index = msr;
         ctxt->msr[ctxt->count++].val = val;
     }
 
diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
index 10039c2d227b..657a3295613d 100644
--- a/xen/arch/x86/include/asm/msr.h
+++ b/xen/arch/x86/include/asm/msr.h
@@ -277,6 +277,8 @@ static inline void wrmsr_tsc_aux(uint32_t val)
     }
 }
 
+uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp);
+
 extern struct msr_policy     raw_msr_policy,
                             host_msr_policy,
                           pv_max_msr_policy,
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 2cc355575d45..5e80c8b47c21 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -435,6 +435,24 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
     return X86EMUL_EXCEPTION;
 }
 
+/*
+ * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
+ * separate CPUID features for this functionality, but only set will be
+ * active.
+ */
+uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp)
+{
+    bool ssbd = cp->feat.ssbd;
+
+    /*
+     * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored)
+     * when STIBP isn't enumerated in hardware.
+     */
+    return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
+            (ssbd       ? SPEC_CTRL_SSBD       : 0) |
+            0);
+}
+
 int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 {
     const struct vcpu *curr = current;
@@ -508,18 +526,9 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
 
     case MSR_SPEC_CTRL:
-        if ( !cp->feat.ibrsb )
-            goto gp_fault; /* MSR available? */
-
-        /*
-         * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored)
-         * when STIBP isn't enumerated in hardware.
-         */
-        rsvd = ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
-                 (cp->feat.ssbd ? SPEC_CTRL_SSBD : 0));
-
-        if ( val & rsvd )
-            goto gp_fault; /* Rsvd bit set? */
+        if ( !cp->feat.ibrsb ||
+             (val & ~msr_spec_ctrl_valid_bits(cp)) )
+            goto gp_fault;
         goto set_reg;
 
     case MSR_PRED_CMD:
-- 
2.11.0

[PATCH 2/8] x86/boot: Collect AMD speculative features earlier during boot

[Andrew Cooper]

All AMD IBRS-related features are in CPUID.0x80000008.ebx.  Collect them in
early_cpu_init() so init_speculative_mitigations() can use them.

Rework the existing logic structure to fill in c->extended_cpuid_level and
separate out the ambiguous use of ebx in an otherwise 0x80000008-specific
logic block.

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>
---
 xen/arch/x86/cpu/common.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 4a163afbfc7e..866f1a516447 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -348,9 +348,13 @@ void __init early_cpu_init(void)
 	}
 
 	eax = cpuid_eax(0x80000000);
-	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
-		ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
-		eax = cpuid_eax(0x80000008);
+	if ((eax >> 16) == 0x8000)
+		c->extended_cpuid_level = eax;
+
+	if (c->extended_cpuid_level >= 0x80000008) {
+		cpuid(0x80000008, &eax,
+		      &c->x86_capability[cpufeat_word(X86_FEATURE_CLZERO)],
+		      &ecx, &edx);
 
 		paddr_bits = eax & 0xff;
 		if (paddr_bits > PADDR_BITS)
@@ -363,10 +367,11 @@ void __init early_cpu_init(void)
 		hap_paddr_bits = ((eax >> 16) & 0xff) ?: paddr_bits;
 		if (hap_paddr_bits > PADDR_BITS)
 			hap_paddr_bits = PADDR_BITS;
+	}
 
+	if (c->extended_cpuid_level >= 0x8000001f)
 		/* Account for SME's physical address space reduction. */
-		paddr_bits -= (ebx >> 6) & 0x3f;
-	}
+		paddr_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;
 
 	if (!(c->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)))
 		park_offline_cpus = opt_mce;
-- 
2.11.0

[PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

Hardware maintains both host and guest versions of MSR_SPEC_CTRL, but guests
run with the logical OR of both values.  Therefore, in principle we want to
clear Xen's value before entering the guest.  However, for migration
compatibiltiy, and for performance reasons with SEV-SNP guests, we want the
ability to use a nonzero value behind the guest's back.  Use vcpu_msrs to hold
this value, with the guest value in the VMCB.

On the VMEntry path, adjusting MSR_SPEC_CTRL must be done after CLGI so as to
be atomic with respect to NMIs/etc.  The loading of spec_ctrl_raw into %eax
was also stale from the unused old code, so can be dropped too.

Implement both pieces of logic as small pieces of C, and alternative the call
to get there based on X86_FEATURE_SC_MSR_HVM.  While adjusting the clobber
lists, drop the stale requirements on the VMExit side.

The common case is that host and "guest-protection" values are both 0, so
maintain a per-cpu last_spec_ctrl value to allow us to skip redundant WRMSRs.
The value needs to live in the cpu_info block for subsequent use with PV
guests, and compatibility with XPTI.

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>

Several points:

1) It would be slightly more efficient to pass curr and cpu_info into
   vm{entry,exit}_spec_ctrl(), but setup of such state can't be in the
   ALTERNATIVE block because then the call displacement won't get fixed up.
   All the additional accesses are hot off the stack, so almost certainly
   negligible compared to the WRMSR.

2) The RAS[:32] flushing side effect is under reconsideration.  It is actually
   a very awkward side effect in practice, and not applicable to any
   implementations (that I'm aware of), but for now, it's the documented safe
   action to take.  Furthermore, it avoids complicating the logic with an
   lfence in the else case for Spectre v1 safety.
---
 xen/arch/x86/hvm/svm/entry.S             | 10 +++++-----
 xen/arch/x86/hvm/svm/svm.c               | 30 ++++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/current.h       |  2 +-
 xen/arch/x86/include/asm/msr.h           |  9 +++++++++
 xen/arch/x86/include/asm/spec_ctrl_asm.h |  7 +++++++
 5 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index 276215d36aff..c718328ac4cf 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -55,11 +55,11 @@ __UNLIKELY_END(nsvm_hap)
         mov  %rsp, %rdi
         call svm_vmenter_helper
 
-        mov VCPU_arch_msrs(%rbx), %rax
-        mov VCPUMSR_spec_ctrl_raw(%rax), %eax
+        clgi
 
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
-        /* SPEC_CTRL_EXIT_TO_SVM   (nothing currently) */
+        /* SPEC_CTRL_EXIT_TO_SVM       Req:                           Clob: C   */
+        ALTERNATIVE "", __stringify(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM
 
         pop  %r15
         pop  %r14
@@ -78,7 +78,6 @@ __UNLIKELY_END(nsvm_hap)
         pop  %rsi
         pop  %rdi
 
-        clgi
         sti
         vmrun
 
@@ -86,8 +85,9 @@ __UNLIKELY_END(nsvm_hap)
 
         GET_CURRENT(bx)
 
-        /* SPEC_CTRL_ENTRY_FROM_SVM    Req: b=curr %rsp=regs/cpuinfo, Clob: ac  */
+        /* SPEC_CTRL_ENTRY_FROM_SVM    Req:                           Clob: ac,C */
         ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
+        ALTERNATIVE "", __stringify(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
         stgi
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index bb6b8e560a9f..8fdb530b4004 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -3086,6 +3086,36 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     vmcb_set_vintr(vmcb, intr);
 }
 
+/* Called with GIF=0. */
+void vmexit_spec_ctrl(void)
+{
+    struct cpu_info *info = get_cpu_info();
+    unsigned int val = info->xen_spec_ctrl;
+
+    /*
+     * Write to MSR_SPEC_CTRL unconditionally, for the RAS[:32] flushing side
+     * effect.
+     */
+    wrmsr(MSR_SPEC_CTRL, val, 0);
+    info->last_spec_ctrl = val;
+}
+
+/* Called with GIF=0. */
+void vmentry_spec_ctrl(void)
+{
+    struct cpu_info *info = get_cpu_info();
+    const struct vcpu *curr = current;
+    unsigned int val = curr->arch.msrs->spec_ctrl.raw;
+
+    if ( val != info->last_spec_ctrl )
+    {
+        wrmsr(MSR_SPEC_CTRL, val, 0);
+        info->last_spec_ctrl = val;
+    }
+
+    /* No Spectre v1 concerns.  Execution is going to hit VMRUN imminently. */
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/include/asm/current.h b/xen/arch/x86/include/asm/current.h
index cfbedc31983f..dc0edd9ed07d 100644
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -56,6 +56,7 @@ struct cpu_info {
     /* See asm/spec_ctrl_asm.h for usage. */
     unsigned int shadow_spec_ctrl;
     uint8_t      xen_spec_ctrl;
+    uint8_t      last_spec_ctrl;
     uint8_t      spec_ctrl_flags;
 
     /*
@@ -73,7 +74,6 @@ struct cpu_info {
      */
     bool         use_pv_cr3;
 
-    unsigned long __pad;
     /* get_stack_bottom() must be 16-byte aligned */
 };
 
diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
index 657a3295613d..ce4fe51afe54 100644
--- a/xen/arch/x86/include/asm/msr.h
+++ b/xen/arch/x86/include/asm/msr.h
@@ -297,6 +297,15 @@ struct vcpu_msrs
      *
      * For VT-x guests, the guest value is held in the MSR guest load/save
      * list.
+     *
+     * For SVM, the guest value lives in the VMCB, and hardware saves/restores
+     * the host value automatically.  However, guests run with the OR of the
+     * host and guest value, which allows Xen to set protections behind the
+     * guest's back.
+     *
+     * We must clear/restore Xen's value before/after VMRUN to avoid unduly
+     * influencing the guest.  In order to support "behind the guest's back"
+     * protections, we load this value (commonly 0) before VMRUN.
      */
     struct {
         uint32_t raw;
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index bf82528a12ae..02b3b18ce69f 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -46,6 +46,9 @@
  *   - On VMX by using MSR load/save lists to have vmentry/exit atomically
  *     load/save the guest value.  Xen's value is loaded in regular code, and
  *     there is no need to use the shadow logic (below).
+ *   - On SVM by altering MSR_SPEC_CTRL inside the CLGI/STGI region.  This
+ *     makes the changes atomic with respect to NMIs/etc, so no need for
+ *     shadowing logic.
  *
  * Factor 2 is harder.  We maintain a shadow_spec_ctrl value, and a use_shadow
  * boolean in the per cpu spec_ctrl_flags.  The synchronous use is:
@@ -67,6 +70,10 @@
  * steps 2 and 6 will restore the shadow value rather than leaving Xen's value
  * loaded and corrupting the value used in guest context.
  *
+ * Additionally, in some cases it is safe to skip writes to MSR_SPEC_CTRL when
+ * we don't require any of the side effects of an identical write.  Maintain a
+ * per-cpu last_spec_ctrl value for this purpose.
+ *
  * The following ASM fragments implement this algorithm.  See their local
  * comments for further details.
  *  - SPEC_CTRL_ENTRY_FROM_PV
-- 
2.11.0

[PATCH 4/8] x86/spec-ctrl: Drop use_spec_ctrl boolean

[Andrew Cooper]

Several bugfixes have reduced the utility of this variable from it's original
purpose, and now all it does is aid in the setup of SCF_ist_wrmsr.

Simplify the logic by drop the variable, and doubling up the setting of
SCF_ist_wrmsr for the PV and HVM blocks, which will make the AMD SPEC_CTRL
support easier to follow.  Leave a comment explaining why SCF_ist_wrmsr is
still necessary for the VMExit case.

No functional change.

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>
---
 xen/arch/x86/spec_ctrl.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index c18cc8aa493a..8a550d0a0902 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -927,7 +927,7 @@ static __init void mds_calculations(uint64_t caps)
 void __init init_speculation_mitigations(void)
 {
     enum ind_thunk thunk = THUNK_DEFAULT;
-    bool use_spec_ctrl = false, ibrs = false, hw_smt_enabled;
+    bool ibrs = false, hw_smt_enabled;
     bool cpu_has_bug_taa;
     uint64_t caps = 0;
 
@@ -1016,19 +1016,21 @@ void __init init_speculation_mitigations(void)
     {
         if ( opt_msr_sc_pv )
         {
-            use_spec_ctrl = true;
+            default_spec_ctrl_flags |= SCF_ist_wrmsr;
             setup_force_cpu_cap(X86_FEATURE_SC_MSR_PV);
         }
 
         if ( opt_msr_sc_hvm )
         {
-            use_spec_ctrl = true;
+            /*
+             * While the guest MSR_SPEC_CTRL value is loaded/saved atomically,
+             * Xen's value is not restored atomically.  An early NMI hitting
+             * the VMExit path needs to restore Xen's value for safety.
+             */
+            default_spec_ctrl_flags |= SCF_ist_wrmsr;
             setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
         }
 
-        if ( use_spec_ctrl )
-            default_spec_ctrl_flags |= SCF_ist_wrmsr;
-
         if ( ibrs )
             default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
     }
-- 
2.11.0

[PATCH 5/8] x86/spec-ctrl: Introduce new has_spec_ctrl boolean

[Andrew Cooper]

Most MSR_SPEC_CTRL setup will be common between Intel and AMD.  Instead of
opencoding an OR of two features everywhere, introduce has_spec_ctrl instead.

Reword the comment above the Intel specific alternatives block to highlight
that it is Intel specific, and pull the setting of default_xen_spec_ctrl.IBRS
out because it will want to be common.

No functional change.

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>
---
 xen/arch/x86/spec_ctrl.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 8a550d0a0902..2072daf66245 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -927,7 +927,7 @@ static __init void mds_calculations(uint64_t caps)
 void __init init_speculation_mitigations(void)
 {
     enum ind_thunk thunk = THUNK_DEFAULT;
-    bool ibrs = false, hw_smt_enabled;
+    bool has_spec_ctrl, ibrs = false, hw_smt_enabled;
     bool cpu_has_bug_taa;
     uint64_t caps = 0;
 
@@ -936,6 +936,8 @@ void __init init_speculation_mitigations(void)
 
     hw_smt_enabled = check_smt_enabled();
 
+    has_spec_ctrl = boot_cpu_has(X86_FEATURE_IBRSB);
+
     /*
      * First, disable the use of retpolines if Xen is using shadow stacks, as
      * they are incompatible.
@@ -973,11 +975,11 @@ void __init init_speculation_mitigations(void)
              */
             else if ( retpoline_safe(caps) )
                 thunk = THUNK_RETPOLINE;
-            else if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+            else if ( has_spec_ctrl )
                 ibrs = true;
         }
         /* Without compiler thunk support, use IBRS if available. */
-        else if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+        else if ( has_spec_ctrl )
             ibrs = true;
     }
 
@@ -1008,10 +1010,7 @@ void __init init_speculation_mitigations(void)
     else if ( thunk == THUNK_JMP )
         setup_force_cpu_cap(X86_FEATURE_IND_THUNK_JMP);
 
-    /*
-     * If we are on hardware supporting MSR_SPEC_CTRL, see about setting up
-     * the alternatives blocks so we can virtualise support for guests.
-     */
+    /* Intel hardware: MSR_SPEC_CTRL alternatives setup. */
     if ( boot_cpu_has(X86_FEATURE_IBRSB) )
     {
         if ( opt_msr_sc_pv )
@@ -1030,11 +1029,12 @@ void __init init_speculation_mitigations(void)
             default_spec_ctrl_flags |= SCF_ist_wrmsr;
             setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
         }
-
-        if ( ibrs )
-            default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
     }
 
+    /* If we have IBRS available, see whether we should use it. */
+    if ( has_spec_ctrl && ibrs )
+        default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
+
     /* If we have SSBD available, see whether we should use it. */
     if ( boot_cpu_has(X86_FEATURE_SSBD) && opt_ssbd )
         default_xen_spec_ctrl |= SPEC_CTRL_SSBD;
@@ -1268,7 +1268,7 @@ void __init init_speculation_mitigations(void)
      * boot won't have any other code running in a position to mount an
      * attack.
      */
-    if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+    if ( has_spec_ctrl )
     {
         bsp_delay_spec_ctrl = !cpu_has_hypervisor && default_xen_spec_ctrl;
 
-- 
2.11.0

[PATCH 6/8] x86/spec-ctrl: Use common MSR_SPEC_CTRL logic for AMD

[Andrew Cooper]

Currently, amd_init_ssbd() works by being the only write to MSR_SPEC_CTRL in
the system.  This ceases to be true when using the common logic.

Include AMD MSR_SPEC_CTRL in has_spec_ctrl to activate the common paths, and
introduce an AMD specific block to control alternatives.

For now, only configure alternatives for HVM.  PV will require more work.

This is a reasonably large change for low level defaults in the common case,
but should have no practical change in behaviour.

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>
---
 xen/arch/x86/cpu/amd.c   |  2 +-
 xen/arch/x86/spec_ctrl.c | 26 ++++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index f87484b7ce61..a8e37dbb1f5c 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -693,7 +693,7 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
 		return;
 
 	if (cpu_has_amd_ssbd) {
-		wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		/* Handled by common MSR_SPEC_CTRL logic */
 		return;
 	}
 
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 2072daf66245..5d08ee866869 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -22,6 +22,7 @@
 #include <xen/param.h>
 #include <xen/warning.h>
 
+#include <asm/hvm/svm/svm.h>
 #include <asm/microcode.h>
 #include <asm/msr.h>
 #include <asm/pv/domain.h>
@@ -936,7 +937,8 @@ void __init init_speculation_mitigations(void)
 
     hw_smt_enabled = check_smt_enabled();
 
-    has_spec_ctrl = boot_cpu_has(X86_FEATURE_IBRSB);
+    has_spec_ctrl = (boot_cpu_has(X86_FEATURE_IBRSB) ||
+                     boot_cpu_has(X86_FEATURE_IBRS));
 
     /*
      * First, disable the use of retpolines if Xen is using shadow stacks, as
@@ -1031,12 +1033,32 @@ void __init init_speculation_mitigations(void)
         }
     }
 
+    /* AMD hardware: MSR_SPEC_CTRL alternatives setup. */
+    if ( boot_cpu_has(X86_FEATURE_IBRS) )
+    {
+        /*
+         * Virtualising MSR_SPEC_CTRL for guests depends on SVM support, which
+         * on real hardware matches the availability of MSR_SPEC_CTRL in the
+         * first place.
+         *
+         * No need for SCF_ist_wrmsr because, because Xen's value is restored
+         * atomically WRT NMIs in the VMExit path.
+         *
+         * TODO Adjust cpu_has_svm_spec_ctrl to be configured earlier on boot
+         */
+        if ( opt_msr_sc_hvm &&
+             (boot_cpu_data.extended_cpuid_level >= 0x8000000a) &&
+             (cpuid_edx(0x8000000a) & (1u << SVM_FEATURE_SPEC_CTRL)) )
+            setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
+    }
+
     /* If we have IBRS available, see whether we should use it. */
     if ( has_spec_ctrl && ibrs )
         default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
 
     /* If we have SSBD available, see whether we should use it. */
-    if ( boot_cpu_has(X86_FEATURE_SSBD) && opt_ssbd )
+    if ( opt_ssbd && (boot_cpu_has(X86_FEATURE_SSBD) ||
+                      boot_cpu_has(X86_FEATURE_AMD_SSBD)) )
         default_xen_spec_ctrl |= SPEC_CTRL_SSBD;
 
     /*
-- 
2.11.0

[PATCH 7/8] x86/msr: AMD MSR_SPEC_CTRL infrastructure

[Andrew Cooper]

Fill in VMCB accessors for spec_ctrl in svm_{get,set}_reg(), and CPUID checks
for all supported bits in guest_{rd,wr}msr().

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>
---
 xen/arch/x86/hvm/svm/svm.c | 9 +++++++++
 xen/arch/x86/msr.c         | 8 +++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8fdb530b4004..bc834556c5f7 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2471,10 +2471,14 @@ static bool svm_get_pending_event(struct vcpu *v, struct x86_event *info)
 
 static uint64_t svm_get_reg(struct vcpu *v, unsigned int reg)
 {
+    const struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
     struct domain *d = v->domain;
 
     switch ( reg )
     {
+    case MSR_SPEC_CTRL:
+        return vmcb->spec_ctrl;
+
     default:
         printk(XENLOG_G_ERR "%s(%pv, 0x%08x) Bad register\n",
                __func__, v, reg);
@@ -2485,10 +2489,15 @@ static uint64_t svm_get_reg(struct vcpu *v, unsigned int reg)
 
 static void svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
 {
+    struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
     struct domain *d = v->domain;
 
     switch ( reg )
     {
+    case MSR_SPEC_CTRL:
+        vmcb->spec_ctrl = val;
+        break;
+
     default:
         printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad register\n",
                __func__, v, reg, val);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 5e80c8b47c21..4ac5b5a048eb 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -265,7 +265,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
         break;
 
     case MSR_SPEC_CTRL:
-        if ( !cp->feat.ibrsb )
+        if ( !cp->feat.ibrsb && !cp->extd.ibrs )
             goto gp_fault;
         goto get_reg;
 
@@ -442,7 +442,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
  */
 uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp)
 {
-    bool ssbd = cp->feat.ssbd;
+    bool ssbd = cp->feat.ssbd || cp->extd.amd_ssbd;
+    bool psfd = cp->extd.psfd;
 
     /*
      * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored)
@@ -450,6 +451,7 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp)
      */
     return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
             (ssbd       ? SPEC_CTRL_SSBD       : 0) |
+            (psfd       ? SPEC_CTRL_PSFD       : 0) |
             0);
 }
 
@@ -526,7 +528,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
 
     case MSR_SPEC_CTRL:
-        if ( !cp->feat.ibrsb ||
+        if ( (!cp->feat.ibrsb && !cp->extd.ibrs) ||
              (val & ~msr_spec_ctrl_valid_bits(cp)) )
             goto gp_fault;
         goto set_reg;
-- 
2.11.0

[PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

[Andrew Cooper]

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), 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>

Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather
than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but
it's also a bit misleading to say 'A' when the PV side won't engage at all
yet.
---
 xen/arch/x86/cpuid.c                        | 16 ++++++++++++----
 xen/include/public/arch-x86/cpufeatureset.h | 18 +++++++++---------
 xen/tools/gen-cpuid.py                      |  5 +++++
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index b5af48324aef..64570148c165 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 HVM 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/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 0b399375566f..dfbf25b9acb3 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) /*   Hardware not vulnerable to SSB */
-XEN_CPUFEATURE(PSFD,          8*32+28) /*   MSR_SPEC_CTRL.PSFD */
+XEN_CPUFEATURE(SSB_NO,        8*32+26) /*S  Hardware not vulnerable to SSB */
+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 b953648b6572..e4915b5961aa 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -290,6 +290,11 @@ def crunch_numbers(state):
 
         # In principle the TSXLDTRK insns could also be considered independent.
         RTM: [TSXLDTRK],
+
+        # AMD speculative controls
+        IBRS: [AMD_STIBP, AMD_SSBD, PSFD,
+               IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
+        AMD_STIBP: [STIBP_ALWAYS],
     }
 
     deep_features = tuple(sorted(deps.keys()))
-- 
2.11.0

Re: [PATCH 1/8] x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL

[Roger Pau Monné]

On Wed, Jan 26, 2022 at 08:44:45AM +0000, Andrew Cooper wrote:
> This bug existed in early in 2018 between MSR_SPEC_CTRL arriving in microcode,
> and SSBD arriving a few months later.  It went unnoticed presumably because
> everyone was busy rebooting everything.
> 
> The same bug will reappear when adding PSFD support.
> 
> Clamp the guest MSR_SPEC_CTRL value to that permitted by CPUID on migrate.
> The guest is already playing with reserved bits at this point, and clamping
> the value will prevent a migration to a less capable host from failing.
> 
> 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>
> ---
>  xen/arch/x86/hvm/hvm.c         | 25 +++++++++++++++++++++++--
>  xen/arch/x86/include/asm/msr.h |  2 ++
>  xen/arch/x86/msr.c             | 33 +++++++++++++++++++++------------
>  3 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d7d3299b431e..c4ddb8607d9c 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1340,6 +1340,7 @@ static const uint32_t msrs_to_send[] = {
>  
>  static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
>  {
> +    const struct domain *d = v->domain;
>      struct hvm_save_descriptor *desc = _p(&h->data[h->cur]);
>      struct hvm_msr *ctxt;
>      unsigned int i;
> @@ -1355,7 +1356,8 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
>      for ( i = 0; i < ARRAY_SIZE(msrs_to_send); ++i )
>      {
>          uint64_t val;
> -        int rc = guest_rdmsr(v, msrs_to_send[i], &val);
> +        unsigned int msr = msrs_to_send[i];
> +        int rc = guest_rdmsr(v, msr, &val);
>  
>          /*
>           * It is the programmers responsibility to ensure that
> @@ -1375,7 +1377,26 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
>          if ( !val )
>              continue; /* Skip empty MSRs. */
>  
> -        ctxt->msr[ctxt->count].index = msrs_to_send[i];
> +        /*
> +         * Guests are given full access to certain MSRs for performance
> +         * reasons.  A consequence is that Xen is unable to enforce that all
> +         * bits disallowed by the CPUID policy yield #GP, and an enterprising
> +         * guest may be able to set and use a bit it ought to leave alone.
> +         *
> +         * When migrating from a more capable host to a less capable one, such
> +         * bits may be rejected by the destination, and the migration failed.
> +         *
> +         * Discard such bits here on the source side.  Such bits have reserved
> +         * behaviour, and the guest has only itself to blame.
> +         */
> +        switch ( msr )
> +        {
> +        case MSR_SPEC_CTRL:
> +            val &= msr_spec_ctrl_valid_bits(d->arch.cpuid);
> +            break;
> +        }

Should you move the check for !val here, in case the clearing done
here zeros the MSR?

> +
> +        ctxt->msr[ctxt->count].index = msr;
>          ctxt->msr[ctxt->count++].val = val;
>      }
>  
> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
> index 10039c2d227b..657a3295613d 100644
> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -277,6 +277,8 @@ static inline void wrmsr_tsc_aux(uint32_t val)
>      }
>  }
>  
> +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp);
> +
>  extern struct msr_policy     raw_msr_policy,
>                              host_msr_policy,
>                            pv_max_msr_policy,
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 2cc355575d45..5e80c8b47c21 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -435,6 +435,24 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>      return X86EMUL_EXCEPTION;
>  }
>  
> +/*
> + * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
> + * separate CPUID features for this functionality, but only set will be
> + * active.
> + */
> +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp)
> +{
> +    bool ssbd = cp->feat.ssbd;
> +
> +    /*
> +     * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored)
> +     * when STIBP isn't enumerated in hardware.
> +     */
> +    return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> +            (ssbd       ? SPEC_CTRL_SSBD       : 0) |
> +            0);

The format here looks weird to me, and I don't get why you
unconditionally or a 0 at the end?

I would also be fine with using cp->feat.ssbd directly here.

Thanks, Roger.

Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

[Andrew Cooper]

On 26/01/2022 08:44, 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), and explicitly enable the CPUID bits for HVM guests.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

SVM guests get rather more speedy with this hunk, which I missed:

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index bc834556c5f7..f11622ed4ff8 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);


I've folded it into v2, but won't repost for just this.

~Andrew

Re: [PATCH 2/8] x86/boot: Collect AMD speculative features earlier during boot

[Roger Pau Monné]

On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote:
> All AMD IBRS-related features are in CPUID.0x80000008.ebx.  Collect them in
> early_cpu_init() so init_speculative_mitigations() can use them.
> 
> Rework the existing logic structure to fill in c->extended_cpuid_level and
> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific
> logic block.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

It would be good to update the comment ahead of early_cpu_init to
mention it now also gather speculation-related fields from CPUID in
order to do early setup of mitigations.

I think you could also use boot_cpu_data in spec_ctrl.c print_details
instead of fetching again the cpuid leafs?

Thanks, Roger.

Re: [PATCH 2/8] x86/boot: Collect AMD speculative features earlier during boot

[Jan Beulich]

On 26.01.2022 13:44, Roger Pau Monné wrote:
> I think you could also use boot_cpu_data in spec_ctrl.c print_details
> instead of fetching again the cpuid leafs?

Like with pre-existing code there iirc the intention is to really log
what is available in hardware, no matter what was subsequently disabled
in software already (e.g. via "cpuid=").

Jan

Re: [PATCH 1/8] x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL

[Andrew Cooper]

On 26/01/2022 12:17, Roger Pau Monne wrote:
> On Wed, Jan 26, 2022 at 08:44:45AM +0000, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index d7d3299b431e..c4ddb8607d9c 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1375,7 +1377,26 @@ static int hvm_save_cpu_msrs(struct vcpu *v, hvm_domain_context_t *h)
>>          if ( !val )
>>              continue; /* Skip empty MSRs. */
>>  
>> -        ctxt->msr[ctxt->count].index = msrs_to_send[i];
>> +        /*
>> +         * Guests are given full access to certain MSRs for performance
>> +         * reasons.  A consequence is that Xen is unable to enforce that all
>> +         * bits disallowed by the CPUID policy yield #GP, and an enterprising
>> +         * guest may be able to set and use a bit it ought to leave alone.
>> +         *
>> +         * When migrating from a more capable host to a less capable one, such
>> +         * bits may be rejected by the destination, and the migration failed.
>> +         *
>> +         * Discard such bits here on the source side.  Such bits have reserved
>> +         * behaviour, and the guest has only itself to blame.
>> +         */
>> +        switch ( msr )
>> +        {
>> +        case MSR_SPEC_CTRL:
>> +            val &= msr_spec_ctrl_valid_bits(d->arch.cpuid);
>> +            break;
>> +        }
> Should you move the check for !val here, in case the clearing done
> here zeros the MSR?

Skipping MSRs with a value of 0 is purely an optimisation to avoid
putting a load of empty MSR records in the stream, but nothing will go
wrong if such records are present.

The cost of the extra logic in Xen to spot the 0 corner case is going to
be larger than the data&downtime saving of 16 bytes for an already-buggy VM.

I can't say I care for fixing it...

>> +
>> +        ctxt->msr[ctxt->count].index = msr;
>>          ctxt->msr[ctxt->count++].val = val;
>>      }
>>  
>> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
>> index 10039c2d227b..657a3295613d 100644
>> --- a/xen/arch/x86/include/asm/msr.h
>> +++ b/xen/arch/x86/include/asm/msr.h
>> @@ -277,6 +277,8 @@ static inline void wrmsr_tsc_aux(uint32_t val)
>>      }
>>  }
>>  
>> +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp);
>> +
>>  extern struct msr_policy     raw_msr_policy,
>>                              host_msr_policy,
>>                            pv_max_msr_policy,
>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>> index 2cc355575d45..5e80c8b47c21 100644
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -435,6 +435,24 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>      return X86EMUL_EXCEPTION;
>>  }
>>  
>> +/*
>> + * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
>> + * separate CPUID features for this functionality, but only set will be
>> + * active.
>> + */
>> +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp)
>> +{
>> +    bool ssbd = cp->feat.ssbd;
>> +
>> +    /*
>> +     * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored)
>> +     * when STIBP isn't enumerated in hardware.
>> +     */
>> +    return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>> +            (ssbd       ? SPEC_CTRL_SSBD       : 0) |
>> +            0);
> The format here looks weird to me, and I don't get why you
> unconditionally or a 0 at the end?
>
> I would also be fine with using cp->feat.ssbd directly here.

See patch 7 to cover both points.

The trailing 0 is like trailing commas, and there to reduce the diffstat
of patches adding new lines.

~Andrew

Re: [PATCH 2/8] x86/boot: Collect AMD speculative features earlier during boot

[Andrew Cooper]

On 26/01/2022 12:44, Roger Pau Monné wrote:
> On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote:
>> All AMD IBRS-related features are in CPUID.0x80000008.ebx.  Collect them in
>> early_cpu_init() so init_speculative_mitigations() can use them.
>>
>> Rework the existing logic structure to fill in c->extended_cpuid_level and
>> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific
>> logic block.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> It would be good to update the comment ahead of early_cpu_init to
> mention it now also gather speculation-related fields from CPUID in
> order to do early setup of mitigations.
>
> I think you could also use boot_cpu_data in spec_ctrl.c print_details
> instead of fetching again the cpuid leafs?

Hmm - I may have a mistake here.

Boot time CPUID handling is giant mess, and I haven't had time to finish
my work to make BSP microcode loading dependent on xmalloc(), allowing
it to move far earlier, and removing the early/late CPUID split.

However, init_speculative_mitigations() is called after late CPUID
setup, so e8b should be suitably collected.  Let me try to figure out
what's going on.

For print_details(), I have a feeling that may have been an artefact of
an early version of the logic, and likely can be cleaned up.

~Andrew

Re: [PATCH 2/8] x86/boot: Collect AMD speculative features earlier during boot

[Andrew Cooper]

On 26/01/2022 13:37, Andrew Cooper wrote:
> On 26/01/2022 12:44, Roger Pau Monné wrote:
>> On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote:
>>> All AMD IBRS-related features are in CPUID.0x80000008.ebx.  Collect them in
>>> early_cpu_init() so init_speculative_mitigations() can use them.
>>>
>>> Rework the existing logic structure to fill in c->extended_cpuid_level and
>>> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific
>>> logic block.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> It would be good to update the comment ahead of early_cpu_init to
>> mention it now also gather speculation-related fields from CPUID in
>> order to do early setup of mitigations.
>>
>> I think you could also use boot_cpu_data in spec_ctrl.c print_details
>> instead of fetching again the cpuid leafs?
> Hmm - I may have a mistake here.
>
> Boot time CPUID handling is giant mess, and I haven't had time to finish
> my work to make BSP microcode loading dependent on xmalloc(), allowing

Sorry.  I mean "independent" here.

~Andrew

> it to move far earlier, and removing the early/late CPUID split.
>
> However, init_speculative_mitigations() is called after late CPUID
> setup, so e8b should be suitably collected.  Let me try to figure out
> what's going on.
>
> For print_details(), I have a feeling that may have been an artefact of
> an early version of the logic, and likely can be cleaned up.
>
> ~Andrew

Re: [PATCH 2/8] x86/boot: Collect AMD speculative features earlier during boot

[Andrew Cooper]

On 26/01/2022 13:47, Andrew Cooper wrote:
> On 26/01/2022 13:37, Andrew Cooper wrote:
>> On 26/01/2022 12:44, Roger Pau Monné wrote:
>>> On Wed, Jan 26, 2022 at 08:44:46AM +0000, Andrew Cooper wrote:
>>>> All AMD IBRS-related features are in CPUID.0x80000008.ebx.  Collect them in
>>>> early_cpu_init() so init_speculative_mitigations() can use them.
>>>>
>>>> Rework the existing logic structure to fill in c->extended_cpuid_level and
>>>> separate out the ambiguous use of ebx in an otherwise 0x80000008-specific
>>>> logic block.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> It would be good to update the comment ahead of early_cpu_init to
>>> mention it now also gather speculation-related fields from CPUID in
>>> order to do early setup of mitigations.
>>>
>>> I think you could also use boot_cpu_data in spec_ctrl.c print_details
>>> instead of fetching again the cpuid leafs?
>> Hmm - I may have a mistake here.
>>
>> Boot time CPUID handling is giant mess, and I haven't had time to finish
>> my work to make BSP microcode loading dependent on xmalloc(), allowing
> Sorry.  I mean "independent" here.
>
> ~Andrew
>
>> it to move far earlier, and removing the early/late CPUID split.
>>
>> However, init_speculative_mitigations() is called after late CPUID
>> setup, so e8b should be suitably collected.  Let me try to figure out
>> what's going on.

And testing shows that everything works fine without this patch.  I must
have had some breakage during development which has resolved itself as
part of cleaning the series up.

Anyway, I'll withdraw this patch.

~Andrew

Re: [PATCH 1/8] x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL

[Jan Beulich]

On 26.01.2022 09:44, Andrew Cooper wrote:
> This bug existed in early in 2018 between MSR_SPEC_CTRL arriving in microcode,
> and SSBD arriving a few months later.  It went unnoticed presumably because
> everyone was busy rebooting everything.
> 
> The same bug will reappear when adding PSFD support.
> 
> Clamp the guest MSR_SPEC_CTRL value to that permitted by CPUID on migrate.
> The guest is already playing with reserved bits at this point, and clamping
> the value will prevent a migration to a less capable host from failing.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich]

On 26.01.2022 09:44, Andrew Cooper wrote:
> 1) It would be slightly more efficient to pass curr and cpu_info into
>    vm{entry,exit}_spec_ctrl(), but setup of such state can't be in the
>    ALTERNATIVE block because then the call displacement won't get fixed up.
>    All the additional accesses are hot off the stack, so almost certainly
>    negligible compared to the WRMSR.

What's wrong with using two instances of ALTERNATIVE, one to setup the
call arguments and the 2nd for just the CALL?

> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -55,11 +55,11 @@ __UNLIKELY_END(nsvm_hap)
>          mov  %rsp, %rdi
>          call svm_vmenter_helper
>  
> -        mov VCPU_arch_msrs(%rbx), %rax
> -        mov VCPUMSR_spec_ctrl_raw(%rax), %eax
> +        clgi
>  
>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
> -        /* SPEC_CTRL_EXIT_TO_SVM   (nothing currently) */
> +        /* SPEC_CTRL_EXIT_TO_SVM       Req:                           Clob: C   */
> +        ALTERNATIVE "", __stringify(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM

I guess the new upper case C after Clob: stands for "all call-clobbered
registers"? In which case ...

> @@ -86,8 +85,9 @@ __UNLIKELY_END(nsvm_hap)
>  
>          GET_CURRENT(bx)
>  
> -        /* SPEC_CTRL_ENTRY_FROM_SVM    Req: b=curr %rsp=regs/cpuinfo, Clob: ac  */
> +        /* SPEC_CTRL_ENTRY_FROM_SVM    Req:                           Clob: ac,C */
>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
> +        ALTERNATIVE "", __stringify(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM

... why the explicit further "ac" here? Is the intention to annotate
every individual ALTERNATIVE this way?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -3086,6 +3086,36 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>      vmcb_set_vintr(vmcb, intr);
>  }
>  
> +/* Called with GIF=0. */
> +void vmexit_spec_ctrl(void)
> +{
> +    struct cpu_info *info = get_cpu_info();
> +    unsigned int val = info->xen_spec_ctrl;
> +
> +    /*
> +     * Write to MSR_SPEC_CTRL unconditionally, for the RAS[:32] flushing side
> +     * effect.
> +     */
> +    wrmsr(MSR_SPEC_CTRL, val, 0);
> +    info->last_spec_ctrl = val;
> +}
> +
> +/* Called with GIF=0. */
> +void vmentry_spec_ctrl(void)
> +{
> +    struct cpu_info *info = get_cpu_info();
> +    const struct vcpu *curr = current;
> +    unsigned int val = curr->arch.msrs->spec_ctrl.raw;
> +
> +    if ( val != info->last_spec_ctrl )
> +    {
> +        wrmsr(MSR_SPEC_CTRL, val, 0);
> +        info->last_spec_ctrl = val;
> +    }

Is this correct for the very first use on a CPU? last_spec_ctrl
starts out as zero afaict, and hence this very first write would be
skipped if the guest value is also zero (which it will be for a
vCPU first launched), even if we have a non-zero value in the MSR
at that point.

Jan

Re: [PATCH 4/8] x86/spec-ctrl: Drop use_spec_ctrl boolean

[Jan Beulich]

On 26.01.2022 09:44, Andrew Cooper wrote:
> Several bugfixes have reduced the utility of this variable from it's original
> purpose, and now all it does is aid in the setup of SCF_ist_wrmsr.
> 
> Simplify the logic by drop the variable, and doubling up the setting of
> SCF_ist_wrmsr for the PV and HVM blocks, which will make the AMD SPEC_CTRL
> support easier to follow.  Leave a comment explaining why SCF_ist_wrmsr is
> still necessary for the VMExit case.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 5/8] x86/spec-ctrl: Introduce new has_spec_ctrl boolean

[Jan Beulich]

On 26.01.2022 09:44, Andrew Cooper wrote:
> Most MSR_SPEC_CTRL setup will be common between Intel and AMD.  Instead of
> opencoding an OR of two features everywhere, introduce has_spec_ctrl instead.
> 
> Reword the comment above the Intel specific alternatives block to highlight
> that it is Intel specific, and pull the setting of default_xen_spec_ctrl.IBRS
> out because it will want to be common.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 6/8] x86/spec-ctrl: Use common MSR_SPEC_CTRL logic for AMD

[Jan Beulich]

On 26.01.2022 09:44, Andrew Cooper wrote:
> Currently, amd_init_ssbd() works by being the only write to MSR_SPEC_CTRL in
> the system.  This ceases to be true when using the common logic.
> 
> Include AMD MSR_SPEC_CTRL in has_spec_ctrl to activate the common paths, and
> introduce an AMD specific block to control alternatives.
> 
> For now, only configure alternatives for HVM.  PV will require more work.
> 
> This is a reasonably large change for low level defaults in the common case,
> but should have no practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

One nit:

> --- a/xen/arch/x86/cpu/amd.c
> @@ -1031,12 +1033,32 @@ void __init init_speculation_mitigations(void)
>          }
>      }
>  
> +    /* AMD hardware: MSR_SPEC_CTRL alternatives setup. */
> +    if ( boot_cpu_has(X86_FEATURE_IBRS) )
> +    {
> +        /*
> +         * Virtualising MSR_SPEC_CTRL for guests depends on SVM support, which
> +         * on real hardware matches the availability of MSR_SPEC_CTRL in the
> +         * first place.
> +         *
> +         * No need for SCF_ist_wrmsr because, because Xen's value is restored
> +         * atomically WRT NMIs in the VMExit path.

There's one "because" too many here.

Jan

Re: [PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

On 26/01/2022 16:50, Jan Beulich wrote:
> On 26.01.2022 09:44, Andrew Cooper wrote:
>> 1) It would be slightly more efficient to pass curr and cpu_info into
>>    vm{entry,exit}_spec_ctrl(), but setup of such state can't be in the
>>    ALTERNATIVE block because then the call displacement won't get fixed up.
>>    All the additional accesses are hot off the stack, so almost certainly
>>    negligible compared to the WRMSR.
> What's wrong with using two instances of ALTERNATIVE, one to setup the
> call arguments and the 2nd for just the CALL?

Hmm

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index c718328ac4cf..1d4be7e97ae2 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -59,6 +59,7 @@ __UNLIKELY_END(nsvm_hap)
 
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
         /* SPEC_CTRL_EXIT_TO_SVM       Req:                          
Clob: C   */
+        ALTERNATIVE "", __stringify(mov %rbx, %rdi; mov %rsp, %rsi),
X86_FEATURE_SC_MSR_HVM
         ALTERNATIVE "", __stringify(call vmentry_spec_ctrl),
X86_FEATURE_SC_MSR_HVM
 
         pop  %r15

is somewhat of a long line, but isn't too terrible.

I'm tempted to switch back to using STR() seeing as we have both and it
is much more concise.
>> --- a/xen/arch/x86/hvm/svm/entry.S
>> +++ b/xen/arch/x86/hvm/svm/entry.S
>> @@ -55,11 +55,11 @@ __UNLIKELY_END(nsvm_hap)
>>          mov  %rsp, %rdi
>>          call svm_vmenter_helper
>>  
>> -        mov VCPU_arch_msrs(%rbx), %rax
>> -        mov VCPUMSR_spec_ctrl_raw(%rax), %eax
>> +        clgi
>>  
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
>> -        /* SPEC_CTRL_EXIT_TO_SVM   (nothing currently) */
>> +        /* SPEC_CTRL_EXIT_TO_SVM       Req:                           Clob: C   */
>> +        ALTERNATIVE "", __stringify(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM
> I guess the new upper case C after Clob: stands for "all call-clobbered
> registers"?

That was the intention, yes.

>  In which case ...
>
>> @@ -86,8 +85,9 @@ __UNLIKELY_END(nsvm_hap)
>>  
>>          GET_CURRENT(bx)
>>  
>> -        /* SPEC_CTRL_ENTRY_FROM_SVM    Req: b=curr %rsp=regs/cpuinfo, Clob: ac  */
>> +        /* SPEC_CTRL_ENTRY_FROM_SVM    Req:                           Clob: ac,C */
>>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
>> +        ALTERNATIVE "", __stringify(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM
> ... why the explicit further "ac" here? Is the intention to annotate
> every individual ALTERNATIVE this way?

Fair point.  I'll switch to just C.

The clobbers are rather more important for the PV side where the logic
has multiple live variables and it's not totally obvious that all GPRs
are available.

>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -3086,6 +3086,36 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>      vmcb_set_vintr(vmcb, intr);
>>  }
>>  
>> +/* Called with GIF=0. */
>> +void vmexit_spec_ctrl(void)
>> +{
>> +    struct cpu_info *info = get_cpu_info();
>> +    unsigned int val = info->xen_spec_ctrl;
>> +
>> +    /*
>> +     * Write to MSR_SPEC_CTRL unconditionally, for the RAS[:32] flushing side
>> +     * effect.
>> +     */
>> +    wrmsr(MSR_SPEC_CTRL, val, 0);
>> +    info->last_spec_ctrl = val;
>> +}
>> +
>> +/* Called with GIF=0. */
>> +void vmentry_spec_ctrl(void)
>> +{
>> +    struct cpu_info *info = get_cpu_info();
>> +    const struct vcpu *curr = current;
>> +    unsigned int val = curr->arch.msrs->spec_ctrl.raw;
>> +
>> +    if ( val != info->last_spec_ctrl )
>> +    {
>> +        wrmsr(MSR_SPEC_CTRL, val, 0);
>> +        info->last_spec_ctrl = val;
>> +    }
> Is this correct for the very first use on a CPU? last_spec_ctrl
> starts out as zero afaict, and hence this very first write would be
> skipped if the guest value is also zero (which it will be for a
> vCPU first launched), even if we have a non-zero value in the MSR
> at that point.

Ish.

We intentionally write MSR_SPEC_CTRL once on each CPU to clear out any
previous-environment settings, but those boot paths need to latch
last_spec_ctrl too for this to work correctly.

Making this safe is slightly nasty.  I think the best option would be to
reorder this patch to be after the patch 6, and tweak the wording in
patch 6's commit message.  That way, we're not adding latching to
later-dropped codepaths.

~Andrew

Re: [PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich]

On 26.01.2022 21:16, Andrew Cooper wrote:
> On 26/01/2022 16:50, Jan Beulich wrote:
>> On 26.01.2022 09:44, Andrew Cooper wrote:
>>> 1) It would be slightly more efficient to pass curr and cpu_info into
>>>    vm{entry,exit}_spec_ctrl(), but setup of such state can't be in the
>>>    ALTERNATIVE block because then the call displacement won't get fixed up.
>>>    All the additional accesses are hot off the stack, so almost certainly
>>>    negligible compared to the WRMSR.
>> What's wrong with using two instances of ALTERNATIVE, one to setup the
>> call arguments and the 2nd for just the CALL?
> 
> Hmm
> 
> diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
> index c718328ac4cf..1d4be7e97ae2 100644
> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -59,6 +59,7 @@ __UNLIKELY_END(nsvm_hap)
>  
>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
>          /* SPEC_CTRL_EXIT_TO_SVM       Req:                          
> Clob: C   */
> +        ALTERNATIVE "", __stringify(mov %rbx, %rdi; mov %rsp, %rsi),
> X86_FEATURE_SC_MSR_HVM
>          ALTERNATIVE "", __stringify(call vmentry_spec_ctrl),
> X86_FEATURE_SC_MSR_HVM
>  
>          pop  %r15
> 
> is somewhat of a long line, but isn't too terrible.
> 
> I'm tempted to switch back to using STR() seeing as we have both and it
> is much more concise.

No objections. In fact I think when I introduced stringify.h over 10 years
back, I didn't realize we already had STR(). Quite certainly first and
foremost because of its bogus placement in xen/config.h (same would go for
at least IS_ALIGNED() as well as KB() and friends).

I wouldn't even mind phasing out stringify.h again. Or maybe we want to
move STR() there ...

Jan

Re: [PATCH 7/8] x86/msr: AMD MSR_SPEC_CTRL infrastructure

[Jan Beulich]

On 26.01.2022 09:44, Andrew Cooper wrote:
> Fill in VMCB accessors for spec_ctrl in svm_{get,set}_reg(), and CPUID checks
> for all supported bits in guest_{rd,wr}msr().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

[Jan Beulich]

On 26.01.2022 09:44, 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), 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>
> 
> Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather
> than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but
> it's also a bit misleading to say 'A' when the PV side won't engage at all
> yet.

I agree with using 'S' at this point for most of them. I'm unsure for
SSB_NO, though - there 'A' would seem more appropriate, and afaict it
would then also be visible to PV guests (since you validly don't make
it dependent upon IBRS or anything else). Aiui this could have been
done even ahead of this work of yours.

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

Following this comment is a cross-vendor adjustment. Shouldn't there
be more of these now? Or has this been a one-off for some reason?

> @@ -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 HVM guests (functional
> +     * availability, or admin choice), hide the feature.
> +     */

Unintended replacement of "PV" by "HVM"?

>      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);

What I had done in a local (and incomplete) patch is pass
X86_FEATURE_SC_MSR_{PV,HVM} into guest_common_feature_adjustments()
and do what you extend above (and then again for HVM) centrally
there. (My gen-cpuid.py adjustment was smaller, so there were even
more bits to clear, and hence it became yet more relevant to avoid
doing this in two places.)

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -290,6 +290,11 @@ def crunch_numbers(state):
>  
>          # In principle the TSXLDTRK insns could also be considered independent.
>          RTM: [TSXLDTRK],
> +
> +        # AMD speculative controls
> +        IBRS: [AMD_STIBP, AMD_SSBD, PSFD,
> +               IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
> +        AMD_STIBP: [STIBP_ALWAYS],
>      }

Could I talk you into moving this ahead of RTM, such that it sits
next to the related Intel stuff?

Jan

Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

[Andrew Cooper]

On 27/01/2022 13:47, Jan Beulich wrote:
> On 26.01.2022 09:44, 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), 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>
>>
>> Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather
>> than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but
>> it's also a bit misleading to say 'A' when the PV side won't engage at all
>> yet.
> I agree with using 'S' at this point for most of them. I'm unsure for
> SSB_NO, though - there 'A' would seem more appropriate, and afaict it
> would then also be visible to PV guests (since you validly don't make
> it dependent upon IBRS or anything else). Aiui this could have been
> done even ahead of this work of yours.

Hmm.  There aren't any AMD CPUs I'm aware of which enumerate SSB_NO, but
it probably ought to be 'A'.  I'll pull this out into a separate change.

>> --- 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
> Following this comment is a cross-vendor adjustment. Shouldn't there
> be more of these now? Or has this been a one-off for some reason?

MSR_PRED_CMD is easy to cross-vendor (just expose the CPUID bit and
MSR), but IIRC the intention here was to be able to configure an Intel
VM with IBPB only and no IBRS.

This was at the same time as the buggy MSR_SPEC_CTRL microcode was out
in the wild and a `1: wrmsr; jmp 1b` loop would reliably take the system
down.


For MSR_SPEC_CTRL, there are three substantially different behaviours. 
This is part of why this series has taken so long to get organised, and
why there's no PV guest support yet.

Attempting to cross-vendor anything related to MSR_SPEC_CTRL will be a
disaster.  Even if you can make the VM not crash (ought to be doable),
it's going to have a very broken idea of its Spectre-v2 safety.

>> @@ -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 HVM guests (functional
>> +     * availability, or admin choice), hide the feature.
>> +     */
> Unintended replacement of "PV" by "HVM"?

Yes.  Too much copy&paste.

~Andrew

Re: [PATCH 3/8] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

On 27/01/2022 07:25, Jan Beulich wrote:
> On 26.01.2022 21:16, Andrew Cooper wrote:
>> On 26/01/2022 16:50, Jan Beulich wrote:
>>> On 26.01.2022 09:44, Andrew Cooper wrote:
>>>> 1) It would be slightly more efficient to pass curr and cpu_info into
>>>>    vm{entry,exit}_spec_ctrl(), but setup of such state can't be in the
>>>>    ALTERNATIVE block because then the call displacement won't get fixed up.
>>>>    All the additional accesses are hot off the stack, so almost certainly
>>>>    negligible compared to the WRMSR.
>>> What's wrong with using two instances of ALTERNATIVE, one to setup the
>>> call arguments and the 2nd for just the CALL?
>> Hmm
>>
>> diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
>> index c718328ac4cf..1d4be7e97ae2 100644
>> --- a/xen/arch/x86/hvm/svm/entry.S
>> +++ b/xen/arch/x86/hvm/svm/entry.S
>> @@ -59,6 +59,7 @@ __UNLIKELY_END(nsvm_hap)
>>  
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
>>          /* SPEC_CTRL_EXIT_TO_SVM       Req:                          
>> Clob: C   */
>> +        ALTERNATIVE "", __stringify(mov %rbx, %rdi; mov %rsp, %rsi),
>> X86_FEATURE_SC_MSR_HVM
>>          ALTERNATIVE "", __stringify(call vmentry_spec_ctrl),
>> X86_FEATURE_SC_MSR_HVM
>>  
>>          pop  %r15
>>
>> is somewhat of a long line, but isn't too terrible.
>>
>> I'm tempted to switch back to using STR() seeing as we have both and it
>> is much more concise.
> No objections. In fact I think when I introduced stringify.h over 10 years
> back, I didn't realize we already had STR(). Quite certainly first and
> foremost because of its bogus placement in xen/config.h (same would go for
> at least IS_ALIGNED() as well as KB() and friends).
>
> I wouldn't even mind phasing out stringify.h again. Or maybe we want to
> move STR() there ...

Now that we've given up using freestanding headers anywhere, there's a
xen/stddef.h shaped hole.

Things like NULL should move across, but it would also be the
appropriate place for ARRAY_SIZE() and pretty much everything else we
expect to be ubiquitous.  Linux includes things like offsetof().

~Andrew