[11/11] powerpc64le: Add glibc-hwcaps support

  The "power10" and "power9" subdirectories are selected in a way
that matches the -mcpu=power10 and -mcpu=power9 options of GCC.
---
 elf/Makefile                                  |  2 +-
 elf/tst-glibc-hwcaps-cache.script             |  6 +++
 sysdeps/powerpc/powerpc64/le/Makefile         | 28 ++++++++++
 .../powerpc/powerpc64/le/dl-hwcaps-subdirs.c  | 46 ++++++++++++++++
 .../powerpc/powerpc64/le/tst-glibc-hwcaps.c   | 54 +++++++++++++++++++
 5 files changed, 135 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c
 create mode 100644 sysdeps/powerpc/powerpc64/le/tst-glibc-hwcaps.c
  

On 09/11/2020 15:41, Florian Weimer via Libc-alpha wrote:
> The "power10" and "power9" subdirectories are selected in a way
> that matches the -mcpu=power10 and -mcpu=power9 options of GCC.

LGTM, a comment about testing below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/Makefile                                  |  2 +-
>  elf/tst-glibc-hwcaps-cache.script             |  6 +++
>  sysdeps/powerpc/powerpc64/le/Makefile         | 28 ++++++++++
>  .../powerpc/powerpc64/le/dl-hwcaps-subdirs.c  | 46 ++++++++++++++++
>  .../powerpc/powerpc64/le/tst-glibc-hwcaps.c   | 54 +++++++++++++++++++
>  5 files changed, 135 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c
>  create mode 100644 sysdeps/powerpc/powerpc64/le/tst-glibc-hwcaps.c
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 0a44b19a0e..60e8662e5d 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1826,7 +1826,7 @@ $(objpfx)argv0test.out: tst-rtld-argv0.sh $(objpfx)ld.so \
>  
>  # Most likely search subdirectories, for each supported architecture.
>  # Used to obtain test coverage wide test coverage.
> -glibc-hwcaps-first-subdirs-for-tests = x86-64-v2
> +glibc-hwcaps-first-subdirs-for-tests = power9 x86-64-v2
>  
>  # The test modules are parameterized by preprocessor macros.
>  LDFLAGS-libmarkermod1-1.so += -Wl,-soname,libmarkermod1.so

Ok.

> diff --git a/elf/tst-glibc-hwcaps-cache.script b/elf/tst-glibc-hwcaps-cache.script
> index 2aef9fb177..a3e3317bbf 100644
> --- a/elf/tst-glibc-hwcaps-cache.script
> +++ b/elf/tst-glibc-hwcaps-cache.script
> @@ -10,3 +10,9 @@ mkdirp 0770 $L/glibc-hwcaps/x86-64-v4
>  cp $B/elf/libmarkermod4-2.so $L/glibc-hwcaps/x86-64-v2/libmarkermod4.so
>  cp $B/elf/libmarkermod4-3.so $L/glibc-hwcaps/x86-64-v3/libmarkermod4.so
>  cp $B/elf/libmarkermod4-4.so $L/glibc-hwcaps/x86-64-v4/libmarkermod4.so
> +
> +mkdirp 0770 $L/glibc-hwcaps/power9
> +cp $B/elf/libmarkermod2-2.so $L/glibc-hwcaps/power9/libmarkermod2.so
> +mkdirp 0770 $L/glibc-hwcaps/power10
> +cp $B/elf/libmarkermod3-2.so $L/glibc-hwcaps/power9/libmarkermod3.so
> +cp $B/elf/libmarkermod3-3.so $L/glibc-hwcaps/power10/libmarkermod3.so

Ok.

> diff --git a/sysdeps/powerpc/powerpc64/le/Makefile b/sysdeps/powerpc/powerpc64/le/Makefile
> index 033dc77b01..7c036b45fc 100644
> --- a/sysdeps/powerpc/powerpc64/le/Makefile
> +++ b/sysdeps/powerpc/powerpc64/le/Makefile
> @@ -188,3 +188,31 @@ ifeq ($(subdir),nptl)
>  CFLAGS-tst-thread_local1.cc += -mno-float128
>  CFLAGS-tst-minstack-throw.cc += -mno-float128
>  endif
> +
> +ifeq ($(subdir),elf)
> +$(objpfx)tst-glibc-hwcaps: \
> +  $(objpfx)libmarkermod2-1.so $(objpfx)libmarkermod3-1.so
> +$(objpfx)tst-glibc-hwcaps.out: \
> +  $(objpfx)libmarkermod2.so \
> +    $(objpfx)glibc-hwcaps/power9/libmarkermod2.so \
> +  $(objpfx)libmarkermod3.so \
> +    $(objpfx)glibc-hwcaps/power9/libmarkermod3.so \
> +    $(objpfx)glibc-hwcaps/power10/libmarkermod3.so \
> +
> +$(objpfx)glibc-hwcaps/power9/libmarkermod2.so: $(objpfx)libmarkermod2-2.so
> +	$(make-target-directory)
> +	cp $< $@
> +$(objpfx)glibc-hwcaps/power9/libmarkermod3.so: $(objpfx)libmarkermod3-2.so
> +	$(make-target-directory)
> +	cp $< $@
> +$(objpfx)glibc-hwcaps/power10/libmarkermod3.so: $(objpfx)libmarkermod3-3.so
> +	$(make-target-directory)
> +	cp $< $@
> +
> +ifeq (no,$(build-hardcoded-path-in-tests))
> +# This is an ld.so.cache test, and RPATH/RUNPATH in the executable
> +# interferes with its test objectives.
> +tests-container += tst-glibc-hwcaps-cache
> +endif
> +
> +endif # $(subdir) == elf

Ok.

> diff --git a/sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c b/sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c
> new file mode 100644
> index 0000000000..0ce76c2fe5
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/le/dl-hwcaps-subdirs.c
> @@ -0,0 +1,46 @@
> +/* Architecture-specific glibc-hwcaps subdirectories.  powerpc64le version.
> +   Copyright (C) 2020 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <dl-hwcaps.h>
> +#include <ldsodefs.h>
> +
> +const char _dl_hwcaps_subdirs[] = "power10:power9";
> +enum { subdirs_count = 2 }; /* Number of components in _dl_hwcaps_subdirs.  */
> +
> +uint32_t
> +_dl_hwcaps_subdirs_active (void)
> +{
> +  int active = 0;
> +
> +  /* Test in reverse preference order.  Altivec and VSX are implied by
> +     the powerpc64le ABI definition.  */
> +
> +  /* POWER9.  GCC enables float128 hardware support for -mcpu=power9.  */
> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00) == 0
> +      || (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_IEEE128) == 0)
> +    return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
> +  ++active;

Should we test PPC_FEATURE2_DARN as well? I think cryptographic and related
libraries might use it as source of entropy.

> +
> +  /* POWER10. GCC defines __MMA__ for -mcpu=power10.  */

Double space after period.

> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_1) == 0
> +      || (GLRO (dl_hwcap2) & PPC_FEATURE2_MMA) == 0)
> +    return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
> +  ++active;
> +
> +  return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
> +}

Ok.

> diff --git a/sysdeps/powerpc/powerpc64/le/tst-glibc-hwcaps.c b/sysdeps/powerpc/powerpc64/le/tst-glibc-hwcaps.c
> new file mode 100644
> index 0000000000..e510fca80a
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/le/tst-glibc-hwcaps.c
> @@ -0,0 +1,54 @@
> +/* glibc-hwcaps subdirectory test.  powerpc64le version.
> +   Copyright (C) 2020 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <sys/auxv.h>
> +#include <sys/param.h>
> +
> +extern int marker2 (void);
> +extern int marker3 (void);
> +
> +/* Return the POWER level, 8 for the baseline.  */
> +static int
> +compute_level (void)
> +{
> +  const char *platform = (const char *) getauxval (AT_PLATFORM);
> +  if (strcmp (platform, "power8") == 0)
> +    return 8;
> +  if (strcmp (platform, "power9") == 0)
> +    return 9;
> +  if (strcmp (platform, "power10") == 0)
> +    return 10;

Why not use HWCAP? Is it to decouple from the loader code that uses it?
I am asking because by using hwcap we can check if this tests is working
on different chips by using qemu-ppc64le with -mcpu.

> +  printf ("warning: unrecognized AT_PLATFORM value: %s\n", platform);
> +  /* Assume that the new platform supports POWER10.  */
> +  return 10;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int level = compute_level ();
> +  printf ("info: detected POWER level: %d\n", level);
> +  TEST_COMPARE (marker2 (), MIN (level - 7, 2));
> +  TEST_COMPARE (marker3 (), MIN (level - 7, 3));
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 

Ok.
  

* Adhemerval Zanella:

>> +uint32_t
>> +_dl_hwcaps_subdirs_active (void)
>> +{
>> +  int active = 0;
>> +
>> +  /* Test in reverse preference order.  Altivec and VSX are implied by
>> +     the powerpc64le ABI definition.  */
>> +
>> +  /* POWER9.  GCC enables float128 hardware support for -mcpu=power9.  */
>> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00) == 0
>> +      || (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_IEEE128) == 0)
>> +    return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
>> +  ++active;
>
> Should we test PPC_FEATURE2_DARN as well? I think cryptographic and related
> libraries might use it as source of entropy.

It does not show up in the compiler preprocessor macros.  There also
have been cases where randomness-generating instructions have been
disabled in firmware (but probably not on POWER).  I think it's safer
not to include cryptographic stuff.

>> +
>> +  /* POWER10. GCC defines __MMA__ for -mcpu=power10.  */
>
> Double space after period.

Thanks, fixed.

>> +/* Return the POWER level, 8 for the baseline.  */
>> +static int
>> +compute_level (void)
>> +{
>> +  const char *platform = (const char *) getauxval (AT_PLATFORM);
>> +  if (strcmp (platform, "power8") == 0)
>> +    return 8;
>> +  if (strcmp (platform, "power9") == 0)
>> +    return 9;
>> +  if (strcmp (platform, "power10") == 0)
>> +    return 10;
>
> Why not use HWCAP? Is it to decouple from the loader code that uses it?
> I am asking because by using hwcap we can check if this tests is working
> on different chips by using qemu-ppc64le with -mcpu.

And that doesn't affect AT_PLATFORM?  That would be a QEMU bug.

Thanks,
Florian
  

On 02/12/2020 10:51, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> +uint32_t
>>> +_dl_hwcaps_subdirs_active (void)
>>> +{
>>> +  int active = 0;
>>> +
>>> +  /* Test in reverse preference order.  Altivec and VSX are implied by
>>> +     the powerpc64le ABI definition.  */
>>> +
>>> +  /* POWER9.  GCC enables float128 hardware support for -mcpu=power9.  */
>>> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00) == 0
>>> +      || (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_IEEE128) == 0)
>>> +    return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
>>> +  ++active;
>>
>> Should we test PPC_FEATURE2_DARN as well? I think cryptographic and related
>> libraries might use it as source of entropy.
> 
> It does not show up in the compiler preprocessor macros.  There also
> have been cases where randomness-generating instructions have been
> disabled in firmware (but probably not on POWER).  I think it's safer
> not to include cryptographic stuff.

GCC does provides it through a compiler builtin, only enabled for
power9 and newer.  And I think hardware entropy instruction are not
used on cryptographic, but also on different fields as simulations.

> 
>>> +
>>> +  /* POWER10. GCC defines __MMA__ for -mcpu=power10.  */
>>
>> Double space after period.
> 
> Thanks, fixed.
> 
>>> +/* Return the POWER level, 8 for the baseline.  */
>>> +static int
>>> +compute_level (void)
>>> +{
>>> +  const char *platform = (const char *) getauxval (AT_PLATFORM);
>>> +  if (strcmp (platform, "power8") == 0)
>>> +    return 8;
>>> +  if (strcmp (platform, "power9") == 0)
>>> +    return 9;
>>> +  if (strcmp (platform, "power10") == 0)
>>> +    return 10;
>>
>> Why not use HWCAP? Is it to decouple from the loader code that uses it?
>> I am asking because by using hwcap we can check if this tests is working
>> on different chips by using qemu-ppc64le with -mcpu.
> 
> And that doesn't affect AT_PLATFORM?  That would be a QEMU bug.

At least not with v5.2.0-rc0-99-g3493c36f03, not sure if is has been 
fixed/change upstream.
  

* Adhemerval Zanella:

> On 02/12/2020 10:51, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>>> +uint32_t
>>>> +_dl_hwcaps_subdirs_active (void)
>>>> +{
>>>> +  int active = 0;
>>>> +
>>>> +  /* Test in reverse preference order.  Altivec and VSX are implied by
>>>> +     the powerpc64le ABI definition.  */
>>>> +
>>>> +  /* POWER9.  GCC enables float128 hardware support for -mcpu=power9.  */
>>>> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00) == 0
>>>> +      || (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_IEEE128) == 0)
>>>> +    return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
>>>> +  ++active;
>>>
>>> Should we test PPC_FEATURE2_DARN as well? I think cryptographic and related
>>> libraries might use it as source of entropy.
>> 
>> It does not show up in the compiler preprocessor macros.  There also
>> have been cases where randomness-generating instructions have been
>> disabled in firmware (but probably not on POWER).  I think it's safer
>> not to include cryptographic stuff.
>
> GCC does provides it through a compiler builtin, only enabled for
> power9 and newer.  And I think hardware entropy instruction are not
> used on cryptographic, but also on different fields as simulations.

Maybe Tulio can comment.  I do not have a strong opinion (although the
x86 precedent is kind of unsettling).

Thanks,
Florian
  

'darn' requires firmware support, so that's why it has a hwcap2 bit
separate from ISA 3.0 (see commit a4700a26 in the kernel tree). Although I
personally have not seen any firmware that disables that on POWER9, it is
safer to assume that ISA 3.0 does not imply that 'darn' is available.

On Wed, 2 Dec 2020 at 11:31, Florian Weimer via Libc-alpha <
libc-alpha@sourceware.org> wrote:

> * Adhemerval Zanella:
>
> > On 02/12/2020 10:51, Florian Weimer wrote:
> >> * Adhemerval Zanella:
> >>
> >>>> +uint32_t
> >>>> +_dl_hwcaps_subdirs_active (void)
> >>>> +{
> >>>> +  int active = 0;
> >>>> +
> >>>> +  /* Test in reverse preference order.  Altivec and VSX are implied
> by
> >>>> +     the powerpc64le ABI definition.  */
> >>>> +
> >>>> +  /* POWER9.  GCC enables float128 hardware support for
> -mcpu=power9.  */
> >>>> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00) == 0
> >>>> +      || (GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_IEEE128) == 0)
> >>>> +    return _dl_hwcaps_subdirs_build_bitmask (subdirs_count, active);
> >>>> +  ++active;
> >>>
> >>> Should we test PPC_FEATURE2_DARN as well? I think cryptographic and
> related
> >>> libraries might use it as source of entropy.
> >>
> >> It does not show up in the compiler preprocessor macros.  There also
> >> have been cases where randomness-generating instructions have been
> >> disabled in firmware (but probably not on POWER).  I think it's safer
> >> not to include cryptographic stuff.
> >
> > GCC does provides it through a compiler builtin, only enabled for
> > power9 and newer.  And I think hardware entropy instruction are not
> > used on cryptographic, but also on different fields as simulations.
>
> Maybe Tulio can comment.  I do not have a strong opinion (although the
> x86 precedent is kind of unsettling).
>
> Thanks,
> Florian
> --
> Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
> Commercial register: Amtsgericht Muenchen, HRB 153243,
> Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael
> O'Neill
>
>
  

Carlos Seo via Libc-alpha <libc-alpha@sourceware.org> writes:

> 'darn' requires firmware support, so that's why it has a hwcap2 bit
> separate from ISA 3.0 (see commit a4700a26 in the kernel tree). Although I
> personally have not seen any firmware that disables that on POWER9, it is
> safer to assume that ISA 3.0 does not imply that 'darn' is available.

I think we have 2 different questions to be answered here:

1. Is it safe to use darn on a processor compatible with the POWER ISA 3.0b?

2. Should glibc-hwcaps test for PPC_FEATURE2_DARN when loading POWER9-specific
   libraries?

I agree with Carlos regarding question #1: it's unsafe to use darn without
testing for PPC_FEATURE2_DARN.

Regarding question #2, I see darn much like HTM even if darn doesn't have its
own -m parameter.  What I mean is: I don't think the entire power9 directory
should be disabled if a kernel (or firmware) decides to disable
PPC_FEATURE2_DARN.

With that said, I don't have a strong opinion on question #2.

By the way, I have the same opinion for scv: it's unsafe to use it without
testing for PPC_FEATURE2_SCV and I think glibc-hwcaps should not require it.
  

* Tulio Magno Quites Machado Filho via Libc-alpha:

> Carlos Seo via Libc-alpha <libc-alpha@sourceware.org> writes:
>
>> 'darn' requires firmware support, so that's why it has a hwcap2 bit
>> separate from ISA 3.0 (see commit a4700a26 in the kernel tree). Although I
>> personally have not seen any firmware that disables that on POWER9, it is
>> safer to assume that ISA 3.0 does not imply that 'darn' is available.
>
> I think we have 2 different questions to be answered here:
>
> 1. Is it safe to use darn on a processor compatible with the POWER ISA 3.0b?
>
> 2. Should glibc-hwcaps test for PPC_FEATURE2_DARN when loading POWER9-specific
>    libraries?
>
> I agree with Carlos regarding question #1: it's unsafe to use darn without
> testing for PPC_FEATURE2_DARN.
>
> Regarding question #2, I see darn much like HTM even if darn doesn't have its
> own -m parameter.  What I mean is: I don't think the entire power9 directory
> should be disabled if a kernel (or firmware) decides to disable
> PPC_FEATURE2_DARN.
>
> With that said, I don't have a strong opinion on question #2.
>
> By the way, I have the same opinion for scv: it's unsafe to use it without
> testing for PPC_FEATURE2_SCV and I think glibc-hwcaps should not require it.

Okay, then the posted patch is okay as is?  May I consider it reviewed?

Thanks,
Florian
  

On 04/12/2020 05:56, Florian Weimer wrote:
> * Tulio Magno Quites Machado Filho via Libc-alpha:
> 
>> Carlos Seo via Libc-alpha <libc-alpha@sourceware.org> writes:
>>
>>> 'darn' requires firmware support, so that's why it has a hwcap2 bit
>>> separate from ISA 3.0 (see commit a4700a26 in the kernel tree). Although I
>>> personally have not seen any firmware that disables that on POWER9, it is
>>> safer to assume that ISA 3.0 does not imply that 'darn' is available.
>>
>> I think we have 2 different questions to be answered here:
>>
>> 1. Is it safe to use darn on a processor compatible with the POWER ISA 3.0b?
>>
>> 2. Should glibc-hwcaps test for PPC_FEATURE2_DARN when loading POWER9-specific
>>    libraries?
>>
>> I agree with Carlos regarding question #1: it's unsafe to use darn without
>> testing for PPC_FEATURE2_DARN.
>>
>> Regarding question #2, I see darn much like HTM even if darn doesn't have its
>> own -m parameter.  What I mean is: I don't think the entire power9 directory
>> should be disabled if a kernel (or firmware) decides to disable
>> PPC_FEATURE2_DARN.
>>
>> With that said, I don't have a strong opinion on question #2.
>>
>> By the way, I have the same opinion for scv: it's unsafe to use it without
>> testing for PPC_FEATURE2_SCV and I think glibc-hwcaps should not require it.
> 
> Okay, then the posted patch is okay as is?  May I consider it reviewed?

LGTM as is, I think we can add PPC_FEATURE2_DARN as an extra hwcap folder
is required.