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

[PATCH v2 0/9] 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 (9):
  x86/cpuid: Advertise SSB_NO to guests by default
  x86/spec-ctrl: Drop use_spec_ctrl boolean
  x86/spec-ctrl: Introduce new has_spec_ctrl boolean
  x86/spec-ctrl: Don't use spec_ctrl_{enter,exit}_idle() for S3
  x86/spec-ctrl: Record the last write to MSR_SPEC_CTRL
  x86/spec-ctrl: Use common MSR_SPEC_CTRL logic for AMD
  x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL
  x86/msr: AMD MSR_SPEC_CTRL infrastructure
  x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default

 xen/arch/x86/acpi/power.c                   |  8 +++-
 xen/arch/x86/cpu/amd.c                      |  2 +-
 xen/arch/x86/cpuid.c                        | 16 +++++--
 xen/arch/x86/hvm/svm/entry.S                | 12 +++---
 xen/arch/x86/hvm/svm/svm.c                  | 40 +++++++++++++++++
 xen/arch/x86/include/asm/current.h          |  2 +-
 xen/arch/x86/include/asm/hvm/svm/svm.h      |  3 ++
 xen/arch/x86/include/asm/msr.h              |  9 ++++
 xen/arch/x86/include/asm/spec_ctrl_asm.h    |  7 +++
 xen/arch/x86/msr.c                          |  8 ++--
 xen/arch/x86/setup.c                        |  5 ++-
 xen/arch/x86/smpboot.c                      |  7 ++-
 xen/arch/x86/spec_ctrl.c                    | 66 ++++++++++++++++++++---------
 xen/include/public/arch-x86/cpufeatureset.h | 18 ++++----
 xen/tools/gen-cpuid.py                      | 14 +++---
 15 files changed, 166 insertions(+), 51 deletions(-)

-- 
2.11.0

[PATCH v2 1/9] x86/cpuid: Advertise SSB_NO to guests by default

[Andrew Cooper]

This is a statement of hardware behaviour, and not related to controls for the
guest kernel to use.  Pass it straight through from hardware.

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>

Not currently enumerated by any CPU I'm aware of.

v2:
 * New
---
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 6e44148a0901..fd8ab2572304 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -266,7 +266,7 @@ 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(VIRT_SSBD,     8*32+25) /*   MSR_VIRT_SPEC_CTRL.SSBD */
-XEN_CPUFEATURE(SSB_NO,        8*32+26) /*   Hardware not vulnerable to SSB */
+XEN_CPUFEATURE(SSB_NO,        8*32+26) /*A  Hardware not vulnerable to SSB */
 XEN_CPUFEATURE(PSFD,          8*32+28) /*   MSR_SPEC_CTRL.PSFD */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0.edx, word 9 */
-- 
2.11.0

Re: [PATCH v2 1/9] x86/cpuid: Advertise SSB_NO to guests by default

[Jan Beulich]

On 28.01.2022 14:29, Andrew Cooper wrote:
> This is a statement of hardware behaviour, and not related to controls for the
> guest kernel to use.  Pass it straight through from hardware.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 1/9] x86/cpuid: Advertise SSB_NO to guests by default

[Roger Pau Monné]

On Fri, Jan 28, 2022 at 01:29:19PM +0000, Andrew Cooper wrote:
> This is a statement of hardware behaviour, and not related to controls for the
> guest kernel to use.  Pass it straight through from hardware.
> 

Not really related to this patch per se, but I think we should expose
AMD_SSBD unconditionally for SPEC_CTRL (and VIRT_SSBD for
VIRT_SPEC_CTRL when supported) in the max policies and implement them
as noop for compatibility reasons?

I would expect CPUs exposing SSB_NO to drop AMD_SSBD and VIRT_SSBD at
some point.

Thanks, Roger.

Re: [PATCH v2 1/9] x86/cpuid: Advertise SSB_NO to guests by default

[Andrew Cooper]

On 31/01/2022 09:41, Roger Pau Monné wrote:
> On Fri, Jan 28, 2022 at 01:29:19PM +0000, Andrew Cooper wrote:
>> This is a statement of hardware behaviour, and not related to controls for the
>> guest kernel to use.  Pass it straight through from hardware.
>>
> Not really related to this patch per se, but I think we should expose
> AMD_SSBD unconditionally for SPEC_CTRL (and VIRT_SSBD for
> VIRT_SPEC_CTRL when supported) in the max policies and implement them
> as noop for compatibility reasons?
>
> I would expect CPUs exposing SSB_NO to drop AMD_SSBD and VIRT_SSBD at
> some point.

Why?  SSBD is an architectural feature.  It's far more likely to turn
into a no-op on SSB_NO hardware, than to disappear, especially as the
MSR is exposed to guests.

VIRT_SSBD is only offered by hypervisors, and should only be offered
when there are members in the migration pool without MSR_SPEC_CTRL.

~Andrew

Re: [PATCH v2 1/9] x86/cpuid: Advertise SSB_NO to guests by default

[Roger Pau Monné]

On Mon, Jan 31, 2022 at 11:15:09AM +0000, Andrew Cooper wrote:
> On 31/01/2022 09:41, Roger Pau Monné wrote:
> > On Fri, Jan 28, 2022 at 01:29:19PM +0000, Andrew Cooper wrote:
> >> This is a statement of hardware behaviour, and not related to controls for the
> >> guest kernel to use.  Pass it straight through from hardware.
> >>
> > Not really related to this patch per se, but I think we should expose
> > AMD_SSBD unconditionally for SPEC_CTRL (and VIRT_SSBD for
> > VIRT_SPEC_CTRL when supported) in the max policies and implement them
> > as noop for compatibility reasons?
> >
> > I would expect CPUs exposing SSB_NO to drop AMD_SSBD and VIRT_SSBD at
> > some point.
> 
> Why?  SSBD is an architectural feature.  It's far more likely to turn
> into a no-op on SSB_NO hardware, than to disappear, especially as the
> MSR is exposed to guests.
> 
> VIRT_SSBD is only offered by hypervisors, and should only be offered
> when there are members in the migration pool without MSR_SPEC_CTRL.

But we should also offer VIRT_SSBD in the max policy if the hardware
reports SSB_NO, because then it's a no-op and would allow for
migration from boxes where we do offer VIRT_SSBD.

Thanks, Roger.

[PATCH v2 2/9] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.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 v2 3/9] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.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 v2 4/9] x86/spec-ctrl: Don't use spec_ctrl_{enter,exit}_idle() for S3

[Andrew Cooper]

'idle' here refers to hlt/mwait.  The S3 path isn't an idle path - it is a
platform reset.

Conditionally clearing IBRS and flushing the store buffers on the way down is
a waste of time.

Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way
back up.  Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or
the next return-to-guest, but that's fragile 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>

v2:
 * New
---
 xen/arch/x86/acpi/power.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 31a56f02d083..ea2bd8bbfe93 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -248,7 +248,6 @@ static int enter_state(u32 state)
         error = 0;
 
     ci = get_cpu_info();
-    spec_ctrl_enter_idle(ci);
     /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */
     ci->spec_ctrl_flags &= ~SCF_ist_wrmsr;
 
@@ -295,7 +294,9 @@ static int enter_state(u32 state)
 
     /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */
     ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
-    spec_ctrl_exit_idle(ci);
+
+    if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+        wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);
 
     if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) )
         wrmsrl(MSR_MCU_OPT_CTRL, default_xen_mcu_opt_ctrl);
-- 
2.11.0

Re: [PATCH v2 4/9] x86/spec-ctrl: Don't use spec_ctrl_{enter,exit}_idle() for S3

[Andrew Cooper]

On 28/01/2022 13:29, Andrew Cooper wrote:
> 'idle' here refers to hlt/mwait.  The S3 path isn't an idle path - it is a
> platform reset.
>
> Conditionally clearing IBRS and flushing the store buffers on the way down is
> a waste of time.
>
> Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way
> back up.  Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or
> the next return-to-guest, but that's fragile 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>
>
> v2:
>  * New
> ---
>  xen/arch/x86/acpi/power.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 31a56f02d083..ea2bd8bbfe93 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -248,7 +248,6 @@ static int enter_state(u32 state)
>          error = 0;
>  
>      ci = get_cpu_info();
> -    spec_ctrl_enter_idle(ci);
>      /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */
>      ci->spec_ctrl_flags &= ~SCF_ist_wrmsr;
>  
> @@ -295,7 +294,9 @@ static int enter_state(u32 state)
>  
>      /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */
>      ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
> -    spec_ctrl_exit_idle(ci);
> +
> +    if ( boot_cpu_has(X86_FEATURE_IBRSB) )
> +        wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);

This logic works rather better when it gets the right variable. 
default_xen_spec_ctrl.

~Andrew

>  
>      if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) )
>          wrmsrl(MSR_MCU_OPT_CTRL, default_xen_mcu_opt_ctrl);

Re: [PATCH v2 4/9] x86/spec-ctrl: Don't use spec_ctrl_{enter,exit}_idle() for S3

[Jan Beulich]

On 28.01.2022 14:29, Andrew Cooper wrote:
> 'idle' here refers to hlt/mwait.  The S3 path isn't an idle path - it is a
> platform reset.
> 
> Conditionally clearing IBRS and flushing the store buffers on the way down is
> a waste of time.

I can buy this for the flushing aspect; I'm less certain about the clearing
of IBRS: Whether the act of clearing is slower than the performance price
of running with it enabled is unknown, I suppose?

> Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way
> back up.  Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or
> the next return-to-guest, but that's fragile behaviour.

I'll assume from your reply that you've adjusted the description as well.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Preferably with the statement above softened a little:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 4/9] x86/spec-ctrl: Don't use spec_ctrl_{enter,exit}_idle() for S3

[Andrew Cooper]

On 31/01/2022 10:15, Jan Beulich wrote:
> On 28.01.2022 14:29, Andrew Cooper wrote:
>> 'idle' here refers to hlt/mwait.  The S3 path isn't an idle path - it is a
>> platform reset.
>>
>> Conditionally clearing IBRS and flushing the store buffers on the way down is
>> a waste of time.
> I can buy this for the flushing aspect; I'm less certain about the clearing
> of IBRS: Whether the act of clearing is slower than the performance price
> of running with it enabled is unknown, I suppose?

There are a handful of instructions from now until the core is powered
down.  The cost of the WRMSR is going to dominate everything else, but
we're still only talking microseconds.

But honestly, the perf aspect isn't relevant.  It's wrong to use the
enter/exit idle helpers here.

>> Furthermore, we want to load default_xen_mcu_opt_ctrl unilaterally on the way
>> back up.  Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or
>> the next return-to-guest, but that's fragile behaviour.
> I'll assume from your reply that you've adjusted the description as well.

I have, yes.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Preferably with the statement above softened a little:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

Re: [PATCH v2 4/9] x86/spec-ctrl: Don't use spec_ctrl_{enter,exit}_idle() for S3

[Andrew Cooper]

On 31/01/2022 11:23, Andrew Cooper wrote:
> On 31/01/2022 10:15, Jan Beulich wrote:
>> On 28.01.2022 14:29, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Preferably with the statement above softened a little:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Thanks.

Commit message now reads:

---8<---
'idle' here refers to hlt/mwait.  The S3 path isn't an idle path - it is a
platform reset.

We need to load default_xen_spec_ctrl unilaterally on the way back up.
Currently it happens as a side effect of X86_FEATURE_SC_MSR_IDLE or the next
return-to-guest, but that's fragile behaviour.

Conversely, there is no need to clear IBRS and flush the store buffers
on the
way down; we're microseconds away from cutting power.
---8<---

~Andrew

[PATCH v2 5/9] x86/spec-ctrl: Record the last write to MSR_SPEC_CTRL

[Andrew Cooper]

In some cases, writes to MSR_SPEC_CTRL do not have interesting side effects,
and we should implement lazy context switching like we do with other MSRs.

In the short term, this will be used by the SVM infrastructure, but I expect
to extend it to other contexts in due course.

Introduce cpu_info.last_spec_ctrl for the purpose, and cache writes made from
the boot/resume paths.  The value can't live in regular per-cpu data when it
is eventually used for PV guests when XPTI might be active.

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:
 * New
---
 xen/arch/x86/acpi/power.c                |  3 +++
 xen/arch/x86/include/asm/current.h       |  2 +-
 xen/arch/x86/include/asm/spec_ctrl_asm.h |  4 ++++
 xen/arch/x86/setup.c                     |  5 ++++-
 xen/arch/x86/smpboot.c                   |  5 +++++
 xen/arch/x86/spec_ctrl.c                 | 10 +++++++---
 6 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index ea2bd8bbfe93..5f2ec74f744a 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -296,7 +296,10 @@ static int enter_state(u32 state)
     ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
 
     if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+    {
         wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);
+        ci->last_spec_ctrl = default_xen_mcu_opt_ctrl;
+    }
 
     if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) )
         wrmsrl(MSR_MCU_OPT_CTRL, default_xen_mcu_opt_ctrl);
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/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index bf82528a12ae..9c0c7622c41f 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -67,6 +67,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
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index e716005145d3..115f8f651734 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1930,9 +1930,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     if ( bsp_delay_spec_ctrl )
     {
-        get_cpu_info()->spec_ctrl_flags &= ~SCF_use_shadow;
+        struct cpu_info *info = get_cpu_info();
+
+        info->spec_ctrl_flags &= ~SCF_use_shadow;
         barrier();
         wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
+        info->last_spec_ctrl = default_xen_spec_ctrl;
     }
 
     /* Jump to the 1:1 virtual mappings of cpu0_stack. */
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 08c0f2d9df04..1cfdf96207d4 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -323,6 +323,8 @@ static void set_cpu_sibling_map(unsigned int cpu)
 
 void start_secondary(void *unused)
 {
+    struct cpu_info *info = get_cpu_info();
+
     /*
      * Dont put anything before smp_callin(), SMP booting is so fragile that we
      * want to limit the things done here to the most necessary things.
@@ -379,7 +381,10 @@ void start_secondary(void *unused)
      * microcode.
      */
     if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+    {
         wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
+        info->last_spec_ctrl = default_xen_spec_ctrl;
+    }
     if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) )
         wrmsrl(MSR_MCU_OPT_CTRL, default_xen_mcu_opt_ctrl);
 
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 2072daf66245..b2fd86ebe587 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -1270,6 +1270,9 @@ void __init init_speculation_mitigations(void)
      */
     if ( has_spec_ctrl )
     {
+        struct cpu_info *info = get_cpu_info();
+        unsigned int val;
+
         bsp_delay_spec_ctrl = !cpu_has_hypervisor && default_xen_spec_ctrl;
 
         /*
@@ -1278,15 +1281,16 @@ void __init init_speculation_mitigations(void)
          */
         if ( bsp_delay_spec_ctrl )
         {
-            struct cpu_info *info = get_cpu_info();
-
             info->shadow_spec_ctrl = 0;
             barrier();
             info->spec_ctrl_flags |= SCF_use_shadow;
             barrier();
         }
 
-        wrmsrl(MSR_SPEC_CTRL, bsp_delay_spec_ctrl ? 0 : default_xen_spec_ctrl);
+        val = bsp_delay_spec_ctrl ? 0 : default_xen_spec_ctrl;
+
+        wrmsrl(MSR_SPEC_CTRL, val);
+        info->last_spec_ctrl = val;
     }
 
     if ( boot_cpu_has(X86_FEATURE_SRBDS_CTRL) )
-- 
2.11.0

Re: [PATCH v2 5/9] x86/spec-ctrl: Record the last write to MSR_SPEC_CTRL

[Jan Beulich]

On 28.01.2022 14:29, Andrew Cooper wrote:
> In some cases, writes to MSR_SPEC_CTRL do not have interesting side effects,
> and we should implement lazy context switching like we do with other MSRs.
> 
> In the short term, this will be used by the SVM infrastructure, but I expect
> to extend it to other contexts in due course.
> 
> Introduce cpu_info.last_spec_ctrl for the purpose, and cache writes made from
> the boot/resume paths.  The value can't live in regular per-cpu data when it
> is eventually used for PV guests when XPTI might be active.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -296,7 +296,10 @@ static int enter_state(u32 state)
>      ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
>  
>      if ( boot_cpu_has(X86_FEATURE_IBRSB) )
> +    {
>          wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);
> +        ci->last_spec_ctrl = default_xen_mcu_opt_ctrl;
> +    }

... wouldn't we better have a write_spec_ctrl() helper doing both,
perhaps with the check-if-same logic added as well (overridable by
a "force" boolean input, unless the cases where the write cannot be
skipped can be entirely determined from previous and new values)?

Jan

Re: [PATCH v2 5/9] x86/spec-ctrl: Record the last write to MSR_SPEC_CTRL

[Andrew Cooper]

On 31/01/2022 10:20, Jan Beulich wrote:
> On 28.01.2022 14:29, Andrew Cooper wrote:
>> In some cases, writes to MSR_SPEC_CTRL do not have interesting side effects,
>> and we should implement lazy context switching like we do with other MSRs.
>>
>> In the short term, this will be used by the SVM infrastructure, but I expect
>> to extend it to other contexts in due course.
>>
>> Introduce cpu_info.last_spec_ctrl for the purpose, and cache writes made from
>> the boot/resume paths.  The value can't live in regular per-cpu data when it
>> is eventually used for PV guests when XPTI might be active.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Technically
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> But ...
>
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -296,7 +296,10 @@ static int enter_state(u32 state)
>>      ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
>>  
>>      if ( boot_cpu_has(X86_FEATURE_IBRSB) )
>> +    {
>>          wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);
>> +        ci->last_spec_ctrl = default_xen_mcu_opt_ctrl;
>> +    }
> ... wouldn't we better have a write_spec_ctrl() helper doing both,
> perhaps with the check-if-same logic added as well (overridable by
> a "force" boolean input, unless the cases where the write cannot be
> skipped can be entirely determined from previous and new values)?

I considered that, but decided against it.  The PV and IST caching logic
is likely to be in asm, meaning that the single piece of C check-if-same
logic is in vmentry_spec_ctrl().

i.e. I think this is the right way forward.  By the end of the PV work,
if there is obvious tidy-up to do, I will do so.

~Andrew

[PATCH v2 6/9] 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.  Also update the
boot/resume paths to configure default_xen_spec_ctrl.

svm.h needs an adjustment to remove a dependency on include order.

For now, only active alternatives for HVM - PV will require more work.  No
functional change, as no alternatives are defined yet for HVM yet.

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:
 * Fix build in some PV Shim configurations
 * Adjust boot/resume paths too
 * Adjust commit message after rearranging in the series
 * Fix typo in comment
---
 xen/arch/x86/acpi/power.c              |  2 +-
 xen/arch/x86/cpu/amd.c                 |  2 +-
 xen/arch/x86/include/asm/hvm/svm/svm.h |  3 +++
 xen/arch/x86/smpboot.c                 |  2 +-
 xen/arch/x86/spec_ctrl.c               | 26 ++++++++++++++++++++++++--
 5 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 5f2ec74f744a..0eae29b5687a 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -295,7 +295,7 @@ static int enter_state(u32 state)
     /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */
     ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
 
-    if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+    if ( boot_cpu_has(X86_FEATURE_IBRSB) || boot_cpu_has(X86_FEATURE_IBRS) )
     {
         wrmsrl(MSR_SPEC_CTRL, default_xen_mcu_opt_ctrl);
         ci->last_spec_ctrl = default_xen_mcu_opt_ctrl;
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/include/asm/hvm/svm/svm.h b/xen/arch/x86/include/asm/hvm/svm/svm.h
index 05e968502694..09c32044ec8a 100644
--- a/xen/arch/x86/include/asm/hvm/svm/svm.h
+++ b/xen/arch/x86/include/asm/hvm/svm/svm.h
@@ -45,6 +45,9 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
         "a" (linear), "c" (asid));
 }
 
+struct cpu_user_regs;
+struct vcpu;
+
 unsigned long *svm_msrbit(unsigned long *msr_bitmap, uint32_t msr);
 void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len);
 void svm_update_guest_cr(struct vcpu *, unsigned int cr, unsigned int flags);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 1cfdf96207d4..22ae4c1b2de9 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -380,7 +380,7 @@ void start_secondary(void *unused)
      * settings.  Note: These MSRs may only become available after loading
      * microcode.
      */
-    if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+    if ( boot_cpu_has(X86_FEATURE_IBRSB) || boot_cpu_has(X86_FEATURE_IBRS) )
     {
         wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
         info->last_spec_ctrl = default_xen_spec_ctrl;
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index b2fd86ebe587..c210bb662f84 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 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

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

[Jan Beulich]

On 28.01.2022 14:29, 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.  Also update the
> boot/resume paths to configure default_xen_spec_ctrl.
> 
> svm.h needs an adjustment to remove a dependency on include order.
> 
> For now, only active alternatives for HVM - PV will require more work.  No
> functional change, as no alternatives are defined yet for HVM yet.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH v2 7/9] 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
compatibility, 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.  The use of double alternative
blocks is due to a quirk of the current infrastructure, where call
displacements only get fixed up for the first replacement instruction.  While
adjusting the clobber lists, drop the stale requirements on the VMExit side.

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>

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.

v2:
 * Split last_spec_ctrl introduction into earlier patch.
 * Use STR() rather than __stringify() for brevity.
 * Use double alt blocks in order to pass function parameters.
---
 xen/arch/x86/hvm/svm/entry.S             | 12 +++++++-----
 xen/arch/x86/hvm/svm/svm.c               | 27 +++++++++++++++++++++++++++
 xen/arch/x86/include/asm/msr.h           |  9 +++++++++
 xen/arch/x86/include/asm/spec_ctrl_asm.h |  3 +++
 4 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index 276215d36aff..190f7095c65c 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -55,11 +55,12 @@ __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 "", STR(mov %rbx, %rdi; mov %rsp, %rsi), X86_FEATURE_SC_MSR_HVM
+        ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM
 
         pop  %r15
         pop  %r14
@@ -78,7 +79,6 @@ __UNLIKELY_END(nsvm_hap)
         pop  %rsi
         pop  %rdi
 
-        clgi
         sti
         vmrun
 
@@ -86,8 +86,10 @@ __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: C   */
         ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
+        ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM
+        ALTERNATIVE "", STR(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..f753bf48c252 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -3086,6 +3086,33 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     vmcb_set_vintr(vmcb, intr);
 }
 
+/* Called with GIF=0. */
+void vmexit_spec_ctrl(struct cpu_info *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(const struct vcpu *curr, struct cpu_info *info)
+{
+    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/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 9c0c7622c41f..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:
-- 
2.11.0

Re: [PATCH v2 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich]

On 28.01.2022 14:29, Andrew Cooper wrote:
> 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
> compatibility, 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.  The use of double alternative
> blocks is due to a quirk of the current infrastructure, where call
> displacements only get fixed up for the first replacement instruction.  While
> adjusting the clobber lists, drop the stale requirements on the VMExit side.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -55,11 +55,12 @@ __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 "", STR(mov %rbx, %rdi; mov %rsp, %rsi), X86_FEATURE_SC_MSR_HVM
> +        ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM

Is there a reason to use a macro for converting to a string here at
all? There are no "inner" macros here which might need expanding. And
"brevity" (as you have in the rev log) would call for

        ALTERNATIVE "", "mov %rbx, %rdi; mov %rsp, %rsi", X86_FEATURE_SC_MSR_HVM
        ALTERNATIVE "", "call vmentry_spec_ctrl", X86_FEATURE_SC_MSR_HVM

.

> @@ -86,8 +86,10 @@ __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: C   */
>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
> +        ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM
> +        ALTERNATIVE "", STR(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM

Same here then, obviously.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -3086,6 +3086,33 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>      vmcb_set_vintr(vmcb, intr);
>  }
>  
> +/* Called with GIF=0. */
> +void vmexit_spec_ctrl(struct cpu_info *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(const struct vcpu *curr, struct cpu_info *info)
> +{
> +    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. */
> +}

These living in SVM code I think their names want to avoid suggesting
they could also be used for VMX (irrespective of us not meaning to use
them there). Or else they want to move to common code, with comments
slightly adjusted.

Jan

Re: [PATCH v2 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

On 31/01/2022 10:33, Jan Beulich wrote:
> On 28.01.2022 14:29, Andrew Cooper wrote:
>> 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
>> compatibility, 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.  The use of double alternative
>> blocks is due to a quirk of the current infrastructure, where call
>> displacements only get fixed up for the first replacement instruction.  While
>> adjusting the clobber lists, drop the stale requirements on the VMExit side.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Again technically:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> But ...
>
>> --- a/xen/arch/x86/hvm/svm/entry.S
>> +++ b/xen/arch/x86/hvm/svm/entry.S
>> @@ -55,11 +55,12 @@ __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 "", STR(mov %rbx, %rdi; mov %rsp, %rsi), X86_FEATURE_SC_MSR_HVM
>> +        ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM
> Is there a reason to use a macro for converting to a string here at
> all? There are no "inner" macros here which might need expanding. And
> "brevity" (as you have in the rev log) would call for
>
>         ALTERNATIVE "", "mov %rbx, %rdi; mov %rsp, %rsi", X86_FEATURE_SC_MSR_HVM
>         ALTERNATIVE "", "call vmentry_spec_ctrl", X86_FEATURE_SC_MSR_HVM

Good point.  I'll switch to plain strings.

>
>
>> @@ -86,8 +86,10 @@ __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: C   */
>>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
>> +        ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM
>> +        ALTERNATIVE "", STR(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM
> Same here then, obviously.
>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -3086,6 +3086,33 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>      vmcb_set_vintr(vmcb, intr);
>>  }
>>  
>> +/* Called with GIF=0. */
>> +void vmexit_spec_ctrl(struct cpu_info *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(const struct vcpu *curr, struct cpu_info *info)
>> +{
>> +    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. */
>> +}
> These living in SVM code I think their names want to avoid suggesting
> they could also be used for VMX (irrespective of us not meaning to use
> them there). Or else they want to move to common code, with comments
> slightly adjusted.

I'll add svm_ prefixes.  I can't see these being useful elsewhere.

~Andrew

Re: [PATCH v2 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich]

On 28.01.2022 14:29, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -55,11 +55,12 @@ __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 "", STR(mov %rbx, %rdi; mov %rsp, %rsi), X86_FEATURE_SC_MSR_HVM
> +        ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM

Both this and ...

> @@ -86,8 +86,10 @@ __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: C   */
>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
> +        ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM
> +        ALTERNATIVE "", STR(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */

... this now effectively violate what the warning comment says, as there
is a RET involved in the C call. If this is not a problem for some reason,
I'd like to ask that the comments be updated accordingly.

Jan

Re: [PATCH v2 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

On 31/01/2022 12:55, Jan Beulich wrote:
> On 28.01.2022 14:29, Andrew Cooper wrote:
>> --- a/xen/arch/x86/hvm/svm/entry.S
>> +++ b/xen/arch/x86/hvm/svm/entry.S
>> @@ -55,11 +55,12 @@ __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 "", STR(mov %rbx, %rdi; mov %rsp, %rsi), X86_FEATURE_SC_MSR_HVM
>> +        ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM
> Both this and ...
>
>> @@ -86,8 +86,10 @@ __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: C   */
>>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
>> +        ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM
>> +        ALTERNATIVE "", STR(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> ... this now effectively violate what the warning comment says, as there
> is a RET involved in the C call. If this is not a problem for some reason,
> I'd like to ask that the comments be updated accordingly.

The `ret` note pertains to two things:
1) RSB underflows falling back to indirect predictions
2) SpectreRSB executing more rets than calls

Aspect 2 is largely theoretical, but can happen with an out of bounds
write which hits the return address on the stack in an otherwise regular
call tree.

Once DO_OVERWRITE_RSB is complete, there are no user RSB entries to
consume.  I know this gets complicated with the RAS[:32] flushing which
is part of why the behaviour is up for consideration, but even the
current code completes the full flush before a ret is executed.

Aspect 1 is a feature seemingly unique to Intel CPUs, and we have to set
MSR_SPEC_CTRL.IBRS to 1 before indirect predictions are "safe".


That said, I stand by the comments as they are.  They're there for other
code to remember to be careful.  I think it is entirely reasonable to
expect the internals of the speculative safety logic to know how to stay
safe.

I'll see how it looks with the helpers inlined.  That's the easiest way
of fixing this issue.

~Andrew

[PATCH v3 7/9] 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
compatibility, 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.

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>

v3:
 * Implement in asm
---
 xen/arch/x86/hvm/svm/entry.S             | 34 +++++++++++++++++++++++++++-----
 xen/arch/x86/include/asm/msr.h           |  9 +++++++++
 xen/arch/x86/include/asm/spec_ctrl_asm.h |  3 +++
 xen/arch/x86/x86_64/asm-offsets.c        |  1 +
 4 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index 276215d36aff..16b642c9e2de 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -55,11 +55,23 @@ __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: b=curr %rsp=regs/cpuinfo, Clob: acd */
+        .macro svm_vmentry_spec_ctrl
+            mov    VCPU_arch_msrs(%rbx), %rax
+            movzbl CPUINFO_last_spec_ctrl(%rsp), %edx
+            mov    VCPUMSR_spec_ctrl_raw(%rax), %eax
+            cmp    %edx, %eax
+            je 1f  /* Skip write if value is correct. */
+            mov    $MSR_SPEC_CTRL, %ecx
+            xor    %edx, %edx
+            wrmsr
+            mov    %al, CPUINFO_last_spec_ctrl(%rsp)
+1:          /* No Spectre v1 concerns.  Execution will hit VMRUN imminently. */
+        .endm
+        ALTERNATIVE "", svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
 
         pop  %r15
         pop  %r14
@@ -78,7 +90,6 @@ __UNLIKELY_END(nsvm_hap)
         pop  %rsi
         pop  %rdi
 
-        clgi
         sti
         vmrun
 
@@ -86,8 +97,21 @@ __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: %rsp=regs/cpuinfo         Clob: acd */
         ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
+
+        .macro svm_vmexit_spec_ctrl
+            /*
+             * Write to MSR_SPEC_CTRL unconditionally, for the RAS[:32]
+             * flushing side effect.
+             */
+            mov    $MSR_SPEC_CTRL, %ecx
+            movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
+            xor    %edx, %edx
+            wrmsr
+            mov    %al, CPUINFO_last_spec_ctrl(%rsp)
+        .endm
+        ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
         stgi
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 9c0c7622c41f..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:
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 649892643fe9..287dac101ad4 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -126,6 +126,7 @@ void __dummy__(void)
     OFFSET(CPUINFO_pv_cr3, struct cpu_info, pv_cr3);
     OFFSET(CPUINFO_shadow_spec_ctrl, struct cpu_info, shadow_spec_ctrl);
     OFFSET(CPUINFO_xen_spec_ctrl, struct cpu_info, xen_spec_ctrl);
+    OFFSET(CPUINFO_last_spec_ctrl, struct cpu_info, last_spec_ctrl);
     OFFSET(CPUINFO_spec_ctrl_flags, struct cpu_info, spec_ctrl_flags);
     OFFSET(CPUINFO_root_pgt_changed, struct cpu_info, root_pgt_changed);
     OFFSET(CPUINFO_use_pv_cr3, struct cpu_info, use_pv_cr3);
-- 
2.11.0

Re: [PATCH v3 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich]

On 31.01.2022 16:36, Andrew Cooper wrote:
> 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
> compatibility,

I think you've explained this to me before, but I can't seem to put
all of it together already now. Could expand on how a non-zero value
behind a guest's back can help with migration compatibility? At the
first glance I would be inclined to say only what the guest actually
gets to see and use can affect its migration.

> 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Preferably with the above expansion and with one further style
issue (see below) taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -55,11 +55,23 @@ __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: b=curr %rsp=regs/cpuinfo, Clob: acd */
> +        .macro svm_vmentry_spec_ctrl
> +            mov    VCPU_arch_msrs(%rbx), %rax
> +            movzbl CPUINFO_last_spec_ctrl(%rsp), %edx
> +            mov    VCPUMSR_spec_ctrl_raw(%rax), %eax
> +            cmp    %edx, %eax
> +            je 1f  /* Skip write if value is correct. */

Wold you mind padding the insn operand properly, in line with all
others nearby?

Jan

Re: [PATCH v3 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

On 01/02/2022 11:47, Jan Beulich wrote:
> On 31.01.2022 16:36, Andrew Cooper wrote:
>> 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
>> compatibility,
> I think you've explained this to me before, but I can't seem to put
> all of it together already now. Could expand on how a non-zero value
> behind a guest's back can help with migration compatibility? At the
> first glance I would be inclined to say only what the guest actually
> gets to see and use can affect its migration.

For VMs which see VIRT_SPEC_CTRL (compatibility with Fam15 thru Zen1),
writes of VIRT_SPEC_CTRL.SSBD (probably) need to use
SSBD-behind-the-guest's back.  I say probably, because I think this is
the least bad implementation option, but until we have working support,
it's still a guess.

For the ultra paranoid, a VM migrating in which can't see PSFD (e.g. for
compatibility with Zen2) should have PSFD set behind it's back.  Except
that SSBD also has an appropriate side effect so that existing "I'm a
piece of critical code" signals that have grown in various OSes continue
to do the safe thing on PSFD-capable hardware.  Given that we don't
activate SSBD by default, we shouldn't default disable PFSD behind an
unaware guest either.

That then leaves the meaning of spec-ctrl=ssbd,psfd because ssbd is
currently system wide (if enabled) on AMD.  This series changes that for
HVM guests, and it will change again shortly for PV guests, and this is
obviously the better default behaviour.  But we could have a system wide
option on top of guest support in most cases if someone sees a need.

>> and for performance reasons with SEV-SNP guests, we want the
>> ability to use a nonzero value behind the guest's back.

For completeness, for SEV-SNP, IBRS needs setting to avoid vmentry
issuing IBPB.  More specifically, the VMexit=>Entry path must not clear
IBRS, at which point hardware knows that nothing can have got into the
indirect predictor.


>>   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.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Preferably with the above expansion and with one further style
> issue (see below) taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks

>
>> --- a/xen/arch/x86/hvm/svm/entry.S
>> +++ b/xen/arch/x86/hvm/svm/entry.S
>> @@ -55,11 +55,23 @@ __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: b=curr %rsp=regs/cpuinfo, Clob: acd */
>> +        .macro svm_vmentry_spec_ctrl
>> +            mov    VCPU_arch_msrs(%rbx), %rax
>> +            movzbl CPUINFO_last_spec_ctrl(%rsp), %edx
>> +            mov    VCPUMSR_spec_ctrl_raw(%rax), %eax
>> +            cmp    %edx, %eax
>> +            je 1f  /* Skip write if value is correct. */
> Wold you mind padding the insn operand properly, in line with all
> others nearby?

Oops yes.  Fixed.

~Andrew

Re: [PATCH v3 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Jan Beulich]

On 01.02.2022 13:28, Andrew Cooper wrote:
> On 01/02/2022 11:47, Jan Beulich wrote:
>> On 31.01.2022 16:36, Andrew Cooper wrote:
>>> 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
>>> compatibility,
>> I think you've explained this to me before, but I can't seem to put
>> all of it together already now. Could expand on how a non-zero value
>> behind a guest's back can help with migration compatibility? At the
>> first glance I would be inclined to say only what the guest actually
>> gets to see and use can affect its migration.
> 
> For VMs which see VIRT_SPEC_CTRL (compatibility with Fam15 thru Zen1),
> writes of VIRT_SPEC_CTRL.SSBD (probably) need to use
> SSBD-behind-the-guest's back.  I say probably, because I think this is
> the least bad implementation option, but until we have working support,
> it's still a guess.

So this is future work (and mentioning just this in the description
would be enough to address my comment), but ...

> For the ultra paranoid, a VM migrating in which can't see PSFD (e.g. for
> compatibility with Zen2) should have PSFD set behind it's back.

... this is something we should be doing right away then?

Jan

Re: [PATCH v3 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper]

On 01/02/2022 12:40, Jan Beulich wrote:
> On 01.02.2022 13:28, Andrew Cooper wrote:
>> On 01/02/2022 11:47, Jan Beulich wrote:
>>> On 31.01.2022 16:36, Andrew Cooper wrote:
>>>> 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
>>>> compatibility,
>>> I think you've explained this to me before, but I can't seem to put
>>> all of it together already now. Could expand on how a non-zero value
>>> behind a guest's back can help with migration compatibility? At the
>>> first glance I would be inclined to say only what the guest actually
>>> gets to see and use can affect its migration.
>> For VMs which see VIRT_SPEC_CTRL (compatibility with Fam15 thru Zen1),
>> writes of VIRT_SPEC_CTRL.SSBD (probably) need to use
>> SSBD-behind-the-guest's back.  I say probably, because I think this is
>> the least bad implementation option, but until we have working support,
>> it's still a guess.
> So this is future work (and mentioning just this in the description
> would be enough to address my comment)

Near future, but yes.

> , but ...
>
>> For the ultra paranoid, a VM migrating in which can't see PSFD (e.g. for
>> compatibility with Zen2) should have PSFD set behind it's back.
> ... this is something we should be doing right away then?

Except for the second half of this paragraph which was an argument as to
why not.

What OSes expose to userspace for "I need speculative safety" works
whether the kernel can see PSFD or not.

~Andrew

[PATCH v2 8/9] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.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 f753bf48c252..aa82fe29befb 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 v2 9/9] 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), 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(-)

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],
-- 
2.11.0

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

[Jan Beulich]

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

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

[Andrew Cooper]

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