| Message ID | 20210620221046.1526418-6-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/nios2: Convert to TranslatorOps | expand |
On Sun, 20 Jun 2021 at 23:18, Richard Henderson <richard.henderson@linaro.org> wrote: > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/nios2/translate.c | 130 ++++++++++++++++++++------------------- > 1 file changed, 67 insertions(+), 63 deletions(-) > > diff --git a/target/nios2/translate.c b/target/nios2/translate.c > index 31653b7912..06705c894d 100644 > --- a/target/nios2/translate.c > +++ b/target/nios2/translate.c > @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp) > } > > /* generate intermediate code for basic block 'tb'. */ > -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) > +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > { > + DisasContext *dc = container_of(dcbase, DisasContext, base); > CPUNios2State *env = cs->env_ptr; > - DisasContext dc1, *dc = &dc1; > - int num_insns; > - > - /* Initialize DC */ > - > - dc->base.tb = tb; > - dc->base.singlestep_enabled = cs->singlestep_enabled; > - dc->base.is_jmp = DISAS_NEXT; > - dc->base.pc_first = tb->pc; > - dc->base.pc_next = tb->pc; > + target_ulong pc = dc->base.pc_first; The local variable doesn't really seem necessary -- you could just write "dc->pc = dc->base.pc_first" and then use "dc->pc" in the calculation of page_insns. > + int page_insns; > > dc->zero = NULL; > - dc->pc = tb->pc; > + dc->pc = pc; > dc->mem_idx = cpu_mmu_index(env, false); > > - /* Set up instruction counts */ > - num_insns = 0; > - if (max_insns > 1) { > - int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4; > - if (max_insns > page_insns) { > - max_insns = page_insns; > - } > - } > + /* Bound the number of insns to execute to those left on the page. */ > + page_insns = -(pc | TARGET_PAGE_MASK) / 4; > + dc->base.max_insns = MIN(page_insns, dc->base.max_insns); > +} > > - gen_tb_start(tb); > - do { > - tcg_gen_insn_start(dc->pc); > - num_insns++; > +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs) > +{ > +} > > - if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) { > - gen_exception(dc, 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. */ > - dc->pc += 4; > - break; > - } > +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs) > +{ > + tcg_gen_insn_start(dcbase->pc_next); > +} > > - if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) { > - gen_io_start(); > - } > +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, > + const CPUBreakpoint *bp) > +{ > + DisasContext *dc = container_of(dcbase, DisasContext, base); > > - /* Decode an instruction */ > - handle_instruction(dc, env); > + gen_exception(dc, 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. > + */ > + dc->pc += 4; Don't we need to increment dc->base.pc_next here, not dc->pc? The generic setting of tb->size in accel/tcg uses "db->pc_next - db->pc_first". Side note: that comment about "setting tb->size below" seems to have been copied-and-pasted into most of the front-ends, but it's wrong: the setting of tb->size is no longer "below" but in the common code. (More generally, some followup patches rationalizing the use of dc->pc along the lines of the Arm dc->pc_curr vs dc->base.pc_next might help. There seems to be a fair amount of code which uses "dc->pc + 4" that could be using pc_next instead.) > + return true; The arm versions of the breakpoint_check hook also set dc->base.is_jmp to DISAS_NORETURN. Are they doing that unnecessarily, or do we need to do that here ? > +} > > - dc->pc += 4; > +static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) > +{ > + DisasContext *dc = container_of(dcbase, DisasContext, base); > + CPUNios2State *env = cs->env_ptr; > > - /* Translation stops when a conditional branch is encountered. > - * Otherwise the subsequent code could get translated several times. > - * Also stop translation when a page boundary is reached. This > - * ensures prefetch aborts occur at the right place. */ > - } while (!dc->base.is_jmp && > - !tcg_op_buf_full() && > - num_insns < max_insns); > + /* Decode an instruction */ > + handle_instruction(dc, env); > + > + dc->base.pc_next += 4; > + dc->pc += 4; This isn't wrong, but I think that a setup like the Arm translator that does dc->pc_curr = s->base.pc_next; code = cpu_ldl_code(env, s->base.pc_next); s->base.pc_next += 4; /* dispatch to handler function here */ would be nicer (dunno whether clearer to do as a single thing or first to do this conversion and then do a followup patch). thanks -- PMM
On 6/28/21 8:57 AM, Peter Maydell wrote: > On Sun, 20 Jun 2021 at 23:18, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/nios2/translate.c | 130 ++++++++++++++++++++------------------- >> 1 file changed, 67 insertions(+), 63 deletions(-) >> >> diff --git a/target/nios2/translate.c b/target/nios2/translate.c >> index 31653b7912..06705c894d 100644 >> --- a/target/nios2/translate.c >> +++ b/target/nios2/translate.c >> @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp) >> } >> >> /* generate intermediate code for basic block 'tb'. */ >> -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) >> +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >> { >> + DisasContext *dc = container_of(dcbase, DisasContext, base); >> CPUNios2State *env = cs->env_ptr; >> - DisasContext dc1, *dc = &dc1; >> - int num_insns; >> - >> - /* Initialize DC */ >> - >> - dc->base.tb = tb; >> - dc->base.singlestep_enabled = cs->singlestep_enabled; >> - dc->base.is_jmp = DISAS_NEXT; >> - dc->base.pc_first = tb->pc; >> - dc->base.pc_next = tb->pc; >> + target_ulong pc = dc->base.pc_first; > > The local variable doesn't really seem necessary -- you could just > write "dc->pc = dc->base.pc_first" and then use "dc->pc" in the > calculation of page_insns. > >> + int page_insns; >> >> dc->zero = NULL; >> - dc->pc = tb->pc; >> + dc->pc = pc; >> dc->mem_idx = cpu_mmu_index(env, false); >> >> - /* Set up instruction counts */ >> - num_insns = 0; >> - if (max_insns > 1) { >> - int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4; >> - if (max_insns > page_insns) { >> - max_insns = page_insns; >> - } >> - } >> + /* Bound the number of insns to execute to those left on the page. */ >> + page_insns = -(pc | TARGET_PAGE_MASK) / 4; >> + dc->base.max_insns = MIN(page_insns, dc->base.max_insns); >> +} >> >> - gen_tb_start(tb); >> - do { >> - tcg_gen_insn_start(dc->pc); >> - num_insns++; >> +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs) >> +{ >> +} >> >> - if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) { >> - gen_exception(dc, 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. */ >> - dc->pc += 4; >> - break; >> - } >> +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs) >> +{ >> + tcg_gen_insn_start(dcbase->pc_next); >> +} >> >> - if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) { >> - gen_io_start(); >> - } >> +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, >> + const CPUBreakpoint *bp) >> +{ >> + DisasContext *dc = container_of(dcbase, DisasContext, base); >> >> - /* Decode an instruction */ >> - handle_instruction(dc, env); >> + gen_exception(dc, 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. >> + */ >> + dc->pc += 4; > > Don't we need to increment dc->base.pc_next here, not dc->pc? The > generic setting of tb->size in accel/tcg uses "db->pc_next - db->pc_first". Yep, thanks. >> + return true; > > The arm versions of the breakpoint_check hook also set dc->base.is_jmp > to DISAS_NORETURN. > Are they doing that unnecessarily, or do we need to do that here ? Here it's done in gen_exception. >> + handle_instruction(dc, env); >> + >> + dc->base.pc_next += 4; >> + dc->pc += 4; > > This isn't wrong, but I think that a setup like the Arm translator > that does > dc->pc_curr = s->base.pc_next; > code = cpu_ldl_code(env, s->base.pc_next); > s->base.pc_next += 4; > /* dispatch to handler function here */ > > would be nicer (dunno whether clearer to do as a single thing or > first to do this conversion and then do a followup patch). You're right that advancing pc_next first and using that instead of pc+4 globally would be a good clean up. r~
diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 31653b7912..06705c894d 100644 --- a/target/nios2/translate.c +++ b/target/nios2/translate.c @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp) } /* generate intermediate code for basic block 'tb'. */ -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) { + DisasContext *dc = container_of(dcbase, DisasContext, base); CPUNios2State *env = cs->env_ptr; - DisasContext dc1, *dc = &dc1; - int num_insns; - - /* Initialize DC */ - - dc->base.tb = tb; - dc->base.singlestep_enabled = cs->singlestep_enabled; - dc->base.is_jmp = DISAS_NEXT; - dc->base.pc_first = tb->pc; - dc->base.pc_next = tb->pc; + target_ulong pc = dc->base.pc_first; + int page_insns; dc->zero = NULL; - dc->pc = tb->pc; + dc->pc = pc; dc->mem_idx = cpu_mmu_index(env, false); - /* Set up instruction counts */ - num_insns = 0; - if (max_insns > 1) { - int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4; - if (max_insns > page_insns) { - max_insns = page_insns; - } - } + /* Bound the number of insns to execute to those left on the page. */ + page_insns = -(pc | TARGET_PAGE_MASK) / 4; + dc->base.max_insns = MIN(page_insns, dc->base.max_insns); +} - gen_tb_start(tb); - do { - tcg_gen_insn_start(dc->pc); - num_insns++; +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs) +{ +} - if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) { - gen_exception(dc, 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. */ - dc->pc += 4; - break; - } +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs) +{ + tcg_gen_insn_start(dcbase->pc_next); +} - if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) { - gen_io_start(); - } +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, + const CPUBreakpoint *bp) +{ + DisasContext *dc = container_of(dcbase, DisasContext, base); - /* Decode an instruction */ - handle_instruction(dc, env); + gen_exception(dc, 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. + */ + dc->pc += 4; + return true; +} - dc->pc += 4; +static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs) +{ + DisasContext *dc = container_of(dcbase, DisasContext, base); + CPUNios2State *env = cs->env_ptr; - /* Translation stops when a conditional branch is encountered. - * Otherwise the subsequent code could get translated several times. - * Also stop translation when a page boundary is reached. This - * ensures prefetch aborts occur at the right place. */ - } while (!dc->base.is_jmp && - !tcg_op_buf_full() && - num_insns < max_insns); + /* Decode an instruction */ + handle_instruction(dc, env); + + dc->base.pc_next += 4; + dc->pc += 4; +} + +static void nios2_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) +{ + DisasContext *dc = container_of(dcbase, DisasContext, base); /* Indicate where the next block should start */ switch (dc->base.is_jmp) { - case DISAS_NEXT: + case DISAS_TOO_MANY: case DISAS_UPDATE: /* Save the current PC back into the CPU register */ tcg_gen_movi_tl(cpu_R[R_PC], dc->pc); tcg_gen_exit_tb(NULL, 0); break; - default: case DISAS_JUMP: /* The jump will already have updated the PC register */ tcg_gen_exit_tb(NULL, 0); @@ -880,25 +877,32 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) case DISAS_NORETURN: /* nothing more to generate */ break; + + default: + g_assert_not_reached(); } +} - /* End off the block */ - gen_tb_end(tb, num_insns); +static void nios2_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu) +{ + qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first)); + log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size); +} - /* Mark instruction starts for the final generated instruction */ - tb->size = dc->pc - tb->pc; - tb->icount = num_insns; +static const TranslatorOps nios2_tr_ops = { + .init_disas_context = nios2_tr_init_disas_context, + .tb_start = nios2_tr_tb_start, + .insn_start = nios2_tr_insn_start, + .breakpoint_check = nios2_tr_breakpoint_check, + .translate_insn = nios2_tr_translate_insn, + .tb_stop = nios2_tr_tb_stop, + .disas_log = nios2_tr_disas_log, +}; -#ifdef DEBUG_DISAS - if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM) - && qemu_log_in_addr_range(tb->pc)) { - FILE *logfile = qemu_log_lock(); - qemu_log("IN: %s\n", lookup_symbol(tb->pc)); - log_target_disas(cs, tb->pc, dc->pc - tb->pc); - qemu_log("\n"); - qemu_log_unlock(logfile); - } -#endif +void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns) +{ + DisasContext dc; + translator_loop(&nios2_tr_ops, &dc.base, cs, tb, max_insns); } void nios2_cpu_dump_state(CPUState *cs, FILE *f, int flags)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/nios2/translate.c | 130 ++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 63 deletions(-) -- 2.25.1