diff mbox

power: Fix VSCR position on ucontext

Message ID ecf16bc6-c756-1147-4a72-03efbdd64ef4@linux.ibm.com
State Superseded
Delegated to: Tulio Magno Quites Machado Filho
Headers show

Commit Message

Rogerio Alves Dec. 19, 2018, 7:11 p.m. UTC
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

Comments

Tulio Magno Quites Machado Filho Jan. 10, 2019, 7:52 p.m. UTC | #1
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!
Szabolcs Nagy Jan. 11, 2019, 12:01 p.m. UTC | #2
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).
Tulio Magno Quites Machado Filho Jan. 11, 2019, 12:15 p.m. UTC | #3
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 Quites Machado Filho Jan. 11, 2019, 7:52 p.m. UTC | #4
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.
Joseph Myers Jan. 11, 2019, 11:48 p.m. UTC | #5
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
Tulio Magno Quites Machado Filho Jan. 12, 2019, 12:50 a.m. UTC | #6
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.
diff mbox

Patch

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