TAAv7 0

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 00/10] TAAv7

Changes since v6:
- Add Michal's patch to allow tsx=on|off|auto via CONFIG
- Rebase to v5.4-rc4
- Changelog, comments and documentation update.

Changes since v5:
- Remove unsafe X86_FEATURE_RTM toggles.

Quoted 8 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 00/10] TAAv7

Changes since v6:
- Add Michal's patch to allow tsx=on|off|auto via CONFIG
- Rebase to v5.4-rc4
- Changelog, comments and documentation update.

Quoted 2 lines

Changes since v5:
- Remove unsafe X86_FEATURE_RTM toggles.

I'm wondering if maybe these patches shouldn't touch the cpu capabilities at all. After all, even with TSX toggled off, TSX is still present -- XBEGIN doesn't give #UD.

Quoted 8 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 00/10] TAAv7

Changes since v6:
- Add Michal's patch to allow tsx=on|off|auto via CONFIG
- Rebase to v5.4-rc4
- Changelog, comments and documentation update.

Quoted 2 lines

Changes since v5:
- Remove unsafe X86_FEATURE_RTM toggles.

I'm wondering if maybe these patches shouldn't touch the cpu capabilities at all. After all, even with TSX toggled off, TSX is still present -- XBEGIN doesn't give #UD. By making this change, we avoid needing to even consider what happens when a cpu capability bit changes after boot.

I think it would be nice to expose whether TSX is enabled somewhere in sysfs, but I'm not convinced that "features" is the place for it.

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 07/10] x86/tsx: Add "auto" option to TSX cmdline parameter

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.

Subject: Re: [PATCH v7 07/10] TAAv7 7

Quoted 6 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 07/10] x86/tsx: Add "auto" option to TSX cmdline parameter

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.

Earlier, you do this:

(quoted diff) +4 -0

+	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
+	    (boot_cpu_has(X86_FEATURE_RTM) ||
+	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
+		setup_force_cpu_bug(X86_BUG_TAA);

Per the other discussion, I think you want to double check if tsx=auto is doing what folks want it to do, because currently I think auto still has the semantics of turning off TSX on everything, rather than just those cases where a VERW mitigation won't suffice/is in use.

(see Thomas's update to the "Re: [PATCH v5 08/11] TAAv5 8" diagram)

It seems that it's a good time to double check if that's what everyone on the distro side is expecting, namely "tsx=auto will disable TSX automatically on those parts impacted by TAA for which VERW mitigation is not available".

This seems to reduce the set where we end up
disabling TSX essentially to a few very recent processors (e.g. CascadeLake some second gen, Bx stepping?). If that's what we are all expecting/are planning to do, I suspect Red Hat would go with tsx=auto as a default since the set of impacted processors can be documented.

Anyway, auto isn't right yet.

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 07/10] x86/tsx: Add "auto" option to TSX cmdline parameter

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                       | 5 +++++
 2 files changed, 10 insertions(+)

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

index ad6b69057bb0..4d63dc8dc2fc 100644
@@ -4856,6 +4856,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 +5 -0

index d031a8b6fb0e..9d7d3ad324b7 100644
@@ -79,6 +79,11 @@ void __init tsx_init(void)
 			tsx_ctrl_state = TSX_CTRL_ENABLE;
 		} else if (!strcmp(arg, "off")) {
 			tsx_ctrl_state = TSX_CTRL_DISABLE;
+		} else if (!strcmp(arg, "auto")) {
+			if (boot_cpu_has_bug(X86_BUG_TAA))
+				tsx_ctrl_state = TSX_CTRL_DISABLE;
+			else
+				tsx_ctrl_state = TSX_CTRL_ENABLE;
 		} else {
 			tsx_ctrl_state = TSX_CTRL_DISABLE;
 			pr_err("tsx: invalid option, defaulting to off\n");

Signature · 1 line

2.20.1

Quoted 7 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 07/10] x86/tsx: Add "auto" option to TSX cmdline parameter

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.

Quoted 2 lines

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

Considering how the whole thing is confusing could you please explicitly state CPU models where auto is different from on as you did in an email in previous email thread?

On Mon 21-10-19 13:25:02, speck for Pawan Gupta wrote:
[...]

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Please explicitly mention that off is active only if TSX there is ucode support for that (aka MSR_IA32_TSX_CTRL).

Quoted 13 lines · speck for Michal Hocko

On Mon 21-10-19 13:25:02, speck for Pawan Gupta wrote:
[...]

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Please explicitly mention that off is active only if TSX there is ucode support for that (aka MSR_IA32_TSX_CTRL).

I'm pretty sure I already asked for this in the last revision. And this is not the first time I've seen ignored feedback. Pawan, please try to take all feedback into account so we don't have to keep repeating ourselves.

Quoted 15 lines · speck for Josh Poimboeuf

Quoted 13 lines · speck for Michal Hocko

On Mon 21-10-19 13:25:02, speck for Pawan Gupta wrote:
[...]

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Please explicitly mention that off is active only if TSX there is ucode support for that (aka MSR_IA32_TSX_CTRL).

Quoted 4 lines

I'm pretty sure I already asked for this in the last revision. And this is not the first time I've seen ignored feedback. Pawan, please try to take all feedback into account so we don't have to keep repeating ourselves.

I am sorry to have missed it. All of the operation on|off|auto are dependent on TSX control being present. The tsx= description states that processors need TSX control support. I would extend it to specifically say ucode update adds TSX control support (aka MSR_IA32_TSX_CTRL).

tsx= [X86] Control Transactional Synchronization
Extensions (TSX) feature in Intel processors that support TSX control.

Quoted 21 lines · speck for Pawan Gupta

Quoted 19 lines · speck for Josh Poimboeuf

Quoted 13 lines · speck for Michal Hocko

On Mon 21-10-19 13:25:02, speck for Pawan Gupta wrote:
[...]

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Please explicitly mention that off is active only if TSX there is ucode support for that (aka MSR_IA32_TSX_CTRL).

I'm pretty sure I already asked for this in the last revision. And this is not the first time I've seen ignored feedback. Pawan, please try to take all feedback into account so we don't have to keep repeating ourselves.

Quoted 4 lines

I am sorry to have missed it. All of the operation on|off|auto are dependent on TSX control being present. The tsx= description states that processors need TSX control support. I would extend it to specifically say ucode update adds TSX control support (aka MSR_IA32_TSX_CTRL).

Thanks! An explicit note about the MSR is important because that is something people can google for.

arch/x86/kernel/cpu/common.c void __init identify_boot_cpu(void) · line 1567

Quoted 12 lines · speck for Pawan Gupta

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

index 897c8302d982..885d4ac2111a 100644
@@ -1561,6 +1561,8 @@ void __init identify_boot_cpu(void)
 #endif
 	cpu_detect_tlb(&boot_cpu_data);
 	setup_cr_pinning();
+
+	tsx_init();
 }
 

Why is this called here instead of in check_bugs()?

arch/x86/kernel/cpu/tsx.c

Quoted 5 lines · speck for Pawan Gupta

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

new file mode 100644
index 000000000000..d031a8b6fb0e

Maybe I've missed it in the mail overflow but why is this file being created?

Is arch/x86/kernel/cpu/bugs.c too big all of a sudden or is there some other thought behind separating the TSX stuff?

Again this is as per the feedback to create a new file for TSX as it is a feature. TAA stuff stays in bugs.c.

Quoted 2 lines · speck for Pawan Gupta

Again this is as per the feedback to create a new file for TSX as it is a feature. TAA stuff stays in bugs.c.

Well, with patchsets like that, it is always good to write a more detailed changelog so that questions like that are answered from it.

Your 0/x message has

"- Refactor TSX code into new tsx.c"

but it would be a lot more helpful if it had the "why" you did it:

" - Carve out the TSX disabling functionality into a separate compilation unit because it is a CPU feature."

This sentence could be a part of the patch commit message even.

Review

Quoted 3 lines · speck for Pawan Gupta

(quoted diff) +3 -0

+void __init tsx_init(void)
+{
+	char arg[20];

Why 20? Magic?

Why not simply 4? We have "on" and "off" + '\0' AFAICT, no?

I will change it to 5 to accommodate "auto" too.

(quoted diff) +7 -0

@@ -268,6 +270,110 @@ static int __init mds_cmdline(char *str)
 }
 early_param("mds", mds_cmdline);
 
+#undef pr_fmt
+#define pr_fmt(fmt)	"TAA: " fmt
+
+/* Default mitigation for TAA-affected CPUs */
+static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW;
+static bool taa_nosmt __ro_after_init;
+

(quoted diff) +4 -0

+	if (!boot_cpu_has(X86_FEATURE_RTM)) {
+		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
+		pr_info("%s\n", taa_strings[taa_mitigation]);
+		return;

(quoted diff) +26 -0

+		pr_info("%s\n", taa_strings[taa_mitigation]);
+		return;
+	}
+
+	if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
+		taa_mitigation = TAA_MITIGATION_VERW;
+	else
+		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
+
+	/*
+	 * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and MDS_NO=1.
+	 * A microcode update fixes this behavior to clear CPU buffers.
+	 * Microcode update also adds support for MSR_IA32_TSX_CTRL which
+	 * is enumerated by ARCH_CAP_TSX_CTRL_MSR bit.
+	 *
+	 * On MDS_NO=1 CPUs if ARCH_CAP_TSX_CTRL_MSR is not set, microcode
+	 * update is required.
+	 */
+	if ((ia32_cap & ARCH_CAP_MDS_NO) &&
+	    !(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
+		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
+
+	/*
+	 * TSX is enabled, select alternate mitigation for TAA which is
+	 * same as MDS. Enable MDS static branch to clear CPU buffers.
+	 *

(quoted diff) +9 -0

+{
+	if (!boot_cpu_has_bug(X86_BUG_TAA))
+		return 0;
+
+	if (!str)
+		return -EINVAL;
+
+	if (!strcmp(str, "off")) {
+		taa_mitigation = TAA_MITIGATION_OFF;

(quoted diff) +20 -2

+
 #undef pr_fmt
 #define pr_fmt(fmt)     "Spectre V1 : " fmt
 

@@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
 #undef pr_fmt
 #define pr_fmt(fmt) fmt
 
-/* 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)
 {
 	/*

@@ -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;
 
 	if (sched_smt_active())

@@ -786,6 +895,7 @@ static void update_mds_branch_idle(void)
 }
 
 #define MDS_MSG_SMT "MDS CPU bug present and SMT on, data leak possible. See https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/mds.html for more details.\n"
+#define TAA_MSG_SMT "TAA CPU bug present and SMT on, data leak possible. See https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/tsx_async_abort.html for more details.\n"
 
 void cpu_bugs_smt_update(void)
 {

@@ -819,6 +929,19 @@ void cpu_bugs_smt_update(void)
 		break;
 	}
 
+	switch (taa_mitigation) {
+	case TAA_MITIGATION_VERW:
+	case TAA_MITIGATION_UCODE_NEEDED:
+		if (sched_smt_active())
+			pr_warn_once(TAA_MSG_SMT);
+		/* TSX is enabled, apply MDS idle buffer clearing. */
+		update_mds_branch_idle();
+		break;
+	case TAA_MITIGATION_TSX_DISABLE:
+	case TAA_MITIGATION_OFF:
+		break;
+	}
+
 	mutex_unlock(&spec_ctrl_mutex);
 }
 

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

index 885d4ac2111a..86f22c1e5912 100644

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

+	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
+		setup_force_cpu_bug(X86_BUG_TAA);
+
 	if (cpu_matches(NO_MELTDOWN))
 		return;
 

Signature · 1 line

2.20.1

Quoted 9 lines

(quoted diff) +9 -0

+{
+	if (!boot_cpu_has_bug(X86_BUG_TAA))
+		return 0;
+
+	if (!str)
+		return -EINVAL;
+
+	if (!strcmp(str, "off")) {
+		taa_mitigation = TAA_MITIGATION_OFF;

Review

Quoted 10 lines

(quoted diff) +10 -0

+	} else if (!strcmp(str, "full")) {
+		taa_mitigation = TAA_MITIGATION_VERW;
+	} else if (!strcmp(str, "full,nosmt")) {
+		taa_mitigation = TAA_MITIGATION_VERW;
+		taa_nosmt = true;
+	}
+
+	return 0;
+}
+early_param("tsx_async_abort", tsx_async_abort_cmdline);

Say what now?!

The previous patch added this:

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.

So what is the final name of command line option now?

Quoted 10 lines

Quoted 1 line

+early_param("tsx_async_abort", tsx_async_abort_cmdline);

Say what now?!

The previous patch added this:

tsx= [X86] Control Transactional Synchronization
Extensions (TSX) feature in Intel processors that support TSX control.

Quoted 6 lines

This parameter controls the TSX feature. The options are:

on - Enable TSX on the system.
off - Disable TSX on the system.

So what is the final name of command line option now?

Yah, Josh just set me straight: there's two flags. Yuck. I'm willing to bet I won't be the only one to get confused by this.

tsx=
taa=

is probably marginally better but someone might have a better idea.

It was called taa= in earlier versions. There was a feedback to change it to tsx_async_abort=.

tsx_async_abort is consistent with sysfs file.

/sys/devices/system/cpu/vulnerabilities/tsx_async_abort

Quoted 2 lines · speck for Pawan Gupta

It was called taa= in earlier versions. There was a feedback to change it to tsx_async_abort=.

I know, I'm just saying it is confusing.

Quoted 3 lines

tsx_async_abort is consistent with sysfs file.

/sys/devices/system/cpu/vulnerabilities/tsx_async_abort

Well, that's not really an argument.

It might have been Linus that objected to "taa" (but I can't recall if it was for the sysfs file name, or the boot argument)

Rationale was that in a couple of years time nobody will remember what "taa" stands for.

Or, a completely different idea: I wonder if we could merge the two options like we do for l1tf= for example where we have

	l1tf=<opt1>,<opt2>

and then do:

tsx=on,async_abort_full
tsx=on,async_abort_full,nosmt
tsx=off

That should even diminish the number of combinations because once you've supplied "tsx=off" for example, async_abort doesn't matter.

Quoted 11 lines · speck for Borislav Petkov

Or, a completely different idea: I wonder if we could merge the two options like we do for l1tf= for example where we have

	l1tf=<opt1>,<opt2>

and then do:

tsx=on,async_abort_full
tsx=on,async_abort_full,nosmt
tsx=off

Quoted 2 lines

That should even diminish the number of combinations because once you've supplied "tsx=off" for example, async_abort doesn't matter.

At first glance I find that more confusing that helpful.

Perspective: TAA is an issue that affects ~3 CPU models. It will be a non-issue on future models.

TSX control is a new CPU feature control that happens to begin with those three models, but will continue to be present on future CPU models.

At first glance I find that more confusing that helpful.

Perspective: TAA is an issue that affects ~3 CPU models. It will be a non-issue on future models.

TSX control is a new CPU feature control that happens to begin with those three models, but will continue to be present on future CPU models.

Quoted 8 lines · Tony

At first glance I find that more confusing that helpful.

Perspective: TAA is an issue that affects ~3 CPU models. It will be a non-issue on future models.

TSX control is a new CPU feature control that happens to begin with those three models, but will continue to be present on future CPU models.

The ",async_abort..." piece is optional, of course. You should be able to use

tsx=on|off

just fine, without the additional option flags. I.e., you get what you ordered principle.

It is the same thing as when you don't specify tsx_async_abort= on the command line now.

I'm not sure what you mean, I think the patches mitigate TAA by default when TSX is on (ignoring SMT of course).

Also, suspending disbelief for a moment and assuming TSX becomes a huge success story and you want to safely enable it 5 years down the road, you'd have to do

tsx=on,tsx_async_abort=full,nosmt,tsx_bug2=full,nosmt .... etc

when you just want to turn the darned thing on without having to worry about specifying all the mitigations for all currently known bugs.

Quoted 9 lines · speck for Josh Poimboeuf

I'm not sure what you mean, I think the patches mitigate TAA by default when TSX is on (ignoring SMT of course).

Also, suspending disbelief for a moment and assuming TSX becomes a huge success story and you want to safely enable it 5 years down the road, you'd have to do

tsx=on,tsx_async_abort=full,nosmt,tsx_bug2=full,nosmt .... etc

Quoted 2 lines

when you just want to turn the darned thing on without having to worry about specifying all the mitigations for all currently known bugs.

But when you want to enable it 5 years from now, you simply do

tsx=on

Why would you even need to supply anything after it?

Because my 4-year-old CPU will need the mitigations ;-)

Quoted 11 lines · speck for Pawan Gupta

(quoted diff) +7 -0

@@ -268,6 +270,110 @@ static int __init mds_cmdline(char *str)
 }
 early_param("mds", mds_cmdline);
 
+#undef pr_fmt
+#define pr_fmt(fmt)	"TAA: " fmt
+
+/* Default mitigation for TAA-affected CPUs */
+static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW;
+static bool taa_nosmt __ro_after_init;
+

Review static int __init mds_cmdline(char *str) · line 289

Quoted 10 lines

(quoted diff) +10 -0

+static const char * const taa_strings[] = {
+	[TAA_MITIGATION_OFF]		= "Vulnerable",
+	[TAA_MITIGATION_UCODE_NEEDED]	= "Vulnerable: Clear CPU buffers attempted, no microcode",
+	[TAA_MITIGATION_VERW]		= "Mitigation: Clear CPU buffers",
+	[TAA_MITIGATION_TSX_DISABLE]	= "Mitigation: TSX disabled",
+};
+
+static void __init taa_select_mitigation(void)
+{
+	u64 ia32_cap = x86_read_arch_cap_msr();

Do that MSR access...

Review static int __init mds_cmdline(char *str) · line 294

Quoted 5 lines

(quoted diff) +5 -0

+
+	if (!boot_cpu_has_bug(X86_BUG_TAA)) {
+		taa_mitigation = TAA_MITIGATION_OFF;
+		return;
+	}

... here.

Review static int __init mds_cmdline(char *str) · line 298

Quoted 4 lines

(quoted diff) +4 -0

+
+	/*
+	 * As X86_BUG_TAA=1, TSX feature is supported by the hardware. If
+	 * TSX was disabled (X86_FEATURE_RTM=0) earlier during tsx_init().

That sentence is having trouble saying whatever it is trying to say. :)

Quoted 4 lines

(quoted diff) +4 -0

+	if (!boot_cpu_has(X86_FEATURE_RTM)) {
+		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
+		pr_info("%s\n", taa_strings[taa_mitigation]);
+		return;

Review static int __init mds_cmdline(char *str) · line 312

Quoted 10 lines

(quoted diff) +10 -0

+	}
+
+	/* All mitigations turned off from cmdline (mitigations=off) */
+	if (cpu_mitigations_off()) {
+		taa_mitigation = TAA_MITIGATION_OFF;
+		return;
+	}
+
+	/* TAA mitigation is turned off from cmdline (tsx_async_abort=off) */
+	if (taa_mitigation == TAA_MITIGATION_OFF) {

		goto out;

and you slap an out label above the pr_info at the end of this function.

To show better what I mean, here's the whole function fixed up:

static void __init taa_select_mitigation(void)

{
	u64 ia32_cap;

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
		goto out;
	}

	if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off()) {
		taa_mitigation = TAA_MITIGATION_OFF;
		goto out;
	}

Review static int __init mds_cmdline(char *str) · line 314

Quoted 2 lines

(quoted diff) +2 -0

+
+static int __init tsx_async_abort_cmdline(char *str)

That function name needs a verb: tsx_async_abort_parse_cmdline()

Quoted 10 lines

(quoted diff) +9 -0

+{
+	if (!boot_cpu_has_bug(X86_BUG_TAA))
+		return 0;
+
+	if (!str)
+		return -EINVAL;
+
+	if (!strcmp(str, "off")) {
+		taa_mitigation = TAA_MITIGATION_OFF;

Review static int __init mds_cmdline(char *str) · line 333

Quoted 10 lines

(quoted diff) +10 -0

+	} else if (!strcmp(str, "full")) {
+		taa_mitigation = TAA_MITIGATION_VERW;
+	} else if (!strcmp(str, "full,nosmt")) {
+		taa_mitigation = TAA_MITIGATION_VERW;
+		taa_nosmt = true;
+	}
+
+	return 0;
+}
+early_param("tsx_async_abort", tsx_async_abort_cmdline);

Say what now?!

The previous patch added this:

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.

So what is the final name of command line option now?

Quoted 9 lines

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

index 885d4ac2111a..86f22c1e5912 100644
@@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (!cpu_matches(NO_SWAPGS))
 		setup_force_cpu_bug(X86_BUG_SWAPGS);
 

arch/x86/kernel/cpu/common.c static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) · line 1134

Quoted 4 lines

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

+	/*
+	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
+	 *	- TSX is supported or
+	 *	- TSX_CTRL is supported

s/supported/present/

arch/x86/kernel/cpu/common.c static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) · line 1142

Quoted 9 lines

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

+	 *
+	 * TSX_CTRL check is needed for cases when TSX could be disabled before
+	 * the kernel boot e.g. kexec
+	 * TSX_CTRL check alone is not sufficient for cases when the microcode
+	 * update is not present or running as guest that don't get TSX_CTRL.
+	 */
+	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
+	    (boot_cpu_has(X86_FEATURE_RTM) ||

You have a @c parameter passed in, use it:

	    cpu_has(c, X86_FEATURE_RTM)

There is a small problem with this, it will set

	taa_mitigation = TAA_MITIGATION_TSX_DISABLE;

when there is no X86_BUG_TAA.

goto will cause it to differ from other mitigations that don't print individual mitigation status when mitigations are turned off globally (when cpu_mitigations_off() is true).

Quoted 5 lines · speck for Pawan Gupta

There is a small problem with this, it will set

	taa_mitigation = TAA_MITIGATION_TSX_DISABLE;

when there is no X86_BUG_TAA.

And?

When the CPU doesn't support TSX, then this practically is "Mitigation:
TSX disabled" because, well, TSX *is* disabled.

Also, from all the possible settings:

        [TAA_MITIGATION_OFF]            = "Vulnerable",
        [TAA_MITIGATION_UCODE_NEEDED]   = "Vulnerable: Clear CPU buffers attempted, no microcode",
        [TAA_MITIGATION_VERW]           = "Mitigation: Clear CPU buffers",
        [TAA_MITIGATION_TSX_DISABLE]    = "Mitigation: TSX disabled",

TAA_MITIGATION_TSX_DISABLE is the one that fits best for the !RTM case, no?

Side effect of RTM check ahead of X86_BUG_TAA will be a dmesg print "Mitigation: TSX disabled" when X86_BUG_TAA is not set.

Quoted 9 lines

Also, from all the possible settings:

        [TAA_MITIGATION_OFF]            = "Vulnerable",
        [TAA_MITIGATION_UCODE_NEEDED]   = "Vulnerable: Clear CPU buffers attempted, no microcode",
        [TAA_MITIGATION_VERW]           = "Mitigation: Clear CPU buffers",
        [TAA_MITIGATION_TSX_DISABLE]    = "Mitigation: TSX disabled",

TAA_MITIGATION_TSX_DISABLE is the one that fits best for the !RTM case, no?

When X86_BUG_TAA is not set sysfs shows "Not affected" irrespective of value of taa_mitigation.

	cpu_show_common()
	{
		if (!boot_cpu_has_bug(bug))
			return sprintf(buf, "Not affected\n");
		[...]

Quoted 2 lines · speck for Pawan Gupta

Side effect of RTM check ahead of X86_BUG_TAA will be a dmesg print "Mitigation: TSX disabled" when X86_BUG_TAA is not set.

So what you're trying to tell me is that you don't want to print that message at all?

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
		return;
	}

How's that?

Quoted 10 lines

Quoted 9 lines

Also, from all the possible settings:

        [TAA_MITIGATION_OFF]            = "Vulnerable",
        [TAA_MITIGATION_UCODE_NEEDED]   = "Vulnerable: Clear CPU buffers attempted, no microcode",
        [TAA_MITIGATION_VERW]           = "Mitigation: Clear CPU buffers",
        [TAA_MITIGATION_TSX_DISABLE]    = "Mitigation: TSX disabled",

TAA_MITIGATION_TSX_DISABLE is the one that fits best for the !RTM case, no?

Quoted 8 lines

When X86_BUG_TAA is not set sysfs shows "Not affected" irrespective of value of taa_mitigation.

	cpu_show_common()
	{
		if (!boot_cpu_has_bug(bug))
			return sprintf(buf, "Not affected\n");
		[...]

Sorry, I can't follow what you're trying to tell me here.

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · speck for Pawan Gupta

Side effect of RTM check ahead of X86_BUG_TAA will be a dmesg print "Mitigation: TSX disabled" when X86_BUG_TAA is not set.

So what you're trying to tell me is that you don't want to print that message at all?

Quoted 6 lines

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
		return;
	}

How's that?

And to perhaps answer your other question from below maybe - I'm assuming you want something like this:

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
		setup_clear_cpu_bug(X86_BUG_TAA);
		return;
	}

This is assuming X86_FEATURE_RTM really mirrors the CPUID bit. And that should be the case since if you do tsx_disable(), it will clear the CPUID bit through the MSR. IOW, when TSX is disabled, you're basically not affected by TAA, of course.

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

Also, from all the possible settings:

        [TAA_MITIGATION_OFF]            = "Vulnerable",
        [TAA_MITIGATION_UCODE_NEEDED]   = "Vulnerable: Clear CPU buffers attempted, no microcode",
        [TAA_MITIGATION_VERW]           = "Mitigation: Clear CPU buffers",
        [TAA_MITIGATION_TSX_DISABLE]    = "Mitigation: TSX disabled",

TAA_MITIGATION_TSX_DISABLE is the one that fits best for the !RTM case, no?

When X86_BUG_TAA is not set sysfs shows "Not affected" irrespective of value of taa_mitigation.

Quoted 7 lines

Quoted 5 lines

	cpu_show_common()
	{
		if (!boot_cpu_has_bug(bug))
			return sprintf(buf, "Not affected\n");
		[...]

Sorry, I can't follow what you're trying to tell me here.

Quoted 13 lines · speck for Borislav Petkov

Quoted 12 lines · Borislav Petkov

Quoted 2 lines · speck for Pawan Gupta

Side effect of RTM check ahead of X86_BUG_TAA will be a dmesg print "Mitigation: TSX disabled" when X86_BUG_TAA is not set.

So what you're trying to tell me is that you don't want to print that message at all?

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
		return;
	}

Quoted 8 lines

Quoted 1 line

How's that?

And to perhaps answer your other question from below maybe - I'm assuming you want something like this:

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
		setup_clear_cpu_bug(X86_BUG_TAA);

I think we first need to agree on whether disabling TSX is the mitigation for X86_BUG_TAA or the lack of bug. Your clearing the bug bit here makes me think that you are treating it as lack of bug. This series treats it as a mitigation when the hardware has the bug. Let me know your thoughts on this.

Quoted 5 lines · speck for Pawan Gupta

I think we first need to agree on whether disabling TSX is the mitigation for X86_BUG_TAA or the lack of bug. Your clearing the bug bit here makes me think that you are treating it as lack of bug. This series treats it as a mitigation when the hardware has the bug. Let me know your thoughts on this.

Ok, I see your point. And yes, let's do what you're suggesting because that mirrors reality optimally:

static void __init taa_select_mitigation(void)

{ 
        u64 ia32_cap;

        if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off()) {
                taa_mitigation = TAA_MITIGATION_OFF;
                return;
        }

	if (!boot_cpu_has(X86_FEATURE_RTM)) {
                taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
                goto out;
        }

cpu_mitigations_off() check after X86_FEATURE_RTM check will fix this.

Review

Quoted 3 lines

(quoted diff) +3 -0

+static void __init taa_select_mitigation(void)
+{
+	u64 ia32_cap = x86_read_arch_cap_msr();

Do that MSR access...

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+	if (!boot_cpu_has_bug(X86_BUG_TAA)) {
+		taa_mitigation = TAA_MITIGATION_OFF;
+		return;
+	}

... here.

Is it okay to move x86_read_arch_cap_msr() further down just before its needed?

	if ((ia32_cap & ARCH_CAP_MDS_NO) &&
			!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 01/10] x86/tsx: Add enumeration support for IA32_TSX_CTRL
MSR

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 and HLE feature
(i.e. it will make CPUID(EAX=7).EBX{bit4} and 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}.

How is HLE unconditionally disabled? Is it done by the above mentioned microcode? Is there a way to enable it?

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 01/10] x86/tsx: Add enumeration support for IA32_TSX_CTRL
MSR

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

Quoted 11 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 01/10] x86/tsx: Add enumeration support for IA32_TSX_CTRL
MSR

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

This should clarify that not *all* TAA-vulnerable CPUs will get IA32_TSX_CTRL, instead only the ones which aren't vulnerable to MDS.

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 03/10] x86/tsx: Add TSX cmdline option with TSX disabled by
default

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". This is because on certain processors 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]>

---
 .../admin-guide/kernel-parameters.txt         |  11 ++
 arch/x86/kernel/cpu/Makefile                  |   2 +-
 arch/x86/kernel/cpu/common.c                  |   2 +
 arch/x86/kernel/cpu/cpu.h                     |  18 +++
 arch/x86/kernel/cpu/intel.c                   |   5 +
 arch/x86/kernel/cpu/tsx.c                     | 114 ++++++++++++++++++
 6 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/cpu/tsx.c

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

index a84a83f8881e..ad6b69057bb0 100644
@@ -4848,6 +4848,17 @@
 			interruptions from clocksource watchdog are not
 			acceptable).
 

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 10/10] x86/tsx: Add sysfs interface to control TSX

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.

Regardless, this patch adds complexity and still seems very fragile. It will add maintenance issues and probably introduce bugs. And, most importantly there's *still* not a real world justification for it. Please drop it from the series.

arch/x86/include/asm/processor.h +1 -1

@@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
 #undef pr_fmt
 #define pr_fmt(fmt) fmt
 
-/* 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)
 {
 	/*

It does provide protection against the "store buffer" leak. But the other buffers(fill buffer and load port) are still SMT vulnerable. Do you prefer it removed?

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

index 885d4ac2111a..86f22c1e5912 100644
@@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (!cpu_matches(NO_SWAPGS))
 		setup_force_cpu_bug(X86_BUG_SWAPGS);
 
+	/*
+	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
+	 *	- TSX is supported or
+	 *	- TSX_CTRL is supported
+	 *

arch/x86/include/asm/processor.h enum mds_mitigations { · line 995

Quoted 13 lines · speck for Pawan Gupta

arch/x86/include/asm/processor.h +5 -0

index 6e0a3b43d027..999b85039128 100644
@@ -988,4 +988,11 @@ enum mds_mitigations {
 	MDS_MITIGATION_VMWERV,
 };
 
+enum taa_mitigations {
+	TAA_MITIGATION_OFF,
+	TAA_MITIGATION_UCODE_NEEDED,
+	TAA_MITIGATION_VERW,
+	TAA_MITIGATION_TSX_DISABLE,

I would rename "TSX_DISABLE" to "TSX_DISABLED", because

a) it matches the verb tense of "UCODE_NEEDED", and
b) the past tense hopefully helps make it slightly clearer that TSX was disabled separately, not as part of the mitigation code itself.

Quoted 10 lines

arch/x86/include/asm/processor.h +1 -1

@@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
 #undef pr_fmt
 #define pr_fmt(fmt) fmt
 
-/* 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)
 {
 	/*

arch/x86/include/asm/processor.h static void update_mds_branch_idle(void) · line 889

Quoted 10 lines

arch/x86/include/asm/processor.h +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;

Sorry, I was out for most of the last two weeks, so I think I left you hanging on this. For context, here's your answer to my previous question about whether the X86_BUG_TAA check makes sense here:

Quoted 3 lines

It does provide protection against the "store buffer" leak. But the other buffers(fill buffer and load port) are still SMT vulnerable. Do you prefer it removed?

Yes, please remove it, for the same reason we used for MDS. There's not much point in partially mitigating TAA here.

Quoted 13 lines

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

index 885d4ac2111a..86f22c1e5912 100644
@@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (!cpu_matches(NO_SWAPGS))
 		setup_force_cpu_bug(X86_BUG_SWAPGS);
 
+	/*
+	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
+	 *	- TSX is supported or
+	 *	- TSX_CTRL is supported
+	 *

arch/x86/kernel/cpu/common.c static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) · line 1145

Quoted 10 lines

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

+	 * TSX_CTRL check is needed for cases when TSX could be disabled before
+	 * the kernel boot e.g. kexec
+	 * TSX_CTRL check alone is not sufficient for cases when the microcode
+	 * update is not present or running as guest that don't get TSX_CTRL.
+	 */
+	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
+	    (boot_cpu_has(X86_FEATURE_RTM) ||
+	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
+		setup_force_cpu_bug(X86_BUG_TAA);
+

I'm finding this logic to be less than 100% convincing, or at least hard to follow. And it's different from most of the other bug checks in this function.

Would the following work instead?

	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);

In other words I would presume all the NO_MDS CPUs listed in 'cpu_vuln_whitelist' are also immune to TAA.

Quoted 77 lines · speck for Josh Poimboeuf

arch/x86/include/asm/processor.h enum mds_mitigations { · line 995

Quoted 13 lines · speck for Pawan Gupta

arch/x86/include/asm/processor.h +5 -0

index 6e0a3b43d027..999b85039128 100644
@@ -988,4 +988,11 @@ enum mds_mitigations {
 	MDS_MITIGATION_VMWERV,
 };
 
+enum taa_mitigations {
+	TAA_MITIGATION_OFF,
+	TAA_MITIGATION_UCODE_NEEDED,
+	TAA_MITIGATION_VERW,
+	TAA_MITIGATION_TSX_DISABLE,

I would rename "TSX_DISABLE" to "TSX_DISABLED", because

a) it matches the verb tense of "UCODE_NEEDED", and
b) the past tense hopefully helps make it slightly clearer that TSX was disabled separately, not as part of the mitigation code itself.

Quoted 10 lines

arch/x86/include/asm/processor.h +1 -1

@@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
 #undef pr_fmt
 #define pr_fmt(fmt) fmt
 
-/* 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)
 {
 	/*

arch/x86/include/asm/processor.h static void update_mds_branch_idle(void) · line 889

Quoted 10 lines

arch/x86/include/asm/processor.h +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;

Sorry, I was out for most of the last two weeks, so I think I left you hanging on this. For context, here's your answer to my previous question about whether the X86_BUG_TAA check makes sense here:

Quoted 3 lines

It does provide protection against the "store buffer" leak. But the other buffers(fill buffer and load port) are still SMT vulnerable. Do you prefer it removed?

Yes, please remove it, for the same reason we used for MDS. There's not much point in partially mitigating TAA here.

Quoted 13 lines

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

index 885d4ac2111a..86f22c1e5912 100644
@@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (!cpu_matches(NO_SWAPGS))
 		setup_force_cpu_bug(X86_BUG_SWAPGS);
 
+	/*
+	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
+	 *	- TSX is supported or
+	 *	- TSX_CTRL is supported
+	 *

arch/x86/kernel/cpu/common.c static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) · line 1145

Quoted 10 lines

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

+	 * TSX_CTRL check is needed for cases when TSX could be disabled before
+	 * the kernel boot e.g. kexec
+	 * TSX_CTRL check alone is not sufficient for cases when the microcode
+	 * update is not present or running as guest that don't get TSX_CTRL.
+	 */
+	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
+	    (boot_cpu_has(X86_FEATURE_RTM) ||
+	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
+		setup_force_cpu_bug(X86_BUG_TAA);
+

I'm finding this logic to be less than 100% convincing, or at least hard to follow. And it's different from most of the other bug checks in this function.

Quoted 7 lines

Would the following work instead?

	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);

In other words I would presume all the NO_MDS CPUs listed in 'cpu_vuln_whitelist' are also immune to TAA.

From a leakage point of view, TAA is just another way to access stale date in the load ports, store buffer or fill buffers.

In practice, the leakage mechanism exists on all MDS-vulnerable, TSX-enabled parts.  Parts which are not vulnerable to MDS leakage in the first place will be similarly unaffected.

On pre MDS_NO=1 parts, the existing VERW and no-smt mitigates TAA as well.

On the current MDS_NO=1 parts, the silicon fix constituting MDS_NO took care of the known leakage cases by forcing a register file write-back of 0 rather than the stale data.  However, the TSX Async Abort case was missed, which is why this generation of parts are still vulnerable.

Therefore, the TAA vulnerability only really matters for TSX=1, MDS_NO=1, TAA_NO=0 parts, because these parts are the ones that are leaky, but have a lower overhead mitigation option than VERW and no-SMT.

Quoted 99 lines · speck for Andrew Cooper

Quoted 77 lines · speck for Josh Poimboeuf

arch/x86/include/asm/processor.h enum mds_mitigations { · line 995

Quoted 13 lines · speck for Pawan Gupta

arch/x86/include/asm/processor.h +5 -0

index 6e0a3b43d027..999b85039128 100644
@@ -988,4 +988,11 @@ enum mds_mitigations {
 	MDS_MITIGATION_VMWERV,
 };
 
+enum taa_mitigations {
+	TAA_MITIGATION_OFF,
+	TAA_MITIGATION_UCODE_NEEDED,
+	TAA_MITIGATION_VERW,
+	TAA_MITIGATION_TSX_DISABLE,

I would rename "TSX_DISABLE" to "TSX_DISABLED", because

a) it matches the verb tense of "UCODE_NEEDED", and
b) the past tense hopefully helps make it slightly clearer that TSX was disabled separately, not as part of the mitigation code itself.

Quoted 10 lines

arch/x86/include/asm/processor.h +1 -1

@@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
 #undef pr_fmt
 #define pr_fmt(fmt) fmt
 
-/* 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)
 {
 	/*

arch/x86/include/asm/processor.h static void update_mds_branch_idle(void) · line 889

Quoted 10 lines

arch/x86/include/asm/processor.h +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;

Sorry, I was out for most of the last two weeks, so I think I left you hanging on this. For context, here's your answer to my previous question about whether the X86_BUG_TAA check makes sense here:

Quoted 3 lines

It does provide protection against the "store buffer" leak. But the other buffers(fill buffer and load port) are still SMT vulnerable. Do you prefer it removed?

Yes, please remove it, for the same reason we used for MDS. There's not much point in partially mitigating TAA here.

Quoted 13 lines

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

index 885d4ac2111a..86f22c1e5912 100644
@@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (!cpu_matches(NO_SWAPGS))
 		setup_force_cpu_bug(X86_BUG_SWAPGS);
 
+	/*
+	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
+	 *	- TSX is supported or
+	 *	- TSX_CTRL is supported
+	 *

arch/x86/kernel/cpu/common.c static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) · line 1145

Quoted 10 lines

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

+	 * TSX_CTRL check is needed for cases when TSX could be disabled before
+	 * the kernel boot e.g. kexec
+	 * TSX_CTRL check alone is not sufficient for cases when the microcode
+	 * update is not present or running as guest that don't get TSX_CTRL.
+	 */
+	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
+	    (boot_cpu_has(X86_FEATURE_RTM) ||
+	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
+		setup_force_cpu_bug(X86_BUG_TAA);
+

I'm finding this logic to be less than 100% convincing, or at least hard to follow. And it's different from most of the other bug checks in this function.

Quoted 7 lines

Would the following work instead?

	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);

In other words I would presume all the NO_MDS CPUs listed in 'cpu_vuln_whitelist' are also immune to TAA.

From a leakage point of view, TAA is just another way to access stale date in the load ports, store buffer or fill buffers.

In practice, the leakage mechanism exists on all MDS-vulnerable, TSX-enabled parts.  Parts which are not vulnerable to MDS leakage in the first place will be similarly unaffected.

On pre MDS_NO=1 parts, the existing VERW and no-smt mitigates TAA as well.

On the current MDS_NO=1 parts, the silicon fix constituting MDS_NO took care of the known leakage cases by forcing a register file write-back of 0 rather than the stale data.  However, the TSX Async Abort case was missed, which is why this generation of parts are still vulnerable.

Quoted 3 lines

Therefore, the TAA vulnerability only really matters for TSX=1, MDS_NO=1, TAA_NO=0 parts, because these parts are the ones that are leaky, but have a lower overhead mitigation option than VERW and no-SMT.

I agree with all of that. So are you saying you agree or disagree with my code snippet?

Quoted 103 lines · speck for Josh Poimboeuf

Quoted 102 lines · speck for Andrew Cooper

Quoted 77 lines · speck for Josh Poimboeuf

arch/x86/include/asm/processor.h enum mds_mitigations { · line 995

Quoted 13 lines · speck for Pawan Gupta

arch/x86/include/asm/processor.h +5 -0

index 6e0a3b43d027..999b85039128 100644
@@ -988,4 +988,11 @@ enum mds_mitigations {
 	MDS_MITIGATION_VMWERV,
 };
 
+enum taa_mitigations {
+	TAA_MITIGATION_OFF,
+	TAA_MITIGATION_UCODE_NEEDED,
+	TAA_MITIGATION_VERW,
+	TAA_MITIGATION_TSX_DISABLE,

I would rename "TSX_DISABLE" to "TSX_DISABLED", because

a) it matches the verb tense of "UCODE_NEEDED", and
b) the past tense hopefully helps make it slightly clearer that TSX was disabled separately, not as part of the mitigation code itself.

Quoted 10 lines

arch/x86/include/asm/processor.h +1 -1

@@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
 #undef pr_fmt
 #define pr_fmt(fmt) fmt
 
-/* 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)
 {
 	/*

arch/x86/include/asm/processor.h static void update_mds_branch_idle(void) · line 889

Quoted 10 lines

arch/x86/include/asm/processor.h +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;

Sorry, I was out for most of the last two weeks, so I think I left you hanging on this. For context, here's your answer to my previous question about whether the X86_BUG_TAA check makes sense here:

Quoted 3 lines

It does provide protection against the "store buffer" leak. But the other buffers(fill buffer and load port) are still SMT vulnerable. Do you prefer it removed?

Yes, please remove it, for the same reason we used for MDS. There's not much point in partially mitigating TAA here.

Quoted 13 lines

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

index 885d4ac2111a..86f22c1e5912 100644
@@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	if (!cpu_matches(NO_SWAPGS))
 		setup_force_cpu_bug(X86_BUG_SWAPGS);
 
+	/*
+	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
+	 *	- TSX is supported or
+	 *	- TSX_CTRL is supported
+	 *

arch/x86/kernel/cpu/common.c static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c) · line 1145

Quoted 10 lines

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

+	 * TSX_CTRL check is needed for cases when TSX could be disabled before
+	 * the kernel boot e.g. kexec
+	 * TSX_CTRL check alone is not sufficient for cases when the microcode
+	 * update is not present or running as guest that don't get TSX_CTRL.
+	 */
+	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
+	    (boot_cpu_has(X86_FEATURE_RTM) ||
+	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
+		setup_force_cpu_bug(X86_BUG_TAA);
+

I'm finding this logic to be less than 100% convincing, or at least hard to follow. And it's different from most of the other bug checks in this function.

Quoted 7 lines

Would the following work instead?

	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);

In other words I would presume all the NO_MDS CPUs listed in 'cpu_vuln_whitelist' are also immune to TAA.

From a leakage point of view, TAA is just another way to access stale date in the load ports, store buffer or fill buffers.

In practice, the leakage mechanism exists on all MDS-vulnerable, TSX-enabled parts.  Parts which are not vulnerable to MDS leakage in the first place will be similarly unaffected.

On pre MDS_NO=1 parts, the existing VERW and no-smt mitigates TAA as well.

On the current MDS_NO=1 parts, the silicon fix constituting MDS_NO took care of the known leakage cases by forcing a register file write-back of 0 rather than the stale data.  However, the TSX Async Abort case was missed, which is why this generation of parts are still vulnerable.

Therefore, the TAA vulnerability only really matters for TSX=1, MDS_NO=1, TAA_NO=0 parts, because these parts are the ones that are leaky, but have a lower overhead mitigation option than VERW and no-SMT.

Quoted 2 lines

I agree with all of that. So are you saying you agree or disagree with my code snippet?

Ah - sorry for being unclear.  Disagree.

Your code snippet will cause some Atom and very old Core parts to also report to be TAA vulnerable, as they are on the MDS whitelist.

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 00/10] TAAv7

Quoted 2 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 00/10] TAAv7

Ok, I ran the pile on a box here:

vendor_id : GenuineIntel
cpu family : 6
model : 158
model name : Intel(R) Core(TM) i5-9600K CPU @ 3.70GHz
stepping : 12

There is some microcode for it:

[    0.000000] microcode: microcode updated early to revision 0xc6, date = 2019-08-14
[    1.005808] microcode: sig=0x906ec, pf=0x2, revision=0xc6

And booting it says:

[    0.197056] tsx_init: enter
[    0.197207] tsx_ctrl_is_supported: CAP MSR: 0x9

This is added by me and it shows that the box is a pre MDS_NO=1 one, i.e., MD_CLEAR mitigates TAA too AFAIU.

Which means, there's no TSX_CTRL_MSR and I cannot disable TSX there.

Which means, boxes like this one don't need the microcode as long as they have MD_CLEAR microcode.

[    0.197363] Spectre V1 : Mitigation: usercopy/swapgs barriers and __user pointer sanitization
[    0.197540] Spectre V2 : Mitigation: Full generic retpoline
[    0.197696] Spectre V2 : Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch
[    0.197871] Spectre V2 : Enabling Restricted Speculation for firmware calls
[    0.198032] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
[    0.198208] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl and seccomp
[    0.198386] MDS: Mitigation: Clear CPU buffers
[    0.198540] TAA: Mitigation: Clear CPU buffers

Makes sense?

FWIW, makes sense to me. It will be especially interesting to see tests on an MDS_NO=1 box with the new microcode.

Quoted 2 lines · speck for Pawan Gupta

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 00/10] TAAv7

Ok, I ran the pile on a box here:

vendor_id : GenuineIntel
cpu family : 6
model : 158
model name : Intel(R) Core(TM) i5-9600K CPU @ 3.70GHz
stepping : 12

There is some microcode for it:

[    0.000000] microcode: microcode updated early to revision 0xc6, date = 2019-08-14
[    1.005808] microcode: sig=0x906ec, pf=0x2, revision=0xc6

And booting it says:

[    0.197056] tsx_init: enter
[    0.197207] tsx_ctrl_is_supported: CAP MSR: 0x9

This is added by me and it shows that the box is a pre MDS_NO=1 one, i.e., MD_CLEAR mitigates TAA too AFAIU.

Which means, there's no TSX_CTRL_MSR and I cannot disable TSX there.

Which means, boxes like this one don't need the microcode as long as they have MD_CLEAR microcode.

[    0.197363] Spectre V1 : Mitigation: usercopy/swapgs barriers and __user pointer sanitization
[    0.197540] Spectre V2 : Mitigation: Full generic retpoline
[    0.197696] Spectre V2 : Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch
[    0.197871] Spectre V2 : Enabling Restricted Speculation for firmware calls
[    0.198032] Spectre V2 : mitigation: Enabling conditional Indirect Branch Prediction Barrier
[    0.198208] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl and seccomp
[    0.198386] MDS: Mitigation: Clear CPU buffers
[    0.198540] TAA: Mitigation: Clear CPU buffers

Makes sense?

Below are the CPUs with MDS_NO=1.

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

From: Pawan Gupta <[email protected]>
Subject: [PATCH v7 06/10] KVM: x86/speculation/taa: Export MDS_NO=0 to guests
when TSX is enabled

Export IA32_ARCH_CAPABILITIES MSR bit MDS_NO=0 to guests on TSX Async Abort(TAA) affected hosts that have TSX enabled and updated microcode. This is required so that the guests don't complain,

"Vulnerable: Clear CPU buffers attempted, no microcode"

when the host has the updated microcode to clear CPU buffers.

Microcode update also adds support for MSR_IA32_TSX_CTRL which is enumerated by the ARCH_CAP_TSX_CTRL bit in IA32_ARCH_CAPABILITIES MSR. Guests can't do this check themselves when the ARCH_CAP_TSX_CTRL bit is not exported to the guests.

In this case export MDS_NO=0 to the guests. When guests have CPUID.MD_CLEAR=1 guests deploy MDS mitigation which also mitigates TAA.

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

Reviewed-by Tony Luck <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

---
 arch/x86/kvm/x86.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/x86/kvm/x86.c +19 -0

index 661e2bf38526..797113fa665d 100644
@@ -1299,6 +1299,25 @@ static u64 kvm_get_arch_capabilities(void)
 	if (!boot_cpu_has_bug(X86_BUG_MDS))
 		data |= ARCH_CAP_MDS_NO;
 
+	/*
+	 * On TAA affected systems, export MDS_NO=0 when:
+	 *	- TSX is enabled on host, i.e. X86_FEATURE_RTM=1.
+	 *	- Updated microcode is present. This is detected by
+	 *	  the presence of ARCH_CAP_TSX_CTRL_MSR. This ensures
+	 *	  VERW clears CPU buffers.
+	 *
+	 * When MDS_NO=0 is exported, guests deploy clear CPU buffer
+	 * mitigation and don't complain:
+	 *
+	 *	"Vulnerable: Clear CPU buffers attempted, no microcode"
+	 *
+	 * If TSX is disabled on the system, guests are also mitigated against
+	 * TAA and clear CPU buffer mitigation is not required for guests.
+	 */
+	if (boot_cpu_has_bug(X86_BUG_TAA) && boot_cpu_has(X86_FEATURE_RTM) &&
+	    (data & ARCH_CAP_TSX_CTRL_MSR))
+		data &= ~ARCH_CAP_MDS_NO;
+
 	return data;
 }
 

On the other hand, removing bits from CPUID or MSRs can cause issues even if TSX is not enabled in VMs. Since most VMs won't have MDS_NO set, I think we should drop this patch for now. After the embargo lifts we can add code to pass TSX_CTRL to the VM just like we do for SPEC_CTRL, including disabling TSX on vmentry/vmexit depending on guest CPUID.

Quoted 6 lines · speck for Paolo Bonzini

On the other hand, removing bits from CPUID or MSRs can cause issues even if TSX is not enabled in VMs. Since most VMs won't have MDS_NO set, I think we should drop this patch for now. After the embargo lifts we can add code to pass TSX_CTRL to the VM just like we do for SPEC_CTRL, including disabling TSX on vmentry/vmexit depending on guest CPUID.

TSX_CTRL is a slow MSR ... so if you do go this path you may also want some heuristic to avoid switching a thread between guests that have different settings for TSX.