Messages in this thread | | | Date | Wed, 18 Jun 2014 19:21:11 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v6 03/10] x86, mpx: add macro cpu_has_mpx |
| |
On Wed, Jun 18, 2014 at 09:25:57AM -0700, Dave Hansen wrote: > On 06/18/2014 07:59 AM, H. Peter Anvin wrote: > > On 06/18/2014 07:35 AM, Dave Hansen wrote: > >> It looks like static_cpu_has() is the right thing to use instead of > >> boot_cpu_has(). But, this doesn't just obfuscate things. > >> > >> We actually _want_ the compiler to cull code out when the config option > >> is off. Things like do_bounds() will see code savings with _some_ kind > >> of #ifdef rather than using static_cpu_has(). > >> > >> So, we can either use the well worn, consistent with other features in > >> x86, cpu_has_$foo approach. Or, we can roll our own macros. > > > > We could do something like: > > > > #define MPX_ENABLED (IS_ENABLED(CONFIG_X86_MPX) && > > static_cpu_has(X86_FEATURE_MPX)) > > How about something like the attached patch? > > This lets us use static_cpu_has() for the checks, and allows us to > easily add new checks for other features that might be compile-time > disabled.
> > > --- > > b/arch/x86/include/asm/cpufeature.h | 26 ++++++++++++++++++++------ > b/arch/x86/kernel/mpx.c | 4 ++-- > b/arch/x86/kernel/traps.c | 2 +- > 3 files changed, 23 insertions(+), 9 deletions(-) > > diff -puN arch/x86/include/asm/cpufeature.h~x86-disabled_mask arch/x86/include/asm/cpufeature.h > --- a/arch/x86/include/asm/cpufeature.h~x86-disabled_mask 2014-06-18 08:48:41.329750895 -0700 > +++ b/arch/x86/include/asm/cpufeature.h 2014-06-18 09:19:19.143546973 -0700 > @@ -339,12 +339,6 @@ extern const char * const x86_power_flag > #define cpu_has_eager_fpu boot_cpu_has(X86_FEATURE_EAGER_FPU) > #define cpu_has_topoext boot_cpu_has(X86_FEATURE_TOPOEXT) > > -#ifdef CONFIG_X86_INTEL_MPX > -#define cpu_has_mpx boot_cpu_has(X86_FEATURE_MPX) > -#else > -#define cpu_has_mpx 0 > -#endif /* CONFIG_X86_INTEL_MPX */ > - > #ifdef CONFIG_X86_64 > > #undef cpu_has_vme > @@ -367,6 +361,22 @@ extern const char * const x86_power_flag > > #endif /* CONFIG_X86_64 */ > > +/* > + * Add features and their corresponding config options here > + * if you want to have the compiler optimize out code that > + * uses them. > + * > + * You should not use this function directly. Use > + * static_cpu_has() so that you also benefit from alternatives > + * when the features are enabled. > + */ > +static __always_inline int __cpu_feature_compile_enabled(u16 bit) > +{ > + if (bit == X86_FEATURE_MPX) > + return IS_ENABLED(CONFIG_X86_MPX);
Right, this should be CONFIG_X86_INTEL_MPX but that's details.
I guess this might grow into a big if-else noodle case but when that happens, we probably could add some sort of mapping between X86_FEATURE bits to CONFIG_ items or so.
Then, you could probably add this call to the enclosing macros static_cpu_has{,_safe} so that it gets evaluated first. The advantage is that the IS_ENABLED test would work very early - always, actually - even before alternatives have run and thus catch the cases where the feature is config-disabled.
Otherwise, asm looks ok.
-- Regards/Gruss, Boris.
Sent from a fat crate under my desk. Formatting is fine. --
| |