Re: [Qemu-devel] [PATCH v2] target-arm: Clean up DISAS_UPDATE usage in AArch32 translation code

[Peter Maydell]

On 13 November 2015 at 21:13, Sergey Fedorov <address@hidden> wrote:
> On 10.11.2015 15:15, Peter Maydell wrote:
>> So the way the 32-bit code works for singlestep is complicated
>> because of the need to handle the conditional instructions,
>> which means you get a lot more cases like "this is a conditional
>> SWI" that need to be handled. A quick summary of some of the
>> possible cases:
>>
>>  * unconditional normal instruction:
>>     -- need to write the PC and condexec bits back to the CPU state
>>     -- then take a singlestep insn (either the architectural one
>>        or the EXCP_DEBUG one depending on which sort of step we are doing)
>>  * unconditional exception-generating instruction
>>     -- for architectural step of SWI/HVC/SMC we need to advance the
>>        singlestep state machine so that they behave correctly
>>     -- generate the relevant exception and then no point writing the
>>        code to take EXCP_DEBUG &c because we won't get to it
>>  * conditional instruction (including cond. branches):
>>     -- earlier code has already written back the PC for the
>>        "condition passed" case
>>     -- write out the code which takes the singlestep exception for
>>        the "condition passed" case
>>     -- then do gen_set_label(dc->condlabel)
>>     -- then the code to take the single step exception after
>>        executing for the "condition failed" case
>>
>> In particular in this bit:
>>         if (dc->condjmp || !dc->is_jmp) {
>>             gen_set_pc_im(dc, dc->pc);
>>             dc->condjmp = 0;
>
> Hi Peter,
>
> Thank you a lot for your explanation! It was really helpful for
> understanding the code :) One thing I wasn't sure of was whether this
> "dc->condjmp = 0" means that "condition failed" codepath below will also
> generate an exception whereas it shouldn't?

You want a singlestep exception for both conditional-insn
failed and conditional-insn passed (either way we've executed
the instruction and should return control to the debugger).

(The code is I think more confusing than it needs to be
and also somewhat repetitive in the way we have the same code for
"handle a trap insn if condjmp" and "handle a trap insn if
not condjmp".)

> Getting into the way the condexec bits handled I see that
> gen_set_condexec() should be called before
> gen_helper_check_breakpoints(), and probably also before
> gen_helper_access_check_cp_reg() because these helpers can raise an
> exception. I'm going to prepare patches for that soon.

Yes, I think this is right in both cases. (I'm kind of
surprised that we haven't noticed the invalid condexec
for conditional Thumb mode system register accesses, but
I guess mostly guests don't try to access registers that
are going to trap.)

thanks
-- PMM