| 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
| ||
|
Message-ID: <20171012080733.4y5wlghjr524cgr3@gmail.com>
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
Powered by blists - more mailing lists