| Message ID | 149942859571.8972.4761014660099212028.stgit@frigg.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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~
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
On 07/11/2017 06:40 AM, Lluís Vilanova wrote: > 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. No, you misunderstand. For SH4 I need to override single-stepping, icount, and anything else that produces a TB smaller than the atomic region. r~
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. > 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. > + > + /* 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); > + > + ops->tb_stop(db, cpu); > + > + if (db->tb->cflags & CF_LAST_IO) { > + gen_io_end(); > + } > + > +done_generating: > + gen_tb_end(db->tb, db->num_insns); > + > + translate_block_tcg_check(db); > + > +#ifdef DEBUG_DISAS > + if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) > + && qemu_log_in_addr_range(db->pc_first)) { > + qemu_log_lock(); > + qemu_log("----------------\n"); > + ops->disas_log(db, cpu); > + qemu_log("\n"); > + qemu_log_unlock(); > + } > +#endif > + > + db->tb->size = db->pc_next - db->pc_first; > + db->tb->icount = db->num_insns; > +} > diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h > index 6c28ef59c3..9b3cb14dfa 100644 > --- a/include/exec/gen-icount.h > +++ b/include/exec/gen-icount.h > @@ -44,7 +44,7 @@ static inline void gen_tb_start(TranslationBlock *tb) > tcg_temp_free_i32(count); > } > > -static void gen_tb_end(TranslationBlock *tb, int num_insns) > +static inline void gen_tb_end(TranslationBlock *tb, int num_insns) > { > if (tb->cflags & CF_USE_ICOUNT) { > /* Update the num_insn immediate parameter now that we know > diff --git a/include/exec/translator.h b/include/exec/translator.h > index 1f9697dd31..f96b66f2bf 100644 > --- a/include/exec/translator.h > +++ b/include/exec/translator.h > @@ -10,6 +10,38 @@ > #ifndef EXEC__TRANSLATOR_H > #define EXEC__TRANSLATOR_H > > +/* > + * Include this header from a target-specific file, and add a > + * > + * DisasContextBase base; > + * > + * member in your target-specific DisasContext. > + */ > + > + > +#include "exec/exec-all.h" > +#include "tcg/tcg.h" > + > + > +/** > + * BreakpointCheckType: > + * @BC_MISS: No hit > + * @BC_HIT_INSN: Hit, but continue translating TB > + * @BC_HIT_TB: Hit, stop translating TB > + * > + * How to react to a breakpoint. A hit means no more breakpoints will be checked > + * for the current instruction. > + * > + * Not all breakpoints associated to an address are necessarily raised by > + * targets (e.g., due to conditions encoded in their flags), so they can decide > + * that a breakpoint missed the address (@BP_MISS). > + */ > +typedef enum BreakpointCheckType { > + BC_MISS, > + BC_HIT_INSN, > + BC_HIT_TB, > +} BreakpointCheckType; > + > /** > * DisasJumpType: > * @DISAS_NEXT: Next instruction in program order. > @@ -36,4 +68,71 @@ typedef enum DisasJumpType { > DISAS_TARGET_12, > } DisasJumpType; > > +/** > + * DisasContextBase: > + * @tb: Translation block for this disassembly. > + * @pc_first: Address of first guest instruction in this TB. > + * @pc_next: Address of next guest instruction in this TB (current during > + * disassembly). > + * @is_jmp: What instruction to disassemble next. > + * @num_insns: Number of translated instructions (including current). > + * @singlestep_enabled: "Hardware" single stepping enabled. > + * > + * Architecture-agnostic disassembly context. > + */ > +typedef struct DisasContextBase { > + TranslationBlock *tb; > + target_ulong pc_first; > + target_ulong pc_next; > + DisasJumpType is_jmp; > + unsigned int num_insns; > + bool singlestep_enabled; > +} DisasContextBase; > + > +/** > + * TranslatorOps: > + * @init_disas_context: Initialize a DisasContext struct (DisasContextBase has > + * already been initialized). > + * @tb_start: Start translating a new TB. > + * @insn_start: Start translating a new instruction. > + * @breakpoint_check: Check if a breakpoint did hit. When called, the breakpoint > + * has already been checked to match the PC. > + * @translate_insn: Disassemble one instruction and return the PC for the next > + * one. Can set db->is_jmp to DISAS_TARGET or above to stop > + * translation. > + * @tb_stop: Stop translating a TB. > + * @disas_log: Print instruction disassembly to log. > + * > + * Target-specific operations for the generic translator loop. > + */ > +typedef struct TranslatorOps { > + void (*init_disas_context)(DisasContextBase *db, CPUState *cpu); > + void (*tb_start)(DisasContextBase *db, CPUState *cpu); > + void (*insn_start)(DisasContextBase *db, CPUState *cpu); > + BreakpointCheckType (*breakpoint_check)(DisasContextBase *db, CPUState *cpu, > + const CPUBreakpoint *bp); > + target_ulong (*translate_insn)(DisasContextBase *db, CPUState *cpu); > + void (*tb_stop)(DisasContextBase *db, CPUState *cpu); > + void (*disas_log)(const DisasContextBase *db, CPUState *cpu); > +} TranslatorOps; > + > +/** > + * translator_loop: > + * @ops: Target-specific operations. > + * @db: Disassembly context. > + * @cpu: Target vCPU. > + * @tb: Translation block. > + * > + * Generic translator loop. > + * > + * Translation will stop in the following cases (in order): > + * - When set by #TranslatorOps::insn_start. > + * - When set by #TranslatorOps::translate_insn. > + * - When the TCG operation buffer is full. > + * - When single-stepping is enabled (system-wide or on the current vCPU). > + * - When too many instructions have been translated. > + */ > +void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > + CPUState *cpu, TranslationBlock *tb); > + > #endif /* EXEC__TRANSLATOR_H */ -- Alex Bennée
Richard Henderson writes: > On 07/11/2017 06:40 AM, Lluís Vilanova wrote: >> 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. > No, you misunderstand. For SH4 I need to override single-stepping, icount, and > anything else that produces a TB smaller than the atomic region. I see, ok. Cheers, Lluis
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
Lluís Vilanova <vilanova@ac.upc.edu> writes: > Alex Bennée writes: > >> Lluís Vilanova <vilanova@ac.upc.edu> writes: > >>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> <snip> >>> + >>> + 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. Yes but in this case the condition is "go forever" unless we break (or goto) out. I'd be happy with the while at the top if it was an actual condition. -- Alex Bennée
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 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: + + /* 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); + } + + ops->tb_stop(db, cpu); + + if (db->tb->cflags & CF_LAST_IO) { + gen_io_end(); + } + +done_generating: + gen_tb_end(db->tb, db->num_insns); + + translate_block_tcg_check(db); + +#ifdef DEBUG_DISAS + if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) + && qemu_log_in_addr_range(db->pc_first)) { + qemu_log_lock(); + qemu_log("----------------\n"); + ops->disas_log(db, cpu); + qemu_log("\n"); + qemu_log_unlock(); + } +#endif + + db->tb->size = db->pc_next - db->pc_first; + db->tb->icount = db->num_insns; +} diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h index 6c28ef59c3..9b3cb14dfa 100644 --- a/include/exec/gen-icount.h +++ b/include/exec/gen-icount.h @@ -44,7 +44,7 @@ static inline void gen_tb_start(TranslationBlock *tb) tcg_temp_free_i32(count); } -static void gen_tb_end(TranslationBlock *tb, int num_insns) +static inline void gen_tb_end(TranslationBlock *tb, int num_insns) { if (tb->cflags & CF_USE_ICOUNT) { /* Update the num_insn immediate parameter now that we know diff --git a/include/exec/translator.h b/include/exec/translator.h index 1f9697dd31..f96b66f2bf 100644 --- a/include/exec/translator.h +++ b/include/exec/translator.h @@ -10,6 +10,38 @@ #ifndef EXEC__TRANSLATOR_H #define EXEC__TRANSLATOR_H +/* + * Include this header from a target-specific file, and add a + * + * DisasContextBase base; + * + * member in your target-specific DisasContext. + */ + + +#include "exec/exec-all.h" +#include "tcg/tcg.h" + + +/** + * BreakpointCheckType: + * @BC_MISS: No hit + * @BC_HIT_INSN: Hit, but continue translating TB + * @BC_HIT_TB: Hit, stop translating TB + * + * How to react to a breakpoint. A hit means no more breakpoints will be checked + * for the current instruction. + * + * Not all breakpoints associated to an address are necessarily raised by + * targets (e.g., due to conditions encoded in their flags), so they can decide + * that a breakpoint missed the address (@BP_MISS). + */ +typedef enum BreakpointCheckType { + BC_MISS, + BC_HIT_INSN, + BC_HIT_TB, +} BreakpointCheckType; + /** * DisasJumpType: * @DISAS_NEXT: Next instruction in program order. @@ -36,4 +68,71 @@ typedef enum DisasJumpType { DISAS_TARGET_12, } DisasJumpType; +/** + * DisasContextBase: + * @tb: Translation block for this disassembly. + * @pc_first: Address of first guest instruction in this TB. + * @pc_next: Address of next guest instruction in this TB (current during + * disassembly). + * @is_jmp: What instruction to disassemble next. + * @num_insns: Number of translated instructions (including current). + * @singlestep_enabled: "Hardware" single stepping enabled. + * + * Architecture-agnostic disassembly context. + */ +typedef struct DisasContextBase { + TranslationBlock *tb; + target_ulong pc_first; + target_ulong pc_next; + DisasJumpType is_jmp; + unsigned int num_insns; + bool singlestep_enabled; +} DisasContextBase; + +/** + * TranslatorOps: + * @init_disas_context: Initialize a DisasContext struct (DisasContextBase has + * already been initialized). + * @tb_start: Start translating a new TB. + * @insn_start: Start translating a new instruction. + * @breakpoint_check: Check if a breakpoint did hit. When called, the breakpoint + * has already been checked to match the PC. + * @translate_insn: Disassemble one instruction and return the PC for the next + * one. Can set db->is_jmp to DISAS_TARGET or above to stop + * translation. + * @tb_stop: Stop translating a TB. + * @disas_log: Print instruction disassembly to log. + * + * Target-specific operations for the generic translator loop. + */ +typedef struct TranslatorOps { + void (*init_disas_context)(DisasContextBase *db, CPUState *cpu); + void (*tb_start)(DisasContextBase *db, CPUState *cpu); + void (*insn_start)(DisasContextBase *db, CPUState *cpu); + BreakpointCheckType (*breakpoint_check)(DisasContextBase *db, CPUState *cpu, + const CPUBreakpoint *bp); + target_ulong (*translate_insn)(DisasContextBase *db, CPUState *cpu); + void (*tb_stop)(DisasContextBase *db, CPUState *cpu); + void (*disas_log)(const DisasContextBase *db, CPUState *cpu); +} TranslatorOps; + +/** + * translator_loop: + * @ops: Target-specific operations. + * @db: Disassembly context. + * @cpu: Target vCPU. + * @tb: Translation block. + * + * Generic translator loop. + * + * Translation will stop in the following cases (in order): + * - When set by #TranslatorOps::insn_start. + * - When set by #TranslatorOps::translate_insn. + * - When the TCG operation buffer is full. + * - When single-stepping is enabled (system-wide or on the current vCPU). + * - When too many instructions have been translated. + */ +void translator_loop(const TranslatorOps *ops, DisasContextBase *db, + CPUState *cpu, TranslationBlock *tb); + #endif /* EXEC__TRANSLATOR_H */
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