[PATCH 0/3] target/s390x: Fix the "ignored match" case in VSTRS

[PATCH 0/3] target/s390x: Fix the "ignored match" case in VSTRS

[Ilya Leoshkevich]

Hi,

this series should hopefully fix the issue with __strstr_arch13(),
which Claudio reported. I have to admit I did not manage to fully
reproduce it, but at least with this change the traces of a simple test
from TCG and real hardware match.

I've also fuzzed the changed helper and strstr() itself; not sure
whether anything generic may come out of it, but here are the links
anyway [1] [2].

Patch 1 makes glibc pick __strstr_arch13() in qemu-user, patch 2 is the
fix and patch 3 is the test (generated from Claudio's strings and
further fuzzer's findings).

[1] https://gist.github.com/iii-i/5adad06d911c46079d4388001b22ab61
[2] https://gist.github.com/iii-i/c425800e75796eae65660491ac511356

Ilya Leoshkevich (3):
  linux-user/elfload: Enable vxe2 on s390x
  target/s390x: Fix the "ignored match" case in VSTRS
  tests/tcg/s390x: Test VSTRS

 linux-user/elfload.c                 |  1 +
 target/s390x/tcg/vec_string_helper.c | 54 ++++++-----------
 tests/tcg/s390x/Makefile.target      |  1 +
 tests/tcg/s390x/vxeh2_vstrs.c        | 88 ++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 37 deletions(-)
 create mode 100644 tests/tcg/s390x/vxeh2_vstrs.c

-- 
2.41.0

[PATCH 1/3] linux-user/elfload: Enable vxe2 on s390x

[Ilya Leoshkevich]

The vxe2 hwcap is not set for programs running in linux-user, but is
set by a Linux kernel running in softmmu. Add it to the former.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/elfload.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 861ec07abcd..33b20548721 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1614,6 +1614,7 @@ uint32_t get_elf_hwcap(void)
     }
     GET_FEATURE(S390_FEAT_VECTOR, HWCAP_S390_VXRS);
     GET_FEATURE(S390_FEAT_VECTOR_ENH, HWCAP_S390_VXRS_EXT);
+    GET_FEATURE(S390_FEAT_VECTOR_ENH2, HWCAP_S390_VXRS_EXT2);
 
     return hwcap;
 }
-- 
2.41.0

[PATCH 2/3] target/s390x: Fix the "ignored match" case in VSTRS

[Ilya Leoshkevich]

Currently the emulation of VSTRS recognizes partial matches in presence
of \0 in the haystack, which, according to PoP, is not correct:

    If the ZS flag is one and a zero byte was detected
    in the second operand, then there can not be a
    partial match ...

Add a check for this. While at it, fold a number of explicitly handled
special cases into the generic logic.

Cc: qemu-stable@nongnu.org
Reported-by: Claudio Fontana <cfontana@suse.de>
Closes: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg00633.html
Fixes: 1d706f314191 ("target/s390x: vxeh2: vector string search")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/vec_string_helper.c | 54 +++++++++-------------------
 1 file changed, 17 insertions(+), 37 deletions(-)

diff --git a/target/s390x/tcg/vec_string_helper.c b/target/s390x/tcg/vec_string_helper.c
index 9b85becdfbf..a19f429768f 100644
--- a/target/s390x/tcg/vec_string_helper.c
+++ b/target/s390x/tcg/vec_string_helper.c
@@ -474,9 +474,9 @@ DEF_VSTRC_CC_RT_HELPER(32)
 static int vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
                  const S390Vector *v4, uint8_t es, bool zs)
 {
-    int substr_elen, substr_0, str_elen, i, j, k, cc;
+    int substr_elen, i, j, k, cc;
     int nelem = 16 >> es;
-    bool eos = false;
+    int str_leftmost_0;
 
     substr_elen = s390_vec_read_element8(v4, 7) >> es;
 
@@ -498,47 +498,20 @@ static int vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
     }
 
     /* If ZS, look for eos in the searched string. */
+    str_leftmost_0 = nelem;
     if (zs) {
         for (k = 0; k < nelem; k++) {
             if (s390_vec_read_element(v2, k, es) == 0) {
-                eos = true;
+                str_leftmost_0 = k;
                 break;
             }
         }
-        str_elen = k;
-    } else {
-        str_elen = nelem;
     }
 
-    substr_0 = s390_vec_read_element(v3, 0, es);
-
-    for (k = 0; ; k++) {
-        for (; k < str_elen; k++) {
-            if (s390_vec_read_element(v2, k, es) == substr_0) {
-                break;
-            }
-        }
-
-        /* If we reached the end of the string, no match. */
-        if (k == str_elen) {
-            cc = eos; /* no match (with or without zero char) */
-            goto done;
-        }
-
-        /* If the substring is only one char, match. */
-        if (substr_elen == 1) {
-            cc = 2; /* full match */
-            goto done;
-        }
-
-        /* If the match begins at the last char, we have a partial match. */
-        if (k == str_elen - 1) {
-            cc = 3; /* partial match */
-            goto done;
-        }
-
+    cc = str_leftmost_0 == nelem ? 0 : 1;  /* No match. */
+    for (k = 0; k < nelem; k++) {
         i = MIN(nelem, k + substr_elen);
-        for (j = k + 1; j < i; j++) {
+        for (j = k; j < i; j++) {
             uint32_t e2 = s390_vec_read_element(v2, j, es);
             uint32_t e3 = s390_vec_read_element(v3, j - k, es);
             if (e2 != e3) {
@@ -546,9 +519,16 @@ static int vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
             }
         }
         if (j == i) {
-            /* Matched up until "end". */
-            cc = i - k == substr_elen ? 2 : 3; /* full or partial match */
-            goto done;
+            /* All elements matched. */
+            if (k > str_leftmost_0) {
+                cc = 1;  /* Ignored match. */
+                k = nelem;
+            } else if (i - k == substr_elen) {
+                cc = 2;  /* Full match. */
+            } else {
+                cc = 3;  /* Partial match. */
+            }
+            break;
         }
     }
 
-- 
2.41.0

[PATCH 3/3] tests/tcg/s390x: Test VSTRS

[Ilya Leoshkevich]

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;
+}
+
+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;
+}
-- 
2.41.0

Re: [PATCH 1/3] linux-user/elfload: Enable vxe2 on s390x

[Richard Henderson]

On 8/4/23 16:03, Ilya Leoshkevich wrote:
> The vxe2 hwcap is not set for programs running in linux-user, but is
> set by a Linux kernel running in softmmu. Add it to the former.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/elfload.c | 1 +
>   1 file changed, 1 insertion(+)

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

r~

Re: [PATCH 1/3] linux-user/elfload: Enable vxe2 on s390x

[David Hildenbrand]

On 05.08.23 01:03, Ilya Leoshkevich wrote:
> The vxe2 hwcap is not set for programs running in linux-user, but is
> set by a Linux kernel running in softmmu. Add it to the former.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/elfload.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 861ec07abcd..33b20548721 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1614,6 +1614,7 @@ uint32_t get_elf_hwcap(void)
>       }
>       GET_FEATURE(S390_FEAT_VECTOR, HWCAP_S390_VXRS);
>       GET_FEATURE(S390_FEAT_VECTOR_ENH, HWCAP_S390_VXRS_EXT);
> +    GET_FEATURE(S390_FEAT_VECTOR_ENH2, HWCAP_S390_VXRS_EXT2);
>   
>       return hwcap;
>   }

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/3] target/s390x: Fix the "ignored match" case in VSTRS

[David Hildenbrand]

On 05.08.23 01:03, Ilya Leoshkevich wrote:
> Currently the emulation of VSTRS recognizes partial matches in presence
> of \0 in the haystack, which, according to PoP, is not correct:
> 
>      If the ZS flag is one and a zero byte was detected
>      in the second operand, then there can not be a
>      partial match ...
> 
> Add a check for this. While at it, fold a number of explicitly handled
> special cases into the generic logic.

Can we split that off? Or doesn't it make sense to split it off after 
fixing the issue?

-- 
Cheers,

David / dhildenb

Re: [PATCH 3/3] tests/tcg/s390x: Test VSTRS

[Claudio Fontana]

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;
> +}

Re: [PATCH 2/3] target/s390x: Fix the "ignored match" case in VSTRS

[Claudio Fontana]

On 8/5/23 01:03, Ilya Leoshkevich wrote:
> Currently the emulation of VSTRS recognizes partial matches in presence
> of \0 in the haystack, which, according to PoP, is not correct:
> 
>     If the ZS flag is one and a zero byte was detected
>     in the second operand, then there can not be a
>     partial match ...
> 
> Add a check for this. While at it, fold a number of explicitly handled
> special cases into the generic logic.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Claudio Fontana <cfontana@suse.de>
> Closes: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg00633.html
> Fixes: 1d706f314191 ("target/s390x: vxeh2: vector string search")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

verified that it fixes the problem I encountered,

Tested-by: Claudio Fontana <cfontana@suse.de>

> ---
>  target/s390x/tcg/vec_string_helper.c | 54 +++++++++-------------------
>  1 file changed, 17 insertions(+), 37 deletions(-)
> 
> diff --git a/target/s390x/tcg/vec_string_helper.c b/target/s390x/tcg/vec_string_helper.c
> index 9b85becdfbf..a19f429768f 100644
> --- a/target/s390x/tcg/vec_string_helper.c
> +++ b/target/s390x/tcg/vec_string_helper.c
> @@ -474,9 +474,9 @@ DEF_VSTRC_CC_RT_HELPER(32)
>  static int vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
>                   const S390Vector *v4, uint8_t es, bool zs)
>  {
> -    int substr_elen, substr_0, str_elen, i, j, k, cc;
> +    int substr_elen, i, j, k, cc;
>      int nelem = 16 >> es;
> -    bool eos = false;
> +    int str_leftmost_0;
>  
>      substr_elen = s390_vec_read_element8(v4, 7) >> es;
>  
> @@ -498,47 +498,20 @@ static int vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
>      }
>  
>      /* If ZS, look for eos in the searched string. */
> +    str_leftmost_0 = nelem;
>      if (zs) {
>          for (k = 0; k < nelem; k++) {
>              if (s390_vec_read_element(v2, k, es) == 0) {
> -                eos = true;
> +                str_leftmost_0 = k;
>                  break;
>              }
>          }
> -        str_elen = k;
> -    } else {
> -        str_elen = nelem;
>      }
>  
> -    substr_0 = s390_vec_read_element(v3, 0, es);
> -
> -    for (k = 0; ; k++) {
> -        for (; k < str_elen; k++) {
> -            if (s390_vec_read_element(v2, k, es) == substr_0) {
> -                break;
> -            }
> -        }
> -
> -        /* If we reached the end of the string, no match. */
> -        if (k == str_elen) {
> -            cc = eos; /* no match (with or without zero char) */
> -            goto done;
> -        }
> -
> -        /* If the substring is only one char, match. */
> -        if (substr_elen == 1) {
> -            cc = 2; /* full match */
> -            goto done;
> -        }
> -
> -        /* If the match begins at the last char, we have a partial match. */
> -        if (k == str_elen - 1) {
> -            cc = 3; /* partial match */
> -            goto done;
> -        }
> -
> +    cc = str_leftmost_0 == nelem ? 0 : 1;  /* No match. */
> +    for (k = 0; k < nelem; k++) {
>          i = MIN(nelem, k + substr_elen);
> -        for (j = k + 1; j < i; j++) {
> +        for (j = k; j < i; j++) {
>              uint32_t e2 = s390_vec_read_element(v2, j, es);
>              uint32_t e3 = s390_vec_read_element(v3, j - k, es);
>              if (e2 != e3) {
> @@ -546,9 +519,16 @@ static int vstrs(S390Vector *v1, const S390Vector *v2, const S390Vector *v3,
>              }
>          }
>          if (j == i) {
> -            /* Matched up until "end". */
> -            cc = i - k == substr_elen ? 2 : 3; /* full or partial match */
> -            goto done;
> +            /* All elements matched. */
> +            if (k > str_leftmost_0) {
> +                cc = 1;  /* Ignored match. */
> +                k = nelem;
> +            } else if (i - k == substr_elen) {
> +                cc = 2;  /* Full match. */
> +            } else {
> +                cc = 3;  /* Partial match. */
> +            }
> +            break;
>          }
>      }
>  

Re: [PATCH 3/3] tests/tcg/s390x: Test VSTRS

[Ilya Leoshkevich]

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

[...]
> 

Re: [PATCH 2/3] target/s390x: Fix the "ignored match" case in VSTRS

[Ilya Leoshkevich]

On Sat, 2023-08-05 at 10:02 +0200, David Hildenbrand wrote:
> On 05.08.23 01:03, Ilya Leoshkevich wrote:
> > Currently the emulation of VSTRS recognizes partial matches in
> > presence
> > of \0 in the haystack, which, according to PoP, is not correct:
> > 
> >      If the ZS flag is one and a zero byte was detected
> >      in the second operand, then there can not be a
> >      partial match ...
> > 
> > Add a check for this. While at it, fold a number of explicitly
> > handled
> > special cases into the generic logic.
> 
> Can we split that off? Or doesn't it make sense to split it off after
> fixing the issue?

I could do this if this is important, e.g., for stable, but I came to
the conclusion that I needed to get rid of the special cases after I
had to add the new check to more than one place.

Re: [PATCH 3/3] tests/tcg/s390x: Test VSTRS

[Claudio Fontana]

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
> 
> [...]
>>
> 

Re: [PATCH 1/3] linux-user/elfload: Enable vxe2 on s390x

[Claudio Fontana]

On 8/5/23 01:03, Ilya Leoshkevich wrote:
> The vxe2 hwcap is not set for programs running in linux-user, but is
> set by a Linux kernel running in softmmu. Add it to the former.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  linux-user/elfload.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 861ec07abcd..33b20548721 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1614,6 +1614,7 @@ uint32_t get_elf_hwcap(void)
>      }
>      GET_FEATURE(S390_FEAT_VECTOR, HWCAP_S390_VXRS);
>      GET_FEATURE(S390_FEAT_VECTOR_ENH, HWCAP_S390_VXRS_EXT);
> +    GET_FEATURE(S390_FEAT_VECTOR_ENH2, HWCAP_S390_VXRS_EXT2);
>  
>      return hwcap;
>  }

Reviewed-by: Claudio Fontana <cfontana@suse.de>

Re: [PATCH 3/3] tests/tcg/s390x: Test VSTRS

[Claudio Fontana]

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;
> +}

Re: [PATCH 3/3] tests/tcg/s390x: Test VSTRS

[Ilya Leoshkevich]

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

Re: [PATCH 2/3] target/s390x: Fix the "ignored match" case in VSTRS

[David Hildenbrand]

On 07.08.23 10:10, Ilya Leoshkevich wrote:
> On Sat, 2023-08-05 at 10:02 +0200, David Hildenbrand wrote:
>> On 05.08.23 01:03, Ilya Leoshkevich wrote:
>>> Currently the emulation of VSTRS recognizes partial matches in
>>> presence
>>> of \0 in the haystack, which, according to PoP, is not correct:
>>>
>>>       If the ZS flag is one and a zero byte was detected
>>>       in the second operand, then there can not be a
>>>       partial match ...
>>>
>>> Add a check for this. While at it, fold a number of explicitly
>>> handled
>>> special cases into the generic logic.
>>
>> Can we split that off? Or doesn't it make sense to split it off after
>> fixing the issue?
> 
> I could do this if this is important, e.g., for stable, but I came to
> the conclusion that I needed to get rid of the special cases after I
> had to add the new check to more than one place.

Okay, so let's keep it as is.

Fortunately you heavily test that code! :)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb