Re: [PATCH v2 5/7] target/nios2: Convert to TranslatorOps

[Richard Henderson]

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~