Re: [PATCH] powerpc: New feature - HWCAP/HWCAP2 bits in the TCB

["Carlos O'Donell" <carlos at redhat dot com>]

On 06/08/2015 05:03 PM, Carlos Eduardo Seo wrote:
> The proposed patch adds a new feature for powerpc. In order to get
> faster access to the HWCAP/HWCAP2 bits, we now store them in the TCB.
> This enables users to write versioned code based on the HWCAP bits
> without going through the overhead of reading them from the auxiliary
> vector.
> 
> A new API is published in ppc.h for get/set the bits in the
> aforementioned memory area (mainly for gcc to use to create
> builtins).
> 
> Testcases for the API functions were also created.
> 
> Tested on ppc32, ppc64 and ppc64le.
> 
> Okay to commit?

(1) Prevent running new applications against old glibc.

You add a new interface to glibc, but provide no way to prevent
new applications that compile with this support from crashing
or behaving badly when run on systems with an older glibc.

Richard Henderson had suggested to me that you could use a dummy
versioned symbol in the code to create a dependency against
GLIBC_2.22 and thus prevent those new binaries from running
on say GLIBC_2.21. You'd never use the versioned symbol for anything.

This would seem a much better way to prevent what will obviously
be a weird failure mode.

Have you considered this failure mode?

At the end of the day it's up to IBM to make the best use of the
tp+offset data stored in the TCB, but every byte you save is another
byte you can use later for something else.

Comments below.
 
> 2015-06-08  Carlos Eduardo Seo  <cseo@linux.vnet.ibm.com>
> 
> 	This patch adds a new feature for powerpc. In order to get faster
> 	access to the HWCAP/HWCAP2 bits, we now store them in the TCB, so
> 	we don't have to deal with the overhead of reading them via the
> 	auxiliary vector. A new API is published in ppc.h for get/set the
> 	bits.

Did you test this for 32-bit, 64-bit, and 64-bit LE?

Static and shared applications? To make sure you got both TLS init paths.

> 	* sysdeps/powerpc/nptl/tcb-offsets.sym: Added new offests
> 	for HWCAP and HWCAP2 in the TCB.
> 	* sysdeps/powerpc/nptl/tls.h: New functionality - stores
> 	the HWCAP and HWCAP2 in the TCB.
> 	(dtv): Added new fields for HWCAP and HWCAP2.
> 	(TLS_INIT_TP): Included calls to add the hwcap/hwcap2
> 	values in the TCB in TP initialization.
> 	(TLS_DEFINE_INIT_TP): Likewise.
> 	(THREAD_GET_HWCAP): New macro.
> 	(THREAD_SET_HWCAP): Likewise.
> 	(THREAD_GET_HWCAP2): Likewise.
> 	(THREAD_SET_HWCAP2): Likewise.
> 	* sysdeps/powerpc/sys/platform/ppc.h: Added new functions
> 	for get/set the HWCAP/HWCAP2 values in the TCB.
> 	(__ppc_get_hwcap): New function.
> 	(__ppc_get_hwcap2): Likewise.
> 	* sysdeps/powerpc/test-get_hwcap.c: Testcase for this
> 	functionality.
> 	* sysdeps/powerpc/test-set_hwcap.c: Testcase for this
> 	functionality.
> 	* sysdeps/powerpc/Makefile: Added testcases to the Makefile.
>

As Joseph pointed out you need to update the manual to describe this new interface.

Added the documentation step to the internals documentation for Platform Headers here:

https://sourceware.org/glibc/wiki/PlatformHeaders

> 
> Index: glibc-working/sysdeps/powerpc/nptl/tcb-offsets.sym
> ===================================================================
> --- glibc-working.orig/sysdeps/powerpc/nptl/tcb-offsets.sym
> +++ glibc-working/sysdeps/powerpc/nptl/tcb-offsets.sym
> @@ -20,6 +20,8 @@ TAR_SAVE			(offsetof (tcbhead_t, tar_sav
>  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))
>  TM_CAPABLE			(offsetof (tcbhead_t, tm_capable) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> +TCB_HWCAP			(offsetof (tcbhead_t, hwcap) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
> +TCB_HWCAP2			(offsetof (tcbhead_t, hwcap2) - TLS_TCB_OFFSET - sizeof (tcbhead_t))
>  #ifndef __ASSUME_PRIVATE_FUTEX
>  PRIVATE_FUTEX_OFFSET		thread_offsetof (header.private_futex)
>  #endif
> Index: glibc-working/sysdeps/powerpc/nptl/tls.h
> ===================================================================
> --- glibc-working.orig/sysdeps/powerpc/nptl/tls.h
> +++ glibc-working/sysdeps/powerpc/nptl/tls.h
> @@ -63,6 +63,9 @@ typedef union dtv
>     are private.  */

Please update the comment for this structure to reflect the reality of those
fields which are public ABI and those which are not.

>  typedef struct
>  {
> +  /* Reservation for HWCAP data.  */
> +  unsigned int hwcap2;
> +  unsigned int hwcap;
>    /* Indicate if HTM capable (ISA 2.07).  */
>    int tm_capable;
>    /* Reservation for Dynamic System Optimizer ABI.  */
> @@ -134,7 +137,11 @@ register void *__thread_register __asm__
>  # 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);  \
> +    unsigned int hwcap = GLRO(dl_hwcap);				      \
> +    unsigned int hwcap2 = GLRO(dl_hwcap2);				      \
> +    THREAD_SET_TM_CAPABLE (hwcap2 & PPC_FEATURE2_HAS_HTM ? 1 : 0);	      \
> +    THREAD_SET_HWCAP (hwcap);						      \
> +    THREAD_SET_HWCAP2 (hwcap2);	

OK.
					      \
>      NULL;								      \
>    })
>  
> @@ -142,7 +149,11 @@ register void *__thread_register __asm__
>  # 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].hwcap2) =	      \
> +      THREAD_GET_HWCAP2 ();

OK.

>  
>  /* Return the address of the dtv for the current thread.  */
>  # define THREAD_DTV() \
> @@ -203,6 +214,32 @@ register void *__thread_register __asm__
>  # define THREAD_SET_TM_CAPABLE(value) \
>      (THREAD_GET_TM_CAPABLE () = (value))
>  
> +/* hwcap & hwcap2 fields in TCB head.  */
> +# define THREAD_GET_HWCAP() \
> +    (((tcbhead_t *) ((char *) __thread_register				      \
> +		     - TLS_TCB_OFFSET))[-1].hwcap)
> +# define THREAD_SET_HWCAP(value) \
> +    if (value & PPC_FEATURE_ARCH_2_06)					      \
> +      value |= PPC_FEATURE_ARCH_2_05 |					      \
> +	       PPC_FEATURE_POWER5_PLUS |				      \
> +	       PPC_FEATURE_POWER5 |					      \
> +	       PPC_FEATURE_POWER4;					      \
> +    else if (value & PPC_FEATURE_ARCH_2_05)				      \
> +      value |= PPC_FEATURE_POWER5_PLUS |				      \
> +             PPC_FEATURE_POWER5 |					      \
> +             PPC_FEATURE_POWER4;					      \
> +    else if (value & PPC_FEATURE_POWER5_PLUS)				      \
> +      value |= PPC_FEATURE_POWER5 |					      \
> +             PPC_FEATURE_POWER4;					      \
> +    else if (value & PPC_FEATURE_POWER5)				      \
> +      value |= PPC_FEATURE_POWER4;					      \
> +    (THREAD_GET_HWCAP () = (value))
> +# define THREAD_GET_HWCAP2() \
> +    (((tcbhead_t *) ((char *) __thread_register				      \
> +                     - TLS_TCB_OFFSET))[-1].hwcap2)
> +# define THREAD_SET_HWCAP2(value) \
> +    (THREAD_GET_HWCAP2 () = (value))
> +

OK modulo Adhemerval's comments to try unify this.

>  /* 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
> Index: glibc-working/sysdeps/powerpc/sys/platform/ppc.h
> ===================================================================
> --- glibc-working.orig/sysdeps/powerpc/sys/platform/ppc.h
> +++ glibc-working/sysdeps/powerpc/sys/platform/ppc.h
> @@ -23,6 +23,86 @@
>  #include <stdint.h>
>  #include <bits/ppc.h>
>  
> +
> +/* Get the hwcap/hwcap2 information from the TCB. Offsets taken
> +   from tcb-offsets.h.  */
> +static inline uint32_t

Should this still inline at -O0, do you want always_inline?

Support C90 and use __inline__?

> +__ppc_get_hwcap (void)
> +{
> +
> +  uint32_t __tcb_hwcap;
> +
> +#ifdef __powerpc64__
> +  register unsigned long __tp __asm__ ("r13");
> +  __asm__ volatile ("lwz %0,-28772(%1)\n"
> +		    : "=r" (__tcb_hwcap)
> +		    : "r" (__tp));

Adhemerval notes, and I note it too, that volatile is not needed.

> +#else
> +  register unsigned long __tp __asm__ ("r2");
> +  __asm__ volatile ("lwz %0,-28724(%1)\n"
> +		    : "=r" (__tcb_hwcap)
> +		    : "r" (__tp));
> +#endif
> +
> +  return __tcb_hwcap;
> +}
> +
> +static inline uint32_t
> +__ppc_get_hwcap2 (void)

Likewise.

> +{
> +
> +  uint32_t __tcb_hwcap2;
> +
> +#ifdef __powerpc64__
> +  register unsigned long __tp __asm__ ("r13");
> +  __asm__ volatile ("lwz %0,-28776(%1)\n"
> +		    : "=r" (__tcb_hwcap2)
> +		    : "r" (__tp));
> +#else
> +  register unsigned long __tp __asm__ ("r2");
> +  __asm__ volatile ("lwz %0,-28728(%1)\n"
> +		    : "=r" (__tcb_hwcap2)
> +		    : "r" (__tp));
> +#endif
> +
> +  return __tcb_hwcap2;
> +}
> +
> +/* Set the hwcap/hwcap2 bits into the designated area in the TCB. Offsets
> +   taken from tcb-offsets.h.  */
> +
> +static inline void
> +__ppc_set_hwcap (uint32_t __hwcap_mask)

Likewise.

> +{
> +#ifdef __powerpc64__
> +  register unsigned long __tp __asm__ ("r13");
> +  __asm__ volatile ("stw %1,-28772(%0)\n"
> +		    :
> +		    : "r" (__tp), "r" (__hwcap_mask));
> +#else
> +  register unsigned long __tp __asm__ ("r2");
> +  __asm__ volatile ("stw %1,-28724(%0)\n"
> +		    :
> +		    : "r" (__tp), "r" (__hwcap_mask));
> +#endif
> +}
> +
> +static inline void
> +__ppc_set_hwcap2 (uint32_t __hwcap2_mask)

Likewise.

> +{
> +#ifdef __powerpc64__
> +  register unsigned long __tp __asm__ ("r13");
> +  __asm__ volatile ("stw %1,-28776(%0)\n"
> +		    :
> +		    : "r" (__tp), "r" (__hwcap2_mask));
> +#else
> +  register unsigned long __tp __asm__ ("r2");
> +  __asm__ volatile ("stw %1,-28728(%0)\n"
> +		    :
> +		    : "r" (__tp), "r" (__hwcap2_mask));
> +#endif
> +}
> +
>  /* Read the Time Base Register.   */
>  static inline uint64_t
>  __ppc_get_timebase (void)
> Index: glibc-working/sysdeps/powerpc/Makefile
> ===================================================================
> --- glibc-working.orig/sysdeps/powerpc/Makefile
> +++ glibc-working/sysdeps/powerpc/Makefile
> @@ -28,7 +28,7 @@ endif
>  
>  ifeq ($(subdir),misc)
>  sysdep_headers += sys/platform/ppc.h
> -tests += test-gettimebase
> +tests += test-gettimebase test-get_hwcap test-set_hwcap

Please make this one test for simplicity.

>  endif
>  
>  ifneq (,$(filter %le,$(config-machine)))
> Index: glibc-working/sysdeps/powerpc/test-get_hwcap.c
> ===================================================================
> --- /dev/null
> +++ glibc-working/sysdeps/powerpc/test-get_hwcap.c
> @@ -0,0 +1,73 @@
> +/* Check __ppc_get_hwcap() 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 and hwcap2 data is stored in the TCB.  */
> +
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +
> +#include <sys/auxv.h>
> +#include <sys/platform/ppc.h>
> +
> +static int
> +do_test (void)
> +{
> +  uint32_t h1, h2, hwcap, hwcap2;
> +
> +  h1 = __ppc_get_hwcap ();
> +  h2 = __ppc_get_hwcap2 ();
> +  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 tls.h when setting the values 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;
> +
> +  if ( h1 != hwcap )
> +    {
> +      printf("Fail: HWCAP is %x. Should be %x\n", h1, hwcap);
> +      return 1;
> +    }
> +
> +  if ( h2 != hwcap2 )
> +    {
> +      printf("Fail: HWCAP2 is %x. Should be %x\n", h2, hwcap2);
> +      return 1;
> +    }
> +
> +    printf("Pass: HWCAP and HWCAP2 are correctly set in the TCB.\n");
> +
> +    return 0;
> +
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"

OK.

> +
> +
> Index: glibc-working/sysdeps/powerpc/test-set_hwcap.c
> ===================================================================
> --- /dev/null
> +++ glibc-working/sysdeps/powerpc/test-set_hwcap.c
> @@ -0,0 +1,63 @@
> +/* Check __ppc_get_hwcap() 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 and hwcap2 data can be stored in the TCB
> +   via the ppc.h API.  */
> +
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +
> +#include <sys/auxv.h>
> +#include <sys/platform/ppc.h>
> +
> +static int
> +do_test (void)
> +{
> +  uint32_t h1, hwcap, hwcap2;
> +
> +  h1 = 0xDEADBEEF;
> +
> +  __ppc_set_hwcap(h1);
> +  hwcap = __ppc_get_hwcap();
> +
> +  if ( h1 != hwcap )
> +    {
> +      printf("Fail: HWCAP is %x. Should be %x\n", h1, hwcap);
> +      return 1;
> +    }
> +
> +  __ppc_set_hwcap2(h1);
> +  hwcap2 = __ppc_get_hwcap2();
> +
> +  if ( h1 != hwcap2 )
> +    {
> +      printf("Fail: HWCAP2 is %x. Should be %x\n", h1, hwcap2);
> +      return 1;
> +    }
> +
> +    printf("Pass: HWCAP and HWCAP2 are correctly set in the TCB.\n");
> +
> +    return 0;
> +
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> +
> +

Cheers,
Carlos.