Re: [PATCH v2 2/6] target/riscv: add riscv,isa to named features

[Alistair Francis]

Alistair

On Fri, Jan 26, 2024 at 11:32 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
> Further discussions after the introduction of rva22 support in QEMU
> revealed that what we've been calling 'named features' are actually
> regular extensions, with their respective riscv,isa DTs. This is
> clarified in [1]. [2] is a bug tracker asking for the profile spec to be
> less cryptic about it.
>
> As far as QEMU goes we understand extensions as something that the user
> can enable/disable in the command line. This isn't the case for named
> features, so we'll have to reach a middle ground.
>
> We'll keep our existing nomenclature 'named features' to refer to any
> extension that the user can't control in the command line. We'll also do
> the following:
>
> - 'svade' and 'zic64b' flags are renamed to 'ext_svade' and
>   'ext_zic64b'. 'ext_svade' and 'ext_zic64b' now have riscv,isa strings and
>   priv_spec versions;
>
> - skip name feature check in cpu_bump_multi_ext_priv_ver(). Now that
>   named features have a riscv,isa and an entry in isa_edata_arr[] we
>   don't need to gate the call to cpu_cfg_ext_get_min_version() anymore.
>
> [1] https://github.com/riscv/riscv-profiles/issues/121
> [2] https://github.com/riscv/riscv-profiles/issues/142
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 17 +++++++++++++----
>  target/riscv/cpu_cfg.h     |  6 ++++--
>  target/riscv/tcg/tcg-cpu.c | 16 ++++++----------
>  3 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 88e8cc868144..28d3cfa8ce59 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -97,6 +97,7 @@ bool riscv_cpu_option_set(const char *optname)
>   * instead.
>   */
>  const RISCVIsaExtData isa_edata_arr[] = {
> +    ISA_EXT_DATA_ENTRY(zic64b, PRIV_VERSION_1_12_0, ext_zic64b),
>      ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
>      ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
>      ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> @@ -171,6 +172,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
>      ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>      ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
> +    ISA_EXT_DATA_ENTRY(svade, PRIV_VERSION_1_11_0, ext_svade),
>      ISA_EXT_DATA_ENTRY(svadu, PRIV_VERSION_1_12_0, ext_svadu),
>      ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
>      ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
> @@ -1510,9 +1512,16 @@ const RISCVCPUMultiExtConfig 
> riscv_cpu_experimental_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +/*
> + * 'Named features' is the name we give to extensions that we
> + * don't want to expose to users. They are either immutable
> + * (always enabled/disable) or they'll vary depending on
> + * the resulting CPU state. They have riscv,isa strings
> + * and priv_ver like regular extensions.
> + */
>  const RISCVCPUMultiExtConfig riscv_cpu_named_features[] = {
> -    MULTI_EXT_CFG_BOOL("svade", svade, true),
> -    MULTI_EXT_CFG_BOOL("zic64b", zic64b, true),
> +    MULTI_EXT_CFG_BOOL("svade", ext_svade, true),
> +    MULTI_EXT_CFG_BOOL("zic64b", ext_zic64b, true),
>
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -2130,7 +2139,7 @@ static RISCVCPUProfile RVA22U64 = {
>          CPU_CFG_OFFSET(ext_zicbop), CPU_CFG_OFFSET(ext_zicboz),
>
>          /* mandatory named features for this profile */
> -        CPU_CFG_OFFSET(zic64b),
> +        CPU_CFG_OFFSET(ext_zic64b),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> @@ -2161,7 +2170,7 @@ static RISCVCPUProfile RVA22S64 = {
>          CPU_CFG_OFFSET(ext_svinval),
>
>          /* rva22s64 named features */
> -        CPU_CFG_OFFSET(svade),
> +        CPU_CFG_OFFSET(ext_svade),
>
>          RISCV_PROFILE_EXT_LIST_END
>      }
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index e241922f89c4..698f926ab1be 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -117,13 +117,15 @@ struct RISCVCPUConfig {
>      bool ext_smepmp;
>      bool rvv_ta_all_1s;
>      bool rvv_ma_all_1s;
> -    bool svade;
> -    bool zic64b;
>
>      uint32_t mvendorid;
>      uint64_t marchid;
>      uint64_t mimpid;
>
> +    /* Named features  */
> +    bool ext_svade;
> +    bool ext_zic64b;
> +
>      /* Vendor-specific custom extensions */
>      bool ext_xtheadba;
>      bool ext_xtheadbb;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 88f92d1c7d2c..90861cc065e5 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -197,12 +197,12 @@ static bool cpu_cfg_offset_is_named_feat(uint32_t 
> ext_offset)
>  static void riscv_cpu_enable_named_feat(RISCVCPU *cpu, uint32_t feat_offset)
>  {
>      switch (feat_offset) {
> -    case CPU_CFG_OFFSET(zic64b):
> +    case CPU_CFG_OFFSET(ext_zic64b):
>          cpu->cfg.cbom_blocksize = 64;
>          cpu->cfg.cbop_blocksize = 64;
>          cpu->cfg.cboz_blocksize = 64;
>          break;
> -    case CPU_CFG_OFFSET(svade):
> +    case CPU_CFG_OFFSET(ext_svade):
>          cpu->cfg.ext_svadu = false;
>          break;
>      default:
> @@ -219,10 +219,6 @@ static void cpu_bump_multi_ext_priv_ver(CPURISCVState 
> *env,
>          return;
>      }
>
> -    if (cpu_cfg_offset_is_named_feat(ext_offset)) {
> -        return;
> -    }
> -
>      ext_priv_ver = cpu_cfg_ext_get_min_version(ext_offset);
>
>      if (env->priv_ver < ext_priv_ver) {
> @@ -349,11 +345,11 @@ static void 
> riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>
>  static void riscv_cpu_update_named_features(RISCVCPU *cpu)
>  {
> -    cpu->cfg.zic64b = cpu->cfg.cbom_blocksize == 64 &&
> -                      cpu->cfg.cbop_blocksize == 64 &&
> -                      cpu->cfg.cboz_blocksize == 64;
> +    cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 &&
> +                          cpu->cfg.cbop_blocksize == 64 &&
> +                          cpu->cfg.cboz_blocksize == 64;
>
> -    cpu->cfg.svade = !cpu->cfg.ext_svadu;
> +    cpu->cfg.ext_svade = !cpu->cfg.ext_svadu;
>  }
>
>  static void riscv_cpu_validate_g(RISCVCPU *cpu)
> --
> 2.43.0
>
>