Re: [Qemu-devel] [PATCH 4/7] target-mips: add MTHC0 and MFHC0 instructions

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 11290 bytes --]

Hi Leon,

On 28/04/15 13:41, Leon Alrae wrote:
> Implement MTHC0 and MFHC0 instructions. In MIPS32 they allow to access
> upper word of extended to 64-bits CP0 registers.
> 
> In MIPS64, when CP0 destination register specified is the EntryLo0 or
> EntryLo1, bits 1:0 of the GPR appear at bits 31:30 of EntryLo0 or
> EntryLo1. This is to compensate for RI and XI, which were shifted to bits
> 63:62 by MTC0 to EntryLo0 or EntryLo1. Therefore creating separate
> functions generating for EntryLo0 and EntryLo1.
> 
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> ---
>  disas/mips.c            |   2 +
>  target-mips/cpu.h       |   1 +
>  target-mips/translate.c | 216 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 219 insertions(+)
> 
> diff --git a/disas/mips.c b/disas/mips.c
> index 1afe0c5..c236495 100644
> --- a/disas/mips.c
> +++ b/disas/mips.c
> @@ -2238,6 +2238,8 @@ const struct mips_opcode mips_builtin_opcodes[] =
>  {"ceil.l.s", "D,S",	0x4600000a, 0xffff003f, WR_D|RD_S|FP_S|FP_D,	0,		I3|I33	},
>  {"ceil.w.d", "D,S",	0x4620000e, 0xffff003f, WR_D|RD_S|FP_S|FP_D,	0,		I2	},
>  {"ceil.w.s", "D,S",	0x4600000e, 0xffff003f, WR_D|RD_S|FP_S,		0,		I2	},
> +{"mfhc0",   "t,G,H",    0x40400000, 0xffe007f8, LCD|WR_t|RD_C0,        0, I33 },
> +{"mthc0",   "t,G,H",    0x40c00000, 0xffe007f8, COD|RD_t|WR_C0|WR_CC,  0, I33 },

whitespace appears to be inconsistent here (the context uses tabs).

>  {"cfc0",    "t,G",	0x40400000, 0xffe007ff,	LCD|WR_t|RD_C0,		0,		I1	},
>  {"cfc1",    "t,G",	0x44400000, 0xffe007ff,	LCD|WR_t|RD_C1|FP_S,	0,		I1	},
>  {"cfc1",    "t,S",	0x44400000, 0xffe007ff,	LCD|WR_t|RD_C1|FP_S,	0,		I1	},
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index bcd1e2b..cfc6ac7 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -473,6 +473,7 @@ struct CPUMIPSState {
>  #define CP0C5_UFE        9
>  #define CP0C5_FRE        8
>  #define CP0C5_SBRI       6
> +#define CP0C5_MVH        5
>  #define CP0C5_UFR        2
>  #define CP0C5_NFExists   0
>      int32_t CP0_Config6;
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index bb219ea..f95b655 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -868,8 +868,10 @@ enum {
>  enum {
>      OPC_MFC0     = (0x00 << 21) | OPC_CP0,
>      OPC_DMFC0    = (0x01 << 21) | OPC_CP0,
> +    OPC_MFHC0    = (0x02 << 21) | OPC_CP0,
>      OPC_MTC0     = (0x04 << 21) | OPC_CP0,
>      OPC_DMTC0    = (0x05 << 21) | OPC_CP0,
> +    OPC_MTHC0    = (0x06 << 21) | OPC_CP0,
>      OPC_MFTR     = (0x08 << 21) | OPC_CP0,
>      OPC_RDPGPR   = (0x0A << 21) | OPC_CP0,
>      OPC_MFMC0    = (0x0B << 21) | OPC_CP0,
> @@ -1423,6 +1425,8 @@ typedef struct DisasContext {
>      int ie;
>      bool bi;
>      bool bp;
> +    uint64_t PAMask;
> +    bool mvh;
>  } DisasContext;
>  
>  enum {
> @@ -1769,6 +1773,13 @@ static inline void check_cp1_registers(DisasContext *ctx, int regs)
>     This is enabled by CP0 Status register MX(24) bit.
>   */
>  
> +static inline void check_mvh(DisasContext *ctx)
> +{
> +    if (unlikely(!(ctx->mvh))) {

superfluous brackets around ctx->mvh.

> +        generate_exception(ctx, EXCP_RI);
> +    }
> +}
> +
>  static inline void check_dsp(DisasContext *ctx)
>  {
>      if (unlikely(!(ctx->hflags & MIPS_HFLAG_DSP))) {
> @@ -4820,6 +4831,60 @@ static void gen_bshfl (DisasContext *ctx, uint32_t op2, int rt, int rd)
>  
>  #ifndef CONFIG_USER_ONLY
>  /* CP0 (MMU and control) */
> +static inline void gen_mthc0_entrylo(TCGv arg, target_ulong off)
> +{
> +    TCGv_i64 t0 = tcg_temp_new_i64();
> +    TCGv_i64 t1 = tcg_temp_new_i64();
> +
> +    tcg_gen_ext_tl_i64(t0, arg);
> +    tcg_gen_ld_i64(t1, cpu_env, off);
> +#if defined(TARGET_MIPS64)
> +    tcg_gen_deposit_i64(t1, t1, t0, 30, 32);
> +#else
> +    tcg_gen_concat32_i64(t1, t1, t0);

I don't get what this case is about. what's wrong with the above case
for MIPS64 and MIPS32?

> +#endif
> +    tcg_gen_st_i64(t1, cpu_env, off);
> +    tcg_temp_free_i64(t1);
> +    tcg_temp_free_i64(t0);
> +}
> +
> +static inline void gen_mthc0_store64(TCGv arg, target_ulong off)
> +{
> +    TCGv_i64 t0 = tcg_temp_new_i64();
> +    TCGv_i64 t1 = tcg_temp_new_i64();
> +
> +    tcg_gen_ext_tl_i64(t0, arg);
> +    tcg_gen_ld_i64(t1, cpu_env, off);
> +    tcg_gen_concat32_i64(t1, t1, t0);
> +    tcg_gen_st_i64(t1, cpu_env, off);
> +    tcg_temp_free_i64(t1);
> +    tcg_temp_free_i64(t0);

simpler to just store a 32-bit value (st32_tl) to the appropriate half,
depending on host endianness? (i.e. +4 if little endian)

> +}
> +
> +static inline void gen_mfhc0_entrylo(TCGv arg, target_ulong off)
> +{
> +    TCGv_i64 t0 = tcg_temp_new_i64();
> +
> +    tcg_gen_ld_i64(t0, cpu_env, off);
> +#if defined(TARGET_MIPS64)
> +    tcg_gen_shri_i64(t0, t0, 30);

need to mask off the xi/ri bits?

> +#else
> +    tcg_gen_shri_i64(t0, t0, 32);

Again, I'm not convinced MIPS32 needs special handling here.

> +#endif
> +    tcg_gen_trunc_i64_tl(arg, t0);
> +    tcg_temp_free_i64(t0);
> +}
> +
> +static inline void gen_mfhc0_load64(TCGv arg, target_ulong off)
> +{
> +    TCGv_i64 t0 = tcg_temp_new_i64();
> +
> +    tcg_gen_ld_i64(t0, cpu_env, off);
> +    tcg_gen_shri_i64(t0, t0, 32);

simpler to just load a signed 32-bit value (ld32s_tl) from the
appropriate half, depending on host endianness? (i.e. +4 if little endian)

> +    tcg_gen_trunc_i64_tl(arg, t0);
> +    tcg_temp_free_i64(t0);
> +}
> +
>  static inline void gen_mfc0_load32 (TCGv arg, target_ulong off)
>  {
>      TCGv_i32 t0 = tcg_temp_new_i32();
> @@ -4850,6 +4915,134 @@ static inline void gen_mtc0_store64 (TCGv arg, target_ulong off)
>      tcg_gen_st_tl(arg, cpu_env, off);
>  }
>  
> +static void gen_mfhc0(DisasContext *ctx, TCGv arg, int reg, int sel)
> +{
> +    const char *rn = "invalid";
> +
> +    if (!(ctx->hflags & MIPS_HFLAG_ELPA)) {

worth reusing the CP0_CHECK stuff?

> +        goto mfhc0_read_zero;
> +    }
> +
> +    switch (reg) {
> +    case 2:
> +        switch (sel) {
> +        case 0:
> +            gen_mfhc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo0));
> +            rn = "EntryLo0";
> +            break;
> +        default:
> +            goto mfhc0_read_zero;
> +        }
> +        break;
> +    case 3:
> +        switch (sel) {
> +        case 0:
> +            gen_mfhc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo1));
> +            rn = "EntryLo1";
> +            break;
> +        default:
> +            goto mfhc0_read_zero;
> +        }
> +        break;
> +    case 17:
> +        switch (sel) {
> +        case 0:
> +            gen_mfhc0_load64(arg, offsetof(CPUMIPSState, lladdr));
> +            rn = "LLAddr";
> +            break;
> +        default:
> +            goto mfhc0_read_zero;
> +        }
> +        break;
> +    case 28:
> +        switch (sel) {
> +        case 0:
> +        case 2:
> +        case 4:
> +        case 6:
> +            gen_mfhc0_load64(arg, offsetof(CPUMIPSState, CP0_TagLo));
> +            rn = "TagLo";
> +            break;
> +        default:
> +            goto mfhc0_read_zero;
> +        }
> +        break;
> +    default:
> +        goto mfhc0_read_zero;
> +    }
> +
> +    (void)rn; /* avoid a compiler warning */
> +    LOG_DISAS("mfhc0 %s (reg %d sel %d)\n", rn, reg, sel);
> +    return;
> +
> +mfhc0_read_zero:
> +    LOG_DISAS("mfhc0 %s (reg %d sel %d)\n", rn, reg, sel);
> +    tcg_gen_movi_tl(arg, 0);
> +}
> +
> +static void gen_mthc0(DisasContext *ctx, TCGv arg, int reg, int sel)
> +{
> +    const char *rn = "invalid";
> +
> +    if (!(ctx->hflags & MIPS_HFLAG_ELPA)) {
> +        goto mthc0_nop;
> +    }
> +
> +    tcg_gen_andi_tl(arg, arg, ctx->PAMask >> 36);

Shouldn't this depend on the register? LLAddr shift varies per core
(CP0_LLAddr_shift), but EntryLo shift will be the same for each core.

Cheers
James

> +
> +    switch (reg) {
> +    case 2:
> +        switch (sel) {
> +        case 0:
> +            gen_mthc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo0));
> +            rn = "EntryLo0";
> +            break;
> +        default:
> +            goto mthc0_nop;
> +        }
> +        break;
> +    case 3:
> +        switch (sel) {
> +        case 0:
> +            gen_mthc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo1));
> +            rn = "EntryLo1";
> +            break;
> +        default:
> +            goto mthc0_nop;
> +        }
> +        break;
> +    case 17:
> +        switch (sel) {
> +        case 0:
> +            gen_mthc0_store64(arg, offsetof(CPUMIPSState, lladdr));
> +            rn = "LLAddr";
> +            break;
> +        default:
> +            goto mthc0_nop;
> +        }
> +        break;
> +    case 28:
> +        switch (sel) {
> +        case 0:
> +        case 2:
> +        case 4:
> +        case 6:
> +            gen_mthc0_store64(arg, offsetof(CPUMIPSState, CP0_TagLo));
> +            rn = "TagLo";
> +            break;
> +        default:
> +            goto mthc0_nop;
> +        }
> +        break;
> +    default:
> +        goto mthc0_nop;
> +    }
> +
> +    (void)rn; /* avoid a compiler warning */
> +mthc0_nop:
> +    LOG_DISAS("mthc0 %s (reg %d sel %d)\n", rn, reg, sel);
> +}
> +
>  static inline void gen_mfc0_unimplemented(DisasContext *ctx, TCGv arg)
>  {
>      if (ctx->insn_flags & ISA_MIPS32R6) {
> @@ -7847,6 +8040,25 @@ static void gen_cp0 (CPUMIPSState *env, DisasContext *ctx, uint32_t opc, int rt,
>          opn = "dmtc0";
>          break;
>  #endif
> +    case OPC_MFHC0:
> +        check_mvh(ctx);
> +        if (rt == 0) {
> +            /* Treat as NOP. */
> +            return;
> +        }
> +        gen_mfhc0(ctx, cpu_gpr[rt], rd, ctx->opcode & 0x7);
> +        opn = "mfhc0";
> +        break;
> +    case OPC_MTHC0:
> +        check_mvh(ctx);
> +        {
> +            TCGv t0 = tcg_temp_new();
> +            gen_load_gpr(t0, rt);
> +            gen_mthc0(ctx, t0, rd, ctx->opcode & 0x7);
> +            tcg_temp_free(t0);
> +        }
> +        opn = "mthc0";
> +        break;
>      case OPC_MFTR:
>          check_insn(ctx, ASE_MT);
>          if (rd == 0) {
> @@ -18571,6 +18783,8 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
>          case OPC_MTC0:
>          case OPC_MFTR:
>          case OPC_MTTR:
> +        case OPC_MFHC0:
> +        case OPC_MTHC0:
>  #if defined(TARGET_MIPS64)
>          case OPC_DMFC0:
>          case OPC_DMTC0:
> @@ -19141,6 +19355,8 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>      ctx.ie = (env->CP0_Config4 >> CP0C4_IE) & 3;
>      ctx.bi = (env->CP0_Config3 >> CP0C3_BI) & 1;
>      ctx.bp = (env->CP0_Config3 >> CP0C3_BP) & 1;
> +    ctx.mvh = (env->CP0_Config5 >> CP0C5_MVH) & 1;
> +    ctx.PAMask = env->PAMask;
>      /* Restore delay slot state from the tb context.  */
>      ctx.hflags = (uint32_t)tb->flags; /* FIXME: maybe use 64 bits here? */
>      ctx.ulri = (env->CP0_Config3 >> CP0C3_ULRI) & 1;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]