Support generic disabling of all XSAVE features

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. 

This patchkit hooks up XSAVE with the generic clearcpuid=... option,
so that disabling a CPUID feature automatically disables the respective
XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.
v4:
Rebase to latest tree. Repost.
v5:
Fix dependency algorithm. Clean dups in table. Rebase.
v6: 
Rebase. No changes to code.
v7:
Rebase. No changes to code.

[PATCH v7 1/5] x86/xsave: Move xsave initialization to after parsing early parameters

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/common.c | 2 --
 arch/x86/kernel/setup.c      | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c9176bae7fd8..fd47692e5ce9 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -903,8 +903,6 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 	}
 
 	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
-	fpu__init_system(c);
-
 #ifdef CONFIG_X86_32
 	/*
 	 * Regardless of whether PCID is enumerated, the SDM says
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0957dd73d127..2260e586295f 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -92,6 +92,7 @@
 #include <asm/processor.h>
 #include <asm/bugs.h>
 #include <asm/kasan.h>
+#include <asm/fpu/internal.h>
 
 #include <asm/vsyscall.h>
 #include <asm/cpu.h>
@@ -992,6 +993,8 @@ void __init setup_arch(char **cmdline_p)
 
 	parse_early_param();
 
+	fpu__init_system(&boot_cpu_data);
+
 #ifdef CONFIG_MEMORY_HOTPLUG
 	/*
 	 * Memory used by the kernel cannot be hot-removed because Linux
-- 
2.13.6

[PATCH v7 2/5] x86/cpuid: Add generic table for cpuid dependencies

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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_id for lguest
v3:
Fix handling of depending issues
Fix dups in the table (Jonathan McDowell)
Cc: Jonathan McDowell <noodles@earth.li>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/cpufeature.h  |  8 +++-
 arch/x86/include/asm/cpufeatures.h |  5 ++
 arch/x86/kernel/cpu/Makefile       |  1 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 94 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/cpuid-deps.c

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index d59c15c3defd..e6145f383ff8 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -125,8 +125,12 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 #define boot_cpu_has(bit)	cpu_has(&boot_cpu_data, bit)
 
 #define set_cpu_cap(c, bit)	set_bit(bit, (unsigned long *)((c)->x86_capability))
-#define clear_cpu_cap(c, bit)	clear_bit(bit, (unsigned long *)((c)->x86_capability))
-#define setup_clear_cpu_cap(bit) do { \
+#define __clear_cpu_cap(c, bit)	clear_bit(bit, (unsigned long *)((c)->x86_capability))
+
+extern void setup_clear_cpu_cap(int bit);
+extern void clear_cpu_cap(struct cpuinfo_x86 *cpu, int bit);
+
+#define __setup_clear_cpu_cap(bit) do { \
 	clear_cpu_cap(&boot_cpu_data, bit);	\
 	set_bit(bit, (unsigned long *)cpu_caps_cleared); \
 } while (0)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 2519c6c801c9..401a70992060 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -21,6 +21,11 @@
  * this feature bit is not displayed in /proc/cpuinfo at all.
  */
 
+/*
+ * When adding new features here that depend on other features,
+ * please update the table in kernel/cpu/cpuid-deps.c
+ */
+
 /* Intel-defined CPU features, CPUID level 0x00000001 (edx), word 0 */
 #define X86_FEATURE_FPU		( 0*32+ 0) /* Onboard FPU */
 #define X86_FEATURE_VME		( 0*32+ 1) /* Virtual Mode Extensions */
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index e17942c131c8..de260fae1017 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -22,6 +22,7 @@ obj-y			+= rdrand.o
 obj-y			+= match.o
 obj-y			+= bugs.o
 obj-$(CONFIG_CPU_FREQ)	+= aperfmperf.o
+obj-y			+= cpuid-deps.o
 
 obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
new file mode 100644
index 000000000000..f3087d46459d
--- /dev/null
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -0,0 +1,94 @@
+/* Declare dependencies between CPUIDs */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <asm/cpufeature.h>
+
+struct cpuid_dep {
+	int feature;
+	int disable;
+};
+
+/*
+ * 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_XSAVE,   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_AVX512F },
+	{ X86_FEATURE_XSAVE,   X86_FEATURE_PKU },
+	{ X86_FEATURE_XSAVE,   X86_FEATURE_MPX },
+	{ X86_FEATURE_XSAVE,   X86_FEATURE_XGETBV1 },
+	{ X86_FEATURE_XMM,     X86_FEATURE_XMM2 },
+	{ X86_FEATURE_XMM2,    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_FMA,     X86_FEATURE_AVX },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512IFMA },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512PF },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512ER },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512CD },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512DQ },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512BW },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512VL },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512VBMI },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4VNNIW },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4FMAPS },
+	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512_VPOPCNTDQ },
+	{ X86_FEATURE_AVX,     X86_FEATURE_AVX2 },
+	{}
+};
+
+static inline void clearfeat(struct cpuinfo_x86 *cpu, int feat)
+{
+	if (!cpu)
+		__setup_clear_cpu_cap(feat);
+	else
+		__clear_cpu_cap(cpu, feat);
+}
+
+static void do_clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat)
+{
+	bool changed;
+	__u32 disable[NCAPINTS + NBUGINTS];
+	unsigned long *disable_mask = (unsigned long *)disable;
+	const struct cpuid_dep *d;
+
+	clearfeat(cpu, feat);
+
+	/* Collect all features to disable, handling dependencies */
+	memset(disable, 0, sizeof(disable));
+	__set_bit(feat, disable_mask);
+	do {
+		changed = false;
+		for (d = cpuid_deps; d->feature; d++) {
+			if (test_bit(d->feature, disable_mask) &&
+			    !__test_and_set_bit(d->disable, disable_mask)) {
+				changed = true;
+				clearfeat(cpu, d->disable);
+			}
+		}
+	} while (changed);
+}
+
+void clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat)
+{
+	do_clear_cpu_cap(cpu, feat);
+}
+
+EXPORT_SYMBOL_GPL(clear_cpu_cap);
+
+void setup_clear_cpu_cap(int feat)
+{
+	do_clear_cpu_cap(NULL, feat);
+}
-- 
2.13.6

[PATCH v7 3/5] x86/cpuid: Make clearcpuid an early param

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index fd47692e5ce9..ff51c61d2df0 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1310,7 +1310,7 @@ static __init int setup_disablecpuid(char *arg)
 
 	return 1;
 }
-__setup("clearcpuid=", setup_disablecpuid);
+early_param("clearcpuid", setup_disablecpuid);
 
 #ifdef CONFIG_X86_64
 DEFINE_PER_CPU_FIRST(union irq_stack_union,
-- 
2.13.6

[PATCH v7 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index f1d5476c9022..2a46efe0b9a9 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -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,13 @@ void __init fpu__init_system_xstate(void)
 		goto out_disable;
 	}
 
+	/*
+	 * 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();
 
 	/* Enable xstate instructions to be able to continue with initialization: */
-- 
2.13.6

[PATCH v7 5/5] x86/xsave: Using generic CPUID clearing when disabling XSAVE

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Now that the generic CPUID clearing understands dependencies,
it's enough to clear the XSAVE CPUID bit to clear all depending
features when XSAVE gets disabled.

So we don't need this hard to maintain explicit list
of features depending on XSAVE anymore. Just call the generic
clear_cpu_cap() function for XSAVE.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 2a46efe0b9a9..1e2d7b7fb96f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -73,26 +73,6 @@ unsigned int fpu_user_xstate_size;
 void fpu__xstate_clear_all_cpu_caps(void)
 {
 	setup_clear_cpu_cap(X86_FEATURE_XSAVE);
-	setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
-	setup_clear_cpu_cap(X86_FEATURE_XSAVEC);
-	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
-	setup_clear_cpu_cap(X86_FEATURE_AVX);
-	setup_clear_cpu_cap(X86_FEATURE_AVX2);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512F);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512IFMA);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512PF);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512ER);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512CD);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512DQ);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512BW);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512VL);
-	setup_clear_cpu_cap(X86_FEATURE_MPX);
-	setup_clear_cpu_cap(X86_FEATURE_XGETBV1);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512VBMI);
-	setup_clear_cpu_cap(X86_FEATURE_PKU);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512_4VNNIW);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512_4FMAPS);
-	setup_clear_cpu_cap(X86_FEATURE_AVX512_VPOPCNTDQ);
 }
 
 /*
-- 
2.13.6

Re: [PATCH v7 1/5] x86/xsave: Move xsave initialization to after parsing early parameters

[Thomas Gleixner]

On Wed, 4 Oct 2017, Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> 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.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v7 2/5] x86/cpuid: Add generic table for cpuid dependencies

[Thomas Gleixner]

On Wed, 4 Oct 2017, Andi Kleen wrote:

Cc: +Borislav

> v2: Add EXPORT_SYMBOL for clear_cpu_id for lguest

clear_cpu_id? You probably mean clear_cpu_cap, but that's moot now that
lguest is gone......

--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
...
>  #define set_cpu_cap(c, bit)	set_bit(bit, (unsigned long *)((c)->x86_capability))
> -#define clear_cpu_cap(c, bit)	clear_bit(bit, (unsigned long *)((c)->x86_capability))
> -#define setup_clear_cpu_cap(bit) do { \
> +#define __clear_cpu_cap(c, bit)	clear_bit(bit, (unsigned long *)((c)->x86_capability))
>
> +
> +extern void setup_clear_cpu_cap(int bit);
> +extern void clear_cpu_cap(struct cpuinfo_x86 *cpu, int bit);
> +
> +#define __setup_clear_cpu_cap(bit) do { \
>  	clear_cpu_cap(&boot_cpu_data, bit);	\
>  	set_bit(bit, (unsigned long *)cpu_caps_cleared); \
>  } while (0)


__clear_cpu_cap() and __setup_clear_cpu_cap() should not be in the header
file. They should be in the new cpuid-deps file so they are not
exposed. While at it please implement them as inlines. There is no reason
for having them as macros.

> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -0,0 +1,94 @@
> +/* Declare dependencies between CPUIDs */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <asm/cpufeature.h>
> +
> +struct cpuid_dep {
> +	int feature;
> +	int disable;
> +};
> +
> +/*
> + * 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_XSAVE,   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_AVX512F },
> +	{ X86_FEATURE_XSAVE,   X86_FEATURE_PKU },
> +	{ X86_FEATURE_XSAVE,   X86_FEATURE_MPX },
> +	{ X86_FEATURE_XSAVE,   X86_FEATURE_XGETBV1 },
> +	{ X86_FEATURE_XMM,     X86_FEATURE_XMM2 },
> +	{ X86_FEATURE_XMM2,    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_FMA,     X86_FEATURE_AVX },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512IFMA },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512PF },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512ER },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512CD },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512DQ },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512BW },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512VL },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512VBMI },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4VNNIW },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4FMAPS },
> +	{ X86_FEATURE_AVX512F, X86_FEATURE_AVX512_VPOPCNTDQ },
> +	{ X86_FEATURE_AVX,     X86_FEATURE_AVX2 },
> +	{}
> +};
> +
> +static inline void clearfeat(struct cpuinfo_x86 *cpu, int feat)

What's wrong with giving this a readable name like clear_feature()? That
function name is definitely not a feat.

*cpu is the worst argument name you could come up with. I was not paying
attention when skimming the patch and assumed that the 'if (!cpu)' check
tests for cpu == 0, aka boot cpu.

Also the feature argument wants to be unsigned int all over the place.

> +{
> +	if (!cpu)
> +		__setup_clear_cpu_cap(feat);
> +	else
> +		__clear_cpu_cap(cpu, feat);
> +}
> +
> +static void do_clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat)
> +{
> +	bool changed;
> +	__u32 disable[NCAPINTS + NBUGINTS];
> +	unsigned long *disable_mask = (unsigned long *)disable;
> +	const struct cpuid_dep *d;
> +
> +	clearfeat(cpu, feat);
> +
> +	/* Collect all features to disable, handling dependencies */
> +	memset(disable, 0, sizeof(disable));
> +	__set_bit(feat, disable_mask);
> +	do {
> +		changed = false;
> +		for (d = cpuid_deps; d->feature; d++) {
> +			if (test_bit(d->feature, disable_mask) &&
> +			    !__test_and_set_bit(d->disable, disable_mask)) {

The logic of this dependency structure is backwards. As the name says its
supposed to express dependencies. So it should actually do so:

struct cpuid_dep {
	unsigned int	feature;
	unsigned int	depends;
};

Then the logic here becomes immediately obvious:

		for (d = cpuid_deps; d->feature; d++) {
			if (!test_bit(d->depends, disabled_mask))
				continue;
			if (test_and_set_bit(d->feature, disabled_mask))
				continue;

			changed = true;
			clear_feature(cpuinfo, d->feature);
		}

Hmm?

Thanks,

	tglx

Re: [PATCH v7 3/5] x86/cpuid: Make clearcpuid an early param

[Thomas Gleixner]

On Wed, 4 Oct 2017, Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> 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.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v7 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling

[Thomas Gleixner]

On Wed, 4 Oct 2017, Andi Kleen wrote:
> +	/*
> +	 * 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);

Please add curly braces around this multi line statement.

  https://marc.info/?l=linux-kernel&m=148467980905537&w=2

Thanks,

	tglx

Re: [PATCH v7 5/5] x86/xsave: Using generic CPUID clearing when disabling XSAVE

[Thomas Gleixner]

On Wed, 4 Oct 2017, Andi Kleen wrote:

> From: Andi Kleen <ak@linux.intel.com>
> 
> Now that the generic CPUID clearing understands dependencies,
> it's enough to clear the XSAVE CPUID bit to clear all depending
> features when XSAVE gets disabled.
> 
> So we don't need this hard to maintain explicit list
> of features depending on XSAVE anymore. Just call the generic
> clear_cpu_cap() function for XSAVE.

This changelog is hard to read and uses terms like 'CPUID clearing'. I know
what you want to say, but this is far from being precise and
understandable.

Aside of that the changelog violates as usual the changelog guidelines in
Documentation/....

Something like this would be clear and precise:

  Clearing a CPU feature with setup_clear_cpu_cap() clears all features
  which depend on it. Expressing feature dependencies in one place is
  easier to maintain than keeping functions like
  fpu__xstate_clear_all_cpu_caps() up to date.

  The features which depend on XSAVE have their dependency expressed in the
  dependency table, so its sufficient to clear X86_FEATURE_XSAVE.

  Remove the explicit clearing of XSAVE dependend features. 

Hmm?

For the code patch itself:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks,

	tglx

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. 

This patchkit hooks up XSAVE with the generic clearcpuid=... option,
so that disabling a CPUID feature automatically disables the respective
XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.
v4:
Rebase to latest tree. Repost.
v5:
Fix dependency algorithm. Clean dups in table. Rebase.
v6: 
Rebase. No changes to code.
v7:
Rebase. No changes to code.
v8:
Address all review comments. Add Reviewed-bys.
Dependency checker restructured for feature->dependency
Add missing dependency for AVX->AVX512F
v9:
Remove redundant dependency AVX512F->XSAVE
Add dependencies for SHA_NI->XMM, XMM->FXSR, FXSR_OPT->FXSR
v10:
Move clearcpuid= option parsing to early xsave parsing,
and don't move the FPU initialization.
Add new clear_bit32/set_bit32
Lots of smaller changes based on review feedback.

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. 

This patchkit hooks up XSAVE with the generic clearcpuid=... option,
so that disabling a CPUID feature automatically disables the respective
XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.
v4:
Rebase to latest tree. Repost.
v5:
Fix dependency algorithm. Clean dups in table. Rebase.
v6: 
Rebase. No changes to code.
v7:
Rebase. No changes to code.
v8:
Address all review comments. Add Reviewed-bys.
Dependency checker restructured for feature->dependency
Add missing dependency for AVX->AVX512F
v9:
Remove redundant dependency AVX512F->XSAVE
Add dependencies for SHA_NI->XMM, XMM->FXSR, FXSR_OPT->FXSR

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. 

This patchkit hooks up XSAVE with the generic clearcpuid=... option,
so that disabling a CPUID feature automatically disables the respective
XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.
v4:
Rebase to latest tree. Repost.
v5:
Fix dependency algorithm. Clean dups in table. Rebase.
v6: 
Rebase. No changes to code.
v7:
Rebase. No changes to code.
v8:
Address all review comments. Add Reviewed-bys.
Dependency checker restructured for feature->dependency
Add missing dependency for AVX->AVX512F

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. This patchkit hooks up XSAVE with the
generic clearcpuid=... option, so that disabling a CPUID feature
automatically disables the respective XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.
v4:
Rebase to latest tree. Repost.
v5:
Fix dependency algorithm. Clean dups in table. Rebase.
v6: 
Rebase. No changes to code.

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. This patchkit hooks up XSAVE with the
generic clearcpuid=... option, so that disabling a CPUID feature
automatically disables the respective XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.
v4:
Rebase to latest tree. Repost.

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. This patchkit hooks up XSAVE with the
generic clearcpuid=... option, so that disabling a CPUID feature
automatically disables the respective XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with clearcpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter
v3:
Repost. No changes to code.

Re: Support generic disabling of all XSAVE features

[Andi Kleen]

Andi Kleen <andi@firstfloor.org> writes:

Any comments on this patchkit? If there are no objections please merge.

-Andi

> For performance testing and debugging it can be useful to disable XSAVE
> features individually. This patchkit hooks up XSAVE with the
> generic clearcpuid=... option, so that disabling a CPUID feature
> automatically disables the respective XSAVE feature.
>
> It also cleans up CPUID dependency management. Currently it's
> possible to generate configurations with cleacpuid that crash.
>
> It replaces an earlier patchkit that did this with special
> case options.
>
> v1:
> Initial post
> v2:
> Work around broken lguest by exporting set_cpu_cap
> Repost with cover letter

Support generic disabling of all XSAVE features

[Andi Kleen]

For performance testing and debugging it can be useful to disable XSAVE
features individually. This patchkit hooks up XSAVE with the
generic clearcpuid=... option, so that disabling a CPUID feature
automatically disables the respective XSAVE feature.

It also cleans up CPUID dependency management. Currently it's
possible to generate configurations with cleacpuid that crash.

It replaces an earlier patchkit that did this with special
case options.

v1:
Initial post
v2:
Work around broken lguest by exporting set_cpu_cap
Repost with cover letter