| Message ID | 20190401104515.39775-4-andrew.murray@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Initial support for CVADP | expand |
On Mon, Apr 01, 2019 at 11:45:11AM +0100, 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. Looks reasonable except for a couple of minor nits below. I had wondered whether putting these accessors out of line would affect any hot paths, but I can't see these used from anything that looks like a hot path. So we're probably fine. cpus_have_const_cap() is preferred for places where this matters, anyway. [...] > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 986ceeacd19f..84ca52fa75e5 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; Now that this doesn't correspond directly to ELF_HWCAP any more and we hide it, can we rename it to avoid confusion? Maybe "kernel_hwcap"? > #ifdef CONFIG_COMPAT > #define COMPAT_ELF_HWCAP_DEFAULT \ > @@ -1947,6 +1946,35 @@ 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) > +{ > + /* > + * We currently only populate the first 32 bits of AT_HWCAP. Please > + * note that for userspace compatibility we guarantee that bit 62 > + * will always be returned as 0. > + */ Presumably also bit 63? It is reasonable to say this here, but I think there should also be a note in Documentation/arm64/elf_hwcaps.txt. [...] Cheers ---Dave
On Tue, Apr 02, 2019 at 03:58:21PM +0100, Dave Martin wrote: > On Mon, Apr 01, 2019 at 11:45:11AM +0100, 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. > > Looks reasonable except for a couple of minor nits below. > > I had wondered whether putting these accessors out of line would affect > any hot paths, but I can't see these used from anything that looks like > a hot path. So we're probably fine. > > cpus_have_const_cap() is preferred for places where this matters, > anyway. > > [...] > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index 986ceeacd19f..84ca52fa75e5 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; > > Now that this doesn't correspond directly to ELF_HWCAP any more and we > hide it, can we rename it to avoid confusion? > > Maybe "kernel_hwcap"? Yes this seems reasonable. > > > #ifdef CONFIG_COMPAT > > #define COMPAT_ELF_HWCAP_DEFAULT \ > > @@ -1947,6 +1946,35 @@ 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) > > +{ > > + /* > > + * We currently only populate the first 32 bits of AT_HWCAP. Please > > + * note that for userspace compatibility we guarantee that bit 62 > > + * will always be returned as 0. > > + */ > > Presumably also bit 63? Yes, I will add this too. > > It is reasonable to say this here, but I think there should also be a > note in Documentation/arm64/elf_hwcaps.txt. This is already present in this series, I'll update it to reflect bit 63 also. Thanks, Andrew Murray > > [...] > > Cheers > ---Dave
Hi, On 02/04/2019 16:06, Andrew Murray wrote: > On Tue, Apr 02, 2019 at 03:58:21PM +0100, Dave Martin wrote: >> On Mon, Apr 01, 2019 at 11:45:11AM +0100, 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. >> >> Looks reasonable except for a couple of minor nits below. >> >> I had wondered whether putting these accessors out of line would affect >> any hot paths, but I can't see these used from anything that looks like >> a hot path. So we're probably fine. >> >> cpus_have_const_cap() is preferred for places where this matters, >> anyway. Btw, thats for cpu_hwcaps, which is completely different from elf_hwcaps. >> >> [...] >> >>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >>> index 986ceeacd19f..84ca52fa75e5 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; >> >> Now that this doesn't correspond directly to ELF_HWCAP any more and we >> hide it, can we rename it to avoid confusion? >> >> Maybe "kernel_hwcap"? > > Yes this seems reasonable. nit: As mentioned above we have "cpu_hwcaps" for the features only internally by the kernel. Naming it "kernel_hwcap" kind of looses the hint that the major purpose is for userspace consumption and could easily confuse with the poorly named "cpu_hwcaps" which should have been called kernel_hwcaps. How about "user_hwcaps" ? Or preferrably something closer to that. Cheers Suzuki
On Tue, Apr 02, 2019 at 04:32:57PM +0100, Suzuki K Poulose wrote: > Hi, > > On 02/04/2019 16:06, Andrew Murray wrote: > >On Tue, Apr 02, 2019 at 03:58:21PM +0100, Dave Martin wrote: > >>On Mon, Apr 01, 2019 at 11:45:11AM +0100, 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. > >> > >>Looks reasonable except for a couple of minor nits below. > >> > >>I had wondered whether putting these accessors out of line would affect > >>any hot paths, but I can't see these used from anything that looks like > >>a hot path. So we're probably fine. > >> > >>cpus_have_const_cap() is preferred for places where this matters, > >>anyway. > > Btw, thats for cpu_hwcaps, which is completely different from elf_hwcaps. Sure, I meant: where we need to test something fast, we can have a cpucap for it, rather than rely on the ELF hwcaps. In practice, we already follow this pattern today: ELF hwcap flags are tested in a few places, but I don't see anything that is fast-path. > > >> > >>[...] > >> > >>>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > >>>index 986ceeacd19f..84ca52fa75e5 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; > >> > >>Now that this doesn't correspond directly to ELF_HWCAP any more and we > >>hide it, can we rename it to avoid confusion? > >> > >>Maybe "kernel_hwcap"? > > > >Yes this seems reasonable. > > nit: > > As mentioned above we have "cpu_hwcaps" for the features only internally > by the kernel. Naming it "kernel_hwcap" kind of looses the hint that the > major purpose is for userspace consumption and could easily confuse with > the poorly named "cpu_hwcaps" which should have been called kernel_hwcaps. > > How about "user_hwcaps" ? Or preferrably something closer to that. Yes, that may be better. Of course, we also have this naming in all the KERNEL_HWCAP #defined now. Since kernel_hwcap is just a static variable now, maybe it's sufficient to stick a comment next to it explaining what it is (and what it isn't). "user_hwcaps" still implies that this might be the userspace view of the flags, which it isn't. But I don't feel strongly about this. If someone wants to make a decision, I'm happy to defer to it. Cheers ---Dave
On Tue, Apr 02, 2019 at 04:55:58PM +0100, Dave Martin wrote: > On Tue, Apr 02, 2019 at 04:32:57PM +0100, Suzuki K Poulose wrote: > > Hi, > > > > On 02/04/2019 16:06, Andrew Murray wrote: > > >On Tue, Apr 02, 2019 at 03:58:21PM +0100, Dave Martin wrote: > > >>On Mon, Apr 01, 2019 at 11:45:11AM +0100, 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. > > >> > > >>Looks reasonable except for a couple of minor nits below. > > >> > > >>I had wondered whether putting these accessors out of line would affect > > >>any hot paths, but I can't see these used from anything that looks like > > >>a hot path. So we're probably fine. > > >> > > >>cpus_have_const_cap() is preferred for places where this matters, > > >>anyway. > > > > Btw, thats for cpu_hwcaps, which is completely different from elf_hwcaps. > > Sure, I meant: where we need to test something fast, we can have a > cpucap for it, rather than rely on the ELF hwcaps. > > In practice, we already follow this pattern today: ELF hwcap flags are > tested in a few places, but I don't see anything that is fast-path. > > > > > >> > > >>[...] > > >> > > >>>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > >>>index 986ceeacd19f..84ca52fa75e5 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; > > >> > > >>Now that this doesn't correspond directly to ELF_HWCAP any more and we > > >>hide it, can we rename it to avoid confusion? > > >> > > >>Maybe "kernel_hwcap"? > > > > > >Yes this seems reasonable. > > > > nit: > > > > As mentioned above we have "cpu_hwcaps" for the features only internally > > by the kernel. Naming it "kernel_hwcap" kind of looses the hint that the > > major purpose is for userspace consumption and could easily confuse with > > the poorly named "cpu_hwcaps" which should have been called kernel_hwcaps. > > > > How about "user_hwcaps" ? Or preferrably something closer to that. > > Yes, that may be better. > > Of course, we also have this naming in all the KERNEL_HWCAP #defined now. > > Since kernel_hwcap is just a static variable now, maybe it's sufficient > to stick a comment next to it explaining what it is (and what it isn't). > "user_hwcaps" still implies that this might be the userspace view of the > flags, which it isn't. > > But I don't feel strongly about this. If someone wants to make a > decision, I'm happy to defer to it. I think changing the name will cause more confusion - there isn't an obvious name for it and needing a comment to explain it hints that this may not be the best approach. As it's a static variable with only 4 uses in the same file it should be pretty clear to anyone interested. Also keeping the same name will help users find it and understand how it has changed if they incorrectly attempt to use it by setting/testing bits on it. Afterall the elf_hwcap variable does still hold the elf_hwcap bits and it's obtained by cpu_get_elf_hwcap. The naming of KERNEL_HWCAP also makes sense in this context. Perhaps a better name would be something like elf_hwcaps implying that there is some mapping required (though this would only last until we run out of space in it and need another one). Shall we stick with what we have? Thanks, Andrew Murray > > Cheers > ---Dave
On Wed, Apr 03, 2019 at 09:53:26AM +0100, Andrew Murray wrote: > On Tue, Apr 02, 2019 at 04:55:58PM +0100, Dave Martin wrote: > > On Tue, Apr 02, 2019 at 04:32:57PM +0100, Suzuki K Poulose wrote: [...] > > > nit: > > > > > > As mentioned above we have "cpu_hwcaps" for the features only internally > > > by the kernel. Naming it "kernel_hwcap" kind of looses the hint that the > > > major purpose is for userspace consumption and could easily confuse with > > > the poorly named "cpu_hwcaps" which should have been called kernel_hwcaps. > > > > > > How about "user_hwcaps" ? Or preferrably something closer to that. > > > > Yes, that may be better. > > > > Of course, we also have this naming in all the KERNEL_HWCAP #defined now. > > > > Since kernel_hwcap is just a static variable now, maybe it's sufficient > > to stick a comment next to it explaining what it is (and what it isn't). > > "user_hwcaps" still implies that this might be the userspace view of the > > flags, which it isn't. > > > > But I don't feel strongly about this. If someone wants to make a > > decision, I'm happy to defer to it. > > I think changing the name will cause more confusion - there isn't an obvious > name for it and needing a comment to explain it hints that this may not be > the best approach. As it's a static variable with only 4 uses in the same > file it should be pretty clear to anyone interested. Also keeping the same > name will help users find it and understand how it has changed if they > incorrectly attempt to use it by setting/testing bits on it. > > Afterall the elf_hwcap variable does still hold the elf_hwcap bits and it's > obtained by cpu_get_elf_hwcap. The naming of KERNEL_HWCAP also makes sense > in this context. > > Perhaps a better name would be something like elf_hwcaps implying that there > is some mapping required (though this would only last until we run out of > space in it and need another one). > > Shall we stick with what we have? I'm happy enough with what you propose: I agree, there's not an obviously a better name, and now that this is local, the scope for confusion is lessened. So, add a comment, but keep whetever name you're happy with. Cheers ---Dave
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index f06e1da1d678..4c766f831de6 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -392,19 +392,12 @@ extern DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE); 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_named_feature(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_named_feature(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 d21fe3314d90..de9a66672ba7 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) @@ -84,14 +85,13 @@ #define KERNEL_HWCAP_DCPODP (ilog2(HWCAP2_DCPODP) + 32) #ifndef __ASSEMBLY__ -#include <linux/kernel.h> /* * 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) @@ -107,6 +107,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 986ceeacd19f..84ca52fa75e5 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 \ @@ -1947,6 +1946,35 @@ 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) +{ + /* + * We currently only populate the first 32 bits of AT_HWCAP. Please + * note that for userspace compatibility we guarantee that bit 62 + * will always be returned as 0. + */ + 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 | 7 +++---- arch/arm64/kernel/cpufeature.c | 32 +++++++++++++++++++++++++++-- 3 files changed, 37 insertions(+), 17 deletions(-)