[2/3] powerpc64le: Check HWCAP bits against compiler build flags

  When built with GCC 11.1 and -mcpu=power9, ld.so prints this error
message when running on POWER8:

Fatal glibc error: CPU lacks ISA 3.00 support (POWER9 or later required)

This approach does not work for the POWER10 because the bootstrap
relocation already uses PCREL instructions, so the detection code does
not actually run.
---
 sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h
  

Hi Florian. I've tested this patch on POWER[8..10] with -mcpu=power9 and
-mcpu=power10 built with GCC 10.3.1.

When built with -mcpu=power10 I got a SIGILL on POWER8 and POWER10.
For testing I'm just running ./libc.so.  I try to inspect it with
gdb. With -mcpu=power9 I can see it passing through dl_hwcap_check
but not with -mcpu=power10.

I maybe doing something wrong though.

---
Lucas A. M. Magalhães

Quoting Florian Weimer via Libc-alpha (2021-05-06 09:30:15)
> When built with GCC 11.1 and -mcpu=power9, ld.so prints this error
> message when running on POWER8:
> 
> Fatal glibc error: CPU lacks ISA 3.00 support (POWER9 or later required)
> 
> This approach does not work for the POWER10 because the bootstrap
> relocation already uses PCREL instructions, so the detection code does
> not actually run.
> ---
>  sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h
> 
> diff --git a/sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h b/sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h
> new file mode 100644
> index 0000000000..6c7949c6d2
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/le/dl-hwcap-check.h
> @@ -0,0 +1,49 @@
> +/* Check for hardware capabilities after HWCAP parsing.  powerpc64le version.
> +   Copyright (C) 2021 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/>.  */
> +
> +#ifndef _DL_HWCAP_CHECK_H
> +#define _DL_HWCAP_CHECK_H
> +
> +#include <ldsodefs.h>
> +
> +static inline void
> +dl_hwcap_check (void)
> +{
> +#ifdef _ARCH_PWR9
> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_00) == 0)
> +    _dl_fatal_printf ("\
> +Fatal glibc error: CPU lacks ISA 3.00 support (POWER9 or later required)\n");
> +#endif
> +#ifdef __FLOAT128_HARDWARE__
> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_HAS_IEEE128) == 0)
> +    _dl_fatal_printf ("\
> +Fatal glibc error: CPU lacks float128 support (POWER 9 or later required)\n");
> +#endif
> +#if defined _ARCH_PWR10 || defined __PCREL__
> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_ARCH_3_1) == 0)
> +    _dl_fatal_printf ("\
> +Fatal glibc error: CPU lacks ISA 3.10 support (POWER10 or later required)\n");
> +#endif
> +#ifdef __MMA__
> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_MMA) == 0)
> +    _dl_fatal_printf ("\
> +Fatal glibc error: CPU lacks MMA support (POWER10 or later required)\n");
> +#endif
> +}
> +
> +#endif /* _DL_HWCAP_CHECK_H */
> -- 
> 2.30.2
> 
>
  

* Lucas A. M. Magalhaes:

> Hi Florian. I've tested this patch on POWER[8..10] with -mcpu=power9 and
> -mcpu=power10 built with GCC 10.3.1.
>
> When built with -mcpu=power10 I got a SIGILL on POWER8 and POWER10.

Do you mean POWER8 and POWER9?

> For testing I'm just running ./libc.so.  I try to inspect it with
> gdb. With -mcpu=power9 I can see it passing through dl_hwcap_check
> but not with -mcpu=power10.
>
> I maybe doing something wrong though.

I tried to allude in the commit message that this is expected.  Maybe I
should drop the POWER10 check?

I do not think this is fixable in the long term because I expect that
POWER10 builds won't need the bootstrap relocation in the dynamic loader
thanks to its PCREL instructions.  (POWER10 should be
PI_STATIC_AND_HIDDEN.)  We could disable POWER10 code for early parts of
the dynamic loader, but that will only work until we start to rely on
PI_STATIC_AND_HIDDEN, which has other benefits.

Thanks,
Florian
  

Quoting Florian Weimer (2021-05-12 05:27:33)
> * Lucas A. M. Magalhaes:
> 
> > Hi Florian. I've tested this patch on POWER[8..10] with -mcpu=power9 and
> > -mcpu=power10 built with GCC 10.3.1.
> >
> > When built with -mcpu=power10 I got a SIGILL on POWER8 and POWER10.
> 
> Do you mean POWER8 and POWER9?
> 
Yes. Sorry about that.

> > For testing I'm just running ./libc.so.  I try to inspect it with
> > gdb. With -mcpu=power9 I can see it passing through dl_hwcap_check
> > but not with -mcpu=power10.
> >
> > I maybe doing something wrong though.
> 
> I tried to allude in the commit message that this is expected.  Maybe I
> should drop the POWER10 check?
> 
Your commit message is fine, I misunderstood it. IMHO it's better to
drop POWER10 check and add a comment explaining why it's not there.

> I do not think this is fixable in the long term because I expect that
> POWER10 builds won't need the bootstrap relocation in the dynamic loader
> thanks to its PCREL instructions.  (POWER10 should be
> PI_STATIC_AND_HIDDEN.)  We could disable POWER10 code for early parts of
> the dynamic loader, but that will only work until we start to rely on
> PI_STATIC_AND_HIDDEN, which has other benefits.
I'm not familiar with this bootstrap relocation but for what I understood
there is early code using PCREL instructions so it SIGILL before running
this code, right?

I don't oppose this change but still don't get why do we need this code?
What's the benefit of the message instead of just SIGILL?

---
Lucas A. M. Magalhães
  

Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:

> I tried to allude in the commit message that this is expected.  Maybe I
> should drop the POWER10 check?

I don't think this is necessary.
Even if you removed the P10 check, I don't think the P9 check is guaranteed to
work because when _ARCH_PWR9 is set, the compiler is allowed to use P9
instructions in this function before the test is executed.

I think this patch is trying to improve the error message to the user.

With that said, it doesn't hurt to copy your explanation as a source code
comment, i.e.:

/* This approach does not work for the POWER10 because the bootstrap
   relocation already uses PCREL instructions, so the detection code does
   not actually run.  */

>> +#ifdef __MMA__
>> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_MMA) == 0)
>> +    _dl_fatal_printf ("\
>> +Fatal glibc error: CPU lacks MMA support (POWER10 or later required)\n");
>> +#endif
>> +}

I'm not sure it's a good idea to require MMA.
__MMA__ is enabled with -mcpu=power10.  However, I'm not aware of any compilers
able to auto-mma code.  In other words, we have to use it explicitly right now.
Which means we shouldn't have MMA instructions in glibc at the moment.
  

* Tulio Magno Quites Machado Filho:

> Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:
>
>> I tried to allude in the commit message that this is expected.  Maybe I
>> should drop the POWER10 check?
>
> I don't think this is necessary.
> Even if you removed the P10 check, I don't think the P9 check is guaranteed to
> work because when _ARCH_PWR9 is set, the compiler is allowed to use P9
> instructions in this function before the test is executed.

Correct, it just emprically works for me.

> I think this patch is trying to improve the error message to the user.

And make sure that there is an error.  We don't want things to crash
later, or after system upgrades using a different compiler version with
more optimizations.

> With that said, it doesn't hurt to copy your explanation as a source code
> comment, i.e.:
>
> /* This approach does not work for the POWER10 because the bootstrap
>    relocation already uses PCREL instructions, so the detection code does
>    not actually run.  */

My point was not so much that the bootstrap relocation uses PCREL
instructions, but we eventually *want* to use it such instructions and
get rid of the bootstrap relocation (like we do for other
PI_STATIC_AND_HIDDEN targets).

>>> +#ifdef __MMA__
>>> +  if ((GLRO (dl_hwcap2) & PPC_FEATURE2_MMA) == 0)
>>> +    _dl_fatal_printf ("\
>>> +Fatal glibc error: CPU lacks MMA support (POWER10 or later required)\n");
>>> +#endif
>>> +}
>
> I'm not sure it's a good idea to require MMA.  __MMA__ is enabled with
> -mcpu=power10.  However, I'm not aware of any compilers able to
> auto-mma code.  In other words, we have to use it explicitly right
> now.  Which means we shouldn't have MMA instructions in glibc at the
> moment.

I think we should require it if it is part of -mcpu=power10.  This check
is not just for glibc code, but for entire distributions using
consistent build flags across everything.  If they don't want MMA for
some reason, they will have to build wuth -mno-mmma.

Thanks,
Florian
  

Florian Weimer <fweimer@redhat.com> writes:

> I think we should require it if it is part of -mcpu=power10.  This check
> is not just for glibc code, but for entire distributions using
> consistent build flags across everything.  If they don't want MMA for
> some reason, they will have to build wuth -mno-mmma.

OK. Good point.
I agree.

Thanks!
  

* Tulio Magno Quites Machado Filho:

> Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:
>
>> I tried to allude in the commit message that this is expected.  Maybe I
>> should drop the POWER10 check?
>
> I don't think this is necessary.
> Even if you removed the P10 check, I don't think the P9 check is guaranteed to
> work because when _ARCH_PWR9 is set, the compiler is allowed to use P9
> instructions in this function before the test is executed.
>
> I think this patch is trying to improve the error message to the user.
>
> With that said, it doesn't hurt to copy your explanation as a source code
> comment, i.e.:
>
> /* This approach does not work for the POWER10 because the bootstrap
>    relocation already uses PCREL instructions, so the detection code does
>    not actually run.  */

What about this instead?

  /* This check is not actually reached when building for POWER10 and
     running on POWER9 because there are faulting PCREL instructions
     before this point.  */

Thanks,
Florian
  

Florian Weimer <fweimer@redhat.com> writes:

> What about this instead?
>
>   /* This check is not actually reached when building for POWER10 and
>      running on POWER9 because there are faulting PCREL instructions
>      before this point.  */

LGTM
  

* Tulio Magno Quites Machado Filho:

> Florian Weimer <fweimer@redhat.com> writes:
>
>> What about this instead?
>>
>>   /* This check is not actually reached when building for POWER10 and
>>      running on POWER9 because there are faulting PCREL instructions
>>      before this point.  */
>
> LGTM

Thanks, then I'll commit the full series tomorrow.

Florian