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 <i...@linux.ibm.com>
> > Co-developed-by: Ulrich Weigand <ulrich.weig...@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
