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

[PATCH v9 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.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
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 v9 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_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 <noodles@earth.li>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/cpufeature.h  |   9 ++-
 arch/x86/include/asm/cpufeatures.h |   5 ++
 arch/x86/kernel/cpu/Makefile       |   1 +
 arch/x86/kernel/cpu/cpuid-deps.c   | 109 +++++++++++++++++++++++++++++++++++++
 4 files changed, 119 insertions(+), 5 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..a0e13ac0c506 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -125,11 +125,10 @@ 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 { \
-	clear_cpu_cap(&boot_cpu_data, bit);	\
-	set_bit(bit, (unsigned long *)cpu_caps_cleared); \
-} while (0)
+
+extern void setup_clear_cpu_cap(unsigned bit);
+extern void clear_cpu_cap(struct cpuinfo_x86 *cinfo, unsigned bit);
+
 #define setup_force_cpu_cap(bit) do { \
 	set_cpu_cap(&boot_cpu_data, bit);	\
 	set_bit(bit, (unsigned long *)cpu_caps_set);	\
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..de599547b440
--- /dev/null
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -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;
+};
+
+/*
+ * 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   },
+	{ 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_AVX512F   },
+	{}
+};
+
+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);
+}
+
+static inline void clear_feature(struct cpuinfo_x86 *cinfo, unsigned feat)
+{
+	if (!cinfo)
+		__setup_clear_cpu_cap(feat);
+	else
+		__clear_cpu_cap(cinfo, feat);
+}
+
+static void do_clear_cpu_cap(struct cpuinfo_x86 *cinfo, unsigned feat)
+{
+	bool changed;
+	__u32 disable[NCAPINTS + NBUGINTS];
+	unsigned long *disable_mask = (unsigned long *)disable;
+	const struct cpuid_dep *d;
+
+	clear_feature(cinfo, feat);
+
+	/* Collect all features to disable, handling dependencies */
+	memset(disable, 0, sizeof(disable));
+	__set_bit(feat, disable_mask);
+	/* Loop until we get a stable state. */
+	do {
+		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);
+}
+
+void clear_cpu_cap(struct cpuinfo_x86 *cinfo, unsigned feat)
+{
+	do_clear_cpu_cap(cinfo, feat);
+}
+
+void setup_clear_cpu_cap(unsigned feat)
+{
+	do_clear_cpu_cap(NULL, feat);
+}
-- 
2.13.6

[PATCH v9 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.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
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 v9 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.

v2:
Add curly brackets (Thomas Gleixner)
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index f1d5476c9022..924bd895b5ee 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,14 @@ 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 v9 5/5] x86/xsave: Remove the explicit clearing of XSAVE dependend features

[Andi Kleen]

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

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.

v2: Use Thomas Gleixner's new description
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
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 924bd895b5ee..bfe8c0c1033d 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 v9 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling

[Thomas Gleixner]

On Fri, 6 Oct 2017, Andi Kleen wrote:

> 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.
> 
> v2:
> Add curly brackets (Thomas Gleixner)
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

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

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

[Thomas Gleixner]

On Fri, 6 Oct 2017, Andi Kleen wrote:

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

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

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

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> 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.

But that's not what the patch does:

> ---
>  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);

This moves the early _FPU_ initialization code, which is more than just xstate 
related...

Also, this is fundamentally a CPU initialization sequence, and this change 
detaches it from the early CPU initialization code. If you want to parse early 
parameters you can add it to fpu__init_parse_early_param() which is there for that 
exact reason.

Also, please fix the titles of your patches as well, you routinely get the 
prefixes wrong, 'git log' should give you an idea about what the current title 
pattern is for FPU code modifications.

Thanks,

	Ingo

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

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -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.

> +
> +/*
> + * 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   },
> +	{ 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_AVX512F   },
> +	{}
> +};

Why isn't this __initdata?

> +
> +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);
> +}
> +
> +static inline void clear_feature(struct cpuinfo_x86 *cinfo, unsigned feat)
> +{
> +	if (!cinfo)
> +		__setup_clear_cpu_cap(feat);
> +	else
> +		__clear_cpu_cap(cinfo, feat);
> +}
> +
> +static void do_clear_cpu_cap(struct cpuinfo_x86 *cinfo, unsigned feat)

Sloppy types: the canonical form is the longer 'unsigned int'..

Plus there's local variable naming sloppiness as well: 'cinfo' is not used 
anywhere in the current x86 code, introducing a new idiosyncratic naming variant 
just increases the complexity of the kernel unnecessarily.

When unsure about what the canonical naming of a commonly used structure is, you 
can type:

	git grep 'struct cpuinfo_x86'

and see how this variable is typically named in over 200 other cases.


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

Those helpers could be used on the natural u32 type of these fields without 
constantly having to force types between u32 and long.

> +
> +	clear_feature(cinfo, feat);
> +
> +	/* Collect all features to disable, handling dependencies */
> +	memset(disable, 0, sizeof(disable));
> +	__set_bit(feat, disable_mask);
> +	/* Loop until we get a stable state. */
> +	do {

Nit: please add a newline to vertically separate the loop initialization from the 
loop.

> +		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);
> +}
> +
> +void clear_cpu_cap(struct cpuinfo_x86 *cinfo, unsigned feat)
> +{
> +	do_clear_cpu_cap(cinfo, feat);
> +}
> +
> +void setup_clear_cpu_cap(unsigned feat)
> +{
> +	do_clear_cpu_cap(NULL, feat);
> +}

There's naming confusion here: sometimes a CPU feature bit is called a 'cap' 
(capability), sometimes 'feat' (feature). Pick 'feature' and use it consistently.

Please review the whole patch set for similar patterns of bugs/problems.

Thanks,

	Ingo

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

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> 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.
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> 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);

Yeah, so there's pre-existing bad naming here that should be fixed before we add 
new complexity to the code: why is the option named 'clearcpuid' while the 
function name is 'disablecpuid'?

Thanks,

	Ingo

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

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> 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.
> 
> v2:
> Add curly brackets (Thomas Gleixner)
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  arch/x86/kernel/fpu/xstate.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index f1d5476c9022..924bd895b5ee 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,14 @@ 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: */

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

Thanks,

	Ingo

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

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> > +	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);

Thanks,

	Ingo

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

[Thomas Gleixner]

On Thu, 12 Oct 2017, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > --- /dev/null
> > +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> > @@ -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.

Thanks,

	tglx

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

[Andi Kleen]

On Thu, Oct 12, 2017 at 05:13:34PM +0200, Thomas Gleixner wrote:
> On Thu, 12 Oct 2017, Ingo Molnar wrote:
> > 
> > * Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > --- /dev/null
> > > +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> > > @@ -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.


Right it was an attempt to save a tiny bit of text space.
On modern x86 CPUs there is no penalty for 16bit except for slightly
larger code.  The table is far bigger than the few additional 16bit prefixes
in the code

   text    data     bss     dec     hex filename
    436       0       0     436     1b4 arch/x86/kernel/cpu/cpuid-deps.o-short
    572       0       0     572     23c arch/x86/kernel/cpu/cpuid-deps.o-int

But can convert to 4 bytes in the next version.

-Andi

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

[Andi Kleen]

> 
> Why isn't this __initdata?

It's referenced during cpu hotplug. Actually it should never
change in this case, but it's hard to tell that to the dependency
checker.

-Andi

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

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 12 Oct 2017, Ingo Molnar wrote:
> > 
> > * Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > --- /dev/null
> > > +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> > > @@ -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.

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

So the only effect the 16-bit variables have is (marginally) worse boot times.

Thanks,

	Ingo

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

[Andi Kleen]

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

It's difficult to make the code __init because it's part of the CPU initialization and
the CPU initialization is called from CPU hotplug. 

It shouldn't do anything after initial boot however, but it's difficult to tell
that to the dependency checker.

In theory it would be possible to restructure cpu initialization to avoid this,
but I suspect it would be rather large and intrusive and probably not worth
it for a couple hundred bytes.

I removed the shorts.

-Andi

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

[Andi Kleen]

> 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);

It's some more complicated because "var" are arrays.
Opencoding is fairly ugly.

I changed the main bitmap to use DECLARE_BITMAP now, which
avoids most of the casts. For accessing c->x86_capability and
cpu_caps_cleared it uses helpers now that do implicit casting.

-Andi

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

[Andi Kleen]

> > +	/*
> > +	 * 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: */
> 
> This patch has similar problems to the ones I reported against 2/5.

I'm not clear what I'm supposed to change here. The array cannot be __initdata
because it is used at cpu hotplug. Nothing else seems to apply.

-Andi