1. Patchew
  2. QEMU
  3. View series

[PATCH] target/s390x: Fix emulation of the VISTR instruction

Thomas Huth posted 1 patch 3 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221011101401.81849-1-thuth@redhat.com
Maintainers: Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>
tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
target/s390x/tcg/translate_vx.c.inc |  2 +-
tests/tcg/s390x/Makefile.target     |  6 ++++
3 files changed, 57 insertions(+), 1 deletion(-)
create mode 100644 tests/tcg/s390x/vf.c
[PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by Thomas Huth 3 years, 3 months ago 
The element size is encoded in the M3 field, not in the M4
field. Let's also add a TCG test that shows the failing
behavior without this fix.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1248
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
 target/s390x/tcg/translate_vx.c.inc |  2 +-
 tests/tcg/s390x/Makefile.target     |  6 ++++
 3 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/s390x/vf.c

diff --git a/tests/tcg/s390x/vf.c b/tests/tcg/s390x/vf.c
new file mode 100644
index 0000000000..fdc424ce7c
--- /dev/null
+++ b/tests/tcg/s390x/vf.c
@@ -0,0 +1,50 @@
+/*
+ * vf: vector facility tests
+ */
+#include <stdint.h>
+#include <stdio.h>
+#include "vx.h"
+
+static inline void vistr(S390Vector *v1, S390Vector *v2,
+                         const uint8_t m3, const uint8_t m5)
+{
+    asm volatile("vistr %[v1], %[v2], %[m3], %[m5]\n"
+                : [v1] "=v" (v1->v)
+                : [v2]  "v" (v2->v)
+                , [m3]  "i" (m3)
+                , [m5]  "i" (m5)
+                : "cc");
+}
+
+static int test_vistr(void)
+{
+    S390Vector vd = {};
+    S390Vector vs16 = {
+        .h[0] = 0x1234, .h[1] = 0x0056, .h[2] = 0x7800, .h[3] = 0x0000,
+        .h[4] = 0x0078, .h[5] = 0x0000, .h[6] = 0x6543, .h[7] = 0x2100
+    };
+    S390Vector vs32 = {
+        .w[0] = 0x12340000, .w[1] = 0x78654300,
+        .w[2] = 0x0, .w[3] = 0x12,
+    };
+
+    vistr(&vd, &vs16, 1, 0);
+    if (vd.h[0] != 0x1234 || vd.h[1] != 0x0056 || vd.h[2] != 0x7800 ||
+        vd.h[3] || vd.h[4] || vd.h[5] || vd.h[6] || vd.h[7]) {
+        puts("ERROR: vitrh failed!");
+        return 1;
+    }
+
+    vistr(&vd, &vs32, 2, 0);
+    if (vd.w[0] != 0x12340000 || vd.w[1] != 0x78654300 || vd.w[2] || vd.w[3]) {
+        puts("ERROR: vitrf failed!");
+        return 1;
+    }
+
+    return 0;
+}
+
+int main(int argc, char *argv[])
+{
+    return test_vistr();
+}
diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc
index 3526ba3e3b..b69c1a111c 100644
--- a/target/s390x/tcg/translate_vx.c.inc
+++ b/target/s390x/tcg/translate_vx.c.inc
@@ -2723,7 +2723,7 @@ static DisasJumpType op_vfene(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_vistr(DisasContext *s, DisasOps *o)
 {
-    const uint8_t es = get_field(s, m4);
+    const uint8_t es = get_field(s, m3);
     const uint8_t m5 = get_field(s, m5);
     static gen_helper_gvec_2 * const g[3] = {
         gen_helper_gvec_vistr8,
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index c830313e67..f8e71a9439 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -18,6 +18,12 @@ TESTS+=signals-s390x
 TESTS+=branch-relative-long
 TESTS+=noexec
 
+Z13_TESTS=vf
+vf: LDFLAGS+=-lm
+$(Z13_TESTS): CFLAGS+=-march=z13 -O2
+TESTS+=$(if $(shell $(CC) -march=z13 -S -o /dev/null -xc /dev/null \
+			 >/dev/null 2>&1 && echo OK),$(Z13_TESTS))
+
 Z14_TESTS=vfminmax
 vfminmax: LDFLAGS+=-lm
 $(Z14_TESTS): CFLAGS+=-march=z14 -O2
-- 
2.31.1
Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by Richard Henderson 3 years, 3 months ago 
On 10/11/22 03:14, Thomas Huth wrote:
> The element size is encoded in the M3 field, not in the M4
> field. Let's also add a TCG test that shows the failing
> behavior without this fix.
> 
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1248
> Signed-off-by: Thomas Huth<thuth@redhat.com>
> ---
>   tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
>   target/s390x/tcg/translate_vx.c.inc |  2 +-
>   tests/tcg/s390x/Makefile.target     |  6 ++++
>   3 files changed, 57 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/s390x/vf.c

As far as the translate and contents of the test go:

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by Alex Bennée 3 years, 3 months ago 
Thomas Huth <thuth@redhat.com> writes:

> The element size is encoded in the M3 field, not in the M4
> field. Let's also add a TCG test that shows the failing
> behavior without this fix.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1248
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
>  target/s390x/tcg/translate_vx.c.inc |  2 +-
>  tests/tcg/s390x/Makefile.target     |  6 ++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/s390x/vf.c
>
> diff --git a/tests/tcg/s390x/vf.c b/tests/tcg/s390x/vf.c
> new file mode 100644
> index 0000000000..fdc424ce7c
> --- /dev/null
> +++ b/tests/tcg/s390x/vf.c
> @@ -0,0 +1,50 @@
> +/*
> + * vf: vector facility tests
> + */
> +#include <stdint.h>
> +#include <stdio.h>
> +#include "vx.h"
> +
> +static inline void vistr(S390Vector *v1, S390Vector *v2,
> +                         const uint8_t m3, const uint8_t m5)
> +{
> +    asm volatile("vistr %[v1], %[v2], %[m3], %[m5]\n"
> +                : [v1] "=v" (v1->v)
> +                : [v2]  "v" (v2->v)
> +                , [m3]  "i" (m3)
> +                , [m5]  "i" (m5)
> +                : "cc");
> +}
> +
> +static int test_vistr(void)
> +{
> +    S390Vector vd = {};
> +    S390Vector vs16 = {
> +        .h[0] = 0x1234, .h[1] = 0x0056, .h[2] = 0x7800, .h[3] = 0x0000,
> +        .h[4] = 0x0078, .h[5] = 0x0000, .h[6] = 0x6543, .h[7] = 0x2100
> +    };
> +    S390Vector vs32 = {
> +        .w[0] = 0x12340000, .w[1] = 0x78654300,
> +        .w[2] = 0x0, .w[3] = 0x12,
> +    };
> +
> +    vistr(&vd, &vs16, 1, 0);
> +    if (vd.h[0] != 0x1234 || vd.h[1] != 0x0056 || vd.h[2] != 0x7800 ||
> +        vd.h[3] || vd.h[4] || vd.h[5] || vd.h[6] || vd.h[7]) {
> +        puts("ERROR: vitrh failed!");
> +        return 1;
> +    }
> +
> +    vistr(&vd, &vs32, 2, 0);
> +    if (vd.w[0] != 0x12340000 || vd.w[1] != 0x78654300 || vd.w[2] || vd.w[3]) {
> +        puts("ERROR: vitrf failed!");
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    return test_vistr();
> +}
> diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc
> index 3526ba3e3b..b69c1a111c 100644
> --- a/target/s390x/tcg/translate_vx.c.inc
> +++ b/target/s390x/tcg/translate_vx.c.inc
> @@ -2723,7 +2723,7 @@ static DisasJumpType op_vfene(DisasContext *s, DisasOps *o)
>  
>  static DisasJumpType op_vistr(DisasContext *s, DisasOps *o)
>  {
> -    const uint8_t es = get_field(s, m4);
> +    const uint8_t es = get_field(s, m3);
>      const uint8_t m5 = get_field(s, m5);
>      static gen_helper_gvec_2 * const g[3] = {
>          gen_helper_gvec_vistr8,
> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> index c830313e67..f8e71a9439 100644
> --- a/tests/tcg/s390x/Makefile.target
> +++ b/tests/tcg/s390x/Makefile.target
> @@ -18,6 +18,12 @@ TESTS+=signals-s390x
>  TESTS+=branch-relative-long
>  TESTS+=noexec
>  
> +Z13_TESTS=vf
> +vf: LDFLAGS+=-lm
> +$(Z13_TESTS): CFLAGS+=-march=z13 -O2
> +TESTS+=$(if $(shell $(CC) -march=z13 -S -o /dev/null -xc /dev/null \
> +			 >/dev/null 2>&1 && echo OK),$(Z13_TESTS))
> +

I didn't realise there where a bunch of compile time tests in the s390x
makefiles. The best practice (since Paolo's refactoring) is to emit a
config-cc.mak to set some variables once, e.g.:

  config-cc.mak: Makefile
          $(quiet-@)( \
              $(call cc-option,-march=armv8.1-a+sve,          CROSS_CC_HAS_SVE); \
              $(call cc-option,-march=armv8.1-a+sve2,         CROSS_CC_HAS_SVE2); \
              $(call cc-option,-march=armv8.3-a,              CROSS_CC_HAS_ARMV8_3); \
              $(call cc-option,-mbranch-protection=standard,  CROSS_CC_HAS_ARMV8_BTI); \
              $(call cc-option,-march=armv8.5-a+memtag,       CROSS_CC_HAS_ARMV8_MTE)) 3> config-cc.mak
  -include config-cc.mak


>  Z14_TESTS=vfminmax
>  vfminmax: LDFLAGS+=-lm
>  $(Z14_TESTS): CFLAGS+=-march=z14 -O2


-- 
Alex Bennée
Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by David Hildenbrand 3 years, 3 months ago 
On 11.10.22 12:14, Thomas Huth wrote:
> The element size is encoded in the M3 field, not in the M4
> field. Let's also add a TCG test that shows the failing
> behavior without this fix.
> 

I'd suggest moving the test to a separate patch and adding a Fixes: tag 
to the fix.

Should be

Fixes: be6324c6b734 ("s390x/tcg: Implement VECTOR ISOLATE STRING")

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1248
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
>   target/s390x/tcg/translate_vx.c.inc |  2 +-
>   tests/tcg/s390x/Makefile.target     |  6 ++++
>   3 files changed, 57 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/s390x/vf.c
> 
> diff --git a/tests/tcg/s390x/vf.c b/tests/tcg/s390x/vf.c
> new file mode 100644
> index 0000000000..fdc424ce7c
> --- /dev/null
> +++ b/tests/tcg/s390x/vf.c

In general, we use "vx" when talking about vector extension. Maybe name 
this vx.c

...

> @@ -0,0 +1,50 @@
> +/*
> + * vf: vector facility tests

And adjust it here as well.

> + */
> +#include <stdint.h>
> +#include <stdio.h>
> +#include "vx.h"
> +
> +static inline void vistr(S390Vector *v1, S390Vector *v2,
> +                         const uint8_t m3, const uint8_t m5)
> +{
> +    asm volatile("vistr %[v1], %[v2], %[m3], %[m5]\n"
> +                : [v1] "=v" (v1->v)
> +                : [v2]  "v" (v2->v)
> +                , [m3]  "i" (m3)
> +                , [m5]  "i" (m5)
> +                : "cc");
> +}
> +
> +static int test_vistr(void)
> +{
> +    S390Vector vd = {};
> +    S390Vector vs16 = {
> +        .h[0] = 0x1234, .h[1] = 0x0056, .h[2] = 0x7800, .h[3] = 0x0000,
> +        .h[4] = 0x0078, .h[5] = 0x0000, .h[6] = 0x6543, .h[7] = 0x2100
> +    };
> +    S390Vector vs32 = {
> +        .w[0] = 0x12340000, .w[1] = 0x78654300,
> +        .w[2] = 0x0, .w[3] = 0x12,
> +    };
> +
> +    vistr(&vd, &vs16, 1, 0);
> +    if (vd.h[0] != 0x1234 || vd.h[1] != 0x0056 || vd.h[2] != 0x7800 ||
> +        vd.h[3] || vd.h[4] || vd.h[5] || vd.h[6] || vd.h[7]) {
> +        puts("ERROR: vitrh failed!");
> +        return 1;
> +    }
> +
> +    vistr(&vd, &vs32, 2, 0);
> +    if (vd.w[0] != 0x12340000 || vd.w[1] != 0x78654300 || vd.w[2] || vd.w[3]) {
> +        puts("ERROR: vitrf failed!");
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    return test_vistr();
> +}
> diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc
> index 3526ba3e3b..b69c1a111c 100644
> --- a/target/s390x/tcg/translate_vx.c.inc
> +++ b/target/s390x/tcg/translate_vx.c.inc
> @@ -2723,7 +2723,7 @@ static DisasJumpType op_vfene(DisasContext *s, DisasOps *o)
>   
>   static DisasJumpType op_vistr(DisasContext *s, DisasOps *o)
>   {
> -    const uint8_t es = get_field(s, m4);
> +    const uint8_t es = get_field(s, m3);
>       const uint8_t m5 = get_field(s, m5);
>       static gen_helper_gvec_2 * const g[3] = {
>           gen_helper_gvec_vistr8,

Apart from that, LGTM.

-- 
Thanks,

David / dhildenb
Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by Thomas Huth 3 years, 3 months ago 
On 11/10/2022 14.30, David Hildenbrand wrote:
> On 11.10.22 12:14, Thomas Huth wrote:
>> The element size is encoded in the M3 field, not in the M4
>> field. Let's also add a TCG test that shows the failing
>> behavior without this fix.
>>
> 
> I'd suggest moving the test to a separate patch and adding a Fixes: tag to 
> the fix.
> 
> Should be
> 
> Fixes: be6324c6b734 ("s390x/tcg: Implement VECTOR ISOLATE STRING")

Ok, can do!

>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1248
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
>>   target/s390x/tcg/translate_vx.c.inc |  2 +-
>>   tests/tcg/s390x/Makefile.target     |  6 ++++
>>   3 files changed, 57 insertions(+), 1 deletion(-)
>>   create mode 100644 tests/tcg/s390x/vf.c
>>
>> diff --git a/tests/tcg/s390x/vf.c b/tests/tcg/s390x/vf.c
>> new file mode 100644
>> index 0000000000..fdc424ce7c
>> --- /dev/null
>> +++ b/tests/tcg/s390x/vf.c
> 
> In general, we use "vx" when talking about vector extension. Maybe name this 
> vx.c

Ok... or maybe "vecstring.c" in case we only want to test the vector string 
functions here? (they are in a separate chapter in the PoP)

  Thomas
Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by David Hildenbrand 3 years, 3 months ago 
On 11.10.22 14:45, Thomas Huth wrote:
> On 11/10/2022 14.30, David Hildenbrand wrote:
>> On 11.10.22 12:14, Thomas Huth wrote:
>>> The element size is encoded in the M3 field, not in the M4
>>> field. Let's also add a TCG test that shows the failing
>>> behavior without this fix.
>>>
>>
>> I'd suggest moving the test to a separate patch and adding a Fixes: tag to
>> the fix.
>>
>> Should be
>>
>> Fixes: be6324c6b734 ("s390x/tcg: Implement VECTOR ISOLATE STRING")
> 
> Ok, can do!
> 
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1248
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
>>>    target/s390x/tcg/translate_vx.c.inc |  2 +-
>>>    tests/tcg/s390x/Makefile.target     |  6 ++++
>>>    3 files changed, 57 insertions(+), 1 deletion(-)
>>>    create mode 100644 tests/tcg/s390x/vf.c
>>>
>>> diff --git a/tests/tcg/s390x/vf.c b/tests/tcg/s390x/vf.c
>>> new file mode 100644
>>> index 0000000000..fdc424ce7c
>>> --- /dev/null
>>> +++ b/tests/tcg/s390x/vf.c
>>
>> In general, we use "vx" when talking about vector extension. Maybe name this
>> vx.c
> 
> Ok... or maybe "vecstring.c" in case we only want to test the vector string
> functions here? (they are in a separate chapter in the PoP)

Also works for me. Or "vx_str.c" or sth like that.

-- 
Thanks,

David / dhildenb
Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction
Posted by Richard Henderson 3 years, 3 months ago 
On 10/11/22 06:24, David Hildenbrand wrote:
>>>> +++ b/tests/tcg/s390x/vf.c
>>>
>>> In general, we use "vx" when talking about vector extension. Maybe name this
>>> vx.c
>>
>> Ok... or maybe "vecstring.c" in case we only want to test the vector string
>> functions here? (they are in a separate chapter in the PoP)
> 
> Also works for me. Or "vx_str.c" or sth like that.

If we're going to bikeshed the name, use the insn being tested.

What you don't want is one big file testing lots of things.
What you do want is lots of little files each testing one thing.


r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.