qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Hildenbrand
Subject: Re: [PATCH v5 1/2] target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting
Date: Mon, 5 Jul 2021 21:18:59 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]