Re: [PATCH v7 15/16] target/riscv/tcg: validate profiles during finalize

[Andrew Jones]

On Tue, Oct 31, 2023 at 05:39:15PM -0300, Daniel Henrique Barboza wrote:
> Enabling a profile and then disabling some of its mandatory extensions
> is a valid use. It can be useful for debugging and testing. But the
> common expected use of enabling a profile is to enable all its mandatory
> extensions.
> 
> Add an user warning when mandatory extensions from an enabled profile
> are disabled in the command line. We're also going to disable the
> profile flag in this case since the profile must include all the
> mandatory extensions. This flag can be exposed by QMP to indicate the
> actual profile state after the CPU is realized.
> 
> After this patch, this will throw warnings:
> 
> -cpu rv64,rva22u64=true,zihintpause=false,zicbom=false,zicboz=false
> 
> qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension 
> zihintpause
> qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension 
> zicbom
> qemu-system-riscv64: warning: Profile rva22u64 mandates disabled extension 
> zicboz
> 
> Note that the following will NOT throw warnings because the profile is
> being enabled last, hence all its mandatory extensions will be enabled:
> 
> -cpu rv64,zihintpause=false,zicbom=false,zicboz=false,rva22u64=true
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/tcg/tcg-cpu.c | 70 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 544f6dd01d..23007b19e4 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -147,6 +147,27 @@ static int cpu_cfg_ext_get_min_version(uint32_t 
> ext_offset)
>      g_assert_not_reached();
>  }
>  
> +static const char *cpu_cfg_ext_get_name(uint32_t ext_offset)
> +{
> +    const RISCVCPUMultiExtConfig *feat;
> +    const RISCVIsaExtData *edata;
> +
> +    for (edata = isa_edata_arr; edata && edata->name; edata++) {

No need to check edata isn't null since we have the end-of-list, just like
feat isn't checked below.

> +        if (edata->ext_enable_offset == ext_offset) {
> +            return edata->name;
> +        }
> +    }
> +
> +    for (feat = riscv_cpu_named_features; feat->name != NULL; feat++) {
> +        if (feat->offset == ext_offset) {
> +            return feat->name;
> +        }
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
> +

extra blank line here

>  static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
>                                      bool value)
>  {
> @@ -631,6 +652,54 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>      riscv_cpu_disable_priv_spec_isa_exts(cpu);
>  }
>  
> +static void riscv_cpu_validate_profile(RISCVCPU *cpu,
> +                                       RISCVCPUProfile *profile)
> +{
> +    const char *warn_msg = "Profile %s mandates disabled extension %s";
> +    bool send_warn = profile->user_set && profile->enabled;
> +    bool profile_impl = true;
> +    int i;
> +
> +    for (i = 0; misa_bits[i] != 0; i++) {
> +        uint32_t bit = misa_bits[i];
> +
> +        if (!(profile->misa_ext & bit)) {
> +            continue;
> +        }
> +
> +        if (!riscv_has_ext(&cpu->env, bit)) {
> +            profile_impl = false;
> +
> +            if (send_warn) {
> +                warn_report(warn_msg, profile->name,
> +                            riscv_get_misa_ext_name(bit));
> +            }
> +        }
> +    }
> +
> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
> +        int ext_offset = profile->ext_offsets[i];
> +
> +        if (!isa_ext_is_enabled(cpu, ext_offset)) {
> +            profile_impl = false;
> +
> +            if (send_warn) {
> +                warn_report(warn_msg, profile->name,
> +                            cpu_cfg_ext_get_name(ext_offset));
> +            }
> +        }
> +    }
> +
> +    profile->enabled = profile_impl;
> +}
> +
> +static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
> +{
> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
> +        riscv_cpu_validate_profile(cpu, riscv_profiles[i]);
> +    }
> +}
> +
>  void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>  {
>      CPURISCVState *env = &cpu->env;
> @@ -649,6 +718,7 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error 
> **errp)
>      }
>  
>      riscv_cpu_validate_named_features(cpu);
> +    riscv_cpu_validate_profiles(cpu);
>  
>      if (cpu->cfg.ext_smepmp && !cpu->cfg.pmp) {
>          /*
> -- 
> 2.41.0
>

Other than the nits,

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