diff mbox

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

Message ID b5f959fd-07e2-1ab0-8ec2-1d554ed0e9ec@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto May 11, 2017, 1:42 p.m. UTC 
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
>

Comments

Siddhesh Poyarekar May 11, 2017, 2:20 p.m. UTC | #1 
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. UTC | #2 
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
diff mbox

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)