[PATCH] x86/tsx: Cope with TSX deprecation on SKL/KBL/CFL/WHL

Andrew Cooper posted 1 patch 2 years, 9 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210608170559.6732-1-andrew.cooper3@citrix.com
docs/misc/xen-command-line.pandoc           | 13 +++++
tools/misc/xen-cpuid.c                      |  2 +-
xen/arch/x86/tsx.c                          | 76 +++++++++++++++++++++++++++--
xen/include/asm-x86/cpufeature.h            |  1 +
xen/include/asm-x86/msr-index.h             |  2 +
xen/include/public/arch-x86/cpufeatureset.h |  1 +
6 files changed, 89 insertions(+), 6 deletions(-)
[PATCH] x86/tsx: Cope with TSX deprecation on SKL/KBL/CFL/WHL
Posted by Andrew Cooper 2 years, 9 months ago
The June 2021 microcode is formally de-featuring TSX on the older Skylake
client CPUs.  The workaround from the March 2019 microcode is being dropped,
and replaced with additions to MSR_TSX_FORCE_ABORT to hide the HLE/RTM CPUID
bits.

With this microcode in place, TSX is disabled by default on these CPUs.
Backwards compatibility is provided in the same way as for TAA - RTM force
aborts, rather than suffering #UD, and the CPUID bits can be hidden to recover
performance.

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>
---
 docs/misc/xen-command-line.pandoc           | 13 +++++
 tools/misc/xen-cpuid.c                      |  2 +-
 xen/arch/x86/tsx.c                          | 76 +++++++++++++++++++++++++++--
 xen/include/asm-x86/cpufeature.h            |  1 +
 xen/include/asm-x86/msr-index.h             |  2 +
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 6 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 1fae872626..3ece83a427 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2309,6 +2309,12 @@ Several microcode updates are relevant:
    and formally retiring HLE from the architecture.  The user can disable TSX
    to mitigate TAA, and elect to hide the HLE/RTM CPUID bits.
 
+ * June 2021, removing the workaround for March 2019 on client CPUs and
+   formally de-featured TSX on SKL/KBL/WHL/CFL (Note: SKX still retains the
+   March 2019 fix).  Introduced the ability to hide the HLE/RTM CPUID bits.
+   PCR3 works fine, and TSX is disabled by default, but the user can re-enable
+   TSX at their own risk, accepting that the memory order erratum is unfixed.
+
 On systems with the ability to configure TSX, this boolean offers system wide
 control of whether TSX is enabled or disabled.
 
@@ -2326,6 +2332,13 @@ control of whether TSX is enabled or disabled.
    ordering errata default to `true` to enable working TSX.  Alternatively,
    selecting `tsx=0` will disable TSX and restore PCR3 to a working state.
 
+   SKX and SKL/KBL/WHL/CFL on pre-June 2021 microcode default to `true`.
+   Alternatively, selecting `tsx=0` will disable TSX and restore PCR3 to a
+   working state.
+
+   SKL/KBL/WHL/CFL on the June 2021 microcode or later default to `false`.
+   Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
+
 ### ucode
 > `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
 
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index d4bc83d8c9..735bcf8f0e 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -162,7 +162,7 @@ static const char *const str_7d0[32] =
     [ 4] = "fsrm",
 
     [ 8] = "avx512-vp2intersect", [ 9] = "srbds-ctrl",
-    [10] = "md-clear",
+    [10] = "md-clear",            [11] = "rtm-always-abort",
     /* 12 */                [13] = "tsx-force-abort",
     [14] = "serialize",
 
diff --git a/xen/arch/x86/tsx.c b/xen/arch/x86/tsx.c
index 338191df7f..085f1c82a7 100644
--- a/xen/arch/x86/tsx.c
+++ b/xen/arch/x86/tsx.c
@@ -60,6 +60,38 @@ void tsx_init(void)
              */
 
             /*
+             * Probe for the June 2021 microcode which de-features TSX on
+             * client parts.  (Note - this is a subset of parts impacted by
+             * the memory ordering errata.)
+             *
+             * RTM_ALWAYS_ABORT enumerates the new functionality, but is also
+             * read as zero if TSX_FORCE_ABORT.ENABLE_RTM has been set before
+             * we run.
+             *
+             * Undo this behaviour in Xen's view of the world.
+             */
+            bool has_rtm_always_abort = cpu_has_rtm_always_abort;
+
+            if ( !has_rtm_always_abort )
+            {
+                uint64_t val;
+
+                rdmsrl(MSR_TSX_FORCE_ABORT, val);
+
+                if ( val & TSX_ENABLE_RTM )
+                    has_rtm_always_abort = true;
+            }
+
+            /*
+             * Always force RTM_ALWAYS_ABORT to be visibile, even if it
+             * currently is.  If the user explicitly opts to enable TSX, we'll
+             * set TSX_FORCE_ABORT.ENABLE_RTM and hide RTM_ALWAYS_ABORT from
+             * the general CPUID scan later.
+             */
+            if ( has_rtm_always_abort )
+                setup_force_cpu_cap(X86_FEATURE_RTM_ALWAYS_ABORT);
+
+            /*
              * If no explicit tsx= option is provided, pick a default.
              *
              * This deliberately overrides the implicit opt_tsx=-3 from
@@ -67,9 +99,16 @@ void tsx_init(void)
              * - parse_spec_ctrl() ran before any CPU details where know.
              * - We now know we're running on a CPU not affected by TAA (as
              *   TSX_FORCE_ABORT is enumerated).
+             * - When RTM_ALWAYS_ABORT is enumerated, TSX malfunctions, so we
+             *   only ever want it enabled by explicit user choice.
+             *
+             * Without RTM_ALWAYS_ABORT, leave TSX active.  In particular,
+             * this includes SKX where TSX is still supported.
+             *
+             * With RTM_ALWAYS_ABORT, disable TSX.
              */
             if ( opt_tsx < 0 )
-                opt_tsx = 1;
+                opt_tsx = !cpu_has_rtm_always_abort;
         }
 
         /*
@@ -90,7 +129,7 @@ void tsx_init(void)
          * Force the features to be visible in Xen's view if we see any of the
          * infrastructure capable of hiding them.
          */
-        if ( cpu_has_tsx_ctrl )
+        if ( cpu_has_tsx_ctrl || cpu_has_tsx_force_abort )
         {
             setup_force_cpu_cap(X86_FEATURE_HLE);
             setup_force_cpu_cap(X86_FEATURE_RTM);
@@ -131,9 +170,36 @@ void tsx_init(void)
         /* Check bottom bit only.  Higher bits are various sentinels. */
         rtm_disabled = !(opt_tsx & 1);
 
-        lo &= ~TSX_FORCE_ABORT_RTM;
-        if ( rtm_disabled )
-            lo |= TSX_FORCE_ABORT_RTM;
+        lo &= ~(TSX_FORCE_ABORT_RTM | TSX_CPUID_CLEAR | TSX_ENABLE_RTM);
+
+        if ( cpu_has_rtm_always_abort )
+        {
+            /*
+             * June 2021 microcode, on a client part with TSX de-featured:
+             *  - There are no mitigations for the TSX memory ordering errata.
+             *  - Performance counter 3 works.  (I.e. it isn't being used by
+             *    microcode to work around the memory ordering errata.)
+             *  - TSX_FORCE_ABORT.FORCE_ABORT_RTM is fixed read1/write-discard.
+             *  - TSX_FORCE_ABORT.TSX_CPUID_CLEAR can be used to hide the
+             *    HLE/RTM CPUID bits.
+             *  - TSX_FORCE_ABORT.ENABLE_RTM may be used to opt in to
+             *    re-enabling RTM, at the users own risk.
+             */
+            lo |= rtm_disabled ? TSX_CPUID_CLEAR : TSX_ENABLE_RTM;
+        }
+        else
+        {
+            /*
+             * Either a server part where TSX isn't de-featured, or pre-June
+             * 2021 microcode:
+             *  - By default, the TSX memory ordering errata is worked around
+             *    in microcode at the cost of Performance Counter 3.
+             *  - "Working TSX" vs "Working PCR3" can be selected by way of
+             *    setting TSX_FORCE_ABORT.FORCE_ABORT_RTM.
+             */
+            if ( rtm_disabled )
+                lo |= TSX_FORCE_ABORT_RTM;
+        }
 
         wrmsr(MSR_TSX_FORCE_ABORT, lo, hi);
     }
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 9f5ae3aa0d..a539a4bacd 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -131,6 +131,7 @@
 #define cpu_has_avx512_4vnniw   boot_cpu_has(X86_FEATURE_AVX512_4VNNIW)
 #define cpu_has_avx512_4fmaps   boot_cpu_has(X86_FEATURE_AVX512_4FMAPS)
 #define cpu_has_avx512_vp2intersect boot_cpu_has(X86_FEATURE_AVX512_VP2INTERSECT)
+#define cpu_has_rtm_always_abort boot_cpu_has(X86_FEATURE_RTM_ALWAYS_ABORT)
 #define cpu_has_tsx_force_abort boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)
 #define cpu_has_serialize       boot_cpu_has(X86_FEATURE_SERIALIZE)
 #define cpu_has_arch_caps       boot_cpu_has(X86_FEATURE_ARCH_CAPS)
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index bd3a3a1e7f..9a772c12b8 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -61,6 +61,8 @@
 
 #define MSR_TSX_FORCE_ABORT                 0x0000010f
 #define  TSX_FORCE_ABORT_RTM                (_AC(1, ULL) <<  0)
+#define  TSX_CPUID_CLEAR                    (_AC(1, ULL) <<  1)
+#define  TSX_ENABLE_RTM                     (_AC(1, ULL) <<  2)
 
 #define MSR_TSX_CTRL                        0x00000122
 #define  TSX_CTRL_RTM_DISABLE               (_AC(1, ULL) <<  0)
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index b65af42436..380b51b1b3 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -265,6 +265,7 @@ XEN_CPUFEATURE(FSRM,          9*32+ 4) /*A  Fast Short REP MOVS */
 XEN_CPUFEATURE(AVX512_VP2INTERSECT, 9*32+8) /*a  VP2INTERSECT{D,Q} insns */
 XEN_CPUFEATURE(SRBDS_CTRL,    9*32+ 9) /*   MSR_MCU_OPT_CTRL and RNGDS_MITG_DIS. */
 XEN_CPUFEATURE(MD_CLEAR,      9*32+10) /*A  VERW clears microarchitectural buffers */
+XEN_CPUFEATURE(RTM_ALWAYS_ABORT, 9*32+11) /*! June 2021 TSX defeaturing in microcode. */
 XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */
 XEN_CPUFEATURE(SERIALIZE,     9*32+14) /*a  SERIALIZE insn */
 XEN_CPUFEATURE(CET_IBT,       9*32+20) /*   CET - Indirect Branch Tracking */
-- 
2.11.0


Re: [PATCH] x86/tsx: Cope with TSX deprecation on SKL/KBL/CFL/WHL
Posted by Jan Beulich 2 years, 9 months ago
On 08.06.2021 19:05, Andrew Cooper wrote:
> --- a/xen/arch/x86/tsx.c
> +++ b/xen/arch/x86/tsx.c
> @@ -60,6 +60,38 @@ void tsx_init(void)
>               */
>  
>              /*
> +             * Probe for the June 2021 microcode which de-features TSX on
> +             * client parts.  (Note - this is a subset of parts impacted by
> +             * the memory ordering errata.)
> +             *
> +             * RTM_ALWAYS_ABORT enumerates the new functionality, but is also
> +             * read as zero if TSX_FORCE_ABORT.ENABLE_RTM has been set before
> +             * we run.
> +             *
> +             * Undo this behaviour in Xen's view of the world.
> +             */
> +            bool has_rtm_always_abort = cpu_has_rtm_always_abort;
> +
> +            if ( !has_rtm_always_abort )
> +            {
> +                uint64_t val;
> +
> +                rdmsrl(MSR_TSX_FORCE_ABORT, val);
> +
> +                if ( val & TSX_ENABLE_RTM )
> +                    has_rtm_always_abort = true;
> +            }
> +
> +            /*
> +             * Always force RTM_ALWAYS_ABORT to be visibile, even if it
> +             * currently is.  If the user explicitly opts to enable TSX, we'll
> +             * set TSX_FORCE_ABORT.ENABLE_RTM and hide RTM_ALWAYS_ABORT from
> +             * the general CPUID scan later.
> +             */
> +            if ( has_rtm_always_abort )
> +                setup_force_cpu_cap(X86_FEATURE_RTM_ALWAYS_ABORT);

I understand the "we'll set" part, but I don't think "we'll hide"
anything explicitly. Aiui it is ...

> @@ -131,9 +170,36 @@ void tsx_init(void)
>          /* Check bottom bit only.  Higher bits are various sentinels. */
>          rtm_disabled = !(opt_tsx & 1);
>  
> -        lo &= ~TSX_FORCE_ABORT_RTM;
> -        if ( rtm_disabled )
> -            lo |= TSX_FORCE_ABORT_RTM;
> +        lo &= ~(TSX_FORCE_ABORT_RTM | TSX_CPUID_CLEAR | TSX_ENABLE_RTM);
> +
> +        if ( cpu_has_rtm_always_abort )
> +        {
> +            /*
> +             * June 2021 microcode, on a client part with TSX de-featured:
> +             *  - There are no mitigations for the TSX memory ordering errata.
> +             *  - Performance counter 3 works.  (I.e. it isn't being used by
> +             *    microcode to work around the memory ordering errata.)
> +             *  - TSX_FORCE_ABORT.FORCE_ABORT_RTM is fixed read1/write-discard.
> +             *  - TSX_FORCE_ABORT.TSX_CPUID_CLEAR can be used to hide the
> +             *    HLE/RTM CPUID bits.
> +             *  - TSX_FORCE_ABORT.ENABLE_RTM may be used to opt in to
> +             *    re-enabling RTM, at the users own risk.
> +             */
> +            lo |= rtm_disabled ? TSX_CPUID_CLEAR : TSX_ENABLE_RTM;

... the setting of TSX_ENABLE_RTM here which, as a result, causes
RTM_ALWAYS_ABORT to be clear. If that's correct, perhaps the wording
in that earlier comment would better be something like "we'll set
TSX_FORCE_ABORT.ENABLE_RTM and hence cause RTM_ALWAYS_ABORT to be
hidden from the general CPUID scan later"?

If this understanding of mine is correct, then preferably with some
suitable adjustment to the comment wording
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Also Intel recommends this for SDVs only - can we consider such a
setup supported (not to speak of security supported) at all? I guess
you mean to express this by saying "at their own risk" in the
cmdline doc? If so, perhaps mentioning this in SUPPORT.md would be
a good thing nevertheless, notwithstanding the fact that we're not
really good at expressing there how command line option use affects
support status.

Jan


Re: [PATCH] x86/tsx: Cope with TSX deprecation on SKL/KBL/CFL/WHL
Posted by Andrew Cooper 2 years, 9 months ago
On 09/06/2021 07:36, Jan Beulich wrote:
> On 08.06.2021 19:05, Andrew Cooper wrote:
>> --- a/xen/arch/x86/tsx.c
>> +++ b/xen/arch/x86/tsx.c
>> @@ -60,6 +60,38 @@ void tsx_init(void)
>>               */
>>  
>>              /*
>> +             * Probe for the June 2021 microcode which de-features TSX on
>> +             * client parts.  (Note - this is a subset of parts impacted by
>> +             * the memory ordering errata.)
>> +             *
>> +             * RTM_ALWAYS_ABORT enumerates the new functionality, but is also
>> +             * read as zero if TSX_FORCE_ABORT.ENABLE_RTM has been set before
>> +             * we run.
>> +             *
>> +             * Undo this behaviour in Xen's view of the world.
>> +             */
>> +            bool has_rtm_always_abort = cpu_has_rtm_always_abort;
>> +
>> +            if ( !has_rtm_always_abort )
>> +            {
>> +                uint64_t val;
>> +
>> +                rdmsrl(MSR_TSX_FORCE_ABORT, val);
>> +
>> +                if ( val & TSX_ENABLE_RTM )
>> +                    has_rtm_always_abort = true;
>> +            }
>> +
>> +            /*
>> +             * Always force RTM_ALWAYS_ABORT to be visibile, even if it
>> +             * currently is.  If the user explicitly opts to enable TSX, we'll
>> +             * set TSX_FORCE_ABORT.ENABLE_RTM and hide RTM_ALWAYS_ABORT from
>> +             * the general CPUID scan later.
>> +             */
>> +            if ( has_rtm_always_abort )
>> +                setup_force_cpu_cap(X86_FEATURE_RTM_ALWAYS_ABORT);
> I understand the "we'll set" part, but I don't think "we'll hide"
> anything explicitly. Aiui it is ...
>
>> @@ -131,9 +170,36 @@ void tsx_init(void)
>>          /* Check bottom bit only.  Higher bits are various sentinels. */
>>          rtm_disabled = !(opt_tsx & 1);
>>  
>> -        lo &= ~TSX_FORCE_ABORT_RTM;
>> -        if ( rtm_disabled )
>> -            lo |= TSX_FORCE_ABORT_RTM;
>> +        lo &= ~(TSX_FORCE_ABORT_RTM | TSX_CPUID_CLEAR | TSX_ENABLE_RTM);
>> +
>> +        if ( cpu_has_rtm_always_abort )
>> +        {
>> +            /*
>> +             * June 2021 microcode, on a client part with TSX de-featured:
>> +             *  - There are no mitigations for the TSX memory ordering errata.
>> +             *  - Performance counter 3 works.  (I.e. it isn't being used by
>> +             *    microcode to work around the memory ordering errata.)
>> +             *  - TSX_FORCE_ABORT.FORCE_ABORT_RTM is fixed read1/write-discard.
>> +             *  - TSX_FORCE_ABORT.TSX_CPUID_CLEAR can be used to hide the
>> +             *    HLE/RTM CPUID bits.
>> +             *  - TSX_FORCE_ABORT.ENABLE_RTM may be used to opt in to
>> +             *    re-enabling RTM, at the users own risk.
>> +             */
>> +            lo |= rtm_disabled ? TSX_CPUID_CLEAR : TSX_ENABLE_RTM;
> ... the setting of TSX_ENABLE_RTM here which, as a result, causes
> RTM_ALWAYS_ABORT to be clear. If that's correct, perhaps the wording
> in that earlier comment would better be something like "we'll set
> TSX_FORCE_ABORT.ENABLE_RTM and hence cause RTM_ALWAYS_ABORT to be
> hidden from the general CPUID scan later"?

Yes - that is the intended meaning.  I'll adjust.

> If this understanding of mine is correct, then preferably with some
> suitable adjustment to the comment wording
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> Also Intel recommends this for SDVs only - can we consider such a
> setup supported (not to speak of security supported) at all? I guess
> you mean to express this by saying "at their own risk" in the
> cmdline doc? If so, perhaps mentioning this in SUPPORT.md would be
> a good thing nevertheless, notwithstanding the fact that we're not
> really good at expressing there how command line option use affects
> support status.

I think this is too fine grained to be expressed in SUPPORT.md, but
given that there is a very clear specific issue, I wouldn't consider
this an unsupported configuration overall.

I don't expect people to want to use TSX on these CPUs in the first
place (which was a factor in choosing off-by-default), but if they do,
there is one specific memory ordering issue of read/writes with a
committed transaction.

None of our code uses RTM, so issues in Xen which manifest with tsx=1
won't be related to TSX being enabled.

Obviously, if someone does report an issue, we can tell them to
re-confirm the issue without tsx=1 just to rule out interactions, but I
don't think it is likely for it to be relevant to reported issues.

~Andrew