Re: [Qemu-devel] [PATCH v12 04/27] target: [tcg] Add generic translation framework

[Richard Henderson]

On 07/07/2017 01:56 AM, Lluís Vilanova wrote:
> +void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
> +                     CPUState *cpu, TranslationBlock *tb)
> +{
> +    int max_insns;
> +
> +    /* Initialize DisasContext */
> +    db->tb = tb;
> +    db->pc_first = tb->pc;
> +    db->pc_next = db->pc_first;
> +    db->is_jmp = DISAS_NEXT;
> +    db->num_insns = 0;
> +    db->singlestep_enabled = cpu->singlestep_enabled;
> +    ops->init_disas_context(db, cpu);
> +
> +    /* Initialize globals */
> +    tcg_clear_temp_count();

The comment is out of date.  But perhaps just expand a bit,

  Now that globals are created, reset the temp count so that
  we can identify leaks.

> +
> +    /* Instruction counting */
> +    max_insns = db->tb->cflags & CF_COUNT_MASK;
> +    if (max_insns == 0) {
> +        max_insns = CF_COUNT_MASK;
> +    }
> +    if (max_insns > TCG_MAX_INSNS) {
> +        max_insns = TCG_MAX_INSNS;
> +    }
> +    if (db->singlestep_enabled || singlestep) {
> +        max_insns = 1;
> +    }
> +
> +    /* Start translating */
> +    gen_tb_start(db->tb);
> +    ops->tb_start(db, cpu);

As I mentioned, we need some way to modify max_insns before the loop start.

(For the ultimate in max_insns modifying needs, see the sh4 patch set I posted 
this morning, wherein I may *extend* max_insns in order to force it to cover a 
region to be executed atomically within one TB, within the lock held by 
cpu_exec_step_atomic.)

Based on that, I recommend

    DisasJumpType status;
    status = ops->tb_start(db, cpu, &max_insns);

Because in my sh4 case, tb_start might itself decide that the only way to 
handle the code is to raise the EXCP_ATOMIC exception and try again within 
cpu_exec_step_atomic.  Which means that we would not enter the while loop at 
all.  Thus,

> +    while (true) {

    while (status == DISAS_NEXT) {

> +        db->num_insns++;
> +        ops->insn_start(db, cpu);
> +
> +        /* Early exit before breakpoint checks */

Better comment maybe?  "The insn_start hook may request early exit..."

And, indeed, perhaps

    status = ops->insn_start(db, cpu);
    if (unlikely(status != DISAS_NEXT)) {
        break;
    }

> +        if (unlikely(db->is_jmp != DISAS_NEXT)) {
> +            break;
> +        }
> +
> +        /* Pass breakpoint hits to target for further processing */
> +        if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
> +            CPUBreakpoint *bp;
> +            QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
> +                if (bp->pc == db->pc_next) {
> +                    BreakpointCheckType bp_check =
> +                        ops->breakpoint_check(db, cpu, bp);
> +                    switch (bp_check) {
> +                    case BC_MISS:
> +                        /* Target ignored this breakpoint, go to next */
> +                        break;
> +                    case BC_HIT_INSN:
> +                        /* Hit, keep translating */
> +                        /*
> +                         * TODO: if we're never going to have more than one
> +                         *       BP in a single address, we can simply use a
> +                         *       bool here.
> +                         */
> +                        goto done_breakpoints;
> +                    case BC_HIT_TB:
> +                        /* Hit, end TB */
> +                        goto done_generating;
> +                    default:
> +                        g_assert_not_reached();
> +                    }
> +                }
> +            }
> +        }
> +    done_breakpoints:
> +
> +        /* Accept I/O on last instruction */
> +        if (db->num_insns == max_insns && (db->tb->cflags & CF_LAST_IO)) {
> +            gen_io_start();
> +        }
> +
> +        /* Disassemble one instruction */
> +        db->pc_next = ops->translate_insn(db, cpu);

I think it would be better to assign to pc_next within the hook.  We don't use 
the value otherwise within the rest of the loop.

But we do use is_jmp, immediately.  So maybe better to follow the changes to 
tb_start and insn_start above.

> +    }
> +
> +    ops->tb_stop(db, cpu);
> +
> +    if (db->tb->cflags & CF_LAST_IO) {
> +        gen_io_end();
> +    }

You can't place this after tb_stop, as it'll never be executed.  We will have 
for certain emitted the exit_tb for all paths by now.

Just place it before tb_stop where it usually resides.  In the case where some 
is_jmp value implies unreached code, and we're using icount, we'll accept the 
dead code.  But in all other cases it will get executed.


r~