Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753081AbdFVKDG (ORCPT ); Thu, 22 Jun 2017 06:03:06 -0400 Received: from the.earth.li ([46.43.34.31]:38652 "EHLO the.earth.li" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752959AbdFVKDF (ORCPT ); Thu, 22 Jun 2017 06:03:05 -0400 X-Greylist: delayed 2038 seconds by postgrey-1.27 at vger.kernel.org; Thu, 22 Jun 2017 06:03:04 EDT Date: Thu, 22 Jun 2017 10:28:59 +0100 From: Jonathan McDowell To: andi@firstfloor.org Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies Message-ID: <20170622092859.GA3714@earth.li> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170621234106.16548-3-andi@firstfloor.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8003 Lines: 202 In article <20170621234106.16548-3-andi@firstfloor.org> you wrote: > Some CPUID features depend on other features. Currently it's > possible to to clear dependent features, but not clear the base features, > which can cause various interesting problems. > This patch implements a generic table to describe dependencies > between CPUID features, to be used by all code that clears > CPUID. > Some subsystems (like XSAVE) had an own implementation of this, > but it's better to do it all in a single place for everyone. > Then clear_cpu_cap and setup_clear_cpu_cap always look up > this table and clear all dependencies too. > This is intended to be a practical table: only for features > that make sense to clear. If someone for example clears FPU, > or other features that are essentially part of the required > base feature set, not much is going to work. Handling > that is right now out of scope. We're only handling > features which can be usefully cleared. > v2: Add EXPORT_SYMBOL for clear_cpu_id for lguest > Signed-off-by: Andi Kleen > --- > arch/x86/include/asm/cpufeature.h | 8 +++- > arch/x86/include/asm/cpufeatures.h | 5 +++ > arch/x86/kernel/cpu/Makefile | 1 + > arch/x86/kernel/cpu/cpuid-deps.c | 92 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 104 insertions(+), 2 deletions(-) > create mode 100644 arch/x86/kernel/cpu/cpuid-deps.c > diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h > index d59c15c3defd..e6145f383ff8 100644 > --- a/arch/x86/include/asm/cpufeature.h > +++ b/arch/x86/include/asm/cpufeature.h > @@ -125,8 +125,12 @@ extern const char * const x86_bug_flags[NBUGINTS*32]; > #define boot_cpu_has(bit) cpu_has(&boot_cpu_data, bit) > > #define set_cpu_cap(c, bit) set_bit(bit, (unsigned long *)((c)->x86_capability)) > -#define clear_cpu_cap(c, bit) clear_bit(bit, (unsigned long *)((c)->x86_capability)) > -#define setup_clear_cpu_cap(bit) do { \ > +#define __clear_cpu_cap(c, bit) clear_bit(bit, (unsigned long *)((c)->x86_capability)) > + > +extern void setup_clear_cpu_cap(int bit); > +extern void clear_cpu_cap(struct cpuinfo_x86 *cpu, int bit); > + > +#define __setup_clear_cpu_cap(bit) do { \ > clear_cpu_cap(&boot_cpu_data, bit); \ > set_bit(bit, (unsigned long *)cpu_caps_cleared); \ > } while (0) > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > index 2701e5f8145b..8f371a5966e7 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 52000010c62e..274fc0fee1e1 100644 > --- a/arch/x86/kernel/cpu/Makefile > +++ b/arch/x86/kernel/cpu/Makefile > @@ -21,6 +21,7 @@ obj-y += common.o > obj-y += rdrand.o > obj-y += match.o > obj-y += bugs.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..08aff02cf2ff > --- /dev/null > +++ b/arch/x86/kernel/cpu/cpuid-deps.c > @@ -0,0 +1,92 @@ > +/* Declare dependencies between CPUIDs */ > +#include > +#include > +#include > +#include > + > +struct cpuid_dep { > + int feature; > + int dep; > +}; This seems a little confusing; I had to read through a couple of times to understand that "dep" represents the feature that needs to be disabled if "feature" is disabled, rather than dep being a dependency of feature. > + > +/* > + * Table of CPUID features that depend on others. > + * > + * This only includes dependencies that can be usefully disabled, not > + * features part of the base set (like FPU). > + */ > +const static struct cpuid_dep cpuid_deps[] = { > + { X86_FEATURE_XSAVE, X86_FEATURE_XSAVEOPT }, > + { X86_FEATURE_XSAVE, X86_FEATURE_XSAVEC }, > + { X86_FEATURE_XSAVE, X86_FEATURE_XSAVES }, > + { X86_FEATURE_XSAVE, X86_FEATURE_AVX }, > + { X86_FEATURE_XSAVE, X86_FEATURE_AVX512F }, > + { X86_FEATURE_XSAVE, X86_FEATURE_PKU }, > + { X86_FEATURE_XSAVE, X86_FEATURE_MPX }, > + { X86_FEATURE_XSAVE, X86_FEATURE_XGETBV1 }, > + { X86_FEATURE_XMM, X86_FEATURE_XMM2 }, > + { X86_FEATURE_XMM2, X86_FEATURE_XMM3 }, > + { X86_FEATURE_XMM2, X86_FEATURE_XMM4_1 }, > + { X86_FEATURE_XMM2, X86_FEATURE_XMM4_2 }, > + { X86_FEATURE_XMM2, X86_FEATURE_XMM3 }, > + { X86_FEATURE_XMM2, X86_FEATURE_PCLMULQDQ }, > + { X86_FEATURE_XMM2, X86_FEATURE_SSSE3 }, > + { X86_FEATURE_FMA, X86_FEATURE_AVX }, > + { X86_FEATURE_XMM2, X86_FEATURE_F16C }, > + { X86_FEATURE_XMM2, X86_FEATURE_AES }, > + { X86_FEATURE_XSAVE, X86_FEATURE_AVX }, > + { X86_FEATURE_XSAVE, X86_FEATURE_AVX512F }, Duplicates from above. (Might it be a better idea for the table to be sorted to ease spotting such things and future patch merging?) > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512IFMA }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512PF }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512ER }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512CD }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512DQ }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512BW }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512VL }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512VBMI }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4VNNIW }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512_4FMAPS }, > + { X86_FEATURE_AVX512F, X86_FEATURE_AVX512_VPOPCNTDQ }, > + { X86_FEATURE_AVX, X86_FEATURE_AVX2 }, > + {} > +}; > + > +static void do_clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat) > +{ > + int i, newfeat; > + bool changed; > + > + if (!cpu) > + __setup_clear_cpu_cap(feat); > + else > + __clear_cpu_cap(cpu, feat); > +again: > + changed = false; > + for (i = 0; cpuid_deps[i].feature; i++) { > + if (feat == cpuid_deps[i].feature) { > + newfeat = cpuid_deps[i].dep; > + if (!cpu) > + __setup_clear_cpu_cap(newfeat); > + else > + __clear_cpu_cap(cpu, newfeat); > + changed = true; > + } > + } > + /* Handle multi-level dependencies */ > + if (changed) { > + feat = newfeat; > + goto again; > + } I don't think this works? You're only picking up the last newfeat to process again. So if I disable X86_FEATURE_XSAVE the last newfeat will be X86_FEATURE_AVX512F and the code won't correctly disable the features which require X86_FEATURE_AVX? > +} > + > +void clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat) > +{ > + do_clear_cpu_cap(cpu, feat); > +} > + > +EXPORT_SYMBOL_GPL(clear_cpu_cap); > + > +void setup_clear_cpu_cap(int feat) > +{ > + do_clear_cpu_cap(NULL, feat); > +} J. -- Web [ < fivemack> it is bruter-force than a really really stupid ] site: http:// [ elephant [on his Python suduku solver] ] Made by www.earth.li/~noodles/ [ ] HuggieTag 0.0.24