power: Fix VSCR position on ucontext
Commit Message
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
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!
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).
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 <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.
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 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.
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
@@ -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,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>
@@ -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