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

[Lluís Vilanova]

Richard Henderson writes:

> 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 <address@hidden>
>>> Reviewed-by: Richard Henderson <address@hidden>
>>> ---
>>> 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.

After a quick check, I see that arm uses both (DISAS_NORETURN and
DISAS_TARGET_N) to exit in different points after the breakpoint. Moxie, mips
and unicore32 use use DISAS_NORETURN, and the rest use DISAS_TARGET_N.

I really don't know if it's safe to unify into a single behaviour. I'm not sure
if some targets will need to differentiate between DISAS_NORETURN and
DISAS_TOO_MANY (e.g., in the tb_stop() hook). As I said, I'd prefer to keep the
current approach that can accommodate all cases, and trim it down afterwards if
we see it's possible. That is, unless you're sure a new proposal can correctly
cover the cases of all targets.

Thanks,
  Lluis