Re: [PATCH v3 04/20] target/riscv: introduce riscv_cpu_add_misa_properties()

[Alistair Francis]

On Thu, Mar 30, 2023 at 3:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Ever since RISCVCPUConfig got introduced users are able to set CPU extensions
> in the command line. User settings are reflected in the cpu->cfg object
> for later use. These properties are used in the target/riscv/cpu.c code,
> most notably in riscv_cpu_validate_set_extensions(), where most of our
> realize time validations are made.
>
> And then there's env->misa_ext, the field where the MISA extensions are
> set, that is read everywhere else. We need to keep env->misa_ext updated
> with cpu->cfg settings, since our validations rely on it, forcing us to
> make register_cpu_props() write cpu->cfg.ext_N flags to cover for named
> CPUs that aren't used named properties but also needs to go through the
> same validation steps. Failing to so will make those name CPUs fail
> validation (see c66ffcd5358b for more info). Not only that, but we also
> need to sync env->misa_ext with cpu->cfg again during realize() time to
> catch any change the user might have done, since the rest of the code
> relies on that.
>
> Making cpu->cfg.ext_N and env->misa_ext reflect each other is not
> needed. What we want is a way for users to enable/disable MISA extensions,
> and there's nothing stopping us from letting the user write env->misa_ext
> directly. Here are the artifacts that will enable us to do that:
>
> - RISCVCPUMisaExtConfig will declare each MISA property;
>
> - cpu_set_misa_ext_cfg() is the setter for each property. We'll write
>   env->misa_ext and env->misa_ext_mask with the appropriate misa_bit;
>   cutting off cpu->cfg.ext_N from the logic;
>
> - cpu_get_misa_ext_cfg() is a getter that will retrieve the current val
>   of the property based on env->misa_ext;
>
> - riscv_cpu_add_misa_properties() will be called in register_cpu_props()
>   to init all MISA properties from the misa_ext_cfgs[] array.
>
> With this infrastructure we'll start to get rid of each cpu->cfg.ext_N
> attribute in the next patches.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

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

Alistair

> ---
>  target/riscv/cpu.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5ae9440aee..d7763ecfa9 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1393,6 +1393,69 @@ static void riscv_cpu_init(Object *obj)
>  #endif /* CONFIG_USER_ONLY */
>  }
>
> +typedef struct RISCVCPUMisaExtConfig {
> +    const char *name;
> +    const char *description;
> +    target_ulong misa_bit;
> +    bool enabled;
> +} RISCVCPUMisaExtConfig;
> +
> +static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
> +                                 void *opaque, Error **errp)
> +{
> +    const RISCVCPUMisaExtConfig *misa_ext_cfg = opaque;
> +    target_ulong misa_bit = misa_ext_cfg->misa_bit;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +    bool value;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (value) {
> +        env->misa_ext |= misa_bit;
> +        env->misa_ext_mask |= misa_bit;
> +    } else {
> +        env->misa_ext &= ~misa_bit;
> +        env->misa_ext_mask &= ~misa_bit;
> +    }
> +}
> +
> +static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
> +                                 void *opaque, Error **errp)
> +{
> +    const RISCVCPUMisaExtConfig *misa_ext_cfg = opaque;
> +    target_ulong misa_bit = misa_ext_cfg->misa_bit;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +    bool value;
> +
> +    value = env->misa_ext & misa_bit;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {};
> +
> +static void riscv_cpu_add_misa_properties(Object *cpu_obj)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
> +        const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i];
> +
> +        object_property_add(cpu_obj, misa_cfg->name, "bool",
> +                            cpu_get_misa_ext_cfg,
> +                            cpu_set_misa_ext_cfg,
> +                            NULL, (void *)misa_cfg);
> +        object_property_set_description(cpu_obj, misa_cfg->name,
> +                                        misa_cfg->description);
> +        object_property_set_bool(cpu_obj, misa_cfg->name,
> +                                 misa_cfg->enabled, NULL);
> +    }
> +}
> +
>  static Property riscv_cpu_extensions[] = {
>      /* Defaults for standard extensions */
>      DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true),
> @@ -1530,6 +1593,8 @@ static void register_cpu_props(Object *obj)
>          return;
>      }
>
> +    riscv_cpu_add_misa_properties(obj);
> +
>      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>          qdev_property_add_static(dev, prop);
>      }
> --
> 2.39.2
>
>