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@xxxxxxx>
> >
> > [...]
> >
> > > 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.
This is a noisy approach though, and I'm not totally convinced it's
better.
What do you think?
Cheers
---Dave
