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

[Leon Alrae]

On 28/04/2015 16:52, James Hogan wrote:
>> 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).

According to QEMU Coding Style we never use tabs, so we cannot avoid
this inconsistency unless we modify all entries in the table.

> 
>>  {"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/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.

Will remove.

> 
>> +        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?

As already clarified in the other email EntryLo.PFNX in MIPS32 starts at 32.

> 
>> +#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)

Using the #ifdef HOST_WORDS_BIGENDIAN seems a bit ugly to me. I
personally prefer the current implementation.

> 
>> +}
>> +
>> +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?

Yeh, it was there before I did the last minute "improvements" (for some
reason I assumed that the trunc below would take care of it so masking
off was not required, but now I can see I was wrong). Thanks for
catching this.

> 
>> +#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?

Currently CP0_CHECK tests only for unimplemented registers which I think
doesn’t fit well here.

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

Yes, it should. I'll correct that.

Thanks,
Leon