| Message ID | 1549459868-45992-4-git-send-email-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initial support for CVADP | expand |
On Wed, Feb 06, 2019 at 01:31:05PM +0000, Andrew Murray wrote: > The introduction of elf_hwcap2 introduced accessors which ensure that > features are set/tested in the appropriate elf_hwcapX variable. > > Let's now mandate access to elf_hwcapX via these accessors by making > elf_hwcapX static within cpufeature.c. > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > --- > arch/arm64/include/asm/cpufeature.h | 19 ++++--------------- > arch/arm64/include/asm/hwcap.h | 6 +++--- > arch/arm64/kernel/cpufeature.c | 33 ++++++++++++++++++++++++++++----- > 3 files changed, 35 insertions(+), 23 deletions(-) [...] > diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h > index 05ee9b9..ced87ad 100644 > --- a/arch/arm64/include/asm/hwcap.h > +++ b/arch/arm64/include/asm/hwcap.h [...] > @@ -97,6 +98,5 @@ enum { > #endif > }; > > -extern unsigned long elf_hwcap, elf_hwcap2; > #endif > #endif > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index d10a455..5b9620d 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -35,11 +35,8 @@ > #include <asm/traps.h> > #include <asm/virt.h> > > -unsigned long elf_hwcap __read_mostly; > -EXPORT_SYMBOL_GPL(elf_hwcap); > - Will this affect out-of-tree modules? I also note an out-of-arch/ use in drivers/clocksource/arm_arch_timer.c, which this series doesn't appear to address (or I missed it). We are allowed to break EXPORT_SYMBOL_GPL()s in general so long as it's not done without any meaningful reason, so maybe this is not a huge concern so long as we catch all the in-tree users. [...] Cheers ---Dave
On Thu, Feb 07, 2019 at 11:47:59AM +0000, Dave Martin wrote: > On Wed, Feb 06, 2019 at 01:31:05PM +0000, Andrew Murray wrote: > > The introduction of elf_hwcap2 introduced accessors which ensure that > > features are set/tested in the appropriate elf_hwcapX variable. > > > > Let's now mandate access to elf_hwcapX via these accessors by making > > elf_hwcapX static within cpufeature.c. > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > --- > > arch/arm64/include/asm/cpufeature.h | 19 ++++--------------- > > arch/arm64/include/asm/hwcap.h | 6 +++--- > > arch/arm64/kernel/cpufeature.c | 33 ++++++++++++++++++++++++++++----- > > 3 files changed, 35 insertions(+), 23 deletions(-) > > [...] > > > diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h > > index 05ee9b9..ced87ad 100644 > > --- a/arch/arm64/include/asm/hwcap.h > > +++ b/arch/arm64/include/asm/hwcap.h > > [...] > > > @@ -97,6 +98,5 @@ enum { > > #endif > > }; > > > > -extern unsigned long elf_hwcap, elf_hwcap2; > > #endif > > #endif > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index d10a455..5b9620d 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -35,11 +35,8 @@ > > #include <asm/traps.h> > > #include <asm/virt.h> > > > > -unsigned long elf_hwcap __read_mostly; > > -EXPORT_SYMBOL_GPL(elf_hwcap); > > - > > Will this affect out-of-tree modules? > > I also note an out-of-arch/ use in > drivers/clocksource/arm_arch_timer.c, which this series doesn't appear > to address (or I missed it). It was in "arm64: HWCAP: add support for ELF_HWCAP2" - though due to it being used by arm32 code I had to use #ifdef CONFIG_ARM64 as cpu_set_feature doesn't exist there. > > We are allowed to break EXPORT_SYMBOL_GPL()s in general so long as it's > not done without any meaningful reason, so maybe this is not a huge > concern so long as we catch all the in-tree users. The benefit of removing the EXPORT_SYMBOL_GPL is that we can encapsulate the elf_hwcap variables with getters/setters that ensure they are modified correctly. This is only relevant now as we split bits between elf_hwcap and elf_hwcap2. My suggestion is to remove the EXPORT_SYMBOL_GPL and then also stick with using just one elf_hwcap variable. As you previously suggested we can then update the ELF_HWCAP{,2} marcos to produce the relevant bit fields. This also has the added benefit of simplifying cpu_{have,set}_feature functions as the number of hwcaps grow. Thanks, Andrew Murray > > [...] > > Cheers > ---Dave
On Mon, Feb 18, 2019 at 03:46:02PM +0000, Andrew Murray wrote: > On Thu, Feb 07, 2019 at 11:47:59AM +0000, Dave Martin wrote: > > On Wed, Feb 06, 2019 at 01:31:05PM +0000, Andrew Murray wrote: > > > The introduction of elf_hwcap2 introduced accessors which ensure that > > > features are set/tested in the appropriate elf_hwcapX variable. > > > > > > Let's now mandate access to elf_hwcapX via these accessors by making > > > elf_hwcapX static within cpufeature.c. > > > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > > --- > > > arch/arm64/include/asm/cpufeature.h | 19 ++++--------------- > > > arch/arm64/include/asm/hwcap.h | 6 +++--- > > > arch/arm64/kernel/cpufeature.c | 33 ++++++++++++++++++++++++++++----- > > > 3 files changed, 35 insertions(+), 23 deletions(-) > > > > [...] > > > > > diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h > > > index 05ee9b9..ced87ad 100644 > > > --- a/arch/arm64/include/asm/hwcap.h > > > +++ b/arch/arm64/include/asm/hwcap.h > > > > [...] > > > > > @@ -97,6 +98,5 @@ enum { > > > #endif > > > }; > > > > > > -extern unsigned long elf_hwcap, elf_hwcap2; > > > #endif > > > #endif > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > > index d10a455..5b9620d 100644 > > > --- a/arch/arm64/kernel/cpufeature.c > > > +++ b/arch/arm64/kernel/cpufeature.c > > > @@ -35,11 +35,8 @@ > > > #include <asm/traps.h> > > > #include <asm/virt.h> > > > > > > -unsigned long elf_hwcap __read_mostly; > > > -EXPORT_SYMBOL_GPL(elf_hwcap); > > > - > > > > Will this affect out-of-tree modules? > > > > I also note an out-of-arch/ use in > > drivers/clocksource/arm_arch_timer.c, which this series doesn't appear > > to address (or I missed it). > > It was in "arm64: HWCAP: add support for ELF_HWCAP2" - though due to it > being used by arm32 code I had to use #ifdef CONFIG_ARM64 as > cpu_set_feature doesn't exist there. Ah, I guess I missed that. > > > > We are allowed to break EXPORT_SYMBOL_GPL()s in general so long as it's > > not done without any meaningful reason, so maybe this is not a huge > > concern so long as we catch all the in-tree users. > > The benefit of removing the EXPORT_SYMBOL_GPL is that we can encapsulate > the elf_hwcap variables with getters/setters that ensure they are modified > correctly. This is only relevant now as we split bits between elf_hwcap and > elf_hwcap2. > > My suggestion is to remove the EXPORT_SYMBOL_GPL and then also stick with > using just one elf_hwcap variable. As you previously suggested we can then > update the ELF_HWCAP{,2} marcos to produce the relevant bit fields. This > also has the added benefit of simplifying cpu_{have,set}_feature functions > as the number of hwcaps grow. I guess this depends on what is decided re the other patches. While it would be nice to avoid exposing elf_hwcap and elf_hwcap2 directly, I'm starting to feel it may be easier to expose them as-is. Cheers ---Dave
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 15d939e..0e6132f 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -395,21 +395,10 @@ 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) -{ - if (num < 32) - elf_hwcap |= BIT(num); - else - elf_hwcap2 |= BIT(num - 32); -} - -static inline bool cpu_have_feature(unsigned int num) -{ - if (num < 32) - return elf_hwcap & BIT(num); - else - return elf_hwcap2 & BIT(num - 32); -} +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); /* 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 05ee9b9..ced87ad 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) @@ -80,8 +81,8 @@ * 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_HWCAP2 elf_hwcap2 +#define ELF_HWCAP cpu_get_elf_hwcap() +#define ELF_HWCAP2 cpu_get_elf_hwcap2() #ifdef CONFIG_COMPAT #define COMPAT_ELF_HWCAP (compat_elf_hwcap) @@ -97,6 +98,5 @@ enum { #endif }; -extern unsigned long elf_hwcap, elf_hwcap2; #endif #endif diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index d10a455..5b9620d 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -35,11 +35,8 @@ #include <asm/traps.h> #include <asm/virt.h> -unsigned long elf_hwcap __read_mostly; -EXPORT_SYMBOL_GPL(elf_hwcap); - -unsigned long elf_hwcap2 __read_mostly; -EXPORT_SYMBOL_GPL(elf_hwcap2); +static unsigned long elf_hwcap __read_mostly; +static unsigned long elf_hwcap2 __read_mostly; #ifdef CONFIG_COMPAT #define COMPAT_ELF_HWCAP_DEFAULT \ @@ -1912,6 +1909,32 @@ bool this_cpu_has_cap(unsigned int n) return false; } +void cpu_set_feature(unsigned int num) +{ + if (num < 32) + elf_hwcap |= BIT(num); + else + elf_hwcap2 |= BIT(num - 32); +} + +bool cpu_have_feature(unsigned int num) +{ + if (num < 32) + return elf_hwcap & BIT(num); + else + return elf_hwcap2 & BIT(num - 32); +} + +unsigned long cpu_get_elf_hwcap(void) +{ + return elf_hwcap; +} + +unsigned long cpu_get_elf_hwcap2(void) +{ + return elf_hwcap2; +} + static void __init setup_system_capabilities(void) { /*
The introduction of elf_hwcap2 introduced accessors which ensure that features are set/tested in the appropriate elf_hwcapX variable. Let's now mandate access to elf_hwcapX via these accessors by making elf_hwcapX static within cpufeature.c. Signed-off-by: Andrew Murray <andrew.murray@arm.com> --- arch/arm64/include/asm/cpufeature.h | 19 ++++--------------- arch/arm64/include/asm/hwcap.h | 6 +++--- arch/arm64/kernel/cpufeature.c | 33 ++++++++++++++++++++++++++++----- 3 files changed, 35 insertions(+), 23 deletions(-)