[PATCH] power: Fix VSCR position on ucontext

[PATCH] power: Fix VSCR position on ucontext

[Rogerio Alves]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

Hi everyone,

I've working on a patch to fix the read of VSCR from ucontext in Power. 
It was reading it from the wrong position on little endian machines 
(ppc64le).

I'll appreciate a review on that patch

Regards



[-- Attachment #2: 0001-powerpc-Fix-VSCR-position-in-ucontext.patch --]
[-- Type: text/x-patch, Size: 4751 bytes --]

From d52c4d0839ee77c0d8a2a0522ab4cbe0ce77587a 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 v2] 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 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    | 84 ++++++++++++++++++++++
 sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h     |  5 ++
 3 files changed, 94 insertions(+)
 create mode 100644 sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c

diff --git a/sysdeps/powerpc/powerpc64/Makefile b/sysdeps/powerpc/powerpc64/Makefile
index a0bd0c9..6e88df1 100644
--- a/sysdeps/powerpc/powerpc64/Makefile
+++ b/sysdeps/powerpc/powerpc64/Makefile
@@ -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
diff --git a/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c b/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
new file mode 100644
index 0000000..3aee460
--- /dev/null
+++ b/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
@@ -0,0 +1,84 @@
+/* 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>
+
+#ifdef _ARCH_PWR8
+
+#include <ucontext.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <altivec.h>
+
+#define SAT 0x1
+
+static int
+do_test (void)
+{
+  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"
+                "vpkudus %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;
+}
+
+#else
+
+static int
+do_test (void)
+{
+  return FAIL_UNSUPPORTED("This test is unsupported on POWER < 8");
+}
+
+#endif /* _ARCH_PWR8_.  */
+
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h b/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
index 1bb6e4c..49a92c5 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
+++ b/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
@@ -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

Re: [PATCH] power: Fix VSCR position on ucontext

[Florian Weimer]

* Rogerio Alves:

> +#ifdef _ARCH_PWR8

Is it possible to perform run-time detection instead?

Thanks,
Florian

Re: [PATCH] power: Fix VSCR position on ucontext

[Gabriel F. T. Gomes]

On Wed, 14 Nov 2018, Florian Weimer wrote:

>* Rogerio Alves:
>
>> +#ifdef _ARCH_PWR8  
>
>Is it possible to perform run-time detection instead?

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.

Re: [PATCH] power: Fix VSCR position on ucontext

[Rogerio Alves]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

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

[-- Attachment #2: 0001-powerpc-Fix-VSCR-position-in-ucontext.patch --]
[-- Type: text/x-patch, Size: 5269 bytes --]

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

diff --git a/sysdeps/powerpc/powerpc64/Makefile b/sysdeps/powerpc/powerpc64/Makefile
index a0bd0c9..6e88df1 100644
--- a/sysdeps/powerpc/powerpc64/Makefile
+++ b/sysdeps/powerpc/powerpc64/Makefile
@@ -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
diff --git a/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c b/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
new file mode 100644
index 0000000..be99108
--- /dev/null
+++ b/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
@@ -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>
diff --git a/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h b/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
index 1bb6e4c..49a92c5 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
+++ b/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
@@ -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

Re: [PATCH] power: Fix VSCR position on ucontext

[Gabriel F. T. Gomes]

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.

Re: [PATCH] power: Fix VSCR position on ucontext

[Tulio Magno Quites Machado Filho]

"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.

-- 
Tulio Magno

Re: [PATCH] power: Fix VSCR position on ucontext

[Gabriel F. T. Gomes]

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).

Re: [PATCH] power: Fix VSCR position on ucontext

[Gabriel F. T. Gomes]

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.  :)

Re: [PATCH] power: Fix VSCR position on ucontext

[Rogerio Alves]

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).
> 

Re: [PATCH] power: Fix VSCR position on ucontext

[Tulio Magno Quites Machado Filho]

"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).

-- 
Tulio Magno

Re: [PATCH] power: Fix VSCR position on ucontext

[Rogerio Alves]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

I am sending patch v4 attached in this email with the fixes proposed 
here. Thank you all for the review on this patch.

Regards

Em 10-12-2018 16:38, Tulio Magno Quites Machado Filho escreveu:
> "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).
> 

Done. Using your suggestion fixed the issue thanks

[-- Attachment #2: 0001-powerpc-Fix-VSCR-position-in-ucontext.patch --]
[-- Type: text/x-patch, Size: 5483 bytes --]

From 2fe5a2b78f6818000ef59084098b6d17d8becd0a 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 v4] 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 v4: fixed as per Tulio review. Using machine push/pop to generate
 power7 instructions.

 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    | 85 ++++++++++++++++++++++
 sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h     |  5 ++
 3 files changed, 95 insertions(+)
 create mode 100644 sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c

diff --git a/sysdeps/powerpc/powerpc64/Makefile b/sysdeps/powerpc/powerpc64/Makefile
index a0bd0c9..6e88df1 100644
--- a/sysdeps/powerpc/powerpc64/Makefile
+++ b/sysdeps/powerpc/powerpc64/Makefile
@@ -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
diff --git a/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c b/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
new file mode 100644
index 0000000..02c06cb
--- /dev/null
+++ b/sysdeps/powerpc/powerpc64/tst-ucontext-ppc64-vscr.c
@@ -0,0 +1,85 @@
+/* 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 (".machine push;\n"
+                ".machine \"power7\";\n"
+                "vspltisb %0,0;\n"
+                "vspltisb %1,-1;\n"
+                "vpkuwus %0,%0,%1;\n"
+                "mfvscr %0;\n"
+                "stvx %0,0,%2;\n"
+                ".machine pop;"
+       : "=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>
diff --git a/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h b/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
index 1bb6e4c..49a92c5 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
+++ b/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h
@@ -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

Re: [PATCH] power: Fix VSCR position on ucontext

[Tulio Magno Quites Machado Filho]

Rogerio Alves <rcardoso@linux.ibm.com> writes:

> Subject: [PATCH v4] 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.


> /* 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 | \

The GNU Coding Standard requires to replace 8 spaces with a tab.
Fixed locally.   ;-)

> +		".machine \"power7\";\n"

s/power7/power5/
Fixed.

The patch looks good to me except for one thing: this is fixing a bug visible
to the user, so we also need a bug report for this.

Could you open the bug and send me the number?  I can incorporate it here
before pushing the patch.

Thanks!

-- 
Tulio Magno

Re: [PATCH] power: Fix VSCR position on ucontext

[Szabolcs Nagy]

On 10/01/2019 19:52, Tulio Magno Quites Machado Filho 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 | \
> 
> The GNU Coding Standard requires to replace 8 spaces with a tab.
> Fixed locally.   ;-)

there was a discussion recently, that actually it does not
require such thing (it's just an unfortunate default in
emacs which makes the code unreadable in any way other than
8 space == 1 tab, also breaks patch review and email replies,
but we have to format this way for the sake of emacs users
who otherwise would mess up white space formatting).

Re: [PATCH] power: Fix VSCR position on ucontext

[Tulio Magno Quites Machado Filho]

Szabolcs Nagy <Szabolcs.Nagy@arm.com> writes:

> On 10/01/2019 19:52, Tulio Magno Quites Machado Filho 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 | \
>> 
>> The GNU Coding Standard requires to replace 8 spaces with a tab.
>> Fixed locally.   ;-)
>
> there was a discussion recently, that actually it does not
> require such thing (it's just an unfortunate default in
> emacs which makes the code unreadable in any way other than
> 8 space == 1 tab, also breaks patch review and email replies,
> but we have to format this way for the sake of emacs users
> who otherwise would mess up white space formatting).

Ack.

Thanks!

-- 
Tulio Magno

Re: [PATCH] power: Fix VSCR position on ucontext

[Tulio Magno Quites Machado Filho]

Tulio Magno Quites Machado Filho <tuliom@ascii.art.br> writes:

> The patch looks good to me except for one thing: this is fixing a bug visible
> to the user, so we also need a bug report for this.
>
> Could you open the bug and send me the number?  I can incorporate it here
> before pushing the patch.

Rogerio reported this as bug #24088.

I added the reference to the commit message and ChangeLog and pushed it as
0bc9bdf159f43c50ec31e9a1670e2e04b5d4060d.

-- 
Tulio Magno

Re: [PATCH] power: Fix VSCR position on ucontext

[Joseph Myers]

I'm seeing the test fail to build for powerpc64-linux-gnu (big-endian) 
with my build-many-glibcs.py bots.  (That's at least with GCC 7 and GCC 8; 
my GCC mainline bot hasn't yet run after that commit.)

../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 (".machine push;\n"
   ^~~
/scratch/jmyers/glibc-bot/build/glibcs/powerpc64-linux-gnu/glibc/sysd-rules:933: recipe for target '/scratch/jmyers/glibc-bot/build/glibcs/powerpc64-linux-gnu/glibc/stdlib/tst-ucontext-ppc64-vscr.o' failed

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] power: Fix VSCR position on ucontext

[Tulio Magno Quites Machado Filho]

Joseph Myers <joseph@codesourcery.com> writes:

> I'm seeing the test fail to build for powerpc64-linux-gnu (big-endian) 
> with my build-many-glibcs.py bots.  (That's at least with GCC 7 and GCC 8; 
> my GCC mainline bot hasn't yet run after that commit.)
>
> ../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 (".machine push;\n"
>    ^~~
> /scratch/jmyers/glibc-bot/build/glibcs/powerpc64-linux-gnu/glibc/sysd-rules:933: recipe for target '/scratch/jmyers/glibc-bot/build/glibcs/powerpc64-linux-gnu/glibc/stdlib/tst-ucontext-ppc64-vscr.o' failed

I can also reproduce this with --with-cpu=power5, --with-cpu=power6 and without
--with-cpu.

-- 
Tulio Magno