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

[Will Deacon <will dot deacon at arm dot com>]

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?

> 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.

Will