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

[Rogerio Alves <rcardoso at linux dot ibm dot com>]

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

>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