Re: [PATCH v3 2/3] target/riscv: Extend isa_ext_data for single letter extensions

[Alistair Francis]

On Fri, Dec 9, 2022 at 12:58 AM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> Currently the ISA string for a CPU is generated from two different
> arrays, one for single letter extensions and another for multi letter
> extensions. Add all the single letter extensions to the isa_ext_data
> array and use it for generating the ISA string. Also drop 'P' and 'Q'
> extensions from the list of single letter extensions as those are not
> supported yet.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>

This breaks the SiFive CPUs (as well as others). A large number of
CPUs set these single letter extensions just with set_misa(), so the
cfg values are never actually set.

We probably want to add something like this (does not compile):

@@ -222,6 +225,10 @@ static void set_misa(CPURISCVState *env, RISCVMXL
mxl, uint32_t ext)
{
    env->misa_mxl_max = env->misa_mxl = mxl;
    env->misa_ext_mask = env->misa_ext = ext;
+
+    if (ext & RVI == RVI) {
+        cpu->cfg.ext_i = true;
+    }
}

Alistair

> ---
>  target/riscv/cpu.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 042fd541b4..8c8f085a80 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -41,8 +41,6 @@
>                               (QEMU_VERSION_MICRO))
>  #define RISCV_CPU_MIMPID    RISCV_CPU_MARCHID
>
> -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> -
>  struct isa_ext_data {
>      const char *name;
>      bool multi_letter;
> @@ -71,6 +69,13 @@ struct isa_ext_data {
>   *    extensions by an underscore.
>   */
>  static const struct isa_ext_data isa_edata_arr[] = {
> +    ISA_EXT_DATA_ENTRY(i, false, PRIV_VERSION_1_10_0, ext_i),
> +    ISA_EXT_DATA_ENTRY(e, false, PRIV_VERSION_1_10_0, ext_e),
> +    ISA_EXT_DATA_ENTRY(m, false, PRIV_VERSION_1_10_0, ext_m),
> +    ISA_EXT_DATA_ENTRY(a, false, PRIV_VERSION_1_10_0, ext_a),
> +    ISA_EXT_DATA_ENTRY(f, false, PRIV_VERSION_1_10_0, ext_f),
> +    ISA_EXT_DATA_ENTRY(d, false, PRIV_VERSION_1_10_0, ext_d),
> +    ISA_EXT_DATA_ENTRY(c, false, PRIV_VERSION_1_10_0, ext_c),
>      ISA_EXT_DATA_ENTRY(h, false, PRIV_VERSION_1_12_0, ext_h),
>      ISA_EXT_DATA_ENTRY(v, false, PRIV_VERSION_1_12_0, ext_v),
>      ISA_EXT_DATA_ENTRY(zicsr, true, PRIV_VERSION_1_10_0, ext_icsr),
> @@ -1196,16 +1201,23 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> -static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int 
> max_str_len)
> +static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str)
>  {
>      char *old = *isa_str;
>      char *new = *isa_str;
>      int i;
>
>      for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> -        if (isa_edata_arr[i].multi_letter &&
> -            isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> -            new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> +        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> +            if (isa_edata_arr[i].multi_letter) {
> +                if (cpu->cfg.short_isa_string) {
> +                    continue;
> +                }
> +                new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> +            } else {
> +                new = g_strconcat(old, isa_edata_arr[i].name, NULL);
> +            }
> +
>              g_free(old);
>              old = new;
>          }
> @@ -1216,19 +1228,12 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char 
> **isa_str, int max_str_len)
>
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
> -    int i;
> -    const size_t maxlen = sizeof("rv128") + sizeof(riscv_single_letter_exts);
> +    const size_t maxlen = sizeof("rv128");
>      char *isa_str = g_new(char, maxlen);
> -    char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
> -    for (i = 0; i < sizeof(riscv_single_letter_exts) - 1; i++) {
> -        if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) {
> -            *p++ = qemu_tolower(riscv_single_letter_exts[i]);
> -        }
> -    }
> -    *p = '\0';
> -    if (!cpu->cfg.short_isa_string) {
> -        riscv_isa_string_ext(cpu, &isa_str, maxlen);
> -    }
> +
> +    snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS);
> +    riscv_isa_string_ext(cpu, &isa_str);
> +
>      return isa_str;
>  }
>
> --
> 2.34.1
>
>