[PATCH 0/4] target/rx: Track PSW.U in tb->flags

[PATCH 0/4] target/rx: Track PSW.U in tb->flags

[Richard Henderson]

This is a follow up to Kawada-san's patch for the problem
of a missed update to the stack pointer in CLRPSW/SETPSW.
This fixes the problem without movcond by tracking the
current state of PSW.U within the TB.


r~


Richard Henderson (4):
  target/rx: Put tb_flags into DisasContext
  target/rx: Store PSW.U in tb->flags
  target/rx: Move DISAS_UPDATE check for write to PSW
  target/rx: Swap stack pointers on clrpsw/setpsw instruction

 target/rx/cpu.h       |  1 +
 target/rx/translate.c | 67 +++++++++++++++++++++++--------------------
 2 files changed, 37 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH 1/4] target/rx: Put tb_flags into DisasContext

[Richard Henderson]

Copy tb->flags into ctx->tb_flags; we'll want to modify
this value throughout the tb in future.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/rx/translate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index 5db8f79a82..785cbd948e 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -32,6 +32,7 @@ typedef struct DisasContext {
     DisasContextBase base;
     CPURXState *env;
     uint32_t pc;
+    uint32_t tb_flags;
 } DisasContext;
 
 typedef struct DisasCompare {
@@ -231,7 +232,7 @@ static inline TCGv rx_load_source(DisasContext *ctx, TCGv mem,
 /* Processor mode check */
 static int is_privileged(DisasContext *ctx, int is_exception)
 {
-    if (FIELD_EX32(ctx->base.tb->flags, PSW, PM)) {
+    if (FIELD_EX32(ctx->tb_flags, PSW, PM)) {
         if (is_exception) {
             gen_helper_raise_privilege_violation(cpu_env);
         }
@@ -2292,6 +2293,7 @@ static void rx_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     CPURXState *env = cs->env_ptr;
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     ctx->env = env;
+    ctx->tb_flags = ctx->base.tb->flags;
 }
 
 static void rx_tr_tb_start(DisasContextBase *dcbase, CPUState *cs)
-- 
2.25.1

[PATCH 2/4] target/rx: Store PSW.U in tb->flags

[Richard Henderson]

With this, we don't need movcond to determine
which stack pointer is current.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/rx/cpu.h       |  1 +
 target/rx/translate.c | 42 +++++++++++++++++++++++-------------------
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/target/rx/cpu.h b/target/rx/cpu.h
index b4abd90ccd..5e285401e8 100644
--- a/target/rx/cpu.h
+++ b/target/rx/cpu.h
@@ -148,6 +148,7 @@ static inline void cpu_get_tb_cpu_state(CPURXState *env, target_ulong *pc,
     *pc = env->pc;
     *cs_base = 0;
     *flags = FIELD_DP32(0, PSW, PM, env->psw_pm);
+    *flags = FIELD_DP32(*flags, PSW, U, env->psw_u);
 }
 
 static inline int cpu_mmu_index(CPURXState *env, bool ifetch)
diff --git a/target/rx/translate.c b/target/rx/translate.c
index 785cbd948e..586342eae7 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -311,9 +311,8 @@ static void psw_cond(DisasCompare *dc, uint32_t cond)
     }
 }
 
-static void move_from_cr(TCGv ret, int cr, uint32_t pc)
+static void move_from_cr(DisasContext *ctx, TCGv ret, int cr, uint32_t pc)
 {
-    TCGv z = tcg_const_i32(0);
     switch (cr) {
     case 0:     /* PSW */
         gen_helper_pack_psw(ret, cpu_env);
@@ -322,8 +321,11 @@ static void move_from_cr(TCGv ret, int cr, uint32_t pc)
         tcg_gen_movi_i32(ret, pc);
         break;
     case 2:     /* USP */
-        tcg_gen_movcond_i32(TCG_COND_NE, ret,
-                            cpu_psw_u, z, cpu_sp, cpu_usp);
+        if (FIELD_EX32(ctx->tb_flags, PSW, U)) {
+            tcg_gen_mov_i32(ret, cpu_sp);
+        } else {
+            tcg_gen_mov_i32(ret, cpu_usp);
+        }
         break;
     case 3:     /* FPSW */
         tcg_gen_mov_i32(ret, cpu_fpsw);
@@ -335,8 +337,11 @@ static void move_from_cr(TCGv ret, int cr, uint32_t pc)
         tcg_gen_mov_i32(ret, cpu_bpc);
         break;
     case 10:    /* ISP */
-        tcg_gen_movcond_i32(TCG_COND_EQ, ret,
-                            cpu_psw_u, z, cpu_sp, cpu_isp);
+        if (FIELD_EX32(ctx->tb_flags, PSW, U)) {
+            tcg_gen_mov_i32(ret, cpu_isp);
+        } else {
+            tcg_gen_mov_i32(ret, cpu_sp);
+        }
         break;
     case 11:    /* FINTV */
         tcg_gen_mov_i32(ret, cpu_fintv);
@@ -350,28 +355,27 @@ static void move_from_cr(TCGv ret, int cr, uint32_t pc)
         tcg_gen_movi_i32(ret, 0);
         break;
     }
-    tcg_temp_free(z);
 }
 
 static void move_to_cr(DisasContext *ctx, TCGv val, int cr)
 {
-    TCGv z;
     if (cr >= 8 && !is_privileged(ctx, 0)) {
         /* Some control registers can only be written in privileged mode. */
         qemu_log_mask(LOG_GUEST_ERROR,
                       "disallow control register write %s", rx_crname(cr));
         return;
     }
-    z = tcg_const_i32(0);
     switch (cr) {
     case 0:     /* PSW */
         gen_helper_set_psw(cpu_env, val);
         break;
     /* case 1: to PC not supported */
     case 2:     /* USP */
-        tcg_gen_mov_i32(cpu_usp, val);
-        tcg_gen_movcond_i32(TCG_COND_NE, cpu_sp,
-                            cpu_psw_u, z,  cpu_usp, cpu_sp);
+        if (FIELD_EX32(ctx->tb_flags, PSW, U)) {
+            tcg_gen_mov_i32(cpu_sp, val);
+        } else {
+            tcg_gen_mov_i32(cpu_usp, val);
+        }
         break;
     case 3:     /* FPSW */
         gen_helper_set_fpsw(cpu_env, val);
@@ -383,10 +387,11 @@ static void move_to_cr(DisasContext *ctx, TCGv val, int cr)
         tcg_gen_mov_i32(cpu_bpc, val);
         break;
     case 10:    /* ISP */
-        tcg_gen_mov_i32(cpu_isp, val);
-        /* if PSW.U is 0, copy isp to r0 */
-        tcg_gen_movcond_i32(TCG_COND_EQ, cpu_sp,
-                            cpu_psw_u, z,  cpu_isp, cpu_sp);
+        if (FIELD_EX32(ctx->tb_flags, PSW, U)) {
+            tcg_gen_mov_i32(cpu_isp, val);
+        } else {
+            tcg_gen_mov_i32(cpu_sp, val);
+        }
         break;
     case 11:    /* FINTV */
         tcg_gen_mov_i32(cpu_fintv, val);
@@ -399,7 +404,6 @@ static void move_to_cr(DisasContext *ctx, TCGv val, int cr)
                       "Unimplement control register %d", cr);
         break;
     }
-    tcg_temp_free(z);
 }
 
 static void push(TCGv val)
@@ -683,7 +687,7 @@ static bool trans_PUSHC(DisasContext *ctx, arg_PUSHC *a)
 {
     TCGv val;
     val = tcg_temp_new();
-    move_from_cr(val, a->cr, ctx->pc);
+    move_from_cr(ctx, val, a->cr, ctx->pc);
     push(val);
     tcg_temp_free(val);
     return true;
@@ -2221,7 +2225,7 @@ static bool trans_MVTC_r(DisasContext *ctx, arg_MVTC_r *a)
 /* mvfc rs, rd */
 static bool trans_MVFC(DisasContext *ctx, arg_MVFC *a)
 {
-    move_from_cr(cpu_regs[a->rd], a->cr, ctx->pc);
+    move_from_cr(ctx, cpu_regs[a->rd], a->cr, ctx->pc);
     return true;
 }
 
-- 
2.25.1

[PATCH 3/4] target/rx: Move DISAS_UPDATE check for write to PSW

[Richard Henderson]

Have one check in move_to_cr instead of one in each
function that calls move_to_cr.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/rx/translate.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index 586342eae7..d8b9ef635c 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -368,6 +368,10 @@ static void move_to_cr(DisasContext *ctx, TCGv val, int cr)
     switch (cr) {
     case 0:     /* PSW */
         gen_helper_set_psw(cpu_env, val);
+        if (is_privileged(ctx, 0)) {
+            /* PSW.{I,U} may be updated here. exit TB. */
+            ctx->base.is_jmp = DISAS_UPDATE;
+        }
         break;
     /* case 1: to PC not supported */
     case 2:     /* USP */
@@ -631,10 +635,6 @@ static bool trans_POPC(DisasContext *ctx, arg_POPC *a)
     val = tcg_temp_new();
     pop(val);
     move_to_cr(ctx, val, a->cr);
-    if (a->cr == 0 && is_privileged(ctx, 0)) {
-        /* PSW.I may be updated here. exit TB. */
-        ctx->base.is_jmp = DISAS_UPDATE;
-    }
     tcg_temp_free(val);
     return true;
 }
@@ -2205,9 +2205,6 @@ static bool trans_MVTC_i(DisasContext *ctx, arg_MVTC_i *a)
 
     imm = tcg_const_i32(a->imm);
     move_to_cr(ctx, imm, a->cr);
-    if (a->cr == 0 && is_privileged(ctx, 0)) {
-        ctx->base.is_jmp = DISAS_UPDATE;
-    }
     tcg_temp_free(imm);
     return true;
 }
@@ -2216,9 +2213,6 @@ static bool trans_MVTC_i(DisasContext *ctx, arg_MVTC_i *a)
 static bool trans_MVTC_r(DisasContext *ctx, arg_MVTC_r *a)
 {
     move_to_cr(ctx, cpu_regs[a->rs], a->cr);
-    if (a->cr == 0 && is_privileged(ctx, 0)) {
-        ctx->base.is_jmp = DISAS_UPDATE;
-    }
     return true;
 }
 
-- 
2.25.1

[PATCH 4/4] target/rx: Swap stack pointers on clrpsw/setpsw instruction

[Richard Henderson]

We properly perform this swap in helper_set_psw for MVTC,
but we missed doing so for the CLRPSW/SETPSW of the U bit.

Reported-by: Tomoaki Kawada <i@yvt.jp>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/rx/translate.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index d8b9ef635c..b34f238790 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -2165,7 +2165,12 @@ static inline void clrsetpsw(DisasContext *ctx, int cb, int val)
             ctx->base.is_jmp = DISAS_UPDATE;
             break;
         case PSW_U:
-            tcg_gen_movi_i32(cpu_psw_u, val);
+            if (FIELD_EX32(ctx->tb_flags, PSW, U) != val) {
+                ctx->tb_flags = FIELD_DP32(ctx->tb_flags, PSW, U, val);
+                tcg_gen_movi_i32(cpu_psw_u, val);
+                tcg_gen_mov_i32(val ? cpu_isp : cpu_usp, cpu_sp);
+                tcg_gen_mov_i32(cpu_sp, val ? cpu_usp : cpu_isp);
+            }
             break;
         default:
             qemu_log_mask(LOG_GUEST_ERROR, "Invalid distination %d", cb);
-- 
2.25.1

Re: [PATCH 0/4] target/rx: Track PSW.U in tb->flags

[Yoshinori Sato]

On Mon, 18 Apr 2022 01:51:26 +0900,
Richard Henderson wrote:
> 
> This is a follow up to Kawada-san's patch for the problem
> of a missed update to the stack pointer in CLRPSW/SETPSW.
> This fixes the problem without movcond by tracking the
> current state of PSW.U within the TB.
> 
> 
> r~
> 
> 
> Richard Henderson (4):
>   target/rx: Put tb_flags into DisasContext
>   target/rx: Store PSW.U in tb->flags
>   target/rx: Move DISAS_UPDATE check for write to PSW
>   target/rx: Swap stack pointers on clrpsw/setpsw instruction
> 
>  target/rx/cpu.h       |  1 +
>  target/rx/translate.c | 67 +++++++++++++++++++++++--------------------
>  2 files changed, 37 insertions(+), 31 deletions(-)
> 
> -- 
> 2.25.1
> 

Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>

-- 
Yosinori Sato

Re: [PATCH 0/4] target/rx: Track PSW.U in tb->flags

[Richard Henderson]

On 4/19/22 00:40, Yoshinori Sato wrote:
> On Mon, 18 Apr 2022 01:51:26 +0900,
> Richard Henderson wrote:
>>
>> This is a follow up to Kawada-san's patch for the problem
>> of a missed update to the stack pointer in CLRPSW/SETPSW.
>> This fixes the problem without movcond by tracking the
>> current state of PSW.U within the TB.
>>
>>
>> r~
>>
>>
>> Richard Henderson (4):
>>    target/rx: Put tb_flags into DisasContext
>>    target/rx: Store PSW.U in tb->flags
>>    target/rx: Move DISAS_UPDATE check for write to PSW
>>    target/rx: Swap stack pointers on clrpsw/setpsw instruction
>>
>>   target/rx/cpu.h       |  1 +
>>   target/rx/translate.c | 67 +++++++++++++++++++++++--------------------
>>   2 files changed, 37 insertions(+), 31 deletions(-)
>>
>> -- 
>> 2.25.1
>>
> 
> Reviewed-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> 

Queued to target-rx-next.

r~