Re: [PATCH v3 3/6] target/riscv: add profile u_parent and s_parent

[Andrew Jones]

On Wed, Jan 15, 2025 at 10:49:54AM -0300, Daniel Henrique Barboza wrote:
> The current 'parent' mechanic for profiles allows for one profile to be
> a child of a previous/older profile, enabling all its extensions (and
> the parent profile itself) and sparing us from tediously listing all
> extensions for every profile.
> 
> This works fine for u-mode profiles. For s-mode profiles this is not
> enough: a s-mode profile extends not only his equivalent u-mode profile
> but also the previous s-mode profile. This means, for example, that
> RVA23S64 extends both RVA23U64 and RVA22S64.
> 
> To fit this usage, rename the existing 'parent' to 'u_parent' and add a
> new 's_parent' attribute for profiles. Handle both like we're doing with
> the previous 'profile' attribute, i.e. if set, enable it. This change

...like we were doing with the previous 'parent'...

> does nothing for the existing profiles but will make RVA23S64 simpler.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c         |  6 ++++--
>  target/riscv/cpu.h         |  3 ++-
>  target/riscv/tcg/tcg-cpu.c | 35 ++++++++++++++++++++++++++---------
>  3 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 6fb4d5f374..e215b1004d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2349,7 +2349,8 @@ static const PropertyInfo prop_marchid = {
>   * doesn't need to be manually enabled by the profile.
>   */
>  static RISCVCPUProfile RVA22U64 = {
> -    .parent = NULL,
> +    .u_parent = NULL,
> +    .s_parent = NULL,
>      .name = "rva22u64",
>      .misa_ext = RVI | RVM | RVA | RVF | RVD | RVC | RVB | RVU,
>      .priv_spec = RISCV_PROFILE_ATTR_UNUSED,
> @@ -2381,7 +2382,8 @@ static RISCVCPUProfile RVA22U64 = {
>   * The remaining features/extensions comes from RVA22U64.
>   */
>  static RISCVCPUProfile RVA22S64 = {
> -    .parent = &RVA22U64,
> +    .u_parent = &RVA22U64,
> +    .s_parent = NULL,
>      .name = "rva22s64",
>      .misa_ext = RVS,
>      .priv_spec = PRIV_VERSION_1_12_0,
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 97713681cb..986131a191 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -81,7 +81,8 @@ const char *riscv_get_misa_ext_description(uint32_t bit);
>  #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
>  
>  typedef struct riscv_cpu_profile {
> -    struct riscv_cpu_profile *parent;
> +    struct riscv_cpu_profile *u_parent;
> +    struct riscv_cpu_profile *s_parent;
>      const char *name;
>      uint32_t misa_ext;
>      bool enabled;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 48be24bbbe..c9e5a3b580 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -713,13 +713,29 @@ static bool riscv_cpu_validate_profile_satp(RISCVCPU 
> *cpu,
>  }
>  #endif
>  
> +static void riscv_cpu_check_parent_profile(RISCVCPU *cpu,
> +                                           RISCVCPUProfile *profile,
> +                                           RISCVCPUProfile *parent)
> +{
> +    const char *parent_name;
> +    bool parent_enabled;
> +
> +    if (!profile->enabled || !parent) {
> +        return;
> +    }
> +
> +    parent_name = parent->name;
> +    parent_enabled = object_property_get_bool(OBJECT(cpu), parent_name, 
> NULL);
> +    profile->enabled = profile->enabled && parent_enabled;

Could drop the 'profile->enabled &&' since we already know
profile->enabled is true from the test above.

> +}
> +
>  static void riscv_cpu_validate_profile(RISCVCPU *cpu,
>                                         RISCVCPUProfile *profile)
>  {
>      CPURISCVState *env = &cpu->env;
>      const char *warn_msg = "Profile %s mandates disabled extension %s";
>      bool send_warn = profile->user_set && profile->enabled;
> -    bool parent_enabled, profile_impl = true;
> +    bool profile_impl = true;
>      int i;
>  
>  #ifndef CONFIG_USER_ONLY
> @@ -773,12 +789,8 @@ static void riscv_cpu_validate_profile(RISCVCPU *cpu,
>  
>      profile->enabled = profile_impl;
>  
> -    if (profile->parent != NULL) {
> -        parent_enabled = object_property_get_bool(OBJECT(cpu),
> -                                                  profile->parent->name,
> -                                                  NULL);
> -        profile->enabled = profile->enabled && parent_enabled;
> -    }
> +    riscv_cpu_check_parent_profile(cpu, profile, profile->u_parent);
> +    riscv_cpu_check_parent_profile(cpu, profile, profile->s_parent);
>  }
>  
>  static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
> @@ -1181,8 +1193,13 @@ static void cpu_set_profile(Object *obj, Visitor *v, 
> const char *name,
>      profile->user_set = true;
>      profile->enabled = value;
>  
> -    if (profile->parent != NULL) {
> -        object_property_set_bool(obj, profile->parent->name,
> +    if (profile->u_parent != NULL) {
> +        object_property_set_bool(obj, profile->u_parent->name,
> +                                 profile->enabled, NULL);
> +    }
> +
> +    if (profile->s_parent != NULL) {
> +        object_property_set_bool(obj, profile->s_parent->name,
>                                   profile->enabled, NULL);
>      }
>  
> -- 
> 2.47.1
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew