Re: [Qemu-devel] [PATCH 1/14] target/mips: Fix MIPS64 MFC0 UserLocal on BE host

[James Hogan]

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 <address@hidden>
> > Cc: Yongbok Kim <address@hidden>
> > Cc: Aurelien Jarno <address@hidden>
> > Cc: Petar Jovanovic <address@hidden>
> > ---
> > 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 <address@hidden>

Cheers
James

signature.asc
Description: Digital signature