Support generic disabling of all XSAVE features

PATCH Superseded Andi Kleen 6 participants 20 messages Oct 5, 2017 – Jul 1, 2019 x86

Share & tools

Andi Kleen

author

2017-10-05 21:53 UTC #0 lore raw

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

Andi Kleen

author

2017-10-05 21:53 UTC #1 lore raw

PATCH v8 1/5 x86/xsave: Move xsave initialization to after parsing early parameters

From: Andi Kleen <[email protected]>

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

Reviewed-by Thomas Gleixner <[email protected]>

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

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

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

index c9176bae7fd8..fd47692e5ce9 100644
@@ -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

arch/x86/kernel/setup.c +3 -0

index 0957dd73d127..2260e586295f 100644
@@ -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

Signature · 1 line

2.13.6

Andi Kleen

author

2017-10-05 21:54 UTC #2 lore raw

PATCH v8 2/5 x86/cpuid: Add generic table for cpuid dependencies

In reply to Andi Kleen

From: Andi Kleen <[email protected]>

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

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

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

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

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

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

Cc Jonathan McDowell <[email protected]>

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

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

arch/x86/include/asm/cpufeature.h +4 -5

index d59c15c3defd..a0e13ac0c506 100644
@@ -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);	\

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

index 2519c6c801c9..401a70992060 100644
@@ -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 */

arch/x86/kernel/cpu/Makefile +1 -0

index e17942c131c8..de260fae1017 100644
@@ -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

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

107 lines not on this page · raw message

Signature · 1 line

2.13.6

Thomas Gleixner

2017-10-05 22:42 UTC #3 lore raw

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

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

Quoted 12 lines · Andi Kleen

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

+/*
+ * Table of CPUID features that depend on others.
+ *
+ * This only includes dependencies that can be usefully disabled, not
+ * features part of the base set (like FPU).
+ */
+const static struct cpuid_dep cpuid_deps[] = {
+	{ X86_FEATURE_XSAVEOPT,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVEC,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVES,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_AVX,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_AVX512F,		X86_FEATURE_XSAVE     },

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

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

Quoted 4 lines

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

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

Andi Kleen

2017-10-05 23:08 UTC #4 lore raw

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

Quoted 15 lines · Thomas Gleixner

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

Quoted 12 lines · Andi Kleen

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

+/*
+ * Table of CPUID features that depend on others.
+ *
+ * This only includes dependencies that can be usefully disabled, not
+ * features part of the base set (like FPU).
+ */
+const static struct cpuid_dep cpuid_deps[] = {
+	{ X86_FEATURE_XSAVEOPT,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVEC,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_XSAVES,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_AVX,		X86_FEATURE_XSAVE     },
+	{ X86_FEATURE_AVX512F,		X86_FEATURE_XSAVE     },

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

True.

Quoted 7 lines

arch/x86/kernel/cpu/cpuid-deps.c line 27
Quoted 5 lines
arch/x86/kernel/cpu/cpuid-deps.c new file +4 -0
+	{ 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

Andi Kleen

2017-10-05 23:35 UTC #5 lore raw

PATCH v8 2/5 x86/cpuid: Add generic table for cpuid dependencies

In reply to Thomas Gleixner

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.

Cc Jonathan McDowell <[email protected]>

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

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

arch/x86/include/asm/cpufeature.h +4 -5

index d59c15c3defd..a0e13ac0c506 100644
@@ -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);	\

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

index 2519c6c801c9..401a70992060 100644
@@ -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 */

arch/x86/kernel/cpu/Makefile +1 -0

index e17942c131c8..de260fae1017 100644
@@ -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

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

107 lines not on this page · raw message

Signature · 1 line

2.13.6

Andi Kleen

author

2017-10-05 21:53 UTC #6 lore raw

PATCH v8 3/5 x86/cpuid: Make clearcpuid an early param

In reply to Andi Kleen

From: Andi Kleen <[email protected]>

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

Reviewed-by Thomas Gleixner <[email protected]>

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

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

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

index fd47692e5ce9..ff51c61d2df0 100644
@@ -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,

Signature · 1 line

2.13.6

Andi Kleen

author

2017-10-05 21:53 UTC #7 lore raw

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

In reply to Andi Kleen

From: Andi Kleen <[email protected]>

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

v2:
Add curly brackets (Thomas Gleixner)

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

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

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

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

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

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

@@ -759,6 +774,14 @@ void __init fpu__init_system_xstate(void)
 		goto out_disable;
 	}
 
+	/*
+	 * 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: */

Signature · 1 line

2.13.6

Guenter Roeck

2017-11-11 23:15 UTC #8 lore raw

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

Quoted 11 lines · Andi Kleen

From: Andi Kleen <[email protected]>
Before enabling XSAVE, not only check the XSAVE specific CPUID bits, but also the base CPUID features of the respective XSAVE feature. This allows to disable individual XSAVE states using the existing clearcpuid= option, which can be useful for performance testing and debugging, and also in general avoids inconsistencies.
v2:
Add curly brackets (Thomas Gleixner)
Signed-off-by Andi Kleen <[email protected]>

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

Quoted 59 lines

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

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

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

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

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

@@ -759,6 +774,14 @@ void __init fpu__init_system_xstate(void)
 		goto out_disable;
 	}
 
+	/*
+	 * 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: */

Andi Kleen

2017-11-12 06:41 UTC #9 lore raw

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

Quoted 10 lines

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

Andi Kleen

author

2017-11-14 01:28 UTC #10 lore raw

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

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

Guenter Roeck

2017-11-14 01:49 UTC #11 lore raw

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

Quoted 5 lines · Andi Kleen

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.

Quoted 2 lines

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

Interesting. What version of qemu are you using ?

Guenter

Andi Kleen

author

2017-11-16 19:03 UTC #12 lore raw

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

Quoted 7 lines

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.
Quoted 2 lines
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

Guenter Roeck

2017-11-16 22:13 UTC #13 lore raw

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

Quoted 6 lines · Andi Kleen

Quoted 6 lines
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.
Quoted 2 lines
I can't reproduce it here with different qemu settings.

Quoted 9 lines

Quoted 1 line
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

Guenter Roeck

2017-11-14 02:33 UTC #14 lore raw

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

In reply to Andi Kleen

Quoted 5 lines · Andi Kleen

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

Vegard Nossum

2019-06-29 15:24 UTC #15 lore raw

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

In reply to Andi Kleen

Quoted 61 lines · Andi Kleen

From: Andi Kleen <[email protected]>

v2:
Add curly brackets (Thomas Gleixner)

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

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

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

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

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

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

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

arch/x86/kernel/fpu/xstate.c void __init fpu__init_system_xstate(void) · line 787

Quoted 10 lines

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

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

Sebastian Andrzej Siewior

2019-07-01 12:17 UTC #16 lore raw

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

Quoted 3 lines · Vegard Nossum

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

Andi Kleen

2019-07-01 16:41 UTC #17 lore raw

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

Quoted 3 lines

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

Andi Kleen

2019-07-01 16:39 UTC #18 lore raw

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

In reply to Vegard Nossum

Quoted 4 lines

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

Andi Kleen

author

2017-10-05 21:53 UTC #19 lore raw

PATCH v8 5/5 x86/xsave: Remove the explicit clearing of XSAVE dependend features

In reply to Andi Kleen

From: Andi Kleen <[email protected]>

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 <[email protected]>

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

---
 arch/x86/kernel/fpu/xstate.c | 20 --------------------
 1 file changed, 20 deletions(-)

arch/x86/kernel/fpu/xstate.c +0 ~~-20~~

index 924bd895b5ee..bfe8c0c1033d 100644
@@ -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);
 }
 
 /*

Signature · 1 line

2.13.6