From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754474AbdJLIHi (ORCPT ); Thu, 12 Oct 2017 04:07:38 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:34669 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752786AbdJLIHh (ORCPT ); Thu, 12 Oct 2017 04:07:37 -0400 X-Google-Smtp-Source: AOwi7QClYFg4hXqmb4eiQrCZOFgKlePTIyl6O8FVqb5IBHyiDb2Be04SqSM9h+SkPwyuC2UumhCQ7A== Date: Thu, 12 Oct 2017 10:07:33 +0200 From: Ingo Molnar To: Andi Kleen Cc: x86@kernel.org, linux-kernel@vger.kernel.org, Andi Kleen , Jonathan McDowell , Thomas Gleixner Subject: Re: [PATCH v9 2/5] x86/cpuid: Add generic table for cpuid dependencies Message-ID: <20171012080733.4y5wlghjr524cgr3@gmail.com> References: <20171007000328.12843-1-andi@firstfloor.org> <20171007000328.12843-3-andi@firstfloor.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171007000328.12843-3-andi@firstfloor.org> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Andi Kleen wrote: > --- /dev/null > +++ b/arch/x86/kernel/cpu/cpuid-deps.c > @@ -0,0 +1,109 @@ > +/* Declare dependencies between CPUIDs */ > +#include > +#include > +#include > +#include > + > +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< + > + 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