Re: [Qemu-devel] [PATCH] target-mips: add MAAR, MAARI register

[Leon Alrae]

On 24/02/16 14:20, Yongbok Kim wrote:
> The MAAR register is a read/write register included in Release 5
> of the architecture that defines the accessibility attributes of
> physical address regions. In particular, MAAR defines whether an
> instruction fetch or data load can speculatively access a memory
> region within the physical address bounds specified by MAAR.
> 
> As QEMU doesn't do speculative access, hence this patch only
> provides ability to access the registers.
> 
> Signed-off-by: Yongbok Kim <address@hidden>
> ---
>  target-mips/cpu.h       |    4 +++
>  target-mips/helper.h    |    6 +++++
>  target-mips/machine.c   |    3 ++
>  target-mips/op_helper.c |   56 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  target-mips/translate.c |   56 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 125 insertions(+), 0 deletions(-)

Is there any reason for not enabling this feature anywhere? I remember
that P5600 supports MAAR.

> 
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index bd23c2a..39e2129 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -162,6 +162,7 @@ typedef struct mips_def_t mips_def_t;
>  #define MIPS_FPU_MAX 1
>  #define MIPS_DSP_ACC 4
>  #define MIPS_KSCRATCH_NUM 6
> +#define MIPS_MAAR_MAX 16 /* Must be an even number. */
>  
>  typedef struct TCState TCState;
>  struct TCState {
> @@ -474,10 +475,13 @@ struct CPUMIPSState {
>  #define CP0C5_SBRI       6
>  #define CP0C5_MVH        5
>  #define CP0C5_LLB        4
> +#define CP0C5_MRP        3
>  #define CP0C5_UFR        2
>  #define CP0C5_NFExists   0
>      int32_t CP0_Config6;
>      int32_t CP0_Config7;
> +    uint64_t CP0_MAAR[MIPS_MAAR_MAX];
> +    int32_t CP0_MAARI;
>      /* XXX: Maybe make LLAddr per-TC? */
>      uint64_t lladdr;
>      target_ulong llval;
> diff --git a/target-mips/helper.h b/target-mips/helper.h
> index 95b9149..8bd39c2 100644
> --- a/target-mips/helper.h
> +++ b/target-mips/helper.h
> @@ -77,6 +77,8 @@ DEF_HELPER_1(mftc0_epc, tl, env)
>  DEF_HELPER_1(mftc0_ebase, tl, env)
>  DEF_HELPER_2(mftc0_configx, tl, env, tl)
>  DEF_HELPER_1(mfc0_lladdr, tl, env)
> +DEF_HELPER_1(mfc0_maar, tl, env)
> +DEF_HELPER_1(mfhc0_maar, tl, env)
>  DEF_HELPER_2(mfc0_watchlo, tl, env, i32)
>  DEF_HELPER_2(mfc0_watchhi, tl, env, i32)
>  DEF_HELPER_1(mfc0_debug, tl, env)
> @@ -88,6 +90,7 @@ DEF_HELPER_1(dmfc0_tccontext, tl, env)
>  DEF_HELPER_1(dmfc0_tcschedule, tl, env)
>  DEF_HELPER_1(dmfc0_tcschefback, tl, env)
>  DEF_HELPER_1(dmfc0_lladdr, tl, env)
> +DEF_HELPER_1(dmfc0_maar, tl, env)
>  DEF_HELPER_2(dmfc0_watchlo, tl, env, i32)
>  #endif /* TARGET_MIPS64 */
>  
> @@ -144,6 +147,9 @@ DEF_HELPER_2(mtc0_config3, void, env, tl)
>  DEF_HELPER_2(mtc0_config4, void, env, tl)
>  DEF_HELPER_2(mtc0_config5, void, env, tl)
>  DEF_HELPER_2(mtc0_lladdr, void, env, tl)
> +DEF_HELPER_2(mtc0_maar, void, env, tl)
> +DEF_HELPER_2(mthc0_maar, void, env, tl)
> +DEF_HELPER_2(mtc0_maari, void, env, tl)
>  DEF_HELPER_3(mtc0_watchlo, void, env, tl, i32)
>  DEF_HELPER_3(mtc0_watchhi, void, env, tl, i32)
>  DEF_HELPER_2(mtc0_xcontext, void, env, tl)
> diff --git a/target-mips/machine.c b/target-mips/machine.c
> index 737f3c2..667bde4 100644
> --- a/target-mips/machine.c
> +++ b/target-mips/machine.c
> @@ -272,6 +272,8 @@ const VMStateDescription vmstate_mips_cpu = {
>          VMSTATE_INT32(env.CP0_Config3, MIPSCPU),
>          VMSTATE_INT32(env.CP0_Config6, MIPSCPU),
>          VMSTATE_INT32(env.CP0_Config7, MIPSCPU),
> +        VMSTATE_UINT64_ARRAY(env.CP0_MAAR, MIPSCPU, MIPS_MAAR_MAX),
> +        VMSTATE_INT32(env.CP0_MAARI, MIPSCPU),

You need to bump version_id.

>          VMSTATE_UINT64(env.lladdr, MIPSCPU),
>          VMSTATE_UINTTL_ARRAY(env.CP0_WatchLo, MIPSCPU, 8),
>          VMSTATE_INT32_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
> @@ -297,3 +299,4 @@ const VMStateDescription vmstate_mips_cpu = {
>          VMSTATE_END_OF_LIST()
>      },
>  };
> +

New blank line at EOF.

> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 684ec92..13aac92 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -881,6 +881,22 @@ target_ulong helper_mfc0_lladdr(CPUMIPSState *env)
>      return (int32_t)(env->lladdr >> env->CP0_LLAddr_shift);
>  }
>  
> +target_ulong helper_mfc0_maar(CPUMIPSState *env)
> +{
> +    if (env->CP0_MAARI < MIPS_MAAR_MAX) {

I don't think this test is needed. This is already checked when guest
tries to change the CP0_MAARI value

> +        return (int32_t) env->CP0_MAAR[env->CP0_MAARI];
> +    }
> +    return 0;
> +}
> +
> +target_ulong helper_mfhc0_maar(CPUMIPSState *env)
> +{
> +    if (env->CP0_MAARI < MIPS_MAAR_MAX) {
> +        return env->CP0_MAAR[env->CP0_MAARI] >> 32;
> +    }
> +    return 0;
> +}
> +
>  target_ulong helper_mfc0_watchlo(CPUMIPSState *env, uint32_t sel)
>  {
>      return (int32_t)env->CP0_WatchLo[sel];
> @@ -947,6 +963,14 @@ target_ulong helper_dmfc0_lladdr(CPUMIPSState *env)
>      return env->lladdr >> env->CP0_LLAddr_shift;
>  }
>  
> +target_ulong helper_dmfc0_maar(CPUMIPSState *env)
> +{
> +    if (env->CP0_MAARI < MIPS_MAAR_MAX) {
> +        return env->CP0_MAAR[env->CP0_MAARI];
> +    }
> +    return 0;
> +}
> +
>  target_ulong helper_dmfc0_watchlo(CPUMIPSState *env, uint32_t sel)
>  {
>      return env->CP0_WatchLo[sel];
> @@ -1570,6 +1594,38 @@ void helper_mtc0_lladdr(CPUMIPSState *env, 
> target_ulong arg1)
>      env->lladdr = (env->lladdr & ~mask) | (arg1 & mask);
>  }
>  
> +void helper_mtc0_maar(CPUMIPSState *env, target_ulong arg1)
> +{
> +    uint64_t mask = ((env->PAMask >> 4) & ~0xFFFull) | 0x3;
> +
> +    if (env->CP0_MAARI < MIPS_MAAR_MAX) {
> +        env->CP0_MAAR[env->CP0_MAARI] = arg1 & mask;
> +    }
> +}
> +
> +void helper_mthc0_maar(CPUMIPSState *env, target_ulong arg1)
> +{
> +    if (env->CP0_MAARI < MIPS_MAAR_MAX) {
> +        env->CP0_MAAR[env->CP0_MAARI] = ((uint64_t) arg1 << 32) |
> +                (env->CP0_MAAR[env->CP0_MAARI] & 0x00000000ffffffffULL);
> +    }
> +}
> +
> +void helper_mtc0_maari(CPUMIPSState *env, target_ulong arg1)
> +{
> +    int index = arg1 & 0x3f;
> +    if (index == 0x3f) {
> +        /* Software may write all ones to INDEX to determine the
> +           maximum value supported. */
> +        env->CP0_MAARI = MIPS_MAAR_MAX - 1;
> +    } else if (index < MIPS_MAAR_MAX) {
> +        env->CP0_MAARI = index;
> +    }
> +    /* Other than the all ones, if the
> +       value written is not supported, then INDEX is unchanged
> +       from its previous value. */
> +}
> +
>  void helper_mtc0_watchlo(CPUMIPSState *env, target_ulong arg1, uint32_t sel)
>  {
>      /* Watch exceptions for instructions, data loads, data stores
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 658926d..70990d4 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1429,6 +1429,7 @@ typedef struct DisasContext {
>      bool mvh;
>      int CP0_LLAddr_shift;
>      bool ps;
> +    bool mrp;
>  } DisasContext;
>  
>  enum {
> @@ -4807,6 +4808,13 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>                               ctx->CP0_LLAddr_shift);
>              rn = "LLAddr";
>              break;
> +        case 1:
> +            if (!ctx->mrp) {
> +                goto mfhc0_read_zero;
> +            }

We should use CP0_CHECK just like in gen_mfc0 etc.

> +            gen_helper_mfhc0_maar(arg, cpu_env);
> +            rn = "MAAR";
> +            break;
>          default:
>              goto mfhc0_read_zero;
>          }
> @@ -4878,6 +4886,13 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>                 treating MTHC0 to LLAddr as NOP. */
>              rn = "LLAddr";
>              break;
> +        case 1:
> +            if (!ctx->mrp) {
> +                goto mthc0_nop;
> +            }

Same.

Thanks,
Leon

> +            gen_helper_mthc0_maar(cpu_env, arg);
> +            rn = "MAAR";
> +            break;
>          default:
>              goto mthc0_nop;
>          }
> @@ -5334,6 +5349,16 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>              gen_helper_mfc0_lladdr(arg, cpu_env);
>              rn = "LLAddr";
>              break;
> +        case 1:
> +            CP0_CHECK(ctx->mrp);
> +            gen_helper_mfc0_maar(arg, cpu_env);
> +            rn = "MAAR";
> +            break;
> +        case 2:
> +            CP0_CHECK(ctx->mrp);
> +            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_MAARI));
> +            rn = "MAARI";
> +            break;
>          default:
>              goto cp0_unimplemented;
>          }
> @@ -5963,6 +5988,16 @@ static void gen_mtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>              gen_helper_mtc0_lladdr(cpu_env, arg);
>              rn = "LLAddr";
>              break;
> +        case 1:
> +            CP0_CHECK(ctx->mrp);
> +            gen_helper_mtc0_maar(cpu_env, arg);
> +            rn = "MAAR";
> +            break;
> +        case 2:
> +            CP0_CHECK(ctx->mrp);
> +            gen_helper_mtc0_maari(cpu_env, arg);
> +            rn = "MAARI";
> +            break;
>          default:
>              goto cp0_unimplemented;
>          }
> @@ -6588,6 +6623,16 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>              gen_helper_dmfc0_lladdr(arg, cpu_env);
>              rn = "LLAddr";
>              break;
> +        case 1:
> +            CP0_CHECK(ctx->mrp);
> +            gen_helper_dmfc0_maar(arg, cpu_env);
> +            rn = "MAAR";
> +            break;
> +        case 2:
> +            CP0_CHECK(ctx->mrp);
> +            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_MAARI));
> +            rn = "MAARI";
> +            break;
>          default:
>              goto cp0_unimplemented;
>          }
> @@ -7209,6 +7254,16 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>              gen_helper_mtc0_lladdr(cpu_env, arg);
>              rn = "LLAddr";
>              break;
> +        case 1:
> +            CP0_CHECK(ctx->mrp);
> +            gen_helper_mtc0_maar(cpu_env, arg);
> +            rn = "MAAR";
> +            break;
> +        case 2:
> +            CP0_CHECK(ctx->mrp);
> +            gen_helper_mtc0_maari(cpu_env, arg);
> +            rn = "MAARI";
> +            break;
>          default:
>              goto cp0_unimplemented;
>          }
> @@ -19611,6 +19666,7 @@ void gen_intermediate_code(CPUMIPSState *env, struct 
> TranslationBlock *tb)
>      ctx.ulri = (env->CP0_Config3 >> CP0C3_ULRI) & 1;
>      ctx.ps = ((env->active_fpu.fcr0 >> FCR0_PS) & 1) ||
>               (env->insn_flags & (INSN_LOONGSON2E | INSN_LOONGSON2F));
> +    ctx.mrp = (env->CP0_Config5 >> CP0C5_MRP) & 1;
>      restore_cpu_state(env, &ctx);
>  #ifdef CONFIG_USER_ONLY
>          ctx.mem_idx = MIPS_HFLAG_UM;
>