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

[Sergey Fedorov <serge.fdrv@gmail.com>]

On 14.11.2015 22:45, Peter Maydell wrote:
> On 13 November 2015 at 21:13, Sergey Fedorov <serge.fdrv@gmail.com> 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).

To be clear, I mean SWI/HVC/SMC exceptions could be generated when
singlestepping through a conditional-insn failed codepath.

>
> (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".)

I think I could make this code more clear and concise as well as put
some comments to describe it :)

Best regards,
Sergey

>
>> 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.)