Re: [Qemu-devel] [PATCH v2 5/9] target-mips: update PageGrain and m{t, f}c0 EntryLo{0, 1}

[Yongbok Kim <yongbok.kim@imgtec.com>]

dm{t,f}c0_entrylo{0,1} is also added.
Please reflect it in the title.

I have other minor comments below.

Reviewed-by: Yongbok Kim <yongbok.kim@imgtec.com>

Regards,
Yongbok

On 08/07/2014 08:57, Leon Alrae wrote:
> PageGrain needs rw bitmask which differs between MIPS architectures.
> In pre-R6 if RIXI is supported, PageGrain.XIE and PageGrain.RIE are writeable,
> whereas in R6 they are read-only 1.
>
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> ---
>   target-mips/cpu.h            |    4 ++++
>   target-mips/helper.h         |    5 +++++
>   target-mips/op_helper.c      |   25 ++++++++++++++++++++++---
>   target-mips/translate.c      |   26 ++++++++++++++++++++++++--
>   target-mips/translate_init.c |    2 ++
>   5 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 5afafd7..8ccb3bb 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -243,7 +243,10 @@ struct CPUMIPSState {
>       target_ulong CP0_Context;
>       target_ulong CP0_KScratch[MIPS_KSCRATCH_NUM];
>       int32_t CP0_PageMask;
> +    int32_t CP0_PageGrain_rw_bitmask;
>       int32_t CP0_PageGrain;
> +#define CP0PG_RIE 31
> +#define CP0PG_XIE 30
>       int32_t CP0_Wired;
>       int32_t CP0_SRSConf0_rw_bitmask;
>       int32_t CP0_SRSConf0;
> @@ -377,6 +380,7 @@ struct CPUMIPSState {
>   #define CP0C3_M    31
>   #define CP0C3_ISA_ON_EXC 16
>   #define CP0C3_ULRI 13
> +#define CP0C3_RXI  12
>   #define CP0C3_DSPP 10
>   #define CP0C3_LPA  7
>   #define CP0C3_VEIC 6
> diff --git a/target-mips/helper.h b/target-mips/helper.h
> index a127db5..e7e0c8c 100644
> --- a/target-mips/helper.h
> +++ b/target-mips/helper.h
> @@ -152,6 +152,11 @@ DEF_HELPER_2(mtc0_datalo, void, env, tl)
>   DEF_HELPER_2(mtc0_taghi, void, env, tl)
>   DEF_HELPER_2(mtc0_datahi, void, env, tl)
>   
> +#if defined(TARGET_MIPS64)
> +DEF_HELPER_2(dmtc0_entrylo0, void, env, i64)
> +DEF_HELPER_2(dmtc0_entrylo1, void, env, i64)
> +#endif
> +
>   /* MIPS MT functions */
>   DEF_HELPER_2(mftgpr, tl, env, i32)
>   DEF_HELPER_2(mftlo, tl, env, i32)
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 3f39305..3579bde 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -1099,9 +1099,18 @@ void helper_mtc0_entrylo0(CPUMIPSState *env, target_ulong arg1)
>   {
>       /* Large physaddr (PABITS) not implemented */
>       /* 1k pages not implemented */
> -    env->CP0_EntryLo0 = arg1 & 0x3FFFFFFF;
> +    target_ulong rxi = arg1 & (env->CP0_PageGrain & (3u << CP0PG_XIE));
> +    env->CP0_EntryLo0 = (arg1 & 0x3FFFFFFF) | (rxi << (CP0EnLo_RI - 31));
rxi << (CP0EnLo_XI - 30) would be natural as CP0PG_XIE is used just above.
Anyway as the result is same, you can ignore.

>   }
>   
> +#if defined(TARGET_MIPS64)
> +void helper_dmtc0_entrylo0(CPUMIPSState *env, uint64_t arg1)
> +{

It might be better to have the comments to indicate the mask is 
calculated based on these conditions just like mtc0_entrylo{0,1}.
However you can ignore this issue.

      /* Large physaddr (PABITS) not implemented */
      /* 1k pages not implemented */

> +    uint64_t rxi = arg1 & ((env->CP0_PageGrain & (3ull << CP0PG_XIE)) << 32);
> +    env->CP0_EntryLo0 = (arg1 & 0x3FFFFFFF) | rxi;
> +}
> +#endif
> +
>   void helper_mtc0_tcstatus(CPUMIPSState *env, target_ulong arg1)
>   {
>       uint32_t mask = env->CP0_TCStatus_rw_bitmask;
> @@ -1266,9 +1275,18 @@ void helper_mtc0_entrylo1(CPUMIPSState *env, target_ulong arg1)
>   {
>       /* Large physaddr (PABITS) not implemented */
>       /* 1k pages not implemented */
> -    env->CP0_EntryLo1 = arg1 & 0x3FFFFFFF;
> +    target_ulong rxi = arg1 & (env->CP0_PageGrain & (3u << CP0PG_XIE));
> +    env->CP0_EntryLo1 = (arg1 & 0x3FFFFFFF) | (rxi << (CP0EnLo_RI - 31));

Same here.
rxi << (CP0EnLo_XI - 30)

>   }
>   
> +#if defined(TARGET_MIPS64)
> +void helper_dmtc0_entrylo1(CPUMIPSState *env, uint64_t arg1)
> +{

same here. it might need the comments.

> +    uint64_t rxi = arg1 & ((env->CP0_PageGrain & (3ull << CP0PG_XIE)) << 32);
> +    env->CP0_EntryLo1 = (arg1 & 0x3FFFFFFF) | rxi;
> +}
> +#endif
> +
>   void helper_mtc0_context(CPUMIPSState *env, target_ulong arg1)
>   {
>       env->CP0_Context = (env->CP0_Context & 0x007FFFFF) | (arg1 & ~0x007FFFFF);
> @@ -1285,7 +1303,8 @@ void helper_mtc0_pagegrain(CPUMIPSState *env, target_ulong arg1)
>       /* SmartMIPS not implemented */
>       /* Large physaddr (PABITS) not implemented */
>       /* 1k pages not implemented */

This comment is not relevant here as the function is not restricting 
because of the issue.
However that information is still valid, you can leave the comment as it 
is.

> -    env->CP0_PageGrain = 0;
> +    env->CP0_PageGrain = (arg1 & env->CP0_PageGrain_rw_bitmask) |
> +                         (env->CP0_PageGrain & ~env->CP0_PageGrain_rw_bitmask);
>   }
>   
>   void helper_mtc0_wired(CPUMIPSState *env, target_ulong arg1)
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index ee18bf3..a8c8318 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1176,6 +1176,7 @@ typedef struct DisasContext {
>       target_ulong btarget;
>       bool ulri;
>       int kscrexist;
> +    bool rxi;
>   } DisasContext;
>   
>   enum {
> @@ -4703,6 +4704,15 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel)
>           switch (sel) {
>           case 0:
>               tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_EntryLo0));
> +#if defined(TARGET_MIPS64)
> +            if (ctx->rxi) {
> +                TCGv tmp = tcg_temp_new();
> +                tcg_gen_andi_tl(tmp, arg, (3ull << 62));
> +                tcg_gen_shri_tl(tmp, tmp, 32);
> +                tcg_gen_or_tl(arg, arg, tmp);
> +                tcg_temp_free(tmp);
> +            }
> +#endif
>               tcg_gen_ext32s_tl(arg, arg);
>               rn = "EntryLo0";
>               break;
> @@ -4749,6 +4759,15 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int reg, int sel)
>           switch (sel) {
>           case 0:
>               tcg_gen_ld_tl(arg, cpu_env, offsetof(CPUMIPSState, CP0_EntryLo1));
> +#if defined(TARGET_MIPS64)
> +            if (ctx->rxi) {
> +                TCGv tmp = tcg_temp_new();
> +                tcg_gen_andi_tl(tmp, arg, (3ull << 62));
> +                tcg_gen_shri_tl(tmp, tmp, 32);
> +                tcg_gen_or_tl(arg, arg, tmp);
> +                tcg_temp_free(tmp);
> +            }
> +#endif
>               tcg_gen_ext32s_tl(arg, arg);
>               rn = "EntryLo1";
>               break;
> @@ -6524,7 +6543,7 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int reg, int sel)
>       case 2:
>           switch (sel) {
>           case 0:
> -            gen_helper_mtc0_entrylo0(cpu_env, arg);
> +            gen_helper_dmtc0_entrylo0(cpu_env, arg);
>               rn = "EntryLo0";
>               break;
>           case 1:
> @@ -6569,7 +6588,7 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int reg, int sel)
>       case 3:
>           switch (sel) {
>           case 0:
> -            gen_helper_mtc0_entrylo1(cpu_env, arg);
> +            gen_helper_dmtc0_entrylo1(cpu_env, arg);
>               rn = "EntryLo1";
>               break;
>           default:
> @@ -17534,6 +17553,7 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
>       ctx.tb = tb;
>       ctx.bstate = BS_NONE;
>       ctx.kscrexist = (env->CP0_Config4 >> CP0C4_KScrExist) & 0xff;
> +    ctx.rxi = (env->CP0_Config3 >> CP0C3_RXI) & 1;
>       /* 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 & (1 << CP0C3_ULRI);
> @@ -17913,6 +17933,8 @@ void cpu_state_reset(CPUMIPSState *env)
>       env->CP0_SRSConf3 = env->cpu_model->CP0_SRSConf3;
>       env->CP0_SRSConf4_rw_bitmask = env->cpu_model->CP0_SRSConf4_rw_bitmask;
>       env->CP0_SRSConf4 = env->cpu_model->CP0_SRSConf4;
> +    env->CP0_PageGrain_rw_bitmask = env->cpu_model->CP0_PageGrain_rw_bitmask;
> +    env->CP0_PageGrain = env->cpu_model->CP0_PageGrain;
>       env->active_fpu.fcr0 = env->cpu_model->CP1_fcr0;
>       env->insn_flags = env->cpu_model->insn_flags;
>   
> diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
> index 67b7837..779afff 100644
> --- a/target-mips/translate_init.c
> +++ b/target-mips/translate_init.c
> @@ -96,6 +96,8 @@ struct mips_def_t {
>       int32_t CP0_SRSConf3;
>       int32_t CP0_SRSConf4_rw_bitmask;
>       int32_t CP0_SRSConf4;
> +    int32_t CP0_PageGrain_rw_bitmask;
> +    int32_t CP0_PageGrain;
>       int insn_flags;
>       enum mips_mmu_types mmu_type;
>   };