Re: [PATCH v3 02/18] target/s390x: Add ilen to unwind data

[Richard Henderson]

On 9/27/19 3:30 AM, David Hildenbrand wrote:
>> +    /* Update ILEN, except for breakpoint, where we didn't load an insn.  */
>> +    if (ilen) {
>> +        env->int_pgm_ilen = ilen;
>> +    }
> 
> I am not completely sure about breakpoint handling and which
> implications we'll have when not setting int_pgm_ilen ...

Yeah.  Possibly to make it simple I should simply assign 2 as the length of a
breakpoint, bearing in mind that there is no a s390 exception to be delivered
-- this is purely a qemu-internal thing, raising EXCP_DEBUG to get back to the
gdbstub interface.

> I wonder if that change can help to better handle exceptions during
> EXECUTE, whereby we have to indicate the EXECUTE instruction and the
> ilen of the EXECUTE instruction (so the pc and ilen of the original
> EXECUTE function, not of the EXECUTE target).

Yes, that's already there.  The ilen of the execute insn is placed in the low 4
bits of env->ex_value, and that's what we record as ilen within extract_insn().

> I don't completely like the current interrupt handling when we have
> "env->ex_value" in "s390_cpu_exec_interrupt()". I'd love to see that
> check go, then we can reuse that function easily e.g., in MVC to test
> and inject exceptions while processing such an interruptible instruction
> - like MVC.

I don't think that reusing s390_cpu_exec_interrupt directly is a good idea.
There's other cleanup that needs to happen when exiting a TB.

What I think you should do instead is check env_neg(env)->icount_decr, exactly
like we do at the start of every basic block, and use that as an indication
that you should exit back to the main loop.


r~