On Wed, 4 Oct 2017, Andi Kleen wrote:
Cc: +Borislav
> v2: Add EXPORT_SYMBOL for clear_cpu_id for lguest
clear_cpu_id? You probably mean clear_cpu_cap, but that's moot now that
lguest is gone......
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
...
> #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)
__clear_cpu_cap() and __setup_clear_cpu_cap() should not be in the header
file. They should be in the new cpuid-deps file so they are not
exposed. While at it please implement them as inlines. There is no reason
for having them as macros.
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -0,0 +1,94 @@
> +/* 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 disable;
> +};
> +
> +/*
> + * 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_XMM2, X86_FEATURE_F16C },
> + { X86_FEATURE_XMM2, X86_FEATURE_AES },
> + { X86_FEATURE_FMA, X86_FEATURE_AVX },
> + { 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 inline void clearfeat(struct cpuinfo_x86 *cpu, int feat)
What's wrong with giving this a readable name like clear_feature()? That
function name is definitely not a feat.
*cpu is the worst argument name you could come up with. I was not paying
attention when skimming the patch and assumed that the 'if (!cpu)' check
tests for cpu == 0, aka boot cpu.
Also the feature argument wants to be unsigned int all over the place.
> +{
> + if (!cpu)
> + __setup_clear_cpu_cap(feat);
> + else
> + __clear_cpu_cap(cpu, feat);
> +}
> +
> +static void do_clear_cpu_cap(struct cpuinfo_x86 *cpu, int feat)
> +{
> + bool changed;
> + __u32 disable[NCAPINTS + NBUGINTS];
> + unsigned long *disable_mask = (unsigned long *)disable;
> + const struct cpuid_dep *d;
> +
> + clearfeat(cpu, feat);
> +
> + /* Collect all features to disable, handling dependencies */
> + memset(disable, 0, sizeof(disable));
> + __set_bit(feat, disable_mask);
> + do {
> + changed = false;
> + for (d = cpuid_deps; d->feature; d++) {
> + if (test_bit(d->feature, disable_mask) &&
> + !__test_and_set_bit(d->disable, disable_mask)) {
The logic of this dependency structure is backwards. As the name says its
supposed to express dependencies. So it should actually do so:
struct cpuid_dep {
unsigned int feature;
unsigned int depends;
};
Then the logic here becomes immediately obvious:
for (d = cpuid_deps; d->feature; d++) {
if (!test_bit(d->depends, disabled_mask))
continue;
if (test_and_set_bit(d->feature, disabled_mask))
continue;
changed = true;
clear_feature(cpuinfo, d->feature);
}
Hmm?
Thanks,
tglx
