Re: [PATCH v6 8/9] target/riscv/cpu.c: remove cfg setup from riscv_cpu_init()

[Alistair Francis]

On Thu, Mar 30, 2023 at 6:11 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We have 4 config settings being done in riscv_cpu_init(): ext_ifencei,
> ext_icsr, mmu and pmp. This is also the constructor of the "riscv-cpu"
> device, which happens to be the parent device of every RISC-V cpu.
>
> The result is that these 4 configs are being set every time, and every
> other CPU should always account for them. CPUs such as sifive_e need to
> disable settings that aren't enabled simply because the parent class
> happens to be enabling it.
>
> Moving all configurations from the parent class to each CPU will
> centralize the config of each CPU into its own init(), which is clearer
> than having to account to whatever happens to be set in the parent
> device. These settings are also being set in register_cpu_props() when
> no 'misa_ext' is set, so for these CPUs we don't need changes. Named
> CPUs will receive all cfgs that the parent were setting into their
> init().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

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

Alistair

> ---
>  target/riscv/cpu.c | 59 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 331272c8a0..4aa119b9bc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -325,7 +325,8 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
>
>  static void riscv_any_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
>  #if defined(TARGET_RISCV32)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
>  #elif defined(TARGET_RISCV64)
> @@ -339,6 +340,12 @@ static void riscv_any_cpu_init(Object *obj)
>  #endif
>
>      env->priv_ver = PRIV_VERSION_LATEST;
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>  }
>
>  #if defined(TARGET_RISCV64)
> @@ -357,12 +364,19 @@ static void rv64_base_cpu_init(Object *obj)
>
>  static void rv64_sifive_u_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>      env->priv_ver = PRIV_VERSION_1_10_0;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
>  #endif
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>  }
>
>  static void rv64_sifive_e_cpu_init(Object *obj)
> @@ -372,10 +386,14 @@ static void rv64_sifive_e_cpu_init(Object *obj)
>
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
>      env->priv_ver = PRIV_VERSION_1_10_0;
> -    cpu->cfg.mmu = false;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
>  #endif
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.pmp = true;
>  }
>
>  static void rv64_thead_c906_cpu_init(Object *obj)
> @@ -403,6 +421,9 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(cpu, VM_1_10_SV39);
>  #endif
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.pmp = true;
>  }
>
>  static void rv128_base_cpu_init(Object *obj)
> @@ -439,12 +460,19 @@ static void rv32_base_cpu_init(Object *obj)
>
>  static void rv32_sifive_u_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>      env->priv_ver = PRIV_VERSION_1_10_0;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
>  #endif
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>  }
>
>  static void rv32_sifive_e_cpu_init(Object *obj)
> @@ -454,10 +482,14 @@ static void rv32_sifive_e_cpu_init(Object *obj)
>
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
>      env->priv_ver = PRIV_VERSION_1_10_0;
> -    cpu->cfg.mmu = false;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
>  #endif
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.pmp = true;
>  }
>
>  static void rv32_ibex_cpu_init(Object *obj)
> @@ -467,11 +499,15 @@ static void rv32_ibex_cpu_init(Object *obj)
>
>      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
>      env->priv_ver = PRIV_VERSION_1_11_0;
> -    cpu->cfg.mmu = false;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
>  #endif
>      cpu->cfg.epmp = true;
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.pmp = true;
>  }
>
>  static void rv32_imafcu_nommu_cpu_init(Object *obj)
> @@ -481,10 +517,14 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
>      env->priv_ver = PRIV_VERSION_1_10_0;
> -    cpu->cfg.mmu = false;
>  #ifndef CONFIG_USER_ONLY
>      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
>  #endif
> +
> +    /* inherited from parent obj via riscv_cpu_init() */
> +    cpu->cfg.ext_ifencei = true;
> +    cpu->cfg.ext_icsr = true;
> +    cpu->cfg.pmp = true;
>  }
>  #endif
>
> @@ -1343,11 +1383,6 @@ static void riscv_cpu_init(Object *obj)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
>
> -    cpu->cfg.ext_ifencei = true;
> -    cpu->cfg.ext_icsr = true;
> -    cpu->cfg.mmu = true;
> -    cpu->cfg.pmp = true;
> -
>      cpu_set_cpustate_pointers(cpu);
>
>  #ifndef CONFIG_USER_ONLY
> --
> 2.39.2
>
>