Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Fix CPU breakpoint handling

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

On 08.10.2015 21:40, Peter Maydell wrote:
> On 28 September 2015 at 11:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> A QEMU breakpoint match is not definitely an architectural breakpoint
>> match. If an exception is generated unconditionally during translation,
>> it is hardly possible to ignore it in the debug exception handler.
>>
>> Generate a call to a helper to check CPU breakpoints and raise an
>> exception only if any breakpoint matches architecturally.
>>
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index ec0936c..426229f 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -11082,11 +11082,14 @@ 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);
>> +                    } else {
>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> We shouldn't just continue here, because now we'll try to generate the
> code for the instruction even in the "we know this will always be a bp"
> case. Also, you've dropped the "advance PC" part which we need so this
> TB is not zero length.

Actually, I was going to do the same way as some architectures (e.g.
alpha) did: always translate one instruction so that TB size is
determined by actual instruction decoding. At least, it makes sense for
AArch32 where we can have 16/32-bit insns. If we advance PC incorrectly,
we will get "Disassembler disagrees with translator over instruction
decoding" warning messages when in_asm log enabled. I can rewrite it
with PC advancement, but at least, I would like to change the
advancement to 4 bytes for A64 translation.

Best regards,
Sergey

>
>> +                    }
>> +                    /* End the TB early; it's likely not going to be executed */
>> +                    dc->is_jmp = DISAS_UPDATE;
> gen_exception_internal_insn sets is_jmp to DISAS_EXC, and then this
> overrides it; so this line should go inside the "is this a BP_CPU bp?"
> if clause. (I think the effect is just that we pointlessly generate some
> unreachable code after the exception generating call.)
>
>> +                    break;
>>                  }
>>              }
>>          }
>> @@ -11209,7 +11212,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>          }
>>      }
>>
>> -done_generating:
>>      gen_tb_end(tb, num_insns);
>>
>>  #ifdef DEBUG_DISAS
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 84a21ac..405d6d0 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -11328,11 +11328,14 @@ static inline void gen_intermediate_code_internal(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);
>> +                    } else {
>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> +                    }
>> +                    /* End the TB early; it's likely not going to be executed */
>> +                    dc->is_jmp = DISAS_UPDATE;
> Similar comments here.
>
> Annoying corner case which I don't think we need to handle necessarily:
> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
> boundary, and the second page is not present, we will end up taking the
> page fault when I think we should take the breakpoint. I can't think
> of a way to get that right, so just commenting that it isn't handled
> right would do.
>
>> +                    break;
>>                  }
>>              }
>>          }
>> --
>> 1.9.1
>>
> Otherwise I think this is the right approach.
>
> thanks
> -- PMM