linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Support generic disabling of all XSAVE features
@ 2017-10-05 21:52 Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 1/5] x86/xsave: Move xsave initialization to after parsing early parameters Andi Kleen
                   ` (4 more replies)
  0 siblings, 5 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 21:52 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v8 1/5] x86/xsave: Move xsave initialization to after parsing early parameters
  2017-10-05 21:52 Support generic disabling of all XSAVE features Andi Kleen
@ 2017-10-05 21:52 ` Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies Andi Kleen
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 21:52 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies
  2017-10-05 21:52 Support generic disabling of all XSAVE features Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 1/5] x86/xsave: Move xsave initialization to after parsing early parameters Andi Kleen
@ 2017-10-05 21:52 ` Andi Kleen
  2017-10-05 22:41   ` Thomas Gleixner
  2017-10-05 21:52 ` [PATCH v8 3/5] x86/cpuid: Make clearcpuid an early param Andi Kleen
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 21:52 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel, Andi Kleen, Jonathan McDowell

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
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   | 107 +++++++++++++++++++++++++++++++++++++
 4 files changed, 117 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..27be2fc9776d
--- /dev/null
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -0,0 +1,107 @@
+/* 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_AVX512F,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_PKU,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_MPX,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XGETBV1,		X86_FEATURE_XSAVE     },
+	{ 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_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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v8 3/5] x86/cpuid: Make clearcpuid an early param
  2017-10-05 21:52 Support generic disabling of all XSAVE features Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 1/5] x86/xsave: Move xsave initialization to after parsing early parameters Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies Andi Kleen
@ 2017-10-05 21:52 ` Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling Andi Kleen
  2017-10-05 21:52 ` [PATCH v8 5/5] x86/xsave: Remove the explicit clearing of XSAVE dependend features Andi Kleen
  4 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 21:52 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-10-05 21:52 Support generic disabling of all XSAVE features Andi Kleen
                   ` (2 preceding siblings ...)
  2017-10-05 21:52 ` [PATCH v8 3/5] x86/cpuid: Make clearcpuid an early param Andi Kleen
@ 2017-10-05 21:52 ` Andi Kleen
  2017-11-11 23:15   ` [v8, " Guenter Roeck
  2019-06-29 15:22   ` [PATCH v8 " Vegard Nossum
  2017-10-05 21:52 ` [PATCH v8 5/5] x86/xsave: Remove the explicit clearing of XSAVE dependend features Andi Kleen
  4 siblings, 2 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 21:52 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v8 5/5] x86/xsave: Remove the explicit clearing of XSAVE dependend features
  2017-10-05 21:52 Support generic disabling of all XSAVE features Andi Kleen
                   ` (3 preceding siblings ...)
  2017-10-05 21:52 ` [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling Andi Kleen
@ 2017-10-05 21:52 ` Andi Kleen
  4 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 21:52 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies
  2017-10-05 21:52 ` [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies Andi Kleen
@ 2017-10-05 22:41   ` Thomas Gleixner
  2017-10-05 23:08     ` Andi Kleen
  2017-10-05 23:34     ` Andi Kleen
  0 siblings, 2 replies; 28+ messages in thread
From: Thomas Gleixner @ 2017-10-05 22:41 UTC (permalink / raw)
  To: Andi Kleen; +Cc: x86, hpa, linux-kernel, Andi Kleen, Jonathan McDowell

On Thu, 5 Oct 2017, Andi Kleen wrote:
> +/*
> + * 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_AVX512F,		X86_FEATURE_XSAVE     },

This looks redundant. AVX512F depends on AVX which itself depends on XSAVE.

> +	{ X86_FEATURE_PKU,		X86_FEATURE_XSAVE     },
> +	{ X86_FEATURE_MPX,		X86_FEATURE_XSAVE     },
> +	{ X86_FEATURE_XGETBV1,		X86_FEATURE_XSAVE     },
> +	{ X86_FEATURE_XMM2,		X86_FEATURE_XMM       },

XMM has no dependency?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies
  2017-10-05 22:41   ` Thomas Gleixner
@ 2017-10-05 23:08     ` Andi Kleen
  2017-10-05 23:34     ` Andi Kleen
  1 sibling, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 23:08 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andi Kleen, x86, hpa, linux-kernel, Jonathan McDowell

On Fri, Oct 06, 2017 at 12:41:56AM +0200, Thomas Gleixner wrote:
> On Thu, 5 Oct 2017, Andi Kleen wrote:
> > +/*
> > + * 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_AVX512F,		X86_FEATURE_XSAVE     },
> 
> This looks redundant. AVX512F depends on AVX which itself depends on XSAVE.

True.

> 
> > +	{ X86_FEATURE_PKU,		X86_FEATURE_XSAVE     },
> > +	{ X86_FEATURE_MPX,		X86_FEATURE_XSAVE     },
> > +	{ X86_FEATURE_XGETBV1,		X86_FEATURE_XSAVE     },
> > +	{ X86_FEATURE_XMM2,		X86_FEATURE_XMM       },
> 
> XMM has no dependency?

Only FXSAVE, I'll add that.

Thanks,

-Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies
  2017-10-05 22:41   ` Thomas Gleixner
  2017-10-05 23:08     ` Andi Kleen
@ 2017-10-05 23:34     ` Andi Kleen
  1 sibling, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-05 23:34 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andi Kleen, x86, hpa, linux-kernel, Jonathan McDowell

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)
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   | 107 +++++++++++++++++++++++++++++++++++++
 4 files changed, 117 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..172301b08ad0
--- /dev/null
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -0,0 +1,107 @@
+/* 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_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_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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-10-05 21:52 ` [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling Andi Kleen
@ 2017-11-11 23:15   ` Guenter Roeck
  2017-11-12  6:41     ` Andi Kleen
  2019-06-29 15:22   ` [PATCH v8 " Vegard Nossum
  1 sibling, 1 reply; 28+ messages in thread
From: Guenter Roeck @ 2017-11-11 23:15 UTC (permalink / raw)
  To: Andi Kleen; +Cc: x86, hpa, linux-kernel, Andi Kleen

On Thu, Oct 05, 2017 at 02:52:55PM -0700, 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>

this patch makes qemu spit out the following warning, possibly because
it doesn't support all expected CPUID features for the various CPUs.

WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.c:614
	fpu__init_system_xstate+0x403/0x721

with associated traceback.

I understand that this is a qemu problem, but does the traceback really
add value ?

Guenter

> ---
>  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: */

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-11-11 23:15   ` [v8, " Guenter Roeck
@ 2017-11-12  6:41     ` Andi Kleen
  2017-11-14  1:28       ` Andi Kleen
  0 siblings, 1 reply; 28+ messages in thread
From: Andi Kleen @ 2017-11-12  6:41 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Andi Kleen, x86, hpa, linux-kernel

> this patch makes qemu spit out the following warning, possibly because
> it doesn't support all expected CPUID features for the various CPUs.
> 
> WARNING: CPU: 0 PID: 0 at arch/x86/kernel/fpu/xstate.c:614
> 	fpu__init_system_xstate+0x403/0x721
> 
> with associated traceback.
> 
> I understand that this is a qemu problem, but does the traceback really
> add value ?

I agree it could be removed.

-Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-11-12  6:41     ` Andi Kleen
@ 2017-11-14  1:28       ` Andi Kleen
  2017-11-14  1:49         ` Guenter Roeck
  2017-11-14  2:33         ` Guenter Roeck
  0 siblings, 2 replies; 28+ messages in thread
From: Andi Kleen @ 2017-11-14  1:28 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Guenter Roeck, Andi Kleen, x86, hpa, linux-kernel

Guenter,

Do you have a command line that reproduces it and the exact log
output?

I can't reproduce it here with different qemu settings.

-Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-11-14  1:28       ` Andi Kleen
@ 2017-11-14  1:49         ` Guenter Roeck
  2017-11-16 19:03           ` Andi Kleen
  2017-11-14  2:33         ` Guenter Roeck
  1 sibling, 1 reply; 28+ messages in thread
From: Guenter Roeck @ 2017-11-14  1:49 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, x86, hpa, linux-kernel

On Mon, Nov 13, 2017 at 05:28:05PM -0800, Andi Kleen wrote:
> Guenter,
> 
> Do you have a command line that reproduces it and the exact log
> output?
> 
Hi Andi,

It is what I use at kerneltests.org.
See https://github.com/groeck/linux-build-test/tree/master/rootfs/x86_64
for configuration and script.

> I can't reproduce it here with different qemu settings.
> 
Interesting. What version of qemu are you using ?

Guenter

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-11-14  1:28       ` Andi Kleen
  2017-11-14  1:49         ` Guenter Roeck
@ 2017-11-14  2:33         ` Guenter Roeck
  1 sibling, 0 replies; 28+ messages in thread
From: Guenter Roeck @ 2017-11-14  2:33 UTC (permalink / raw)
  To: Andi Kleen, Andi Kleen; +Cc: x86, hpa, linux-kernel

On 11/13/2017 05:28 PM, Andi Kleen wrote:
> Guenter,
> 
> Do you have a command line that reproduces it and the exact log
> output?
> 

Sorry, I forgot: Logs are at http://kerneltests.org/builders/qemu-x86_64-next.

Guenter

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-11-14  1:49         ` Guenter Roeck
@ 2017-11-16 19:03           ` Andi Kleen
  2017-11-16 22:13             ` Guenter Roeck
  0 siblings, 1 reply; 28+ messages in thread
From: Andi Kleen @ 2017-11-16 19:03 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Andi Kleen, Andi Kleen, x86, hpa, linux-kernel

> It is what I use at kerneltests.org.
> See https://github.com/groeck/linux-build-test/tree/master/rootfs/x86_64
> for configuration and script.
> 
> > I can't reproduce it here with different qemu settings.
> > 
> Interesting. What version of qemu are you using ?

I tested with your config: -no-kvm -cpu Broadwell-noTSX -M q35 and still
don't see any boot warnings. This is 

QEMU emulator version 2.9.1(qemu-2.9.1-2.fc26)

If you're on an older version it may be already fixed in qemu then.

-Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [v8, 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-11-16 19:03           ` Andi Kleen
@ 2017-11-16 22:13             ` Guenter Roeck
  0 siblings, 0 replies; 28+ messages in thread
From: Guenter Roeck @ 2017-11-16 22:13 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, x86, hpa, linux-kernel

On Thu, Nov 16, 2017 at 11:03:47AM -0800, Andi Kleen wrote:
> > It is what I use at kerneltests.org.
> > See https://github.com/groeck/linux-build-test/tree/master/rootfs/x86_64
> > for configuration and script.
> > 
> > > I can't reproduce it here with different qemu settings.
> > > 
> > Interesting. What version of qemu are you using ?
> 
> I tested with your config: -no-kvm -cpu Broadwell-noTSX -M q35 and still
> don't see any boot warnings. This is 
> 
> QEMU emulator version 2.9.1(qemu-2.9.1-2.fc26)
> 
> If you're on an older version it may be already fixed in qemu then.
> 

I see the problem with both qemu 2.9.1 and 2.10.1. Interestingly, I don't
see it with an official Debian 2.5.0 build. Maybe it is the result of some
configuration option in my build.

Let's just ignore it for now.

Guenter

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2017-10-05 21:52 ` [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling Andi Kleen
  2017-11-11 23:15   ` [v8, " Guenter Roeck
@ 2019-06-29 15:22   ` Vegard Nossum
  2019-07-01 12:17     ` Sebastian Andrzej Siewior
  2019-07-01 16:39     ` Andi Kleen
  1 sibling, 2 replies; 28+ messages in thread
From: Vegard Nossum @ 2019-06-29 15:22 UTC (permalink / raw)
  To: Andi Kleen, x86; +Cc: hpa, linux-kernel, Andi Kleen


On 10/5/17 11:52 PM, 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>
> ---
>   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: */
> 

Hi,

The commit for this patch in mainline
(ccb18db2ab9d923df07e7495123fe5fb02329713) causes the kernel to hang on
boot when passing the "nofxsr" option:

$ kvm -cpu host -kernel arch/x86/boot/bzImage -append "console=ttyS0 
nofxsr earlyprintk=ttyS0" -serial stdio -display none -smp 2
early console in extract_kernel
input_data: 0x0000000001dea276
input_len: 0x0000000000500704
output: 0x0000000001000000
output_len: 0x00000000012c79b4
kernel_total_size: 0x0000000000f24000
booted via startup_32()
Physical KASLR using RDRAND RDTSC...
Virtual KASLR using RDRAND RDTSC...

Decompressing Linux... Parsing ELF... Performing relocations... done.
Booting the kernel.
[..hang..]

If I revert it from Linus's tree (~5.2-rc6) then it boots again:

early console in extract_kernel
input_data: 0x00000000024192e9
input_len: 0x00000000005d8ea1
output: 0x0000000001000000
output_len: 0x00000000019c7fa4
kernel_total_size: 0x000000000162c000
trampoline_32bit: 0x000000000009d000
booted via startup_32()
Physical KASLR using RDRAND RDTSC...
Virtual KASLR using RDRAND RDTSC...

Decompressing Linux... Parsing ELF... Performing relocations... done.
Booting the kernel.
Linux version 5.2.0-rc6+ (vegard@t460) (gcc version 5.5.0 20171010 
(Ubuntu 5.5.0-12ubuntu1~16.04)) #98 SMP PREEMPT Sat Jun 29 17:13:31 CEST 
2019
Command line: console=ttyS0 nofxsr earlyprintk=ttyS0
[..normal boot..]

/proc/cpuinfo inside the VM is:

processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 78
model name      : Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
stepping        : 3
microcode       : 0x1
cpu MHz         : 2496.000
cache size      : 4096 KB
physical id     : 0
siblings        : 1
core id         : 0
cpu cores       : 1
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 13
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge 
mca cmov pat pse36 clflush mmx fxsr sse sse2 ss syscall nx pdpe1gb 
rdtscp lm constant_tsc arch_perfmon rep_good nopl cpuid tsc_known_freq 
pni pclmulqdq vmx ssse3 fma cx16 pcid sse4_1 sse4_2 x2apic movbe popcnt 
tsc_deadline_timer aes xsave avx f16c rdrand hypervisor lahf_lm abm 
3dnowprefetch cpuid_fault invpcid_single pti ssbd ibrs ibpb tpr_shadow 
vnmi flexpriority ept vpid ept_ad fsgsbase tsc_adjust bmi1 hle avx2 smep 
bmi2 erms invpcid rtm mpx rdseed adx smap clflushopt xsaveopt xsavec 
xgetbv1 xsaves arat
bugs            : cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass 
l1tf mds
bogomips        : 4992.00
clflush size    : 64
cache_alignment : 64
address sizes   : 40 bits physical, 48 bits virtual
power management:


Vegard

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2019-06-29 15:22   ` [PATCH v8 " Vegard Nossum
@ 2019-07-01 12:17     ` Sebastian Andrzej Siewior
  2019-07-01 16:41       ` Andi Kleen
  2019-07-01 16:39     ` Andi Kleen
  1 sibling, 1 reply; 28+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-07-01 12:17 UTC (permalink / raw)
  To: Vegard Nossum; +Cc: Andi Kleen, x86, hpa, linux-kernel, Andi Kleen

On 2019-06-29 17:22:59 [+0200], Vegard Nossum wrote:
> The commit for this patch in mainline
> (ccb18db2ab9d923df07e7495123fe5fb02329713) causes the kernel to hang on
> boot when passing the "nofxsr" option:

as a result of nofxsr we do:
[0]	setup_clear_cpu_cap(X86_FEATURE_FXSR);
[1]	setup_clear_cpu_cap(X86_FEATURE_FXSR_OPT);
[2]	setup_clear_cpu_cap(X86_FEATURE_XMM);


the commit in question removes then XFEATURE_MASK_SSE from
`xfeatures_mask'.
Boot stops in fpu__init_cpu_xstate() / xsetbv() due to #GP:
|If an attempt is made to set XCR0[2:1] to 10b.
(from Vol. 2C).

[1] is "harmless". Dropping [2] does not fix the issue because [0]
still clears all three flags due to 
| static const struct cpuid_dep cpuid_deps[] = {
…
|      { X86_FEATURE_XMM,              X86_FEATURE_FXSR      },

Clearing additionally XMM2 (and adding the missing bits to
xsave_cpuid_features/xfeature_names) would boot further.
Later it crashes in raid6 while probing for AVX/2 code…

Disabling XMM+XMM2 in order get (and fixing it up for AVX+AVX2) would
give use XSAVE instead of FSAVE.
This won't work on 64bit userland because it expects SSE to be around
(and FXSR to save the SSE bits).
Even my 32bit Debian Wheezy doesn't work because it wants FXSR :)

So if it is unlikely to have XSAVE but no FXSR I would suggest to add
"fpu__xstate_clear_all_cpu_caps()" to nofxsr and behave like "nofxsr
noxsave".

Sebastian

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2019-06-29 15:22   ` [PATCH v8 " Vegard Nossum
  2019-07-01 12:17     ` Sebastian Andrzej Siewior
@ 2019-07-01 16:39     ` Andi Kleen
  1 sibling, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2019-07-01 16:39 UTC (permalink / raw)
  To: Vegard Nossum; +Cc: Andi Kleen, x86, hpa, linux-kernel

> 
> The commit for this patch in mainline
> (ccb18db2ab9d923df07e7495123fe5fb02329713) causes the kernel to hang on
> boot when passing the "nofxsr" option:

Thanks.

Hmm, I'm not sure nofxsr ever worked on 64bit. Certainly SSE cannot be
saved/restored in any other way during the context switch. 

So even if you pass it successfully I doubt user space will really work
for very long.  64bit binaries require SSE.

AFAIK it is only useful on systems without SSE, presumably
running 32bit kernels.

Should check that case.

My recommended solution would be to just get rid of the option.

Presumably it's just some old chicken bit.

-Anfi


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling
  2019-07-01 12:17     ` Sebastian Andrzej Siewior
@ 2019-07-01 16:41       ` Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2019-07-01 16:41 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Vegard Nossum, Andi Kleen, x86, hpa, linux-kernel

> So if it is unlikely to have XSAVE but no FXSR I would suggest to add
> "fpu__xstate_clear_all_cpu_caps()" to nofxsr and behave like "nofxsr
> noxsave".

Thanks for the analysis Sebastian. Makes sense.

This would likely work, but I think I would rather just remove the option.

-Andi

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-10-13 21:56 Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-13 21:56 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-10-07  0:03 Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-07  0:03 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-10-04 23:49 Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-10-04 23:49 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-09-19 22:26 Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-09-19 22:26 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-07-25  0:43 Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-07-25  0:43 UTC (permalink / raw)
  To: x86; +Cc: hpa, linux-kernel

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-06-21 23:41 Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-06-21 23:41 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: Support generic disabling of all XSAVE features
  2017-06-07 23:29 Andi Kleen
@ 2017-06-13  0:17 ` Andi Kleen
  0 siblings, 0 replies; 28+ messages in thread
From: Andi Kleen @ 2017-06-13  0:17 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Support generic disabling of all XSAVE features
@ 2017-06-07 23:29 Andi Kleen
  2017-06-13  0:17 ` Andi Kleen
  0 siblings, 1 reply; 28+ messages in thread
From: Andi Kleen @ 2017-06-07 23:29 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2019-07-01 16:41 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-05 21:52 Support generic disabling of all XSAVE features Andi Kleen
2017-10-05 21:52 ` [PATCH v8 1/5] x86/xsave: Move xsave initialization to after parsing early parameters Andi Kleen
2017-10-05 21:52 ` [PATCH v8 2/5] x86/cpuid: Add generic table for cpuid dependencies Andi Kleen
2017-10-05 22:41   ` Thomas Gleixner
2017-10-05 23:08     ` Andi Kleen
2017-10-05 23:34     ` Andi Kleen
2017-10-05 21:52 ` [PATCH v8 3/5] x86/cpuid: Make clearcpuid an early param Andi Kleen
2017-10-05 21:52 ` [PATCH v8 4/5] x86/xsave: Make XSAVE check the base CPUID features before enabling Andi Kleen
2017-11-11 23:15   ` [v8, " Guenter Roeck
2017-11-12  6:41     ` Andi Kleen
2017-11-14  1:28       ` Andi Kleen
2017-11-14  1:49         ` Guenter Roeck
2017-11-16 19:03           ` Andi Kleen
2017-11-16 22:13             ` Guenter Roeck
2017-11-14  2:33         ` Guenter Roeck
2019-06-29 15:22   ` [PATCH v8 " Vegard Nossum
2019-07-01 12:17     ` Sebastian Andrzej Siewior
2019-07-01 16:41       ` Andi Kleen
2019-07-01 16:39     ` Andi Kleen
2017-10-05 21:52 ` [PATCH v8 5/5] x86/xsave: Remove the explicit clearing of XSAVE dependend features Andi Kleen
  -- strict thread matches above, loose matches on Subject: below --
2017-10-13 21:56 Support generic disabling of all XSAVE features Andi Kleen
2017-10-07  0:03 Andi Kleen
2017-10-04 23:49 Andi Kleen
2017-09-19 22:26 Andi Kleen
2017-07-25  0:43 Andi Kleen
2017-06-21 23:41 Andi Kleen
2017-06-07 23:29 Andi Kleen
2017-06-13  0:17 ` Andi Kleen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).