Patchwork [6/8] tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date May 11, 2017, 1:42 p.m.
Message ID <b5f959fd-07e2-1ab0-8ec2-1d554ed0e9ec@linaro.org>
Download mbox | patch
Permalink /patch/20394/
State New
Headers show

Comments

Adhemerval Zanella Netto - May 11, 2017, 1:42 p.m.
On 10/05/2017 11:47, Siddhesh Poyarekar wrote:
> Drop _dl_hwcap_mask when building with tunables.  This completes the
> transition of hwcap_mask reading from _dl_hwcap_mask to tunables.
> 
> 	* elf/dl-cache.c: Include dl-tunables.h.
> 	(_dl_load_cache_lookup)[HAVE_TUNABLES]: Read
> 	glibc.tune.hwcap_mask.
> 	* elf/dl-hwcaps.c: Include dl-tunables.h.
> 	(_dl_important_hwcaps)[HAVE_TUNABLES]: Read and update
> 	glibc.tune.hwcap_mask.
> 	* sysdeps/sparc/sparc32/dl-machine.h: Likewise.
> 	* elf/dl-support.c (_dl_hwcap2)[HAVE_TUNABLES]: Drop
> 	_dl_hwcap_mask.
> 	* elf/dl-tunables.c (__tunable_set_val): Make a hidden alias.
> 	* elf/dl-tunables.h (__tunable_set_val): Likewise.
> 	* elf/rtld.c (rtld_global_ro)[HAVE_TUNABLES]: Drop
> 	_dl_hwcap_mask.
> 	(process_envvars)[HAVE_TUNABLES]: Likewise.
> 	* sysdeps/generic/ldsodefs.h (rtld_global_ro)[HAVE_TUNABLES]:
> 	Likewise.

This breaks x86 build with --enable-tunables:

In file included from ../sysdeps/x86_64/ldsodefs.h:54:0,
                 from ../sysdeps/gnu/ldsodefs.h:46,
                 from ../sysdeps/unix/sysv/linux/ldsodefs.h:25,
                 from rtld.c:29:
../sysdeps/x86/cpu-features.c: In function ‘init_cpu_features’:
../sysdeps/generic/ldsodefs.h:451:36: error: ‘struct rtld_global_ro’ has no member named ‘_dl_hwcap_mask’; did you mean ‘_dl_debug_mask’?
 #  define GLRO(name) _rtld_local_ro._##name
                                    ^
../sysdeps/x86/cpu-features.c:319:3: note: in expansion of macro ‘GLRO’
   GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
   ^~~~

I have tried an obvious patch:



But even though build succeeds, the elf/tst-env-setuid fails on x86_64 (still
investigating why).

> ---
>  elf/dl-cache.c                     |  9 ++++++++-
>  elf/dl-hwcaps.c                    | 15 +++++++++++++--
>  elf/dl-support.c                   |  2 ++
>  elf/dl-tunables.c                  |  1 +
>  elf/dl-tunables.h                  |  2 ++
>  elf/rtld.c                         |  4 ++++
>  sysdeps/generic/ldsodefs.h         |  2 ++
>  sysdeps/sparc/sparc32/dl-machine.h |  7 ++++++-
>  8 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 017c78a..b7ae05c 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -24,6 +24,7 @@
>  #include <dl-procinfo.h>
>  #include <stdint.h>
>  #include <_itoa.h>
> +#include <dl-tunables.h>
>  
>  #ifndef _DL_PLATFORMS_COUNT
>  # define _DL_PLATFORMS_COUNT 0
> @@ -258,8 +259,14 @@ _dl_load_cache_lookup (const char *name)
>        if (platform != (uint64_t) -1)
>  	platform = 1ULL << platform;
>  
> +#if HAVE_TUNABLES
> +      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask, uint64_t);
> +#else
> +      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
> +#endif
> +
>  #define _DL_HWCAP_TLS_MASK (1LL << 63)
> -      uint64_t hwcap_exclude = ~((GLRO(dl_hwcap) & GLRO(dl_hwcap_mask))
> +      uint64_t hwcap_exclude = ~((GLRO(dl_hwcap) & hwcap_mask)
>  				 | _DL_HWCAP_PLATFORM | _DL_HWCAP_TLS_MASK);
>  
>        /* Only accept hwcap if it's for the right platform.  */
> diff --git a/elf/dl-hwcaps.c b/elf/dl-hwcaps.c
> index c437397..6a46441a 100644
> --- a/elf/dl-hwcaps.c
> +++ b/elf/dl-hwcaps.c
> @@ -24,6 +24,7 @@
>  #include <ldsodefs.h>
>  
>  #include <dl-procinfo.h>
> +#include <dl-tunables.h>
>  
>  #ifdef _DL_FIRST_PLATFORM
>  # define _DL_FIRST_EXTRA (_DL_FIRST_PLATFORM + _DL_PLATFORMS_COUNT)
> @@ -37,8 +38,13 @@ internal_function
>  _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
>  		      size_t *max_capstrlen)
>  {
> +#if HAVE_TUNABLES
> +  uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask, uint64_t);
> +#else
> +  uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
> +#endif
>    /* Determine how many important bits are set.  */
> -  uint64_t masked = GLRO(dl_hwcap) & GLRO(dl_hwcap_mask);
> +  uint64_t masked = GLRO(dl_hwcap) & hwcap_mask;
>    size_t cnt = platform != NULL;
>    size_t n, m;
>    size_t total;
> @@ -125,7 +131,12 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
>  	 LD_HWCAP_MASK environment variable (or default HWCAP_IMPORTANT).
>  	 So there is no way to request ignoring an OS-supplied dsocap
>  	 string and bit like you can ignore an OS-supplied HWCAP bit.  */
> -      GLRO(dl_hwcap_mask) |= (uint64_t) mask << _DL_FIRST_EXTRA;
> +      hwcap_mask |= (uint64_t) mask << _DL_FIRST_EXTRA;
> +#if HAVE_TUNABLES
> +      TUNABLE_UPDATE_VAL (glibc, tune, hwcap_mask, &hwcap_mask);
> +#else
> +      GLRO(dl_hwcap_mask) = hwcap_mask;
> +#endif
>        size_t len;
>        for (const char *p = dsocaps; p < dsocaps + dsocapslen; p += len + 1)
>  	{
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 3c46a7a..c22be85 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -164,6 +164,7 @@ uint64_t _dl_hwcap2 __attribute__ ((nocommon));
>  /* The value of the FPU control word the kernel will preset in hardware.  */
>  fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
>  
> +#if !HAVE_TUNABLES
>  /* This is not initialized to HWCAP_IMPORTANT, matching the definition
>     of _dl_important_hwcaps, below, where no hwcap strings are ever
>     used.  This mask is still used to mediate the lookups in the cache
> @@ -171,6 +172,7 @@ fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
>     LD_HWCAP_MASK environment variable here), there is no real point in
>     setting _dl_hwcap nonzero below, but we do anyway.  */
>  uint64_t _dl_hwcap_mask __attribute__ ((nocommon));
> +#endif
>  
>  /* Prevailing state of the stack.  Generally this includes PF_X, indicating it's
>   * executable but this isn't true for all platforms.  */
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index abf10e5..be7733f 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -497,3 +497,4 @@ cb:
>    if (callback)
>      callback (&cur->val);
>  }
> +rtld_hidden_def (__tunable_set_val)
> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index 7a1124a..79a29a2 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -70,6 +70,8 @@ extern void __tunables_init (char **);
>  extern void __tunable_set_val (tunable_id_t, void *, tunable_callback_t);
>  extern void __tunable_update_val (tunable_id_t, void *);
>  
> +rtld_hidden_proto (__tunable_set_val)
> +
>  /* Get and return a tunable value.  */
>  # define TUNABLE_GET(__top, __ns, __id, __type) \
>  ({									      \
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 319ef06..3746653 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -159,7 +159,9 @@ struct rtld_global_ro _rtld_global_ro attribute_relro =
>      ._dl_debug_fd = STDERR_FILENO,
>      ._dl_use_load_bias = -2,
>      ._dl_correct_cache_id = _DL_CACHE_DEFAULT_ID,
> +#if !HAVE_TUNABLES
>      ._dl_hwcap_mask = HWCAP_IMPORTANT,
> +#endif
>      ._dl_lazy = 1,
>      ._dl_fpu_control = _FPU_DEFAULT,
>      ._dl_pagesize = EXEC_PAGESIZE,
> @@ -2402,6 +2404,7 @@ process_envvars (enum mode *modep)
>  	    _dl_show_auxv ();
>  	  break;
>  
> +#if !HAVE_TUNABLES
>  	case 10:
>  	  /* Mask for the important hardware capabilities.  */
>  	  if (!__libc_enable_secure
> @@ -2409,6 +2412,7 @@ process_envvars (enum mode *modep)
>  	    GLRO(dl_hwcap_mask) = __strtoul_internal (&envline[11], NULL,
>  						      0, 0);
>  	  break;
> +#endif
>  
>  	case 11:
>  	  /* Path where the binary is found.  */
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index f26a8b2..695ac24 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -515,8 +515,10 @@ struct rtld_global_ro
>    /* Mask for hardware capabilities that are available.  */
>    EXTERN uint64_t _dl_hwcap;
>  
> +#if !HAVE_TUNABLES
>    /* Mask for important hardware capabilities we honour. */
>    EXTERN uint64_t _dl_hwcap_mask;
> +#endif
>  
>  #ifdef HAVE_AUX_VECTOR
>    /* Pointer to the auxv list supplied to the program at startup.  */
> diff --git a/sysdeps/sparc/sparc32/dl-machine.h b/sysdeps/sparc/sparc32/dl-machine.h
> index cf7272f..6923e47 100644
> --- a/sysdeps/sparc/sparc32/dl-machine.h
> +++ b/sysdeps/sparc/sparc32/dl-machine.h
> @@ -39,7 +39,12 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr)
>        /* XXX The following is wrong!  Dave Miller rejected to implement it
>  	 correctly.  If this causes problems shoot *him*!  */
>  #ifdef SHARED
> -      return GLRO(dl_hwcap) & GLRO(dl_hwcap_mask) & HWCAP_SPARC_V9;
> +# if HAVE_TUNABLES
> +      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask);
> +# else
> +      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
> +# endif
> +      return GLRO(dl_hwcap) & hwcap_mask & HWCAP_SPARC_V9;
>  #else
>        return GLRO(dl_hwcap) & HWCAP_SPARC_V9;
>  #endif
>
Siddhesh Poyarekar - May 11, 2017, 2:20 p.m.
On Thursday 11 May 2017 07:12 PM, Adhemerval Zanella wrote:
> On 10/05/2017 11:47, Siddhesh Poyarekar wrote:
>> Drop _dl_hwcap_mask when building with tunables.  This completes the
>> transition of hwcap_mask reading from _dl_hwcap_mask to tunables.
>>
>> 	* elf/dl-cache.c: Include dl-tunables.h.
>> 	(_dl_load_cache_lookup)[HAVE_TUNABLES]: Read
>> 	glibc.tune.hwcap_mask.
>> 	* elf/dl-hwcaps.c: Include dl-tunables.h.
>> 	(_dl_important_hwcaps)[HAVE_TUNABLES]: Read and update
>> 	glibc.tune.hwcap_mask.
>> 	* sysdeps/sparc/sparc32/dl-machine.h: Likewise.
>> 	* elf/dl-support.c (_dl_hwcap2)[HAVE_TUNABLES]: Drop
>> 	_dl_hwcap_mask.
>> 	* elf/dl-tunables.c (__tunable_set_val): Make a hidden alias.
>> 	* elf/dl-tunables.h (__tunable_set_val): Likewise.
>> 	* elf/rtld.c (rtld_global_ro)[HAVE_TUNABLES]: Drop
>> 	_dl_hwcap_mask.
>> 	(process_envvars)[HAVE_TUNABLES]: Likewise.
>> 	* sysdeps/generic/ldsodefs.h (rtld_global_ro)[HAVE_TUNABLES]:
>> 	Likewise.
> 
> This breaks x86 build with --enable-tunables:
> 
> In file included from ../sysdeps/x86_64/ldsodefs.h:54:0,
>                  from ../sysdeps/gnu/ldsodefs.h:46,
>                  from ../sysdeps/unix/sysv/linux/ldsodefs.h:25,
>                  from rtld.c:29:
> ../sysdeps/x86/cpu-features.c: In function ‘init_cpu_features’:
> ../sysdeps/generic/ldsodefs.h:451:36: error: ‘struct rtld_global_ro’ has no member named ‘_dl_hwcap_mask’; did you mean ‘_dl_debug_mask’?
>  #  define GLRO(name) _rtld_local_ro._##name
>                                     ^
> ../sysdeps/x86/cpu-features.c:319:3: note: in expansion of macro ‘GLRO’
>    GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
>    ^~~~
> 
> I have tried an obvious patch:
> 
> diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
> index b481f50..51a6c20 100644
> --- a/sysdeps/x86/cpu-features.c
> +++ b/sysdeps/x86/cpu-features.c
> @@ -19,6 +19,7 @@
>  #include <cpuid.h>
>  #include <cpu-features.h>
>  #include <dl-hwcap.h>
> +#include <elf/dl-tunables.h>
>  
>  static void
>  get_common_indeces (struct cpu_features *cpu_features,
> @@ -316,7 +317,11 @@ no_cpuid:
>    /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
>    GLRO(dl_platform) = NULL;
>    GLRO(dl_hwcap) = 0;
> +#if HAVE_TUNABLES
> +  TUNABLE_UPDATE_VAL (glibc, tune, hwcap_mask, &(uint64_t){HWCAP_IMPORTANT});
> +#else
>    GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
> +#endif
>  
>  # ifdef __x86_64__
>    if (cpu_features->kind == arch_kind_intel)
> 
> 
> But even though build succeeds, the elf/tst-env-setuid fails on x86_64 (still
> investigating why).

That's odd, I'm pretty sure I tested x86 before aarch64.  let me check too.

Thanks,
Siddhesh
Siddhesh Poyarekar - May 11, 2017, 2:32 p.m.
On Thursday 11 May 2017 07:50 PM, Siddhesh Poyarekar wrote:
>>  static void
>>  get_common_indeces (struct cpu_features *cpu_features,
>> @@ -316,7 +317,11 @@ no_cpuid:
>>    /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
>>    GLRO(dl_platform) = NULL;
>>    GLRO(dl_hwcap) = 0;
>> +#if HAVE_TUNABLES
>> +  TUNABLE_UPDATE_VAL (glibc, tune, hwcap_mask, &(uint64_t){HWCAP_IMPORTANT});
>> +#else
>>    GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
>> +#endif
>>  
>>  # ifdef __x86_64__
>>    if (cpu_features->kind == arch_kind_intel)
>>
>>
>> But even though build succeeds, the elf/tst-env-setuid fails on x86_64 (still
>> investigating why).

OK, I figured out what happened - I did not take into account HJ's
recent patch that introduced hwcap_mask into cpu-features.c, breaking
all of this.

The correct fix here is to remove the dl_hwcap_mask initialization for
tunables because tunables already initializes it correctly.  I'll fix up
and repost the entire series minus the first patch which is relatively
simple anyway.

Siddhesh

Patch

diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index b481f50..51a6c20 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -19,6 +19,7 @@ 
 #include <cpuid.h>
 #include <cpu-features.h>
 #include <dl-hwcap.h>
+#include <elf/dl-tunables.h>
 
 static void
 get_common_indeces (struct cpu_features *cpu_features,
@@ -316,7 +317,11 @@  no_cpuid:
   /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
   GLRO(dl_platform) = NULL;
   GLRO(dl_hwcap) = 0;
+#if HAVE_TUNABLES
+  TUNABLE_UPDATE_VAL (glibc, tune, hwcap_mask, &(uint64_t){HWCAP_IMPORTANT});
+#else
   GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
+#endif
 
 # ifdef __x86_64__
   if (cpu_features->kind == arch_kind_intel)