[PATCH v2 0/2] target/s390x/tcg: Set STCK/STCKF condition code after the store

[PATCH v2 0/2] target/s390x/tcg: Set STCK/STCKF condition code after the store

[Ilya Leoshkevich]

Hi,

Ido reported an assertion failure after failing STCKF and provided a
small assembly snippet that causes it.

This series fixes the issue and adds a test based on that snippet.

Best regards,
Ilya

Ilya Leoshkevich (2):
  target/s390x/tcg: Set STCK/STCKF condition code after the store
  tests/tcg/s390x: Test STCKF condition code on a faulting store

 target/s390x/tcg/insn-data.h.inc |  4 +--
 target/s390x/tcg/translate.c     |  2 ++
 tests/tcg/s390x/Makefile.target  |  1 +
 tests/tcg/s390x/stckf.c          | 44 ++++++++++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/s390x/stckf.c

-- 
2.55.0

[PATCH v2 1/2] target/s390x/tcg: Set STCK/STCKF condition code after the store

[Ilya Leoshkevich]

STORE CLOCK [FAST] to an inaccessible address aborts QEMU:

  $ qemu-s390x ./stckf
  ERROR:cc_helper.c:128:cc_calc_addu: assertion failed: (carry_out <= 1)

op_stck() sets the condition code with gen_op_movi_cc() before the
output operand store, which is deferred to wout_m1_64(). Assigning a
constant condition code discards the lazy CC values, so the optimizer
drops the writes that produced them. When the store then raises an
exception, the instruction is suppressed and
s390x_restore_state_to_opc() reinstates the cc_op recorded at the start
of STCK[F], but cc_src/cc_dst now hold stale values, so the next
condition code evaluation reads garbage.

Fix by performing the store manually. The alternative of not discarding
in gen_op_movi_cc() keeps the inputs live, but results in less optimal
code.

Reported-by: Ido Plat <Ido.Plat1@ibm.com>
Fixes: 434c91a5f4ed ("target-s390: Convert STCK")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/insn-data.h.inc | 4 ++--
 target/s390x/tcg/translate.c     | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 0d5392eac54..1ea72248a6a 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -887,8 +887,8 @@
     C(0xe32f, STRVG,   RXY_a, Z,   la2, r1_o, new, m1_64, rev64, 0)
 
 /* STORE CLOCK */
-    F(0xb205, STCK,    S,     Z,   la2, 0, new, m1_64, stck, 0, IF_IO)
-    F(0xb27c, STCKF,   S,     SCF, la2, 0, new, m1_64, stck, 0, IF_IO)
+    F(0xb205, STCK,    S,     Z,   la2, 0, new, 0, stck, 0, IF_IO)
+    F(0xb27c, STCKF,   S,     SCF, la2, 0, new, 0, stck, 0, IF_IO)
 /* STORE CLOCK EXTENDED */
     F(0xb278, STCKE,   S,     Z,   0, a2, 0, 0, stcke, 0, IF_IO)
 
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 82165ac1ec0..1b6023168bb 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -4108,7 +4108,9 @@ static DisasJumpType op_stap(DisasContext *s, DisasOps *o)
 static DisasJumpType op_stck(DisasContext *s, DisasOps *o)
 {
     gen_helper_stck(o->out, tcg_env);
+    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_BEUQ);
     /* ??? We don't implement clock states.  */
+    /* Set the CC after the store; a suppressed store must preserve it. */
     gen_op_movi_cc(s, 0);
     return DISAS_NEXT;
 }
-- 
2.55.0

[PATCH v2 2/2] tests/tcg/s390x: Test STCKF condition code on a faulting store

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

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 0ca030ded01..6a5bf8b7813 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -50,6 +50,7 @@ TESTS+=cvb
 TESTS+=ts
 TESTS+=ex-smc
 TESTS+=divide-to-integer
+TESTS+=stckf
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/stckf.c b/tests/tcg/s390x/stckf.c
new file mode 100644
index 00000000000..51c8c9df9f8
--- /dev/null
+++ b/tests/tcg/s390x/stckf.c
@@ -0,0 +1,44 @@
+/*
+ * Test that a faulting STORE CLOCK FAST does not clobber the condition code.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+static void handle_sigsegv(int sig, siginfo_t *info, void *ucontext)
+{
+    mcontext_t *mcontext = &((ucontext_t *)ucontext)->uc_mcontext;
+
+    /* The condition code must be the one set by SLGR, not garbage. */
+    _exit(((mcontext->psw.mask >> 44) & 3) == 3 ? EXIT_SUCCESS : EXIT_FAILURE);
+}
+
+int main(void)
+{
+    struct sigaction act = {
+        .sa_sigaction = handle_sigsegv,
+        .sa_flags = SA_SIGINFO,
+    };
+    int err;
+
+    err = sigaction(SIGSEGV, &act, NULL);
+    assert(err == 0);
+
+    asm volatile(
+        "lghi %%r1,100\n"
+        "lghi %%r2,0\n"
+        "clgr %%r1,%%r2\n"  /* CC_OP_LTUGTU_64 */
+                            /* cc_src=100 is not valid for CC_OP_SUBU */
+        "ipm %%r0\n"        /* force cc_src to env */
+        "lghi %%r3,5\n"
+        "lghi %%r4,3\n"
+        "slgr %%r3,%%r4\n"  /* CC_OP_SUBU, cc=3 */
+        "lghi %%r5,0\n"
+        "stckf 0(%%r5)\n"   /* faults; cc must stay 3 */
+        : : : "r0", "r1", "r2", "r3", "r4", "r5", "cc", "memory");
+
+    return EXIT_FAILURE;
+}
-- 
2.55.0