| Message ID | ffb745754ddd4f389935cfb2ad4b6f3e560a2655.1500378931.git-series.james.hogan@imgtec.com |
|---|---|
| State | New |
| Headers | show |
On 18/07/2017 12:55, James Hogan wrote: > Using MFC0 to read CP0_UserLocal uses tcg_gen_ld32s_tl, however > CP0_UserLocal is a target_ulong. On a big endian host with a MIPS64 > target this reads and sign extends the more significant half of the > 64-bit register. > > Fix this by using ld_tl to load the whole target_ulong and ext32s_tl to > sign extend it, as done for various other target_ulong COP0 registers. > > Fixes: d279279e2b5c ("target-mips: implement UserLocal Register") > Signed-off-by: James Hogan <james.hogan@imgtec.com> > Cc: Yongbok Kim <yongbok.kim@imgtec.com> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Petar Jovanovic <petar.jovanovic@imgtec.com> > --- > Changes in v2: > - New patch. > --- > target/mips/translate.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 3022f349cb2a..556aba969a12 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -5138,8 +5138,9 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel) > goto cp0_unimplemented; > case 2: > CP0_CHECK(ctx->ulri); > - tcg_gen_ld32s_tl(arg, cpu_env, > - offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > + tcg_gen_ld_tl(arg, cpu_env, > + offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > + tcg_gen_ext32s_tl(arg, arg); > rn = "UserLocal"; > break; > default: > Reviewed-by: Yongbok Kim <yongbok.kim@imgtec.com> Regards, Yongbok
On 2017-07-18 12:55, James Hogan wrote: > Using MFC0 to read CP0_UserLocal uses tcg_gen_ld32s_tl, however > CP0_UserLocal is a target_ulong. On a big endian host with a MIPS64 > target this reads and sign extends the more significant half of the > 64-bit register. > > Fix this by using ld_tl to load the whole target_ulong and ext32s_tl to > sign extend it, as done for various other target_ulong COP0 registers. > > Fixes: d279279e2b5c ("target-mips: implement UserLocal Register") > Signed-off-by: James Hogan <james.hogan@imgtec.com> > Cc: Yongbok Kim <yongbok.kim@imgtec.com> > Cc: Aurelien Jarno <aurelien@aurel32.net> > Cc: Petar Jovanovic <petar.jovanovic@imgtec.com> > --- > Changes in v2: > - New patch. > --- > target/mips/translate.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/target/mips/translate.c b/target/mips/translate.c > index 3022f349cb2a..556aba969a12 100644 > --- a/target/mips/translate.c > +++ b/target/mips/translate.c > @@ -5138,8 +5138,9 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel) > goto cp0_unimplemented; > case 2: > CP0_CHECK(ctx->ulri); > - tcg_gen_ld32s_tl(arg, cpu_env, > - offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > + tcg_gen_ld_tl(arg, cpu_env, > + offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > + tcg_gen_ext32s_tl(arg, arg); > rn = "UserLocal"; > break; > default: I think this is what gen_mfc0_load64() does, that said this whole area probably need a rework (see for example how inefficiently gen_mfc0_load32 is implemented). So: Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
On Wed, Jul 19, 2017 at 12:27:50PM +0200, Aurelien Jarno wrote: > On 2017-07-18 12:55, James Hogan wrote: > > Using MFC0 to read CP0_UserLocal uses tcg_gen_ld32s_tl, however > > CP0_UserLocal is a target_ulong. On a big endian host with a MIPS64 > > target this reads and sign extends the more significant half of the > > 64-bit register. > > > > Fix this by using ld_tl to load the whole target_ulong and ext32s_tl to > > sign extend it, as done for various other target_ulong COP0 registers. > > > > Fixes: d279279e2b5c ("target-mips: implement UserLocal Register") > > Signed-off-by: James Hogan <james.hogan@imgtec.com> > > Cc: Yongbok Kim <yongbok.kim@imgtec.com> > > Cc: Aurelien Jarno <aurelien@aurel32.net> > > Cc: Petar Jovanovic <petar.jovanovic@imgtec.com> > > --- > > Changes in v2: > > - New patch. > > --- > > target/mips/translate.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/target/mips/translate.c b/target/mips/translate.c > > index 3022f349cb2a..556aba969a12 100644 > > --- a/target/mips/translate.c > > +++ b/target/mips/translate.c > > @@ -5138,8 +5138,9 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel) > > goto cp0_unimplemented; > > case 2: > > CP0_CHECK(ctx->ulri); > > - tcg_gen_ld32s_tl(arg, cpu_env, > > - offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > > + tcg_gen_ld_tl(arg, cpu_env, > > + offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > > + tcg_gen_ext32s_tl(arg, arg); > > rn = "UserLocal"; > > break; > > default: > > I think this is what gen_mfc0_load64() does, that said this whole area Ah yes, that could do with some wider use (and possibly s/64/tl/ or something). > probably need a rework (see for example how inefficiently > gen_mfc0_load32 is implemented). So: Erm, doesn't gen_mfc0_load32() fail to sign extend as it should when used for mfc0...? > > Reviewed-by: Aurelien Jarno <aurelien@aurel32.net> Cheers James
On 2017-07-19 14:44, James Hogan wrote: > On Wed, Jul 19, 2017 at 12:27:50PM +0200, Aurelien Jarno wrote: > > On 2017-07-18 12:55, James Hogan wrote: > > > Using MFC0 to read CP0_UserLocal uses tcg_gen_ld32s_tl, however > > > CP0_UserLocal is a target_ulong. On a big endian host with a MIPS64 > > > target this reads and sign extends the more significant half of the > > > 64-bit register. > > > > > > Fix this by using ld_tl to load the whole target_ulong and ext32s_tl to > > > sign extend it, as done for various other target_ulong COP0 registers. > > > > > > Fixes: d279279e2b5c ("target-mips: implement UserLocal Register") > > > Signed-off-by: James Hogan <james.hogan@imgtec.com> > > > Cc: Yongbok Kim <yongbok.kim@imgtec.com> > > > Cc: Aurelien Jarno <aurelien@aurel32.net> > > > Cc: Petar Jovanovic <petar.jovanovic@imgtec.com> > > > --- > > > Changes in v2: > > > - New patch. > > > --- > > > target/mips/translate.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/mips/translate.c b/target/mips/translate.c > > > index 3022f349cb2a..556aba969a12 100644 > > > --- a/target/mips/translate.c > > > +++ b/target/mips/translate.c > > > @@ -5138,8 +5138,9 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel) > > > goto cp0_unimplemented; > > > case 2: > > > CP0_CHECK(ctx->ulri); > > > - tcg_gen_ld32s_tl(arg, cpu_env, > > > - offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > > > + tcg_gen_ld_tl(arg, cpu_env, > > > + offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); > > > + tcg_gen_ext32s_tl(arg, arg); > > > rn = "UserLocal"; > > > break; > > > default: > > > > I think this is what gen_mfc0_load64() does, that said this whole area > > Ah yes, that could do with some wider use (and possibly s/64/tl/ or > something). > > > probably need a rework (see for example how inefficiently > > gen_mfc0_load32 is implemented). So: > > Erm, doesn't gen_mfc0_load32() fail to sign extend as it should when > used for mfc0...? Yes, this is correct. tcg_gen_ext_i32_tl sign-extends the value when converting it from TCGv_i32 to TCGv_tl. tcg_gen_extu_i32_tl is the op which zero-extends the value. Basically TCG sign-extends the value "by default", like for MIPS. Aurelien
diff --git a/target/mips/translate.c b/target/mips/translate.c index 3022f349cb2a..556aba969a12 100644 --- a/target/mips/translate.c +++ b/target/mips/translate.c @@ -5138,8 +5138,9 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel) goto cp0_unimplemented; case 2: CP0_CHECK(ctx->ulri); - tcg_gen_ld32s_tl(arg, cpu_env, - offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); + tcg_gen_ld_tl(arg, cpu_env, + offsetof(CPUMIPSState, active_tc.CP0_UserLocal)); + tcg_gen_ext32s_tl(arg, arg); rn = "UserLocal"; break; default:
Using MFC0 to read CP0_UserLocal uses tcg_gen_ld32s_tl, however CP0_UserLocal is a target_ulong. On a big endian host with a MIPS64 target this reads and sign extends the more significant half of the 64-bit register. Fix this by using ld_tl to load the whole target_ulong and ext32s_tl to sign extend it, as done for various other target_ulong COP0 registers. Fixes: d279279e2b5c ("target-mips: implement UserLocal Register") Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: Yongbok Kim <yongbok.kim@imgtec.com> Cc: Aurelien Jarno <aurelien@aurel32.net> Cc: Petar Jovanovic <petar.jovanovic@imgtec.com> --- Changes in v2: - New patch. --- target/mips/translate.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)