[PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[Ilya Leoshkevich]

qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr: it
should be a pointer to the instruction following the illegal
instruction, but at the moment it is a pointer to the illegal
instruction itself. This breaks OpenJDK, which relies on this value.

Patch 1 fixes the issue, patch 2 adds a test.

v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
v1 -> v2: Use a better buglink (Cornelia), simplify the inline asm
          magic in the test and add an explanation (David).

v2: https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
v2 -> v3: Fix SIGSEGV handling (found when trying to run valgrind under
          qemu-user).

Ilya Leoshkevich (2):
  target/s390x: Fix SIGILL psw.addr reporting
  tests/tcg/s390x: Test SIGILL and SIGSEGV handling

 linux-user/s390x/cpu_loop.c     |  23 ++++-
 target/s390x/excp_helper.c      |  71 +++++++-------
 target/s390x/internal.h         |   1 +
 target/s390x/mem_helper.c       |   2 +-
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/signal.c        | 163 ++++++++++++++++++++++++++++++++
 6 files changed, 224 insertions(+), 37 deletions(-)
 create mode 100644 tests/tcg/s390x/signal.c

-- 
2.31.1

[PATCH v3 1/2] target/s390x: Fix SIGILL psw.addr reporting

[Ilya Leoshkevich]

When a s390x CPU attempts to execute an illegal instruction, an
operation exception is recognized. This is a suppressing exception,
which means that the PSW is advanced by the length of the illegal
instruction.

On the real hardware or in qemu-system-s390x the kernel then raises
SIGILL with si_addr pointing to the suppressed instruction and
psw.addr containing the updated PSW.

Unfortunately qemu-s390x sets both to the address of the suppressed
instruction at the moment. Fix by sharing the PSW advancement logic
with qemu-system-s390x and setting si_addr to the address of the
instruction that raised the exception.

Sharing the PSW advancement logic means, however, that now the helpers
and the CPU loop must follow the hardware and the kernel a little bit
more strictly. In particular:

* Unmapped memory accesses should result in a translation rather than
  in an addressing exception.
* Protection exception must rewind psw.addr.
* Translation exception must cause a SIGSEGV.

While at it, also fix the handling of addressing and privileged
operation exceptions.

Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/s390x/cpu_loop.c | 23 ++++++++++--
 target/s390x/excp_helper.c  | 71 ++++++++++++++++++++-----------------
 target/s390x/internal.h     |  1 +
 target/s390x/mem_helper.c   |  2 +-
 4 files changed, 60 insertions(+), 37 deletions(-)

diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
index f2d1215fb1..72ba9170ed 100644
--- a/linux-user/s390x/cpu_loop.c
+++ b/linux-user/s390x/cpu_loop.c
@@ -21,6 +21,7 @@
 #include "qemu-common.h"
 #include "qemu.h"
 #include "cpu_loop-common.h"
+#include "internal.h"
 
 /* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
 #define S390X_FAIL_ADDR_MASK -4096LL
@@ -29,6 +30,7 @@ void cpu_loop(CPUS390XState *env)
 {
     CPUState *cs = env_cpu(env);
     int trapnr, n, sig;
+    target_ulong excp_psw_addr;
     target_siginfo_t info;
     target_ulong addr;
     abi_long ret;
@@ -38,6 +40,7 @@ void cpu_loop(CPUS390XState *env)
         trapnr = cpu_exec(cs);
         cpu_exec_end(cs);
         process_queued_cpu_work(cs);
+        excp_psw_addr = env->psw.addr;
 
         switch (trapnr) {
         case EXCP_INTERRUPT:
@@ -66,15 +69,29 @@ void cpu_loop(CPUS390XState *env)
             n = TARGET_TRAP_BRKPT;
             goto do_signal_pc;
         case EXCP_PGM:
+            s390_cpu_program_interrupt_advance_psw(env);
             n = env->int_pgm_code;
             switch (n) {
             case PGM_OPERATION:
-            case PGM_PRIVILEGED:
                 sig = TARGET_SIGILL;
                 n = TARGET_ILL_ILLOPC;
                 goto do_signal_pc;
-            case PGM_PROTECTION:
+            case PGM_PRIVILEGED:
+                sig = TARGET_SIGILL;
+                n = TARGET_ILL_PRVOPC;
+                goto do_signal_pc;
             case PGM_ADDRESSING:
+                sig = TARGET_SIGILL;
+                n = TARGET_ILL_ILLADR;
+                goto do_signal_pc;
+            case PGM_PROTECTION:
+                /*
+                 * Protection exception handler is special: it is the only one
+                 * that rewinds psw.addr.
+                 */
+                env->psw.addr = excp_psw_addr;
+                /* FALL THROUGH */
+            case PGM_PAGE_TRANS:
                 sig = TARGET_SIGSEGV;
                 /* XXX: check env->error_code */
                 n = TARGET_SEGV_MAPERR;
@@ -131,7 +148,7 @@ void cpu_loop(CPUS390XState *env)
             break;
 
         do_signal_pc:
-            addr = env->psw.addr;
+            addr = excp_psw_addr;
         do_signal:
             info.si_signo = sig;
             info.si_errno = 0;
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 20625c2c8f..4133e93978 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -82,6 +82,42 @@ void HELPER(data_exception)(CPUS390XState *env, uint32_t dxc)
     tcg_s390_data_exception(env, dxc, GETPC());
 }
 
+void s390_cpu_program_interrupt_advance_psw(CPUS390XState *env)
+{
+    switch (env->int_pgm_code) {
+    case PGM_PER:
+        if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
+            break;
+        }
+        /* FALL THROUGH */
+    case PGM_OPERATION:
+    case PGM_PRIVILEGED:
+    case PGM_EXECUTE:
+    case PGM_PROTECTION:
+    case PGM_ADDRESSING:
+    case PGM_SPECIFICATION:
+    case PGM_DATA:
+    case PGM_FIXPT_OVERFLOW:
+    case PGM_FIXPT_DIVIDE:
+    case PGM_DEC_OVERFLOW:
+    case PGM_DEC_DIVIDE:
+    case PGM_HFP_EXP_OVERFLOW:
+    case PGM_HFP_EXP_UNDERFLOW:
+    case PGM_HFP_SIGNIFICANCE:
+    case PGM_HFP_DIVIDE:
+    case PGM_TRANS_SPEC:
+    case PGM_SPECIAL_OP:
+    case PGM_OPERAND:
+    case PGM_HFP_SQRT:
+    case PGM_PC_TRANS_SPEC:
+    case PGM_ALET_SPEC:
+    case PGM_MONITOR:
+        /* advance the PSW if our exception is not nullifying */
+        env->psw.addr += env->int_pgm_ilen;
+        break;
+    }
+}
+
 #if defined(CONFIG_USER_ONLY)
 
 void s390_cpu_do_interrupt(CPUState *cs)
@@ -95,7 +131,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
 {
     S390CPU *cpu = S390_CPU(cs);
 
-    trigger_pgm_exception(&cpu->env, PGM_ADDRESSING);
+    trigger_pgm_exception(&cpu->env, PGM_PAGE_TRANS);
     /* On real machines this value is dropped into LowMem.  Since this
        is userland, simply put this someplace that cpu_loop can find it.  */
     cpu->env.__excp_addr = address;
@@ -202,38 +238,7 @@ static void do_program_interrupt(CPUS390XState *env)
 
     assert(ilen == 2 || ilen == 4 || ilen == 6);
 
-    switch (env->int_pgm_code) {
-    case PGM_PER:
-        if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) {
-            break;
-        }
-        /* FALL THROUGH */
-    case PGM_OPERATION:
-    case PGM_PRIVILEGED:
-    case PGM_EXECUTE:
-    case PGM_PROTECTION:
-    case PGM_ADDRESSING:
-    case PGM_SPECIFICATION:
-    case PGM_DATA:
-    case PGM_FIXPT_OVERFLOW:
-    case PGM_FIXPT_DIVIDE:
-    case PGM_DEC_OVERFLOW:
-    case PGM_DEC_DIVIDE:
-    case PGM_HFP_EXP_OVERFLOW:
-    case PGM_HFP_EXP_UNDERFLOW:
-    case PGM_HFP_SIGNIFICANCE:
-    case PGM_HFP_DIVIDE:
-    case PGM_TRANS_SPEC:
-    case PGM_SPECIAL_OP:
-    case PGM_OPERAND:
-    case PGM_HFP_SQRT:
-    case PGM_PC_TRANS_SPEC:
-    case PGM_ALET_SPEC:
-    case PGM_MONITOR:
-        /* advance the PSW if our exception is not nullifying */
-        env->psw.addr += ilen;
-        break;
-    }
+    s390_cpu_program_interrupt_advance_psw(env);
 
     qemu_log_mask(CPU_LOG_INT,
                   "%s: code=0x%x ilen=%d psw: %" PRIx64 " %" PRIx64 "\n",
diff --git a/target/s390x/internal.h b/target/s390x/internal.h
index 11515bb617..9f1665ccbf 100644
--- a/target/s390x/internal.h
+++ b/target/s390x/internal.h
@@ -272,6 +272,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
 void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                    MMUAccessType access_type,
                                    int mmu_idx, uintptr_t retaddr);
+void s390_cpu_program_interrupt_advance_psw(CPUS390XState *cpu);
 
 
 /* fpu_helper.c */
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index f6a7d29273..dff19cba60 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -147,7 +147,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
     flags = page_get_flags(addr);
     if (!(flags & (access_type == MMU_DATA_LOAD ?  PAGE_READ : PAGE_WRITE_ORG))) {
         env->__excp_addr = addr;
-        flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING;
+        flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_PAGE_TRANS;
         if (nonfault) {
             return flags;
         }
-- 
2.31.1

[PATCH v3 2/2] tests/tcg/s390x: Test SIGILL and SIGSEGV handling

[Ilya Leoshkevich]

Verify that s390x-specific uc_mcontext.psw.addr is reported correctly.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/signal.c        | 163 ++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 tests/tcg/s390x/signal.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 241ef28f61..cdb7d85316 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -8,3 +8,4 @@ TESTS+=exrl-trtr
 TESTS+=pack
 TESTS+=mvo
 TESTS+=mvc
+TESTS+=signal
diff --git a/tests/tcg/s390x/signal.c b/tests/tcg/s390x/signal.c
new file mode 100644
index 0000000000..c25d7dd019
--- /dev/null
+++ b/tests/tcg/s390x/signal.c
@@ -0,0 +1,163 @@
+#include <assert.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+/*
+ * Various instructions that generate SIGILL and SIGSEGV. They could have been
+ * defined in a separate .s file, but this would complicate the build, so the
+ * inline asm is used instead.
+ */
+
+void illegal_op(void);
+void after_illegal_op(void);
+asm(".globl\tillegal_op\n"
+    "illegal_op:\t.byte\t0x00,0x00\n"
+    "\t.globl\tafter_illegal_op\n"
+    "after_illegal_op:\tbr\t%r14");
+
+void stg(void *dst, unsigned long src);
+asm(".globl\tstg\n"
+    "stg:\tstg\t%r3,0(%r2)\n"
+    "\tbr\t%r14");
+
+void mvc_8(void *dst, void *src);
+asm(".globl\tmvc_8\n"
+    "mvc_8:\tmvc\t0(8,%r2),0(%r3)\n"
+    "\tbr\t%r14");
+
+static void safe_puts(const char *s)
+{
+    write(0, s, strlen(s));
+    write(0, "\n", 1);
+}
+
+enum exception {
+    exception_operation,
+    exception_translation,
+    exception_protection,
+};
+
+static struct {
+    int sig;
+    void *addr;
+    unsigned long psw_addr;
+    enum exception exception;
+} expected;
+
+static void handle_signal(int sig, siginfo_t *info, void *ucontext)
+{
+    void *page;
+    int err;
+
+    if (sig != expected.sig) {
+        safe_puts("[  FAILED  ] wrong signal");
+        _exit(1);
+    }
+
+    if (info->si_addr != expected.addr) {
+        safe_puts("[  FAILED  ] wrong si_addr");
+        _exit(1);
+    }
+
+    if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr != expected.psw_addr) {
+        safe_puts("[  FAILED  ] wrong psw.addr");
+        _exit(1);
+    }
+
+    switch (expected.exception) {
+    case exception_translation:
+        page = mmap(expected.addr, 4096, PROT_READ | PROT_WRITE,
+                    MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
+        if (page != expected.addr) {
+            safe_puts("[  FAILED  ] mmap() failed");
+            _exit(1);
+        }
+        break;
+    case exception_protection:
+        err = mprotect(expected.addr, 4096, PROT_READ | PROT_WRITE);
+        if (err != 0) {
+            safe_puts("[  FAILED  ] mprotect() failed");
+            _exit(1);
+        }
+        break;
+    }
+}
+
+static void check_sigsegv(void *func, enum exception exception,
+                          unsigned long val)
+{
+    int prot;
+    unsigned long *page;
+    unsigned long *addr;
+    int err;
+
+    prot = exception == exception_translation ? PROT_NONE : PROT_READ;
+    page = mmap(NULL, 4096, prot, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    assert(page != MAP_FAILED);
+    if (exception == exception_translation) {
+        /* Hopefully nothing will be mapped at this address. */
+        err = munmap(page, 4096);
+        assert(err == 0);
+    }
+    addr = page + (val & 0x1ff);
+
+    expected.sig = SIGSEGV;
+    expected.addr = page;
+    expected.psw_addr = (unsigned long)func;
+    expected.exception = exception;
+    if (func == stg) {
+        stg(addr, val);
+    } else {
+        assert(func == mvc_8);
+        mvc_8(addr, &val);
+    }
+    assert(*addr == val);
+
+    err = munmap(page, 4096);
+    assert(err == 0);
+}
+
+int main(void)
+{
+    struct sigaction act;
+    int err;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_sigaction = handle_signal;
+    act.sa_flags = SA_SIGINFO;
+    err = sigaction(SIGILL, &act, NULL);
+    assert(err == 0);
+    err = sigaction(SIGSEGV, &act, NULL);
+    assert(err == 0);
+
+    safe_puts("[ RUN      ] Operation exception");
+    expected.sig = SIGILL;
+    expected.addr = illegal_op;
+    expected.psw_addr = (unsigned long)after_illegal_op;
+    expected.exception = exception_operation;
+    illegal_op();
+    safe_puts("[       OK ]");
+
+    safe_puts("[ RUN      ] Translation exception from stg");
+    check_sigsegv(stg, exception_translation, 42);
+    safe_puts("[       OK ]");
+
+    safe_puts("[ RUN      ] Translation exception from mvc");
+    check_sigsegv(mvc_8, exception_translation, 4242);
+    safe_puts("[       OK ]");
+
+    safe_puts("[ RUN      ] Protection exception from stg");
+    check_sigsegv(stg, exception_protection, 424242);
+    safe_puts("[       OK ]");
+
+    safe_puts("[ RUN      ] Protection exception from mvc");
+    check_sigsegv(mvc_8, exception_protection, 42424242);
+    safe_puts("[       OK ]");
+
+    safe_puts("[  PASSED  ]");
+
+    return 0;
+}
-- 
2.31.1

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210602002210.3144559-1-iii@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210602002210.3144559-1-iii@linux.ibm.com
Subject: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1622589584-22571-1-git-send-email-tsimpson@quicinc.com -> patchew/1622589584-22571-1-git-send-email-tsimpson@quicinc.com
 * [new tag]         patchew/20210602002210.3144559-1-iii@linux.ibm.com -> patchew/20210602002210.3144559-1-iii@linux.ibm.com
Switched to a new branch 'test'
495bc5c tests/tcg/s390x: Test SIGILL and SIGSEGV handling
2e28eb9 target/s390x: Fix SIGILL psw.addr reporting

=== OUTPUT BEGIN ===
1/2 Checking commit 2e28eb9526d2 (target/s390x: Fix SIGILL psw.addr reporting)
2/2 Checking commit 495bc5cfd59a (tests/tcg/s390x: Test SIGILL and SIGSEGV handling)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#26: 
new file mode 100644

ERROR: externs should be avoided in .c files
#44: FILE: tests/tcg/s390x/signal.c:14:
+void illegal_op(void);

ERROR: externs should be avoided in .c files
#45: FILE: tests/tcg/s390x/signal.c:15:
+void after_illegal_op(void);

ERROR: externs should be avoided in .c files
#51: FILE: tests/tcg/s390x/signal.c:21:
+void stg(void *dst, unsigned long src);

ERROR: externs should be avoided in .c files
#56: FILE: tests/tcg/s390x/signal.c:26:
+void mvc_8(void *dst, void *src);

total: 4 errors, 1 warnings, 167 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210602002210.3144559-1-iii@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[David Hildenbrand]

On 02.06.21 02:22, Ilya Leoshkevich wrote:
> qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr: it
> should be a pointer to the instruction following the illegal
> instruction, but at the moment it is a pointer to the illegal
> instruction itself. This breaks OpenJDK, which relies on this value.
> 
> Patch 1 fixes the issue, patch 2 adds a test.
> 
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
> v1 -> v2: Use a better buglink (Cornelia), simplify the inline asm
>            magic in the test and add an explanation (David).
> 
> v2: https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
> v2 -> v3: Fix SIGSEGV handling (found when trying to run valgrind under
>            qemu-user).
> 

There might still be something wrong:

https://gitlab.com/qemu-project/qemu/-/issues/319

At least it smells like some more signal (mis)handling.


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[Christian Borntraeger]

On 10.06.21 11:49, David Hildenbrand wrote:
> On 02.06.21 02:22, Ilya Leoshkevich wrote:
>> qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr: it
>> should be a pointer to the instruction following the illegal
>> instruction, but at the moment it is a pointer to the illegal
>> instruction itself. This breaks OpenJDK, which relies on this value.
>>
>> Patch 1 fixes the issue, patch 2 adds a test.
>>
>> v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
>> v1 -> v2: Use a better buglink (Cornelia), simplify the inline asm
>>            magic in the test and add an explanation (David).
>>
>> v2: https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
>> v2 -> v3: Fix SIGSEGV handling (found when trying to run valgrind under
>>            qemu-user).
>>
> 
> There might still be something wrong:
> 
> https://gitlab.com/qemu-project/qemu/-/issues/319
> 
> At least it smells like some more signal (mis)handling.

Yes there is more. Expect a patch (in a different area of signals)
from the ecosystem team soon.

Re: [PATCH v3 2/2] tests/tcg/s390x: Test SIGILL and SIGSEGV handling

[jonathan.albrecht]

On 2021-06-01 8:22 pm, Ilya Leoshkevich wrote:
> Verify that s390x-specific uc_mcontext.psw.addr is reported correctly.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tests/tcg/s390x/Makefile.target |   1 +
>  tests/tcg/s390x/signal.c        | 163 ++++++++++++++++++++++++++++++++
>  2 files changed, 164 insertions(+)
>  create mode 100644 tests/tcg/s390x/signal.c
> 
> diff --git a/tests/tcg/s390x/Makefile.target 
> b/tests/tcg/s390x/Makefile.target
> index 241ef28f61..cdb7d85316 100644
> --- a/tests/tcg/s390x/Makefile.target
> +++ b/tests/tcg/s390x/Makefile.target
> @@ -8,3 +8,4 @@ TESTS+=exrl-trtr
>  TESTS+=pack
>  TESTS+=mvo
>  TESTS+=mvc
> +TESTS+=signal
> diff --git a/tests/tcg/s390x/signal.c b/tests/tcg/s390x/signal.c
> new file mode 100644
> index 0000000000..c25d7dd019
> --- /dev/null
> +++ b/tests/tcg/s390x/signal.c
> @@ -0,0 +1,163 @@
> +#include <assert.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <ucontext.h>
> +#include <unistd.h>
> +
> +/*
> + * Various instructions that generate SIGILL and SIGSEGV. They could 
> have been
> + * defined in a separate .s file, but this would complicate the build, 
> so the
> + * inline asm is used instead.
> + */
> +
> +void illegal_op(void);
> +void after_illegal_op(void);
> +asm(".globl\tillegal_op\n"
> +    "illegal_op:\t.byte\t0x00,0x00\n"
> +    "\t.globl\tafter_illegal_op\n"
> +    "after_illegal_op:\tbr\t%r14");
> +
> +void stg(void *dst, unsigned long src);
> +asm(".globl\tstg\n"
> +    "stg:\tstg\t%r3,0(%r2)\n"
> +    "\tbr\t%r14");
> +
> +void mvc_8(void *dst, void *src);
> +asm(".globl\tmvc_8\n"
> +    "mvc_8:\tmvc\t0(8,%r2),0(%r3)\n"
> +    "\tbr\t%r14");
> +
> +static void safe_puts(const char *s)
> +{
> +    write(0, s, strlen(s));
> +    write(0, "\n", 1);
> +}
> +
> +enum exception {
> +    exception_operation,
> +    exception_translation,
> +    exception_protection,
> +};
> +
> +static struct {
> +    int sig;
> +    void *addr;
> +    unsigned long psw_addr;
> +    enum exception exception;
> +} expected;
> +
> +static void handle_signal(int sig, siginfo_t *info, void *ucontext)
> +{
> +    void *page;
> +    int err;
> +
> +    if (sig != expected.sig) {
> +        safe_puts("[  FAILED  ] wrong signal");
> +        _exit(1);
> +    }
> +
> +    if (info->si_addr != expected.addr) {
> +        safe_puts("[  FAILED  ] wrong si_addr");
> +        _exit(1);
> +    }
> +
> +    if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr != 
> expected.psw_addr) {
> +        safe_puts("[  FAILED  ] wrong psw.addr");
> +        _exit(1);
> +    }
> +
> +    switch (expected.exception) {

When I try to run 'make test-tcg' gcc 9.3.0 is complaining about a 
missing case:
/home/jalbrecht/src/qemu/tests/tcg/s390x/signal.c: In function 
'handle_signal':
/home/jalbrecht/src/qemu/tests/tcg/s390x/signal.c:70:5: error: 
enumeration value 'exception_operation' not handled in switch 
[-Werror=switch]
    70 |     switch (expected.exception) {
       |     ^~~~~~
cc1: all warnings being treated as errors

> +    case exception_translation:
> +        page = mmap(expected.addr, 4096, PROT_READ | PROT_WRITE,
> +                    MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> +        if (page != expected.addr) {
> +            safe_puts("[  FAILED  ] mmap() failed");
> +            _exit(1);
> +        }
> +        break;
> +    case exception_protection:
> +        err = mprotect(expected.addr, 4096, PROT_READ | PROT_WRITE);
> +        if (err != 0) {
> +            safe_puts("[  FAILED  ] mprotect() failed");
> +            _exit(1);
> +        }
> +        break;
> +    }
> +}
> +
> +static void check_sigsegv(void *func, enum exception exception,
> +                          unsigned long val)
> +{
> +    int prot;
> +    unsigned long *page;
> +    unsigned long *addr;
> +    int err;
> +
> +    prot = exception == exception_translation ? PROT_NONE : PROT_READ;
> +    page = mmap(NULL, 4096, prot, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +    assert(page != MAP_FAILED);
> +    if (exception == exception_translation) {
> +        /* Hopefully nothing will be mapped at this address. */
> +        err = munmap(page, 4096);
> +        assert(err == 0);
> +    }
> +    addr = page + (val & 0x1ff);
> +
> +    expected.sig = SIGSEGV;
> +    expected.addr = page;
> +    expected.psw_addr = (unsigned long)func;
> +    expected.exception = exception;
> +    if (func == stg) {
> +        stg(addr, val);
> +    } else {
> +        assert(func == mvc_8);
> +        mvc_8(addr, &val);
> +    }
> +    assert(*addr == val);
> +
> +    err = munmap(page, 4096);
> +    assert(err == 0);
> +}
> +
> +int main(void)
> +{
> +    struct sigaction act;
> +    int err;
> +
> +    memset(&act, 0, sizeof(act));
> +    act.sa_sigaction = handle_signal;
> +    act.sa_flags = SA_SIGINFO;
> +    err = sigaction(SIGILL, &act, NULL);
> +    assert(err == 0);
> +    err = sigaction(SIGSEGV, &act, NULL);
> +    assert(err == 0);
> +
> +    safe_puts("[ RUN      ] Operation exception");
> +    expected.sig = SIGILL;
> +    expected.addr = illegal_op;
> +    expected.psw_addr = (unsigned long)after_illegal_op;
> +    expected.exception = exception_operation;
> +    illegal_op();
> +    safe_puts("[       OK ]");
> +
> +    safe_puts("[ RUN      ] Translation exception from stg");
> +    check_sigsegv(stg, exception_translation, 42);
> +    safe_puts("[       OK ]");
> +
> +    safe_puts("[ RUN      ] Translation exception from mvc");
> +    check_sigsegv(mvc_8, exception_translation, 4242);
> +    safe_puts("[       OK ]");
> +
> +    safe_puts("[ RUN      ] Protection exception from stg");
> +    check_sigsegv(stg, exception_protection, 424242);
> +    safe_puts("[       OK ]");
> +
> +    safe_puts("[ RUN      ] Protection exception from mvc");
> +    check_sigsegv(mvc_8, exception_protection, 42424242);
> +    safe_puts("[       OK ]");
> +
> +    safe_puts("[  PASSED  ]");
> +
> +    return 0;
> +}

Re: [PATCH v3 2/2] tests/tcg/s390x: Test SIGILL and SIGSEGV handling

[Ilya Leoshkevich]

On Fri, 2021-06-18 at 09:47 -0400, jonathan.albrecht wrote:
> On 2021-06-01 8:22 pm, Ilya Leoshkevich wrote:
> > Verify that s390x-specific uc_mcontext.psw.addr is reported
> > correctly.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tests/tcg/s390x/Makefile.target |   1 +
> >  tests/tcg/s390x/signal.c        | 163
> > ++++++++++++++++++++++++++++++++
> >  2 files changed, 164 insertions(+)
> >  create mode 100644 tests/tcg/s390x/signal.c
> > 

[...]

> > +static void handle_signal(int sig, siginfo_t *info, void
> > *ucontext)
> > +{
> > +    void *page;
> > +    int err;
> > +
> > +    if (sig != expected.sig) {
> > +        safe_puts("[  FAILED  ] wrong signal");
> > +        _exit(1);
> > +    }
> > +
> > +    if (info->si_addr != expected.addr) {
> > +        safe_puts("[  FAILED  ] wrong si_addr");
> > +        _exit(1);
> > +    }
> > +
> > +    if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr != 
> > expected.psw_addr) {
> > +        safe_puts("[  FAILED  ] wrong psw.addr");
> > +        _exit(1);
> > +    }
> > +
> > +    switch (expected.exception) {
> 
> When I try to run 'make test-tcg' gcc 9.3.0 is complaining about a 
> missing case:
> /home/jalbrecht/src/qemu/tests/tcg/s390x/signal.c: In function 
> 'handle_signal':
> /home/jalbrecht/src/qemu/tests/tcg/s390x/signal.c:70:5: error: 
> enumeration value 'exception_operation' not handled in switch 
> [-Werror=switch]
>     70 |     switch (expected.exception) {
>        |     ^~~~~~
> cc1: all warnings being treated as errors

I wonder how I didn't catch this, since I'm testing on Ubuntu 20.04
as well. Thank you, I will fix this.

Best regards,
Ilya

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[Ilya Leoshkevich]

On Thu, 2021-06-10 at 11:49 +0200, David Hildenbrand wrote:
> On 02.06.21 02:22, Ilya Leoshkevich wrote:
> > qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr:
> > it
> > should be a pointer to the instruction following the illegal
> > instruction, but at the moment it is a pointer to the illegal
> > instruction itself. This breaks OpenJDK, which relies on this
> > value.
> > 
> > Patch 1 fixes the issue, patch 2 adds a test.
> > 
> > v1:
> > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
> > v1 -> v2: Use a better buglink (Cornelia), simplify the inline asm
> >            magic in the test and add an explanation (David).
> > 
> > v2:
> > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
> > v2 -> v3: Fix SIGSEGV handling (found when trying to run valgrind
> > under
> >            qemu-user).
> > 
> 
> There might still be something wrong:
> 
> https://gitlab.com/qemu-project/qemu/-/issues/319
> 
> At least it smells like some more signal (mis)handling.
> 
> 

I've taken another look, and it must be compare-and-trap SIGFPE/SIGILL
mixup. I think I will just fix it here in v4.

Best regards,
Ilya

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[jonathan.albrecht]

On 2021-06-21 8:00 am, Ilya Leoshkevich wrote:
> On Thu, 2021-06-10 at 11:49 +0200, David Hildenbrand wrote:
>> On 02.06.21 02:22, Ilya Leoshkevich wrote:
>> > qemu-s390x puts a wrong value into SIGILL's siginfo_t's psw.addr:
>> > it
>> > should be a pointer to the instruction following the illegal
>> > instruction, but at the moment it is a pointer to the illegal
>> > instruction itself. This breaks OpenJDK, which relies on this
>> > value.
>> >
>> > Patch 1 fixes the issue, patch 2 adds a test.
>> >
>> > v1:
>> > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
>> > v1 -> v2: Use a better buglink (Cornelia), simplify the inline asm
>> >            magic in the test and add an explanation (David).
>> >
>> > v2:
>> > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
>> > v2 -> v3: Fix SIGSEGV handling (found when trying to run valgrind
>> > under
>> >            qemu-user).
>> >
>> 
>> There might still be something wrong:
>> 
>> https://gitlab.com/qemu-project/qemu/-/issues/319
>> 
>> At least it smells like some more signal (mis)handling.
>> 
>> 
> 
> I've taken another look, and it must be compare-and-trap SIGFPE/SIGILL
> mixup. I think I will just fix it here in v4.

Yes, I've been looking at it too and found it is a compare-and-trap
SIGFPE/SIGILL mixup. I was about to send out a patch if you want
to wait. I should be able to send it out in an hour.

Jon

> 
> Best regards,
> Ilya

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[Ilya Leoshkevich]

On Mon, 2021-06-21 at 09:12 -0400, jonathan.albrecht wrote:
> On 2021-06-21 8:00 am, Ilya Leoshkevich wrote:
> > On Thu, 2021-06-10 at 11:49 +0200, David Hildenbrand wrote:
> > > On 02.06.21 02:22, Ilya Leoshkevich wrote:
> > > > qemu-s390x puts a wrong value into SIGILL's siginfo_t's
> > > > psw.addr:
> > > > it
> > > > should be a pointer to the instruction following the illegal
> > > > instruction, but at the moment it is a pointer to the illegal
> > > > instruction itself. This breaks OpenJDK, which relies on this
> > > > value.
> > > > 
> > > > Patch 1 fixes the issue, patch 2 adds a test.
> > > > 
> > > > v1:
> > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
> > > > v1 -> v2: Use a better buglink (Cornelia), simplify the inline
> > > > asm
> > > >            magic in the test and add an explanation (David).
> > > > 
> > > > v2:
> > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
> > > > v2 -> v3: Fix SIGSEGV handling (found when trying to run
> > > > valgrind
> > > > under
> > > >            qemu-user).
> > > > 
> > > 
> > > There might still be something wrong:
> > > 
> > > https://gitlab.com/qemu-project/qemu/-/issues/319
> > > 
> > > At least it smells like some more signal (mis)handling.
> > > 
> > > 
> > 
> > I've taken another look, and it must be compare-and-trap
> > SIGFPE/SIGILL
> > mixup. I think I will just fix it here in v4.
> 
> Yes, I've been looking at it too and found it is a compare-and-trap
> SIGFPE/SIGILL mixup. I was about to send out a patch if you want
> to wait. I should be able to send it out in an hour.
> 
> Jon

Sure, please go ahead. I'll simply rebase my v4 on top of your patch
then.

Best regards,
Ilya

Re: [PATCH v3 0/2] target/s390x: Fix SIGILL psw.addr reporting

[jonathan.albrecht]

On 2021-06-21 9:44 am, Ilya Leoshkevich wrote:
> On Mon, 2021-06-21 at 09:12 -0400, jonathan.albrecht wrote:
>> On 2021-06-21 8:00 am, Ilya Leoshkevich wrote:
>> > On Thu, 2021-06-10 at 11:49 +0200, David Hildenbrand wrote:
>> > > On 02.06.21 02:22, Ilya Leoshkevich wrote:
>> > > > qemu-s390x puts a wrong value into SIGILL's siginfo_t's
>> > > > psw.addr:
>> > > > it
>> > > > should be a pointer to the instruction following the illegal
>> > > > instruction, but at the moment it is a pointer to the illegal
>> > > > instruction itself. This breaks OpenJDK, which relies on this
>> > > > value.
>> > > >
>> > > > Patch 1 fixes the issue, patch 2 adds a test.
>> > > >
>> > > > v1:
>> > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06592.html
>> > > > v1 -> v2: Use a better buglink (Cornelia), simplify the inline
>> > > > asm
>> > > >            magic in the test and add an explanation (David).
>> > > >
>> > > > v2:
>> > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-05/msg06649.html
>> > > > v2 -> v3: Fix SIGSEGV handling (found when trying to run
>> > > > valgrind
>> > > > under
>> > > >            qemu-user).
>> > > >
>> > >
>> > > There might still be something wrong:
>> > >
>> > > https://gitlab.com/qemu-project/qemu/-/issues/319
>> > >
>> > > At least it smells like some more signal (mis)handling.
>> > >
>> > >
>> >
>> > I've taken another look, and it must be compare-and-trap
>> > SIGFPE/SIGILL
>> > mixup. I think I will just fix it here in v4.
>> 
>> Yes, I've been looking at it too and found it is a compare-and-trap
>> SIGFPE/SIGILL mixup. I was about to send out a patch if you want
>> to wait. I should be able to send it out in an hour.
>> 
>> Jon
> 
> Sure, please go ahead. I'll simply rebase my v4 on top of your patch
> then.
> 
> Best regards,
> Ilya

Yes, please add it to your v4 if it looks ok.

Jon