[PATCH v5 0/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[PATCH v5 0/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP 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.
A similar problem exists for SIGFPE and SIGTRAP.

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).

v3: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg00299.html
v3 -> v4: Fix compiling the test on Ubuntu 20.04 (Jonathan).

v4: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05848.html
v4 -> v5: Greatly simplify the fix (Ulrich).

Note: the compare-and-trap SIGFPE issue is being fixed separately.
https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05690.html

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

 linux-user/s390x/cpu_loop.c     |   5 +
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/signal.c        | 165 ++++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 tests/tcg/s390x/signal.c

-- 
2.31.1

[PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[Ilya Leoshkevich]

For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
instruction, and at the instruction for other signals. Currently under
qemu-user it always points at the instruction.

Fix by advancing psw.addr for these signals.

Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Co-developed-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
---
 linux-user/s390x/cpu_loop.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
index 30568139df..230217feeb 100644
--- a/linux-user/s390x/cpu_loop.c
+++ b/linux-user/s390x/cpu_loop.c
@@ -133,6 +133,11 @@ void cpu_loop(CPUS390XState *env)
 
         do_signal_pc:
             addr = env->psw.addr;
+            /*
+             * For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
+             * instruction.
+             */
+            env->psw.addr += env->int_pgm_ilen;
         do_signal:
             info.si_signo = sig;
             info.si_errno = 0;
-- 
2.31.1

[PATCH v5 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        | 165 ++++++++++++++++++++++++++++++++
 2 files changed, 166 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..37c6665075
--- /dev/null
+++ b/tests/tcg/s390x/signal.c
@@ -0,0 +1,165 @@
+#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;
+    default:
+        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 v5 0/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210623023250.3667563-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: 20210623023250.3667563-1-iii@linux.ibm.com
Subject: [PATCH v5 0/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP 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/20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com -> patchew/20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com
 * [new tag]         patchew/20210623023250.3667563-1-iii@linux.ibm.com -> patchew/20210623023250.3667563-1-iii@linux.ibm.com
Switched to a new branch 'test'
56bc4f3 tests/tcg/s390x: Test SIGILL and SIGSEGV handling
b6b6d39 target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

=== OUTPUT BEGIN ===
1/2 Checking commit b6b6d3978456 (target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting)
2/2 Checking commit 56bc4f3bb893 (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, 169 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/20210623023250.3667563-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 v5 0/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[Cornelia Huck]

On Wed, Jun 23 2021, Ilya Leoshkevich <iii@linux.ibm.com> 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.
> A similar problem exists for SIGFPE and SIGTRAP.
>
> 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).
>
> v3: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg00299.html
> v3 -> v4: Fix compiling the test on Ubuntu 20.04 (Jonathan).
>
> v4: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05848.html
> v4 -> v5: Greatly simplify the fix (Ulrich).
>
> Note: the compare-and-trap SIGFPE issue is being fixed separately.
> https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05690.html
>
> Ilya Leoshkevich (2):
>   target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting
>   tests/tcg/s390x: Test SIGILL and SIGSEGV handling
>
>  linux-user/s390x/cpu_loop.c     |   5 +
>  tests/tcg/s390x/Makefile.target |   1 +
>  tests/tcg/s390x/signal.c        | 165 ++++++++++++++++++++++++++++++++
>  3 files changed, 171 insertions(+)
>  create mode 100644 tests/tcg/s390x/signal.c

What's the status of this and
<20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com>? linux-user
is not really my turf, but it would be sad if this fell through the
cracks.

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

[Laurent Vivier]

Le 02/07/2021 à 12:34, Cornelia Huck a écrit :
> On Wed, Jun 23 2021, Ilya Leoshkevich <iii@linux.ibm.com> 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.
>> A similar problem exists for SIGFPE and SIGTRAP.
>>
>> 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).
>>
>> v3: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg00299.html
>> v3 -> v4: Fix compiling the test on Ubuntu 20.04 (Jonathan).
>>
>> v4: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05848.html
>> v4 -> v5: Greatly simplify the fix (Ulrich).
>>
>> Note: the compare-and-trap SIGFPE issue is being fixed separately.
>> https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05690.html
>>
>> Ilya Leoshkevich (2):
>>   target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting
>>   tests/tcg/s390x: Test SIGILL and SIGSEGV handling
>>
>>  linux-user/s390x/cpu_loop.c     |   5 +
>>  tests/tcg/s390x/Makefile.target |   1 +
>>  tests/tcg/s390x/signal.c        | 165 ++++++++++++++++++++++++++++++++
>>  3 files changed, 171 insertions(+)
>>  create mode 100644 tests/tcg/s390x/signal.c
> 
> What's the status of this and
> <20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com>? linux-user
> is not really my turf, but it would be sad if this fell through the
> cracks.
> 

If from the S390x point of view they are correct, I can collect them via linux-user.

Thanks,
Laurent

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

[Ulrich Weigand]

On Fri, Jul 02, 2021 at 02:01:47PM +0200, Laurent Vivier wrote:
> Le 02/07/2021 à 12:34, Cornelia Huck a écrit :
> > On Wed, Jun 23 2021, Ilya Leoshkevich <iii@linux.ibm.com> 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.
> >> A similar problem exists for SIGFPE and SIGTRAP.
> >>
> >> 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).
> >>
> >> v3: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg00299.html
> >> v3 -> v4: Fix compiling the test on Ubuntu 20.04 (Jonathan).
> >>
> >> v4: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05848.html
> >> v4 -> v5: Greatly simplify the fix (Ulrich).
> >>
> >> Note: the compare-and-trap SIGFPE issue is being fixed separately.
> >> https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05690.html
> >>
> >> Ilya Leoshkevich (2):
> >>   target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting
> >>   tests/tcg/s390x: Test SIGILL and SIGSEGV handling
> >>
> >>  linux-user/s390x/cpu_loop.c     |   5 +
> >>  tests/tcg/s390x/Makefile.target |   1 +
> >>  tests/tcg/s390x/signal.c        | 165 ++++++++++++++++++++++++++++++++
> >>  3 files changed, 171 insertions(+)
> >>  create mode 100644 tests/tcg/s390x/signal.c
> > 
> > What's the status of this and
> > <20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com>? linux-user
> > is not really my turf, but it would be sad if this fell through the
> > cracks.
> > 
> 
> If from the S390x point of view they are correct, I can collect them via linux-user.

It's certainly correct that SIGILL, SIGFPE and SIGTRAP are delivered with psw.addr
pointing *after* the faulting instruction, that forms in effect part of the kernel
ABI on s390x.  We're planning to document this in the next revision of the ABI
document, see here: https://github.com/IBM/s390x-abi/issues/2

I can also confirm that this patch fixes the problems I was seeing when running
the s390x wasmtime JIT under qemu.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[Cornelia Huck]

On Fri, Jul 02 2021, Laurent Vivier <laurent@vivier.eu> wrote:

> Le 02/07/2021 à 12:34, Cornelia Huck a écrit :
>> On Wed, Jun 23 2021, Ilya Leoshkevich <iii@linux.ibm.com> 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.
>>> A similar problem exists for SIGFPE and SIGTRAP.
>>>
>>> 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).
>>>
>>> v3: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg00299.html
>>> v3 -> v4: Fix compiling the test on Ubuntu 20.04 (Jonathan).
>>>
>>> v4: https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05848.html
>>> v4 -> v5: Greatly simplify the fix (Ulrich).
>>>
>>> Note: the compare-and-trap SIGFPE issue is being fixed separately.
>>> https://lists.nongnu.org/archive/html/qemu-devel/2021-06/msg05690.html
>>>
>>> Ilya Leoshkevich (2):
>>>   target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting
>>>   tests/tcg/s390x: Test SIGILL and SIGSEGV handling
>>>
>>>  linux-user/s390x/cpu_loop.c     |   5 +
>>>  tests/tcg/s390x/Makefile.target |   1 +
>>>  tests/tcg/s390x/signal.c        | 165 ++++++++++++++++++++++++++++++++
>>>  3 files changed, 171 insertions(+)
>>>  create mode 100644 tests/tcg/s390x/signal.c
>> 
>> What's the status of this and
>> <20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com>? linux-user
>> is not really my turf, but it would be sad if this fell through the
>> cracks.
>> 
>
> If from the S390x point of view they are correct, I can collect them via linux-user.

Thanks!

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[David Hildenbrand]

On 23.06.21 04:32, Ilya Leoshkevich wrote:
> For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
> instruction, and at the instruction for other signals. Currently under
> qemu-user it always points at the instruction.
> 
> Fix by advancing psw.addr for these signals.
> 
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Co-developed-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
> ---
>   linux-user/s390x/cpu_loop.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
> index 30568139df..230217feeb 100644
> --- a/linux-user/s390x/cpu_loop.c
> +++ b/linux-user/s390x/cpu_loop.c
> @@ -133,6 +133,11 @@ void cpu_loop(CPUS390XState *env)
>   
>           do_signal_pc:
>               addr = env->psw.addr;
> +            /*
> +             * For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
> +             * instruction.
> +             */
> +            env->psw.addr += env->int_pgm_ilen;

We also reach this path via EXCP_DEBUG. How can we expect 
env->int_pgm_ilen to contain something sensible in that case?


-- 
Thanks,

David / dhildenb

Re: [PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[Ilya Leoshkevich]

On Mon, 2021-07-05 at 11:36 +0200, David Hildenbrand wrote:
> On 23.06.21 04:32, Ilya Leoshkevich wrote:
> > For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
> > instruction, and at the instruction for other signals. Currently
> > under
> > qemu-user it always points at the instruction.
> > 
> > Fix by advancing psw.addr for these signals.
> > 
> > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > Co-developed-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
> > ---
> >   linux-user/s390x/cpu_loop.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/linux-user/s390x/cpu_loop.c b/linux-
> > user/s390x/cpu_loop.c
> > index 30568139df..230217feeb 100644
> > --- a/linux-user/s390x/cpu_loop.c
> > +++ b/linux-user/s390x/cpu_loop.c
> > @@ -133,6 +133,11 @@ void cpu_loop(CPUS390XState *env)
> >   
> >           do_signal_pc:
> >               addr = env->psw.addr;
> > +            /*
> > +             * For SIGILL, SIGFPE and SIGTRAP the PSW must point
> > after the
> > +             * instruction.
> > +             */
> > +            env->psw.addr += env->int_pgm_ilen;
> 
> We also reach this path via EXCP_DEBUG. How can we expect 
> env->int_pgm_ilen to contain something sensible in that case?

You are right, this breaks breakpoints after getting any PGM exception
(they turn into "Program received signal SIGTRAP, Trace/breakpoint
trap." instead of the usual "Breakpoint N").

We don't need a PSW rewind here, since it's already incremented
throught the following sequence:

1) GDB asks QEMU to set a breakpoint using a $Z0 packet.
2) translator_loop() notices the breakpoint and calls
   s390x_tr_breakpoint_check().
3) s390x_tr_breakpoint_check() sets DisasContextBase.is_jmp to
   DISAS_PC_STALE and increments DisasContextBase.pc_next by 2.
4) translator_loop() calls s390x_tr_tb_stop().
5) s390x_tr_tb_stop() calls update_psw_addr(), so the JITed code
   increments the PSWA by 2 as well.
6) s390x_tr_tb_stop() calls gen_exception(EXCP_DEBUG).

What do you think about the following amend?

--- a/linux-user/s390x/cpu_loop.c
+++ b/linux-user/s390x/cpu_loop.c
@@ -64,7 +64,13 @@ void cpu_loop(CPUS390XState *env)
         case EXCP_DEBUG:
             sig = TARGET_SIGTRAP;
             n = TARGET_TRAP_BRKPT;
-            goto do_signal_pc;
+            /*
+             * For SIGTRAP the PSW must point after the instruction,
which it
+             * already does thanks to s390x_tr_tb_stop(). si_addr
doesn't need
+             * to be filled.
+             */
+            addr = 0;
+            goto do_signal;
         case EXCP_PGM:
             n = env->int_pgm_code;
             switch (n) {
@@ -134,8 +140,7 @@ void cpu_loop(CPUS390XState *env)
         do_signal_pc:
             addr = env->psw.addr;
             /*
-             * For SIGILL, SIGFPE and SIGTRAP the PSW must point after
the
-             * instruction.
+             * For SIGILL and SIGFPE the PSW must point after the
instruction.
              */
             env->psw.addr += env->int_pgm_ilen;
         do_signal:

Best regards,
Ilya

Re: [PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[David Hildenbrand]

On 05.07.21 19:24, Ilya Leoshkevich wrote:
> On Mon, 2021-07-05 at 11:36 +0200, David Hildenbrand wrote:
>> On 23.06.21 04:32, Ilya Leoshkevich wrote:
>>> For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
>>> instruction, and at the instruction for other signals. Currently
>>> under
>>> qemu-user it always points at the instruction.
>>>
>>> Fix by advancing psw.addr for these signals.
>>>
>>> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> Co-developed-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
>>> ---
>>>    linux-user/s390x/cpu_loop.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/linux-user/s390x/cpu_loop.c b/linux-
>>> user/s390x/cpu_loop.c
>>> index 30568139df..230217feeb 100644
>>> --- a/linux-user/s390x/cpu_loop.c
>>> +++ b/linux-user/s390x/cpu_loop.c
>>> @@ -133,6 +133,11 @@ void cpu_loop(CPUS390XState *env)
>>>    
>>>            do_signal_pc:
>>>                addr = env->psw.addr;
>>> +            /*
>>> +             * For SIGILL, SIGFPE and SIGTRAP the PSW must point
>>> after the
>>> +             * instruction.
>>> +             */
>>> +            env->psw.addr += env->int_pgm_ilen;
>>
>> We also reach this path via EXCP_DEBUG. How can we expect
>> env->int_pgm_ilen to contain something sensible in that case?
> 
> You are right, this breaks breakpoints after getting any PGM exception
> (they turn into "Program received signal SIGTRAP, Trace/breakpoint
> trap." instead of the usual "Breakpoint N").
> 
> We don't need a PSW rewind here, since it's already incremented
> throught the following sequence:
> 
> 1) GDB asks QEMU to set a breakpoint using a $Z0 packet.
> 2) translator_loop() notices the breakpoint and calls
>     s390x_tr_breakpoint_check().
> 3) s390x_tr_breakpoint_check() sets DisasContextBase.is_jmp to
>     DISAS_PC_STALE and increments DisasContextBase.pc_next by 2.
> 4) translator_loop() calls s390x_tr_tb_stop().
> 5) s390x_tr_tb_stop() calls update_psw_addr(), so the JITed code
>     increments the PSWA by 2 as well.
> 6) s390x_tr_tb_stop() calls gen_exception(EXCP_DEBUG).
> 
> What do you think about the following amend?
> 
> --- a/linux-user/s390x/cpu_loop.c
> +++ b/linux-user/s390x/cpu_loop.c
> @@ -64,7 +64,13 @@ void cpu_loop(CPUS390XState *env)
>           case EXCP_DEBUG:
>               sig = TARGET_SIGTRAP;
>               n = TARGET_TRAP_BRKPT;
> -            goto do_signal_pc;
> +            /*
> +             * For SIGTRAP the PSW must point after the instruction,
> which it
> +             * already does thanks to s390x_tr_tb_stop(). si_addr
> doesn't need
> +             * to be filled.
> +             */
> +            addr = 0;
> +            goto do_signal;

Looks better to me, but I'm not an expert on signals, so I cannot tell 
what si_addr is supposed to contain in that case.

-- 
Thanks,

David / dhildenb

Re: [PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[Ilya Leoshkevich]

On Mon, 2021-07-05 at 21:18 +0200, David Hildenbrand wrote:
> On 05.07.21 19:24, Ilya Leoshkevich wrote:
> > On Mon, 2021-07-05 at 11:36 +0200, David Hildenbrand wrote:
> > > On 23.06.21 04:32, Ilya Leoshkevich wrote:
> > > > For SIGILL, SIGFPE and SIGTRAP the PSW must point after the
> > > > instruction, and at the instruction for other signals. Currently
> > > > under
> > > > qemu-user it always points at the instruction.
> > > > 
> > > > Fix by advancing psw.addr for these signals.
> > > > 
> > > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > Co-developed-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
> > > > ---
> > > >    linux-user/s390x/cpu_loop.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/linux-user/s390x/cpu_loop.c b/linux-
> > > > user/s390x/cpu_loop.c
> > > > index 30568139df..230217feeb 100644
> > > > --- a/linux-user/s390x/cpu_loop.c
> > > > +++ b/linux-user/s390x/cpu_loop.c
> > > > @@ -133,6 +133,11 @@ void cpu_loop(CPUS390XState *env)
> > > >    
> > > >            do_signal_pc:
> > > >                addr = env->psw.addr;
> > > > +            /*
> > > > +             * For SIGILL, SIGFPE and SIGTRAP the PSW must point
> > > > after the
> > > > +             * instruction.
> > > > +             */
> > > > +            env->psw.addr += env->int_pgm_ilen;
> > > 
> > > We also reach this path via EXCP_DEBUG. How can we expect
> > > env->int_pgm_ilen to contain something sensible in that case?
> > 
> > You are right, this breaks breakpoints after getting any PGM
> > exception
> > (they turn into "Program received signal SIGTRAP, Trace/breakpoint
> > trap." instead of the usual "Breakpoint N").
> > 
> > We don't need a PSW rewind here, since it's already incremented
> > throught the following sequence:
> > 
> > 1) GDB asks QEMU to set a breakpoint using a $Z0 packet.
> > 2) translator_loop() notices the breakpoint and calls
> >     s390x_tr_breakpoint_check().
> > 3) s390x_tr_breakpoint_check() sets DisasContextBase.is_jmp to
> >     DISAS_PC_STALE and increments DisasContextBase.pc_next by 2.
> > 4) translator_loop() calls s390x_tr_tb_stop().
> > 5) s390x_tr_tb_stop() calls update_psw_addr(), so the JITed code
> >     increments the PSWA by 2 as well.
> > 6) s390x_tr_tb_stop() calls gen_exception(EXCP_DEBUG).
> > 
> > What do you think about the following amend?
> > 
> > --- a/linux-user/s390x/cpu_loop.c
> > +++ b/linux-user/s390x/cpu_loop.c
> > @@ -64,7 +64,13 @@ void cpu_loop(CPUS390XState *env)
> >           case EXCP_DEBUG:
> >               sig = TARGET_SIGTRAP;
> >               n = TARGET_TRAP_BRKPT;
> > -            goto do_signal_pc;
> > +            /*
> > +             * For SIGTRAP the PSW must point after the instruction,
> > which it
> > +             * already does thanks to s390x_tr_tb_stop(). si_addr
> > doesn't need
> > +             * to be filled.
> > +             */
> > +            addr = 0;
> > +            goto do_signal;
> 
> Looks better to me, but I'm not an expert on signals, so I cannot tell 
> what si_addr is supposed to contain in that case.
> 

Thanks, I'll send a v6 then. I used rt_sigaction(2) man here:

    When SIGTRAP is delivered in response to a ptrace(2) event
    (PTRACE_EVENT_foo), si_addr is not populated

I think EXCP_DEBUG corresponds only to this case - there doesn't
seem to be a way to generate it without attaching gdb.

Re: [PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting

[Ulrich Weigand]

On Mon, Jul 05, 2021 at 10:19:56PM +0200, Ilya Leoshkevich wrote:
> On Mon, 2021-07-05 at 21:18 +0200, David Hildenbrand wrote:
> > 
> > Looks better to me, but I'm not an expert on signals, so I cannot tell 
> > what si_addr is supposed to contain in that case.
> > 
> 
> Thanks, I'll send a v6 then. I used rt_sigaction(2) man here:
> 
>     When SIGTRAP is delivered in response to a ptrace(2) event
>     (PTRACE_EVENT_foo), si_addr is not populated
> 
> I think EXCP_DEBUG corresponds only to this case - there doesn't
> seem to be a way to generate it without attaching gdb.

The s390x Linux kernel does in fact set si_addr to the address of
the instruction triggering the signal for SIGTRAP, just like for
SIGILL or SIGFPE.  On the other hand, GDB does not rely on that
(since this is not the case on other platforms, like the man page
above indicates), so this looks OK to me.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

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

[jonathan.albrecht]

On 2021-07-02 8:01 am, Laurent Vivier wrote:
> Le 02/07/2021 à 12:34, Cornelia Huck a écrit :
>> On Wed, Jun 23 2021, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>> 
>> What's the status of this and
>> <20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com>? 
>> linux-user
>> is not really my turf, but it would be sad if this fell through the
>> cracks.
>> 
> 
> If from the S390x point of view they are correct, I can collect them
> via linux-user.
> 

IIUC, the latest versions of these two series should be ready now:

https://lore.kernel.org/qemu-devel/20210705210434.45824-1-iii@linux.ibm.com/
https://lore.kernel.org/qemu-devel/20210709160459.4962-1-jonathan.albrecht@linux.vnet.ibm.com/

Can they still get into 6.1?

Thanks,

Jon

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

[Ilya Leoshkevich]

On Mon, 2021-07-12 at 10:59 -0400, jonathan.albrecht wrote:
> On 2021-07-02 8:01 am, Laurent Vivier wrote:
> > Le 02/07/2021 à 12:34, Cornelia Huck a écrit :
> > > On Wed, Jun 23 2021, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> > > 
> > > 
> > > What's the status of this and
> > > <20210621141452.2045-1-jonathan.albrecht@linux.vnet.ibm.com>? 
> > > linux-user
> > > is not really my turf, but it would be sad if this fell through the
> > > cracks.
> > > 
> > 
> > If from the S390x point of view they are correct, I can collect them
> > via linux-user.
> > 
> 
> IIUC, the latest versions of these two series should be ready now:
> 
> https://lore.kernel.org/qemu-devel/20210705210434.45824-1-iii@linux.ibm.com/
> https://lore.kernel.org/qemu-devel/20210709160459.4962-1-jonathan.albrecht@linux.vnet.ibm.com/
> 
> Can they still get into 6.1?
> 
> Thanks,
> 
> Jon

The test patch from my v6 series hasn't been reviewed yet:

https://lore.kernel.org/qemu-devel/20210705210434.45824-3-iii@linux.ibm.com/

Could anyone have a look, please?

Best regards,
Ilya