| Message ID | 20230614165934.1370440-4-kbastian@mail.uni-paderborn.de |
|---|---|
| State | New |
| Headers | show |
| Series | TriCore Privilege Levels | expand |
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 > @@ -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. 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. 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. r~
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? Thanks, Bastian [1] https://www.infineon.com/dgdl/Infineon-AURIX_TC3xx_Architecture_vol1-UserManual-v01_00-EN.pdf?fileId=5546d46276fb756a01771bc4c2e33bdd
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
diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c index 026e15f3e0..17b78c501c 100644 --- a/target/tricore/op_helper.c +++ b/target/tricore/op_helper.c @@ -2839,7 +2839,18 @@ void helper_rslcx(CPUTriCoreState *env) 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); + } } uint32_t helper_psw_read(CPUTriCoreState *env) diff --git a/target/tricore/translate.c b/target/tricore/translate.c index edbc319fa1..baf13fc205 100644 --- a/target/tricore/translate.c +++ b/target/tricore/translate.c @@ -331,6 +331,7 @@ static void gen_swapmsk(DisasContext *ctx, int reg, TCGv ea) tcg_gen_mov_tl(cpu_gpr_d[reg], temp); } +static inline void gen_save_pc(target_ulong pc); /* We generate loads and store to core special function register (csfr) through the function gen_mfcr and gen_mtcr. To handle access permissions, we use 3 @@ -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); } else { switch (offset) {
the CPU can change the privilege level by writing the corresponding bits in PSW. If this happens all instructions after this 'mtcr' in the TB are translated with the wrong privilege level. So we have to exit to the cpu_loop() and start translating again with the new privilege level. Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> --- target/tricore/op_helper.c | 11 +++++++++++ target/tricore/translate.c | 2 ++ 2 files changed, 13 insertions(+)