* 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ messages in thread
end of thread, other threads:[~2019-07-01 16:41 UTC | newest] Thread overview: 20+ 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
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).