Re: [PATCH v6 05/22] target/riscv: Create xl field in env

[Alistair Francis]

On Wed, Jan 19, 2022 at 1:36 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
>
>
> On 2022/1/19 上午11:24, Alistair Francis wrote:
> > On Thu, Jan 13, 2022 at 9:50 PM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> > I'm not clear on why this is better?
>
> Current xlen has been used in helper functions and many other places.
> The computation of current xlen  is not so trivial, so that we should
> recompute it as little as possible.
> Fortunately, xlen only changes in very seldom cases, such as exception,
> misa write,  mstatus write, cpu reset, migration load.
> So that we can only recompute XLEN in this places and cache it into
> CPURISCVState.

Sounds good! Do you mind adding that to the commit message

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> Thanks
> Zhiwei
>
> >
> > Alistair
> >
> >> ---
> >>   target/riscv/cpu.c        |  1 +
> >>   target/riscv/cpu.h        | 31 +++++++++++++++++++++++++++++++
> >>   target/riscv/cpu_helper.c | 34 ++--------------------------------
> >>   target/riscv/csr.c        |  2 ++
> >>   target/riscv/machine.c    | 10 ++++++++++
> >>   5 files changed, 46 insertions(+), 32 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 9bc25d3055..54c1cf8ec5 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -399,6 +399,7 @@ static void riscv_cpu_reset(DeviceState *dev)
> >>       /* mmte is supposed to have pm.current hardwired to 1 */
> >>       env->mmte |= (PM_EXT_INITIAL | MMTE_M_PM_CURRENT);
> >>   #endif
> >> +    env->xl = riscv_cpu_mxl(env);
> >>       cs->exception_index = RISCV_EXCP_NONE;
> >>       env->load_res = -1;
> >>       set_default_nan_mode(1, &env->fp_status);
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 4d63086765..65fd849bef 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -145,6 +145,7 @@ struct CPURISCVState {
> >>       uint32_t misa_mxl_max;  /* max mxl for this cpu */
> >>       uint32_t misa_ext;      /* current extensions */
> >>       uint32_t misa_ext_mask; /* max ext for this cpu */
> >> +    uint32_t xl;            /* current xlen */
> >>
> >>       /* 128-bit helpers upper part return value */
> >>       target_ulong retxh;
> >> @@ -443,6 +444,36 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState 
> >> *env)
> >>   }
> >>   #endif
> >>
> >> +#if defined(TARGET_RISCV32)
> >> +#define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
> >> +#else
> >> +static inline RISCVMXL cpu_recompute_xl(CPURISCVState *env)
> >> +{
> >> +    RISCVMXL xl = env->misa_mxl;
> >> +#if !defined(CONFIG_USER_ONLY)
> >> +    /*
> >> +     * When emulating a 32-bit-only cpu, use RV32.
> >> +     * When emulating a 64-bit cpu, and MXL has been reduced to RV32,
> >> +     * MSTATUSH doesn't have UXL/SXL, therefore XLEN cannot be widened
> >> +     * back to RV64 for lower privs.
> >> +     */
> >> +    if (xl != MXL_RV32) {
> >> +        switch (env->priv) {
> >> +        case PRV_M:
> >> +            break;
> >> +        case PRV_U:
> >> +            xl = get_field(env->mstatus, MSTATUS64_UXL);
> >> +            break;
> >> +        default: /* PRV_S | PRV_H */
> >> +            xl = get_field(env->mstatus, MSTATUS64_SXL);
> >> +            break;
> >> +        }
> >> +    }
> >> +#endif
> >> +    return xl;
> >> +}
> >> +#endif
> >> +
> >>   /*
> >>    * Encode LMUL to lmul as follows:
> >>    *     LMUL    vlmul    lmul
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index 434a83e66a..32ea066ef0 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -35,37 +35,6 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> >>   #endif
> >>   }
> >>
> >> -static RISCVMXL cpu_get_xl(CPURISCVState *env)
> >> -{
> >> -#if defined(TARGET_RISCV32)
> >> -    return MXL_RV32;
> >> -#elif defined(CONFIG_USER_ONLY)
> >> -    return MXL_RV64;
> >> -#else
> >> -    RISCVMXL xl = riscv_cpu_mxl(env);
> >> -
> >> -    /*
> >> -     * When emulating a 32-bit-only cpu, use RV32.
> >> -     * When emulating a 64-bit cpu, and MXL has been reduced to RV32,
> >> -     * MSTATUSH doesn't have UXL/SXL, therefore XLEN cannot be widened
> >> -     * back to RV64 for lower privs.
> >> -     */
> >> -    if (xl != MXL_RV32) {
> >> -        switch (env->priv) {
> >> -        case PRV_M:
> >> -            break;
> >> -        case PRV_U:
> >> -            xl = get_field(env->mstatus, MSTATUS64_UXL);
> >> -            break;
> >> -        default: /* PRV_S | PRV_H */
> >> -            xl = get_field(env->mstatus, MSTATUS64_SXL);
> >> -            break;
> >> -        }
> >> -    }
> >> -    return xl;
> >> -#endif
> >> -}
> >> -
> >>   void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> >>                             target_ulong *cs_base, uint32_t *pflags)
> >>   {
> >> @@ -145,7 +114,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, 
> >> target_ulong *pc,
> >>       }
> >>   #endif
> >>
> >> -    flags = FIELD_DP32(flags, TB_FLAGS, XL, cpu_get_xl(env));
> >> +    flags = FIELD_DP32(flags, TB_FLAGS, XL, env->xl);
> >>
> >>       *pflags = flags;
> >>   }
> >> @@ -361,6 +330,7 @@ void riscv_cpu_set_mode(CPURISCVState *env, 
> >> target_ulong newpriv)
> >>       }
> >>       /* tlb_flush is unnecessary as mode is contained in mmu_idx */
> >>       env->priv = newpriv;
> >> +    env->xl = cpu_recompute_xl(env);
> >>
> >>       /*
> >>        * Clear the load reservation - otherwise a reservation placed in one
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index e7578f3e0f..b282a642f5 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -585,6 +585,7 @@ static RISCVException write_mstatus(CPURISCVState 
> >> *env, int csrno,
> >>           mstatus = set_field(mstatus, MSTATUS64_UXL, xl);
> >>       }
> >>       env->mstatus = mstatus;
> >> +    env->xl = cpu_recompute_xl(env);
> >>
> >>       return RISCV_EXCP_NONE;
> >>   }
> >> @@ -700,6 +701,7 @@ static RISCVException write_misa(CPURISCVState *env, 
> >> int csrno,
> >>       /* flush translation cache */
> >>       tb_flush(env_cpu(env));
> >>       env->misa_ext = val;
> >> +    env->xl = riscv_cpu_mxl(env);
> >>       return RISCV_EXCP_NONE;
> >>   }
> >>
> >> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> >> index 13b9ab375b..e1d1029e88 100644
> >> --- a/target/riscv/machine.c
> >> +++ b/target/riscv/machine.c
> >> @@ -185,10 +185,20 @@ static const VMStateDescription vmstate_rv128 = {
> >>       }
> >>   };
> >>
> >> +static int riscv_cpu_post_load(void *opaque, int version_id)
> >> +{
> >> +    RISCVCPU *cpu = opaque;
> >> +    CPURISCVState *env = &cpu->env;
> >> +
> >> +    env->xl = cpu_recompute_xl(env);
> >> +    return 0;
> >> +}
> >> +
> >>   const VMStateDescription vmstate_riscv_cpu = {
> >>       .name = "cpu",
> >>       .version_id = 3,
> >>       .minimum_version_id = 3,
> >> +    .post_load = riscv_cpu_post_load,
> >>       .fields = (VMStateField[]) {
> >>           VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> >>           VMSTATE_UINT64_ARRAY(env.fpr, RISCVCPU, 32),
> >> --
> >> 2.25.1
> >>
> >>