Re: [PATCH v3 01/20] target/riscv: sync env->misa_ext* with cpu->cfg in realize()

[Alistair Francis]

On Thu, Mar 30, 2023 at 3:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> When riscv_cpu_realize() starts we're guaranteed to have cpu->cfg.ext_N
> properties updated. The same can't be said about env->misa_ext*, since
> the user might enable/disable MISA extensions in the command line, and
> env->misa_ext* won't caught these changes. The current solution is to
> sync everything at the end of validate_set_extensions(), checking every
> cpu->cfg.ext_N value to do a set_misa() in the end.
>
> The last change we're making in the MISA cfg flags are in the G
> extension logic, enabling IMAFG if cpu->cfg_ext.g is enabled. Otherwise
> we're not making any changes in MISA bits ever since realize() starts.
>
> There's no reason to postpone misa_ext updates until the end of the
> validation. Let's do it earlier, during realize(), in a new helper
> called riscv_cpu_sync_misa_cfg(). If cpu->cfg.ext_g is enabled, do it
> again by updating env->misa_ext* directly.
>
> This is a pre-requisite to allow riscv_cpu_validate_set_extensions() to
> use riscv_has_ext() instead of cpu->cfg.ext_N to validate the MISA
> extensions, which is our end goal here.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>  target/riscv/cpu.c | 94 +++++++++++++++++++++++++++-------------------
>  1 file changed, 56 insertions(+), 38 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1e97473af2..2711d80e16 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -804,12 +804,11 @@ static void riscv_cpu_disas_set_info(CPUState *s, 
> disassemble_info *info)
>
>  /*
>   * Check consistency between chosen extensions while setting
> - * cpu->cfg accordingly, doing a set_misa() in the end.
> + * cpu->cfg accordingly.
>   */
>  static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>  {
>      CPURISCVState *env = &cpu->env;
> -    uint32_t ext = 0;
>
>      /* Do some ISA extension error checking */
>      if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
> @@ -824,6 +823,9 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
> *cpu, Error **errp)
>          cpu->cfg.ext_d = true;
>          cpu->cfg.ext_icsr = true;
>          cpu->cfg.ext_ifencei = true;
> +
> +        env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> +        env->misa_ext_mask = env->misa_ext;
>      }
>
>      if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
> @@ -962,39 +964,8 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
> *cpu, Error **errp)
>          cpu->cfg.ext_zksh = true;
>      }
>
> -    if (cpu->cfg.ext_i) {
> -        ext |= RVI;
> -    }
> -    if (cpu->cfg.ext_e) {
> -        ext |= RVE;
> -    }
> -    if (cpu->cfg.ext_m) {
> -        ext |= RVM;
> -    }
> -    if (cpu->cfg.ext_a) {
> -        ext |= RVA;
> -    }
> -    if (cpu->cfg.ext_f) {
> -        ext |= RVF;
> -    }
> -    if (cpu->cfg.ext_d) {
> -        ext |= RVD;
> -    }
> -    if (cpu->cfg.ext_c) {
> -        ext |= RVC;
> -    }
> -    if (cpu->cfg.ext_s) {
> -        ext |= RVS;
> -    }
> -    if (cpu->cfg.ext_u) {
> -        ext |= RVU;
> -    }
> -    if (cpu->cfg.ext_h) {
> -        ext |= RVH;
> -    }
>      if (cpu->cfg.ext_v) {
>          int vext_version = VEXT_VERSION_1_00_0;
> -        ext |= RVV;
>          if (!is_power_of_2(cpu->cfg.vlen)) {
>              error_setg(errp,
>                         "Vector extension VLEN must be power of 2");
> @@ -1032,11 +1003,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
> *cpu, Error **errp)
>          }
>          set_vext_version(env, vext_version);
>      }
> -    if (cpu->cfg.ext_j) {
> -        ext |= RVJ;
> -    }
> -
> -    set_misa(env, env->misa_mxl, ext);
>  }
>
>  #ifndef CONFIG_USER_ONLY
> @@ -1121,6 +1087,50 @@ static void riscv_cpu_finalize_features(RISCVCPU *cpu, 
> Error **errp)
>  #endif
>  }
>
> +static void riscv_cpu_sync_misa_cfg(CPURISCVState *env)
> +{
> +    uint32_t ext = 0;
> +
> +    if (riscv_cpu_cfg(env)->ext_i) {

It's probably worth storing riscv_cpu_cfg(env) in a variable

But either way:

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

Alistair

> +        ext |= RVI;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_e) {
> +        ext |= RVE;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_m) {
> +        ext |= RVM;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_a) {
> +        ext |= RVA;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_f) {
> +        ext |= RVF;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_d) {
> +        ext |= RVD;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_c) {
> +        ext |= RVC;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_s) {
> +        ext |= RVS;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_u) {
> +        ext |= RVU;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_h) {
> +        ext |= RVH;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_v) {
> +        ext |= RVV;
> +    }
> +    if (riscv_cpu_cfg(env)->ext_j) {
> +        ext |= RVJ;
> +    }
> +
> +    env->misa_ext = env->misa_ext_mask = ext;
> +}
> +
>  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -1156,6 +1166,14 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> **errp)
>          set_priv_version(env, priv_version);
>      }
>
> +    /*
> +     * We can't be sure of whether we set defaults during cpu_init()
> +     * or whether the user enabled/disabled some bits via cpu->cfg
> +     * flags. Sync env->misa_ext with cpu->cfg now to allow us to
> +     * use just env->misa_ext later.
> +     */
> +    riscv_cpu_sync_misa_cfg(env);
> +
>      /* Force disable extensions if priv spec version does not match */
>      for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
>          if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
> --
> 2.39.2
>
>