Re: [PATCH v2 3/6] arm64: HWCAP: encapsulate elf_hwcap

[Andrew Murray <andrew.murray@arm.com>]

On Thu, Feb 21, 2019 at 06:45:08PM +0000, Dave P Martin wrote:
> On Thu, Feb 21, 2019 at 12:20:54PM +0000, Andrew Murray wrote:
> > The introduction of AT_HWCAP2 introduced accessors which ensure that
> > hwcap features are set and tested appropriately.
> > 
> > Let's now mandate access to elf_hwcap via these accessors by making
> > elf_hwcap static within cpufeature.c.
> 
> Since elf_hwcap now survives and retains a compatible encoding for
> HWCAP_foo, I'm wondering whether it would be simpler to drop this patch.
> 
> Although this will help push people to use the new helpers, the need to
> do that seems reduced now.
> 
> People falling off the end of the hwcaps will discover that there is no
> HWCAP_foo for the feature they want, only HWCAP2_foo (but no elf_hwcap2
> to look for it in), or KERNEL_HWCAP_foo (which should get them
> thinking).
> 
> What do you think?

You're right that people will find the appropiate functions to set the
relevant hwcaps. But I think my motivation for this comes from the
perspective of code maintainability...

There is absolutely no functional benefit to exposing the elf_hwcap
variable to the rest of the kernel - yet doing so will result in users
using elf_hwcap instead of the helpers. If we later change the type of
elf_hwcap, or split the bits into multiple variables (e.g. elf_hwcap2) or
even change the mapping from UAPI to kernel, then modifications need to be
made at each call site. Whereas if we reduce the visibility of elf_hwcap
then we encapsulate all the bit fiddling in one place.

Also, taking this approach forces us into this ugly suitation...

+#ifdef CONFIG_ARM64
+               if (cpu_have_feature_name(EVTSTRM))
+#else
                if (elf_hwcap & HWCAP_EVTSTRM)
+#endif

It's not nice, but it's a lot less fragile than expecting cross-platform
agreement on the bit value of particular hwcaps.

Is this a good enough reason to keep it?

Thanks,

Andrew Murray

> 
> [...]
> 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 6a477a3..d57a179 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -35,8 +35,7 @@
> >  #include <asm/traps.h>
> >  #include <asm/virt.h>
> >  
> > -unsigned long elf_hwcap __read_mostly;
> > -EXPORT_SYMBOL_GPL(elf_hwcap);
> > +static unsigned long elf_hwcap __read_mostly;
> >  
> >  #ifdef CONFIG_COMPAT
> >  #define COMPAT_ELF_HWCAP_DEFAULT	\
> > @@ -1909,6 +1908,30 @@ bool this_cpu_has_cap(unsigned int n)
> >  	return false;
> >  }
> >  
> > +void cpu_set_feature(unsigned int num)
> > +{
> > +	WARN_ON(num >= MAX_CPU_FEATURES);
> > +	elf_hwcap |= BIT(num);
> > +}
> > +EXPORT_SYMBOL_GPL(cpu_set_feature);
> > +
> > +bool cpu_have_feature(unsigned int num)
> > +{
> > +	WARN_ON(num >= MAX_CPU_FEATURES);
> > +	return elf_hwcap & BIT(num);
> > +}
> > +EXPORT_SYMBOL_GPL(cpu_have_feature);
> > +
> > +unsigned long cpu_get_elf_hwcap(void)
> > +{
> > +	return lower_32_bits(elf_hwcap);
> > +}
> > +
> > +unsigned long cpu_get_elf_hwcap2(void)
> > +{
> > +	return upper_32_bits(elf_hwcap);
> > +}
> > +
> 
> Similarly, pushing all this out of line to enable elf_hwcap to be hidden
> may be more effort than it is really worth (?)
> 
> Cheers
> ---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel