Re: [PATCH v2 12/13] target/riscv: Add a preferred ISA extension rule

[Alistair Francis]

On Wed, Jul 24, 2024 at 9:32 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> In addition to the implied rule, a preferred rule will be useful
> where an ISA extension may require a list of ISA extension to be
> enabled to use all the features defined in that extension. All
> these extensions may not be implied in the ISA.

This seems practically the same as an implied rule, I'm not sure we
need a separate list of rules

Alistair

>
> This patch just introduces a new preferred rule which allows
> to enable multiple extensions together without burdening the qemu
> commandline user.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  target/riscv/cpu.c         |  4 ++++
>  target/riscv/cpu.h         | 17 ++++++++++++++
>  target/riscv/tcg/tcg-cpu.c | 57 
> ++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 393d1d67120e..22ba43c7ff2a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2665,6 +2665,10 @@ RISCVCPUImpliedExtsRule 
> *riscv_multi_ext_implied_rules[] = {
>      NULL
>  };
>
> +RISCVCPUPreferredExtsRule *riscv_multi_ext_preferred_rules[] = {
> +    NULL
> +};
> +
>  static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index af25550a4a54..d775866344f5 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -142,10 +142,27 @@ typedef struct riscv_cpu_implied_exts_rule {
>      const uint32_t implied_multi_exts[];
>  } RISCVCPUImpliedExtsRule;
>
> +typedef struct riscv_cpu_preferred_exts_rule {
> +#ifndef CONFIG_USER_ONLY
> +    /*
> +     * Bitmask indicates the rule enabled status for the harts.
> +     * This enhancement is only available in system-mode QEMU,
> +     * as we don't have a good way (e.g. mhartid) to distinguish
> +     * the SMP cores in user-mode QEMU.
> +     */
> +    unsigned long *enabled;
> +#endif
> +    /* multi extension offset. */
> +    const uint32_t ext;
> +    const uint32_t preferred_multi_exts[];
> +} RISCVCPUPreferredExtsRule;
> +
>  extern RISCVCPUImpliedExtsRule *riscv_misa_ext_implied_rules[];
>  extern RISCVCPUImpliedExtsRule *riscv_multi_ext_implied_rules[];
> +extern RISCVCPUPreferredExtsRule *riscv_multi_ext_preferred_rules[];
>
>  #define RISCV_IMPLIED_EXTS_RULE_END -1
> +#define RISCV_PREFRRED_EXTS_RULE_END RISCV_IMPLIED_EXTS_RULE_END
>
>  #define MMU_USER_IDX 3
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 1c9a87029423..d8f74720815a 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -32,6 +32,7 @@
>  #include "hw/core/accel-cpu.h"
>  #include "hw/core/tcg-cpu-ops.h"
>  #include "tcg/tcg.h"
> +#include <stdio.h>
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/boards.h"
>  #endif
> @@ -733,6 +734,7 @@ static void riscv_cpu_validate_profiles(RISCVCPU *cpu)
>  static void riscv_cpu_init_ext_rules(void)
>  {
>      RISCVCPUImpliedExtsRule *rule;
> +    RISCVCPUPreferredExtsRule *prule;
>  #ifndef CONFIG_USER_ONLY
>      MachineState *ms = MACHINE(qdev_get_machine());
>  #endif
> @@ -760,22 +762,40 @@ static void riscv_cpu_init_ext_rules(void)
>                              GUINT_TO_POINTER(rule->ext), (gpointer)rule);
>      }
>
> +    for (i = 0; (prule = riscv_multi_ext_preferred_rules[i]); i++) {
> +#ifndef CONFIG_USER_ONLY
> +        prule->enabled = bitmap_new(ms->smp.cpus);
> +#endif
> +        g_hash_table_insert(multi_ext_enabling_rules,
> +                            GUINT_TO_POINTER(prule->ext), (gpointer)prule);
> +    }
> +
>      initialized = true;
>  }
>
>  static void cpu_enable_ext_rule(RISCVCPU *cpu,
> -                                    RISCVCPUImpliedExtsRule *rule)
> +                                RISCVCPUImpliedExtsRule *rule,
> +                                RISCVCPUPreferredExtsRule *prule)
>  {
>      CPURISCVState *env = &cpu->env;
>      RISCVCPUImpliedExtsRule *ir;
> +    RISCVCPUPreferredExtsRule *pr;
>      bool enabled = false;
>      int i;
>
>  #ifndef CONFIG_USER_ONLY
> -    enabled = test_bit(cpu->env.mhartid, rule->enabled);
> +    if (rule) {
> +        enabled = test_bit(cpu->env.mhartid, rule->enabled);
> +    } else if (prule) {
> +        enabled = test_bit(cpu->env.mhartid, prule->enabled);
> +    } else {
> +        return;
> +    }
>  #endif
> +    if (enabled)
> +        return;
>
> -    if (!enabled) {
> +    if (rule) {
>          /* Enable the implied MISAs. */
>          if (rule->implied_misa_exts) {
>              riscv_cpu_set_misa_ext(env,
> @@ -787,7 +807,7 @@ static void cpu_enable_ext_rule(RISCVCPU *cpu,
>                                               GUINT_TO_POINTER(misa_bits[i]));
>
>                      if (ir) {
> -                        cpu_enable_ext_rule(cpu, ir);
> +                        cpu_enable_ext_rule(cpu, ir, NULL);
>                      }
>                  }
>              }
> @@ -803,12 +823,27 @@ static void cpu_enable_ext_rule(RISCVCPU *cpu,
>                                              rule->implied_multi_exts[i]));
>
>              if (ir) {
> -                cpu_enable_ext_rule(cpu, ir);
> +                cpu_enable_ext_rule(cpu, ir, NULL);
>              }
>          }
>
>  #ifndef CONFIG_USER_ONLY
>          bitmap_set(rule->enabled, cpu->env.mhartid, 1);
> +#endif
> +    } else if (prule) {
> +        /* Enable the preferred extensions. */
> +        for (i = 0;
> +          prule->preferred_multi_exts[i] != RISCV_PREFRRED_EXTS_RULE_END; 
> i++) {
> +            cpu_cfg_ext_auto_update(cpu, prule->preferred_multi_exts[i], 
> true);
> +            pr = g_hash_table_lookup(multi_ext_enabling_rules,
> +                                     GUINT_TO_POINTER(
> +                                     prule->preferred_multi_exts[i]));
> +            if (pr) {
> +                cpu_enable_ext_rule(cpu, NULL, prule);
> +            }
> +        }
> +#ifndef CONFIG_USER_ONLY
> +        bitmap_set(prule->enabled, cpu->env.mhartid, 1);
>  #endif
>      }
>  }
> @@ -847,6 +882,7 @@ static void cpu_enable_zc_implied_rules(RISCVCPU *cpu)
>  static void riscv_cpu_enable_ext_rules(RISCVCPU *cpu)
>  {
>      RISCVCPUImpliedExtsRule *rule;
> +    RISCVCPUPreferredExtsRule *prule;
>      int i;
>
>      /* Enable the implied extensions for Zc. */
> @@ -855,14 +891,21 @@ static void riscv_cpu_enable_ext_rules(RISCVCPU *cpu)
>      /* Enable the implied MISAs. */
>      for (i = 0; (rule = riscv_misa_ext_implied_rules[i]); i++) {
>          if (riscv_has_ext(&cpu->env, rule->ext)) {
> -            cpu_enable_ext_rule(cpu, rule);
> +            cpu_enable_ext_rule(cpu, rule, NULL);
>          }
>      }
>
>      /* Enable the implied extensions. */
>      for (i = 0; (rule = riscv_multi_ext_implied_rules[i]); i++) {
>          if (isa_ext_is_enabled(cpu, rule->ext)) {
> -            cpu_enable_ext_rule(cpu, rule);
> +            cpu_enable_ext_rule(cpu, rule, NULL);
> +        }
> +    }
> +
> +    /* Enable the preferred extensions. */
> +    for (i = 0; (prule = riscv_multi_ext_preferred_rules[i]); i++) {
> +        if (isa_ext_is_enabled(cpu, prule->ext)) {
> +            cpu_enable_ext_rule(cpu, NULL, prule);
>          }
>      }
>  }
>
> --
> 2.34.1
>
>