Re: [PATCH 3/4] target/tricore: Honour privilege changes on PSW write

[Bastian Koppelmann]

On Thu, Jun 15, 2023 at 05:15:28PM +0200, Bastian Koppelmann wrote:
> On Thu, Jun 15, 2023 at 09:37:23AM +0200, Richard Henderson wrote:
> > On 6/14/23 18:59, Bastian Koppelmann wrote:
> > >   void helper_psw_write(CPUTriCoreState *env, uint32_t arg)
> > >   {
> > > +    uint32_t old_priv, new_priv;
> > > +    CPUState *cs;
> > > +
> > > +    old_priv = extract32(env->PSW, 10, 2);
> > >       psw_write(env, arg);
> > > +    new_priv = extract32(env->PSW, 10, 2);
> > > +
> > > +    if (old_priv != new_priv) {
> > > +        cs = env_cpu(env);
> > > +        env->PC = env->PC + 4;
> > > +        cpu_loop_exit(cs);
> > > +    }
> > >   }
> > 
> > I think you should unconditionally end the TB after write to PSW. I think
> > that you should not manipulate the PC here, nor use cpu_loop_exit.
> > 
> > You should add
> > 
> > #define DISAS_EXIT         DISAS_TARGET_0
> > #define DISAS_EXIT_UPDATE  DISAS_TARGET_1
> 
> ok.
> 
> > 
> > > @@ -378,6 +379,7 @@ static inline void gen_mtcr(DisasContext *ctx, TCGv 
> > > r1,
> > >      if (ctx->priv == TRICORE_PRIV_SM) {
> > >          /* since we're caching PSW make this a special case */
> > >          if (offset == 0xfe04) {
> > > +            gen_save_pc(ctx->base.pc_next);
> > >              gen_helper_psw_write(cpu_env, r1);
> > 
> > Instead set ctx->base.is_jmp = DISAS_EXIT,
> > 
> > and in tricore_tr_tb_stop add
> > 
> >     case DISAS_EXIT_UPDATE:
> >         gen_save_pc(ctx->base.pc_next);
> >         /* fall through */
> >     case DISAS_EXIT:
> >         tcg_gen_exit_tb(NULL, 0);
> >         break;
> > 
> > There are a number of places (e.g. rfe), which can then use DISAS_EXIT
> > instead of issuing the exit directly.
> 
> ok.
> 
> > 
> > I'll also say that there are a number of other places using tcg_gen_exit_tb
> > which should instead be using tcg_gen_lookup_and_goto_ptr -- all of the
> > indirect branches for instance.  I would suggest adding
> > 
> > #define DISAS_JUMP    DISAS_TARGET_2
> > 
> > to handle those, again with the code within tricore_tr_tb_stop.
> 
> I'll look into that. However, this is out of scope for this patch series.
> 
> > 
> > Finally, does JLI really clobber A[11] before branching to A[a]?
> > If so, this could use a comment, because it looks like a bug.
> 
> Yes, it does. A[11] is the link register (not only by convention), so it is 
> hard
> coded to save the return address to A[11]. See [1] page 29. Why does it look 
> like a bug to you?

You're right this is a bug. If A[a] = A[11], then we're overwriting the jump
address. We have to save A[a] into a temp and then save A[11].

Thanks for finding this!

Cheers,
Bastian