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

[David Hildenbrand <david@redhat.com>]

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