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

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Alex Bennée writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:

>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> accel/tcg/Makefile.objs   |    1
>> accel/tcg/translator.c    |  152 +++++++++++++++++++++++++++++++++++++++++++++
>> include/exec/gen-icount.h |    2 -
>> include/exec/translator.h |   99 +++++++++++++++++++++++++++++
>> 4 files changed, 253 insertions(+), 1 deletion(-)
>> create mode 100644 accel/tcg/translator.c
>> 
>> diff --git a/accel/tcg/Makefile.objs b/accel/tcg/Makefile.objs
>> index f173cd5397..3a5da5357c 100644
>> --- a/accel/tcg/Makefile.objs
>> +++ b/accel/tcg/Makefile.objs
>> @@ -1,3 +1,4 @@
>> obj-$(CONFIG_SOFTMMU) += tcg-all.o
>> obj-$(CONFIG_SOFTMMU) += cputlb.o
>> obj-y += cpu-exec.o cpu-exec-common.o translate-all.o translate-common.o
>> +obj-y += translator.o

> There is a merge conflict here with the current master.

I'll rebase for v13.


>> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
>> new file mode 100644
>> index 0000000000..9e0343cbb1
>> --- /dev/null
>> +++ b/accel/tcg/translator.c
>> @@ -0,0 +1,152 @@
>> +/*
>> + * Generic intermediate code generation.
>> + *
>> + * Copyright (C) 2016-2017 Lluís Vilanova <vilanova@ac.upc.edu>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +#include "qemu/error-report.h"
>> +#include "cpu.h"
>> +#include "tcg/tcg.h"
>> +#include "tcg/tcg-op.h"
>> +#include "exec/exec-all.h"
>> +#include "exec/gen-icount.h"
>> +#include "exec/log.h"
>> +#include "exec/translator.h"
>> +
>> +
>> +static inline void translate_block_tcg_check(const DisasContextBase *db)
>> +{
>> +    if (tcg_check_temp_count()) {
>> +        error_report("warning: TCG temporary leaks before "TARGET_FMT_lx,
>> +                     db->pc_next);
>> +    }
>> +}
>> +
>> +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();
>> +
>> +    /* 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);
>> +
>> +    while (true) {
>> +        db->num_insns++;
>> +        ops->insn_start(db, cpu);
>> +
>> +        /* Early exit before breakpoint checks */
>> +        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:

> For the sake of keeping the core loop easy to follow maybe it would be
> better to have a helper function for the breakpoint handling? Really
> there is only one result from the helper which is do we continue the
> loop or jump to done_generating.

The new v13 has a much simpler loop that will hopefully address your concerns.

>> +
>> +        /* 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);
>> +
>> +        /**************************************************/
>> +        /* Conditions to stop translation                 */
>> +        /**************************************************/
>> +
>> +        /* Target-specific conditions set by disassembly */
>> +        if (db->is_jmp != DISAS_NEXT) {
>> +            break;
>> +        }
>> +
>> +        /* Too many instructions */
>> +        if (tcg_op_buf_full() || db->num_insns >= max_insns) {
>> +            db->is_jmp = DISAS_TOO_MANY;
>> +            break;
>> +        }
>> +
>> +        translate_block_tcg_check(db);
>> +    }

> This may be a personal taste thing but having while(true) {} and breaks
> is harder to follow than do { stuff } while (!done);

I think it is. I prefer to see the loop condition up-front, unless a do-while
makes the condition logic substantially simpler.


Thanks,
  Lluis