Re: [PATCH v3 04/16] target/riscv: move 'mmu' to riscv_cpu_properties[]

[Alistair Francis <alistair23@gmail.com>]

On Thu, Jan 4, 2024 at 3:43 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Commit 7f0bdfb5bfc ("target/riscv/cpu.c: remove cfg setup from
> riscv_cpu_init()") already did some of the work by making some
> cpu_init() functions to explictly enable their own 'mmu' default.
>
> The generic CPUs didn't get update by that commit, so they are still
> relying on the defaults set by the 'mmu' option. But having 'mmu' and
> 'pmp' being default=true will force CPUs that doesn't implement these
> options to set them to 'false' in their cpu_init(), which isn't ideal.
>
> We'll move 'mmu' to riscv_cpu_properties[] without any defaults, i.e.
> the default will be 'false'. Compensate it by manually setting 'mmu =
> true' to the generic CPUs that requires it.
>
> Implement a setter for it to forbid the 'mmu' setting to be changed for
> vendor CPUs. This will allow the option to exist for all CPUs and, at
> the same time, protect vendor CPUs from undesired changes:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu sifive-e51,mmu=true
> qemu-system-riscv64: can't apply global sifive-e51-riscv-cpu.mmu=true:
>    CPU 'sifive-e51' does not allow changing the value of 'mmu'
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 55 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e90b70c0a7..9f1407b73f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -419,6 +419,8 @@ static void riscv_max_cpu_init(Object *obj)
>      CPURISCVState *env = &cpu->env;
>      RISCVMXL mlx = MXL_RV64;
>
> +    cpu->cfg.mmu = true;
> +
>  #ifdef TARGET_RISCV32
>      mlx = MXL_RV32;
>  #endif
> @@ -433,7 +435,11 @@ static void riscv_max_cpu_init(Object *obj)
>  #if defined(TARGET_RISCV64)
>  static void rv64_base_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +
> +    cpu->cfg.mmu = true;
> +
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV64, 0);
>      /* Set latest version of privileged specification */
> @@ -551,13 +557,18 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
>
>  static void rv128_base_cpu_init(Object *obj)
>  {
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +
>      if (qemu_tcg_mttcg_enabled()) {
>          /* Missing 128-bit aligned atomics */
>          error_report("128-bit RISC-V currently does not work with Multi "
>                       "Threaded TCG. Please use: -accel tcg,thread=single");
>          exit(EXIT_FAILURE);
>      }
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +
> +    cpu->cfg.mmu = true;
> +
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV128, 0);
>      /* Set latest version of privileged specification */
> @@ -569,7 +580,11 @@ static void rv128_base_cpu_init(Object *obj)
>  #else
>  static void rv32_base_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +
> +    cpu->cfg.mmu = true;
> +
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV32, 0);
>      /* Set latest version of privileged specification */
> @@ -1550,8 +1565,38 @@ static const PropertyInfo prop_pmu_mask = {
>      .set = prop_pmu_mask_set,
>  };
>
> +static void prop_mmu_set(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    bool value;
> +
> +    visit_type_bool(v, name, &value, errp);
> +
> +    if (cpu->cfg.mmu != value && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, "mmu", errp);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.mmu = value;
> +}
> +
> +static void prop_mmu_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    bool value = RISCV_CPU(obj)->cfg.mmu;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_mmu = {
> +    .name = "mmu",
> +    .get = prop_mmu_get,
> +    .set = prop_mmu_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -    DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
>      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
>
>      DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
> @@ -1572,6 +1617,8 @@ static Property riscv_cpu_properties[] = {
>      {.name = "pmu-mask", .info = &prop_pmu_mask},
>      {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
>
> +    {.name = "mmu", .info = &prop_mmu},
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> --
> 2.43.0
>
>