[PATCH 0/3] x86/amd: Hardware speculative controls

[PATCH 0/3] x86/amd: Hardware speculative controls

[Andrew Cooper]

This is the very beginning the work to start using AMD hardware speculative
controls on Zen2 or later.

The extent of the work is very tangled, and breaks down roughly like this:

1) Teach Xen to use AMD's MSR_SPEC_CTRL and context switch per vCPU.  It
   requires editing the common MSR_SPEC_CTRL logic, at which point it would be
   short sighted not to include Intel's eIBRS at the same time.
2) Expose MSR_SPEC_CTRL to AMD guests, along with all the hint bits.
3) Implement MSR_VIRT_SPEC_CTRL for guests, in terms of MSR_SPEC_CTRL.SSBD.
   Will be off by default on Zen2 and later, but needs to be usable for
   migration compatibility.
4) Implement legacy Memory Disambiguation context switching for pre-Zen2
   parts, and expose MSR_VIRT_SPEC_CTRL to guests on older parts.

In terms of end results, this is what the hardware feature look like on
various generations of AMD CPU:

Fam15h (Opteron 6212):
  (XEN) Speculative mitigation facilities:
  (XEN)   Hardware hints:
  (XEN)   Hardware features: IBPB
  (XEN)   Compiled-in support: INDIRECT_THUNK SHADOW_PAGING
  (XEN)   Xen settings: BTI-Thunk LFENCE, SPEC_CTRL: No, Other: IBPB BRANCH_HARDEN
  (XEN)   Support for HVM VMs: RSB
  (XEN)   Support for PV VMs: RSB
  (XEN)   XPTI (64-bit PV only): Dom0 disabled, DomU disabled (without PCID)
  (XEN)   PV L1TF shadowing: Dom0 disabled, DomU disabled

Zen1:
  (XEN) Speculative mitigation facilities:
  (XEN)   Hardware hints:
  (XEN)   Hardware features: IBPB
  (XEN)   Compiled-in support: INDIRECT_THUNK SHADOW_PAGING
  (XEN)   Xen settings: BTI-Thunk LFENCE, SPEC_CTRL: No, Other: IBPB BRANCH_HARDEN
  (XEN)   Support for HVM VMs: RSB
  (XEN)   Support for PV VMs: RSB
  (XEN)   XPTI (64-bit PV only): Dom0 disabled, DomU disabled (without PCID)
  (XEN)   PV L1TF shadowing: Dom0 disabled, DomU disabled

Zen2:
  (XEN) Speculative mitigation facilities:
  (XEN)   Hardware hints: IBRS_FAST IBRS_SAME_MODE
  (XEN)   Hardware features: IBPB IBRS STIBP SSBD
  (XEN)   Compiled-in support: INDIRECT_THUNK SHADOW_PAGING
  (XEN)   Xen settings: BTI-Thunk LFENCE, SPEC_CTRL: No, Other: IBPB BRANCH_HARDEN
  (XEN)   Support for HVM VMs: RSB
  (XEN)   Support for PV VMs: RSB
  (XEN)   XPTI (64-bit PV only): Dom0 disabled, DomU disabled (without PCID)
  (XEN)   PV L1TF shadowing: Dom0 disabled, DomU disabled

Zen3:
  (XEN) Speculative mitigation facilities:
  (XEN)   Hardware hints: STIBP_ALWAYS IBRS_FAST IBRS_SAME_MODE
  (XEN)   Hardware features: IBPB IBRS STIBP SSBD PSFD
  (XEN)   Compiled-in support: INDIRECT_THUNK SHADOW_PAGING
  (XEN)   Xen settings: BTI-Thunk LFENCE, SPEC_CTRL: No, Other: IBPB BRANCH_HARDEN
  (XEN)   Support for HVM VMs: RSB
  (XEN)   Support for PV VMs: RSB
  (XEN)   XPTI (64-bit PV only): Dom0 disabled, DomU disabled (with PCID)
  (XEN)   PV L1TF shadowing: Dom0 disabled, DomU disabled

Although full support for all of this is a way off yet.

To start with, make the existing `spec-ctrl=no-ssbd` command line option
actually work on Zen3.

Andrew Cooper (3):
  x86/spec-ctrl: Split the "Hardware features" diagnostic line
  x86/amd: Enumeration for speculative features/hints
  x86/amd: Use newer SSBD mechanisms if they exist

 tools/libs/light/libxl_cpuid.c              | 10 +++++
 tools/misc/xen-cpuid.c                      |  8 +++-
 xen/arch/x86/cpu/amd.c                      | 69 +++++++++++++++++++++--------
 xen/arch/x86/cpu/cpu.h                      |  1 +
 xen/arch/x86/cpu/hygon.c                    | 10 +----
 xen/arch/x86/hvm/svm/svm.c                  |  1 +
 xen/arch/x86/hvm/svm/vmcb.c                 |  1 +
 xen/arch/x86/spec_ctrl.c                    | 44 +++++++++++-------
 xen/include/asm-x86/cpufeature.h            |  5 +++
 xen/include/asm-x86/hvm/svm/svm.h           |  2 +
 xen/include/asm-x86/hvm/svm/vmcb.h          |  4 +-
 xen/include/asm-x86/msr-index.h             |  3 ++
 xen/include/public/arch-x86/cpufeatureset.h | 10 +++++
 13 files changed, 122 insertions(+), 46 deletions(-)

-- 
2.11.0

[PATCH 1/3] x86/spec-ctrl: Split the "Hardware features" diagnostic line

[Andrew Cooper]

Separate the read-only hints from the features requiring active actions on
Xen's behalf.

Also take the opportunity split the IBRS/IBPB and IBPB mess.  More features
with overlapping enumeration are on the way, and and it is not useful to split
them like this.

No practical 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 | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 739b7913ff86..9bf0fbf99813 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -317,23 +317,30 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
 
     printk("Speculative mitigation facilities:\n");
 
-    /* Hardware features which pertain to speculative mitigations. */
-    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
-           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
-           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
-           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
-           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
-           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
-           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "",
-           (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
-           (caps & ARCH_CAPS_IBRS_ALL)              ? " IBRS_ALL"  : "",
-           (caps & ARCH_CAPS_RDCL_NO)               ? " RDCL_NO"   : "",
-           (caps & ARCH_CAPS_RSBA)                  ? " RSBA"      : "",
-           (caps & ARCH_CAPS_SKIP_L1DFL)            ? " SKIP_L1DFL": "",
-           (caps & ARCH_CAPS_SSB_NO)                ? " SSB_NO"    : "",
-           (caps & ARCH_CAPS_MDS_NO)                ? " MDS_NO"    : "",
-           (caps & ARCH_CAPS_TSX_CTRL)              ? " TSX_CTRL"  : "",
-           (caps & ARCH_CAPS_TAA_NO)                ? " TAA_NO"    : "");
+    /*
+     * Hardware read-only information, stating immunity to certain issues, or
+     * suggestions of which mitigation to use.
+     */
+    printk("  Hardware hints:%s%s%s%s%s%s%s\n",
+           (caps & ARCH_CAPS_RDCL_NO)                        ? " RDCL_NO"        : "",
+           (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",
+           (caps & ARCH_CAPS_RSBA)                           ? " RSBA"           : "",
+           (caps & ARCH_CAPS_SKIP_L1DFL)                     ? " SKIP_L1DFL"     : "",
+           (caps & ARCH_CAPS_SSB_NO)                         ? " SSB_NO"         : "",
+           (caps & ARCH_CAPS_MDS_NO)                         ? " MDS_NO"         : "",
+           (caps & ARCH_CAPS_TAA_NO)                         ? " TAA_NO"         : "");
+
+    /* Hardware features which need driving to mitigate issues. */
+    printk("  Hardware features:%s%s%s%s%s%s%s%s\n",
+           (e8b  & cpufeat_mask(X86_FEATURE_IBPB)) ||
+           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB))          ? " IBPB"           : "",
+           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB))          ? " IBRS"           : "",
+           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP))          ? " STIBP"          : "",
+           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))           ? " SSBD"           : "",
+           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH))      ? " L1D_FLUSH"      : "",
+           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR))       ? " MD_CLEAR"       : "",
+           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL))     ? " SRBDS_CTRL"     : "",
+           (caps & ARCH_CAPS_TSX_CTRL)                       ? " TSX_CTRL"       : "");
 
     /* Compiled-in support which pertains to mitigations. */
     if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) || IS_ENABLED(CONFIG_SHADOW_PAGING) )
-- 
2.11.0

Re: [PATCH 1/3] x86/spec-ctrl: Split the "Hardware features" diagnostic line

[Jan Beulich]

On 17.08.2021 16:30, Andrew Cooper wrote:
> Separate the read-only hints from the features requiring active actions on
> Xen's behalf.
> 
> Also take the opportunity split the IBRS/IBPB and IBPB mess.  More features
> with overlapping enumeration are on the way, and and it is not useful to split
> them like this.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a remark and a question:

> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -317,23 +317,30 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
>  
>      printk("Speculative mitigation facilities:\n");
>  
> -    /* Hardware features which pertain to speculative mitigations. */
> -    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
> -           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
> -           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
> -           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
> -           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
> -           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
> -           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "",
> -           (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
> -           (caps & ARCH_CAPS_IBRS_ALL)              ? " IBRS_ALL"  : "",
> -           (caps & ARCH_CAPS_RDCL_NO)               ? " RDCL_NO"   : "",
> -           (caps & ARCH_CAPS_RSBA)                  ? " RSBA"      : "",
> -           (caps & ARCH_CAPS_SKIP_L1DFL)            ? " SKIP_L1DFL": "",
> -           (caps & ARCH_CAPS_SSB_NO)                ? " SSB_NO"    : "",
> -           (caps & ARCH_CAPS_MDS_NO)                ? " MDS_NO"    : "",
> -           (caps & ARCH_CAPS_TSX_CTRL)              ? " TSX_CTRL"  : "",
> -           (caps & ARCH_CAPS_TAA_NO)                ? " TAA_NO"    : "");
> +    /*
> +     * Hardware read-only information, stating immunity to certain issues, or
> +     * suggestions of which mitigation to use.
> +     */
> +    printk("  Hardware hints:%s%s%s%s%s%s%s\n",
> +           (caps & ARCH_CAPS_RDCL_NO)                        ? " RDCL_NO"        : "",
> +           (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",

I take it you flipped the order of these two to match the ordering
of their bit numbers? I'm slightly inclined to ask whether we
wouldn't better stay with what we had, as I could imagine users
having not sufficiently flexible text matching in place somewhere.
But I'm not going to insist. It only occurred to me and is, unlike
for the IBRS/IBPB re-arrangement of the other part, easily possible
here.

> +           (caps & ARCH_CAPS_RSBA)                           ? " RSBA"           : "",
> +           (caps & ARCH_CAPS_SKIP_L1DFL)                     ? " SKIP_L1DFL"     : "",
> +           (caps & ARCH_CAPS_SSB_NO)                         ? " SSB_NO"         : "",
> +           (caps & ARCH_CAPS_MDS_NO)                         ? " MDS_NO"         : "",
> +           (caps & ARCH_CAPS_TAA_NO)                         ? " TAA_NO"         : "");

I'm curious why we do not report IF_PSCHANGE_MC_NO here.

Jan

Re: [PATCH 1/3] x86/spec-ctrl: Split the "Hardware features" diagnostic line

[Andrew Cooper]

On 19/08/2021 15:38, Jan Beulich wrote:
> On 17.08.2021 16:30, Andrew Cooper wrote:
>> Separate the read-only hints from the features requiring active actions on
>> Xen's behalf.
>>
>> Also take the opportunity split the IBRS/IBPB and IBPB mess.  More features
>> with overlapping enumeration are on the way, and and it is not useful to split
>> them like this.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.

> with a remark and a question:
>
>> --- a/xen/arch/x86/spec_ctrl.c
>> +++ b/xen/arch/x86/spec_ctrl.c
>> @@ -317,23 +317,30 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
>>  
>>      printk("Speculative mitigation facilities:\n");
>>  
>> -    /* Hardware features which pertain to speculative mitigations. */
>> -    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "",
>> -           (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
>> -           (caps & ARCH_CAPS_IBRS_ALL)              ? " IBRS_ALL"  : "",
>> -           (caps & ARCH_CAPS_RDCL_NO)               ? " RDCL_NO"   : "",
>> -           (caps & ARCH_CAPS_RSBA)                  ? " RSBA"      : "",
>> -           (caps & ARCH_CAPS_SKIP_L1DFL)            ? " SKIP_L1DFL": "",
>> -           (caps & ARCH_CAPS_SSB_NO)                ? " SSB_NO"    : "",
>> -           (caps & ARCH_CAPS_MDS_NO)                ? " MDS_NO"    : "",
>> -           (caps & ARCH_CAPS_TSX_CTRL)              ? " TSX_CTRL"  : "",
>> -           (caps & ARCH_CAPS_TAA_NO)                ? " TAA_NO"    : "");
>> +    /*
>> +     * Hardware read-only information, stating immunity to certain issues, or
>> +     * suggestions of which mitigation to use.
>> +     */
>> +    printk("  Hardware hints:%s%s%s%s%s%s%s\n",
>> +           (caps & ARCH_CAPS_RDCL_NO)                        ? " RDCL_NO"        : "",
>> +           (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",
> I take it you flipped the order of these two to match the ordering
> of their bit numbers?

Yes.  IIRC, the first draft spec had the bits in the opposite order, and
I presumably forgot to flip the printk() when correcting msr-index.h

>  I'm slightly inclined to ask whether we
> wouldn't better stay with what we had, as I could imagine users
> having not sufficiently flexible text matching in place somewhere.
> But I'm not going to insist. It only occurred to me and is, unlike
> for the IBRS/IBPB re-arrangement of the other part, easily possible
> here.

dmesg is not and never can will be an ABI.

Amongst other things, `xl dmesg | grep` fails at boot on large systems
(because you keep on refusing to let in patches which bump the size of
the pre-dynamic console), or after sufficient uptime when the contents
has wrapped.

If you want an ABI, then it ought to be in xenhypfs or some other
hypercall, where the information is guaranteed to be available at any
point in time.

>> +           (caps & ARCH_CAPS_RSBA)                           ? " RSBA"           : "",
>> +           (caps & ARCH_CAPS_SKIP_L1DFL)                     ? " SKIP_L1DFL"     : "",
>> +           (caps & ARCH_CAPS_SSB_NO)                         ? " SSB_NO"         : "",
>> +           (caps & ARCH_CAPS_MDS_NO)                         ? " MDS_NO"         : "",
>> +           (caps & ARCH_CAPS_TAA_NO)                         ? " TAA_NO"         : "");
> I'm curious why we do not report IF_PSCHANGE_MC_NO here.

It isn't a speculative sidechannel vulnerability.

It is "error in the instruction fetch leads to a completely dead CPU",
and is reported in the EPT setup logic.

MSR_ARCH_CAPS really is just "misc CPUID bits" which were done in an MSR
because of microcode patch space.

~Andrew

Re: [PATCH 1/3] x86/spec-ctrl: Split the "Hardware features" diagnostic line

[Jan Beulich]

On 24.08.2021 14:57, Andrew Cooper wrote:
> On 19/08/2021 15:38, Jan Beulich wrote:
>> On 17.08.2021 16:30, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/spec_ctrl.c
>>> +++ b/xen/arch/x86/spec_ctrl.c
>>> @@ -317,23 +317,30 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
>>>  
>>>      printk("Speculative mitigation facilities:\n");
>>>  
>>> -    /* Hardware features which pertain to speculative mitigations. */
>>> -    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "",
>>> -           (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
>>> -           (caps & ARCH_CAPS_IBRS_ALL)              ? " IBRS_ALL"  : "",
>>> -           (caps & ARCH_CAPS_RDCL_NO)               ? " RDCL_NO"   : "",
>>> -           (caps & ARCH_CAPS_RSBA)                  ? " RSBA"      : "",
>>> -           (caps & ARCH_CAPS_SKIP_L1DFL)            ? " SKIP_L1DFL": "",
>>> -           (caps & ARCH_CAPS_SSB_NO)                ? " SSB_NO"    : "",
>>> -           (caps & ARCH_CAPS_MDS_NO)                ? " MDS_NO"    : "",
>>> -           (caps & ARCH_CAPS_TSX_CTRL)              ? " TSX_CTRL"  : "",
>>> -           (caps & ARCH_CAPS_TAA_NO)                ? " TAA_NO"    : "");
>>> +    /*
>>> +     * Hardware read-only information, stating immunity to certain issues, or
>>> +     * suggestions of which mitigation to use.
>>> +     */
>>> +    printk("  Hardware hints:%s%s%s%s%s%s%s\n",
>>> +           (caps & ARCH_CAPS_RDCL_NO)                        ? " RDCL_NO"        : "",
>>> +           (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",
>> I take it you flipped the order of these two to match the ordering
>> of their bit numbers?
> 
> Yes.  IIRC, the first draft spec had the bits in the opposite order, and
> I presumably forgot to flip the printk() when correcting msr-index.h
> 
>>  I'm slightly inclined to ask whether we
>> wouldn't better stay with what we had, as I could imagine users
>> having not sufficiently flexible text matching in place somewhere.
>> But I'm not going to insist. It only occurred to me and is, unlike
>> for the IBRS/IBPB re-arrangement of the other part, easily possible
>> here.
> 
> dmesg is not and never can will be an ABI.

Well, sure, I understand this. I said "slightly" not because I would
use the log this way, but because I know of at least similar (ab)uses
of logs.

> Amongst other things, `xl dmesg | grep` fails at boot on large systems
> (because you keep on refusing to let in patches which bump the size of
> the pre-dynamic console),

And instead I've taken the time to reduce boot time verbosity. Plus -
the last attempt must have been years ago. Given good enough arguments
and little enough undesirable (side) effects, I'm sure I could be
convinced. (But yes, especially the "good enough" aspect is definitely
pretty subjective. Yet then I could be easily outvoted if others agree
with the provided reasoning.)

> or after sufficient uptime when the contents has wrapped.

The boot log can easily be made persistent on disk before it can wrap.

Jan

Re: [PATCH 1/3] x86/spec-ctrl: Split the "Hardware features" diagnostic line

[Andrew Cooper]

On 24/08/2021 14:15, Jan Beulich wrote:
> On 24.08.2021 14:57, Andrew Cooper wrote:
>> On 19/08/2021 15:38, Jan Beulich wrote:
>>> On 17.08.2021 16:30, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/spec_ctrl.c
>>>> +++ b/xen/arch/x86/spec_ctrl.c
>>>> @@ -317,23 +317,30 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
>>>>  
>>>>      printk("Speculative mitigation facilities:\n");
>>>>  
>>>> -    /* Hardware features which pertain to speculative mitigations. */
>>>> -    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
>>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
>>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
>>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
>>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "",
>>>> -           (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "",
>>>> -           (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
>>>> -           (caps & ARCH_CAPS_IBRS_ALL)              ? " IBRS_ALL"  : "",
>>>> -           (caps & ARCH_CAPS_RDCL_NO)               ? " RDCL_NO"   : "",
>>>> -           (caps & ARCH_CAPS_RSBA)                  ? " RSBA"      : "",
>>>> -           (caps & ARCH_CAPS_SKIP_L1DFL)            ? " SKIP_L1DFL": "",
>>>> -           (caps & ARCH_CAPS_SSB_NO)                ? " SSB_NO"    : "",
>>>> -           (caps & ARCH_CAPS_MDS_NO)                ? " MDS_NO"    : "",
>>>> -           (caps & ARCH_CAPS_TSX_CTRL)              ? " TSX_CTRL"  : "",
>>>> -           (caps & ARCH_CAPS_TAA_NO)                ? " TAA_NO"    : "");
>>>> +    /*
>>>> +     * Hardware read-only information, stating immunity to certain issues, or
>>>> +     * suggestions of which mitigation to use.
>>>> +     */
>>>> +    printk("  Hardware hints:%s%s%s%s%s%s%s\n",
>>>> +           (caps & ARCH_CAPS_RDCL_NO)                        ? " RDCL_NO"        : "",
>>>> +           (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",
>>> I take it you flipped the order of these two to match the ordering
>>> of their bit numbers?
>> Yes.  IIRC, the first draft spec had the bits in the opposite order, and
>> I presumably forgot to flip the printk() when correcting msr-index.h
>>
>>>  I'm slightly inclined to ask whether we
>>> wouldn't better stay with what we had, as I could imagine users
>>> having not sufficiently flexible text matching in place somewhere.
>>> But I'm not going to insist. It only occurred to me and is, unlike
>>> for the IBRS/IBPB re-arrangement of the other part, easily possible
>>> here.
>> dmesg is not and never can will be an ABI.
> Well, sure, I understand this. I said "slightly" not because I would
> use the log this way, but because I know of at least similar (ab)uses
> of logs.

Lots of details which are currently only available in `xl dmesg` should
be available via a real hypercall.

The domain creation improvement work is making headway with retrofitting
proper details to virt_caps/etc, but there is loads more data needing
exposing.

>
>> Amongst other things, `xl dmesg | grep` fails at boot on large systems
>> (because you keep on refusing to let in patches which bump the size of
>> the pre-dynamic console),
> And instead I've taken the time to reduce boot time verbosity. Plus -
> the last attempt must have been years ago. Given good enough arguments
> and little enough undesirable (side) effects, I'm sure I could be
> convinced. (But yes, especially the "good enough" aspect is definitely
> pretty subjective. Yet then I could be easily outvoted if others agree
> with the provided reasoning.)
>
>> or after sufficient uptime when the contents has wrapped.
> The boot log can easily be made persistent on disk before it can wrap.

All failures we've had with customers are the fixed initdata buffer
wrapping before it gets dynamically allocated.  As a consequence, we
unilaterally up _CONRING_SIZE to 64k.

And yes - reducing the verbosity of the ACPI tables is a good thing, but
there's plenty more in need of shrinking.

~Andrew

[PATCH 2/3] x86/amd: Enumeration for speculative features/hints

[Andrew Cooper]

There is a step change in speculation protections between the Zen1 and Zen2
microarchitectures.

Zen1 and older have no special support.  Control bits in non-architectural
MSRs are used to make lfence be dispatch-serialising (Spectre v1), and to
disable Memory Disambiguation (Speculative Store Bypass).  IBPB was
retrofitted in a microcode update, and software methods are required for
Spectre v2 protections.

Because the bit controlling Memory Disambiguation is model specific,
hypervisors are expected to expose a MSR_VIRT_SPEC_CTRL interface which
abstracts the model specific details.

Zen2 and later implement the MSR_SPEC_CTRL interface in hardware, and
virtualise the interface for HVM guests to use.  A number of hint bits are
specified too to help guide OS software to the most efficient mitigation
strategy.

Zen3 introduced a new feature, Predictive Store Forwarding, along with a
control to disable it in sensitive code.

Add CPUID and VMCB details for all the new functionality.

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 current revision of the APM is buggy with the spec_ctrl VMCB field.  It is
currently described as a dword, but a correction is pending to change it to
qword like every other MSR.
---
 tools/libs/light/libxl_cpuid.c              | 10 ++++++++++
 tools/misc/xen-cpuid.c                      |  8 +++++++-
 xen/arch/x86/hvm/svm/svm.c                  |  1 +
 xen/arch/x86/hvm/svm/vmcb.c                 |  1 +
 xen/include/asm-x86/cpufeature.h            |  5 +++++
 xen/include/asm-x86/hvm/svm/svm.h           |  2 ++
 xen/include/asm-x86/hvm/svm/vmcb.h          |  4 +++-
 xen/include/asm-x86/msr-index.h             |  3 +++
 xen/include/public/arch-x86/cpufeatureset.h | 10 ++++++++++
 9 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index b2c673841a45..5ed7a87180b8 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -274,8 +274,18 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
         {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
         {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
         {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
+        {"ibrs",         0x80000008, NA, CPUID_REG_EBX, 14,  1},
+        {"amd-stibp",    0x80000008, NA, CPUID_REG_EBX, 15,  1},
+        {"ibrs-always",  0x80000008, NA, CPUID_REG_EBX, 16,  1},
+        {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17,  1},
+        {"ibrs-fast",    0x80000008, NA, CPUID_REG_EBX, 18,  1},
+        {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19,  1},
         {"no-lmsl",      0x80000008, NA, CPUID_REG_EBX, 20,  1},
         {"ppin",         0x80000008, NA, CPUID_REG_EBX, 23,  1},
+        {"amd-ssbd",     0x80000008, NA, CPUID_REG_EBX, 24,  1},
+        {"virt-ssbd",    0x80000008, NA, CPUID_REG_EBX, 25,  1},
+        {"ssb-no",       0x80000008, NA, CPUID_REG_EBX, 26,  1},
+        {"psfd",         0x80000008, NA, CPUID_REG_EBX, 28,  1},
 
         {"nc",           0x80000008, NA, CPUID_REG_ECX,  0,  8},
         {"apicidsize",   0x80000008, NA, CPUID_REG_ECX, 12,  4},
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 735bcf8f0e60..713c1657f4df 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -148,12 +148,18 @@ static const char *const str_e8b[32] =
     [ 0] = "clzero",
     [ 2] = "rstr-fp-err-ptrs",
 
-    /* [ 8] */            [ 9] = "wbnoinvd",
+    /* [ 8] */                 [ 9] = "wbnoinvd",
 
     [12] = "ibpb",
+    [14] = "ibrs",             [15] = "amd-stibp",
+    [16] = "ibrs-always",      [17] = "stibp-always",
+    [18] = "ibrs-fast",        [19] = "ibrs-same-mode",
 
     [20] = "no-lmsl",
     /* [22] */                 [23] = "ppin",
+    [24] = "amd-ssbd",         [25] = "virt-ssbd",
+    [26] = "ssb-no",
+    [28] = "psfd",
 };
 
 static const char *const str_7d0[32] =
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 642a64b747ae..8dc92c8b9f96 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1659,6 +1659,7 @@ const struct hvm_function_table * __init start_svm(void)
     P(cpu_has_pause_thresh, "Pause-Intercept Filter Threshold");
     P(cpu_has_tsc_ratio, "TSC Rate MSR");
     P(cpu_has_svm_sss, "NPT Supervisor Shadow Stack");
+    P(cpu_has_svm_spec_ctrl, "MSR_SPEC_CTRL virtualisation");
 #undef P
 
     if ( !printed )
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 373d5d4af47e..55da9302e5d7 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -271,6 +271,7 @@ static void __init __maybe_unused build_assertions(void)
     BUILD_BUG_ON(offsetof(typeof(vmcb), rsp)                  != 0x5d8);
     BUILD_BUG_ON(offsetof(typeof(vmcb), rax)                  != 0x5f8);
     BUILD_BUG_ON(offsetof(typeof(vmcb), _g_pat)               != 0x668);
+    BUILD_BUG_ON(offsetof(typeof(vmcb), spec_ctrl)            != 0x6e0);
 
     /* Check struct segment_register against the VMCB segment layout. */
     BUILD_BUG_ON(sizeof(vmcb.es)       != 16);
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 5f6b83f71c21..d9604ff58a49 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -128,6 +128,11 @@
 /* CPUID level 0x80000007.edx */
 #define cpu_has_itsc            boot_cpu_has(X86_FEATURE_ITSC)
 
+/* CPUID level 0x80000008.ebx */
+#define cpu_has_amd_ssbd        boot_cpu_has(X86_FEATURE_AMD_SSBD)
+#define cpu_has_virt_ssbd       boot_cpu_has(X86_FEATURE_VIRT_SSBD)
+#define cpu_has_ssb_no          boot_cpu_has(X86_FEATURE_SSB_NO)
+
 /* CPUID level 0x00000007:0.edx */
 #define cpu_has_avx512_4vnniw   boot_cpu_has(X86_FEATURE_AVX512_4VNNIW)
 #define cpu_has_avx512_4fmaps   boot_cpu_has(X86_FEATURE_AVX512_4FMAPS)
diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h
index bee939156f4f..05e968502694 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -76,6 +76,7 @@ extern u32 svm_feature_flags;
 #define SVM_FEATURE_VLOADSAVE     15 /* virtual vmload/vmsave */
 #define SVM_FEATURE_VGIF          16 /* Virtual GIF */
 #define SVM_FEATURE_SSS           19 /* NPT Supervisor Shadow Stacks */
+#define SVM_FEATURE_SPEC_CTRL     20 /* MSR_SPEC_CTRL virtualisation */
 
 #define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f)))
 #define cpu_has_svm_npt       cpu_has_svm_feature(SVM_FEATURE_NPT)
@@ -91,6 +92,7 @@ extern u32 svm_feature_flags;
 #define cpu_has_tsc_ratio     cpu_has_svm_feature(SVM_FEATURE_TSCRATEMSR)
 #define cpu_has_svm_vloadsave cpu_has_svm_feature(SVM_FEATURE_VLOADSAVE)
 #define cpu_has_svm_sss       cpu_has_svm_feature(SVM_FEATURE_SSS)
+#define cpu_has_svm_spec_ctrl cpu_has_svm_feature(SVM_FEATURE_SPEC_CTRL)
 
 #define SVM_PAUSEFILTER_INIT    4000
 #define SVM_PAUSETHRESH_INIT    1000
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index 9e1e42f4941c..4fa2ddfb2ff2 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -521,7 +521,9 @@ struct vmcb_struct {
     u64 _lastbranchtoip;        /* cleanbit 10 */
     u64 _lastintfromip;         /* cleanbit 10 */
     u64 _lastinttoip;           /* cleanbit 10 */
-    u64 res17[301];
+    u64 res17[9];
+    u64 spec_ctrl;
+    u64 res18[291];
 };
 
 struct svm_domain {
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index a14841055f0e..903923e5a58b 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -33,6 +33,7 @@
 #define  SPEC_CTRL_IBRS                     (_AC(1, ULL) <<  0)
 #define  SPEC_CTRL_STIBP                    (_AC(1, ULL) <<  1)
 #define  SPEC_CTRL_SSBD                     (_AC(1, ULL) <<  2)
+#define  SPEC_CTRL_PSFD                     (_AC(1, ULL) <<  7)
 
 #define MSR_PRED_CMD                        0x00000049
 #define  PRED_CMD_IBPB                      (_AC(1, ULL) <<  0)
@@ -137,6 +138,8 @@
 #define  VM_CR_INIT_REDIRECTION             (_AC(1, ULL) <<  1)
 #define  VM_CR_SVM_DISABLE                  (_AC(1, ULL) <<  4)
 
+#define MSR_VIRT_SPEC_CTRL                  0xc001011f /* Layout matches MSR_SPEC_CTRL */
+
 /*
  * Legacy MSR constants in need of cleanup.  No new MSRs below this comment.
  */
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 380b51b1b3b8..f1b072cf44cc 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -255,8 +255,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(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(PSFD,          8*32+28) /*   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 */
-- 
2.11.0

Re: [PATCH 2/3] x86/amd: Enumeration for speculative features/hints

[Jan Beulich]

On 17.08.2021 16:30, Andrew Cooper wrote:
> There is a step change in speculation protections between the Zen1 and Zen2
> microarchitectures.
> 
> Zen1 and older have no special support.  Control bits in non-architectural
> MSRs are used to make lfence be dispatch-serialising (Spectre v1), and to
> disable Memory Disambiguation (Speculative Store Bypass).  IBPB was
> retrofitted in a microcode update, and software methods are required for
> Spectre v2 protections.
> 
> Because the bit controlling Memory Disambiguation is model specific,
> hypervisors are expected to expose a MSR_VIRT_SPEC_CTRL interface which
> abstracts the model specific details.
> 
> Zen2 and later implement the MSR_SPEC_CTRL interface in hardware, and
> virtualise the interface for HVM guests to use.  A number of hint bits are
> specified too to help guide OS software to the most efficient mitigation
> strategy.
> 
> Zen3 introduced a new feature, Predictive Store Forwarding, along with a
> control to disable it in sensitive code.
> 
> Add CPUID and VMCB details for all the new functionality.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -274,8 +274,18 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
>          {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
>          {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
>          {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
> +        {"ibrs",         0x80000008, NA, CPUID_REG_EBX, 14,  1},
> +        {"amd-stibp",    0x80000008, NA, CPUID_REG_EBX, 15,  1},
> +        {"ibrs-always",  0x80000008, NA, CPUID_REG_EBX, 16,  1},
> +        {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17,  1},
> +        {"ibrs-fast",    0x80000008, NA, CPUID_REG_EBX, 18,  1},
> +        {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19,  1},

Here and below, how about dropping the "mode" part of the name?
I can't seem to be able to think of any other "same" that could
possibly apply here.

Jan

Re: [PATCH 2/3] x86/amd: Enumeration for speculative features/hints

[Andrew Cooper]

On 19/08/2021 15:47, Jan Beulich wrote:
> On 17.08.2021 16:30, Andrew Cooper wrote:
>> There is a step change in speculation protections between the Zen1 and Zen2
>> microarchitectures.
>>
>> Zen1 and older have no special support.  Control bits in non-architectural
>> MSRs are used to make lfence be dispatch-serialising (Spectre v1), and to
>> disable Memory Disambiguation (Speculative Store Bypass).  IBPB was
>> retrofitted in a microcode update, and software methods are required for
>> Spectre v2 protections.
>>
>> Because the bit controlling Memory Disambiguation is model specific,
>> hypervisors are expected to expose a MSR_VIRT_SPEC_CTRL interface which
>> abstracts the model specific details.
>>
>> Zen2 and later implement the MSR_SPEC_CTRL interface in hardware, and
>> virtualise the interface for HVM guests to use.  A number of hint bits are
>> specified too to help guide OS software to the most efficient mitigation
>> strategy.
>>
>> Zen3 introduced a new feature, Predictive Store Forwarding, along with a
>> control to disable it in sensitive code.
>>
>> Add CPUID and VMCB details for all the new functionality.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one suggestion:
>
>> --- a/tools/libs/light/libxl_cpuid.c
>> +++ b/tools/libs/light/libxl_cpuid.c
>> @@ -274,8 +274,18 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
>>          {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
>>          {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
>>          {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
>> +        {"ibrs",         0x80000008, NA, CPUID_REG_EBX, 14,  1},
>> +        {"amd-stibp",    0x80000008, NA, CPUID_REG_EBX, 15,  1},
>> +        {"ibrs-always",  0x80000008, NA, CPUID_REG_EBX, 16,  1},
>> +        {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17,  1},
>> +        {"ibrs-fast",    0x80000008, NA, CPUID_REG_EBX, 18,  1},
>> +        {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19,  1},
> Here and below, how about dropping the "mode" part of the name?
> I can't seem to be able to think of any other "same" that could
> possibly apply here.

ibrs-same is very ambiguous.  The only other reasonable reasonable
alternative I can think of is ibrs-psmp as an abbreviation for
ProvideSameModeProtection.  Obviously, the "Provides" bit of that can't
be dropped.

~Andrew

Re: [PATCH 2/3] x86/amd: Enumeration for speculative features/hints

[Jan Beulich]

On 24.08.2021 15:26, Andrew Cooper wrote:
> On 19/08/2021 15:47, Jan Beulich wrote:
>> On 17.08.2021 16:30, Andrew Cooper wrote:
>>> There is a step change in speculation protections between the Zen1 and Zen2
>>> microarchitectures.
>>>
>>> Zen1 and older have no special support.  Control bits in non-architectural
>>> MSRs are used to make lfence be dispatch-serialising (Spectre v1), and to
>>> disable Memory Disambiguation (Speculative Store Bypass).  IBPB was
>>> retrofitted in a microcode update, and software methods are required for
>>> Spectre v2 protections.
>>>
>>> Because the bit controlling Memory Disambiguation is model specific,
>>> hypervisors are expected to expose a MSR_VIRT_SPEC_CTRL interface which
>>> abstracts the model specific details.
>>>
>>> Zen2 and later implement the MSR_SPEC_CTRL interface in hardware, and
>>> virtualise the interface for HVM guests to use.  A number of hint bits are
>>> specified too to help guide OS software to the most efficient mitigation
>>> strategy.
>>>
>>> Zen3 introduced a new feature, Predictive Store Forwarding, along with a
>>> control to disable it in sensitive code.
>>>
>>> Add CPUID and VMCB details for all the new functionality.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with one suggestion:
>>
>>> --- a/tools/libs/light/libxl_cpuid.c
>>> +++ b/tools/libs/light/libxl_cpuid.c
>>> @@ -274,8 +274,18 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
>>>          {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
>>>          {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
>>>          {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
>>> +        {"ibrs",         0x80000008, NA, CPUID_REG_EBX, 14,  1},
>>> +        {"amd-stibp",    0x80000008, NA, CPUID_REG_EBX, 15,  1},
>>> +        {"ibrs-always",  0x80000008, NA, CPUID_REG_EBX, 16,  1},
>>> +        {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17,  1},
>>> +        {"ibrs-fast",    0x80000008, NA, CPUID_REG_EBX, 18,  1},
>>> +        {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19,  1},
>> Here and below, how about dropping the "mode" part of the name?
>> I can't seem to be able to think of any other "same" that could
>> possibly apply here.
> 
> ibrs-same is very ambiguous.

I'm curious as to why you think so.

>  The only other reasonable reasonable
> alternative I can think of is ibrs-psmp as an abbreviation for
> ProvideSameModeProtection.  Obviously, the "Provides" bit of that can't
> be dropped.

Then better stay with what you have I would say - for me "psmp"
immediately raises the question "What strange kind of SMP?"
While not tied to any formal naming, I could see "ibrs-sm" as
an option ...

Jan

Re: [PATCH 2/3] x86/amd: Enumeration for speculative features/hints

[Andrew Cooper]

On 24/08/2021 16:15, Jan Beulich wrote:
> On 24.08.2021 15:26, Andrew Cooper wrote:
>> On 19/08/2021 15:47, Jan Beulich wrote:
>>> On 17.08.2021 16:30, Andrew Cooper wrote:
>>>> There is a step change in speculation protections between the Zen1 and Zen2
>>>> microarchitectures.
>>>>
>>>> Zen1 and older have no special support.  Control bits in non-architectural
>>>> MSRs are used to make lfence be dispatch-serialising (Spectre v1), and to
>>>> disable Memory Disambiguation (Speculative Store Bypass).  IBPB was
>>>> retrofitted in a microcode update, and software methods are required for
>>>> Spectre v2 protections.
>>>>
>>>> Because the bit controlling Memory Disambiguation is model specific,
>>>> hypervisors are expected to expose a MSR_VIRT_SPEC_CTRL interface which
>>>> abstracts the model specific details.
>>>>
>>>> Zen2 and later implement the MSR_SPEC_CTRL interface in hardware, and
>>>> virtualise the interface for HVM guests to use.  A number of hint bits are
>>>> specified too to help guide OS software to the most efficient mitigation
>>>> strategy.
>>>>
>>>> Zen3 introduced a new feature, Predictive Store Forwarding, along with a
>>>> control to disable it in sensitive code.
>>>>
>>>> Add CPUID and VMCB details for all the new functionality.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> with one suggestion:
>>>
>>>> --- a/tools/libs/light/libxl_cpuid.c
>>>> +++ b/tools/libs/light/libxl_cpuid.c
>>>> @@ -274,8 +274,18 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
>>>>          {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1},
>>>>          {"wbnoinvd",     0x80000008, NA, CPUID_REG_EBX,  9,  1},
>>>>          {"ibpb",         0x80000008, NA, CPUID_REG_EBX, 12,  1},
>>>> +        {"ibrs",         0x80000008, NA, CPUID_REG_EBX, 14,  1},
>>>> +        {"amd-stibp",    0x80000008, NA, CPUID_REG_EBX, 15,  1},
>>>> +        {"ibrs-always",  0x80000008, NA, CPUID_REG_EBX, 16,  1},
>>>> +        {"stibp-always", 0x80000008, NA, CPUID_REG_EBX, 17,  1},
>>>> +        {"ibrs-fast",    0x80000008, NA, CPUID_REG_EBX, 18,  1},
>>>> +        {"ibrs-same-mode", 0x80000008, NA, CPUID_REG_EBX, 19,  1},
>>> Here and below, how about dropping the "mode" part of the name?
>>> I can't seem to be able to think of any other "same" that could
>>> possibly apply here.
>> ibrs-same is very ambiguous.
> I'm curious as to why you think so.

Same what?  There are load of plausible options, e.g. "privilege".

"mode" is the second most important piece of info, behind ibrs.

>
>>   The only other reasonable reasonable
>> alternative I can think of is ibrs-psmp as an abbreviation for
>> ProvideSameModeProtection.  Obviously, the "Provides" bit of that can't
>> be dropped.
> Then better stay with what you have I would say - for me "psmp"
> immediately raises the question "What strange kind of SMP?"
> While not tied to any formal naming, I could see "ibrs-sm" as
> an option ...

That's an initialisation of a shortening, and too far removed from the
original context IMO.

Given nothing better, I'll stick with ibrs-same-mode.

~Andrew

[PATCH 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Andrew Cooper]

The opencoded legacy Memory Disambiguation logic in init_amd() neglected
Fam19h for the Zen3 microarchitecture.

In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h
Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it.

Implement the algorithm given in AMD's SSBD whitepaper, and leave a
printk_once() behind in the case that no controls can be found.

This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off
Memory Disambiguation on Fam19h/Zen3 systems.

This still remains a single system-wide setting (for now), and is not context
switched between vCPUs.  As such, it doesn't interact with Intel's use of
MSR_SPEC_CTRL and default_xen_spec_ctrl (yet).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/amd.c   | 69 +++++++++++++++++++++++++++++++++++-------------
 xen/arch/x86/cpu/cpu.h   |  1 +
 xen/arch/x86/cpu/hygon.c | 10 +------
 xen/arch/x86/spec_ctrl.c |  5 +++-
 4 files changed, 57 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 2260eef3aab5..567565199373 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -681,6 +681,56 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
 			  c->x86_capability);
 }
 
+/*
+ * Refer to the AMD Speculative Store Bypass whitepaper:
+ * https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
+ */
+void amd_init_ssbd(const struct cpuinfo_x86 *c)
+{
+	int bit = -1;
+
+	if (cpu_has_ssb_no)
+		return;
+
+	if (cpu_has_amd_ssbd) {
+		wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		return;
+	}
+
+	if (cpu_has_virt_ssbd) {
+		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		return;
+	}
+
+	switch (c->x86) {
+	case 0x15: bit = 54; break;
+	case 0x16: bit = 33; break;
+	case 0x17:
+	case 0x18: bit = 10; break;
+	}
+
+	if (bit >= 0) {
+		uint64_t val, mask = 1ull << bit;
+
+		if (rdmsr_safe(MSR_AMD64_LS_CFG, val) ||
+		    ({
+			    val &= ~mask;
+			    if ( opt_ssbd )
+				    val |= mask;
+			    false;
+		    }) ||
+		    wrmsr_safe(MSR_AMD64_LS_CFG, val) ||
+		    ({
+			    rdmsrl(MSR_AMD64_LS_CFG, val);
+			    (val & mask) != (opt_ssbd * mask);
+		    }))
+			bit = -1;
+	}
+
+	if (bit < 0)
+		printk_once(XENLOG_ERR "No SSBD controls available\n");
+}
+
 static void init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -731,24 +781,7 @@ static void init_amd(struct cpuinfo_x86 *c)
 	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
 		amd_init_lfence(c);
 
-	/*
-	 * If the user has explicitly chosen to disable Memory Disambiguation
-	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
-	 */
-	if (opt_ssbd) {
-		int bit = -1;
-
-		switch (c->x86) {
-		case 0x15: bit = 54; break;
-		case 0x16: bit = 33; break;
-		case 0x17: bit = 10; break;
-		}
-
-		if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
-			value |= 1ull << bit;
-			wrmsr_safe(MSR_AMD64_LS_CFG, value);
-		}
-	}
+	amd_init_ssbd(c);
 
 	/* MFENCE stops RDTSC speculation */
 	if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 1ac3b2867a04..1a5b3918b37e 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c);
 void early_init_amd(struct cpuinfo_x86 *c);
 void amd_log_freq(const struct cpuinfo_x86 *c);
 void amd_init_lfence(struct cpuinfo_x86 *c);
+void amd_init_ssbd(const struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
index 67e23c5df9e3..56792146739e 100644
--- a/xen/arch/x86/cpu/hygon.c
+++ b/xen/arch/x86/cpu/hygon.c
@@ -33,15 +33,7 @@ static void init_hygon(struct cpuinfo_x86 *c)
 	unsigned long long value;
 
 	amd_init_lfence(c);
-
-	/*
-	 * If the user has explicitly chosen to disable Memory Disambiguation
-	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
-	 */
-	if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
-		value |= 1ull << 10;
-		wrmsr_safe(MSR_AMD64_LS_CFG, value);
-	}
+	amd_init_ssbd(c);
 
 	/* MFENCE stops RDTSC speculation */
 	if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 9bf0fbf99813..0850afc09358 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -326,20 +326,23 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",
            (caps & ARCH_CAPS_RSBA)                           ? " RSBA"           : "",
            (caps & ARCH_CAPS_SKIP_L1DFL)                     ? " SKIP_L1DFL"     : "",
+           (e8b  & cpufeat_mask(X86_FEATURE_SSB_NO)) ||
            (caps & ARCH_CAPS_SSB_NO)                         ? " SSB_NO"         : "",
            (caps & ARCH_CAPS_MDS_NO)                         ? " MDS_NO"         : "",
            (caps & ARCH_CAPS_TAA_NO)                         ? " TAA_NO"         : "");
 
     /* Hardware features which need driving to mitigate issues. */
-    printk("  Hardware features:%s%s%s%s%s%s%s%s\n",
+    printk("  Hardware features:%s%s%s%s%s%s%s%s%s\n",
            (e8b  & cpufeat_mask(X86_FEATURE_IBPB)) ||
            (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB))          ? " IBPB"           : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB))          ? " IBRS"           : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_STIBP))          ? " STIBP"          : "",
+           (e8b  & cpufeat_mask(X86_FEATURE_AMD_SSBD)) ||
            (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))           ? " SSBD"           : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH))      ? " L1D_FLUSH"      : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR))       ? " MD_CLEAR"       : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL))     ? " SRBDS_CTRL"     : "",
+           (e8b  & cpufeat_mask(X86_FEATURE_VIRT_SSBD))      ? " VIRT_SSBD"      : "",
            (caps & ARCH_CAPS_TSX_CTRL)                       ? " TSX_CTRL"       : "");
 
     /* Compiled-in support which pertains to mitigations. */
-- 
2.11.0

Re: [PATCH 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Jan Beulich]

On 17.08.2021 16:30, Andrew Cooper wrote:
> The opencoded legacy Memory Disambiguation logic in init_amd() neglected
> Fam19h for the Zen3 microarchitecture.
> 
> In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h
> Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it.
> 
> Implement the algorithm given in AMD's SSBD whitepaper, and leave a
> printk_once() behind in the case that no controls can be found.
> 
> This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off
> Memory Disambiguation on Fam19h/Zen3 systems.

Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be
to turn _on_ Memory Disambiguation, unless the original comment was
the wrong way round? I'm also concerned by this behavioral change:
I think opt_ssbd would want to become a tristate, such that not
specifying the option at all will not also result in turning the bit
off even if it was on for some reason (firmware?). Similarly
"spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -681,6 +681,56 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>  			  c->x86_capability);
>  }
>  
> +/*
> + * Refer to the AMD Speculative Store Bypass whitepaper:
> + * https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
> + */
> +void amd_init_ssbd(const struct cpuinfo_x86 *c)
> +{
> +	int bit = -1;
> +
> +	if (cpu_has_ssb_no)
> +		return;
> +
> +	if (cpu_has_amd_ssbd) {
> +		wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> +		return;
> +	}
> +
> +	if (cpu_has_virt_ssbd) {
> +		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> +		return;
> +	}
> +
> +	switch (c->x86) {
> +	case 0x15: bit = 54; break;
> +	case 0x16: bit = 33; break;
> +	case 0x17:
> +	case 0x18: bit = 10; break;
> +	}
> +
> +	if (bit >= 0) {
> +		uint64_t val, mask = 1ull << bit;
> +
> +		if (rdmsr_safe(MSR_AMD64_LS_CFG, val) ||
> +		    ({
> +			    val &= ~mask;
> +			    if ( opt_ssbd )

Nit: No spaces inside the parentheses here.

Jan

Re: [PATCH 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Andrew Cooper]

On 19/08/2021 15:59, Jan Beulich wrote:
> On 17.08.2021 16:30, Andrew Cooper wrote:
>> The opencoded legacy Memory Disambiguation logic in init_amd() neglected
>> Fam19h for the Zen3 microarchitecture.
>>
>> In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h
>> Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it.
>>
>> Implement the algorithm given in AMD's SSBD whitepaper, and leave a
>> printk_once() behind in the case that no controls can be found.
>>
>> This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off
>> Memory Disambiguation on Fam19h/Zen3 systems.
> Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be
> to turn _on_ Memory Disambiguation, unless the original comment was
> the wrong way round? I'm also concerned by this behavioral change:
> I think opt_ssbd would want to become a tristate, such that not
> specifying the option at all will not also result in turning the bit
> off even if it was on for some reason (firmware?). Similarly
> "spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect.

I messed that bit of the description up.  I means `spec-ctrl=ssb`, i.e.
the non-default value.

We do not disable Memory Disambiguation (the speculative feature which
causes the Speculative Store Bypass vulnerability) by default (due to
the perf hit), but if the user explicitly asks for it using the
available command line option, nothing currently happens on Fam19h.

~Andrew

Re: [PATCH 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Jan Beulich]

On 24.08.2021 15:39, Andrew Cooper wrote:
> On 19/08/2021 15:59, Jan Beulich wrote:
>> On 17.08.2021 16:30, Andrew Cooper wrote:
>>> The opencoded legacy Memory Disambiguation logic in init_amd() neglected
>>> Fam19h for the Zen3 microarchitecture.
>>>
>>> In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h
>>> Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it.
>>>
>>> Implement the algorithm given in AMD's SSBD whitepaper, and leave a
>>> printk_once() behind in the case that no controls can be found.
>>>
>>> This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off
>>> Memory Disambiguation on Fam19h/Zen3 systems.
>> Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be
>> to turn _on_ Memory Disambiguation, unless the original comment was
>> the wrong way round? I'm also concerned by this behavioral change:
>> I think opt_ssbd would want to become a tristate, such that not
>> specifying the option at all will not also result in turning the bit
>> off even if it was on for some reason (firmware?). Similarly
>> "spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect.
> 
> I messed that bit of the description up.  I means `spec-ctrl=ssb`, i.e.
> the non-default value.
> 
> We do not disable Memory Disambiguation (the speculative feature which
> causes the Speculative Store Bypass vulnerability) by default (due to
> the perf hit), but if the user explicitly asks for it using the
> available command line option, nothing currently happens on Fam19h.

Oh, I see. Yet (nit) then still "spec-ctrl=ssbd".

Jan

[PATCH v2 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Andrew Cooper]

The opencoded legacy Memory Disambiguation logic in init_amd() neglected
Fam19h for the Zen3 microarchitecture.  In practice, all Zen2 based system
have the architectural MSR_SPEC_CTRL and the SSBD bit within it.

Implement the algorithm given in AMD's SSBD whitepaper, and leave a
printk_once() behind in the case that no controls can be found.

This now means that a user explicitly choosing `spec-ctrl=ssbd` will properly
turn off Memory Disambiguation on Fam19h/Zen3 systems.

This still remains a single system-wide setting (for now), and is not context
switched between vCPUs.  As such, it doesn't interact with Intel's use of
MSR_SPEC_CTRL and default_xen_spec_ctrl (yet).

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

v2:
 * Fix whitespace style in amd_init_ssbd()
 * Rewrite commit message with the correct command line syntax
---
 xen/arch/x86/cpu/amd.c   | 69 +++++++++++++++++++++++++++++++++++-------------
 xen/arch/x86/cpu/cpu.h   |  1 +
 xen/arch/x86/cpu/hygon.c | 10 +------
 xen/arch/x86/spec_ctrl.c |  5 +++-
 4 files changed, 57 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 2260eef3aab5..3f6a8e6aa3b9 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -681,6 +681,56 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
 			  c->x86_capability);
 }
 
+/*
+ * Refer to the AMD Speculative Store Bypass whitepaper:
+ * https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
+ */
+void amd_init_ssbd(const struct cpuinfo_x86 *c)
+{
+	int bit = -1;
+
+	if (cpu_has_ssb_no)
+		return;
+
+	if (cpu_has_amd_ssbd) {
+		wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		return;
+	}
+
+	if (cpu_has_virt_ssbd) {
+		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		return;
+	}
+
+	switch (c->x86) {
+	case 0x15: bit = 54; break;
+	case 0x16: bit = 33; break;
+	case 0x17:
+	case 0x18: bit = 10; break;
+	}
+
+	if (bit >= 0) {
+		uint64_t val, mask = 1ull << bit;
+
+		if (rdmsr_safe(MSR_AMD64_LS_CFG, val) ||
+		    ({
+			    val &= ~mask;
+			    if (opt_ssbd)
+				    val |= mask;
+			    false;
+		    }) ||
+		    wrmsr_safe(MSR_AMD64_LS_CFG, val) ||
+		    ({
+			    rdmsrl(MSR_AMD64_LS_CFG, val);
+			    (val & mask) != (opt_ssbd * mask);
+		    }))
+			bit = -1;
+	}
+
+	if (bit < 0)
+		printk_once(XENLOG_ERR "No SSBD controls available\n");
+}
+
 static void init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -731,24 +781,7 @@ static void init_amd(struct cpuinfo_x86 *c)
 	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
 		amd_init_lfence(c);
 
-	/*
-	 * If the user has explicitly chosen to disable Memory Disambiguation
-	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
-	 */
-	if (opt_ssbd) {
-		int bit = -1;
-
-		switch (c->x86) {
-		case 0x15: bit = 54; break;
-		case 0x16: bit = 33; break;
-		case 0x17: bit = 10; break;
-		}
-
-		if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
-			value |= 1ull << bit;
-			wrmsr_safe(MSR_AMD64_LS_CFG, value);
-		}
-	}
+	amd_init_ssbd(c);
 
 	/* MFENCE stops RDTSC speculation */
 	if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 1ac3b2867a04..1a5b3918b37e 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c);
 void early_init_amd(struct cpuinfo_x86 *c);
 void amd_log_freq(const struct cpuinfo_x86 *c);
 void amd_init_lfence(struct cpuinfo_x86 *c);
+void amd_init_ssbd(const struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
index 67e23c5df9e3..56792146739e 100644
--- a/xen/arch/x86/cpu/hygon.c
+++ b/xen/arch/x86/cpu/hygon.c
@@ -33,15 +33,7 @@ static void init_hygon(struct cpuinfo_x86 *c)
 	unsigned long long value;
 
 	amd_init_lfence(c);
-
-	/*
-	 * If the user has explicitly chosen to disable Memory Disambiguation
-	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
-	 */
-	if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
-		value |= 1ull << 10;
-		wrmsr_safe(MSR_AMD64_LS_CFG, value);
-	}
+	amd_init_ssbd(c);
 
 	/* MFENCE stops RDTSC speculation */
 	if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index c310a7f6ac96..f0c67d41b85f 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -326,20 +326,23 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            (caps & ARCH_CAPS_IBRS_ALL)                       ? " IBRS_ALL"       : "",
            (caps & ARCH_CAPS_RSBA)                           ? " RSBA"           : "",
            (caps & ARCH_CAPS_SKIP_L1DFL)                     ? " SKIP_L1DFL"     : "",
+           (e8b  & cpufeat_mask(X86_FEATURE_SSB_NO)) ||
            (caps & ARCH_CAPS_SSB_NO)                         ? " SSB_NO"         : "",
            (caps & ARCH_CAPS_MDS_NO)                         ? " MDS_NO"         : "",
            (caps & ARCH_CAPS_TAA_NO)                         ? " TAA_NO"         : "");
 
     /* Hardware features which need driving to mitigate issues. */
-    printk("  Hardware features:%s%s%s%s%s%s%s%s\n",
+    printk("  Hardware features:%s%s%s%s%s%s%s%s%s\n",
            (e8b  & cpufeat_mask(X86_FEATURE_IBPB)) ||
            (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB))          ? " IBPB"           : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB))          ? " IBRS"           : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_STIBP))          ? " STIBP"          : "",
+           (e8b  & cpufeat_mask(X86_FEATURE_AMD_SSBD)) ||
            (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))           ? " SSBD"           : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH))      ? " L1D_FLUSH"      : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR))       ? " MD_CLEAR"       : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL))     ? " SRBDS_CTRL"     : "",
+           (e8b  & cpufeat_mask(X86_FEATURE_VIRT_SSBD))      ? " VIRT_SSBD"      : "",
            (caps & ARCH_CAPS_TSX_CTRL)                       ? " TSX_CTRL"       : "");
 
     /* Compiled-in support which pertains to mitigations. */
-- 
2.11.0

Re: [PATCH v2 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Jan Beulich]

On 07.09.2021 18:19, Andrew Cooper wrote:
> The opencoded legacy Memory Disambiguation logic in init_amd() neglected
> Fam19h for the Zen3 microarchitecture.  In practice, all Zen2 based system
> have the architectural MSR_SPEC_CTRL and the SSBD bit within it.

Don't you mean Zen3 in the 2nd sentence? Otherwise there's a missing
connect between both sentences.

> Implement the algorithm given in AMD's SSBD whitepaper, and leave a
> printk_once() behind in the case that no controls can be found.
> 
> This now means that a user explicitly choosing `spec-ctrl=ssbd` will properly
> turn off Memory Disambiguation on Fam19h/Zen3 systems.
> 
> This still remains a single system-wide setting (for now), and is not context
> switched between vCPUs.  As such, it doesn't interact with Intel's use of
> MSR_SPEC_CTRL and default_xen_spec_ctrl (yet).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Jan

Re: [PATCH v2 3/3] x86/amd: Use newer SSBD mechanisms if they exist

[Andrew Cooper]

On 08/09/2021 11:43, Jan Beulich wrote:
> On 07.09.2021 18:19, Andrew Cooper wrote:
>> The opencoded legacy Memory Disambiguation logic in init_amd() neglected
>> Fam19h for the Zen3 microarchitecture.  In practice, all Zen2 based system
>> have the architectural MSR_SPEC_CTRL and the SSBD bit within it.
> Don't you mean Zen3 in the 2nd sentence? Otherwise there's a missing
> connect between both sentences.

No.  Zen2/Rome has MSR_SPEC_CTRL.

The point is that Zen2 and later shouldn't be using MSR_AMD64_LS_CFG in
the first place.  I'll tweak the wording.

>
>> Implement the algorithm given in AMD's SSBD whitepaper, and leave a
>> printk_once() behind in the case that no controls can be found.
>>
>> This now means that a user explicitly choosing `spec-ctrl=ssbd` will properly
>> turn off Memory Disambiguation on Fam19h/Zen3 systems.
>>
>> This still remains a single system-wide setting (for now), and is not context
>> switched between vCPUs.  As such, it doesn't interact with Intel's use of
>> MSR_SPEC_CTRL and default_xen_spec_ctrl (yet).
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew