Re: [PATCH v3 2/7] arm64: HWCAP: add support for AT_HWCAP2

[Dave Martin <Dave dot Martin at arm dot com>]

On Mon, Apr 01, 2019 at 11:45:10AM +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 compliments the

Nit: maybe "complements"?  (I've always been a bit fuzzy on the precise
distinction, though.)

> 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>
> ---
>  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           | 49 +++++++++++++++++-
>  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 +++
>  14 files changed, 117 insertions(+), 60 deletions(-)
> 

[...]

> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index e505e1fbd2b9..f06e1da1d678 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -14,15 +14,8 @@
>  #include <asm/hwcap.h>
>  #include <asm/sysreg.h>
>  
> -/*
> - * In the arm64 world (as in the ARM world), elf_hwcap is used both internally
> - * in the kernel and for user space to keep track of which optional features
> - * are supported by the current system. So let's map feature 'x' to HWCAP_x.
> - * Note that HWCAP_x constants are bit fields so we need to take the log.
> - */
> -
> -#define MAX_CPU_FEATURES	(8 * sizeof(elf_hwcap))
> -#define cpu_feature(x)		ilog2(HWCAP_ ## x)
> +#define MAX_CPU_FEATURES	64
> +#define cpu_feature(x)		(KERNEL_HWCAP_ ## x)

Nit: do we need the () here?  They may be defensive, but I'm not sure
they're required.

[...]

> diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
> index 400b80b49595..d21fe3314d90 100644
> --- a/arch/arm64/include/asm/hwcap.h
> +++ b/arch/arm64/include/asm/hwcap.h
> @@ -39,12 +39,59 @@
>  #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 KERNEL_HWCAP_FP			ilog2(HWCAP_FP)
> +#define KERNEL_HWCAP_ASIMD		ilog2(HWCAP_ASIMD)
> +#define KERNEL_HWCAP_EVTSTRM		ilog2(HWCAP_EVTSTRM)
> +#define KERNEL_HWCAP_AES		ilog2(HWCAP_AES)
> +#define KERNEL_HWCAP_PMULL		ilog2(HWCAP_PMULL)
> +#define KERNEL_HWCAP_SHA1		ilog2(HWCAP_SHA1)
> +#define KERNEL_HWCAP_SHA2		ilog2(HWCAP_SHA2)
> +#define KERNEL_HWCAP_CRC32		ilog2(HWCAP_CRC32)
> +#define KERNEL_HWCAP_ATOMICS		ilog2(HWCAP_ATOMICS)
> +#define KERNEL_HWCAP_FPHP		ilog2(HWCAP_FPHP)
> +#define KERNEL_HWCAP_ASIMDHP		ilog2(HWCAP_ASIMDHP)
> +#define KERNEL_HWCAP_CPUID		ilog2(HWCAP_CPUID)
> +#define KERNEL_HWCAP_ASIMDRDM		ilog2(HWCAP_ASIMDRDM)
> +#define KERNEL_HWCAP_JSCVT		ilog2(HWCAP_JSCVT)
> +#define KERNEL_HWCAP_FCMA		ilog2(HWCAP_FCMA)
> +#define KERNEL_HWCAP_LRCPC		ilog2(HWCAP_LRCPC)
> +#define KERNEL_HWCAP_DCPOP		ilog2(HWCAP_DCPOP)
> +#define KERNEL_HWCAP_SHA3		ilog2(HWCAP_SHA3)
> +#define KERNEL_HWCAP_SM3		ilog2(HWCAP_SM3)
> +#define KERNEL_HWCAP_SM4		ilog2(HWCAP_SM4)
> +#define KERNEL_HWCAP_ASIMDDP		ilog2(HWCAP_ASIMDDP)
> +#define KERNEL_HWCAP_SHA512		ilog2(HWCAP_SHA512)
> +#define KERNEL_HWCAP_SVE		ilog2(HWCAP_SVE)
> +#define KERNEL_HWCAP_ASIMDFHM		ilog2(HWCAP_ASIMDFHM)
> +#define KERNEL_HWCAP_DIT		ilog2(HWCAP_DIT)
> +#define KERNEL_HWCAP_USCAT		ilog2(HWCAP_USCAT)
> +#define KERNEL_HWCAP_ILRCPC		ilog2(HWCAP_ILRCPC)
> +#define KERNEL_HWCAP_FLAGM		ilog2(HWCAP_FLAGM)
> +#define KERNEL_HWCAP_SSBS		ilog2(HWCAP_SSBS)
> +#define KERNEL_HWCAP_SB			ilog2(HWCAP_SB)
> +#define KERNEL_HWCAP_PACA		ilog2(HWCAP_PACA)
> +#define KERNEL_HWCAP_PACG		ilog2(HWCAP_PACG)
> +#define KERNEL_HWCAP_DCPODP		(ilog2(HWCAP2_DCPODP) + 32)

Nit: can we wrap this so that the "+ 32" doesn't have to be spelled out
each time?

If we are splitting ths CVADP support from this patch, then dropping
such a wrapper macro here (maybe with a comment) will serve as a
placeholder for whichever patch wins the race for the first HWCAP2
flag.

Say

#define __khwcap2_feature(x) (ilog2(HWCAP2_ ## xx) + 32)

(Optionally, we could also have __khwcap_feature() too so that
everything looks nice and regular.)

[...]

> 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

I wonder whether we can have a generic definition for this:

#define cpu_set_named_feature(x) (elf_hwcap |= HWCAP_ ## x)

seems a reasonable fallback when the arch doesn't provide its own
version.


Although we don't have many instances, it would still be nice to avoid
ifdeffery creeping in.

[...]

We can probably pull the Documentation/arm64/elf_hwcaps.txt changes into
this patch.

It probably makes sense to pull the Documentation/arm64/elf_hwcaps.txt
updates alongside this patch in the series (or even incorporate them
into this patch, since they're not huge.)

Other than that, looks reasonable to me.

Cheers
---Dave