This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCHv3] powerpc: ABI change - add HWCAP/HWCAP2/platform info to TCB
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Carlos Eduardo Seo <cseo at linux dot vnet dot ibm dot com>, munroesj at linux dot vnet dot ibm dot com
- Cc: Peter Bergner <bergner at vnet dot ibm dot com>, Roland McGrath <roland at hack dot frob dot com>, Joseph Myers <joseph at codesourcery dot com>, GNU C Library <libc-alpha at sourceware dot org>, Tulio Machado <tuliom at linux dot vnet dot ibm dot com>, "Steven J. Munroe" <sjmunroe at us dot ibm dot com>
- Date: Wed, 7 Oct 2015 21:56:45 -0400
- Subject: Re: [PATCHv3] powerpc: ABI change - add HWCAP/HWCAP2/platform info to TCB
- Authentication-results: sourceware.org; auth=none
- References: <4BE991CB-F601-4D63-A416-5991FE870FC4 at linux dot vnet dot ibm dot com> <alpine dot DEB dot 2 dot 10 dot 1509232114070 dot 10585 at digraph dot polyomino dot org dot uk> <B650304C-6207-46AC-B6A8-FA29959B305D at linux dot vnet dot ibm dot com> <alpine dot DEB dot 2 dot 10 dot 1509232141250 dot 10585 at digraph dot polyomino dot org dot uk> <1443046986 dot 23503 dot 68 dot camel at otta> <alpine dot DEB dot 2 dot 10 dot 1509232234500 dot 10585 at digraph dot polyomino dot org dot uk> <1443050731 dot 23503 dot 78 dot camel at otta> <alpine dot DEB dot 2 dot 10 dot 1509232340470 dot 10585 at digraph dot polyomino dot org dot uk> <1443057751 dot 13186 dot 6 dot camel at otta> <alpine dot DEB dot 2 dot 10 dot 1509240126140 dot 10585 at digraph dot polyomino dot org dot uk> <1443058522 dot 13186 dot 9 dot camel at otta> <1443654009 dot 4885 dot 4 dot camel at oc7878010663> <B2EB15A6-FEB5-4DBA-8719-0BAA5B65EF5E at linux dot vnet dot ibm dot com>
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.
(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.