TAAv5 0

Move IA32_ARCH_CAPABILITIES MSR read to a helper function. If the CPU doesn't support this MSR return 0. Use this function in following commits to read IA32_ARCH_CAPABILITIES MSR.

Signed-off-by Pawan Gupta <[email protected]>

Reviewed-by Mark Gross <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 arch/x86/kernel/cpu/common.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

arch/x86/kernel/cpu/common.c +0 -0

index f125bf7ecb6f..af9e357b2ca9 100644

Quoted 1 line

+static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)

		^^^^^^

Please audit your newly added functions whether they can be __init too.

Disable TSX by default on bootup. If IA32_TSX_CTRL MSR is not present, TSX state stays NOT_SUPPORTED which is the compile time default, otherwise change TSX state to DISABLE. This is because on certain processsors TSX may be used as a part of a speculative side channel attack.

Signed-off-by Pawan Gupta <[email protected]>

Reviewed-by Mark Gross <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 arch/x86/kernel/cpu/Makefile |  2 +-
 arch/x86/kernel/cpu/cpu.h    |  4 ++
 arch/x86/kernel/cpu/intel.c  |  2 +
 arch/x86/kernel/cpu/tsx.c    | 72 ++++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/cpu/tsx.c

arch/x86/kernel/cpu/Makefile +1 -1

index d7a1e5a9331c..890f60083eca 100644
@@ -30,7 +30,7 @@ obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
 
 ifdef CONFIG_CPU_SUP_INTEL
-obj-y			+= intel.o intel_pconfig.o
+obj-y			+= intel.o intel_pconfig.o tsx.o
 obj-$(CONFIG_PM)	+= intel_epb.o
 endif
 obj-$(CONFIG_CPU_SUP_AMD)		+= amd.o

arch/x86/kernel/cpu/cpu.h +4 -0

index c0e2407abdd6..d864ec4180cc 100644
@@ -62,4 +62,8 @@ unsigned int aperfmperf_get_khz(int cpu);
 
 extern void x86_spec_ctrl_setup_ap(void);
 
+extern void tsx_init(struct cpuinfo_x86 *c);
+
+extern u64 read_ia32_arch_cap(void);
+
 #endif /* ARCH_X86_CPU_H */

arch/x86/kernel/cpu/intel.c +2 -0

index 8d6d92ebeb54..b1d6c96f6b88 100644
@@ -761,6 +761,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 		detect_tme(c);
 
 	init_intel_misc_features(c);
+
+	tsx_init(c);
 }
 
 #ifdef CONFIG_X86_32

arch/x86/kernel/cpu/tsx.c new file +61 -0

new file mode 100644
index 000000000000..c549750dd7c8
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Transactional Synchronization Extensions (TSX) control.
+ *
+ * Copyright (C) 2019 Intel Corporation
+ *
+ * Author:
+ *	Pawan Gupta <[email protected]>
+ */
+
+#include <linux/processor.h>
+#include <linux/cpufeature.h>
+
+#include "cpu.h"
+
+static enum tsx_ctrl_states {
+	TSX_CTRL_ENABLE,
+	TSX_CTRL_DISABLE,
+	TSX_CTRL_NOT_SUPPORTED,
+} tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
+
+static void tsx_disable(void)
+{
+	u64 tsx;
+
+	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
+
+	/* Force all transactions to immediately abort */
+	tsx |= TSX_CTRL_RTM_DISABLE;
+	/*
+	 * Ensure TSX support is not enumerated in CPUID.
+	 * This is visible to userspace and will ensure they
+	 * do not waste resources trying TSX transactions that
+	 * will always abort.
+	 */
+	tsx |= TSX_CTRL_CPUID_CLEAR;
+
+	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
+}
+
+static bool tsx_ctrl_is_supported(void)
+{
+	u64 ia32_cap = read_ia32_arch_cap();
+
+	/*
+	 * TSX is controlled via MSR_IA32_TSX_CTRL.  However,
+	 * support for this MSR is enumerated by ARCH_CAP_TSX_MSR bit
+	 * in MSR_IA32_ARCH_CAPABILITIES.
+	 */
+	return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR);
+}
+
+void tsx_init(struct cpuinfo_x86 *c)
+{
+	if (!tsx_ctrl_is_supported())
+		return;
+
+	/*
+	 * Default to TSX_CTRL_DISABLE. This is because on certain processors
+	 * TSX may be used as part of a speculative side channel attack.
+	 */

Quoted 9 lines

(quoted diff) +9 -0

+void tsx_init(struct cpuinfo_x86 *c)
+{
+	if (!tsx_ctrl_is_supported())
+		return;
+
+	/*
+	 * Default to TSX_CTRL_DISABLE. This is because on certain processors
+	 * TSX may be used as part of a speculative side channel attack.
+	 */

Review

Quoted 10 lines

(quoted diff) +10 -0

+	tsx_ctrl_state = TSX_CTRL_DISABLE;
+
+	tsx_disable();
+	/*
+	 * tsx_disable() will change the state of the
+	 * RTM CPUID bit.  Clear it here since it is now
+	 * expected to be not set.
+	 */
+	clear_cpu_cap(c, X86_FEATURE_RTM);
+	setup_clear_cpu_cap(X86_FEATURE_RTM);

You shouldn't be doing both clear_cpu_cap *and* setup_clear_cpu_cap().

Also, please fold patch 4 into this patch or at least make the contents of tsx_init() in this patch make some sense.

Add kernel cmdline parameter "tsx" to control the Transactional Synchronization Extensions (TSX) feature. On CPUs that support TSX control, use "tsx=on|off" to enable or disable TSX. Not specifying this option is equivalent to "tsx=off".

Signed-off-by Pawan Gupta <[email protected]>

Reviewed-by Mark Gross <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 .../admin-guide/kernel-parameters.txt         | 11 +++
 arch/x86/kernel/cpu/tsx.c                     | 97 ++++++++++++++++---
 2 files changed, 95 insertions(+), 13 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +11 -0

index 4c1971960afa..832537d59562 100644
@@ -4813,6 +4813,17 @@
 			interruptions from clocksource watchdog are not
 			acceptable).
 
+	tsx=		[X86] Control Transactional Synchronization
+			Extensions (TSX) feature in Intel processors that
+			support TSX control.
+
+			This parameter controls the TSX feature. The options are:
+
+			on	- Enable TSX on the system.
+			off	- Disable TSX on the system.
+
+			Not specifying this option is equivalent to tsx=off.
+
 	turbografx.map[2|3]=	[HW,JOY]
 			TurboGraFX parallel port interface
 			Format:

arch/x86/kernel/cpu/tsx.c +49 -5

index c549750dd7c8..1c0cee7a7d46 100644
@@ -19,6 +19,30 @@ static enum tsx_ctrl_states {
 	TSX_CTRL_NOT_SUPPORTED,
 } tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
 
+static enum tsx_user_cmds {
+	TSX_USER_CMD_NONE,
+	TSX_USER_CMD_ON,
+	TSX_USER_CMD_OFF,
+} tsx_user_cmd = TSX_USER_CMD_NONE;
+
+static int __init tsx_cmdline(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	/*
+	 * tsx_en/disable() are only called when
+	 * X86_FEATURE_RTM and TSX_CTRL MSR are supported.
+	 */
+	if (!strcmp(str, "on"))
+		tsx_user_cmd = TSX_USER_CMD_ON;
+	else if (!strcmp(str, "off"))
+		tsx_user_cmd = TSX_USER_CMD_OFF;
+
+	return 0;
+}
+early_param("tsx", tsx_cmdline);
+
 static void tsx_disable(void)
 {
 	u64 tsx;

@@ -38,6 +62,24 @@ static void tsx_disable(void)
 	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
 }
 
+static void tsx_enable(void)
+{
+	u64 tsx;
+
+	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
+
+	/* Enable the RTM feature in the cpu */
+	tsx &= ~TSX_CTRL_RTM_DISABLE;
+	/*
+	 * Ensure TSX support is enumerated in CPUID.
+	 * This is visible to userspace and will ensure they
+	 * can enumerate and use the TSX feature.
+	 */
+	tsx &= ~TSX_CTRL_CPUID_CLEAR;
+
+	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
+}
+
 static bool tsx_ctrl_is_supported(void)
 {
 	u64 ia32_cap = read_ia32_arch_cap();

@@ -55,18 +97,47 @@ void tsx_init(struct cpuinfo_x86 *c)
 	if (!tsx_ctrl_is_supported())
 		return;
 
-	/*
-	 * Default to TSX_CTRL_DISABLE. This is because on certain processors
-	 * TSX may be used as part of a speculative side channel attack.
-	 */
-	tsx_ctrl_state = TSX_CTRL_DISABLE;
+	switch (tsx_user_cmd) {
+	case TSX_USER_CMD_ON:
+		tsx_ctrl_state = TSX_CTRL_ENABLE;
+		break;
+	case TSX_USER_CMD_OFF:
+		tsx_ctrl_state = TSX_CTRL_DISABLE;
+		break;

Transactional Synchronization Extensions (TSX) is an extension to the x86 instruction set architecture (ISA) that adds Hardware Transactional Memory (HTM) support. Changing TSX state currently requires a reboot. This may not be desirable when rebooting imposes a huge penalty. Add support to control TSX feature via a new sysfs file:
/sys/devices/system/cpu/hw_tx_mem

- Writing 0|off|N|n to this file disables TSX feature on all the CPUs. This is equivalent to boot parameter tsx=off.
- Writing 1|on|Y|y to this file enables TSX feature on all the CPUs. This is equivalent to boot parameter tsx=on.
- Reading from this returns the status of TSX feature.
- When TSX control is not supported this interface is not visible in sysfs.

Changing the TSX state from this interface also updates CPUID.RTM feature bit. From the kernel side, this feature bit doesn't result in any ALTERNATIVE code patching. No memory allocations are done to save/restore user state. No code paths in outside of the tests for vulnerability to TAA are dependent on the value of the feature bit. In general the kernel doesn't care whether RTM is present or not.

Applications typically look at CPUID bits once at startup (or when first calling into a library that uses the feature). So we have a couple of cases to cover:

1) An application started and saw that RTM was enabled, so began to use it. Then TSX was disabled. Net result in this case is that the application will keep trying to use RTM, but every xbegin() will immediately abort the transaction. This has a performance impact to the application, but it doesn't affect correctness because all users of RTM must have a fallback path for when the transaction aborts. Note that even if an application is in the middle of a transaction when we disable RTM, we are safe. The XPI that we use to update the TSX_CTRL MSR will abort the transaction (just as any interrupt would abort a transaction).

2) An application starts and sees RTM is not available. So it will always use alternative paths. Even if TSX is enabled and RTM is set, applications in general do not re-evaluate their choice so will continue to run in non-TSX mode.

Signed-off-by Pawan Gupta <[email protected]>

Reviewed-by Mark Gross <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 .../ABI/testing/sysfs-devices-system-cpu      |  23 ++++
 arch/x86/kernel/cpu/tsx.c                     | 103 ++++++++++++++++++
 drivers/base/cpu.c                            |  32 +++++-
 include/linux/cpu.h                           |   6 +
 4 files changed, 163 insertions(+), 1 deletion(-)

Documentation/ABI/testing/sysfs-devices-system-cpu +23 -0

index 5f7d7b14fa44..0c3c8c462285 100644
@@ -562,3 +562,26 @@ Description:	Umwait control
 			  or C0.2 state. The time is an unsigned 32-bit number.
 			  Note that a value of zero means there is no limit.
 			  Low order two bits must be zero.
+
+What:		/sys/devices/system/cpu/hw_tx_mem
+Date:		August 2019
+Contact:	Pawan Gupta <[email protected]>
+		Linux kernel mailing list <[email protected]>
+Description:	Hardware Transactional Memory (HTM) control.
+
+		Read/write interface to control HTM feature for all the CPUs in
+		the system.  This interface is only present on platforms that
+		support HTM control. HTM is a hardware feature to speed up the
+		execution of multi-threaded software through lock elision. An
+		example of HTM implementation is Intel Transactional
+		Synchronization Extensions (TSX).
+
+			Read returns the status of HTM feature.
+
+			0: HTM is disabled
+			1: HTM is enabled
+
+			Write sets the state of HTM feature.
+
+			0: Disables HTM
+			1: Enables HTM

arch/x86/kernel/cpu/tsx.c +9 -0

index 73a0e5af3720..2cea038fdcba 100644
@@ -10,9 +10,12 @@
 
 #include <linux/processor.h>
 #include <linux/cpufeature.h>
+#include <linux/cpu.h>
 
 #include "cpu.h"
 
+static DEFINE_MUTEX(tsx_mutex);
+
 static enum tsx_ctrl_states {
 	TSX_CTRL_ENABLE,
 	TSX_CTRL_DISABLE,

@@ -150,3 +153,103 @@ void tsx_init(struct cpuinfo_x86 *c)
 		setup_force_cpu_cap(X86_FEATURE_RTM);
 	}
 }
+
+static void tsx_update_this_cpu(void *arg)
+{
+	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+	unsigned long enable = (unsigned long)arg;
+

Quoted 10 lines

(quoted diff) +10 -0

+static void tsx_update_this_cpu(void *arg)
+{
+	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+	unsigned long enable = (unsigned long)arg;
+
+	if (enable) {
+		tsx_enable();
+		set_cpu_cap(c, X86_FEATURE_RTM);
+		setup_force_cpu_cap(X86_FEATURE_RTM);
+	} else {

Review

Quoted 5 lines

(quoted diff) +5 -0

+		tsx_disable();
+		clear_cpu_cap(c, X86_FEATURE_RTM);
+		setup_clear_cpu_cap(X86_FEATURE_RTM);
+	}
+}

You can't use these setup_... functions after boot like this. Also, I'm not sure you can safely even set_cpu_cap(...) after boot.

Review

Quoted 7 lines

(quoted diff) +6 -0

+
+static void tsx_update_on_each_cpu(bool val)
+{
+	get_online_cpus();
+	on_each_cpu(tsx_update_this_cpu, (void *)val, 1);
+	put_online_cpus();

I sure hope that on_each_cpu() is sensible enough to automatically do the get_online_cpus() thing. Is it not?

Transactional Synchronization Extensions (TSX) may be used on certain processors as part of a speculative side channel attack. A microcode update for existing processors that are vulnerable to this attack will add a new MSR, IA32_TSX_CTRL to allow the system administrator the option to disable TSX as one of the possible mitigations. [Note that future processors that are not vulnerable will also support the IA32_TSX_CTRL MSR]. Add defines for the new IA32_TSX_CTRL MSR and its bits.

TSX has two sub-features:

1. Restricted Transactional Memory (RTM) is an explicitly-used feature where new instructions begin and end TSX transactions.
2. Hardware Lock Elision (HLE) is implicitly used when certain kinds of "old" style locks are used by software.

Bit 7 of the IA32_ARCH_CAPABILITIES indicates the presence of the IA32_TSX_CTRL MSR.

There are two control bits in IA32_TSX_CTRL MSR:

Bit 0: When set it disables the Restricted Transactional Memory (RTM)
sub-feature of TSX (will force all transactions to abort on the XBEGIN instruction).

Bit 1: When set it disables the enumeration of the RTM feature (i.e.
it will make CPUID(EAX=7).EBX{bit11} read as 0).

The other TSX sub-feature, Hardware Lock Elision (HLE), is unconditionally disabled but still enumerated as present by CPUID(EAX=7).EBX{bit4}.

Quoted 25 lines · speck for Pawan Gupta

Transactional Synchronization Extensions (TSX) may be used on certain processors as part of a speculative side channel attack. A microcode update for existing processors that are vulnerable to this attack will add a new MSR, IA32_TSX_CTRL to allow the system administrator the option to disable TSX as one of the possible mitigations. [Note that future processors that are not vulnerable will also support the IA32_TSX_CTRL MSR]. Add defines for the new IA32_TSX_CTRL MSR and its bits.

TSX has two sub-features:

1. Restricted Transactional Memory (RTM) is an explicitly-used feature where new instructions begin and end TSX transactions.
2. Hardware Lock Elision (HLE) is implicitly used when certain kinds of "old" style locks are used by software.

Bit 7 of the IA32_ARCH_CAPABILITIES indicates the presence of the IA32_TSX_CTRL MSR.

There are two control bits in IA32_TSX_CTRL MSR:

Bit 0: When set it disables the Restricted Transactional Memory (RTM)
sub-feature of TSX (will force all transactions to abort on the XBEGIN instruction).

Quoted 5 lines

Bit 1: When set it disables the enumeration of the RTM feature (i.e.
it will make CPUID(EAX=7).EBX{bit11} read as 0).

The other TSX sub-feature, Hardware Lock Elision (HLE), is unconditionally disabled but still enumerated as present by CPUID(EAX=7).EBX{bit4}.

Does the kernel need to intercept CPUID to clear the HLE bit to avoid potential performance loss? I can imagine some programs using alternate code paths that are better in the non-HLE case.

Transactional Synchronization Extensions (TSX) is an extension to the x86 instruction set architecture (ISA) that adds Hardware Transactional Memory (HTM) support. Changing TSX state currently requires a reboot. This may not be desirable when rebooting imposes a huge penalty. Add support to control TSX feature via a new sysfs file:
/sys/devices/system/cpu/hw_tx_mem

- Writing 0|off|N|n to this file disables TSX feature on all the CPUs. This is equivalent to boot parameter tsx=off.
- Writing 1|on|Y|y to this file enables TSX feature on all the CPUs. This is equivalent to boot parameter tsx=on.
- Reading from this returns the status of TSX feature.
- When TSX control is not supported this interface is not visible in sysfs.

Changing the TSX state from this interface also updates CPUID.RTM feature bit. From the kernel side, this feature bit doesn't result in any ALTERNATIVE code patching. No memory allocations are done to save/restore user state. No code paths in outside of the tests for vulnerability to TAA are dependent on the value of the feature bit. In general the kernel doesn't care whether RTM is present or not.

Applications typically look at CPUID bits once at startup (or when first calling into a library that uses the feature). So we have a couple of cases to cover:

1) An application started and saw that RTM was enabled, so began to use it. Then TSX was disabled. Net result in this case is that the application will keep trying to use RTM, but every xbegin() will immediately abort the transaction. This has a performance impact to the application, but it doesn't affect correctness because all users of RTM must have a fallback path for when the transaction aborts. Note that even if an application is in the middle of a transaction when we disable RTM, we are safe. The XPI that we use to update the TSX_CTRL MSR will abort the transaction (just as any interrupt would abort a transaction).

2) An application starts and sees RTM is not available. So it will always use alternative paths. Even if TSX is enabled and RTM is set, applications in general do not re-evaluate their choice so will continue to run in non-TSX mode.

Signed-off-by Pawan Gupta <[email protected]>

Reviewed-by Mark Gross <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 .../ABI/testing/sysfs-devices-system-cpu      |  23 ++++
 arch/x86/kernel/cpu/tsx.c                     | 103 ++++++++++++++++++
 drivers/base/cpu.c                            |  32 +++++-
 include/linux/cpu.h                           |   6 +
 4 files changed, 163 insertions(+), 1 deletion(-)

Documentation/ABI/testing/sysfs-devices-system-cpu +23 -0

index 5f7d7b14fa44..0c3c8c462285 100644
@@ -562,3 +562,26 @@ Description:	Umwait control
 			  or C0.2 state. The time is an unsigned 32-bit number.
 			  Note that a value of zero means there is no limit.
 			  Low order two bits must be zero.
+
+What:		/sys/devices/system/cpu/hw_tx_mem
+Date:		August 2019
+Contact:	Pawan Gupta <[email protected]>
+		Linux kernel mailing list <[email protected]>
+Description:	Hardware Transactional Memory (HTM) control.
+
+		Read/write interface to control HTM feature for all the CPUs in
+		the system.  This interface is only present on platforms that
+		support HTM control. HTM is a hardware feature to speed up the
+		execution of multi-threaded software through lock elision. An
+		example of HTM implementation is Intel Transactional
+		Synchronization Extensions (TSX).
+
+			Read returns the status of HTM feature.
+
+			0: HTM is disabled
+			1: HTM is enabled
+
+			Write sets the state of HTM feature.
+
+			0: Disables HTM
+			1: Enables HTM

arch/x86/kernel/cpu/tsx.c +0 -0

index 73a0e5af3720..2cea038fdcba 100644

arch/x86/kernel/cpu/tsx.c +49 -0

+ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	enum tsx_ctrl_states requested_state;
+	ssize_t ret;
+	bool val;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&tsx_mutex);
+
+	if (val) {
+		tsx_user_cmd = TSX_USER_CMD_ON;
+		requested_state = TSX_CTRL_ENABLE;
+	} else {
+		tsx_user_cmd = TSX_USER_CMD_OFF;
+		requested_state = TSX_CTRL_DISABLE;
+	}
+
+	/* Current state is same as the reqested state, do nothing */
+	if (tsx_ctrl_state == requested_state)
+		goto exit;
+
+	tsx_ctrl_state = requested_state;
+
+	/*
+	 * Changing the TSX state from this interface also updates CPUID.RTM
+	 * feature  bit. From the kernel side, this feature bit doesn't result
+	 * in any ALTERNATIVE code patching.  No memory allocations are done to
+	 * save/restore user state. No code paths in outside of the tests for
+	 * vulnerability to TAA are dependent on the value of the feature bit.
+	 * In general the kernel doesn't care whether RTM is present or not.
+	 *
+	 * From the user side it is a bit fuzzier. Applications typically look
+	 * at CPUID bits once at startup (or when first calling into a library
+	 * that uses the feature).  So we have a couple of cases to cover:
+	 *
+	 * 1) An application started and saw that RTM was enabled, so began
+	 *    to use it. Then TSX was disabled.  Net result in this case is
+	 *    that the application will keep trying to use RTM, but every
+	 *    xbegin() will immediately abort the transaction. This has a
+	 *    performance impact to the application, but it doesn't affect
+	 *    correctness because all users of RTM must have a fallback path
+	 *    for when the transaction aborts. Note that even if an application
+	 *    is in the middle of a transaction when we disable RTM, we are
+	 *    safe. The XPI that we use to update the TSX_CTRL MSR will abort
+	 *    the transaction (just as any interrupt would abort a

Quoted 1 line

+static DEFINE_MUTEX(tsx_mutex);

I think I asked this before, but in looking at the code I still can't figure it out. What exactly is this protecting?

It looks like you want to keep only one "writer" out of the sysfs store function at a time, but:

Quoted 25 lines

(quoted diff) +25 -0

+ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	enum tsx_ctrl_states requested_state;
+	ssize_t ret;
+	bool val;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&tsx_mutex);
+
+	if (val) {
+		tsx_user_cmd = TSX_USER_CMD_ON;
+		requested_state = TSX_CTRL_ENABLE;
+	} else {
+		tsx_user_cmd = TSX_USER_CMD_OFF;
+		requested_state = TSX_CTRL_DISABLE;
+	}
+
+	/* Current state is same as the reqested state, do nothing */
+	if (tsx_ctrl_state == requested_state)
+		goto exit;
+

Review

Quoted 5 lines

(quoted diff) +5 -0

+	tsx_ctrl_state = requested_state;
+
+	tsx_update_on_each_cpu(val);
+exit:
+	mutex_unlock(&tsx_mutex);

What I think you want to do is just protect the tsx_update_on_each_cpu() function, right?

So, you are locking _outside_ of a function call? That's a sure way to madness over time. If this function is so special that it can not be called multiple times at once, then put the lock _inside_ the function, right?

Otherwise you could have other places call that function, and this single lock is not going to protect anything :(

And why is tsx_user_cmd needed, and global?

Quoted 33 lines

Quoted 1 line

+static DEFINE_MUTEX(tsx_mutex);

I think I asked this before, but in looking at the code I still can't figure it out. What exactly is this protecting?

It looks like you want to keep only one "writer" out of the sysfs store function at a time, but:

Quoted 25 lines

(quoted diff) +25 -0

+ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	enum tsx_ctrl_states requested_state;
+	ssize_t ret;
+	bool val;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&tsx_mutex);
+
+	if (val) {
+		tsx_user_cmd = TSX_USER_CMD_ON;
+		requested_state = TSX_CTRL_ENABLE;
+	} else {
+		tsx_user_cmd = TSX_USER_CMD_OFF;
+		requested_state = TSX_CTRL_DISABLE;
+	}
+
+	/* Current state is same as the reqested state, do nothing */
+	if (tsx_ctrl_state == requested_state)
+		goto exit;
+

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	tsx_ctrl_state = requested_state;
+
+	tsx_update_on_each_cpu(val);
+exit:
+	mutex_unlock(&tsx_mutex);

What I think you want to do is just protect the tsx_update_on_each_cpu() function, right?

Also I believe below two operations needs to be under a lock. Without the lock if there are two writers and one is preempted in between these operations there is a possibility that tsx_ctrl_state and TSX hardware state could go out of sync.

	tsx_ctrl_state = requested_state;

	// 1st writer gets preempted here
	// 2nd writer flips tsx_ctrl_state and writes to the MSR.
	// 1st writer wakes up and only writes to the MSR

	tsx_update_on_each_cpu(val);
	// tsx_ctrl_state and hardware state would be different here.

Chances of this happening is rare but still a possibility. The lock would prevent such a condition.

Quoted 53 lines · speck for Pawan Gupta

Quoted 33 lines

Quoted 1 line

+static DEFINE_MUTEX(tsx_mutex);

I think I asked this before, but in looking at the code I still can't figure it out. What exactly is this protecting?

It looks like you want to keep only one "writer" out of the sysfs store function at a time, but:

Quoted 25 lines

(quoted diff) +25 -0

+ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	enum tsx_ctrl_states requested_state;
+	ssize_t ret;
+	bool val;
+
+	ret = kstrtobool(buf, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&tsx_mutex);
+
+	if (val) {
+		tsx_user_cmd = TSX_USER_CMD_ON;
+		requested_state = TSX_CTRL_ENABLE;
+	} else {
+		tsx_user_cmd = TSX_USER_CMD_OFF;
+		requested_state = TSX_CTRL_DISABLE;
+	}
+
+	/* Current state is same as the reqested state, do nothing */
+	if (tsx_ctrl_state == requested_state)
+		goto exit;
+

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	tsx_ctrl_state = requested_state;
+
+	tsx_update_on_each_cpu(val);
+exit:
+	mutex_unlock(&tsx_mutex);

What I think you want to do is just protect the tsx_update_on_each_cpu() function, right?

Also I believe below two operations needs to be under a lock. Without the lock if there are two writers and one is preempted in between these operations there is a possibility that tsx_ctrl_state and TSX hardware state could go out of sync.

	tsx_ctrl_state = requested_state;

	// 1st writer gets preempted here
	// 2nd writer flips tsx_ctrl_state and writes to the MSR.
	// 1st writer wakes up and only writes to the MSR

Quoted 5 lines

	tsx_update_on_each_cpu(val);
	// tsx_ctrl_state and hardware state would be different here.

Chances of this happening is rare but still a possibility. The lock would prevent such a condition.

Greg, is it not a possible scenario for tsx_ctrl_state and MSR write to be under the lock.

On Thu, Oct 10, 2019 at 02:31:51PM -0700, speck for Pawan Gupta wrote:
I'm sorry, but I do not understand what you are trying to state here.

Look at the code, and the lock, and determine what it really is doing and document that please. Right now I can't determine it and it looks pointless to me, so I must be missing something.

TSX Async Abort (TAA) is a side channel vulnerability to the internal buffers in some Intel processors similar to Microachitectural Data Sampling (MDS). In this case certain loads may speculatively pass invalid data to dependent operations when an asynchronous abort condition is pending in a TSX transaction. This includes loads with no fault or assist condition. Such loads may speculatively expose stale data from the uarch data structures as in MDS. Scope of exposure is within the same-thread and cross-thread. This issue affects all current processors that support TSX, but do not have ARCH_CAP_TAA_NO (bit 8) set in MSR_IA32_ARCH_CAPABILITIES.

(quoted diff) +1 -1

-/* Update the static key controlling the MDS CPU buffer clear in idle */
+/* Update the static key controlling the MDS and TAA CPU buffer clear in idle */
 static void update_mds_branch_idle(void)
 {
 	/*

Quoted 6 lines

(quoted diff) +1 -1

-/* Update the static key controlling the MDS CPU buffer clear in idle */
+/* Update the static key controlling the MDS and TAA CPU buffer clear in idle */
 static void update_mds_branch_idle(void)
 {
 	/*

Review static void update_mds_branch_idle(void) · line 889

Quoted 10 lines

(quoted diff) +4 -1

@@ -775,8 +881,11 @@ static void update_mds_branch_idle(void)
 	 * The other variants cannot be mitigated when SMT is enabled, so
 	 * clearing the buffers on idle just to prevent the Store Buffer
 	 * repartitioning leak would be a window dressing exercise.
+	 *
+	 * Apply idle buffer clearing to TAA affected CPUs also.
 	 */
-	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY))
+	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY) &&
+	    !boot_cpu_has_bug(X86_BUG_TAA))
 		return;

I'm not sure this belongs here. The existing check was for X86_BUG_MSBDS_ONLY, which is the only MDS variant which can be mitigated in idle with SMT enabled. If the other MDS variants are present (which is true for most CPUs) then this function doesn't enable the MDS idle mitigation, because it wouldn't be sufficient.

So does this fully mitigate TAA on idle threads with SMT enabled? If not then this change is probably pointless.

Transactional Synchronization Extensions (TSX) is an extension to the x86 instruction set architecture (ISA) that adds Hardware Transactional Memory (HTM) support. Changing TSX state currently requires a reboot. This may not be desirable when rebooting imposes a huge penalty.

Quoted 4 lines · speck for Pawan Gupta

Transactional Synchronization Extensions (TSX) is an extension to the x86 instruction set architecture (ISA) that adds Hardware Transactional Memory (HTM) support. Changing TSX state currently requires a reboot. This may not be desirable when rebooting imposes a huge penalty.

This is still missing a real world justification for the added complexity. Don't production users typically know at boot time whether they plan to use TSX?

It looks like this patch should be combined with patch 10, since otherwise this patch causes a regression in the sysfs mitigation reporting, right?

And again... this patch needs to be last. Even after the documentation. If it triggers documentation changes then those changes should be separated out and included in this patch. Otherwise, this patch may block the merging of the documentation patch.

Quoted 8 lines

Quoted 4 lines

Transactional Synchronization Extensions (TSX) is an extension to the x86 instruction set architecture (ISA) that adds Hardware Transactional Memory (HTM) support. Changing TSX state currently requires a reboot. This may not be desirable when rebooting imposes a huge penalty.

This is still missing a real world justification for the added complexity. Don't production users typically know at boot time whether they plan to use TSX?

I am not sure about this.

Platforms which are not affected by X86_BUG_TAA may want the TSX feature enabled. Add "auto" option to the TSX cmdline parameter. When tsx=auto, disable TSX when X86_BUG_TAA is present, otherwise enable TSX.

More details on X86_BUG_TAA can be found here:
https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html

Signed-off-by Pawan Gupta <[email protected]>

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 Documentation/admin-guide/kernel-parameters.txt | 5 +++++
 arch/x86/kernel/cpu/tsx.c                       | 9 +++++++++
 2 files changed, 14 insertions(+)

Documentation/admin-guide/kernel-parameters.txt +5 -0

index 832537d59562..d7b48c38c6e5 100644
@@ -4821,6 +4821,11 @@
 
 			on	- Enable TSX on the system.
 			off	- Disable TSX on the system.
+			auto	- Disable TSX if X86_BUG_TAA is present,
+				  otherwise enable TSX on the system.
+
+			More details on X86_BUG_TAA are here:
+			Documentation/admin-guide/hw-vuln/tsx_async_abort.rst
 
 			Not specifying this option is equivalent to tsx=off.
 

arch/x86/kernel/cpu/tsx.c +9 -0

index 1c0cee7a7d46..73a0e5af3720 100644
@@ -23,6 +23,7 @@ static enum tsx_user_cmds {
 	TSX_USER_CMD_NONE,
 	TSX_USER_CMD_ON,
 	TSX_USER_CMD_OFF,
+	TSX_USER_CMD_AUTO,
 } tsx_user_cmd = TSX_USER_CMD_NONE;
 
 static int __init tsx_cmdline(char *str)

@@ -38,6 +39,8 @@ static int __init tsx_cmdline(char *str)
 		tsx_user_cmd = TSX_USER_CMD_ON;
 	else if (!strcmp(str, "off"))
 		tsx_user_cmd = TSX_USER_CMD_OFF;
+	else if (!strcmp(str, "auto"))
+		tsx_user_cmd = TSX_USER_CMD_AUTO;
 
 	return 0;
 }

@@ -104,6 +107,12 @@ void tsx_init(struct cpuinfo_x86 *c)
 	case TSX_USER_CMD_OFF:
 		tsx_ctrl_state = TSX_CTRL_DISABLE;
 		break;
+	case TSX_USER_CMD_AUTO:
+		if (boot_cpu_has_bug(X86_BUG_TAA))
+			tsx_ctrl_state = TSX_CTRL_DISABLE;
+		else
+			tsx_ctrl_state = TSX_CTRL_ENABLE;
+		break;
 	case TSX_USER_CMD_NONE:
 	default:
 		/*

Signature · 1 line

2.20.1

Quoted 6 lines · speck for Pawan Gupta

Platforms which are not affected by X86_BUG_TAA may want the TSX feature enabled. Add "auto" option to the TSX cmdline parameter. When tsx=auto, disable TSX when X86_BUG_TAA is present, otherwise enable TSX.

More details on X86_BUG_TAA can be found here:
https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html

This patch is still keeping the default for tsx=off unless I got lost in the enable/disable logic, right?
The earlier arguments for going this way was that there are no real users of TSX in production. This is not really the case though. At least SAP HANA seems to benefit from TSX - publicly available information can be found [1][2][3]. We have talked to SAP guys today and this is still the case.

I do understand that there are likely not that many other production users of the feature but I believe that we should keep the default update friendly and sticking with the auto semantic by default is both in line with other mitigations and also reduces the risk of regressions.

The real question is whether machines on which SAP/HANA runs are just random boxes/VMs which would be affected by the 'update and reboot world' treatment or whether SAP/HANA runs on dedicated systems which are carefully administrated by sysadmins with brains turned on.

If it's the random box affected by 'update and reboot world' then disabling TSX by default is obviously going to be a source of regressions and we have to bite the bullet.

If not, then the sysadmins will anyway do some very careful evaluation and checking, read documentation and add 'WE_REALLY_WANT_TSX' on the kernel command line. In that case I rather disgruntle a few sysadmins and gain a useful setup for everyone else.

not everybody (by far) is reading those. If this regressess on a handful of platforms, we (together with the OS vendor) could probably handle the followup reports, but if it happens all across the board, it'd surely be overwhelming, to put it mildly. It'll" basically promptly escalate to a support disaster."

Quoted 5 lines

not everybody (by far) is reading those. If this regressess on a=20 handful of platforms, we (together with the OS vendor) could=20 probably handle the followup reports, but if it happens all across=20 the board, it'd surely be overwhelming, to put it mildly. It'll" basically promptly escalate to a support disaster."

Ok, how about we make it a config option: CONFIG_TSX_DEFAULT_ON

and we - distro kernels - enable it. Mainline leaves it off.

Problem solved.

Hmmm?

Quoted 5 lines · speck for Borislav Petkov

Quoted 5 lines

not everybody (by far) is reading those. If this regressess on a=20 handful of platforms, we (together with the OS vendor) could=20 probably handle the followup reports, but if it happens all across=20 the board, it'd surely be overwhelming, to put it mildly. It'll" basically promptly escalate to a support disaster."

Quoted 7 lines

Ok, how about we make it a config option: CONFIG_TSX_DEFAULT_ON

and we - distro kernels - enable it. Mainline leaves it off.

Problem solved.

Hmmm?

If noone would strongly object to that config option, I'll happily send a followup patch on top of Pawan's series that'd be adding it (defaulting to 'n', as that appears to be the common consensus for upstream for totally understandable reasons).

Otherwise, some distros might just have to flip the default in the sources by non-upstream patch, I am afraid (even though we haven't made it to the point where are are making that decision yet).

Quoted 7 lines · speck for Jiri Kosina

Quoted 5 lines · speck for Borislav Petkov

Quoted 5 lines

not everybody (by far) is reading those. If this regressess on a=20 handful of platforms, we (together with the OS vendor) could=20 probably handle the followup reports, but if it happens all across=20 the board, it'd surely be overwhelming, to put it mildly. It'll" basically promptly escalate to a support disaster."

I feared that :)

Quoted 13 lines

Quoted 7 lines

Ok, how about we make it a config option: CONFIG_TSX_DEFAULT_ON

and we - distro kernels - enable it. Mainline leaves it off.

Problem solved.

Hmmm?

If noone would strongly object to that config option, I'll happily send a followup patch on top of Pawan's series that'd be adding it (defaulting to 'n', as that appears to be the common consensus for upstream for totally understandable reasons).

Quoted 3 lines

Otherwise, some distros might just have to flip the default in the sources by non-upstream patch, I am afraid (even though we haven't made it to the point where are are making that decision yet).

The config option is fine.

OK, so what about this patch on top of Pawan's series? I have to say I am not really entirely happy about yet another config option. In principle this is not much different from the HT where we decided to stay enabled even though it is vulnerable to side channels. But I do understand that much more people will notice HT off than TSX off.

Anyway here is the patch
---
From 9666e91b63cd6213d362d04289e1bcbbe2050bc3 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 15 Oct 2019 11:21:01 +0200
Subject: [PATCH] x86, tsx: allow to set tsx=auto by a config option

There is a general consensus that TSX usage is not largely spread while the history shows there is a non trivial space for side channel attacks possible. Therefore the tsx is disabled by default even on platforms that might have a safe implementation of TSX according to the current knowledge. This is a fair trade off to make.

There are, however, workloads that really do benefit from using TSX and updating to a newer kernel with TSX disabled might introduce a noticeable regressions. This would be especially a problem for Linux distributions which will provide TAA mitigations.

Introduce X86_INTEL_ENABLE_SAFE_TSX config option to override the default tsx=off semantic and make tsx=auto a default which is more update friendly.

Suggested-by Borislav Petkov <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Since all (or most?) modern Intel CPUs are vulnerable to TAA, defaulting to tsx=auto would effectively be the same as defaulting to tsx=off, right? How does this help with regressions?

Quoted 3 lines · speck for Josh Poimboeuf

Since all (or most?) modern Intel CPUs are vulnerable to TAA, defaulting to tsx=auto would effectively be the same as defaulting to tsx=off, right? How does this help with regressions?

The mitigation is only needed on CPUs where verw doesn't have the buffer clearing semantics.

Quoted 6 lines

Quoted 3 lines

Since all (or most?) modern Intel CPUs are vulnerable to TAA, defaulting to tsx=auto would effectively be the same as defaulting to tsx=off, right? How does this help with regressions?

The mitigation is only needed on CPUs where verw doesn't have the buffer clearing semantics.

Can you elaborate? I have no idea what you're trying to say and how it relates to my question :-)

Quoted 9 lines · speck for Josh Poimboeuf

Quoted 6 lines

Quoted 3 lines

Since all (or most?) modern Intel CPUs are vulnerable to TAA, defaulting to tsx=auto would effectively be the same as defaulting to tsx=off, right? How does this help with regressions?

The mitigation is only needed on CPUs where verw doesn't have the buffer clearing semantics.

Can you elaborate? I have no idea what you're trying to say and how it relates to my question :-)

Only those CPUs with TSX *and* with MDS_NO need TSX disabled in order to protect from this issues.

The CPUs that don't enumarate MDS_NO (and therefore got ucode update with verw buffer-clearing semantics) are fully mitigated against TAA by MDS mitigations already.

Therefore the set of CPUs where we *really* need to turn of TSX in order to protect from TAA is currently rather minimal (CascadeLake-B, WhiskeyLake-V, CommitLake, CoffeeLake-R), so force-disabling on all CPUs covers way bigger set of platforms than actually needed.

Signature · 2 lines

Jiri Kosina
SUSE Labs

Quoted 9 lines · speck for Josh Poimboeuf

Quoted 6 lines

Quoted 3 lines

Since all (or most?) modern Intel CPUs are vulnerable to TAA, defaulting to tsx=auto would effectively be the same as defaulting to tsx=off, right? How does this help with regressions?

The mitigation is only needed on CPUs where verw doesn't have the buffer clearing semantics.

Can you elaborate? I have no idea what you're trying to say and how it relates to my question :-)

Only those CPUs with TSX *and* with MDS_NO need TSX disabled in order to protect from this issues.

The CPUs that don't enumarate MDS_NO (and therefore got ucode update with verw buffer-clearing semantics) are fully mitigated against TAA by MDS mitigations already.

Therefore the set of CPUs where we *really* need to turn of TSX in order to protect from TAA is currently rather minimal (CascadeLake-B, WhiskeyLake-V, CommitLake, CoffeeLake-R), so force-disabling on all CPUs covers way bigger set of platforms than actually needed.

Maybe I'm missing something. Isn't there going to be a ucode update for MDS_NO parts, which does the verw buffer clearing? In that case there's no need to disable TSX, and instead the verw mitigation could be used, if desired.

AFAICT, the patch allows to set the default to tsx=auto, which disables TSX on *all* vulnerable parts, not just the MDS_NO ones. I don't see how that would prevent user regressions.

It sounds like maybe you're suggesting something else, that TSX should only be disabled on vulnerable MDS_NO parts?

Quoted 4 lines

Maybe I'm missing something. Isn't there going to be a ucode update for MDS_NO parts, which does the verw buffer clearing? In that case there's no need to disable TSX, and instead the verw mitigation could be used, if desired.

My understanding was that MDS_NO CPUs will only get ucode update that exposes TSX control MSR, and nothing else.

Quoted 4 lines

AFAICT, the patch allows to set the default to tsx=auto, which disables TSX on *all* vulnerable parts, not just the MDS_NO ones. I don't see how that would prevent user regressions.

Quoted 2 lines

It sounds like maybe you're suggesting something else, that TSX should only be disabled on vulnerable MDS_NO parts?

OK, let me take a look at the code again. I definitely thought that's what 'auto' indeed does.

Quoted 7 lines · speck for Jiri Kosina

Quoted 4 lines

Maybe I'm missing something. Isn't there going to be a ucode update for MDS_NO parts, which does the verw buffer clearing? In that case there's no need to disable TSX, and instead the verw mitigation could be used, if desired.

My understanding was that MDS_NO CPUs will only get ucode update that exposes TSX control MSR, and nothing else.

Quoted 9 lines

Quoted 6 lines

AFAICT, the patch allows to set the default to tsx=auto, which disables TSX on *all* vulnerable parts, not just the MDS_NO ones. I don't see how that would prevent user regressions.

It sounds like maybe you're suggesting something else, that TSX should only be disabled on vulnerable MDS_NO parts?

OK, let me take a look at the code again. I definitely thought that's what 'auto' indeed does.

OK, so you are right and I misunderstood the logic in the code, sorry.

Then the only purpose of 'auto' really is getting TSX enabled on future CPUs which would eventually have ARCH_CAP_TAA_NO=1; so pretty useless for preventing regressions.

So yeah, I agree, 'auto' is actually useless to prevent regressions, and I believe we want some other 'auto' (*), which would actually disable TSX only if (X86_BUG_TAA && !MD_CLEAR), agreed?

(*) I'd actually prefer to convert the current 'auto' to this new
semantics; it'll keep TSX enabled on future CPUs without X86_BUG_TAA, and it'll prevent regressions in unnecessary cases.

Quoted 12 lines · speck for Jiri Kosina

Quoted 4 lines

Maybe I'm missing something. Isn't there going to be a ucode update for MDS_NO parts, which does the verw buffer clearing? In that case there's no need to disable TSX, and instead the verw mitigation could be used, if desired.

My understanding was that MDS_NO CPUs will only get ucode update that exposes TSX control MSR, and nothing else.

Quoted 4 lines

AFAICT, the patch allows to set the default to tsx=auto, which disables TSX on *all* vulnerable parts, not just the MDS_NO ones. I don't see how that would prevent user regressions.

Quoted 5 lines

Quoted 2 lines

It sounds like maybe you're suggesting something else, that TSX should only be disabled on vulnerable MDS_NO parts?

OK, let me take a look at the code again. I definitely thought that's what 'auto' indeed does.

OK, so you are right and I misunderstood the logic in the code, sorry.

Then the only purpose of 'auto' really is getting TSX enabled on future CPUs which would eventually have ARCH_CAP_TAA_NO=1; so pretty useless for preventing regressions.

So yeah, I agree, 'auto' is actually useless to prevent regressions, and I believe we want some other 'auto' (*), which would actually disable TSX only if (X86_BUG_TAA && !MD_CLEAR), agreed?

(*) I'd actually prefer to convert the current 'auto' to this new
semantics; it'll keep TSX enabled on future CPUs without X86_BUG_TAA, and it'll prevent regressions in unnecessary cases.

tsx_ctrl_is_supported() checks for the TSX_CTRL MSR support and then only attempts at disabling TSX. MDS_NO=0 CPUs will not get the ucode update for TSX control, therefore tsx=auto wont cause regression on older CPUs.

Quoted 4 lines · speck for Pawan Gupta

tsx_ctrl_is_supported() checks for the TSX_CTRL MSR support and then only attempts at disabling TSX. MDS_NO=0 CPUs will not get the ucode update for TSX control, therefore tsx=auto wont cause regression on older CPUs.

OK, that piece of information finally made it to make sense again :)

So I believe distros still want the option (Michal's patch) to default to 'auto', so that actual heavy users of TSX will get the right thing once they update their CPUs to !TAA_BUG ones, but it's less urgent that I originally thought.

So if I understand correctly, you're postulating that distros want:

a) TAA_BUG && MDS_NO=0 => TSX on
b) TAA_BUG && MDS_NO=1 => TSX off
c) !TAA_BUG => TSX on

How are you reaching that conclusion? It seems horribly confusing for TSX users, but again maybe I'm missing something.

It seems to me that "heavy users of TSX" would want tsx=on, no matter what. And so we would need to leave that as the default in order to not break those users.

But then we're not defaulting to safe behavior, which is confusing too, because we almost (SMT being the exception) always did in the past for all the other previous issues.

So I believe the above is the best compromise between having as few regressions as possible, while still maintaining security (by default).

I agree it's not nice, but nothing is, if you ask me.

But again, given Pawan's (surprising to me, I'd still like to find this detail documented somewhere in Intel's materials) statement that there will never be a situation when some older CPU with MDS_NO will see ARCH_CAP_TSX_CTRL_MSR exposed by new ucode, I see this as much less pressing issue, and we could get away with tsx=off, as the number immediate regressions will be minimal.

Quoted 9 lines · speck for Josh Poimboeuf

So if I understand correctly, you're postulating that distros want:

a) TAA_BUG && MDS_NO=0 => TSX on
b) TAA_BUG && MDS_NO=1 => TSX off
c) !TAA_BUG => TSX on

How are you reaching that conclusion? It seems horribly confusing for TSX users, but again maybe I'm missing something.

Quoted 3 lines

It seems to me that "heavy users of TSX" would want tsx=on, no matter what. And so we would need to leave that as the default in order to not break those users.

But then we're not defaulting to safe behavior, which is confusing too, because we almost (SMT being the exception) always did in the past for all the other previous issues.

Why wouldn't it be safe behavior? For both TAA_BUG cases, we can mitigate TSX with verw buffer clearing.

On Tue, Oct 15, 2019 at 11:14:03PM +0200, speck for Jiri Kosina wrote:

Quoted 6 lines

OK, that piece of information finally made it to make sense again :)

So I believe distros still want the option (Michal's patch) to default to 'auto', so that actual heavy users of TSX will get the right thing once they update their CPUs to !TAA_BUG ones, but it's less urgent that I originally thought.

So if I understand correctly, you're postulating that distros want:

a) TAA_BUG && MDS_NO=0 => TSX on
b) TAA_BUG && MDS_NO=1 => TSX off
c) !TAA_BUG => TSX on

Quoted 1 line · speck for Josh Poimboeuf

On Tue, Oct 15, 2019 at 11:14:03PM +0200, speck for Jiri Kosina wrote:

[...]

Quoted 7 lines

Quoted 6 lines

OK, that piece of information finally made it to make sense again :)

So I believe distros still want the option (Michal's patch) to default to 'auto', so that actual heavy users of TSX will get the right thing once they update their CPUs to !TAA_BUG ones, but it's less urgent that I originally thought.

Quoted 5 lines

So if I understand correctly, you're postulating that distros want:

a) TAA_BUG && MDS_NO=0 => TSX on
b) TAA_BUG && MDS_NO=1 => TSX off
c) !TAA_BUG => TSX on

[...]

I think this should be:

a) TAA_BUG && MD_CLEAR=1 => TSX on
b) TAA_BUG && MD_CLEAR=0 => TSX off
c) !TAA_BUG => TSX on

As I understand it, with currently released microcode, no CPUs have both MDS_NO and MD_CLEAR set. But with the pending microcode updates, CPUs with MDS_NO=1 will also get MD_CLEAR=1 and we can use VERW to mitigate against TAA.

From 9666e91b63cd6213d362d04289e1bcbbe2050bc3 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 15 Oct 2019 11:21:01 +0200
Subject: [PATCH] x86, tsx: allow to set tsx=auto by a config option

There is a general consensus that TSX usage is not largely spread while the history shows there is a non trivial space for side channel attacks possible. Therefore the tsx is disabled by default even on platforms that might have a safe implementation of TSX according to the current knowledge. This is a fair trade off to make.

There are, however, workloads that really do benefit from using TSX and updating to a newer kernel with TSX disabled might introduce a noticeable regressions. This would be especially a problem for Linux distributions which will provide TAA mitigations.

Introduce X86_INTEL_ENABLE_SAFE_TSX config option to override the default tsx=off semantic and make tsx=auto a default which is more update friendly.

Quoted 16 lines · speck for Michal Hocko

From 9666e91b63cd6213d362d04289e1bcbbe2050bc3 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Tue, 15 Oct 2019 11:21:01 +0200
Subject: [PATCH] x86, tsx: allow to set tsx=auto by a config option

There is a general consensus that TSX usage is not largely spread while the history shows there is a non trivial space for side channel attacks possible. Therefore the tsx is disabled by default even on platforms that might have a safe implementation of TSX according to the current knowledge. This is a fair trade off to make.

There are, however, workloads that really do benefit from using TSX and updating to a newer kernel with TSX disabled might introduce a noticeable regressions. This would be especially a problem for Linux distributions which will provide TAA mitigations.

Quoted 3 lines

Introduce X86_INTEL_ENABLE_SAFE_TSX config option to override the default tsx=off semantic and make tsx=auto a default which is more update friendly.

So given the recent clarifications, especially around older MDS_NO=0 CPUs, I think it'd actually make more sense to turn this into a config option that would simply allow to chose what the default behavior should be, chosing from tristate off/on/auto.

arch/x86/Kconfig +7 -0

+config X86_INTEL_TSX_MODE_OFF
+	bool "off"
+	help
+	  TSX is always disabled - equals tsx=off command line parameter.
+
+config X86_INTEL_TSX_MODE_ON
+	bool "on"

On Wed 16-10-19 09:54:34, speck for Michal Hocko wrote:
[...]

Quoted 7 lines

arch/x86/Kconfig +7 -0

+config X86_INTEL_TSX_MODE_OFF
+	bool "off"
+	help
+	  TSX is always disabled - equals tsx=off command line parameter.
+
+config X86_INTEL_TSX_MODE_ON
+	bool "on"

arch/x86/Kconfig config X86_INTEL_ENABLE_SAFE_TSX · line 1986

Quoted 10 lines

arch/x86/Kconfig +10 -0

+	help
+	  TSX is always enabled on TSX capable HW - equals tsx=on command line
+	  parameter.
+
+config X86_INTEL_TSX_MODE_AUTO
+	bool "auto"
+	help
+	  TSX is enabled on TSX capable HW that is believed to be safe against
+	  side channel attacks- equals tsx=auto command line parameter.
+endchoice

I have just reread the discussion again and I have to say that there is more confusion and uncertainty than I would like to see. Can somebody from Intel give an authoritative answer to the following question please?

Is tsx=auto going to lead to any different state than tsx=on? In other words does auto mode make any sense at all?

Quoted 25 lines · speck for Michal Hocko

On Wed 16-10-19 09:54:34, speck for Michal Hocko wrote:
[...]

Quoted 7 lines

arch/x86/Kconfig +7 -0

+config X86_INTEL_TSX_MODE_OFF
+	bool "off"
+	help
+	  TSX is always disabled - equals tsx=off command line parameter.
+
+config X86_INTEL_TSX_MODE_ON
+	bool "on"

arch/x86/Kconfig config X86_INTEL_ENABLE_SAFE_TSX · line 1986

Quoted 10 lines

arch/x86/Kconfig +10 -0

+	help
+	  TSX is always enabled on TSX capable HW - equals tsx=on command line
+	  parameter.
+
+config X86_INTEL_TSX_MODE_AUTO
+	bool "auto"
+	help
+	  TSX is enabled on TSX capable HW that is believed to be safe against
+	  side channel attacks- equals tsx=auto command line parameter.
+endchoice

I have just reread the discussion again and I have to say that there is more confusion and uncertainty than I would like to see. Can somebody from Intel give an authoritative answer to the following question please?

Quoted 2 lines

Is tsx=auto going to lead to any different state than tsx=on? In other words does auto mode make any sense at all?

Is it only my vacation induced spark of mental sanity or is this whole TAA thing a complete trainwreck again?

We're at version 5 and more than 3 month since the first RFC got posted and we are still debating which CPUs are affected and which migitations are going to be deployed depending on the CPU advertized misfeatures and the eventually surfacing microcode?

Can we please stop this complete waste of time right now and start over with clarifying the situation? I.e. someone at Intel needs to sit down and write up a matrix:

TAA-Affected | MDS_NO | VERW works | TSX_MSR | Resulting action
-------------|--------|------------|---------|-----------------
 No          |   X    |     X      |    Y    | ?
 No          |   X    |     X      |    N    | None
 Yes         |   0    |     0      |    0    | SNAFU
 ...         |        |            |         |

You surely can fill in the rest on your own, right?

And exactly that information wants to be in the admin documentation or in a separate Documentation/x86/taa.rst as well.

Quoted 2 lines · speck for Michal Hocko

Is tsx=auto going to lead to any different state than tsx=on? In other words does auto mode make any sense at all?

Is it only my vacation induced spark of mental sanity or is this whole TAA thing a complete trainwreck again?

We're at version 5 and more than 3 month since the first RFC got posted and we are still debating which CPUs are affected and which migitations are going to be deployed depending on the CPU advertized misfeatures and the eventually surfacing microcode?

Can we please stop this complete waste of time right now and start over with clarifying the situation? I.e. someone at Intel needs to sit down and write up a matrix:

TAA-Affected | MDS_NO | VERW works | TSX_MSR | Resulting action
-------------|--------|------------|---------|-----------------
 No          |   X    |     X      |    Y    | ?
 No          |   X    |     X      |    N    | None
 Yes         |   0    |     0      |    0    | SNAFU
 ...         |        |            |         |

You surely can fill in the rest on your own, right?

And exactly that information wants to be in the admin documentation or in a separate Documentation/x86/taa.rst as well.

On Wed, Oct 16, 2019 at 02:15:08PM +0200, speck for Thomas Gleixner wrote:
Below is the matrix for the impact of tsx=<on|off|auto> cmdline options on state of TAA mitigation, VERW behavior and TSX feature for various combinations of MSR_IA32_ARCH_CAPABILITIES bits.

1. tsx=auto

+----------+----------+----------------+---------------+--------------+-------------------+
|  MSR_IA32_ARCH_CAPABILITIES bits     |           Result with cmdline tsx=auto           |
+----------+----------+----------------+---------------+--------------+-------------------+
|  TAA_NO  |  MDS_NO  |  TSX_CTRL_MSR  |  VERW clears  |  TSX state   |  TAA mitigation   |
|          |          |                |  CPU buffers  | after bootup |                   |
+==========+==========+================+===============+==============+===================+
|    0     |    0     |       0        |      Yes      |  HW default  |    Same as MDS    |
+----------+----------+----------------+---------------+--------------+-------------------+
|    0     |    0     |       1        | Invalid case  | Invalid case |   Invalid case    |
+----------+----------+----------------+---------------+--------------+-------------------+

Let me know if there are any questions.

On Thu 17-10-19 17:14:07, speck for Pawan Gupta wrote:
[...]

Quoted 1 line

Let me know if there are any questions.

is there any list of CPUs that would benefit from tsx=auto compared to tsx=on? In other words do we really need both?

Quoted 6 lines · speck for Michal Hocko

On Thu 17-10-19 17:14:07, speck for Pawan Gupta wrote:
[...]

Quoted 1 line

Let me know if there are any questions.

is there any list of CPUs that would benefit from tsx=auto compared to tsx=on? In other words do we really need both?

Below is the list of TAA affected CPUs that will get TSX_CTRL MSR.

+----------------------------+------------------+------------+
|            Name            |  Family / model  |  Stepping  |
+============================+==================+============+
| Whiskey Lake (ULT refresh) |      06_8E       |    0xC     |
+----------------------------+------------------+------------+
|    2nd gen Cascade Lake    |      06_55       |    6, 7    |
+----------------------------+------------------+------------+
|       Coffee Lake R        |      06_9E       |    0xD     |
+----------------------------+------------------+------------+

tsx=auto on these CPUs will disable TSX.
tsx=on will keep TSX enabled.

Quoted 20 lines · speck for Pawan Gupta

Quoted 6 lines · speck for Michal Hocko

On Thu 17-10-19 17:14:07, speck for Pawan Gupta wrote:
[...]

Quoted 1 line

Let me know if there are any questions.

is there any list of CPUs that would benefit from tsx=auto compared to tsx=on? In other words do we really need both?

Below is the list of TAA affected CPUs that will get TSX_CTRL MSR.

+----------------------------+------------------+------------+
|            Name            |  Family / model  |  Stepping  |
+============================+==================+============+
| Whiskey Lake (ULT refresh) |      06_8E       |    0xC     |
+----------------------------+------------------+------------+
|    2nd gen Cascade Lake    |      06_55       |    6, 7    |
+----------------------------+------------------+------------+
|       Coffee Lake R        |      06_9E       |    0xD     |
+----------------------------+------------------+------------+

Quoted 2 lines

tsx=auto on these CPUs will disable TSX.
tsx=on will keep TSX enabled.

Quoted 21 lines · Josh Poimboeuf

Quoted 20 lines · speck for Pawan Gupta

Quoted 6 lines · speck for Michal Hocko

On Thu 17-10-19 17:14:07, speck for Pawan Gupta wrote:
[...]

Quoted 1 line

Let me know if there are any questions.

is there any list of CPUs that would benefit from tsx=auto compared to tsx=on? In other words do we really need both?

Below is the list of TAA affected CPUs that will get TSX_CTRL MSR.

+----------------------------+------------------+------------+
|            Name            |  Family / model  |  Stepping  |
+============================+==================+============+
| Whiskey Lake (ULT refresh) |      06_8E       |    0xC     |
+----------------------------+------------------+------------+
|    2nd gen Cascade Lake    |      06_55       |    6, 7    |
+----------------------------+------------------+------------+
|       Coffee Lake R        |      06_9E       |    0xD     |
+----------------------------+------------------+------------+

Quoted 2 lines

Quoted 2 lines

tsx=auto on these CPUs will disable TSX.
tsx=on will keep TSX enabled.

[ Please ignore my previous email, it had a glaring typo ]

Just to clarify, is this list identical to the list of CPUs which are vulnerable to TAA and have MDS_NO=1?

Quoted 20 lines · speck for Josh Poimboeuf

Quoted 19 lines · Josh Poimboeuf

Quoted 18 lines · speck for Pawan Gupta

Quoted 5 lines · speck for Michal Hocko

On Thu 17-10-19 17:14:07, speck for Pawan Gupta wrote:
[...]

Quoted 1 line

Let me know if there are any questions.

is there any list of CPUs that would benefit from tsx=auto compared to tsx=on? In other words do we really need both?

Below is the list of TAA affected CPUs that will get TSX_CTRL MSR.

+----------------------------+------------------+------------+
|            Name            |  Family / model  |  Stepping  |
+============================+==================+============+
| Whiskey Lake (ULT refresh) |      06_8E       |    0xC     |
+----------------------------+------------------+------------+
|    2nd gen Cascade Lake    |      06_55       |    6, 7    |
+----------------------------+------------------+------------+
|       Coffee Lake R        |      06_9E       |    0xD     |
+----------------------------+------------------+------------+

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

tsx=auto on these CPUs will disable TSX.
tsx=on will keep TSX enabled.

[ Please ignore my previous email, it had a glaring typo ]

Just to clarify, is this list identical to the list of CPUs which are vulnerable to TAA and have MDS_NO=1?

That is correct.

What about Commit Lake parts, because that is explicitly called out in the list of MDS_NO=1, TAA-vulnerable parts in initial whitepaper about TAA.

Quoted 2 lines · speck for Andrew Cooper

What about Commit Lake parts, because that is explicitly called out in the list of MDS_NO=1, TAA-vulnerable parts in initial whitepaper about TAA.

We've dropped Comet Lake from that list (someone pointed out that it doesn't have TSX).

Quoted 5 lines · speck for Thomas Gleixner

The real question is whether machines on which SAP/HANA runs are just random boxes/VMs which would be affected by the 'update and reboot world' treatment or whether SAP/HANA runs on dedicated systems which are carefully administrated by sysadmins with brains turned on.

Quoted 8 lines

If it's the random box affected by 'update and reboot world' then disabling TSX by default is obviously going to be a source of regressions and we have to bite the bullet.

If not, then the sysadmins will anyway do some very careful evaluation and checking, read documentation and add 'WE_REALLY_WANT_TSX' on the kernel command line. In that case I rather disgruntle a few sysadmins and gain a useful setup for everyone else.

[ if this archive is ever going to be published, please remove this mail
from it, thanks ]

So we talked to SAP about exactly this, and they expressed a real worry along the lines of (just my paraphrase, not really verbatim quote of anybody):

"of course we will put it into a SAP note (*), but realistically, not everybody (by far) is reading those. If this regressess on a handful of platforms, we (together with the OS vendor) could probably handle the followup reports, but if it happens all across the board, it'd surely be overwhelming, to put it mildly. It'll" basically promptly escalate to a support disaster."