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

[Peter Maydell]

On 18 September 2015 at 17:33, Sergey Fedorov <address@hidden> wrote:
> On 18.09.2015 17:14, Peter Maydell wrote:
>> 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.
>>
>
> 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.

thanks
-- PMM