power: Fix VSCR position on ucontext
Commit Message
Hi everyone,
Here is patch v3 with the suggestions given. Sorry for take me that long
to reply. Thanks for the review
Regards
Em 16-11-2018 11:18, Gabriel F. T. Gomes escreveu:
> On Wed, 14 Nov 2018, Florian Weimer wrote:
>
>> * Rogerio Alves:
>>
>>> +#ifdef _ARCH_PWR8
>>
>> Is it possible to perform run-time detection instead?
Now I am using auxval to get if the arch is supported for the test
>
> That's a fair point. Even though we have bots that test powerpc64
> (big-endian) builds configured with `--with-cpu=power8', that's not the
> default. Runtime detection could make this test work on default builds
> (provided they are run on a POWER8 machine).
>
>> + /* Set SAT bit in VSCR register. */
>> + asm volatile ("vspltisb %0,0\n"
>> + "vspltisb %1,-1\n"
>> + "vpkudus %0,%0,%1\n"
>
> Also, if you replace `vpkudus' (only available since Power ISA 2.07) with
> `vpkuwus' (available since PowerISA 2.03) the test remains valid and it
> could be tested on older machines.
>
Done
Comments
On Fri, 07 Dec 2018, Rogerio Alves wrote:
>+/* This test is supported only on POWER 5 or higher. */
>+#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
>+ PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
>+ PPC_FEATURE2_ARCH_2_07)
Is this actually needed? Glibc has code to fill all the bits for older
architectures in sysdeps/powerpc/hwcapinfo.c [1]. So, as far as I can
see, you only need to test for AT_HWCAP & PPC_FEATURE_POWER5.
[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/hwcapinfo.c;h=a09b18309324423d0cdf04e88367899a4396bab7;hb=HEAD#l47
>+static int
>+do_test (void)
>+{
>+
>+ if (!(getauxval(AT_HWCAP2) & PPC_CPU_SUPPORTED))
>+ {
>+ if (!(getauxval(AT_HWCAP) & PPC_CPU_SUPPORTED))
>+ FAIL_UNSUPPORTED("This test is unsupported on POWER < 5\n");
>+ }
Similarly.
"Gabriel F. T. Gomes" <gabriel@inconstante.eti.br> writes:
> [ text/plain ]
> On Fri, 07 Dec 2018, Rogerio Alves wrote:
>
>>+/* This test is supported only on POWER 5 or higher. */
>>+#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
>>+ PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
>>+ PPC_FEATURE2_ARCH_2_07)
>
> Is this actually needed? Glibc has code to fill all the bits for older
> architectures in sysdeps/powerpc/hwcapinfo.c [1]. So, as far as I can
> see, you only need to test for AT_HWCAP & PPC_FEATURE_POWER5.
Yes, it's indeed required to check all these bits when using getauxval().
> [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/hwcapinfo.c;h=a09b18309324423d0cdf04e88367899a4396bab7;hb=HEAD#l47
This code is preparing the data for __builtin_cpu_supports().
So, when using this built-in, it wouldn't be necessary to check for all those
AT_HWCAP bits.
On Fri, 07 Dec 2018, Gabriel F. T. Gomes wrote:
>On Fri, 07 Dec 2018, Rogerio Alves wrote:
>
>>+/* This test is supported only on POWER 5 or higher. */
>>+#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
>>+ PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
>>+ PPC_FEATURE2_ARCH_2_07)
>
>Is this actually needed? Glibc has code to fill all the bits for older
>architectures in sysdeps/powerpc/hwcapinfo.c [1]. So, as far as I can
>see, you only need to test for AT_HWCAP & PPC_FEATURE_POWER5.
>
>[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/hwcapinfo.c;h=a09b18309324423d0cdf04e88367899a4396bab7;hb=HEAD#l47
Hrm, that's only true for the hwcap info that is copied into the TCB, not
when accessing it with getauxval, so my comment is wrong (as Rogerio
kindly pointed out to me in a private message... thanks).
So, nevermind this comment.
On the other hand, on powerpc64 builds configured with --with-cpu set to
power4, power5, and power6, (but not to power8 or power7), I got the
following error message:
../sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c: In function ‘do_test’:
../sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c:51:3: error: inconsistent operand constraints in an ‘asm’
asm volatile ("vspltisb %0,0\n"
^~~
I guess we didn't get this error message before, because of the
`#ifdef ARCH_PWR8' statement. So, unless you know a better way to fix
this, you should reintroduce the `#ifdef' statement (now, you could use
ARCH_PWR7).
On Mon, 10 Dec 2018, Tulio Magno Quites Machado Filho wrote:
>"Gabriel F. T. Gomes" <gabriel@inconstante.eti.br> writes:
>
>> [ text/plain ]
>> On Fri, 07 Dec 2018, Rogerio Alves wrote:
>>
>>>+/* This test is supported only on POWER 5 or higher. */
>>>+#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
>>>+ PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
>>>+ PPC_FEATURE2_ARCH_2_07)
>>
>> Is this actually needed? Glibc has code to fill all the bits for older
>> architectures in sysdeps/powerpc/hwcapinfo.c [1]. So, as far as I can
>> see, you only need to test for AT_HWCAP & PPC_FEATURE_POWER5.
>
>Yes, it's indeed required to check all these bits when using getauxval().
>
>> [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/hwcapinfo.c;h=a09b18309324423d0cdf04e88367899a4396bab7;hb=HEAD#l47
>
>This code is preparing the data for __builtin_cpu_supports().
>So, when using this built-in, it wouldn't be necessary to check for all those
>AT_HWCAP bits.
Oh, I see. Thanks for the explanation. :)
Em 10-12-2018 15:36, Gabriel F. T. Gomes escreveu:
> On Fri, 07 Dec 2018, Gabriel F. T. Gomes wrote:
>
>> On Fri, 07 Dec 2018, Rogerio Alves wrote:
>>
>>> +/* This test is supported only on POWER 5 or higher. */
>>> +#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
>>> + PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
>>> + PPC_FEATURE2_ARCH_2_07)
>>
>> Is this actually needed? Glibc has code to fill all the bits for older
>> architectures in sysdeps/powerpc/hwcapinfo.c [1]. So, as far as I can
>> see, you only need to test for AT_HWCAP & PPC_FEATURE_POWER5.
>>
>> [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/hwcapinfo.c;h=a09b18309324423d0cdf04e88367899a4396bab7;hb=HEAD#l47
>
> Hrm, that's only true for the hwcap info that is copied into the TCB, not
> when accessing it with getauxval, so my comment is wrong (as Rogerio
> kindly pointed out to me in a private message... thanks).
>
> So, nevermind this comment.
>
>
> On the other hand, on powerpc64 builds configured with --with-cpu set to
> power4, power5, and power6, (but not to power8 or power7), I got the
> following error message:
>
> ../sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c: In function ‘do_test’:
> ../sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c:51:3: error: inconsistent operand constraints in an ‘asm’
> asm volatile ("vspltisb %0,0\n"
> ^~~
>
Oh Ok I'll make some tests here. Thanks.
> I guess we didn't get this error message before, because of the
> `#ifdef ARCH_PWR8' statement. So, unless you know a better way to fix
> this, you should reintroduce the `#ifdef' statement (now, you could use
> ARCH_PWR7).
>
"Gabriel F. T. Gomes" <gabriel@inconstante.eti.br> writes:
> On Fri, 07 Dec 2018, Gabriel F. T. Gomes wrote:
>
>>On Fri, 07 Dec 2018, Rogerio Alves wrote:
>>
>>>+/* This test is supported only on POWER 5 or higher. */
>>>+#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
>>>+ PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
>>>+ PPC_FEATURE2_ARCH_2_07)
>>
>>Is this actually needed? Glibc has code to fill all the bits for older
>>architectures in sysdeps/powerpc/hwcapinfo.c [1]. So, as far as I can
>>see, you only need to test for AT_HWCAP & PPC_FEATURE_POWER5.
>>
>>[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/hwcapinfo.c;h=a09b18309324423d0cdf04e88367899a4396bab7;hb=HEAD#l47
>
> Hrm, that's only true for the hwcap info that is copied into the TCB, not
> when accessing it with getauxval, so my comment is wrong (as Rogerio
> kindly pointed out to me in a private message... thanks).
>
> So, nevermind this comment.
>
>
> On the other hand, on powerpc64 builds configured with --with-cpu set to
> power4, power5, and power6, (but not to power8 or power7), I got the
> following error message:
>
> ../sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c: In function ‘do_test’:
> ../sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c:51:3: error: inconsistent operand constraints in an ‘asm’
> asm volatile ("vspltisb %0,0\n"
> ^~~
>
> I guess we didn't get this error message before, because of the
> `#ifdef ARCH_PWR8' statement. So, unless you know a better way to fix
> this, you should reintroduce the `#ifdef' statement (now, you could use
> ARCH_PWR7).
You can use:
.machine push;
.machine "power7";
...
.machine pop
It should be safe to use with the current minimum required Binutils i.e.
support for POWER7 was already available in GNU Binutils 2.25 (even P8 was
supported).
From f7d3468236f17f0d1c1ca5acf3865bc89ac27125 Mon Sep 17 00:00:00 2001
From: Rogerio Alves <rcardoso@linux.ibm.com>
Date: Mon, 5 Nov 2018 10:18:38 -0600
Subject: [PATCH v3] powerpc: Fix VSCR position in ucontext.
This patch fix VSCR position on ucontext. VSCR was read in the wrong
position on ucontext structure because it was ignoring the machine
endianess.
2018-11-05 Rogerio A. Cardoso <rcardoso@linux.ibm.com>
* sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
(struct _libc_vscr, vscr_t): Added ifdef to fix read of VSCR.
* sysdeps/powerpc/powerpc64/Makefile: Add tst-ucontext-ppc64-vscr.c
to test list.
* sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c: New test file.
---
Patch v3: fixed as per Florian and Gabriel review. Using getauxval to get
if (runtime) the test is supported instead using ifdef. Changed
vpkudus por vpkuwus since it's supported by POWER 5 or higher.
Patch v2: fixed as per Gabriel and Segher review. Fix formating and changelog
Change the assembly inline to a better version. Changed ifdef to POWER8.
sysdeps/powerpc/powerpc64/Makefile | 5 ++
.../powerpc/powerpc64/tst-ucontext-ppc64-vscr.c | 82 ++++++++++++++++++++++
sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h | 5 ++
3 files changed, 92 insertions(+)
create mode 100644 sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
@@ -59,3 +59,8 @@ $(objpfx)tst-setjmp-bug21895-static.out: $(objpfx)setjmp-bug21895.so
tst-setjmp-bug21895-static-ENV = \
LD_LIBRARY_PATH=$(objpfx):$(common-objpfx):$(common-objpfx)setjmp:$(common-objpfx)elf
endif
+
+ifeq ($(subdir),stdlib)
+CFLAGS-tst-ucontext-ppc64-vscr.c += -maltivec
+tests += tst-ucontext-ppc64-vscr
+endif
new file mode 100644
@@ -0,0 +1,82 @@
+/* Test if POWER vscr read by ucontext.
+ Copyright (C) 2018 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 <support/check.h>
+#include <sys/auxv.h>
+#include <ucontext.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <altivec.h>
+
+#define SAT 0x1
+
+/* This test is supported only on POWER 5 or higher. */
+#define PPC_CPU_SUPPORTED (PPC_FEATURE_POWER5 | PPC_FEATURE_POWER5_PLUS | \
+ PPC_FEATURE_ARCH_2_05 | PPC_FEATURE_ARCH_2_06 | \
+ PPC_FEATURE2_ARCH_2_07)
+static int
+do_test (void)
+{
+
+ if (!(getauxval(AT_HWCAP2) & PPC_CPU_SUPPORTED))
+ {
+ if (!(getauxval(AT_HWCAP) & PPC_CPU_SUPPORTED))
+ FAIL_UNSUPPORTED("This test is unsupported on POWER < 5\n");
+ }
+
+ uint32_t vscr[4] __attribute__ ((aligned (16)));
+ uint32_t* vscr_ptr = vscr;
+ uint32_t vscr_word;
+ ucontext_t ucp;
+ __vector __int128_t v0 = {0};
+ __vector __int128_t v1 = {0};
+
+ /* Set SAT bit in VSCR register. */
+ asm volatile ("vspltisb %0,0\n"
+ "vspltisb %1,-1\n"
+ "vpkuwus %0,%0,%1\n"
+ "mfvscr %0\n"
+ "stvx %0,0,%2"
+ : "=v" (v0), "=v" (v1)
+ : "r" (vscr_ptr)
+ : "memory"
+ );
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+ vscr_word = vscr[0];
+#else
+ vscr_word = vscr[3];
+#endif
+
+ if ((vscr_word & SAT) != SAT)
+ {
+ FAIL_EXIT1("FAIL: SAT bit is not set.\n");
+ }
+
+ if (getcontext (&ucp))
+ {
+ FAIL_EXIT1("FAIL: getcontext error\n");
+ }
+ if (ucp.uc_mcontext.v_regs->vscr.vscr_word != vscr_word)
+ {
+ FAIL_EXIT1("FAIL: ucontext vscr does not match with vscr\n");
+ }
+ return 0;
+}
+
+#include <support/test-driver.c>
@@ -98,8 +98,13 @@ typedef double fpregset_t[__NFPREG];
a whole quadword speedup save/restore. */
typedef struct _libc_vscr
{
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
unsigned int __pad[3];
unsigned int __ctx(vscr_word);
+#else
+ unsigned int __ctx(vscr_word);
+ unsigned int __pad[3];
+#endif
} vscr_t;
/* Container for Altivec/VMX registers and status.
--
2.7.4