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:

quoted

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

[...]

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel