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

[PATCH v2 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).

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

 linux-user/s390x/cpu_loop.c     |  6 ++-
 target/s390x/excp_helper.c      | 69 ++++++++++++++++++---------------
 target/s390x/internal.h         |  1 +
 tests/tcg/s390x/Makefile.target |  1 +
 tests/tcg/s390x/sigill.c        | 52 +++++++++++++++++++++++++
 5 files changed, 96 insertions(+), 33 deletions(-)
 create mode 100644 tests/tcg/s390x/sigill.c

-- 
2.31.1

[PATCH v2 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.

Buglink: https://gitlab.com/qemu-project/qemu/-/issues/319
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 linux-user/s390x/cpu_loop.c |  6 +++-
 target/s390x/excp_helper.c  | 69 ++++++++++++++++++++-----------------
 target/s390x/internal.h     |  1 +
 3 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/linux-user/s390x/cpu_loop.c b/linux-user/s390x/cpu_loop.c
index f2d1215fb1..6f5462d4f8 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,6 +69,7 @@ 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:
@@ -131,7 +135,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..0a323967ae 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)
@@ -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 */
-- 
2.31.1

[PATCH v2 2/2] tests/tcg/s390x: Test SIGILL 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/sigill.c        | 52 +++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 tests/tcg/s390x/sigill.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 241ef28f61..8699d829a5 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+=sigill
diff --git a/tests/tcg/s390x/sigill.c b/tests/tcg/s390x/sigill.c
new file mode 100644
index 0000000000..aab560b30f
--- /dev/null
+++ b/tests/tcg/s390x/sigill.c
@@ -0,0 +1,52 @@
+#include <assert.h>
+#include <signal.h>
+#include <string.h>
+#include <ucontext.h>
+#include <unistd.h>
+
+/*
+ * The labels for the instruction that generates a SIGILL and for the one that
+ * follows it. They could have been defined in a separate .s file, but this
+ * would complicate the build, so use the inline asm instead.
+ */
+
+void expected_si_addr(void);
+void expected_psw_addr(void);
+
+asm(".globl\texpected_si_addr\n"
+    "expected_si_addr:\t.byte\t0x00,0x00\n"
+    "\t.globl\texpected_psw_addr\n"
+    "expected_psw_addr:\tbr\t%r14");
+
+static void handle_signal(int sig, siginfo_t *info, void *ucontext)
+{
+    if (sig != SIGILL) {
+        _exit(1);
+    }
+
+    if (info->si_addr != expected_si_addr) {
+        _exit(2);
+    }
+
+    if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr !=
+            (unsigned long)expected_psw_addr) {
+        _exit(3);
+    }
+}
+
+int main(void)
+{
+    struct sigaction act;
+    int ret;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_sigaction = handle_signal;
+    act.sa_flags = SA_SIGINFO;
+
+    ret = sigaction(SIGILL, &act, NULL);
+    assert(ret == 0);
+
+    expected_si_addr();
+
+    return 0;
+}
-- 
2.31.1

Re: [PATCH v2 2/2] tests/tcg/s390x: Test SIGILL handling

[David Hildenbrand]

On 21.05.21 13:19, 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/sigill.c        | 52 +++++++++++++++++++++++++++++++++
>   2 files changed, 53 insertions(+)
>   create mode 100644 tests/tcg/s390x/sigill.c
> 
> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> index 241ef28f61..8699d829a5 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+=sigill
> diff --git a/tests/tcg/s390x/sigill.c b/tests/tcg/s390x/sigill.c
> new file mode 100644
> index 0000000000..aab560b30f
> --- /dev/null
> +++ b/tests/tcg/s390x/sigill.c
> @@ -0,0 +1,52 @@
> +#include <assert.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <ucontext.h>
> +#include <unistd.h>
> +
> +/*
> + * The labels for the instruction that generates a SIGILL and for the one that
> + * follows it. They could have been defined in a separate .s file, but this
> + * would complicate the build, so use the inline asm instead.
> + */
> +
> +void expected_si_addr(void);
> +void expected_psw_addr(void);
> +
> +asm(".globl\texpected_si_addr\n"
> +    "expected_si_addr:\t.byte\t0x00,0x00\n"
> +    "\t.globl\texpected_psw_addr\n"
> +    "expected_psw_addr:\tbr\t%r14");
> +
> +static void handle_signal(int sig, siginfo_t *info, void *ucontext)
> +{
> +    if (sig != SIGILL) {
> +        _exit(1);
> +    }
> +
> +    if (info->si_addr != expected_si_addr) {
> +        _exit(2);
> +    }
> +
> +    if (((ucontext_t *)ucontext)->uc_mcontext.psw.addr !=
> +            (unsigned long)expected_psw_addr) {
> +        _exit(3);
> +    }
> +}
> +
> +int main(void)
> +{
> +    struct sigaction act;
> +    int ret;
> +
> +    memset(&act, 0, sizeof(act));
> +    act.sa_sigaction = handle_signal;
> +    act.sa_flags = SA_SIGINFO;
> +
> +    ret = sigaction(SIGILL, &act, NULL);
> +    assert(ret == 0);
> +
> +    expected_si_addr();
> +
> +    return 0;
> +}
> 

That makes it much clearer, thanks.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210521111908.2843735-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: 20210521111908.2843735-1-iii@linux.ibm.com
Subject: [PATCH v2 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
   0b5acf8..da9076f  master     -> master
 * [new tag]         patchew/20210521111908.2843735-1-iii@linux.ibm.com -> patchew/20210521111908.2843735-1-iii@linux.ibm.com
Switched to a new branch 'test'
195f303 tests/tcg/s390x: Test SIGILL handling
37f7389 target/s390x: Fix SIGILL psw.addr reporting

=== OUTPUT BEGIN ===
1/2 Checking commit 37f73891cb22 (target/s390x: Fix SIGILL psw.addr reporting)
2/2 Checking commit 195f303f366e (tests/tcg/s390x: Test SIGILL 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
#43: FILE: tests/tcg/s390x/sigill.c:13:
+void expected_si_addr(void);

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

total: 2 errors, 1 warnings, 56 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/20210521111908.2843735-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 v2 0/2] target/s390x: Fix SIGILL psw.addr reporting

[Ilya Leoshkevich]

On Fri, 2021-05-21 at 13:19 +0200, 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).
> 
> Ilya Leoshkevich (2):
>   target/s390x: Fix SIGILL psw.addr reporting
>   tests/tcg/s390x: Test SIGILL handling
> 
>  linux-user/s390x/cpu_loop.c     |  6 ++-
>  target/s390x/excp_helper.c      | 69 ++++++++++++++++++-------------
> --
>  target/s390x/internal.h         |  1 +
>  tests/tcg/s390x/Makefile.target |  1 +
>  tests/tcg/s390x/sigill.c        | 52 +++++++++++++++++++++++++
>  5 files changed, 96 insertions(+), 33 deletions(-)
>  create mode 100644 tests/tcg/s390x/sigill.c

Hi,

Is there anything I need to do to have this merged?

Regarding the style checker warning: I could move the function
declaration to a separate header, but from my perspective this would
make the test less readable.

Best regards,
Ilya

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

[Ilya Leoshkevich]

On Tue, 2021-06-01 at 14:38 +0200, Ilya Leoshkevich wrote:
> On Fri, 2021-05-21 at 13:19 +0200, 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).
> > 
> > Ilya Leoshkevich (2):
> >   target/s390x: Fix SIGILL psw.addr reporting
> >   tests/tcg/s390x: Test SIGILL handling
> > 
> >  linux-user/s390x/cpu_loop.c     |  6 ++-
> >  target/s390x/excp_helper.c      | 69 ++++++++++++++++++-------------
> > --
> >  target/s390x/internal.h         |  1 +
> >  tests/tcg/s390x/Makefile.target |  1 +
> >  tests/tcg/s390x/sigill.c        | 52 +++++++++++++++++++++++++
> >  5 files changed, 96 insertions(+), 33 deletions(-)
> >  create mode 100644 tests/tcg/s390x/sigill.c
> 
> Hi,
> 
> Is there anything I need to do to have this merged?
> 
> Regarding the style checker warning: I could move the function
> declaration to a separate header, but from my perspective this would
> make the test less readable.
> 
> Best regards,
> Ilya

I was just finalizing the similar series for SIGSEGV, when I
realized that the problem that it solves is actually caused by this
one. So please don't merge it yet (in case you were planning to), and
let me send a combined v3.

Best regards,
Ilya