Message ID | 7c7515de-c512-e399-fae6-60df57440e9a@arm.com |
---|---|
State | New |
Headers | show |
Series | [1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT | expand |
On 06/28/2018 11:52 PM, Szabolcs Nagy wrote: > This partially reverts > > commit f82e9672ad89ea1ef40bbe1af71478e255e87c5e > Author: Siddhesh Poyarekar <siddhesh@sourceware.org> > > aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK > > The idea was to make it possible to disable cpuid based ifunc resolution > in glibc by changing the hwcap mask which the user could already control. > > However the hwcap mask has an orthogonal role: it specifies additional > library search paths for the dynamic linker. So "cpuid" got added to I don't think that is correct[1]; I understood hwcap_mask to be a general tool that allows you to mask out hardware capabilities as needed and the library search paths feature happens to be a user of this. Another user is elf_machine_matches_host in sparc code for example, which uses to match the binary with supported hardware capabilities. > Meanwhile there is a tunable to set the cpu explicitly so it is possible > to disable the cpuid based dispatch without using a hwcap mask: However, I'm not too attached to the hwcap_mask way of disabling CPUID, so removing the aarch64 check is OK with me. It was a good quick option to implement back when the tune.cpu tunable was not in (and they eventually went in together IIRC, so it was a moot point) but I agree that it is redundant now. That said, it would be nice to hear from others (especially distro folks) since it may have been advertised as a way to disable the ifunc selection. Siddhesh [1] I don't have the historical reference to assert that it is definitely wrong, so if you have one I'll happily correct myself.
On 29/06/18 08:18, Siddhesh Poyarekar wrote: > On 06/28/2018 11:52 PM, Szabolcs Nagy wrote: >> This partially reverts >> >> commit f82e9672ad89ea1ef40bbe1af71478e255e87c5e >> Author: Siddhesh Poyarekar <siddhesh@sourceware.org> >> >> aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK >> >> The idea was to make it possible to disable cpuid based ifunc resolution >> in glibc by changing the hwcap mask which the user could already control. >> >> However the hwcap mask has an orthogonal role: it specifies additional >> library search paths for the dynamic linker. So "cpuid" got added to > > I don't think that is correct[1]; I understood hwcap_mask to be a general tool that allows you to mask out hardware capabilities as needed and > the library search paths feature happens to be a user of this. Another user is elf_machine_matches_host in sparc code for example, which uses to > match the binary with supported hardware capabilities. > searching libraries in ".../cpuid" path or allowing cpuid based ifunc dispatch are quite different requirements, when HWCAP_CPUID is available i think those two should be possible to control independently (especially since searching "cpuid" is almost never wanted and the ifunc dispatch is almost always wanted). the generic code currently only uses the mask to affect the library search path, the intention may be different, but my statement is defacto true and i don't see a good reason to reuse the mask for something else that may have conflicting requirement. >> Meanwhile there is a tunable to set the cpu explicitly so it is possible >> to disable the cpuid based dispatch without using a hwcap mask: > > However, I'm not too attached to the hwcap_mask way of disabling CPUID, so removing the aarch64 check is OK with me. It was a good quick option > to implement back when the tune.cpu tunable was not in (and they eventually went in together IIRC, so it was a moot point) but I agree that it > is redundant now. > > That said, it would be nice to hear from others (especially distro folks) since it may have been advertised as a way to disable the ifunc > selection. > ok, i will wait for feedback for a while, but if you don't have an objection i consider this change safe. > Siddhesh > > [1] I don't have the historical reference to assert that it is definitely wrong, so if you have one I'll happily correct myself.
On Friday 29 June 2018 09:43 PM, Szabolcs Nagy wrote: > searching libraries in ".../cpuid" path or allowing cpuid > based ifunc dispatch are quite different requirements, when > HWCAP_CPUID is available i think those two should be possible > to control independently (especially since searching "cpuid" > is almost never wanted and the ifunc dispatch is almost always > wanted). > > the generic code currently only uses the mask to affect the > library search path, the intention may be different, but my > statement is defacto true and i don't see a good reason to > reuse the mask for something else that may have conflicting > requirement. This is a different argument and I don't disagree with it. > ok, i will wait for feedback for a while, but if you don't > have an objection i consider this change safe. Thanks, I also believe the change is safe, just that I am not sure if hwcap_mask is a more popular than tune.cpu to switch off ifuncs since I haven't been in touch with distro development for a while. Hence my request for more feedback. Siddhesh
On 29/06/18 18:22, Siddhesh Poyarekar wrote: > On Friday 29 June 2018 09:43 PM, Szabolcs Nagy wrote: >> ok, i will wait for feedback for a while, but if you don't >> have an objection i consider this change safe. > > Thanks, I also believe the change is safe, just that I am not sure if > hwcap_mask is a more popular than tune.cpu to switch off ifuncs since I > haven't been in touch with distro development for a while. Hence my > request for more feedback. > now committed.
diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c index 203f839408..39eba0186f 100644 --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c @@ -53,9 +53,6 @@ get_midr_from_mcpu (const char *mcpu) static inline void init_cpu_features (struct cpu_features *cpu_features) { - uint64_t hwcap_mask = GET_HWCAP_MASK(); - uint64_t hwcap = GLRO (dl_hwcap) & hwcap_mask; - register uint64_t midr = UINT64_MAX; #if HAVE_TUNABLES @@ -69,7 +66,7 @@ init_cpu_features (struct cpu_features *cpu_features) allows it. */ if (midr == UINT64_MAX) { - if (hwcap & HWCAP_CPUID) + if (GLRO (dl_hwcap) & HWCAP_CPUID) asm volatile ("mrs %0, midr_el1" : "=r"(midr)); else midr = 0; diff --git a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h index 6887713149..c0dcce15a2 100644 --- a/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h +++ b/sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h @@ -27,9 +27,8 @@ /* We cannot provide a general printing function. */ #define _dl_procinfo(type, word) -1 -/* HWCAP_CPUID should be available by default to influence IFUNC as well as - library search. */ -#define HWCAP_IMPORTANT HWCAP_CPUID +/* No additional library search paths. */ +#define HWCAP_IMPORTANT 0 static inline const char * __attribute__ ((unused))