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
