[PATCH v2 0/5] target/s390x: CC fixes

[PATCH v2 0/5] target/s390x: CC fixes

[Ilya Leoshkevich]

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg10251.html
v1 -> v2: Introduce op_laa_addu64() (Richard).
          Add the ADD LOGICAL WITH CARRY test. This was intended to
          catch the issue mentioned by David, but did not succeed. I
          thought it would still be worthwile to include it as a
          regression test.

Hi,

This series fixes two issues with updating CC. David was suggesting a
bigger rewrite [1], but I did not dare do this (yet). Instead, these
are targeted fixes: patch 1 helps with installing Fedora, and patch 3
addresses something I noticed when reviewing the code. Patches 2, 4 and
5 are tests.

Best regards,
Ilya

[1] https://gitlab.com/qemu-project/qemu/-/issues/1865#note_1545060658

Ilya Leoshkevich (5):
  target/s390x: Fix CLC corrupting cc_src
  tests/tcg/s390x: Test CLC with inaccessible second operand
  target/s390x: Fix LAALG not updating cc_src
  tests/tcg/s390x: Test LAALG with negative cc_src
  tests/tcg/s390x: Test ADD LOGICAL WITH CARRY

 target/s390x/tcg/insn-data.h.inc         |   2 +-
 target/s390x/tcg/translate.c             |  26 +++-
 tests/tcg/s390x/Makefile.target          |   3 +
 tests/tcg/s390x/add-logical-with-carry.c | 156 +++++++++++++++++++++++
 tests/tcg/s390x/clc.c                    |  48 +++++++
 tests/tcg/s390x/laalg.c                  |  27 ++++
 6 files changed, 257 insertions(+), 5 deletions(-)
 create mode 100644 tests/tcg/s390x/add-logical-with-carry.c
 create mode 100644 tests/tcg/s390x/clc.c
 create mode 100644 tests/tcg/s390x/laalg.c

-- 
2.41.0

[PATCH v2 1/5] target/s390x: Fix CLC corrupting cc_src

[Ilya Leoshkevich]

CLC updates cc_src before accessing the second operand; if the latter
is inaccessible, the former ends up containing a bogus value.

Fix by reading cc_src into a temporary first.

Fixes: 4f7403d52b1c ("target-s390: Convert CLC")
Closes: https://gitlab.com/qemu-project/qemu/-/issues/1865
Cc: qemu-stable@nongnu.org
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/translate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 4bae1509f50..a0d6a2a35dd 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2007,6 +2007,7 @@ static DisasJumpType op_cksm(DisasContext *s, DisasOps *o)
 static DisasJumpType op_clc(DisasContext *s, DisasOps *o)
 {
     int l = get_field(s, l1);
+    TCGv_i64 src;
     TCGv_i32 vl;
     MemOp mop;
 
@@ -2016,9 +2017,11 @@ static DisasJumpType op_clc(DisasContext *s, DisasOps *o)
     case 4:
     case 8:
         mop = ctz32(l + 1) | MO_TE;
-        tcg_gen_qemu_ld_tl(cc_src, o->addr1, get_mem_index(s), mop);
+        /* Do not update cc_src yet: loading cc_dst may cause an exception. */
+        src = tcg_temp_new_i64();
+        tcg_gen_qemu_ld_tl(src, o->addr1, get_mem_index(s), mop);
         tcg_gen_qemu_ld_tl(cc_dst, o->in2, get_mem_index(s), mop);
-        gen_op_update2_cc_i64(s, CC_OP_LTUGTU_64, cc_src, cc_dst);
+        gen_op_update2_cc_i64(s, CC_OP_LTUGTU_64, src, cc_dst);
         return DISAS_NEXT;
     default:
         vl = tcg_constant_i32(l);
-- 
2.41.0

[PATCH v2 2/5] tests/tcg/s390x: Test CLC with inaccessible second operand

[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/clc.c           | 48 +++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
 create mode 100644 tests/tcg/s390x/clc.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 826f0a18e43..ccd4f4e68de 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -41,6 +41,7 @@ TESTS+=larl
 TESTS+=mdeb
 TESTS+=cgebra
 TESTS+=clgebr
+TESTS+=clc
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/clc.c b/tests/tcg/s390x/clc.c
new file mode 100644
index 00000000000..e14189bd75e
--- /dev/null
+++ b/tests/tcg/s390x/clc.c
@@ -0,0 +1,48 @@
+/*
+ * Test the CLC instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+static void handle_sigsegv(int sig, siginfo_t *info, void *ucontext)
+{
+    mcontext_t *mcontext = &((ucontext_t *)ucontext)->uc_mcontext;
+    if (mcontext->gregs[0] != 600) {
+        write(STDERR_FILENO, "bad r0\n", 7);
+        _exit(EXIT_FAILURE);
+    }
+    if (((mcontext->psw.mask >> 44) & 3) != 1) {
+        write(STDERR_FILENO, "bad cc\n", 7);
+        _exit(EXIT_FAILURE);
+    }
+    _exit(EXIT_SUCCESS);
+}
+
+int main(void)
+{
+    register unsigned long r0 asm("r0");
+    unsigned long mem = 42, rhs = 500;
+    struct sigaction act;
+    int err;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_sigaction = handle_sigsegv;
+    act.sa_flags = SA_SIGINFO;
+    err = sigaction(SIGSEGV, &act, NULL);
+    assert(err == 0);
+
+    r0 = 100;
+    asm("algr %[r0],%[rhs]\n"
+        "clc 0(8,%[mem]),0(0)\n"  /* The 2nd operand will cause a SEGV. */
+        : [r0] "+r" (r0)
+        : [mem] "r" (&mem)
+        , [rhs] "r" (rhs)
+        : "cc", "memory");
+
+    return EXIT_FAILURE;
+}
-- 
2.41.0

[PATCH v2 3/5] target/s390x: Fix LAALG not updating cc_src

[Ilya Leoshkevich]

LAALG uses op_laa() and wout_addu64(). The latter expects cc_src to be
set, but the former does not do it. This can lead to assertion failures
if something sets cc_src to neither 0 nor 1 before.

Fix by introducing op_laa_addu64(), which sets cc_src, and using it for
LAALG.

Fixes: 4dba4d6fef61 ("target/s390x: Use atomic operations for LOAD AND OP")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/insn-data.h.inc |  2 +-
 target/s390x/tcg/translate.c     | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 0bfd88d3c3a..2f07f39d9cb 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -442,7 +442,7 @@
     D(0xebe8, LAAG,    RSY_a, ILA, r3, a2, new, in2_r1, laa, adds64, MO_TEUQ)
 /* LOAD AND ADD LOGICAL */
     D(0xebfa, LAAL,    RSY_a, ILA, r3_32u, a2, new, in2_r1_32, laa, addu32, MO_TEUL)
-    D(0xebea, LAALG,   RSY_a, ILA, r3, a2, new, in2_r1, laa, addu64, MO_TEUQ)
+    D(0xebea, LAALG,   RSY_a, ILA, r3, a2, new, in2_r1, laa_addu64, addu64, MO_TEUQ)
 /* LOAD AND AND */
     D(0xebf4, LAN,     RSY_a, ILA, r3_32s, a2, new, in2_r1_32, lan, nz32, MO_TESL)
     D(0xebe4, LANG,    RSY_a, ILA, r3, a2, new, in2_r1, lan, nz64, MO_TEUQ)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index a0d6a2a35dd..62ab2be8b12 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2677,17 +2677,32 @@ static DisasJumpType op_kxb(DisasContext *s, DisasOps *o)
     return DISAS_NEXT;
 }
 
-static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
+static DisasJumpType help_laa(DisasContext *s, DisasOps *o, bool addu64)
 {
     /* The real output is indeed the original value in memory;
        recompute the addition for the computation of CC.  */
     tcg_gen_atomic_fetch_add_i64(o->in2, o->in2, o->in1, get_mem_index(s),
                                  s->insn->data | MO_ALIGN);
     /* However, we need to recompute the addition for setting CC.  */
-    tcg_gen_add_i64(o->out, o->in1, o->in2);
+    if (addu64) {
+        tcg_gen_movi_i64(cc_src, 0);
+        tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
+    } else {
+        tcg_gen_add_i64(o->out, o->in1, o->in2);
+    }
     return DISAS_NEXT;
 }
 
+static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
+{
+    return help_laa(s, o, false);
+}
+
+static DisasJumpType op_laa_addu64(DisasContext *s, DisasOps *o)
+{
+    return help_laa(s, o, true);
+}
+
 static DisasJumpType op_lan(DisasContext *s, DisasOps *o)
 {
     /* The real output is indeed the original value in memory;
-- 
2.41.0

[PATCH v2 4/5] tests/tcg/s390x: Test LAALG with negative cc_src

[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/laalg.c         | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 tests/tcg/s390x/laalg.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index ccd4f4e68de..a476547b659 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -42,6 +42,7 @@ TESTS+=mdeb
 TESTS+=cgebra
 TESTS+=clgebr
 TESTS+=clc
+TESTS+=laalg
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/laalg.c b/tests/tcg/s390x/laalg.c
new file mode 100644
index 00000000000..797d168bb16
--- /dev/null
+++ b/tests/tcg/s390x/laalg.c
@@ -0,0 +1,27 @@
+/*
+ * Test the LAALG instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <stdlib.h>
+
+int main(void)
+{
+    unsigned long cc = 0, op1, op2 = 40, op3 = 2;
+
+    asm("slgfi %[cc],1\n"  /* Set cc_src = -1. */
+        "laalg %[op1],%[op3],%[op2]\n"
+        "ipm %[cc]"
+        : [cc] "+r" (cc)
+        , [op1] "=r" (op1)
+        , [op2] "+T" (op2)
+        : [op3] "r" (op3)
+        : "cc");
+
+    assert(cc == 0xffffffff10ffffff);
+    assert(op1 == 40);
+    assert(op2 == 42);
+
+    return EXIT_SUCCESS;
+}
-- 
2.41.0

[PATCH v2 5/5] tests/tcg/s390x: Test ADD LOGICAL WITH CARRY

[Ilya Leoshkevich]

Add a test that tries different combinations of ADD LOGICAL WITH
CARRY instructions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target          |   1 +
 tests/tcg/s390x/add-logical-with-carry.c | 156 +++++++++++++++++++++++
 2 files changed, 157 insertions(+)
 create mode 100644 tests/tcg/s390x/add-logical-with-carry.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index a476547b659..0e670f3f8b9 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -43,6 +43,7 @@ TESTS+=cgebra
 TESTS+=clgebr
 TESTS+=clc
 TESTS+=laalg
+TESTS+=add-logical-with-carry
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/add-logical-with-carry.c b/tests/tcg/s390x/add-logical-with-carry.c
new file mode 100644
index 00000000000..d982f8a651b
--- /dev/null
+++ b/tests/tcg/s390x/add-logical-with-carry.c
@@ -0,0 +1,156 @@
+/*
+ * Test ADD LOGICAL WITH CARRY instructions.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <stdio.h>
+#include <stdlib.h>
+
+static const struct test {
+    const char *name;
+    unsigned long values[3];
+    unsigned long exp_sum;
+    int exp_cc;
+} tests[] = {
+    /*
+     * Each test starts with CC 0 and executes two chained ADD LOGICAL WITH
+     * CARRY instructions on three input values. The values must be compatible
+     * with both 32- and 64-bit test functions.
+     */
+
+    /* NAME       VALUES       EXP_SUM EXP_CC */
+    { "cc0->cc0", {0, 0, 0},   0,      0, },
+    { "cc0->cc1", {0, 0, 42},  42,     1, },
+    /* cc0->cc2 is not possible */
+    /* cc0->cc3 is not possible */
+    /* cc1->cc0 is not possible */
+    { "cc1->cc1", {-3, 1, 1},  -1,     1, },
+    { "cc1->cc2", {-3, 1, 2},  0,      2, },
+    { "cc1->cc3", {-3, 1, -1}, -3,     3, },
+    /* cc2->cc0 is not possible */
+    { "cc2->cc1", {-1, 1, 1},  2,      1, },
+    { "cc2->cc2", {-1, 1, -1}, 0,      2, },
+    /* cc2->cc3 is not possible */
+    /* cc3->cc0 is not possible */
+    { "cc3->cc1", {-1, 2, 1},  3,      1, },
+    { "cc3->cc2", {-1, 2, -2}, 0,      2, },
+    { "cc3->cc3", {-1, 2, -1}, 1,      3, },
+};
+
+/* Test ALCR (register variant) followed by ALC (memory variant). */
+static unsigned long test32rm(unsigned long a, unsigned long b,
+                              unsigned long c, int *cc)
+{
+    unsigned int a32 = a, b32 = b, c32 = c;
+
+    asm("xr %[cc],%[cc]\n"
+        "alcr %[a],%[b]\n"
+        "alc %[a],%[c]\n"
+        "ipm %[cc]"
+        : [a] "+&r" (a32), [cc] "+&r" (*cc)
+        : [b] "r" (b32), [c] "T" (c32)
+        : "cc");
+    *cc >>= 28;
+
+    return (int)a32;
+}
+
+/* Test ALC (memory variant) followed by ALCR (register variant). */
+static unsigned long test32mr(unsigned long a, unsigned long b,
+                              unsigned long c, int *cc)
+{
+    unsigned int a32 = a, b32 = b, c32 = c;
+
+    asm("xr %[cc],%[cc]\n"
+        "alc %[a],%[b]\n"
+        "alcr %[c],%[a]\n"
+        "ipm %[cc]"
+        : [a] "+&r" (a32), [c] "+&r" (c32), [cc] "+&r" (*cc)
+        : [b] "T" (b32)
+        : "cc");
+    *cc >>= 28;
+
+    return (int)c32;
+}
+
+/* Test ALCGR (register variant) followed by ALCG (memory variant). */
+static unsigned long test64rm(unsigned long a, unsigned long b,
+                              unsigned long c, int *cc)
+{
+    asm("xr %[cc],%[cc]\n"
+        "alcgr %[a],%[b]\n"
+        "alcg %[a],%[c]\n"
+        "ipm %[cc]"
+        : [a] "+&r" (a), [cc] "+&r" (*cc)
+        : [b] "r" (b), [c] "T" (c)
+        : "cc");
+    *cc >>= 28;
+    return a;
+}
+
+/* Test ALCG (memory variant) followed by ALCGR (register variant). */
+static unsigned long test64mr(unsigned long a, unsigned long b,
+                              unsigned long c, int *cc)
+{
+    asm("xr %[cc],%[cc]\n"
+        "alcg %[a],%[b]\n"
+        "alcgr %[c],%[a]\n"
+        "ipm %[cc]"
+        : [a] "+&r" (a), [c] "+&r" (c), [cc] "+&r" (*cc)
+        : [b] "T" (b)
+        : "cc");
+    *cc >>= 28;
+    return c;
+}
+
+static const struct test_func {
+    const char *name;
+    unsigned long (*ptr)(unsigned long, unsigned long, unsigned long, int *);
+} test_funcs[] = {
+    { "test32rm", test32rm },
+    { "test32mr", test32mr },
+    { "test64rm", test64rm },
+    { "test64mr", test64mr },
+};
+
+static const struct test_perm {
+    const char *name;
+    size_t a_idx, b_idx, c_idx;
+} test_perms[] = {
+    { "a, b, c", 0, 1, 2 },
+    { "b, a, c", 1, 0, 2 },
+};
+
+int main(void)
+{
+    unsigned long a, b, c, sum;
+    int result = EXIT_SUCCESS;
+    const struct test_func *f;
+    const struct test_perm *p;
+    size_t i, j, k;
+    const struct test *t;
+    int cc;
+
+    for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
+        t = &tests[i];
+        for (j = 0; j < sizeof(test_funcs) / sizeof(test_funcs[0]); j++) {
+            f = &test_funcs[j];
+            for (k = 0; k < sizeof(test_perms) / sizeof(test_perms[0]); k++) {
+                p = &test_perms[k];
+                a = t->values[p->a_idx];
+                b = t->values[p->b_idx];
+                c = t->values[p->c_idx];
+                sum = f->ptr(a, b, c, &cc);
+                if (sum != t->exp_sum || cc != t->exp_cc) {
+                    fprintf(stderr,
+                            "[  FAILED  ] %s %s(0x%lx, 0x%lx, 0x%lx) returned 0x%lx cc %d, expected 0x%lx cc %d\n",
+                            t->name, f->name, a, b, c, sum, cc,
+                            t->exp_sum, t->exp_cc);
+                    result = EXIT_FAILURE;
+                }
+            }
+        }
+    }
+
+    return result;
+}
-- 
2.41.0

Re: [PATCH v2 2/5] tests/tcg/s390x: Test CLC with inaccessible second operand

[Richard Henderson]

On 11/6/23 01:31, 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/clc.c           | 48 +++++++++++++++++++++++++++++++++
>   2 files changed, 49 insertions(+)
>   create mode 100644 tests/tcg/s390x/clc.c

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

r~

Re: [PATCH v2 3/5] target/s390x: Fix LAALG not updating cc_src

[Richard Henderson]

On 11/6/23 01:31, Ilya Leoshkevich wrote:
> LAALG uses op_laa() and wout_addu64(). The latter expects cc_src to be
> set, but the former does not do it. This can lead to assertion failures
> if something sets cc_src to neither 0 nor 1 before.
> 
> Fix by introducing op_laa_addu64(), which sets cc_src, and using it for
> LAALG.
> 
> Fixes: 4dba4d6fef61 ("target/s390x: Use atomic operations for LOAD AND OP")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/insn-data.h.inc |  2 +-
>   target/s390x/tcg/translate.c     | 19 +++++++++++++++++--
>   2 files changed, 18 insertions(+), 3 deletions(-)

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


r~

Re: [PATCH v2 4/5] tests/tcg/s390x: Test LAALG with negative cc_src

[Richard Henderson]

On 11/6/23 01:31, 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/laalg.c         | 27 +++++++++++++++++++++++++++
>   2 files changed, 28 insertions(+)
>   create mode 100644 tests/tcg/s390x/laalg.c

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


r~

Re: [PATCH v2 1/5] target/s390x: Fix CLC corrupting cc_src

[David Hildenbrand]

On 06.11.23 10:31, Ilya Leoshkevich wrote:
> CLC updates cc_src before accessing the second operand; if the latter
> is inaccessible, the former ends up containing a bogus value.
> 
> Fix by reading cc_src into a temporary first.
> 
> Fixes: 4f7403d52b1c ("target-s390: Convert CLC")
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/1865
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/translate.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index 4bae1509f50..a0d6a2a35dd 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2007,6 +2007,7 @@ static DisasJumpType op_cksm(DisasContext *s, DisasOps *o)
>   static DisasJumpType op_clc(DisasContext *s, DisasOps *o)
>   {
>       int l = get_field(s, l1);
> +    TCGv_i64 src;
>       TCGv_i32 vl;
>       MemOp mop;
>   
> @@ -2016,9 +2017,11 @@ static DisasJumpType op_clc(DisasContext *s, DisasOps *o)
>       case 4:
>       case 8:
>           mop = ctz32(l + 1) | MO_TE;
> -        tcg_gen_qemu_ld_tl(cc_src, o->addr1, get_mem_index(s), mop);
> +        /* Do not update cc_src yet: loading cc_dst may cause an exception. */
> +        src = tcg_temp_new_i64();
> +        tcg_gen_qemu_ld_tl(src, o->addr1, get_mem_index(s), mop);
>           tcg_gen_qemu_ld_tl(cc_dst, o->in2, get_mem_index(s), mop);
> -        gen_op_update2_cc_i64(s, CC_OP_LTUGTU_64, cc_src, cc_dst);
> +        gen_op_update2_cc_i64(s, CC_OP_LTUGTU_64, src, cc_dst);
>           return DISAS_NEXT;
>       default:
>           vl = tcg_constant_i32(l);

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

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/5] target/s390x: Fix LAALG not updating cc_src

[David Hildenbrand]

On 06.11.23 10:31, Ilya Leoshkevich wrote:
> LAALG uses op_laa() and wout_addu64(). The latter expects cc_src to be
> set, but the former does not do it. This can lead to assertion failures
> if something sets cc_src to neither 0 nor 1 before.
> 
> Fix by introducing op_laa_addu64(), which sets cc_src, and using it for
> LAALG.
> 
> Fixes: 4dba4d6fef61 ("target/s390x: Use atomic operations for LOAD AND OP")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/insn-data.h.inc |  2 +-
>   target/s390x/tcg/translate.c     | 19 +++++++++++++++++--
>   2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
> index 0bfd88d3c3a..2f07f39d9cb 100644
> --- a/target/s390x/tcg/insn-data.h.inc
> +++ b/target/s390x/tcg/insn-data.h.inc
> @@ -442,7 +442,7 @@
>       D(0xebe8, LAAG,    RSY_a, ILA, r3, a2, new, in2_r1, laa, adds64, MO_TEUQ)
>   /* LOAD AND ADD LOGICAL */
>       D(0xebfa, LAAL,    RSY_a, ILA, r3_32u, a2, new, in2_r1_32, laa, addu32, MO_TEUL)
> -    D(0xebea, LAALG,   RSY_a, ILA, r3, a2, new, in2_r1, laa, addu64, MO_TEUQ)
> +    D(0xebea, LAALG,   RSY_a, ILA, r3, a2, new, in2_r1, laa_addu64, addu64, MO_TEUQ)
>   /* LOAD AND AND */
>       D(0xebf4, LAN,     RSY_a, ILA, r3_32s, a2, new, in2_r1_32, lan, nz32, MO_TESL)
>       D(0xebe4, LANG,    RSY_a, ILA, r3, a2, new, in2_r1, lan, nz64, MO_TEUQ)
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index a0d6a2a35dd..62ab2be8b12 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2677,17 +2677,32 @@ static DisasJumpType op_kxb(DisasContext *s, DisasOps *o)
>       return DISAS_NEXT;
>   }
>   
> -static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
> +static DisasJumpType help_laa(DisasContext *s, DisasOps *o, bool addu64)
>   {
>       /* The real output is indeed the original value in memory;
>          recompute the addition for the computation of CC.  */
>       tcg_gen_atomic_fetch_add_i64(o->in2, o->in2, o->in1, get_mem_index(s),
>                                    s->insn->data | MO_ALIGN);
>       /* However, we need to recompute the addition for setting CC.  */
> -    tcg_gen_add_i64(o->out, o->in1, o->in2);
> +    if (addu64) {
> +        tcg_gen_movi_i64(cc_src, 0);
> +        tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
> +    } else {
> +        tcg_gen_add_i64(o->out, o->in1, o->in2);
> +    }
>       return DISAS_NEXT;
>   }
>   
> +static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
> +{
> +    return help_laa(s, o, false);
> +}
> +
> +static DisasJumpType op_laa_addu64(DisasContext *s, DisasOps *o)
> +{
> +    return help_laa(s, o, true);
> +}
> +
>   static DisasJumpType op_lan(DisasContext *s, DisasOps *o)
>   {
>       /* The real output is indeed the original value in memory;

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

-- 
Cheers,

David / dhildenb