Re: [PATCH for-8.1 v4 12/25] target/riscv/cpu.c: redesign register_cpu_props()

[LIU Zhiwei <zhiwei_liu@linux.alibaba.com>]

Hi Daniel,

I want to share my opinions about the cpu->cfg and misa.


Two suggestions:

1) The cpu->cfg should be set only once in cpu initialization 
phrase(cpu_init_fn or cpu_realize_fn), and never changes any more in 
other times(for example write_misa).

2) Set the misa only when cpu->cfg is ready.


In my mind, we should setting the misa and cfg in this way.

1) setting cfg  and misa_mxl in xxx_cpu_init.  Don't call set_misa here.

2) register and setting cfg for general cpus by the infrastructure.

3) check the cfg in cpu_realize_fn stage in a special function. Don't 
change cpu->cfg, just pass it as a parameter.

4)  expand the cpu->cfg, such as for RVG.

5)  setting the misa and misa_max

6) when write_misa, construct a cfg for the new misa value. If the cfg 
is legal after checking it against with the cpu->cfg, write it directly 
into misa. Don't change the cpu->cfg here.


Best Regards,
Zhiwei

On 2023/3/23 6:19, Daniel Henrique Barboza wrote:
> Now that the function is a no-op if 'env.misa_ext != 0', and no one that
> are setting misa_ext != 0 is calling it because set_misa() is setting
> the cpu cfg accordingly, remove the now deprecated code and rename the
> function to register_generic_cpu_props().
>
> This function is now doing exactly what the name says: it is creating
> user-facing properties to allow changes in the CPU cfg via the QEMU
> command line, setting default values if no user input is provided.
>
> Note that there's the possibility of a CPU to set a certain misa value
> and, at the same, also want user-facing flags and defaults from this
> function. This is not the case since commit 26b2bc58599c ("target/riscv:
> Don't expose the CPU properties on names CPUs"), but given that this is
> also a possibility, clarify in the function that using this function
> will overwrite existing values in cpu->cfg.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 48 ++++++++++------------------------------------
>   1 file changed, 10 insertions(+), 38 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index df5c0bda70..0e56a1c01f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -221,7 +221,7 @@ static const char * const riscv_intr_names[] = {
>       "reserved"
>   };
>   
> -static void register_cpu_props(Object *obj);
> +static void register_generic_cpu_props(Object *obj);
>   
>   const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
>   {
> @@ -386,7 +386,7 @@ static void rv64_base_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       /* We set this in the realise function */
>       set_misa(env, MXL_RV64, 0);
> -    register_cpu_props(obj);
> +    register_generic_cpu_props(obj);
>       /* Set latest version of privileged specification */
>       env->priv_ver = PRIV_VERSION_LATEST;
>   #ifndef CONFIG_USER_ONLY
> @@ -472,7 +472,7 @@ static void rv128_base_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       /* We set this in the realise function */
>       set_misa(env, MXL_RV128, 0);
> -    register_cpu_props(obj);
> +    register_generic_cpu_props(obj);
>       /* Set latest version of privileged specification */
>       env->priv_ver = PRIV_VERSION_LATEST;
>   #ifndef CONFIG_USER_ONLY
> @@ -485,7 +485,7 @@ static void rv32_base_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       /* We set this in the realise function */
>       set_misa(env, MXL_RV32, 0);
> -    register_cpu_props(obj);
> +    register_generic_cpu_props(obj);
>       /* Set latest version of privileged specification */
>       env->priv_ver = PRIV_VERSION_LATEST;
>   #ifndef CONFIG_USER_ONLY
> @@ -572,7 +572,7 @@ static void riscv_host_cpu_init(Object *obj)
>   #elif defined(TARGET_RISCV64)
>       set_misa(env, MXL_RV64, 0);
>   #endif
> -    register_cpu_props(obj);
> +    register_generic_cpu_props(obj);
>   }
>   #endif
>   
> @@ -1554,44 +1554,16 @@ static Property riscv_cpu_extensions[] = {
>   };
>   
>   /*
> - * Register CPU props based on env.misa_ext. If a non-zero
> - * value was set, register only the required cpu->cfg.ext_*
> - * properties and leave. env.misa_ext = 0 means that we want
> - * all the default properties to be registered.
> + * Register generic CPU props with user-facing flags declared
> + * in riscv_cpu_extensions[].
> + *
> + * Note that this will overwrite existing values in cpu->cfg.
>    */
> -static void register_cpu_props(Object *obj)
> +static void register_generic_cpu_props(Object *obj)
>   {
> -    RISCVCPU *cpu = RISCV_CPU(obj);
> -    uint32_t misa_ext = cpu->env.misa_ext;
>       Property *prop;
>       DeviceState *dev = DEVICE(obj);
>   
> -    /*
> -     * If misa_ext is not zero, set cfg properties now to
> -     * allow them to be read during riscv_cpu_realize()
> -     * later on.
> -     */
> -    if (cpu->env.misa_ext != 0) {
> -        cpu->cfg.ext_i = misa_ext & RVI;
> -        cpu->cfg.ext_e = misa_ext & RVE;
> -        cpu->cfg.ext_m = misa_ext & RVM;
> -        cpu->cfg.ext_a = misa_ext & RVA;
> -        cpu->cfg.ext_f = misa_ext & RVF;
> -        cpu->cfg.ext_d = misa_ext & RVD;
> -        cpu->cfg.ext_v = misa_ext & RVV;
> -        cpu->cfg.ext_c = misa_ext & RVC;
> -        cpu->cfg.ext_s = misa_ext & RVS;
> -        cpu->cfg.ext_u = misa_ext & RVU;
> -        cpu->cfg.ext_h = misa_ext & RVH;
> -        cpu->cfg.ext_j = misa_ext & RVJ;
> -
> -        /*
> -         * We don't want to set the default riscv_cpu_extensions
> -         * in this case.
> -         */
> -        return;
> -    }
> -
>       for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>           qdev_property_add_static(dev, prop);
>       }