Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1672160 > unrolled thread

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

Started byAndi Kleen <andi@firstfloor.org>
First post2017-06-22 01:50 +0200
Last post2017-06-22 12:10 +0200
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies Andi Kleen <andi@firstfloor.org> - 2017-06-22 01:50 +0200
    Re: [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies Jonathan McDowell <noodles@earth.li> - 2017-06-22 12:10 +0200

#1672160 — [PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies

FromAndi Kleen <andi@firstfloor.org>
Date2017-06-22 01:50 +0200
Subject[PATCH 2/5] x86/cpuid: Add generic table for cpuid dependencies
Message-ID<tUXxM-4Ng-21@gated-at.bofh.it>
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_id for lguest
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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 <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <asm/cpufeature.h>
+
+struct cpuid_dep {
+	int feature;
+	int dep;
+};
+
+/*
+ * 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 },
+	{ 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;
+	}
+}
+
+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);
+}
-- 
2.9.4

[toc] | [next] | [standalone]

#1672488

FromJonathan McDowell <noodles@earth.li>
Date2017-06-22 12:10 +0200
Message-ID<tV7dM-3dT-23@gated-at.bofh.it>
In reply to#1672160
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 <ak@linux.intel.com>
> ---
>  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 <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <asm/cpufeature.h>
> +
> +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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web