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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 12 Oct 2017 10:07:33 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Andi Kleen <andi@...stfloor.org>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Andi Kleen <ak@...ux.intel.com>,
        Jonathan McDowell <noodles@...th.li>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v9 2/5] x86/cpuid: Add generic table for cpuid
 dependencies


* Andi Kleen <andi@...stfloor.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