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

[Jonathan McDowell]

In article <20170621234106.16548-3-andi@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
>  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