Re: [Qemu-arm] [PATCH v1 2/6] target/arm/translate.c: make DISAS_UPDATE match declared semantics

[Richard Henderson]

On 07/10/2017 06:13 AM, Peter Maydell wrote:
> On 10 July 2017 at 16:47, Alex Bennée <address@hidden> wrote:
> > DISAS_UPDATE should be used when the wider CPU state other than just
> > the PC has been updated and we should therefor exit the TCG runtime
> > and return to the main execution loop rather assuming DISAS_JUMP would
> > do that.
> >
> > As some DISAS_UPDATE users may update the PC dynamically via a helper
> > we also push the updating to the PC to hhe call sites which set
> > ->is_jmp to DISAS_UPDATE.
> >
> > Signed-off-by: Alex Bennée <address@hidden>
> > ---
> >   target/arm/translate.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/arm/translate.c b/target/arm/translate.c
> > index 0862f9e4aa..f9c4aee1b6 100644
> > --- a/target/arm/translate.c
> > +++ b/target/arm/translate.c
> > @@ -4430,6 +4430,7 @@ static void gen_msr_banked(DisasContext *s, int r, int 
> > sysm, int rn)
> >       tcg_temp_free_i32(tcg_tgtmode);
> >       tcg_temp_free_i32(tcg_regno);
> >       tcg_temp_free_i32(tcg_reg);
> > +    gen_set_pc_im(s, s->pc);
> >       s->is_jmp = DISAS_UPDATE;
> >   }
> >
> > @@ -4452,6 +4453,7 @@ static void gen_mrs_banked(DisasContext *s, int r, int 
> > sysm, int rn)
> >       tcg_temp_free_i32(tcg_tgtmode);
> >       tcg_temp_free_i32(tcg_regno);
> >       store_reg(s, rn, tcg_reg);
> > +    gen_set_pc_im(s, s->pc);
> >       s->is_jmp = DISAS_UPDATE;
> >   }
> >
> > @@ -8058,6 +8060,7 @@ static void gen_srs(DisasContext *s,
> >           tcg_temp_free_i32(tmp);
> >       }
> >       tcg_temp_free_i32(addr);
> > +    gen_set_pc_im(s, s->pc);
> >       s->is_jmp = DISAS_UPDATE;
> >   }
> >
> > @@ -8146,6 +8149,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int 
> > insn)
> >               /* setend */
> >               if (((insn >> 9) & 1) != !!(s->be_data == MO_BE)) {
> >                   gen_helper_setend(cpu_env);
> > +                gen_set_pc_im(s, s->pc);
> >                   s->is_jmp = DISAS_UPDATE;
> >               }
> >               return;
> > @@ -11619,6 +11623,7 @@ static void disas_thumb_insn(CPUARMState *env, 
> > DisasContext *s)
> >                   ARCH(6);
> >                   if (((insn >> 3) & 1) != !!(s->be_data == MO_BE)) {
> >                       gen_helper_setend(cpu_env);
> > +                    gen_set_pc_im(s, s->pc);
> >                       s->is_jmp = DISAS_UPDATE;
> >                   }
> >                   break;
> > @@ -12076,7 +12081,6 @@ void gen_intermediate_code(CPUARMState *env, 
> > TranslationBlock *tb)
> >               break;
> >           case DISAS_NEXT:
> >           case DISAS_UPDATE:
> > -            gen_set_pc_im(dc, dc->pc);
> >               /* fall through */
> >           default:
> >               /* FIXME: Single stepping a WFI insn will not halt the CPU. */
> > @@ -12095,12 +12099,11 @@ void gen_intermediate_code(CPUARMState *env, 
> > TranslationBlock *tb)
> >           case DISAS_NEXT:
> >               gen_goto_tb(dc, 1, dc->pc);
> >               break;
> > -        case DISAS_UPDATE:
> > -            gen_set_pc_im(dc, dc->pc);
> > -            /* fall through */
> >           case DISAS_JUMP:
> >               gen_goto_ptr();
> >               break;
> > +        case DISAS_UPDATE:
> > +            /* fall through */
> >           default:
> >               /* indicate that the hash table must be used to find the next TB 
> > */
> >               tcg_gen_exit_tb(0);
>
> I'm not a great fan of this, because we've taken five cases that
> all want the same behaviour for ending the TB, and we've made
> them handle part of it themselves rather than just letting
> the end-of-TB code do it for them.

I agree.

Indeed, the fact that you've found 5 cases that are all the same suggests there 
*should* be common handling for them, and you're undoing that.

Enumeratiors are not expensive.  If you found that none of the existing cases 
works for some case, then add another enumerator.


r~