[PATCH v3 0/3] Fix BRASL and BRCL with large negative offsets

[PATCH v3 0/3] Fix BRASL and BRCL with large negative offsets

[Ilya Leoshkevich]

Hi,

I noticed that sometimes jumping backwards leads to crashes or hangs.
The problem is a missing cast.
Patches 1 and 2 fix the problem, patch 3 adds a test.


v1: https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03356.html
v1 -> v2:
- Skip the test if mmap() fails (Richard).
- Replace test opcodes with inline asm (David). Since we now want to
  skip the test if the code cannot be mapped (e.g. on a 31-bit host),
  we shouldn't be asking the loader to map the code right away. So
  the mmap() approach stays.

v2: https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03387.html
v2 -> v3:
- Use MAP_NORESERVE (Christian / David).
- Unhardcode 0x100000006 (Richard).

Best regards,
Ilya

Ilya Leoshkevich (3):
  s390x/tcg: Fix BRASL with a large negative offset
  s390x/tcg: Fix BRCL with a large negative offset
  tests/tcg/s390x: Test BRASL and BRCL with large negative offsets

 target/s390x/tcg/translate.c           |  4 +-
 tests/tcg/s390x/Makefile.target        |  1 +
 tests/tcg/s390x/branch-relative-long.c | 68 ++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/s390x/branch-relative-long.c

-- 
2.35.1

[PATCH v3 1/3] s390x/tcg: Fix BRASL with a large negative offset

[Ilya Leoshkevich]

When RI2 is 0x80000000, qemu enters an infinite loop instead of jumping
backwards. Fix by adding a missing cast, like in in2_ri2().

Fixes: 8ac33cdb8bfb ("Convert BRANCH AND SAVE")
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 904b51542f..41c8696185 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1597,7 +1597,7 @@ static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
 static DisasJumpType op_basi(DisasContext *s, DisasOps *o)
 {
     pc_to_link_info(o->out, s, s->pc_tmp);
-    return help_goto_direct(s, s->base.pc_next + 2 * get_field(s, i2));
+    return help_goto_direct(s, s->base.pc_next + (int64_t)get_field(s, i2) * 2);
 }
 
 static DisasJumpType op_bc(DisasContext *s, DisasOps *o)
-- 
2.35.1

[PATCH v3 2/3] s390x/tcg: Fix BRCL with a large negative offset

[Ilya Leoshkevich]

When RI2 is 0x80000000, qemu enters an infinite loop instead of jumping
backwards. Fix by adding a missing cast, like in in2_ri2().

Fixes: 7233f2ed1717 ("target-s390: Convert BRANCH ON CONDITION")
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 41c8696185..5acfc0ff9b 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1201,7 +1201,7 @@ static DisasJumpType help_branch(DisasContext *s, DisasCompare *c,
                                  bool is_imm, int imm, TCGv_i64 cdest)
 {
     DisasJumpType ret;
-    uint64_t dest = s->base.pc_next + 2 * imm;
+    uint64_t dest = s->base.pc_next + (int64_t)imm * 2;
     TCGLabel *lab;
 
     /* Take care of the special cases first.  */
-- 
2.35.1

[PATCH v3 3/3] tests/tcg/s390x: Test BRASL and BRCL with large negative offsets

[Ilya Leoshkevich]

Add a small test in order to prevent regressions.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target        |  1 +
 tests/tcg/s390x/branch-relative-long.c | 68 ++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)
 create mode 100644 tests/tcg/s390x/branch-relative-long.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 257c568c58..fd34b130f7 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -15,6 +15,7 @@ TESTS+=mvc
 TESTS+=shift
 TESTS+=trap
 TESTS+=signals-s390x
+TESTS+=branch-relative-long
 
 ifneq ($(HAVE_GDB_BIN),)
 GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
diff --git a/tests/tcg/s390x/branch-relative-long.c b/tests/tcg/s390x/branch-relative-long.c
new file mode 100644
index 0000000000..94219afcad
--- /dev/null
+++ b/tests/tcg/s390x/branch-relative-long.c
@@ -0,0 +1,68 @@
+#include <stddef.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+
+#define DEFINE_ASM(_name, _code) \
+    extern const char _name[]; \
+    extern const char _name ## _end[]; \
+    asm("    .globl " #_name "\n" \
+        #_name ":\n" \
+        "    " _code "\n" \
+        "    .globl " #_name "_end\n" \
+        #_name "_end:\n");
+
+DEFINE_ASM(br_r14, "br %r14");
+DEFINE_ASM(brasl_r0, "brasl %r0,.-0x100000000");
+DEFINE_ASM(brcl_0xf, "brcl 0xf,.-0x100000000");
+
+struct test {
+    const char *code;
+    const char *code_end;
+};
+
+static const struct test tests[] = {
+    {
+        .code = brasl_r0,
+        .code_end = brasl_r0_end,
+    },
+    {
+        .code = brcl_0xf,
+        .code_end = brcl_0xf_end,
+    },
+};
+
+int main(void)
+{
+    unsigned char *buf;
+    size_t length = 0;
+    size_t i;
+
+    for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
+        size_t test_length = 0x100000000 + (tests[i].code_end - tests[i].code);
+
+        if (test_length > length) {
+            length = test_length;
+        }
+    }
+
+    buf = mmap(NULL, length, PROT_READ | PROT_WRITE | PROT_EXEC,
+               MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
+    if (buf == MAP_FAILED) {
+        perror("SKIP: mmap() failed");
+        return 0;
+    }
+
+    memcpy(buf, br_r14, br_r14_end - br_r14);
+    for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
+        void (*code)(void) = (void *)(buf + 0x100000000);
+
+        memcpy(code, tests[i].code, tests[i].code_end - tests[i].code);
+        code();
+        memset(code, 0, tests[i].code_end - tests[i].code);
+    }
+
+    munmap(buf, length);
+
+    return 0;
+}
-- 
2.35.1

Re: [PATCH v3 0/3] Fix BRASL and BRCL with large negative offsets

[Thomas Huth]

On 14/03/2022 11.42, Ilya Leoshkevich wrote:
> Hi,
> 
> I noticed that sometimes jumping backwards leads to crashes or hangs.
> The problem is a missing cast.
> Patches 1 and 2 fix the problem, patch 3 adds a test.
> 
> 
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03356.html
> v1 -> v2:
> - Skip the test if mmap() fails (Richard).
> - Replace test opcodes with inline asm (David). Since we now want to
>    skip the test if the code cannot be mapped (e.g. on a 31-bit host),
>    we shouldn't be asking the loader to map the code right away. So
>    the mmap() approach stays.
> 
> v2: https://lists.nongnu.org/archive/html/qemu-devel/2022-03/msg03387.html
> v2 -> v3:
> - Use MAP_NORESERVE (Christian / David).
> - Unhardcode 0x100000006 (Richard).
> 
> Best regards,
> Ilya
> 
> Ilya Leoshkevich (3):
>    s390x/tcg: Fix BRASL with a large negative offset
>    s390x/tcg: Fix BRCL with a large negative offset
>    tests/tcg/s390x: Test BRASL and BRCL with large negative offsets
> 
>   target/s390x/tcg/translate.c           |  4 +-
>   tests/tcg/s390x/Makefile.target        |  1 +
>   tests/tcg/s390x/branch-relative-long.c | 68 ++++++++++++++++++++++++++
>   3 files changed, 71 insertions(+), 2 deletions(-)
>   create mode 100644 tests/tcg/s390x/branch-relative-long.c

Thanks, queued to my s390x-next branch now:

https://gitlab.com/thuth/qemu/-/commits/s390x-next/

  Thomas