From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751482AbdJEN0H (ORCPT ); Thu, 5 Oct 2017 09:26:07 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:33967 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751280AbdJEN0F (ORCPT ); Thu, 5 Oct 2017 09:26:05 -0400 Date: Thu, 5 Oct 2017 15:25:52 +0200 (CEST) From: Thomas Gleixner To: Andi Kleen cc: x86@kernel.org, "H. Peter Anvin" , LKML , Andi Kleen , Jonathan McDowell , Borislav Petkov Subject: Re: [PATCH v7 2/5] x86/cpuid: Add generic table for cpuid dependencies In-Reply-To: <20171004234930.5685-3-andi@firstfloor.org> Message-ID: References: <20171004234930.5685-1-andi@firstfloor.org> <20171004234930.5685-3-andi@firstfloor.org> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > +#include > +#include > +#include > + > +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