qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Petar Jovanovic
Subject: Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
Date: Thu, 29 May 2014 13:01:38 +0000

> 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.

Initial version of this patch did not have this flag, but it was added
as requested in code review.
What do you suggest?

____________________________________
From: Aurelien Jarno address@hidden
Sent: Thursday, May 29, 2014 2:49 PM
To: Petar Jovanovic
Cc: address@hidden; Petar Jovanovic; address@hidden; address@hidden
Subject: Re: [v3 PATCH] target-mips: implement UserLocal Register

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]