Re: [PATCHv3] powerpc: ABI change - add HWCAP/HWCAP2/platform info to TCB

[Steven Munroe <munroesj at linux dot vnet dot ibm dot com>]

On Wed, 2015-10-07 at 21:56 -0400, Carlos O'Donell wrote:
> On 10/02/2015 11:48 AM, Carlos Eduardo Seo wrote:
> >> > On Sep 30, 2015, at 8:00 PM, Steven Munroe <munroesj@linux.vnet.ibm.com> wrote:
> >> > 
> >> > With this agreed to, and the changes suggested by Joseph and Roland, I
> >> > think we have consensus.
> >> > 
> >> > Carlos please proceed.
> >> > 
> > Okay. Here is a patch that incorporates the changes suggested by
> > Joseph and Roland. Changed the versioned symbol name and namespace.
> 
> Excellent work integrating the various suggestions and requirements,
> There are still a few lingering problems though, and I expect within
> one or two more iterations we'll have this done.
> 
> (a) Reference to GLIBC_2.23 in ppc.h functions?
> 
> It is clear that Peter Bergner is going to use gcc to emit code that
> references the TCB offsets directly to produce the builtins, and that
> a data reference to __parse_hwcap_and_convert_at_platform will ensure
> the application depends on a versioned dependency of GLIBC_2.23 to
> avoid application compatibility issues.
> 
> However, it doesn't appear that __ppc_get_hwcap or __ppc_get_at_platform
> generate any references to __parse_hwcap_and_convert_at_platform? These
> two functions as added in the manual could be used by applications
> directly by including the platform header and calling those functions.
> Such applications would have no such references to __parse* and as such
> would not be protected by the dynamic loader library version checking.
> The same referecne to __parse_hwcap_and_convert_at_platform as gcc is
> using needs to be emitted in the inline assembly from those functions
> and you should double check that using these functions results in a binary
> that has a reference to a GLIBC_2.23 versioned symbol, and that running
> it on an older glibc causes an error.
> 

We can drop these (__ppc_get_hwcap or __ppc_get_at_platform) if we can
guarantee that the TCB fields are initialized before we need to run
library init or CTORs. This guarantee applies to static and dynamic
linking.

> (b) Definition of _dl_powerpc_platforms in test program.
> 
> This is an indication that something is wrong. No user code should ever
> need to define a _dl_* data symbol. Everything should just work for -static
> and -shared, otherwise you have something wrong and still need to fix it.
> 
> Why is this definition needed?
> 
> (c) Avoid custom dl-support.c
> 
> Are you sure you can't avoid this? Can we add AT_PLATFORM directly to the
> case statement used in _dl_aux_init?
> 
> (c) libc-start.c handling of AT_*
> 
> This seems like your double processing these entries here and in _dl_aux_init?
> Could you please verify this?
> 
> (d) Nit-pics about FAIL/PASS
> 
> See POSIX compliant testing notes from DejaGNU.
> 
> > 0001-powerpc-Add-hwcap-hwcap2-platform-data-to-TCB.patch
> > 
> > 
> > From 3c6f51f776766d2136e4f66519537a8dd24303ca Mon Sep 17 00:00:00 2001
> > From: Carlos Eduardo Seo <cseo@linux.vnet.ibm.com>
> > Date: Fri, 2 Oct 2015 12:36:48 -0300
> > Subject: [PATCH] powerpc: Add hwcap/hwcap2/platform data to TCB.
> > 
> > This patch adds a new feature for powerpc.  In order to get faster access to
> > the HWCAP/HWCAP2 bits and platform number (i.e. for implementing
> > __builtin_cpu_is () / __builtin_cpu_supports () in GCC) without the overhead of
> > reading from the auxiliary vector, we now reserve space for them in the TCB.
> > This is an ABI change for GLIBC 2.23.
> > 
> > A new versioned symbol '__parse_hwcap_and_convert_at_platform' is available to
> > get the data from the auxiliary vector and parse it, and store it for later use
> > in the TLS initialization code.  This function is called very early
> > (in _dl_sysdep_start () via DL_PLATFORM_INFO for the dynamic linking case, and
> > in __libc_start_main () for the static linking case) to make sure the data is
> > available at the time of TLS initialization.
> > 
> > A new API is published in ppc.h to get the HWCAP+HWCAP2 and platform number
> > data from the TCB as well.
> > 
> > 2015-10-02  Carlos Eduardo Seo  <cseo@linux.vnet.ibm.com>
> > 
> > 	* manual/platform.texi: Added a manual entry for the new API.
> > 	* sysdeps/powerpc/Makefile: Added testcases and new hwcapinfo file to
> > 	the Makefile.
> > 	* sysdeps/powerpc/Versions: Added new
> > 	__parse_hwcap_and_convert_at_platform symbol to GLIBC-2.23.
> > 	* sysdeps/powerpc/hwcapinfo.c: New file.
> > 	(__tcb_parse_hwcap_and_convert_at_platform): New function to initialize
> > 	and parse hwcap, hwcap2 and platform number information.
> > 	* sysdeps/powerpc/hwcapinfo.h: New file.  Creates global variables
> > 	to store HWCAP+HWCAP2 and platform number.
> > 	* sysdeps/powerpc/nptl/tcb-offsets.sym: Added new offests
> > 	for HWCAP+HWCAP2 and platform number in the TCB.
> > 	* sysdeps/powerpc/nptl/tls.h: New functionality.  Stores
> > 	the HWCAP, HWCAP2 and platform number in the TCB.
> > 	(dtv): Added new fields for HWCAP+HWCAP2 and platform number.
> > 	(TLS_INIT_TP): Included calls to add the hwcap and
> > 	at_platform values in the TCB in TP initialization.
> > 	(TLS_DEFINE_INIT_TP): Likewise.
> > 	(THREAD_GET_HWCAP): New macro.
> > 	(THREAD_SET_HWCAP): Likewise.
> > 	(THREAD_GET_AT_PLATFORM): Likewise.
> > 	(THREAD_SET_AT_PLATFORM): Likewise.
> > 	* sysdeps/powerpc/powerpc32/dl-machine.h:
> > 	(dl_platform_init): New function that calls
> > 	__parse_hwcap_and_convert_at_platform for the dymanic linking case for
> > 	powerpc32.
> > 	* sysdeps/powerpc/powerpc64/dl-machine.h: Likewise, for powerpc64.
> > 	* sysdeps/powerpc/sys/platform/ppc.h: Added new functions
> > 	to get the HWCAP+HWCAP2 and platform number values from the TCB.
> > 	* sysdeps/powerpc/test-get_hwcap-static.c: New file.  Testcase for
> > 	this functionality, static linking case.
> > 	* sysdeps/powerpc/test-get_hwcap.c: New file.  Likewise, dynamic
> > 	linking case.
> > 	* sysdeps/unix/sysv/linux/powerpc/dl-support.c: New file.
> > 	Adds support for AT_PLATFORM in static glibc.
> > 	* sysdeps/unix/sysv/linux/powerpc/libc-start.c: Added call to
> > 	__parse_hwcap_and_convert_at_platform for the static linking case.
> > 	* sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist:
> > 	Included the new __parse_hwcap_and_convert_at_platform symbol in the
> > 	ABI list for GLIBC 2.23.
> > 	* sysdeps/unix/sysv/linux/powerpc/powerpc64/libc-le.abilist:
> > 	Likewise.
> > 	* sysdeps/unix/sysv/linux/powerpc/powerpc64/libc.abilist:
> > 	Likewise.
> > ---
> >  manual/platform.texi                               |  20 +++
> >  sysdeps/powerpc/Makefile                           |  12 +-
> >  sysdeps/powerpc/Versions                           |   5 +
> >  sysdeps/powerpc/hwcapinfo.c                        |  71 +++++++++
> >  sysdeps/powerpc/hwcapinfo.h                        |  30 ++++
> >  sysdeps/powerpc/nptl/tcb-offsets.sym               |   8 +
> >  sysdeps/powerpc/nptl/tls.h                         |  44 +++++-
> >  sysdeps/powerpc/powerpc32/dl-machine.h             |  14 ++
> >  sysdeps/powerpc/powerpc64/dl-machine.h             |  14 ++
> >  sysdeps/powerpc/sys/platform/ppc.h                 |  57 +++++++
> >  sysdeps/powerpc/test-get_hwcap-static.c            |  24 +++
> >  sysdeps/powerpc/test-get_hwcap.c                   | 164 +++++++++++++++++++++
> >  sysdeps/unix/sysv/linux/powerpc/dl-support.c       |  25 ++++
> >  sysdeps/unix/sysv/linux/powerpc/libc-start.c       |  24 ++-
> >  .../unix/sysv/linux/powerpc/powerpc32/ld.abilist   |   3 +
> >  .../sysv/linux/powerpc/powerpc64/ld-le.abilist     |   3 +
> >  .../unix/sysv/linux/powerpc/powerpc64/ld.abilist   |   3 +
> >  17 files changed, 514 insertions(+), 7 deletions(-)
> >  create mode 100644 sysdeps/powerpc/hwcapinfo.c
> >  create mode 100644 sysdeps/powerpc/hwcapinfo.h
> >  create mode 100644 sysdeps/powerpc/test-get_hwcap-static.c
> >  create mode 100644 sysdeps/powerpc/test-get_hwcap.c
> >  create mode 100644 sysdeps/unix/sysv/linux/powerpc/dl-support.c
> > 
> > diff --git a/manual/platform.texi b/manual/platform.texi
> > index cb16664..e47b170 100644
> > --- a/manual/platform.texi
> > +++ b/manual/platform.texi
> > @@ -14,6 +14,26 @@
> >  Facilities specific to PowerPC that are not specific to a particular
> >  operating system are declared in @file{sys/platform/ppc.h}.
> >  
> > +@deftypefun {uint64_t} __ppc_get_hwcap (void)
> > +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> > +Read the current value of the hardware capability (HWCAP and HCWCAP2) bits.
> > +This function, as opposed to @code{getauxval}, is designed to be as fast
> > +as possible including caching the result to reduce access latency.
> > +
> > +The result returned is a single doubleword containing the HWCAP value in the
> > +32 high-order bits and the HWCAP2 value in the lower 32 bits.
> 
> OK.
> 
> > +@end deftypefun
> > +
> > +@deftypefun {uint32_t} __ppc_get_at_platform (void)
> > +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> > +Read the current value of the platform number.  This function, as opposed to
> > +@code{getauxval}, is designed to be as fast as possible including caching the
> > +result to reduce access latency.
> > +
> > +The platform number identifies the current processor.  It is derived from
> > +the AT_PLATFORM variable in the auxiliary vector.
> > +@end deftypefun
> > +
> 
> OK.
> 
> >  @deftypefun {uint64_t} __ppc_get_timebase (void)
> >  @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> >  Read the current value of the Time Base Register.
> > diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
> > index 533d763..77dcb7d 100644
> > --- a/sysdeps/powerpc/Makefile
> > +++ b/sysdeps/powerpc/Makefile
> > @@ -4,10 +4,10 @@ endif
> >  
> >  ifeq ($(subdir),elf)
> >  # extra shared linker files to link into dl-allobjs.so and libc
> > -sysdep-dl-routines += dl-machine
> > -sysdep_routines += dl-machine
> > +sysdep-dl-routines += dl-machine hwcapinfo
> > +sysdep_routines += dl-machine hwcapinfo
> 
> OK.
> 
> >  # extra shared linker files to link only into dl-allobjs.so
> > -sysdep-rtld-routines += dl-machine
> > +sysdep-rtld-routines += dl-machine hwcapinfo
> 
> OK.
> 
> >  # Don't optimize GD tls sequence to LE.
> >  LDFLAGS-tst-tlsopt-powerpc += -Wl,--no-tls-optimize
> >  tests += tst-tlsopt-powerpc
> > @@ -26,6 +26,12 @@ gen-as-const-headers += rtld-global-offsets.sym
> >  gen-as-const-headers += locale-defines.sym
> >  endif
> >  
> > +ifeq ($(subdir),nptl)
> > +sysdep_headers += sys/platform/ppc.h
> > +tests += test-get_hwcap test-get_hwcap-static
> > +tests-static += test-get_hwcap-static
> > +endif
> > +
> 
> OK.
> 
> >  ifeq ($(subdir),misc)
> >  sysdep_headers += sys/platform/ppc.h
> >  tests += test-gettimebase
> > diff --git a/sysdeps/powerpc/Versions b/sysdeps/powerpc/Versions
> > index 2aebf7c..4af8de2 100644
> > --- a/sysdeps/powerpc/Versions
> > +++ b/sysdeps/powerpc/Versions
> > @@ -20,4 +20,9 @@ ld {
> >    GLIBC_2.22 {
> >      __tls_get_addr_opt;
> >    }
> > +  GLIBC_2.23 {
> > +    # symbol used to version control when the ABI started to specify that HWCAP
> > +    # and AT_PLATFORM data should be stored into the TCB.
> 
> Please mention that gcc will emit a data reference to this symbol when it generates
> code that uses the new TCB entries.
> 
> OK with that change.
> 
> > +    __parse_hwcap_and_convert_at_platform;
> > +  }
> >  }
> > diff --git a/sysdeps/powerpc/hwcapinfo.c b/sysdeps/powerpc/hwcapinfo.c
> > new file mode 100644
> > index 0000000..0afacdf
> > --- /dev/null
> > +++ b/sysdeps/powerpc/hwcapinfo.c
> > @@ -0,0 +1,71 @@
> > +/* powerpc HWCAP/HWCAP2 and AT_PLATFORM data pre-processing.
> > +   Copyright (C) 2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#include <unistd.h>
> > +#include <shlib-compat.h>
> > +#include <dl-procinfo.h>
> > +
> > +uint64_t __tcb_hwcap __attribute__ ((visibility ("hidden")));
> > +uint32_t __tcb_platform __attribute__ ((visibility ("hidden")));
> 
> OK.
> 
> I hope that's enough bits ;-)
> 
> > +
> > +/* This function parses the HWCAP/HWCAP2 fields, adding the previous supported
> > +   ISA bits, as well as converting the AT_PLATFORM string to a number.  This
> > +   data is stored in two global variables that can be used later by the
> > +   powerpc-specific code to store it into the TCB.  */
> > +void
> > +__tcb_parse_hwcap_and_convert_at_platform (void)
> > +{
> > +
> > +  uint64_t h1, h2;
> > +
> > +  /* Read AT_PLATFORM string from auxv and convert it to a number.  */
> > +  __tcb_platform = _dl_string_platform (GLRO (dl_platform));
> > +
> > +  /* Read HWCAP and HWCAP2 from auxv.  */
> > +  h1 = GLRO (dl_hwcap);
> > +  h2 = GLRO (dl_hwcap2);
> > +
> > +  /* hwcap contains only the latest supported ISA, the code checks which is
> > +     and fills the previous supported ones.  */
> > +
> > +  if (h1 & PPC_FEATURE_ARCH_2_06)
> > +      h1 |= PPC_FEATURE_ARCH_2_05 |	      \
> > +	    PPC_FEATURE_POWER5_PLUS |	      \
> > +	    PPC_FEATURE_POWER5 |	      \
> > +	    PPC_FEATURE_POWER4;
> > +    else if (h1 & PPC_FEATURE_ARCH_2_05)
> > +      h1 |= PPC_FEATURE_POWER5_PLUS |	      \
> > +	    PPC_FEATURE_POWER5 |	      \
> > +	    PPC_FEATURE_POWER4;
> > +    else if (h1 & PPC_FEATURE_POWER5_PLUS)
> > +      h1 |= PPC_FEATURE_POWER5 |	      \
> > +	    PPC_FEATURE_POWER4;
> > +    else if (h1 & PPC_FEATURE_POWER5)
> > +      h1|= PPC_FEATURE_POWER4;
> > +
> > +  /* Consolidate both HWCAP and HWCAP2 into a single doubleword so that
> > +     we can read both in a single load later.  */
> > +  __tcb_hwcap = h2;
> > +  __tcb_hwcap = (h1 << 32) | __tcb_hwcap;
> 
> OK.
> 
> > +
> > +}
> > +#if IS_IN (rtld)
> > +versioned_symbol (ld, __tcb_parse_hwcap_and_convert_at_platform, \
> > +		  __parse_hwcap_and_convert_at_platform, GLIBC_2_23);
> 
> OK.
> 
> > +#endif
> > +
> > diff --git a/sysdeps/powerpc/hwcapinfo.h b/sysdeps/powerpc/hwcapinfo.h
> > new file mode 100644
> > index 0000000..72bcea8
> > --- /dev/null
> > +++ b/sysdeps/powerpc/hwcapinfo.h
> > @@ -0,0 +1,30 @@
> > +/* powerpc HWCAP/HWCAP2 and AT_PLATFORM data pre-processing.
> > +   Copyright (C) 2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#include <stdint.h>
> > +
> > +#ifndef HWCAPINFO_H
> > +# define HWCAPINFO_H
> > +
> > +extern uint64_t __tcb_hwcap  attribute_hidden;
> > +extern uint32_t __tcb_platform attribute_hidden;
> > +
> > +extern void __tcb_parse_hwcap_and_convert_at_platform (void);
> 
> OK.
> 
> > +
> > +#endif
> > +
> > diff --git a/sysdeps/powerpc/nptl/tcb-offsets.sym b/sysdeps/powerpc/nptl/tcb-offsets.sym
> > index d955142..f580e69 100644
> > --- a/sysdeps/powerpc/nptl/tcb-offsets.sym
> > +++ b/sysdeps/powerpc/nptl/tcb-offsets.sym
> > @@ -19,7 +19,15 @@ POINTER_GUARD			(offsetof (tcbhead_t, pointer_guard) - TLS_TCB_OFFSET - sizeof (
> >  TAR_SAVE			(offsetof (tcbhead_t, tar_save) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> >  DSO_SLOT1			(offsetof (tcbhead_t, dso_slot1) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> >  DSO_SLOT2			(offsetof (tcbhead_t, dso_slot2) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> > +#ifdef __powerpc64__
> > +TCB_AT_PLATFORM			(offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
> > +#endif
> >  TM_CAPABLE			(offsetof (tcbhead_t, tm_capable) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> > +#ifndef __powerpc64__
> > +TCB_AT_PLATFORM			(offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
> > +PADDING				(offsetof (tcbhead_t, padding) - TLS_TCB_OFFSET - sizeof(tcbhead_t))
> > +#endif
> > +TCB_HWCAP			(offsetof (tcbhead_t, hwcap) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> 
> OK.
> 
> >  #ifndef __ASSUME_PRIVATE_FUTEX
> >  PRIVATE_FUTEX_OFFSET		thread_offsetof (header.private_futex)
> >  #endif
> > diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
> > index 1f3d97a..7b6682c 100644
> > --- a/sysdeps/powerpc/nptl/tls.h
> > +++ b/sysdeps/powerpc/nptl/tls.h
> > @@ -44,6 +44,8 @@ typedef union dtv
> >  
> >  #ifndef __ASSEMBLER__
> >  
> > +# include <hwcapinfo.h>
> > +
> >  /* Get system call information.  */
> >  # include <sysdep.h>
> >  
> > @@ -63,8 +65,24 @@ typedef union dtv
> >     are private.  */
> >  typedef struct
> >  {
> > +  /* Reservation for HWCAP data.  To be accessed by GCC in
> > +     __builtin_cpu_supports(), so it is a part of public ABI.  */
> 
> s/part of/part of the/g
> 
> OK with that change.
> 
> > +  uint64_t hwcap;
> > +  /* Reservation for AT_PLATFORM data.  To be accessed by GCC in
> > +     __builtin_cpu_is(), so it is a part of public ABI.  Since there
> 
> Likewise.
> 
> > +     are different ABIs for 32 and 64 bit, we put this field in a
> > +     previously empty padding space for powerpc64.  */
> > +#ifndef __powerpc64__
> > +  /* Padding to maintain alignment.  */
> > +  uint32_t padding;
> > +  uint32_t at_platform;
> > +#endif
> >    /* Indicate if HTM capable (ISA 2.07).  */
> > -  int tm_capable;
> > +  uint32_t tm_capable;
> > +  /* Reservation for AT_PLATFORM data - powerpc64.  */
> > +#ifdef __powerpc64__
> > +  uint32_t at_platform;
> > +#endif
> >    /* Reservation for Dynamic System Optimizer ABI.  */
> >    uintptr_t dso_slot2;
> >    uintptr_t dso_slot1;
> > @@ -134,7 +152,9 @@ register void *__thread_register __asm__ ("r13");
> >  # define TLS_INIT_TP(tcbp) \
> >    ({ 									      \
> >      __thread_register = (void *) (tcbp) + TLS_TCB_OFFSET;		      \
> > -    THREAD_SET_TM_CAPABLE (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_HTM ? 1 : 0);  \
> > +    THREAD_SET_TM_CAPABLE (__tcb_hwcap & PPC_FEATURE2_HAS_HTM ? 1 : 0);	      \
> > +    THREAD_SET_HWCAP (__tcb_hwcap);					      \
> > +    THREAD_SET_AT_PLATFORM (__tcb_platform);				      \
> 
> OK, setup main thread.
> 
> >      NULL;								      \
> >    })
> >  
> > @@ -142,7 +162,11 @@ register void *__thread_register __asm__ ("r13");
> >  # define TLS_DEFINE_INIT_TP(tp, pd) \
> >      void *tp = (void *) (pd) + TLS_TCB_OFFSET + TLS_PRE_TCB_SIZE;	      \
> >      (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].tm_capable) =	      \
> > -      THREAD_GET_TM_CAPABLE ();
> > +      THREAD_GET_TM_CAPABLE ();						      \
> > +    (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].hwcap) =	      \
> > +      THREAD_GET_HWCAP ();						      \
> > +    (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].at_platform) =	      \
> > +      THREAD_GET_AT_PLATFORM ();
> 
> OK, setup all other threads.
> 
> >  
> >  /* Return the address of the dtv for the current thread.  */
> >  # define THREAD_DTV() \
> > @@ -203,6 +227,20 @@ register void *__thread_register __asm__ ("r13");
> >  # define THREAD_SET_TM_CAPABLE(value) \
> >      (THREAD_GET_TM_CAPABLE () = (value))
> >  
> > +/* hwcap field in TCB head.  */
> > +# define THREAD_GET_HWCAP() \
> > +    (((tcbhead_t *) ((char *) __thread_register				      \
> > +		     - TLS_TCB_OFFSET))[-1].hwcap)
> > +# define THREAD_SET_HWCAP(value) \
> > +    (THREAD_GET_HWCAP () = (value))
> > +
> > +/* at_platform field in TCB head.  */
> > +# define THREAD_GET_AT_PLATFORM() \
> > +    (((tcbhead_t *) ((char *) __thread_register				      \
> > +		     - TLS_TCB_OFFSET))[-1].at_platform)
> > +# define THREAD_SET_AT_PLATFORM(value) \
> > +    (THREAD_GET_AT_PLATFORM () = (value))
> > +
> 
> OK.
> 
> >  /* l_tls_offset == 0 is perfectly valid on PPC, so we have to use some
> >     different value to mean unset l_tls_offset.  */
> >  # define NO_TLS_OFFSET		-1
> > diff --git a/sysdeps/powerpc/powerpc32/dl-machine.h b/sysdeps/powerpc/powerpc32/dl-machine.h
> > index 8b0c067..8d2f311 100644
> > --- a/sysdeps/powerpc/powerpc32/dl-machine.h
> > +++ b/sysdeps/powerpc/powerpc32/dl-machine.h
> > @@ -24,6 +24,7 @@
> >  #include <assert.h>
> >  #include <dl-tls.h>
> >  #include <dl-irel.h>
> > +#include <hwcapinfo.h>
> >  
> >  /* Translate a processor specific dynamic tag to the index
> >     in l_info array.  */
> > @@ -150,6 +151,19 @@ __elf_preferred_address(struct link_map *loader, size_t maplength,
> >  #define ELF_MACHINE_NO_REL 1
> >  #define ELF_MACHINE_NO_RELA 0
> >  
> > +/* We define an initialization function to initialize HWCAP/HWCAP2 and
> > +   platform data so it can be copied into the TCB later.  This is called
> > +   very early in _dl_sysdep_start for dynamically linked binaries.  */
> > +#ifdef SHARED
> > +# define DL_PLATFORM_INIT dl_platform_init ()
> > +
> > +static inline void __attribute__ ((unused))
> > +dl_platform_init (void)
> > +{
> > +  __tcb_parse_hwcap_and_convert_at_platform ();
> > +}
> > +#endif
> 
> OK.
> 
> > +
> >  /* Set up the loaded object described by MAP so its unrelocated PLT
> >     entries will jump to the on-demand fixup code in dl-runtime.c.
> >     Also install a small trampoline to be used by entries that have
> > diff --git a/sysdeps/powerpc/powerpc64/dl-machine.h b/sysdeps/powerpc/powerpc64/dl-machine.h
> > index 0576781..dc0f522 100644
> > --- a/sysdeps/powerpc/powerpc64/dl-machine.h
> > +++ b/sysdeps/powerpc/powerpc64/dl-machine.h
> > @@ -26,6 +26,7 @@
> >  #include <sys/param.h>
> >  #include <dl-tls.h>
> >  #include <sysdep.h>
> > +#include <hwcapinfo.h>
> >  
> >  /* Translate a processor specific dynamic tag to the index
> >     in l_info array.  */
> > @@ -296,6 +297,19 @@ BODY_PREFIX "_dl_start_user:\n"						\
> >  #define ELF_MACHINE_NO_REL 1
> >  #define ELF_MACHINE_NO_RELA 0
> >  
> > +/* We define an initialization function to initialize HWCAP/HWCAP2 and
> > +   platform data so it can be copied into the TCB later.  This is called
> > +   very early in _dl_sysdep_start for dynamically linked binaries.  */
> > +#ifdef SHARED
> > +# define DL_PLATFORM_INIT dl_platform_init ()
> > +
> > +static inline void __attribute__ ((unused))
> > +dl_platform_init (void)
> > +{
> > +  __tcb_parse_hwcap_and_convert_at_platform ();
> > +}
> > +#endif
> 
> OK.
> 
> > +
> >  /* Stuff for the PLT.  */
> >  #if _CALL_ELF != 2
> >  #define PLT_INITIAL_ENTRY_WORDS 3
> > diff --git a/sysdeps/powerpc/sys/platform/ppc.h b/sysdeps/powerpc/sys/platform/ppc.h
> > index 2594606..dab03ac 100644
> > --- a/sysdeps/powerpc/sys/platform/ppc.h
> > +++ b/sysdeps/powerpc/sys/platform/ppc.h
> > @@ -23,6 +23,63 @@
> >  #include <stdint.h>
> >  #include <bits/ppc.h>
> >  
> > +/* Offsets copied from tcb-offsets.h.  */
> > +
> > +#ifdef __powerpc64__
> > +# define __TPREG     "r13"
> > +# define __HWCAPOFF -28776
> > +# define __ATPLATOFF -28764
> > +#else
> > +# define __TPREG     "r2"
> > +# define __HWCAPOFF -28736
> > +# define __HWCAP2OFF -28732
> > +# define __ATPLATOFF -28724
> > +#endif
> 
> OK.
> 
> > +
> > +/* Get the hwcap/hwcap2 information from the TCB.  */
> > +
> > +static __inline__ uint64_t
> > +__ppc_get_hwcap (void)
> > +{
> > +
> > +  uint64_t __tcb_hwcap;
> > +
> > +  register unsigned long __tp __asm__ (__TPREG);
> > +#ifdef __powerpc64__
> > +  __asm__  ("ld %0,%1(%2)\n"
> > +	    : "=r" (__tcb_hwcap)
> > +	    : "i" (__HWCAPOFF), "b" (__tp));
> > +#else
> > +  uint64_t h1, h2;
> > +
> > +  __asm__ ("lwz %0,%1(%2)\n"
> > +      : "=r" (h1)
> > +      : "i" (__HWCAPOFF), "b" (__tp));
> > +  __asm__ ("lwz %0,%1(%2)\n"
> > +      : "=r" (h2)
> > +      : "i" (__HWCAP2OFF), "b" (__tp));
> > +  __tcb_hwcap = (h1 >> 32) << 32 | (h2 >> 32);
> > +#endif
> > +
> > +  return __tcb_hwcap;
> > +}
> > +
> > +/* Get the AT_PLATFORM number from the TCB.  */
> > +
> > +static __inline__ uint32_t
> > +__ppc_get_at_platform (void)
> > +{
> > +
> > +  uint32_t __tcb_at_platform;
> > +
> > +  register unsigned long __tp __asm__ (__TPREG);
> > +  __asm__  ("lwz %0,%1(%2)\n"
> > +	    : "=r" (__tcb_at_platform)
> > +	    : "i" (__ATPLATOFF), "b" (__tp));
> > +
> > +  return __tcb_at_platform;
> > +}
> > +
> 
> Not OK. Neither of these functions reference the dummy symbol we're
> using to ensure compatibility?
> 
> It would appear to me that an application including the platform
> specific header, and calling the inline function, would not have any
> reference against GLIBC_2.23 symbols, and would therefore fail to
> run correctly on older glibc.
> 
> >  /* Read the Time Base Register.   */
> >  static inline uint64_t
> >  __ppc_get_timebase (void)
> > diff --git a/sysdeps/powerpc/test-get_hwcap-static.c b/sysdeps/powerpc/test-get_hwcap-static.c
> > new file mode 100644
> > index 0000000..965d8bd
> > --- /dev/null
> > +++ b/sysdeps/powerpc/test-get_hwcap-static.c
> > @@ -0,0 +1,24 @@
> > +/* Check __ppc_get_hwcap() and __ppc_get_at_plaftorm() functionality.
> > +   Copyright (C) 2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +/* Tests if the hwcap, hwcap2 and platform data are stored in the TCB.  */
> > +
> > +#define STATIC_TST_HWCAP 1
> > +
> > +#include "test-get_hwcap.c"
> > +
> 
> OK.
> 
> > diff --git a/sysdeps/powerpc/test-get_hwcap.c b/sysdeps/powerpc/test-get_hwcap.c
> > new file mode 100644
> > index 0000000..1d2dfbf
> > --- /dev/null
> > +++ b/sysdeps/powerpc/test-get_hwcap.c
> > @@ -0,0 +1,164 @@
> > +/* Check __ppc_get_hwcap() and __ppc_get_at_plaftorm() functionality.
> > +   Copyright (C) 2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +/* Tests if the hwcap, hwcap2 and platform data are stored in the TCB.  */
> > +
> > +#include <inttypes.h>
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <pthread.h>
> > +
> > +#include <sys/auxv.h>
> > +#include <sys/platform/ppc.h>
> > +
> > +#include <dl-procinfo.h>
> > +
> > +#ifndef STATIC_TST_HWCAP
> > +/* We need to duplicate this from dl-procinfo.c so _dl_string_platform can
> > +   use it (dynamic linking case only - static case can get this from
> > +   dl-support).  */
> > +const char _dl_powerpc_platforms[14][12]
> > += {
> > +    [PPC_PLATFORM_POWER4] = "power4",
> > +    [PPC_PLATFORM_PPC970] = "ppc970",
> > +    [PPC_PLATFORM_POWER5] = "power5",
> > +    [PPC_PLATFORM_POWER5_PLUS] = "power5+",
> > +    [PPC_PLATFORM_POWER6] = "power6",
> > +    [PPC_PLATFORM_CELL_BE] = "ppc-cell-be",
> > +    [PPC_PLATFORM_POWER6X] = "power6x",
> > +    [PPC_PLATFORM_POWER7] = "power7",
> > +    [PPC_PLATFORM_PPCA2] = "ppca2",
> > +    [PPC_PLATFORM_PPC405] = "ppc405",
> > +    [PPC_PLATFORM_PPC440] = "ppc440",
> > +    [PPC_PLATFORM_PPC464] = "ppc464",
> > +    [PPC_PLATFORM_PPC476] = "ppc476",
> > +    [PPC_PLATFORM_POWER8] = "power8",
> > +  };
> > +#endif
> > +
> 
> This does not look correct.
> 
> There should be no scenario where a user program is required to have
> a _dl_* symbol to make one of the API routines work. It should work
> transparently in -static and -shared compilations.
> 
> You need to fix something or explain clearly what's going on here.
> 
> > +uint64_t check_tcbhwcap (long tid)
> > +{
> > +
> > +  uint32_t a1, at_platform;
> > +  uint64_t h1, h2, hwcap, hwcap2;
> > +  const char *at_platform_string;
> > +
> > +  /* Testing the get functions and check if the hwcap/hwcap2 data is
> > +     correctly initialized by TLS_TP_INIT.  */
> > +
> > +  h1 = __ppc_get_hwcap ();
> > +  hwcap = getauxval (AT_HWCAP);
> > +  hwcap2 = getauxval (AT_HWCAP2);
> > +
> > +  /* hwcap contains only the latest supported ISA, the code checks which is
> > +     and fills the previous supported ones.  This is necessary because the
> > +     same is done in hwcapinfo.c when setting the values that are copied to
> > +     the TCB.  */
> > +
> > +  if (hwcap & PPC_FEATURE_ARCH_2_06)
> > +    hwcap |= PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_POWER5_PLUS |
> > +	     PPC_FEATURE_POWER5 | PPC_FEATURE_POWER4;
> > +  else if (hwcap & PPC_FEATURE_ARCH_2_05)
> > +    hwcap |= PPC_FEATURE_POWER5_PLUS | PPC_FEATURE_POWER5 | PPC_FEATURE_POWER4;
> > +  else if (hwcap & PPC_FEATURE_POWER5_PLUS)
> > +    hwcap |= PPC_FEATURE_POWER5 | PPC_FEATURE_POWER4;
> > +  else if (hwcap & PPC_FEATURE_POWER5)
> > +    hwcap |= PPC_FEATURE_POWER4;
> > +
> > +  h2 = (hwcap << 32) + hwcap2;
> > +
> > +  if ( h1 != h2 )
> > +    {
> > +      printf ("Fail: __ppc_get_hwcap() - HWCAP is %" PRIx64 ". Should be %"
> > +	      PRIx64 " for thread %ld.\n", h1, h2, tid);
> > +      return 1;
> 
> Not OK. You should run all tests not quit at the first failure.
> 
> Please use "FAIL:" not "Fail:"
> 
> > +    }
> > +
> > +  /* Same test for the platform number.  */
> > +  a1 = __ppc_get_at_platform ();
> > +  at_platform_string = (const char *) getauxval (AT_PLATFORM);
> > +  at_platform = _dl_string_platform (at_platform_string);
> > +
> > +  if ( a1 != at_platform )
> > +    {
> > +      printf ("Fail: __ppc_get_at_platform() - AT_PLATFORM is %x. Should be %x"
> > +	     " for thread %ld\n", a1, at_platform, tid);
> > +      return 1;
> > +    }
> > +
> > +  return 0;
> 
> This should be `return err`, where err is set to one if any of the
> tests failed, or zero if no tests failed.
> 
> > +}
> > +
> > +void *t1 (void *tid)
> > +{
> > +  if (check_tcbhwcap ((long) tid))
> > +    {
> > +      pthread_exit (tid);
> > +    }
> > +
> > +  pthread_exit (NULL);
> > +
> > +}
> 
> OK.
> 
> > +
> > +static int
> > +do_test (void)
> > +{
> > +
> > +  pthread_t threads[2];
> > +  pthread_attr_t attr;
> > +  pthread_attr_init (&attr);
> > +  pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_JOINABLE);
> > +  void *status;
> > +
> > +  long i = 0;
> > +
> > +  /* Check for main.  */
> > +  if (check_tcbhwcap (i))
> > +    {
> > +      return 1;
> > +    }
> > +
> > +  /* Check for other thread.  */
> > +  i++;
> > +  if (pthread_create (&threads[i], &attr, t1, (void *)i))
> > +    {
> > +      printf ("Fail: error creating thread %ld.\n", i);
> > +      return 1;
> > +    }
> 
> OK, thanks for using threads.
> 
> Again, note the use of "Fail:" should be "FAIL:", like the POSIX
> conforming test framework.
> 
> See:
> http://www.gnu.org/software/dejagnu/manual/x47.html#posix
> 
> > +
> > +  pthread_attr_destroy (&attr);
> > +  if (pthread_join (threads[i], &status))
> > +    {
> > +      printf ("Fail: error joining thread %ld.\n", i);
> > +      return 1;
> > +    }
> > +  if (status)
> > +    {
> > +      return 1;
> > +    }
> > +
> > +  printf("Pass: HWCAP, HWCAP2 and AT_PLATFORM are correctly set in the TCB for"
> > +	 " all threads.\n");
> 
> Similarly "PASS"
> 
> > +
> > +  pthread_exit (NULL);
> > +
> > +}
> > +
> > +#define TEST_FUNCTION do_test ()
> > +#include "../test-skeleton.c"
> > +
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-support.c b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
> > new file mode 100644
> > index 0000000..a672eca
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
> > @@ -0,0 +1,25 @@
> > +/* Operating system support for dynamic linking code in static glibc.
> > +   Linux/PPC version.
> > +   Copyright (C) 2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library.  If not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +#define DL_PLATFORM_AUXV				\
> > +	case AT_PLATFORM:				\
> > +	  GLRO (dl_platform) = (void *) av->a_un.a_val;	\
> > +	  break;
> 
> This is not quite right. If you can use GLRO (dl_platform) during a static
> link, then every other platform should be able to do so also? Why can't
> you just add AT_PLATFORM directly into the case statement in _dl_aux_init
> like it is already listed in _dl_sysdep_start (shared version)?
> 
> Note that cleaner is:
> 
> dl-auxv.h [DL_PLATFORM_AUXV]: Define.
> dl-support.c: Include dl-auxv.h and elf/dl-support.c
> 
> Like alpha does it, but that's not all that important. Here I'm looking
> to fix this across architectures if possible.
> 
> > +
> > +#include <elf/dl-support.c>
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/libc-start.c b/sysdeps/unix/sysv/linux/powerpc/libc-start.c
> > index a9364c7..209a16d 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/libc-start.c
> > +++ b/sysdeps/unix/sysv/linux/powerpc/libc-start.c
> > @@ -20,6 +20,9 @@
> >  #include <ldsodefs.h>
> >  #include <sysdep.h>
> >  
> > +#ifndef SHARED
> > +#include <hwcapinfo.h>
> > +#endif
> >  
> >  int __cache_line_size attribute_hidden;
> >  /* The main work is done in the generic function.  */
> > @@ -68,15 +71,34 @@ __libc_start_main (int argc, char **argv,
> >        rtld_fini = NULL;
> >      }
> >  
> > -  /* Initialize the __cache_line_size variable from the aux vector.  */
> > +  /* Initialize the __cache_line_size variable from the aux vector.  For the
> > +     static case, we also need _dl_hwcap, _dl_hwcap2 and _dl_platform, so we
> > +     can call __tcb_parse_hwcap_and_convert_at_platform ().  */
> >    for (ElfW (auxv_t) * av = auxvec; av->a_type != AT_NULL; ++av)
> >      switch (av->a_type)
> >        {
> >        case AT_DCACHEBSIZE:
> >  	__cache_line_size = av->a_un.a_val;
> >  	break;
> > +#ifndef SHARED
> > +      case AT_HWCAP:
> > +	_dl_hwcap = (unsigned long int) av->a_un.a_val;
> > +	break;
> > +      case AT_HWCAP2:
> > +	_dl_hwcap2 = (unsigned long int) av->a_un.a_val;
> > +	break;
> > +      case AT_PLATFORM:
> > +	_dl_platform = (void *) av->a_un.a_val;
> > +	break;
> > +#endif
> 
> Why do you need this?
> 
> In the static case this calls generic_libc_start, which calls
> _dl_aux_init, which does this:
> 
> 247       case AT_HWCAP:
> 248         GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val;
> 249         break;
> 250       case AT_HWCAP2:
> 251         GLRO(dl_hwcap2) = (unsigned long int) av->a_un.a_val;
> 252         break;
> 
> ... but doesn't does AT_PLATFORM, which is part of the discussion above.
> 
> It seems to me like you're doing this work twice?
> 
> You could also follow up with a cleanup patch that moves AT_DCACHEBSIZE
> processing into the power-specific dl-auxv.h/dl-support.c, and remove
> it from libc-start.c, where it's out of place.
> 
> >        }
> >  
> > +  /* Initialize hwcap/hwcap2 and platform data so it can be copied to
> > +     the TCB later in __libc_setup_tls (). (static case only).  */
> > +#ifndef SHARED
> > +  __tcb_parse_hwcap_and_convert_at_platform ();
> > +#endif
> > +
> 
> OK.
> 
> >    return generic_start_main (stinfo->main, argc, argv, auxvec,
> >  			     stinfo->init, stinfo->fini, rtld_fini,
> >  			     stack_on_entry);
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/ld.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/ld.abilist
> > index 7d24961..3fd3802 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/ld.abilist
> > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/ld.abilist
> > @@ -13,6 +13,9 @@ GLIBC_2.1
> >  GLIBC_2.22
> >   GLIBC_2.22 A
> >   __tls_get_addr_opt F
> > +GLIBC_2.23
> > + GLIBC_2.23 A
> > + __parse_hwcap_and_convert_at_platform F
> 
> OK.
> 
> >  GLIBC_2.3
> >   GLIBC_2.3 A
> >   __tls_get_addr F
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld-le.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld-le.abilist
> > index 3174e21..3b3717c 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld-le.abilist
> > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld-le.abilist
> > @@ -12,3 +12,6 @@ GLIBC_2.17
> >  GLIBC_2.22
> >   GLIBC_2.22 A
> >   __tls_get_addr_opt F
> > +GLIBC_2.23
> > + GLIBC_2.23 A
> > + __parse_hwcap_and_convert_at_platform F
> 
> OK.
> 
> > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld.abilist
> > index d8c4201..2823d13 100644
> > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld.abilist
> > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/ld.abilist
> > @@ -1,6 +1,9 @@
> >  GLIBC_2.22
> >   GLIBC_2.22 A
> >   __tls_get_addr_opt F
> > +GLIBC_2.23
> > + GLIBC_2.23 A
> > + __parse_hwcap_and_convert_at_platform F
> 
> OK.
> 
> >  GLIBC_2.3
> >   GLIBC_2.3 A
> >   __libc_memalign F
> > -- 2.3.8 (Apple Git-58)
> 
> Cheers,
> Carlos.
>