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

Ingo Molnar
2017-10-12T08:07:38Z

* Andi Kleen <andi@firstfloor.org> wrote:

> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -0,0 +1,109 @@
> +/* 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;
> +};

Why are these 16-bit fields? 16-bit data types should be avoided as much as
possible, as they generate suboptimal code.

> +
> +/*
> + * 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_FXSR_OPT, X86_FEATURE_FXSR },
> + { 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_SHA_NI, 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 },
> + {}
> +};

Why isn't this __initdata?

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

Sloppy types: the canonical form is the longer 'unsigned int'..

Plus there's local variable naming sloppiness as well: 'cinfo' is not used
anywhere in the current x86 code, introducing a new idiosyncratic naming variant
just increases the complexity of the kernel unnecessarily.

When unsure about what the canonical naming of a commonly used structure is, you
can type:

git grep 'struct cpuinfo_x86'

and see how this variable is typically named in over 200 other cases.

> + bool changed;
> + __u32 disable[NCAPINTS + NBUGINTS];
> + unsigned long *disable_mask = (unsigned long *)disable;
> + const struct cpuid_dep *d;

The constant forced types are really ugly and permeate the whole code. Please
introduce some suitably named helpers in the x86 bitops file that work on local
variables:

var &= ~(1<<bit);
var |= 1<<bit;

Those helpers could be used on the natural u32 type of these fields without
constantly having to force types between u32 and long.

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

Nit: please add a newline to vertically separate the loop initialization from the
loop.

> + 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);
> +}

There's naming confusion here: sometimes a CPU feature bit is called a 'cap'
(capability), sometimes 'feat' (feature). Pick 'feature' and use it consistently.

Please review the whole patch set for similar patterns of bugs/problems.

Thanks,

Ingo