Patchwork [v3,3/7] arm64: HWCAP: encapsulate elf_hwcap

login
register
mail settings
Submitter Andrew Murray
Date April 1, 2019, 10:45 a.m.
Message ID <20190401104515.39775-4-andrew.murray@arm.com>
Download mbox | patch
Permalink /patch/32116/
State New
Headers show

Comments

Andrew Murray - April 1, 2019, 10:45 a.m.
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(-)
Dave Martin - April 2, 2019, 2:58 p.m.
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
Andrew Murray - April 2, 2019, 3:06 p.m.
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
Suzuki K Poulose - April 2, 2019, 3:32 p.m.
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
Dave Martin - April 2, 2019, 3:55 p.m.
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
Andrew Murray - April 3, 2019, 8:53 a.m.
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
Dave Martin - April 3, 2019, 9:13 a.m.
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

Patch

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)
 {
 	/*