Message ID | 20220416032009.1897719-1-i@yvt.jp |
---|---|
State | New |
Headers | show |
Series | target/rx: swap stack pointers on clrpsw/setpsw instruction | expand |
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~
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.
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);
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(+)