Discussion:
[PATCH] power: Fix VSCR position on ucontext
(too old to reply)
Rogerio Alves
2018-11-14 16:14:18 UTC
Permalink
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
Florian Weimer
2018-11-14 18:00:17 UTC
Permalink
+#ifdef _ARCH_PWR8
Is it possible to perform run-time detection instead?

Thanks,
Florian
Gabriel F. T. Gomes
2018-11-16 13:18:25 UTC
Permalink
Post by Florian Weimer
+#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).
Post by Florian Weimer
+ /* 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.
Rogerio Alves
2018-12-07 18:10:06 UTC
Permalink
Hi everyone,

Here is patch v3 with the suggestions given. Sorry for take me that long
to reply. Thanks for the review

Regards
Post by Gabriel F. T. Gomes
Post by Florian Weimer
+#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
Post by Gabriel F. T. Gomes
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).
Post by Florian Weimer
+ /* 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
Gabriel F. T. Gomes
2018-12-07 18:29:59 UTC
Permalink
+/* 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
2018-12-10 17:36:58 UTC
Permalink
Post by Gabriel F. T. Gomes
+/* 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).
Tulio Magno Quites Machado Filho
2018-12-10 18:38:28 UTC
Permalink
Post by Gabriel F. T. Gomes
Post by Gabriel F. T. Gomes
+/* 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
../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
Rogerio Alves
2018-12-10 18:12:41 UTC
Permalink
Post by Gabriel F. T. Gomes
Post by Gabriel F. T. Gomes
+/* 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
../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.
Post by Gabriel F. T. Gomes
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
2018-12-10 18:08:19 UTC
Permalink
[ text/plain ]
+/* 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. :)
Loading...