Re: [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping

[Leon Alrae]

On 19/11/2014 17:29, Maciej W. Rozycki wrote:
> qemu-mips32-addr.diff
> Index: qemu-git-trunk/target-mips/cpu.h
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/cpu.h     2014-11-12 07:41:26.597542010 
> +0000
> +++ qemu-git-trunk/target-mips/cpu.h  2014-11-12 07:41:26.597542010 +0000
> @@ -843,10 +843,12 @@ static inline void compute_hflags(CPUMIP
>          env->hflags |= MIPS_HFLAG_64;
>      }
>  
> -    if (((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) &&
> -        !(env->CP0_Status & (1 << CP0St_UX))) {
> +    if (!(env->insn_flags & ISA_MIPS3)) {
>          env->hflags |= MIPS_HFLAG_AWRAP;
> -    } else if (env->insn_flags & ISA_MIPS32R6) {
> +    } else if (((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) &&
> +               !(env->CP0_Status & (1 << CP0St_UX))) {
> +        env->hflags |= MIPS_HFLAG_AWRAP;
> +    } else if (env->insn_flags & ISA_MIPS64R6) {
>          /* Address wrapping for Supervisor and Kernel is specified in R6 */
>          if ((((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_SM) &&
>               !(env->CP0_Status & (1 << CP0St_SX))) ||
> Index: qemu-git-trunk/target-mips/translate.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/translate.c       2014-11-12 
> 07:41:26.597542010 +0000
> +++ qemu-git-trunk/target-mips/translate.c    2014-11-12 07:41:26.597542010 
> +0000
> @@ -10724,6 +10724,7 @@ static void gen_mips16_save (DisasContex
>  {
>      TCGv t0 = tcg_temp_new();
>      TCGv t1 = tcg_temp_new();
> +    TCGv t2 = tcg_temp_new();
>      int args, astatic;
>  
>      switch (aregs) {
> @@ -10782,7 +10783,8 @@ static void gen_mips16_save (DisasContex
>      gen_load_gpr(t0, 29);
>  
>  #define DECR_AND_STORE(reg) do {                                 \
> -        tcg_gen_subi_tl(t0, t0, 4);                              \
> +        tcg_gen_movi_tl(t2, -4);                                 \

Wouldn't it be better to move this line outside of the macro to avoid
generating unnecessary tcg ops? DECR_AND_STORE is called multiple times
and t2 doesn't change in-between.

> +        gen_op_addr_add(ctx, t0, t0, t2);                        \
>          gen_load_gpr(t1, reg);                                   \
>          tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL); \
>      } while (0)
> @@ -10866,9 +10868,11 @@ static void gen_mips16_save (DisasContex
>      }
>  #undef DECR_AND_STORE
>  
> -    tcg_gen_subi_tl(cpu_gpr[29], cpu_gpr[29], framesize);
> +    tcg_gen_movi_tl(t2, -framesize);
> +    gen_op_addr_add(ctx, cpu_gpr[29], cpu_gpr[29], t2);
>      tcg_temp_free(t0);
>      tcg_temp_free(t1);
> +    tcg_temp_free(t2);
>  }
>  
>  static void gen_mips16_restore (DisasContext *ctx,
> @@ -10879,11 +10883,14 @@ static void gen_mips16_restore (DisasCon
>      int astatic;
>      TCGv t0 = tcg_temp_new();
>      TCGv t1 = tcg_temp_new();
> +    TCGv t2 = tcg_temp_new();
>  
> -    tcg_gen_addi_tl(t0, cpu_gpr[29], framesize);
> +    tcg_gen_movi_tl(t2, framesize);
> +    gen_op_addr_add(ctx, t0, cpu_gpr[29], t2);
>  
>  #define DECR_AND_LOAD(reg) do {                            \
> -        tcg_gen_subi_tl(t0, t0, 4);                        \
> +        tcg_gen_movi_tl(t2, -4);                           \

The same here.

> +        gen_op_addr_add(ctx, t0, t0, t2);                  \
>          tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL); \
>          gen_store_gpr(t1, reg);                            \
>      } while (0)
> @@ -10967,9 +10974,11 @@ static void gen_mips16_restore (DisasCon
>      }
>  #undef DECR_AND_LOAD
>  
> -    tcg_gen_addi_tl(cpu_gpr[29], cpu_gpr[29], framesize);
> +    tcg_gen_movi_tl(t2, framesize);
> +    gen_op_addr_add(ctx, cpu_gpr[29], cpu_gpr[29], t2);
>      tcg_temp_free(t0);
>      tcg_temp_free(t1);
> +    tcg_temp_free(t2);
>  }
>  
>  static void gen_addiupc (DisasContext *ctx, int rx, int imm,
> 

Otherwise,

Reviewed-by: Leon Alrae <address@hidden>