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

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

[Thomas Huth]

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

[David Hildenbrand]

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

[Thomas Huth]

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

[David Hildenbrand]

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

[Richard Henderson]

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~

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

[Alex Bennée]

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

[Richard Henderson]

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~