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

[Thomas Gleixner <tglx@linutronix.de>]

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