| Message ID | 20230804233748.218935-4-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/s390x: Fix the "ignored match" case in VSTRS | expand |
On 8/5/23 01:03, Ilya Leoshkevich wrote: > Add a small test to prevent regressions. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tests/tcg/s390x/Makefile.target | 1 + > tests/tcg/s390x/vxeh2_vstrs.c | 88 +++++++++++++++++++++++++++++++++ > 2 files changed, 89 insertions(+) > create mode 100644 tests/tcg/s390x/vxeh2_vstrs.c > > diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target > index 1fc98099070..8ba36e5985b 100644 > --- a/tests/tcg/s390x/Makefile.target > +++ b/tests/tcg/s390x/Makefile.target > @@ -73,6 +73,7 @@ ifneq ($(CROSS_CC_HAS_Z15),) > Z15_TESTS=vxeh2_vs > Z15_TESTS+=vxeh2_vcvt > Z15_TESTS+=vxeh2_vlstr > +Z15_TESTS+=vxeh2_vstrs > $(Z15_TESTS): CFLAGS+=-march=z15 -O2 > TESTS+=$(Z15_TESTS) > endif > diff --git a/tests/tcg/s390x/vxeh2_vstrs.c b/tests/tcg/s390x/vxeh2_vstrs.c > new file mode 100644 > index 00000000000..313ec1d728f > --- /dev/null > +++ b/tests/tcg/s390x/vxeh2_vstrs.c > @@ -0,0 +1,88 @@ > +/* > + * Test the VSTRS instruction. > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > +#include <assert.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include "vx.h" > + > +static inline __attribute__((__always_inline__)) int > +vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3, > + const S390Vector *v4, const uint8_t m5, const uint8_t m6) > +{ > + int cc; > + > + asm("vstrs %[v1],%[v2],%[v3],%[v4],%[m5],%[m6]\n" > + "ipm %[cc]" > + : [v1] "=v" (v1->v) > + , [cc] "=r" (cc) > + : [v2] "v" (v2->v) > + , [v3] "v" (v3->v) > + , [v4] "v" (v4->v) > + , [m5] "i" (m5) > + , [m6] "i" (m6) > + : "cc"); > + > + return (cc >> 28) & 3; Following the POp, I am puzzled by the use of an "int" to contain the register result of the IPM instruction, should it not be a 64bit variable? bits 0-31 are left unchanged / are uninteresting, is that enough to avoid having to use a properly sized variable? I see that this is done elsewhere in the tests (sometimes a 64bit variable is used, sometimes just 'int'), so I assume it's probably fine. Otherwise lgtm, Claudio > +} > + > +static void test_ignored_match(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0x222000205e410000ULL, .d[1] = 0}; > + S390Vector v3 = {.d[0] = 0x205e410000000000ULL, .d[1] = 0}; > + S390Vector v4 = {.d[0] = 3, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 2) == 1); > + assert(v1.d[0] == 16); > + assert(v1.d[1] == 0); > +} > + > +static void test_empty_needle(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0x5300000000000000ULL, .d[1] = 0}; > + S390Vector v3 = {.d[0] = 0, .d[1] = 0}; > + S390Vector v4 = {.d[0] = 0, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 0) == 2); > + assert(v1.d[0] == 0); > + assert(v1.d[1] == 0); > +} > + > +static void test_max_length(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0x1122334455667700ULL, .d[1] = 0}; > + S390Vector v3 = {.d[0] = 0, .d[1] = 0}; > + S390Vector v4 = {.d[0] = 16, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 0) == 3); > + assert(v1.d[0] == 7); > + assert(v1.d[1] == 0); > +} > + > +static void test_no_match(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0xffffff000fffff00ULL, .d[1] = 0x82b}; > + S390Vector v3 = {.d[0] = 0xfffffffeffffffffULL, > + .d[1] = 0xffffffff00000000ULL}; > + S390Vector v4 = {.d[0] = 11, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 2) == 1); > + assert(v1.d[0] == 16); > + assert(v1.d[1] == 0); > +} > + > +int main(void) > +{ > + test_ignored_match(); > + test_empty_needle(); > + test_max_length(); > + test_no_match(); > + return EXIT_SUCCESS; > +}
On Sun, 2023-08-06 at 13:05 +0200, Claudio Fontana wrote: > On 8/5/23 01:03, Ilya Leoshkevich wrote: > > Add a small test to prevent regressions. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > tests/tcg/s390x/Makefile.target | 1 + > > tests/tcg/s390x/vxeh2_vstrs.c | 88 > > +++++++++++++++++++++++++++++++++ > > 2 files changed, 89 insertions(+) > > create mode 100644 tests/tcg/s390x/vxeh2_vstrs.c > > > > diff --git a/tests/tcg/s390x/Makefile.target > > b/tests/tcg/s390x/Makefile.target > > index 1fc98099070..8ba36e5985b 100644 > > --- a/tests/tcg/s390x/Makefile.target > > +++ b/tests/tcg/s390x/Makefile.target > > @@ -73,6 +73,7 @@ ifneq ($(CROSS_CC_HAS_Z15),) > > Z15_TESTS=vxeh2_vs > > Z15_TESTS+=vxeh2_vcvt > > Z15_TESTS+=vxeh2_vlstr > > +Z15_TESTS+=vxeh2_vstrs > > $(Z15_TESTS): CFLAGS+=-march=z15 -O2 > > TESTS+=$(Z15_TESTS) > > endif > > diff --git a/tests/tcg/s390x/vxeh2_vstrs.c > > b/tests/tcg/s390x/vxeh2_vstrs.c > > new file mode 100644 > > index 00000000000..313ec1d728f > > --- /dev/null > > +++ b/tests/tcg/s390x/vxeh2_vstrs.c > > @@ -0,0 +1,88 @@ > > +/* > > + * Test the VSTRS instruction. > > + * > > + * SPDX-License-Identifier: GPL-2.0-or-later > > + */ > > +#include <assert.h> > > +#include <stdint.h> > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include "vx.h" > > + > > +static inline __attribute__((__always_inline__)) int > > +vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3, > > + const S390Vector *v4, const uint8_t m5, const uint8_t m6) > > +{ > > + int cc; > > + > > + asm("vstrs %[v1],%[v2],%[v3],%[v4],%[m5],%[m6]\n" > > + "ipm %[cc]" > > + : [v1] "=v" (v1->v) > > + , [cc] "=r" (cc) > > + : [v2] "v" (v2->v) > > + , [v3] "v" (v3->v) > > + , [v4] "v" (v4->v) > > + , [m5] "i" (m5) > > + , [m6] "i" (m6) > > + : "cc"); > > + > > + return (cc >> 28) & 3; > Following the POp, I am puzzled by the use of an "int" to contain the > register result of the IPM instruction, should it not be a 64bit > variable? > bits 0-31 are left unchanged / are uninteresting, is that enough to > avoid having to use a properly sized variable? The compiler understands that if the type is int, then the asm block will not touch the upper 32 bits of the register that's assigned to it. This observation is used not only in the QEMU tests, but also all over arch/s390 in the Linux kernel. > > I see that this is done elsewhere in the tests (sometimes a 64bit > variable is used, sometimes just 'int'), so I assume it's probably > fine. > > Otherwise lgtm, > > Claudio [...] >
On 8/7/23 10:08, Ilya Leoshkevich wrote: > On Sun, 2023-08-06 at 13:05 +0200, Claudio Fontana wrote: >> On 8/5/23 01:03, Ilya Leoshkevich wrote: >>> Add a small test to prevent regressions. >>> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> >>> --- >>> tests/tcg/s390x/Makefile.target | 1 + >>> tests/tcg/s390x/vxeh2_vstrs.c | 88 >>> +++++++++++++++++++++++++++++++++ >>> 2 files changed, 89 insertions(+) >>> create mode 100644 tests/tcg/s390x/vxeh2_vstrs.c >>> >>> diff --git a/tests/tcg/s390x/Makefile.target >>> b/tests/tcg/s390x/Makefile.target >>> index 1fc98099070..8ba36e5985b 100644 >>> --- a/tests/tcg/s390x/Makefile.target >>> +++ b/tests/tcg/s390x/Makefile.target >>> @@ -73,6 +73,7 @@ ifneq ($(CROSS_CC_HAS_Z15),) >>> Z15_TESTS=vxeh2_vs >>> Z15_TESTS+=vxeh2_vcvt >>> Z15_TESTS+=vxeh2_vlstr >>> +Z15_TESTS+=vxeh2_vstrs >>> $(Z15_TESTS): CFLAGS+=-march=z15 -O2 >>> TESTS+=$(Z15_TESTS) >>> endif >>> diff --git a/tests/tcg/s390x/vxeh2_vstrs.c >>> b/tests/tcg/s390x/vxeh2_vstrs.c >>> new file mode 100644 >>> index 00000000000..313ec1d728f >>> --- /dev/null >>> +++ b/tests/tcg/s390x/vxeh2_vstrs.c >>> @@ -0,0 +1,88 @@ >>> +/* >>> + * Test the VSTRS instruction. >>> + * >>> + * SPDX-License-Identifier: GPL-2.0-or-later >>> + */ >>> +#include <assert.h> >>> +#include <stdint.h> >>> +#include <stdio.h> >>> +#include <stdlib.h> >>> +#include "vx.h" >>> + >>> +static inline __attribute__((__always_inline__)) int >>> +vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3, >>> + const S390Vector *v4, const uint8_t m5, const uint8_t m6) >>> +{ >>> + int cc; >>> + >>> + asm("vstrs %[v1],%[v2],%[v3],%[v4],%[m5],%[m6]\n" >>> + "ipm %[cc]" >>> + : [v1] "=v" (v1->v) >>> + , [cc] "=r" (cc) >>> + : [v2] "v" (v2->v) >>> + , [v3] "v" (v3->v) >>> + , [v4] "v" (v4->v) >>> + , [m5] "i" (m5) >>> + , [m6] "i" (m6) >>> + : "cc"); >>> + >>> + return (cc >> 28) & 3; >> Following the POp, I am puzzled by the use of an "int" to contain the >> register result of the IPM instruction, should it not be a 64bit >> variable? >> bits 0-31 are left unchanged / are uninteresting, is that enough to >> avoid having to use a properly sized variable? > > The compiler understands that if the type is int, then the asm block > will not touch the upper 32 bits of the register that's assigned to it. > This observation is used not only in the QEMU tests, but also all over > arch/s390 in the Linux kernel. Thank you, Claudio >> >> I see that this is done elsewhere in the tests (sometimes a 64bit >> variable is used, sometimes just 'int'), so I assume it's probably >> fine. >> >> Otherwise lgtm, >> >> Claudio > > [...] >> >
On 8/5/23 01:03, Ilya Leoshkevich wrote: > Add a small test to prevent regressions. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> Something seems off in the wiring of the make check target? I built with: ./configure --target-list=s390x-linux-user,s390x-softmmu make -j make -j check-help ... Individual test suites: make check-qtest-TARGET Run qtest tests for given target make check-qtest Run qtest tests make check-unit Run qobject tests make check-qapi-schema Run QAPI schema tests make check-block Run block tests make check-tcg Run TCG tests ... make -j check-tcg changing dir to build for make "check-tcg"... make[1]: Entering directory '/root/git/qemu/build' make[1]: Nothing to be done for 'check-tcg'. make[1]: Leaving directory '/root/git/qemu/build' Why is this not running any tests for tcg? I tried also to run the general make check, but even in this case the tcg tests do not seem to trigger. Thanks, Claudio > --- > tests/tcg/s390x/Makefile.target | 1 + > tests/tcg/s390x/vxeh2_vstrs.c | 88 +++++++++++++++++++++++++++++++++ > 2 files changed, 89 insertions(+) > create mode 100644 tests/tcg/s390x/vxeh2_vstrs.c > > diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target > index 1fc98099070..8ba36e5985b 100644 > --- a/tests/tcg/s390x/Makefile.target > +++ b/tests/tcg/s390x/Makefile.target > @@ -73,6 +73,7 @@ ifneq ($(CROSS_CC_HAS_Z15),) > Z15_TESTS=vxeh2_vs > Z15_TESTS+=vxeh2_vcvt > Z15_TESTS+=vxeh2_vlstr > +Z15_TESTS+=vxeh2_vstrs > $(Z15_TESTS): CFLAGS+=-march=z15 -O2 > TESTS+=$(Z15_TESTS) > endif > diff --git a/tests/tcg/s390x/vxeh2_vstrs.c b/tests/tcg/s390x/vxeh2_vstrs.c > new file mode 100644 > index 00000000000..313ec1d728f > --- /dev/null > +++ b/tests/tcg/s390x/vxeh2_vstrs.c > @@ -0,0 +1,88 @@ > +/* > + * Test the VSTRS instruction. > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > +#include <assert.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include "vx.h" > + > +static inline __attribute__((__always_inline__)) int > +vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3, > + const S390Vector *v4, const uint8_t m5, const uint8_t m6) > +{ > + int cc; > + > + asm("vstrs %[v1],%[v2],%[v3],%[v4],%[m5],%[m6]\n" > + "ipm %[cc]" > + : [v1] "=v" (v1->v) > + , [cc] "=r" (cc) > + : [v2] "v" (v2->v) > + , [v3] "v" (v3->v) > + , [v4] "v" (v4->v) > + , [m5] "i" (m5) > + , [m6] "i" (m6) > + : "cc"); > + > + return (cc >> 28) & 3; > +} > + > +static void test_ignored_match(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0x222000205e410000ULL, .d[1] = 0}; > + S390Vector v3 = {.d[0] = 0x205e410000000000ULL, .d[1] = 0}; > + S390Vector v4 = {.d[0] = 3, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 2) == 1); > + assert(v1.d[0] == 16); > + assert(v1.d[1] == 0); > +} > + > +static void test_empty_needle(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0x5300000000000000ULL, .d[1] = 0}; > + S390Vector v3 = {.d[0] = 0, .d[1] = 0}; > + S390Vector v4 = {.d[0] = 0, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 0) == 2); > + assert(v1.d[0] == 0); > + assert(v1.d[1] == 0); > +} > + > +static void test_max_length(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0x1122334455667700ULL, .d[1] = 0}; > + S390Vector v3 = {.d[0] = 0, .d[1] = 0}; > + S390Vector v4 = {.d[0] = 16, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 0) == 3); > + assert(v1.d[0] == 7); > + assert(v1.d[1] == 0); > +} > + > +static void test_no_match(void) > +{ > + S390Vector v1; > + S390Vector v2 = {.d[0] = 0xffffff000fffff00ULL, .d[1] = 0x82b}; > + S390Vector v3 = {.d[0] = 0xfffffffeffffffffULL, > + .d[1] = 0xffffffff00000000ULL}; > + S390Vector v4 = {.d[0] = 11, .d[1] = 0}; > + > + assert(vstrs(&v1, &v2, &v3, &v4, 0, 2) == 1); > + assert(v1.d[0] == 16); > + assert(v1.d[1] == 0); > +} > + > +int main(void) > +{ > + test_ignored_match(); > + test_empty_needle(); > + test_max_length(); > + test_no_match(); > + return EXIT_SUCCESS; > +}
On Thu, Aug 17, 2023 at 11:37:29AM +0200, Claudio Fontana wrote: > On 8/5/23 01:03, Ilya Leoshkevich wrote: > > Add a small test to prevent regressions. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > Something seems off in the wiring of the make check target? > > I built with: > > ./configure --target-list=s390x-linux-user,s390x-softmmu > > make -j > make -j check-help > > ... > > Individual test suites: > make check-qtest-TARGET Run qtest tests for given target > make check-qtest Run qtest tests > make check-unit Run qobject tests > make check-qapi-schema Run QAPI schema tests > make check-block Run block tests > make check-tcg Run TCG tests > > > ... > > make -j check-tcg > > changing dir to build for make "check-tcg"... > make[1]: Entering directory '/root/git/qemu/build' > make[1]: Nothing to be done for 'check-tcg'. > make[1]: Leaving directory '/root/git/qemu/build' > > > Why is this not running any tests for tcg? > > I tried also to run the general make check, > but even in this case the tcg tests do not seem to trigger. > > Thanks, > > Claudio Hi, I believe you need either s390x-linux-gnu-gcc or docker/podman to run the tcg tests. Best regards, Ilya
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target index 1fc98099070..8ba36e5985b 100644 --- a/tests/tcg/s390x/Makefile.target +++ b/tests/tcg/s390x/Makefile.target @@ -73,6 +73,7 @@ ifneq ($(CROSS_CC_HAS_Z15),) Z15_TESTS=vxeh2_vs Z15_TESTS+=vxeh2_vcvt Z15_TESTS+=vxeh2_vlstr +Z15_TESTS+=vxeh2_vstrs $(Z15_TESTS): CFLAGS+=-march=z15 -O2 TESTS+=$(Z15_TESTS) endif diff --git a/tests/tcg/s390x/vxeh2_vstrs.c b/tests/tcg/s390x/vxeh2_vstrs.c new file mode 100644 index 00000000000..313ec1d728f --- /dev/null +++ b/tests/tcg/s390x/vxeh2_vstrs.c @@ -0,0 +1,88 @@ +/* + * Test the VSTRS instruction. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ +#include <assert.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include "vx.h" + +static inline __attribute__((__always_inline__)) int +vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3, + const S390Vector *v4, const uint8_t m5, const uint8_t m6) +{ + int cc; + + asm("vstrs %[v1],%[v2],%[v3],%[v4],%[m5],%[m6]\n" + "ipm %[cc]" + : [v1] "=v" (v1->v) + , [cc] "=r" (cc) + : [v2] "v" (v2->v) + , [v3] "v" (v3->v) + , [v4] "v" (v4->v) + , [m5] "i" (m5) + , [m6] "i" (m6) + : "cc"); + + return (cc >> 28) & 3; +} + +static void test_ignored_match(void) +{ + S390Vector v1; + S390Vector v2 = {.d[0] = 0x222000205e410000ULL, .d[1] = 0}; + S390Vector v3 = {.d[0] = 0x205e410000000000ULL, .d[1] = 0}; + S390Vector v4 = {.d[0] = 3, .d[1] = 0}; + + assert(vstrs(&v1, &v2, &v3, &v4, 0, 2) == 1); + assert(v1.d[0] == 16); + assert(v1.d[1] == 0); +} + +static void test_empty_needle(void) +{ + S390Vector v1; + S390Vector v2 = {.d[0] = 0x5300000000000000ULL, .d[1] = 0}; + S390Vector v3 = {.d[0] = 0, .d[1] = 0}; + S390Vector v4 = {.d[0] = 0, .d[1] = 0}; + + assert(vstrs(&v1, &v2, &v3, &v4, 0, 0) == 2); + assert(v1.d[0] == 0); + assert(v1.d[1] == 0); +} + +static void test_max_length(void) +{ + S390Vector v1; + S390Vector v2 = {.d[0] = 0x1122334455667700ULL, .d[1] = 0}; + S390Vector v3 = {.d[0] = 0, .d[1] = 0}; + S390Vector v4 = {.d[0] = 16, .d[1] = 0}; + + assert(vstrs(&v1, &v2, &v3, &v4, 0, 0) == 3); + assert(v1.d[0] == 7); + assert(v1.d[1] == 0); +} + +static void test_no_match(void) +{ + S390Vector v1; + S390Vector v2 = {.d[0] = 0xffffff000fffff00ULL, .d[1] = 0x82b}; + S390Vector v3 = {.d[0] = 0xfffffffeffffffffULL, + .d[1] = 0xffffffff00000000ULL}; + S390Vector v4 = {.d[0] = 11, .d[1] = 0}; + + assert(vstrs(&v1, &v2, &v3, &v4, 0, 2) == 1); + assert(v1.d[0] == 16); + assert(v1.d[1] == 0); +} + +int main(void) +{ + test_ignored_match(); + test_empty_needle(); + test_max_length(); + test_no_match(); + return EXIT_SUCCESS; +}
Add a small test to prevent regressions. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tests/tcg/s390x/Makefile.target | 1 + tests/tcg/s390x/vxeh2_vstrs.c | 88 +++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 tests/tcg/s390x/vxeh2_vstrs.c