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

[Dave Martin <Dave.Martin@arm.com>]

On Tue, Apr 16, 2019 at 02:51:57PM +0100, Will Deacon wrote:
> On Tue, Apr 09, 2019 at 10:52:40AM +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>
> > ---
> >  Documentation/arm64/elf_hwcaps.txt       | 14 +++--
> >  arch/arm64/crypto/aes-ce-ccm-glue.c      |  2 +-
> >  arch/arm64/crypto/aes-neonbs-glue.c      |  2 +-
> >  arch/arm64/crypto/chacha-neon-glue.c     |  2 +-
> >  arch/arm64/crypto/crct10dif-ce-glue.c    |  4 +-
> >  arch/arm64/crypto/ghash-ce-glue.c        |  8 +--
> >  arch/arm64/crypto/nhpoly1305-neon-glue.c |  2 +-
> >  arch/arm64/crypto/sha256-glue.c          |  4 +-
> >  arch/arm64/include/asm/cpufeature.h      | 22 ++++----
> >  arch/arm64/include/asm/hwcap.h           | 52 ++++++++++++++++++-
> >  arch/arm64/include/uapi/asm/hwcap.h      |  2 +-
> >  arch/arm64/kernel/cpufeature.c           | 66 ++++++++++++------------
> >  arch/arm64/kernel/cpuinfo.c              |  2 +-
> >  arch/arm64/kernel/fpsimd.c               |  4 +-
> >  drivers/clocksource/arm_arch_timer.c     |  8 +++
> >  15 files changed, 131 insertions(+), 63 deletions(-)
> > 
> > diff --git a/Documentation/arm64/elf_hwcaps.txt b/Documentation/arm64/elf_hwcaps.txt
> > index 13d6691b37be..c04f8e87bab8 100644
> > --- a/Documentation/arm64/elf_hwcaps.txt
> > +++ b/Documentation/arm64/elf_hwcaps.txt
> > @@ -13,9 +13,9 @@ architected discovery mechanism available to userspace code at EL0. The
> >  kernel exposes the presence of these features to userspace through a set
> >  of flags called hwcaps, exposed in the auxilliary vector.
> >  
> > -Userspace software can test for features by acquiring the AT_HWCAP entry
> > -of the auxilliary vector, and testing whether the relevant flags are
> > -set, e.g.
> > +Userspace software can test for features by acquiring the AT_HWCAP or
> > +AT_HWCAP2 entry of the auxiliary vector, and testing whether the relevant
> > +flags are set, e.g.
> >  
> >  bool floating_point_is_present(void)
> >  {
> > @@ -194,3 +194,11 @@ HWCAP_PACG
> >      Functionality implied by ID_AA64ISAR1_EL1.GPA == 0b0001 or
> >      ID_AA64ISAR1_EL1.GPI == 0b0001, as described by
> >      Documentation/arm64/pointer-authentication.txt.
> > +
> > +
> > +4. Unused AT_HWCAP bits
> > +-----------------------
> > +
> > +Each AT_HWCAP and AT_HWCAP2 entry provides for up to 32 hwcaps contained
> > +in bits [31:0]. For interoperation with userspace we guarantee that bits
> > +62 and 63 of AT_HWCAP will always be returned as 0.
> 
> I'm a little nervous about the first sentence here, since it could be
> taken to mean that we will never allocate 61:32. Mind if I drop it?

Ack: I don't think we want to say explicitly that we will never use
those bits, apart from AT_HWCAP[63:62] for which there are specific
reasons.

(For now of course, we won't use them.)

> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index aa4ec53281ce..6cc8aff83805 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -833,7 +833,11 @@ static void arch_timer_evtstrm_enable(int divider)
> >  	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
> >  			| ARCH_TIMER_VIRT_EVT_EN;
> >  	arch_timer_set_cntkctl(cntkctl);
> > +#ifdef CONFIG_ARM64
> > +	cpu_set_named_feature(EVTSTRM);
> > +#else
> >  	elf_hwcap |= HWCAP_EVTSTRM;
> > +#endif
> >  #ifdef CONFIG_COMPAT
> >  	compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
> >  #endif
> > @@ -1055,7 +1059,11 @@ static int arch_timer_cpu_pm_notify(struct notifier_block *self,
> >  	} else if (action == CPU_PM_ENTER_FAILED || action == CPU_PM_EXIT) {
> >  		arch_timer_set_cntkctl(__this_cpu_read(saved_cntkctl));
> >  
> > +#ifdef CONFIG_ARM64
> > +		if (cpu_have_named_feature(EVTSTRM))
> > +#else
> >  		if (elf_hwcap & HWCAP_EVTSTRM)
> > +#endif
> 
> I think this is an indication that the abstraction isn't quite right and
> should probably be done in an arch-helped via asm/arch_timer.h. However,
> that can be done as a separate patch later on.

It probably does make sense to add an arch-specific helper for that.

Given that we don't want to encourage this kind of poking about in
elf_hwcap.  It might make sense to have a single-purpose helper just for
checking this flag.

Cheers
---Dave