On Tue, Apr 02, 2019 at 03:58:31PM +0100, Dave Martin wrote:
> 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.)
Yes this is the correct spelling (as I'm pretty sure the cpu_have_feature
helper doesn't have a tips jar).
>
> > 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>
> > ---
> > 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.
I guess not, checkpatch doesn't complain - I'll remove them.
>
> [...]
>
> > 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.)
Yes this makes sense, thanks for the suggestion.
>
> [...]
>
> > 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)
You mean specific to arm32?
I will do this, along with a cpu_get_named_feature - but I think I'd prefer
to do this in a separate series.
>
> 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.)
Yes that's OK.
>
> Other than that, looks reasonable to me.
Thanks,
Andrew Murray
>
> Cheers
> ---Dave
