[PATCH 0/4] target/s390x: CC fixes

[PATCH 0/4] target/s390x: CC fixes

[Ilya Leoshkevich]

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 and 4
are tests.

Best regards,
Ilya

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

Ilya Leoshkevich (4):
  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

 target/s390x/tcg/translate.c    | 10 ++++---
 tests/tcg/s390x/Makefile.target |  2 ++
 tests/tcg/s390x/clc.c           | 48 +++++++++++++++++++++++++++++++++
 tests/tcg/s390x/laalg.c         | 27 +++++++++++++++++++
 4 files changed, 84 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/s390x/clc.c
 create mode 100644 tests/tcg/s390x/laalg.c

-- 
2.41.0

[PATCH 1/4] 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
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 2/4] 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 3/4] target/s390x: Fix LAALG not updating cc_src

[Ilya Leoshkevich]

LAALG uses op_laa() and cout_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 setting cc_src in op_laa().

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/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index a0d6a2a35dd..db386ea94d8 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2684,7 +2684,8 @@ static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
     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);
+    tcg_gen_movi_i64(cc_src, 0);
+    tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
     return DISAS_NEXT;
 }
 
-- 
2.41.0

[PATCH 4/4] 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

Re: [PATCH 0/4] target/s390x: CC fixes

[David Hildenbrand]

On 31.10.23 06:32, Ilya Leoshkevich wrote:
> 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

I started coding that up but was distracted by other things; last time I 
looked at that, I concluded that the way we are calculating the carry in 
not suitable when we're doing two additions (like ADD LOGICAL WITH CARRY).

In addition, the way cout_addu32/cout_addu64 relies on cc_src/cc_dst is 
wrong in some occurrences.

> are targeted fixes: patch 1 helps with installing Fedora, and patch 3
> addresses something I noticed when reviewing the code. Patches 2 and 4
> are tests.

I assume you are only scratching the surface with your fixes and we 
might be better off just doing CC_OP_ADDU / CC_OP_SUBU like CC_OP_ADD_32 
/ CC_OP_ADD_64 / ... to fix all issues involved.

> 
> Best regards,
> Ilya
> 
> [1] https://gitlab.com/qemu-project/qemu/-/issues/1865#note_1545060658
> 
> Ilya Leoshkevich (4):
>    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
> 
>   target/s390x/tcg/translate.c    | 10 ++++---
>   tests/tcg/s390x/Makefile.target |  2 ++
>   tests/tcg/s390x/clc.c           | 48 +++++++++++++++++++++++++++++++++
>   tests/tcg/s390x/laalg.c         | 27 +++++++++++++++++++
>   4 files changed, 84 insertions(+), 3 deletions(-)
>   create mode 100644 tests/tcg/s390x/clc.c
>   create mode 100644 tests/tcg/s390x/laalg.c
> 

-- 
Cheers,

David / dhildenb

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

[Richard Henderson]

On 10/30/23 22:32, 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
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/translate.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)

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


r~

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

[Richard Henderson]

On 10/30/23 22:32, Ilya Leoshkevich wrote:
> +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");
> +

You could just as easily set cc based on CHI or something to avoid hard-coding r0, or even 
clobbering an output register at all.

But I guess there's little point bike shedding this too much...

r~

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

[Richard Henderson]

On 10/30/23 22:32, Ilya Leoshkevich wrote:
> LAALG uses op_laa() and cout_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 setting cc_src in op_laa().
> 
> 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/translate.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index a0d6a2a35dd..db386ea94d8 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -2684,7 +2684,8 @@ static DisasJumpType op_laa(DisasContext *s, DisasOps *o)
>       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);
> +    tcg_gen_movi_i64(cc_src, 0);
> +    tcg_gen_add2_i64(o->out, cc_src, o->in1, cc_src, o->in2, cc_src);
>       return DISAS_NEXT;
>   }
>   

This is wrong, or at least not ideal -- op_laa is not *only* used with cout_addu64.
I think this would be better with a second version of op_laa.


r~

Re: [PATCH 0/4] target/s390x: CC fixes

[Ilya Leoshkevich]

On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote:
> On 31.10.23 06:32, Ilya Leoshkevich wrote:
> > 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
> 
> I started coding that up but was distracted by other things; last
> time I 
> looked at that, I concluded that the way we are calculating the carry
> in 
> not suitable when we're doing two additions (like ADD LOGICAL WITH
> CARRY).

Do you per chance remember any details? IIUC the code in question is:

static DisasJumpType op_addc64(DisasContext *s, DisasOps *o)
{
    compute_carry(s);

    TCGv_i64 zero = tcg_constant_i64(0);
    tcg_gen_add2_i64(o->out, cc_src, o->in1, zero, cc_src, zero);
    tcg_gen_add2_i64(o->out, cc_src, o->out, cc_src, o->in2, zero);

    return DISAS_NEXT;
}

This looks correct to me, because the 128-bit result of the first
addition is passed fully to the second addition, and the upper half is
always either 0 or 1.

I played with chaining ADD-LOGICAL-WITH-CARRYs with various inputs, and
could not produce any wrong calculations in the emulation.

Best regards,
Ilya

[...]

Re: [PATCH 0/4] target/s390x: CC fixes

[David Hildenbrand]

On 03.11.23 17:44, Ilya Leoshkevich wrote:
> On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote:
>> On 31.10.23 06:32, Ilya Leoshkevich wrote:
>>> 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
>>
>> I started coding that up but was distracted by other things; last
>> time I
>> looked at that, I concluded that the way we are calculating the carry
>> in
>> not suitable when we're doing two additions (like ADD LOGICAL WITH
>> CARRY).
> 
> Do you per chance remember any details? IIUC the code in question is:

Unfortunately, I don't. I thought there would be a case where we could 
overflow twice, and result in a carry value of 2. Or some other weird 
corner case where the result would not be expressive.

Maybe I was daydreaming, let me see if I can re-discover what I found 
(should have taken notes but was just briefly looking at this).

If not, your fixes might be just good enough.

-- 
Cheers,

David / dhildenb

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

[Ilya Leoshkevich]

On Tue, 2023-10-31 at 15:53 -0700, Richard Henderson wrote:
> On 10/30/23 22:32, Ilya Leoshkevich wrote:
> > +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");
> > +
> 
> You could just as easily set cc based on CHI or something to avoid
> hard-coding r0, or even 
> clobbering an output register at all.

The point of hardcoding r0 is rather to be able to check its value in
handle_sigsegv(). While this was not buggy, I still wanted to make sure
that the updated value is "committed" despite SEGV.

> 
> But I guess there's little point bike shedding this too much...
> 
> r~