Re: [PATCH v4 1/6] arm64: HWCAP: add support for AT_HWCAP2

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

On Wed, Apr 03, 2019 at 05:33:03PM +0100, Dave Martin wrote:
> On Wed, Apr 03, 2019 at 05:06:23PM +0100, Andrew Murray wrote:
> > On Wed, Apr 03, 2019 at 02:21:12PM +0100, Dave Martin wrote:
> > > On Wed, Apr 03, 2019 at 11:56:23AM +0100, Andrew Murray wrote:
> > > > As we will exhaust the first 32 bits of AT_HWCAP let's start
> > > > exposing AT_HWCAP2 to userspace to give us up to 64 caps.
> > > > 
> > > > Whilst it's possible to use the remaining 32 bits of AT_HWCAP, we
> > > > prefer to expand into AT_HWCAP2 in order to provide a consistent
> > > > view to userspace between ILP32 and LP64. However internal to the
> > > > kernel we prefer to continue to use the full space of elf_hwcap.
> > > > 
> > > > To reduce complexity and allow for future expansion, we now
> > > > represent hwcaps in the kernel as ordinals and use a
> > > > KERNEL_HWCAP_ prefix. This allows us to support automatic feature
> > > > based module loading for all our hwcaps.
> > > > 
> > > > We introduce cpu_set_feature to set hwcaps which complements the
> > > > existing cpu_have_feature helper. These helpers allow us to clean
> > > > up existing direct uses of elf_hwcap and reduce any future effort
> > > > required to move beyond 64 caps.
> > > > 
> > > > For convenience we also introduce cpu_{have,set}_named_feature which
> > > > makes use of the cpu_feature macro to allow providing a hwcap name
> > > > without a {KERNEL_}HWCAP_ prefix.
> > > > 
> > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com>
> > > 
> > > [...]
> > > 
> > > > diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
> > > > index 400b80b49595..1f38a2740f7a 100644
> > > > --- a/arch/arm64/include/asm/hwcap.h
> > > > +++ b/arch/arm64/include/asm/hwcap.h
> > > > @@ -39,12 +39,61 @@
> > > >  #define COMPAT_HWCAP2_SHA2	(1 << 3)
> > > >  #define COMPAT_HWCAP2_CRC32	(1 << 4)
> > > >  
> > > > +/*
> > > > + * For userspace we represent hwcaps as a collection of HWCAP{,2}_x bitfields
> > > > + * as described in uapi/asm/hwcap.h. For the kernel we represent hwcaps as
> > > > + * natural numbers (in a single range of size MAX_CPU_FEATURES) defined here
> > > > + * with prefix KERNEL_HWCAP_ mapped to their HWCAP{,2}_x counterpart.
> > > > + *
> > > > + * Hwcaps should be set and tested within the kernel via the
> > > > + * cpu_{set,have}_named_feature(feature) where feature is the unique suffix
> > > > + * of KERNEL_HWCAP_{feature}.
> > > > + */
> > > > +#define __khwcap_feature(x)		ilog2(HWCAP_ ## x)
> > > 
> > > Hmm, I didn't spot this before, but we should probably include
> > > <linux/log2.h>.  This isn't asm-friendly however.
> > 
> > Doh!
> > 
> > > 
> > > <asm/hwcap.h> gets included (unnecessarily?) by arch/arm64/mm/proc.S and
> > > arch/arm64/include/uapi/asm/ptrace.h.
> > 
> > I also can't see any reason why either of these files includes hwcap.h...
> 
> Maybe we could just drop that include from proc.S.
> 
> > > Rather than risk breaking a UAPI header, can we remove the ilog2() here
> > > and add it back into cpu_feature() where it was originally?
> > 
> > No I don't think we can. 
> 
> Agreed: userspace may be relying (however unwisely) on getting the
> hwcaps as a side-effect of <uapi/asm/ptrace.h>, so we can't do much
> about that one without taking a risk.
> 
> > > There may be a reason why this didn't work that I've forgotten...
> > 
> > We need the UAPI HWCAP_xx's to be bitfields and we've decided that we should
> > limit them to 32 bits. Thus UAPI HWCAP2_xx's will also live within the first
> > 32 bits meaning that we can't distinguish between them based on their value.
> > 
> > This isn't ideal within the kernel, as it means if we store the value
> > anywhere (such as struct arm64_cpu_capabilities) then we need to also store
> > some additional information to identify if it's AT_HWCAP or AT_HWCAP2.
> 
> But we could keep shadow kernel #defines that (for hwcap2) are shifted
> up by 32 bits?  This required anything that deals with hwcap numbers to
> cope with them being giant numbers that fit in an unsigned long, not
> just small intergers (which possibly doesn't work without core changes?)
> 
> > In some cases (automatic hwcap based module loading) it's not possible to work
> > around this - which is why arm32 can only support this for their elf_hwcap2.
> > The approach this series takes allows automatic module loading to work based
> > on any hwcap.
> > 
> > The solutions I can come up with at the moment are:
> > 
> >  - hard code the mapping without ilog2, as follows, though this is error
> >    prone
> > 
> > #define KERNEL_HWCAP_ASIMD              2
> > 
> >  - Move the #ifndef __ASSEMBLY__ in include/asm/hwcap.h above the definitions
> >    of KERNEL_HWCAP_xx and include <linux/log2.h> under __ASSEMBLY__. This works
> >    but we can't test for hwcaps in assembly - maybe this isn't a problem?
> 
> Since this is a kernel header, this is probably OK: is asm needs the
> hwcaps, sooner or later someone will need to fix it.
> 
> Possibly there are out-of-tree drivers relying on using the hwcaps from
> assembly, but that's probably their own problem.
> 
> So, either move the #ifndef for simplicity, or introduce the ordinals
> into <uapi/asm/hwcap.h>:
> 
> #define __HWCAP_NR_FP		0
> #define __HWCAP_NR_ASIMD	1
> #define __HWCAP_NR_EVTSTRM	2
> 
> ...
> 
> #define HWCAP_FP	(1UL << __HWCAP_NR_FP)
> #define HWCAP_ASIMD 	(1UL << __HWCAP_NR_ASIMD)
> #define HWCAP_EVTSTRM	(1UL << __HWCAP_NR_EVTSTRM)
> 
> ...
> 
> #define __HWCAP2_NR_DCPODP	0
> 
> #define HWCAP2_DCPODP	(1UL << __HWCAP2_NR_DCPODP)
> 
> ...
> 
> then use the __HWCAP{,2}_NR_ constants directly place of the
> KERNEL_HWCAP_ #defines, or define the KERNEL_HWCAP defined in terms of
> them.

Though we'd still have to add 32 to the HWCAP2's for asm/hwcap.h (thus
justifying the continued use of KERNEL_HWCAP).

> 
> This is a noisy approach though, and I'm not totally convinced it's
> better.
> 
> What do you think?

The only downside to this, is that we create another set of defines per
HWCAP (bringing it up to 3) - these feels excessive.

I'll stick with moving the #ifndef for now.

Thanks,

Andrew Murray

> 
> Cheers
> ---Dave