Re: [PATCH v2 2/6] arm64: HWCAP: add support for AT_HWCAP2

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

On Thu, Feb 21, 2019 at 12:20:53PM +0000, 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
> 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}_feature_name 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    |  2 +-
>  arch/arm64/crypto/ghash-ce-glue.c        |  6 +--
>  arch/arm64/crypto/nhpoly1305-neon-glue.c |  2 +-
>  arch/arm64/crypto/sha256-glue.c          |  4 +-
>  arch/arm64/include/asm/cpufeature.h      | 19 ++++++---
>  arch/arm64/include/asm/hwcap.h           | 40 ++++++++++++++++++-
>  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, 108 insertions(+), 53 deletions(-)

[...]

> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index dfcfba7..dd21a32 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -17,12 +17,12 @@
>  /*
>   * 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.
> + * are supported by the current system. So let's map feature 'x' to
> + * KERNEL_HWCAP_x.

This doesn't read quite right now.

The purpose of this paragraph seems to be that the kernel and user
views have the same encoding, so we can just map x to UAPI HWCAP_x
definition.

This isn't true any more: we now consider the kernel (in elf_hwcap) and
user encodings (in AT_HWCAP) distinct, but for backwards compatibility
reasons HWCAP_x == BIT(KERNEL_HWCAP_x) for the first 32 hwcaps.

So it would be worth rewriting this whole comment to describe the new
situation, rather than just tweaking it.


I'm not sure whether it is more natural to put the comment in
<asm/hwcap.h> or here: you could put it in one place with a brief
pointer in the other.

>   */
>
> -#define MAX_CPU_FEATURES(8 * sizeof(elf_hwcap))
> -#define cpu_feature(x)ilog2(HWCAP_ ## x)
> +#define MAX_CPU_FEATURES64
> +#define cpu_feature(x)(KERNEL_HWCAP_ ## x)
>
>  #ifndef __ASSEMBLY__
>
> @@ -396,10 +396,19 @@ extern struct static_key_false arm64_const_caps_ready;
>
>  bool this_cpu_has_cap(unsigned int cap);
>
> +static inline void cpu_set_feature(unsigned int num)
> +{
> +WARN_ON(num >= MAX_CPU_FEATURES);
> +elf_hwcap |= BIT(num);
> +}
> +#define cpu_set_feature_name(name) cpu_set_feature(cpu_feature(name))
> +
>  static inline bool cpu_have_feature(unsigned int num)
>  {
> -return elf_hwcap & (1UL << num);
> +WARN_ON(num >= MAX_CPU_FEATURES);
> +return elf_hwcap & BIT(num);
>  }
> +#define cpu_have_feature_name(name) cpu_have_feature(cpu_feature(name))

This is bikeshedding, but I'd say that CPUs don't have (feature) names;
they have features (which have names).

So I might prefer to call these something like:

cpu_set_named_feature()
cpu_have_named_feature()

(May not be worth it unless you respin the series for another reason
though.)

>  /* System capability check for constant caps */
>  static inline bool __cpus_have_const_cap(int num)
> diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
> index 400b80b..7549c72 100644
> --- a/arch/arm64/include/asm/hwcap.h
> +++ b/arch/arm64/include/asm/hwcap.h
> @@ -39,12 +39,50 @@
>  #define COMPAT_HWCAP2_SHA2(1 << 3)
>  #define COMPAT_HWCAP2_CRC32(1 << 4)
>
> +/*
> + * KERNEL_HWCAP flags - for elf_hwcap (in kernel)
> + */
> +#define KERNEL_HWCAP_FPilog2(HWCAP_FP)
> +#define KERNEL_HWCAP_ASIMDilog2(HWCAP_ASIMD)

[...]

> +#define KERNEL_HWCAP_SBilog2(HWCAP_SB)
> +#define KERNEL_HWCAP_PACAilog2(HWCAP_PACA)
> +#define KERNEL_HWCAP_PACGilog2(HWCAP_PACG)
> +#define KERNEL_HWCAP_DCPODP(ilog2(HWCAP2_DCPODP) + 32)

For ABI purposes, we should take the opportunity to document the status
of the currently unused bits.

For interoperation with the glibc ifunc resolver ABI, we may want to
reserve a bit among AT_HWCAP [63:32] or AT_HWCAP2 [31:0] that will
never be used by the kernel and always passed to userspace as 0.

I'm envisaging code such as

foo resolver(unsigned long hwcaps, unsigned int num_at_hwcaps,
unsigned long const *at_hwcaps)
{
if ((hwcaps & _GLIBC_EXTRA_HWCAPS) &&
num_at_hwcaps >= 2 &&
at_hwcaps[1] && HWCAP2_FOO)
/* feature present */
}

We would need that _GLIBC_EXTRA_HWCAPS to distinguish the second and
third arguments from uninitialised junk that would be passed by older
glibc versions.

Glibc might or might not choose to try and wegde AT_HWCAP2 in the top
bits of the first argument instead of bits [63:32] of AT_HWCAP (which
we expect to be zero for now, but could still be made reachable via the
at_hwcaps pointer).

Coordination would be needed if glibc carries on using the
<uapi/asm/hwcap.h> HWCAP{,2}_foo defines for here while doing tricks
of this sort.

Szabolcs may have a view on whether this is needed / useful.


If so, we should document any required guarantees now so that we don't
accidentally violate them during future maintenance.  For the benefit
of userspace folks, it may be a good idea to have some clear statement
in Documentation/arm64/ also.

Because of the ABI implications here, if would also be good idea to copy
the libc-alpha mailing list, and possibly also linux-api.

> +
>  #ifndef __ASSEMBLY__
>  /*
>   * This yields a mask that user programs can use to figure out what
>   * instruction set this cpu supports.
>   */
> -#define ELF_HWCAP(elf_hwcap)
> +#define ELF_HWCAPlower_32_bits(elf_hwcap)
> +#define ELF_HWCAP2upper_32_bits(elf_hwcap)

Should we have #include <linux/kernel.h> here somewhere?

[...]

> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 9a7d4dc..4e8d3b4 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -778,7 +778,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_feature_name(EVTSTRM);
> +#else
>  elf_hwcap |= HWCAP_EVTSTRM;
> +#endif

This is a little nasty.

You could give arch/arm its own cpu_set_feature_name() (or whatever it's
called), depending on how keen Russell is to pick it up.

Or for this particular case, stick with the old code (which we now get
away with because elf_hwcap is still exported).  The case of an
HWCAP_foo flag name that happens to have the same semantics on both arm
and arm64 is a pretty esoteric one, so it's not the end of the world if
the standard helpers don't deal with it.

To avoid future accidents you could replace the #ifdef with a comment at
each site (which will also allow people to track down this patch when
looking at that code).

>  #ifdef CONFIG_COMPAT
>  compat_elf_hwcap |= COMPAT_HWCAP_EVTSTRM;
>  #endif
> @@ -1000,7 +1004,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_feature_name(EVTSTRM))
> +#else
>  if (elf_hwcap & HWCAP_EVTSTRM)
> +#endif

Ditto.

[...]

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel