| Message ID | 20210623023250.3667563-2-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/s390x: Fix SIGILL/SIGFPE/SIGTRAP psw.addr reporting | expand |
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?
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
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.
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.
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
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;
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(+)