[PATCH 1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT

[PATCH 1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

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
the search paths when it was set in the default mask (HWCAP_IMPORTANT),
which is not useful behaviour, the hwcap masking should not be reused
in the cpu features code.

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:

   GLIBC_TUNABLES=glibc.tune.cpu=generic

2018-06-28  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* sysdeps/unix/sysv/linux/aarch64/cpu-features.c (init_cpu_features):
	Use dl_hwcap without masking.
	* sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h (HWCAP_IMPORTANT):
	Remove HWCAP_CPUID.
---
  sysdeps/unix/sysv/linux/aarch64/cpu-features.c | 5 +----
  sysdeps/unix/sysv/linux/aarch64/dl-procinfo.h  | 5 ++---
  2 files changed, 3 insertions(+), 7 deletions(-)


[-- Attachment #2: 0001-aarch64-Remove-HWCAP_CPUID-from-HWCAP_IMPORTANT.diff --]
[-- Type: text/x-patch, Size: 1440 bytes --]

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))

Re: [PATCH 1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT

[Siddhesh Poyarekar]

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.

Re: [PATCH 1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT

[Szabolcs Nagy]

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.

Re: [PATCH 1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT

[Siddhesh Poyarekar]

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

Re: [PATCH 1/2] aarch64: Remove HWCAP_CPUID from HWCAP_IMPORTANT

[Szabolcs Nagy]

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.