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

[Peter Maydell]

On 18 September 2015 at 15:07, Sergey Fedorov <address@hidden> wrote:
> On 18.09.2015 16:50, Peter Maydell wrote:
>> On 14 September 2015 at 11:51, Sergey Fedorov <address@hidden> 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.

thanks
-- PMM