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
