Support generic disabling of all XSAVE features

From: Andi Kleen <[email protected]>

Move the XSAVE initialization code to be after parsing early parameters. I don't see any reason why the FPU code needs to be initialized that early, nothing else in the initialization phase uses XSAVE. This is useful to be able to handle command line parameters in the XSAVE initialization code.

---
 arch/x86/kernel/cpu/common.c | 2 --
 arch/x86/kernel/setup.c      | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

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

index c9176bae7fd8..fd47692e5ce9 100644

From: Andi Kleen <[email protected]>

Some CPUID features depend on other features. Currently it's possible to to clear dependent features, but not clear the base features, which can cause various interesting problems.

This patch implements a generic table to describe dependencies between CPUID features, to be used by all code that clears CPUID.

Some subsystems (like XSAVE) had an own implementation of this, but it's better to do it all in a single place for everyone.

Then clear_cpu_cap and setup_clear_cpu_cap always look up this table and clear all dependencies too.

This is intended to be a practical table: only for features that make sense to clear. If someone for example clears FPU, or other features that are essentially part of the required base feature set, not much is going to work. Handling that is right now out of scope. We're only handling features which can be usefully cleared.

v2: Add EXPORT_SYMBOL for clear_cpu_cap for lguest
v3:
Fix handling of depending issues
Fix dups in the table (Jonathan McDowell)
v4:
Remove EXPORT_SYMBOL again as lguest is gone
Restructure dependencies as feature, dependency (Thomas Gleixner) Move some code from header file to C file and turn macros into inlines (dito) Simplify if conditions (dito)
Add missing dependency for AVX512F->AVX
v5:
Remove redundant dependency for AVX512F->XSAVE (Thomas Gleixner) Add missing dependency for XMM->FXSR (Thomas Gleixner)
v6:
Add SHA_NI->XMM and FXSR_OPT->FXSAVE dependencies

Cc Jonathan McDowell <[email protected]>

Signed-off-by Andi Kleen <[email protected]>

arch/x86/kernel/cpu/cpuid-deps.c new file +33 -0

+
+/*
+ * Table of CPUID features that depend on others.
+ *
+ * This only includes dependencies that can be usefully disabled, not
+ * features part of the base set (like FPU).
+ */
+const static struct cpuid_dep cpuid_deps[] = {
+	{ X86_FEATURE_XSAVEOPT,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVEC,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVES,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_AVX,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_PKU,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_MPX,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XGETBV1,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_FXSR_OPT,		X86_FEATURE_FXSR      },
+	{ X86_FEATURE_XMM,		X86_FEATURE_FXSR      },
+	{ X86_FEATURE_XMM2,		X86_FEATURE_XMM       },
+	{ X86_FEATURE_XMM3,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_XMM4_1,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_XMM4_2,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_XMM3,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_PCLMULQDQ,	X86_FEATURE_XMM2      },
+	{ X86_FEATURE_SSSE3,		X86_FEATURE_XMM2,     },
+	{ X86_FEATURE_F16C,		X86_FEATURE_XMM2,     },
+	{ X86_FEATURE_AES,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_SHA_NI,		X86_FEATURE_XMM2      },
+	{ X86_FEATURE_FMA,		X86_FEATURE_AVX       },
+	{ X86_FEATURE_AVX2,		X86_FEATURE_AVX,      },
+	{ X86_FEATURE_AVX512F,		X86_FEATURE_AVX,      },
+	{ X86_FEATURE_AVX512IFMA,	X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512PF,		X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_AVX512ER,		X86_FEATURE_AVX512F   },

arch/x86/kernel/cpu/cpuid-deps.c new file +11 -0

+
+static inline void __clear_cpu_cap(struct cpuinfo_x86 *cinfo, unsigned bit)
+{
+	clear_bit(bit, (unsigned long *)(cinfo->x86_capability));
+}
+
+static inline void __setup_clear_cpu_cap(unsigned bit)
+{
+	clear_cpu_cap(&boot_cpu_data, bit);
+	set_bit(bit, (unsigned long *)cpu_caps_cleared);
+}

arch/x86/kernel/cpu/cpuid-deps.c new file +12 -0

+		changed = false;
+		for (d = cpuid_deps; d->feature; d++) {
+			if (!test_bit(d->depends, disable_mask))
+				continue;
+			if (__test_and_set_bit(d->feature, disable_mask))
+				continue;
+
+			changed = true;
+			clear_feature(cinfo, d->feature);
+		}
+	} while (changed);
+}

arch/x86/kernel/cpu/cpuid-deps.c line 79

Quoted 4 lines

arch/x86/kernel/cpu/cpuid-deps.c new file +4 -0

+	bool changed;
+	__u32 disable[NCAPINTS + NBUGINTS];
+	unsigned long *disable_mask = (unsigned long *)disable;
+	const struct cpuid_dep *d;

The constant forced types are really ugly and permeate the whole code. Please introduce some suitably named helpers in the x86 bitops file that work on local variables:

	var &= ~(1<<bit);
	var |= 1<<bit;

So instead of adding helpers the 1<< ops can be written out explicitly - they are easy to read after all. Can also be:

	var &= ~BIT(bit);
	var |= BIT(bit);

arch/x86/kernel/cpu/cpuid-deps.c line 10

Quoted 13 lines · Andi Kleen

arch/x86/kernel/cpu/cpuid-deps.c new file +10 -0

@@ -0,0 +1,109 @@
+/* Declare dependencies between CPUIDs */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <asm/cpufeature.h>
+
+struct cpuid_dep {
+	unsigned short feature;
+	unsigned short depends;
+};

Why are these 16-bit fields? 16-bit data types should be avoided as much as possible, as they generate suboptimal code.

Quoted 19 lines · Ingo Molnar

arch/x86/kernel/cpu/cpuid-deps.c line 10

Quoted 13 lines · Andi Kleen

arch/x86/kernel/cpu/cpuid-deps.c new file +10 -0

@@ -0,0 +1,109 @@
+/* Declare dependencies between CPUIDs */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <asm/cpufeature.h>
+
+struct cpuid_dep {
+	unsigned short feature;
+	unsigned short depends;
+};

Why are these 16-bit fields? 16-bit data types should be avoided as much as possible, as they generate suboptimal code.

I was looking at that as well and decided that we preferrably have a compressed data structure. The code which walks the table is hardly performance critical and the difference in text size is marginal.

Quoted 19 lines · Ingo Molnar

arch/x86/kernel/cpu/cpuid-deps.c line 10

Quoted 13 lines · Andi Kleen

arch/x86/kernel/cpu/cpuid-deps.c new file +10 -0

@@ -0,0 +1,109 @@
+/* Declare dependencies between CPUIDs */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <asm/cpufeature.h>
+
+struct cpuid_dep {
+	unsigned short feature;
+	unsigned short depends;
+};

Why are these 16-bit fields? 16-bit data types should be avoided as much as possible, as they generate suboptimal code.

I was looking at that as well and decided that we preferrably have a compressed data structure. The code which walks the table is hardly performance critical and the difference in text size is marginal.

Quoted 3 lines

I was looking at that as well and decided that we preferrably have a compressed data structure. The code which walks the table is hardly performance critical and the difference in text size is marginal.

So the code should all be __init (once that is fixed), hence data and text size literally does not matter - it gets freed.

Why isn't this __initdata?

From: Andi Kleen <[email protected]>

Make clearcpuid= an early param, to make sure it is parsed before the XSAVE initialization. This allows to modify XSAVE state by clearing specific CPUID bits.

Reviewed-by Thomas Gleixner <[email protected]>

Signed-off-by Andi Kleen <[email protected]>

---
 arch/x86/kernel/cpu/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index fd47692e5ce9..ff51c61d2df0 100644

From: Andi Kleen <[email protected]>

Before enabling XSAVE, not only check the XSAVE specific CPUID bits, but also the base CPUID features of the respective XSAVE feature. This allows to disable individual XSAVE states using the existing clearcpuid= option, which can be useful for performance testing and debugging, and also in general avoids inconsistencies.

v2:
Add curly brackets (Thomas Gleixner)

Signed-off-by Andi Kleen <[email protected]>

From: Andi Kleen <[email protected]>

Before enabling XSAVE, not only check the XSAVE specific CPUID bits, but also the base CPUID features of the respective XSAVE feature. This allows to disable individual XSAVE states using the existing clearcpuid= option, which can be useful for performance testing and debugging, and also in general avoids inconsistencies.

v2:
Add curly brackets (Thomas Gleixner)

Signed-off-by Andi Kleen <[email protected]>

---
 arch/x86/kernel/fpu/xstate.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

arch/x86/kernel/fpu/xstate.c +16 -0

index f1d5476c9022..924bd895b5ee 100644
@@ -15,6 +15,7 @@
 #include <asm/fpu/xstate.h>
 
 #include <asm/tlbflush.h>
+#include <asm/cpufeature.h>
 
 /*
  * Although we spell it out in here, the Processor Trace

@@ -36,6 +37,19 @@ static const char *xfeature_names[] =
 	"unknown xstate feature"	,
 };
 
+static short xsave_cpuid_features[] = {
+	X86_FEATURE_FPU,
+	X86_FEATURE_XMM,
+	X86_FEATURE_AVX,
+	X86_FEATURE_MPX,
+	X86_FEATURE_MPX,
+	X86_FEATURE_AVX512F,
+	X86_FEATURE_AVX512F,
+	X86_FEATURE_AVX512F,
+	X86_FEATURE_INTEL_PT,
+	X86_FEATURE_PKU,
+};
+
 /*
  * Mask of xstate features supported by the CPU and the kernel:
  */

@@ -726,6 +740,7 @@ void __init fpu__init_system_xstate(void)
 	unsigned int eax, ebx, ecx, edx;
 	static int on_boot_cpu __initdata = 1;
 	int err;
+	int i;
 
 	WARN_ON_FPU(!on_boot_cpu);
 	on_boot_cpu = 0;

@@ -759,6 +774,14 @@ void __init fpu__init_system_xstate(void)
 		goto out_disable;
 	}
 
+	/*

Quoted 10 lines

arch/x86/kernel/fpu/xstate.c +8 -0

+	/*
+	 * Clear XSAVE features that are disabled in the normal CPUID.
+	 */
+	for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) {
+		if (!boot_cpu_has(xsave_cpuid_features[i]))
+			xfeatures_mask &= ~BIT(i);
+	}
+
 	xfeatures_mask &= fpu__get_supported_xfeatures_mask();

Quoted 1 line

/* Enable xstate instructions to be able to continue with initialization: */

This patch has similar problems to the ones I reported against 2/5.