Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling

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

On 18.09.2015 17:14, Peter Maydell wrote:
> On 18 September 2015 at 15:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 18.09.2015 16:50, Peter Maydell wrote:
>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>> --- a/target-arm/translate-a64.c
>>>> +++ b/target-arm/translate-a64.c
>>>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>>>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>>              QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>>>>                  if (bp->pc == dc->pc) {
>>>> -                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>> -                    /* Advance PC so that clearing the breakpoint will
>>>> -                       invalidate this TB.  */
>>>> -                    dc->pc += 2;
>>>> -                    goto done_generating;
>>>> +                    if (bp->flags & BP_CPU) {
>>>> +                        gen_helper_check_breakpoints(cpu_env);
>>>> +                        break;
>>>> +                    } else {
>>>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>> +                        goto done_generating;
>>>> +                    }
>>> You seem to have dropped the "advance the PC" code -- why is that ok?
>>>
>> I also dropped the immediately following goto statement. With these
>> changes PC is advanced in the same way as it happens during normal
>> translation. That is because we actually have to do the instruction
>> translation process here to support the case when a breakpoint with
>> matching PC is architecturally mismatched. As I understand, that
>> "advance the PC" code was necessary to produce a TB with non-zero size
>> so that it can be invalidated later when we clear the breakpoint.
> OK, that makes sense for the BP_CPU case but you still have the
> "goto done_generating;" in the else clause...
>
> Also, should we maybe make this TB be only one insn long even for
> the BP_CPU case? It seems like in the common case we will only
> execute one insn.
>

Right, I have to fix this PC advancement. But I can't think of why we
will only execute one insn...

Best,
Sergey