target/rx: swap stack pointers on clrpsw/setpsw instruction

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

Posted by Tomoaki Kawada 3 years, 9 months ago

The control register field PSW.U determines which stack pointer register
(ISP or USP) is mapped as R0. In QEMU, this is implemented by having a
value copied between ISP or USP and R0 whenever PSW.U is updated or
access to ISP/USP is made by an mvtc/mvic instruction. However, this
update process was incorrectly omitted in the clrpsw/setpsw (clear/set
PSW) instructions, causing stack pointers to go out-of-sync.

This patch updates the clrpsw/setpsw translator to handle PSW.U updates
correctly and fix this problem.

Signed-off-by: Tomoaki Kawada <i@yvt.jp>
---
 target/rx/translate.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target/rx/translate.c b/target/rx/translate.c
index 5db8f79a82..c282433fb7 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -2135,6 +2135,7 @@ enum {
 
 static inline void clrsetpsw(DisasContext *ctx, int cb, int val)
 {
+    TCGv z;
     if (cb < 8) {
         switch (cb) {
         case PSW_C:
@@ -2160,7 +2161,22 @@ static inline void clrsetpsw(DisasContext *ctx, int cb, int val)
             ctx->base.is_jmp = DISAS_UPDATE;
             break;
         case PSW_U:
+            z = tcg_const_i32(0);
+
+            /* (PSW.U ? USP : ISP) = R0 */
+            tcg_gen_movcond_i32(TCG_COND_NE, cpu_usp,
+                                cpu_psw_u, z, cpu_sp, cpu_usp);
+            tcg_gen_movcond_i32(TCG_COND_EQ, cpu_isp,
+                                cpu_psw_u, z, cpu_sp, cpu_isp);
+
+            /* Set PSW.U */
             tcg_gen_movi_i32(cpu_psw_u, val);
+
+            /* R0 = (PSW.U ? USP : ISP) */
+            tcg_gen_movcond_i32(TCG_COND_NE, cpu_sp,
+                                cpu_psw_u, z, cpu_usp, cpu_isp);
+
+            tcg_temp_free(z);
             break;
         default:
             qemu_log_mask(LOG_GUEST_ERROR, "Invalid distination %d", cb);
-- 
2.35.1

Re: [PATCH] target/rx: swap stack pointers on clrpsw/setpsw instruction

Posted by Yoshinori Sato 3 years, 9 months ago

On Sat, 16 Apr 2022 12:20:09 +0900,
Tomoaki Kawada wrote:
> 
> The control register field PSW.U determines which stack pointer register
> (ISP or USP) is mapped as R0. In QEMU, this is implemented by having a
> value copied between ISP or USP and R0 whenever PSW.U is updated or
> access to ISP/USP is made by an mvtc/mvic instruction. However, this
> update process was incorrectly omitted in the clrpsw/setpsw (clear/set
> PSW) instructions, causing stack pointers to go out-of-sync.
> 
> This patch updates the clrpsw/setpsw translator to handle PSW.U updates
> correctly and fix this problem.
> 
> Signed-off-by: Tomoaki Kawada <i@yvt.jp>
> ---
>  target/rx/translate.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/target/rx/translate.c b/target/rx/translate.c
> index 5db8f79a82..c282433fb7 100644
> --- a/target/rx/translate.c
> +++ b/target/rx/translate.c
> @@ -2135,6 +2135,7 @@ enum {
>  
>  static inline void clrsetpsw(DisasContext *ctx, int cb, int val)
>  {
> +    TCGv z;
>      if (cb < 8) {
>          switch (cb) {
>          case PSW_C:
> @@ -2160,7 +2161,22 @@ static inline void clrsetpsw(DisasContext *ctx, int cb, int val)
>              ctx->base.is_jmp = DISAS_UPDATE;
>              break;
>          case PSW_U:
> +            z = tcg_const_i32(0);
> +
> +            /* (PSW.U ? USP : ISP) = R0 */
> +            tcg_gen_movcond_i32(TCG_COND_NE, cpu_usp,
> +                                cpu_psw_u, z, cpu_sp, cpu_usp);
> +            tcg_gen_movcond_i32(TCG_COND_EQ, cpu_isp,
> +                                cpu_psw_u, z, cpu_sp, cpu_isp);
> +
> +            /* Set PSW.U */
>              tcg_gen_movi_i32(cpu_psw_u, val);
> +
> +            /* R0 = (PSW.U ? USP : ISP) */
> +            tcg_gen_movcond_i32(TCG_COND_NE, cpu_sp,
> +                                cpu_psw_u, z, cpu_usp, cpu_isp);
> +
> +            tcg_temp_free(z);
>              break;
>          default:
>              qemu_log_mask(LOG_GUEST_ERROR, "Invalid distination %d", cb);
> -- 
> 2.35.1
> 

Overall looks good.
I have same comment as Richard.
Can you fix it like his comment?

Thaks.

-- 
Yosinori Sato

Re: [PATCH] target/rx: swap stack pointers on clrpsw/setpsw instruction

Posted by Richard Henderson 3 years, 9 months ago

On 4/15/22 20:20, Tomoaki Kawada wrote:
> The control register field PSW.U determines which stack pointer register
> (ISP or USP) is mapped as R0. In QEMU, this is implemented by having a
> value copied between ISP or USP and R0 whenever PSW.U is updated or
> access to ISP/USP is made by an mvtc/mvic instruction. However, this
> update process was incorrectly omitted in the clrpsw/setpsw (clear/set
> PSW) instructions, causing stack pointers to go out-of-sync.

Good catch.

>           case PSW_U:
> +            z = tcg_const_i32(0);

Use tcg_constant_i32(), which does not require the free at the end.

> +
> +            /* (PSW.U ? USP : ISP) = R0 */
> +            tcg_gen_movcond_i32(TCG_COND_NE, cpu_usp,
> +                                cpu_psw_u, z, cpu_sp, cpu_usp);
> +            tcg_gen_movcond_i32(TCG_COND_EQ, cpu_isp,
> +                                cpu_psw_u, z, cpu_sp, cpu_isp);

Ok.

> +            /* Set PSW.U */
>               tcg_gen_movi_i32(cpu_psw_u, val);
> +
> +            /* R0 = (PSW.U ? USP : ISP) */
> +            tcg_gen_movcond_i32(TCG_COND_NE, cpu_sp,
> +                                cpu_psw_u, z, cpu_usp, cpu_isp);

You don't need a movcond here, because you know exactly what the new value of psw_u is 
during translate: val.  This should be an if statement here.


r~