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

[Lluís Vilanova]

Richard Henderson writes:

> On 07/07/2017 01:56 AM, Lluís Vilanova wrote:
[...]
>> +
>> +    /* 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;
>     }

Since other hooks can set db->is_jmp and return values (breakpoint_check), I'll
stick with db->is_jmp instead. Then tb_start can return max_insns, and generic
code can refine it with checks like single-stepping.


>> +        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.

As before, for consistency I prefer to set is_jmp instead of returning it on
hooks, since breakpoint_check() already has a return value and can set is_jmp.

Then, a future series adding tracing events to this loop uses db->pc_next in the
generic loop, so it is going to be used.


>> +    }
>> +
>> +    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.

Ok!


Thanks,
  Lluis