| Message ID | 1550751657-30252-4-git-send-email-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for CVADP | expand |
On Thu, Feb 21, 2019 at 12:20:54PM +0000, Andrew Murray wrote: > The introduction of AT_HWCAP2 introduced accessors which ensure that > hwcap features are set and tested appropriately. > > Let's now mandate access to elf_hwcap via these accessors by making > elf_hwcap static within cpufeature.c. Since elf_hwcap now survives and retains a compatible encoding for HWCAP_foo, I'm wondering whether it would be simpler to drop this patch. Although this will help push people to use the new helpers, the need to do that seems reduced now. People falling off the end of the hwcaps will discover that there is no HWCAP_foo for the feature they want, only HWCAP2_foo (but no elf_hwcap2 to look for it in), or KERNEL_HWCAP_foo (which should get them thinking). What do you think? [...] > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 6a477a3..d57a179 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -35,8 +35,7 @@ > #include <asm/traps.h> > #include <asm/virt.h> > > -unsigned long elf_hwcap __read_mostly; > -EXPORT_SYMBOL_GPL(elf_hwcap); > +static unsigned long elf_hwcap __read_mostly; > > #ifdef CONFIG_COMPAT > #define COMPAT_ELF_HWCAP_DEFAULT\ > @@ -1909,6 +1908,30 @@ bool this_cpu_has_cap(unsigned int n) > return false; > } > > +void cpu_set_feature(unsigned int num) > +{ > +WARN_ON(num >= MAX_CPU_FEATURES); > +elf_hwcap |= BIT(num); > +} > +EXPORT_SYMBOL_GPL(cpu_set_feature); > + > +bool cpu_have_feature(unsigned int num) > +{ > +WARN_ON(num >= MAX_CPU_FEATURES); > +return elf_hwcap & BIT(num); > +} > +EXPORT_SYMBOL_GPL(cpu_have_feature); > + > +unsigned long cpu_get_elf_hwcap(void) > +{ > +return lower_32_bits(elf_hwcap); > +} > + > +unsigned long cpu_get_elf_hwcap2(void) > +{ > +return upper_32_bits(elf_hwcap); > +} > + Similarly, pushing all this out of line to enable elf_hwcap to be hidden may be more effort than it is really worth (?) 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.
On Thu, Feb 21, 2019 at 06:45:08PM +0000, Dave P Martin wrote: > On Thu, Feb 21, 2019 at 12:20:54PM +0000, Andrew Murray wrote: > > The introduction of AT_HWCAP2 introduced accessors which ensure that > > hwcap features are set and tested appropriately. > > > > Let's now mandate access to elf_hwcap via these accessors by making > > elf_hwcap static within cpufeature.c. > > Since elf_hwcap now survives and retains a compatible encoding for > HWCAP_foo, I'm wondering whether it would be simpler to drop this patch. > > Although this will help push people to use the new helpers, the need to > do that seems reduced now. > > People falling off the end of the hwcaps will discover that there is no > HWCAP_foo for the feature they want, only HWCAP2_foo (but no elf_hwcap2 > to look for it in), or KERNEL_HWCAP_foo (which should get them > thinking). > > What do you think? You're right that people will find the appropiate functions to set the relevant hwcaps. But I think my motivation for this comes from the perspective of code maintainability... There is absolutely no functional benefit to exposing the elf_hwcap variable to the rest of the kernel - yet doing so will result in users using elf_hwcap instead of the helpers. If we later change the type of elf_hwcap, or split the bits into multiple variables (e.g. elf_hwcap2) or even change the mapping from UAPI to kernel, then modifications need to be made at each call site. Whereas if we reduce the visibility of elf_hwcap then we encapsulate all the bit fiddling in one place. Also, taking this approach forces us into this ugly suitation... +#ifdef CONFIG_ARM64 + if (cpu_have_feature_name(EVTSTRM)) +#else if (elf_hwcap & HWCAP_EVTSTRM) +#endif It's not nice, but it's a lot less fragile than expecting cross-platform agreement on the bit value of particular hwcaps. Is this a good enough reason to keep it? Thanks, Andrew Murray > > [...] > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 6a477a3..d57a179 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -35,8 +35,7 @@ > > #include <asm/traps.h> > > #include <asm/virt.h> > > > > -unsigned long elf_hwcap __read_mostly; > > -EXPORT_SYMBOL_GPL(elf_hwcap); > > +static unsigned long elf_hwcap __read_mostly; > > > > #ifdef CONFIG_COMPAT > > #define COMPAT_ELF_HWCAP_DEFAULT \ > > @@ -1909,6 +1908,30 @@ bool this_cpu_has_cap(unsigned int n) > > return false; > > } > > > > +void cpu_set_feature(unsigned int num) > > +{ > > + WARN_ON(num >= MAX_CPU_FEATURES); > > + elf_hwcap |= BIT(num); > > +} > > +EXPORT_SYMBOL_GPL(cpu_set_feature); > > + > > +bool cpu_have_feature(unsigned int num) > > +{ > > + WARN_ON(num >= MAX_CPU_FEATURES); > > + return elf_hwcap & BIT(num); > > +} > > +EXPORT_SYMBOL_GPL(cpu_have_feature); > > + > > +unsigned long cpu_get_elf_hwcap(void) > > +{ > > + return lower_32_bits(elf_hwcap); > > +} > > + > > +unsigned long cpu_get_elf_hwcap2(void) > > +{ > > + return upper_32_bits(elf_hwcap); > > +} > > + > > Similarly, pushing all this out of line to enable elf_hwcap to be hidden > may be more effort than it is really worth (?) > > Cheers > ---Dave
On Wed, Mar 27, 2019 at 02:03:11PM +0000, Andrew Murray wrote: > On Thu, Feb 21, 2019 at 06:45:08PM +0000, Dave P Martin wrote: > > On Thu, Feb 21, 2019 at 12:20:54PM +0000, Andrew Murray wrote: > > > The introduction of AT_HWCAP2 introduced accessors which ensure that > > > hwcap features are set and tested appropriately. > > > > > > Let's now mandate access to elf_hwcap via these accessors by making > > > elf_hwcap static within cpufeature.c. > > > > Since elf_hwcap now survives and retains a compatible encoding for > > HWCAP_foo, I'm wondering whether it would be simpler to drop this patch. > > > > Although this will help push people to use the new helpers, the need to > > do that seems reduced now. > > > > People falling off the end of the hwcaps will discover that there is no > > HWCAP_foo for the feature they want, only HWCAP2_foo (but no elf_hwcap2 > > to look for it in), or KERNEL_HWCAP_foo (which should get them > > thinking). > > > > What do you think? > > You're right that people will find the appropiate functions to set the > relevant hwcaps. But I think my motivation for this comes from the > perspective of code maintainability... > > There is absolutely no functional benefit to exposing the elf_hwcap > variable to the rest of the kernel - yet doing so will result in users > using elf_hwcap instead of the helpers. If we later change the type of > elf_hwcap, or split the bits into multiple variables (e.g. elf_hwcap2) or > even change the mapping from UAPI to kernel, then modifications need to be > made at each call site. Whereas if we reduce the visibility of elf_hwcap > then we encapsulate all the bit fiddling in one place. > > Also, taking this approach forces us into this ugly suitation... > > +#ifdef CONFIG_ARM64 > + if (cpu_have_feature_name(EVTSTRM)) > +#else > if (elf_hwcap & HWCAP_EVTSTRM) > +#endif > > It's not nice, but it's a lot less fragile than expecting cross-platform > agreement on the bit value of particular hwcaps. > > Is this a good enough reason to keep it? Yes, I'm happy for us to go forward with your approach here. Although it makes a bit more diff noise now, I agree it's the cleaner option for the future. Cheers ---Dave
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index dd21a32..a5acce4 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -395,19 +395,12 @@ extern struct static_key_false arm64_const_caps_ready; for_each_set_bit(cap, cpu_hwcaps, ARM64_NCAPS) bool this_cpu_has_cap(unsigned int cap); +void cpu_set_feature(unsigned int num); +bool cpu_have_feature(unsigned int num); +unsigned long cpu_get_elf_hwcap(void); +unsigned long cpu_get_elf_hwcap2(void); -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) -{ - WARN_ON(num >= MAX_CPU_FEATURES); - return elf_hwcap & BIT(num); -} #define cpu_have_feature_name(name) cpu_have_feature(cpu_feature(name)) /* System capability check for constant caps */ diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h index 7549c72..170fb94 100644 --- a/arch/arm64/include/asm/hwcap.h +++ b/arch/arm64/include/asm/hwcap.h @@ -17,6 +17,7 @@ #define __ASM_HWCAP_H #include <uapi/asm/hwcap.h> +#include <asm/cpufeature.h> #define COMPAT_HWCAP_HALF (1 << 1) #define COMPAT_HWCAP_THUMB (1 << 2) @@ -81,8 +82,8 @@ * This yields a mask that user programs can use to figure out what * instruction set this cpu supports. */ -#define ELF_HWCAP lower_32_bits(elf_hwcap) -#define ELF_HWCAP2 upper_32_bits(elf_hwcap) +#define ELF_HWCAP cpu_get_elf_hwcap() +#define ELF_HWCAP2 cpu_get_elf_hwcap2() #ifdef CONFIG_COMPAT #define COMPAT_ELF_HWCAP (compat_elf_hwcap) @@ -98,6 +99,5 @@ enum { #endif }; -extern unsigned long elf_hwcap; #endif #endif diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 6a477a3..d57a179 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -35,8 +35,7 @@ #include <asm/traps.h> #include <asm/virt.h> -unsigned long elf_hwcap __read_mostly; -EXPORT_SYMBOL_GPL(elf_hwcap); +static unsigned long elf_hwcap __read_mostly; #ifdef CONFIG_COMPAT #define COMPAT_ELF_HWCAP_DEFAULT \ @@ -1909,6 +1908,30 @@ bool this_cpu_has_cap(unsigned int n) return false; } +void cpu_set_feature(unsigned int num) +{ + WARN_ON(num >= MAX_CPU_FEATURES); + elf_hwcap |= BIT(num); +} +EXPORT_SYMBOL_GPL(cpu_set_feature); + +bool cpu_have_feature(unsigned int num) +{ + WARN_ON(num >= MAX_CPU_FEATURES); + return elf_hwcap & BIT(num); +} +EXPORT_SYMBOL_GPL(cpu_have_feature); + +unsigned long cpu_get_elf_hwcap(void) +{ + return lower_32_bits(elf_hwcap); +} + +unsigned long cpu_get_elf_hwcap2(void) +{ + return upper_32_bits(elf_hwcap); +} + static void __init setup_system_capabilities(void) { /*
The introduction of AT_HWCAP2 introduced accessors which ensure that hwcap features are set and tested appropriately. Let's now mandate access to elf_hwcap via these accessors by making elf_hwcap static within cpufeature.c. Signed-off-by: Andrew Murray <andrew.murray@arm.com> --- arch/arm64/include/asm/cpufeature.h | 15 ++++----------- arch/arm64/include/asm/hwcap.h | 6 +++--- arch/arm64/kernel/cpufeature.c | 27 +++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 16 deletions(-)