Re: [Qemu-devel] [PATCH v13 18/26] target/arm: [tcg] Port to breakpoint_check

[Richard Henderson <rth@twiddle.net>]

On 07/14/2017 07:26 AM, Richard Henderson wrote:
> On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
>> Incrementally paves the way towards using the generic instruction translation
>> loop.
>>
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>> ---
>>   target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index b9183fc511..55bef09739 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase 
>> *dcbase, CPUState *cpu)
>>   #endif
>>   }
>> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>> +                                    const CPUBreakpoint *bp)
>> +{
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>> +
>> +    if (bp->flags & BP_CPU) {
>> +        gen_set_condexec(dc);
>> +        gen_set_pc_im(dc, dc->pc);
>> +        gen_helper_check_breakpoints(cpu_env);
>> +        /* End the TB early; it's likely not going to be executed */
>> +        dc->base.is_jmp = DISAS_UPDATE;
>> +    } else {
>> +        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> +        /* The address covered by the breakpoint must be
>> +           included in [tb->pc, tb->pc + tb->size) in order
>> +           to for it to be properly cleared -- thus we
>> +           increment the PC here so that the logic setting
>> +           tb->size below does the right thing.  */
>> +        /* TODO: Advance PC by correct instruction length to
>> +         * avoid disassembler error messages */
>> +        dc->pc += 2;
>> +        dc->base.is_jmp = DISAS_NORETURN;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   /* generate intermediate code for basic block 'tb'.  */
>>   void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>>   {
>> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs, 
>> TranslationBlock *tb)
>>           if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>               CPUBreakpoint *bp;
>>               QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>> -                if (bp->pc == dc->pc) {
>> -                    if (bp->flags & BP_CPU) {
>> -                        gen_set_condexec(dc);
>> -                        gen_set_pc_im(dc, dc->pc);
>> -                        gen_helper_check_breakpoints(cpu_env);
>> -                        /* End the TB early; it's likely not going to be 
>> executed */
>> -                        dc->base.is_jmp = DISAS_UPDATE;
> 
> Oh I see what you're doing there in the main loop.
> And I see that you're copying existing behaviour.
> 
> That said, I do wonder if there's a better way.
> 
> Looking back at the original patch (5d98bf8f), there do not
> seem to have been any other side effects intended; simply
> "single step" any insn for which this bp condition is met.
> 
> Another way to handle this would be if we could adjust max_insns = num_insns.
> That would cause the loop to exit after the current insn, with DISAS_TOO_MANY 
> if nothing else.

Another possibility is is_jmp = DISAS_TOO_MANY, and exit the translation loop 
after the breakpoint check only for is_jmp > DISAS_TOO_MANY.  That allows all 
of the DISAS_TARGET_N values to exit as well.


r~