Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register

[Aurelien Jarno]

On Thu, May 29, 2014 at 02:58:20AM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
> 
> From MIPS documentation (Volume III):
> 
> UserLocal Register (CP0 Register 4, Select 2)
> Compliance Level: Recommended.
> 
> The UserLocal register is a read-write register that is not interpreted by
> the hardware and conditionally readable via the RDHWR instruction.
> 
> This register only exists if the Config3-ULRI register field is set.
> 
> Privileged software may write this register with arbitrary information and
> make it accessible to unprivileged software via register 29 (ULR) of the
> RDHWR instruction. To do so, bit 29 of the HWREna register must be set to a
> 1 to enable unprivileged access to the register.
> 
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
> v3:
> - new hflag MIPS_HFLAG_HWRENA_ULR introduced, it is set when ULR bit
>   from HWREna is set
> - helper rdhwr_ul removed, now the checks for rdhwr are done at
>   translation time
> - CPU_SAVE_VERSION switched to 4, load_tc supports both (3 and 4)
>   version ids
> 
> v2:
> - Defined MIPS_HFLAG_CP0UL flag, checks are now based on hflags
> - CP0_UserLocal moved to struct TCState
> - Added tc->CP0_UserLocal in save_tc/load_tc in target-mips/machine.c
> - Reused CP0_UserLocal field for user-mode purpose
> 
>  linux-user/mips/target_cpu.h |    2 +-
>  linux-user/syscall.c         |    2 +-
>  target-mips/cpu.h            |   13 +++++++---
>  target-mips/machine.c        |   13 +++++++---
>  target-mips/op_helper.c      |   14 ++++++++++-
>  target-mips/translate.c      |   55 
> +++++++++++++++++++++++++++++++++++++++---
>  6 files changed, 85 insertions(+), 14 deletions(-)
> 
> diff --git a/linux-user/mips/target_cpu.h b/linux-user/mips/target_cpu.h
> index ba8e9eb..19b8855 100644
> --- a/linux-user/mips/target_cpu.h
> +++ b/linux-user/mips/target_cpu.h
> @@ -30,7 +30,7 @@ static inline void cpu_clone_regs(CPUMIPSState *env, 
> target_ulong newsp)
>  
>  static inline void cpu_set_tls(CPUMIPSState *env, target_ulong newtls)
>  {
> -    env->tls_value = newtls;
> +    env->active_tc.CP0_UserLocal = newtls;
>  }
>  
>  #endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 6efeeff..fda8dd6 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8686,7 +8686,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  #ifdef TARGET_NR_set_thread_area
>      case TARGET_NR_set_thread_area:
>  #if defined(TARGET_MIPS)
> -      ((CPUMIPSState *) cpu_env)->tls_value = arg1;
> +      ((CPUMIPSState *) cpu_env)->active_tc.CP0_UserLocal = arg1;
>        ret = 0;
>        break;
>  #elif defined(TARGET_CRIS)
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 6c2014e..67bf441 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -167,6 +167,7 @@ struct TCState {
>      target_ulong CP0_TCSchedule;
>      target_ulong CP0_TCScheFBack;
>      int32_t CP0_Debug_tcstatus;
> +    target_ulong CP0_UserLocal;
>  };
>  
>  typedef struct CPUMIPSState CPUMIPSState;
> @@ -361,6 +362,7 @@ struct CPUMIPSState {
>      int32_t CP0_Config3;
>  #define CP0C3_M    31
>  #define CP0C3_ISA_ON_EXC 16
> +#define CP0C3_ULRI 13
>  #define CP0C3_DSPP 10
>  #define CP0C3_LPA  7
>  #define CP0C3_VEIC 6
> @@ -469,6 +471,10 @@ struct CPUMIPSState {
>      /* MIPS DSP resources access. */
>  #define MIPS_HFLAG_DSP   0x40000  /* Enable access to MIPS DSP resources. */
>  #define MIPS_HFLAG_DSPR2 0x80000  /* Enable access to MIPS DSPR2 resources. 
> */
> +    /* Extra flags about UserLocal register. */
> +#define MIPS_HFLAG_CP0UL      0x100000 /* CP0_UserLocal is implemented. */
> +#define MIPS_HFLAG_HWRENA_ULR 0x200000 /* ULR bit from HWREna is set. */
> +#define MIPS_HFLAG_UL_MASK  (MIPS_HFLAG_CP0UL | MIPS_HFLAG_HWRENA_ULR)

While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance
reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is
always the same (true or false) during all the run time of the
qemu-system-mips binary, and thus we don't need to take care of code
generated with it being true or being false.


>      target_ulong btarget;        /* Jump / branch target               */
>      target_ulong bcond;          /* Branch condition (if needed)       */
>  
> @@ -478,8 +484,6 @@ struct CPUMIPSState {
>      uint32_t CP0_TCStatus_rw_bitmask; /* Read/write bits in CP0_TCStatus */
>      int insn_flags; /* Supported instruction set */
>  
> -    target_ulong tls_value; /* For usermode emulation */
> -
>      CPU_COMMON
>  
>      /* Fields from here on are preserved across CPU reset. */
> @@ -522,7 +526,7 @@ void mips_cpu_list (FILE *f, fprintf_function 
> cpu_fprintf);
>  extern void cpu_wrdsp(uint32_t rs, uint32_t mask_num, CPUMIPSState *env);
>  extern uint32_t cpu_rddsp(uint32_t mask_num, CPUMIPSState *env);
>  
> -#define CPU_SAVE_VERSION 3
> +#define CPU_SAVE_VERSION 4
>  
>  /* MMU modes definitions. We carefully match the indices with our
>     hflags layout. */
> @@ -681,7 +685,8 @@ static inline void cpu_get_tb_cpu_state(CPUMIPSState 
> *env, target_ulong *pc,
>  {
>      *pc = env->active_tc.PC;
>      *cs_base = 0;
> -    *flags = env->hflags & (MIPS_HFLAG_TMASK | MIPS_HFLAG_BMASK);
> +    *flags = env->hflags & (MIPS_HFLAG_TMASK | MIPS_HFLAG_BMASK |
> +                            MIPS_HFLAG_UL_MASK);
>  }
>  
>  static inline int mips_vpe_active(CPUMIPSState *env)
> diff --git a/target-mips/machine.c b/target-mips/machine.c
> index 0a07db8..0f36c9e 100644
> --- a/target-mips/machine.c
> +++ b/target-mips/machine.c
> @@ -25,6 +25,7 @@ static void save_tc(QEMUFile *f, TCState *tc)
>      qemu_put_betls(f, &tc->CP0_TCSchedule);
>      qemu_put_betls(f, &tc->CP0_TCScheFBack);
>      qemu_put_sbe32s(f, &tc->CP0_Debug_tcstatus);
> +    qemu_put_betls(f, &tc->CP0_UserLocal);
>  }
>  
>  static void save_fpu(QEMUFile *f, CPUMIPSFPUContext *fpu)
> @@ -151,7 +152,7 @@ void cpu_save(QEMUFile *f, void *opaque)
>          save_fpu(f, &env->fpus[i]);
>  }
>  
> -static void load_tc(QEMUFile *f, TCState *tc)
> +static void load_tc(QEMUFile *f, TCState *tc, int version_id)
>  {
>      int i;
>  
> @@ -173,6 +174,9 @@ static void load_tc(QEMUFile *f, TCState *tc)
>      qemu_get_betls(f, &tc->CP0_TCSchedule);
>      qemu_get_betls(f, &tc->CP0_TCScheFBack);
>      qemu_get_sbe32s(f, &tc->CP0_Debug_tcstatus);
> +    if (version_id >= 4) {
> +        qemu_get_betls(f, &tc->CP0_UserLocal);
> +    }
>  }
>  
>  static void load_fpu(QEMUFile *f, CPUMIPSFPUContext *fpu)
> @@ -194,11 +198,12 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
>      MIPSCPU *cpu = mips_env_get_cpu(env);
>      int i;
>  
> -    if (version_id != 3)
> +    if (version_id < 3) {
>          return -EINVAL;
> +    }
>  
>      /* Load active TC */
> -    load_tc(f, &env->active_tc);
> +    load_tc(f, &env->active_tc, version_id);
>  
>      /* Load active FPU */
>      load_fpu(f, &env->active_fpu);
> @@ -299,7 +304,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
>  
>      /* Load inactive TC state */
>      for (i = 0; i < MIPS_SHADOW_SET_MAX; i++)
> -        load_tc(f, &env->tcs[i]);
> +        load_tc(f, &env->tcs[i], version_id);
>      for (i = 0; i < MIPS_FPU_MAX; i++)
>          load_fpu(f, &env->fpus[i]);
>  
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 8af931a..6e3ce13 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -1301,7 +1301,19 @@ void helper_mtc0_srsconf4(CPUMIPSState *env, 
> target_ulong arg1)
>  
>  void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong arg1)
>  {
> -    env->CP0_HWREna = arg1 & 0x0000000F;
> +    uint32_t mask = 0x0000000F;
> +
> +    if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
> +        mask |= (1 << 29);
> +
> +        if (arg1 & (1 << 29)) {
> +            env->hflags |= MIPS_HFLAG_HWRENA_ULR;
> +        } else {
> +            env->hflags &= ~MIPS_HFLAG_HWRENA_ULR;
> +        }
> +    }
> +
> +    env->CP0_HWREna = arg1 & mask;
>  }
>  
>  void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1)
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 13cf29b..37852c7 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -4214,7 +4214,17 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 1:
>  //            gen_helper_mfc0_contextconfig(arg); /* SmartMIPS ASE */
>              rn = "ContextConfig";
> +            goto die;
>  //            break;
> +        case 2:
> +            if (ctx->hflags & MIPS_HFLAG_CP0UL) {
> +                tcg_gen_ld32s_tl(arg, cpu_env,
> +                                 offsetof(CPUMIPSState,
> +                                          active_tc.CP0_UserLocal));
> +                rn = "UserLocal";
> +            } else {
> +                tcg_gen_movi_tl(arg, 0);
> +            }
>          default:
>              goto die;
>          }
> @@ -4801,7 +4811,15 @@ static void gen_mtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 1:
>  //            gen_helper_mtc0_contextconfig(cpu_env, arg); /* SmartMIPS ASE 
> */
>              rn = "ContextConfig";
> +            goto die;
>  //            break;
> +        case 2:
> +            if (ctx->hflags & MIPS_HFLAG_CP0UL) {
> +                tcg_gen_st_tl(arg, cpu_env,
> +                              offsetof(CPUMIPSState, 
> active_tc.CP0_UserLocal));
> +                rn = "UserLocal";
> +            }
> +            break;
>          default:
>              goto die;
>          }
> @@ -4861,6 +4879,7 @@ static void gen_mtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 0:
>              check_insn(ctx, ISA_MIPS32R2);
>              gen_helper_mtc0_hwrena(cpu_env, arg);
> +            ctx->bstate = BS_STOP;
>              rn = "HWREna";
>              break;
>          default:
> @@ -5405,7 +5424,17 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 1:
>  //            gen_helper_dmfc0_contextconfig(arg); /* SmartMIPS ASE */
>              rn = "ContextConfig";
> +            goto die;
>  //            break;
> +        case 2:
> +            if (ctx->hflags & MIPS_HFLAG_CP0UL) {
> +                tcg_gen_ld_tl(arg, cpu_env,
> +                              offsetof(CPUMIPSState, 
> active_tc.CP0_UserLocal));
> +                rn = "UserLocal";
> +            } else {
> +                tcg_gen_movi_tl(arg, 0);
> +            }
> +            break;
>          default:
>              goto die;
>          }
> @@ -5977,7 +6006,15 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 1:
>  //           gen_helper_mtc0_contextconfig(cpu_env, arg); /* SmartMIPS ASE */
>              rn = "ContextConfig";
> +            goto die;
>  //           break;
> +        case 2:
> +            if (ctx->hflags & MIPS_HFLAG_CP0UL) {
> +                tcg_gen_st_tl(arg, cpu_env,
> +                              offsetof(CPUMIPSState, 
> active_tc.CP0_UserLocal));
> +                rn = "UserLocal";
> +            }
> +            break;
>          default:
>              goto die;
>          }
> @@ -6037,6 +6074,7 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 0:
>              check_insn(ctx, ISA_MIPS32R2);
>              gen_helper_mtc0_hwrena(cpu_env, arg);
> +            ctx->bstate = BS_STOP;
>              rn = "HWREna";
>              break;
>          default:
> @@ -9059,12 +9097,20 @@ static void gen_rdhwr(DisasContext *ctx, int rt, int 
> rd)
>          break;
>      case 29:
>  #if defined(CONFIG_USER_ONLY)
> -        tcg_gen_ld_tl(t0, cpu_env, offsetof(CPUMIPSState, tls_value));
> +        tcg_gen_ld_tl(t0, cpu_env,
> +                      offsetof(CPUMIPSState, active_tc.CP0_UserLocal));
>          gen_store_gpr(t0, rt);
>          break;
>  #else
> -        /* XXX: Some CPUs implement this in hardware.
> -           Not supported yet. */
> +        if ((ctx->hflags & MIPS_HFLAG_CP0) ||
> +            (ctx->hflags & MIPS_HFLAG_HWRENA_ULR)) {
> +            tcg_gen_ld_tl(t0, cpu_env,
> +                          offsetof(CPUMIPSState, active_tc.CP0_UserLocal));
> +            gen_store_gpr(t0, rt);
> +        } else {
> +            generate_exception(ctx, EXCP_RI);
> +        }
> +        break;
>  #endif
>      default:            /* Invalid */
>          MIPS_INVAL("rdhwr");
> @@ -16005,6 +16051,9 @@ void cpu_state_reset(CPUMIPSState *env)
>      if (env->CP0_Config3 & (1 << CP0C3_DSPP)) {
>          env->CP0_Status |= (1 << CP0St_MX);
>      }
> +    if (env->CP0_Config3 & (1 << CP0C3_ULRI)) {
> +        env->hflags |= MIPS_HFLAG_CP0UL;
> +    }
>  # if defined(TARGET_MIPS64)
>      /* For MIPS64, init FR bit to 1 if FPU unit is there and bit is 
> writable. */
>      if ((env->CP0_Config1 & (1 << CP0C1_FP)) &&
> -- 
> 1.7.9.5
> 
> 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net