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

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

On 18.09.2015 19:36, Peter Maydell wrote:
> On 18 September 2015 at 17:33, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> 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...
> Well, typically we'll take the BP exception in the helper function.
> There's nothing inherently wrong with translating further code
> after this insn, but it's usually not going to be executed, so
> we might as well end the TB early.
>

Thank, it is clear now. What about getting rid of "goto done generating"
and always translate one insn to update PC accordingly? Sometimes in_asm
log complains about that when disassembler and translator disagree about
the instruction size.

Best,
Sergey