Re: [PATCH v3 03/16] target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 5:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Every property in riscv_cpu_options[] will be migrated to
> riscv_cpu_properties[]. This will make their default values init
> earlier, allowing cpu_init() functions to overwrite them. We'll also
> implement common getters and setters that both accelerators will use,
> allowing them to share validations that TCG is doing.
>
> At the same time, some options (namely 'vlen', 'elen' and the cache
> blocksizes) need a way of tracking if the user set a value for them.
> This is benign for TCG since the cost of always validating these values
> are small, but for KVM we need syscalls to read the host values to make
> the validations, thus knowing whether the user didn't touch the values
> makes a difference.
>
> We'll track user setting for these properties using a hash, like we do
> in the TCG driver. All riscv cpu options will update this hash in case
> the user sets it. The KVM driver will use this hash to minimize the
> amount of syscalls done.
>
> For now, both 'pmu-mask' and 'pmu-num' shouldn't be changed for vendor
> CPUs. The existing setter for 'pmu-num' is changed to add this
> restriction. New getters and setters are required for 'pmu-mask'
>
> While we're at it, add a 'static' modifier to 'prop_pmu_num' since we're
> not exporting it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 96 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 65cfa6c740..e90b70c0a7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -53,6 +53,15 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, 
> RVD, RVV,
>  #define BYTE(x)   (x)
>  #endif
>
> +/* Hash that stores general user set numeric options */
> +static GHashTable *general_user_opts;
> +
> +static void cpu_option_add_user_setting(const char *optname, uint32_t value)
> +{
> +    g_hash_table_insert(general_user_opts, (gpointer)optname,
> +                        GUINT_TO_POINTER(value));
> +}
> +
>  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
>      {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
>
> @@ -1218,11 +1227,15 @@ static void riscv_cpu_post_init(Object *obj)
>
>  static void riscv_cpu_init(Object *obj)
>  {
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>  #ifndef CONFIG_USER_ONLY
>      qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
>                        IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
>  #endif /* CONFIG_USER_ONLY */
>
> +    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
> +
>      /*
>       * The timer and performance counters extensions were supported
>       * in QEMU before they were added as discrete extensions in the
> @@ -1232,6 +1245,9 @@ static void riscv_cpu_init(Object *obj)
>       */
>      RISCV_CPU(obj)->cfg.ext_zicntr = true;
>      RISCV_CPU(obj)->cfg.ext_zihpm = true;
> +
> +    /* Default values for non-bool cpu properties */
> +    cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
>  }
>
>  typedef struct misa_ext_info {
> @@ -1431,26 +1447,51 @@ const RISCVCPUMultiExtConfig 
> riscv_cpu_deprecated_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static bool riscv_cpu_is_vendor(Object *obj)
> +{
> +    return !riscv_cpu_is_generic(obj);
> +}
> +
> +static void cpu_set_prop_err(RISCVCPU *cpu, const char *propname,
> +                             Error **errp)
> +{
> +    g_autofree char *cpuname = riscv_cpu_get_name(cpu);
> +    error_setg(errp, "CPU '%s' does not allow changing the value of '%s'",
> +               cpuname, propname);
> +}
> +
>  static void prop_pmu_num_set(Object *obj, Visitor *v, const char *name,
>                               void *opaque, Error **errp)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
> -    uint8_t pmu_num;
> +    uint8_t pmu_num, curr_pmu_num;
> +    uint32_t pmu_mask;
>
>      visit_type_uint8(v, name, &pmu_num, errp);
>
> +    curr_pmu_num = ctpop32(cpu->cfg.pmu_mask);
> +
> +    if (pmu_num != curr_pmu_num && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %u\n",
> +                          name, curr_pmu_num);
> +        return;
> +    }
> +
>      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
>          error_setg(errp, "Number of counters exceeds maximum available");
>          return;
>      }
>
>      if (pmu_num == 0) {
> -        cpu->cfg.pmu_mask = 0;
> +        pmu_mask = 0;
>      } else {
> -        cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
> +        pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
>      }
>
>      warn_report("\"pmu-num\" property is deprecated; use \"pmu-mask\"");
> +    cpu->cfg.pmu_mask = pmu_mask;
> +    cpu_option_add_user_setting("pmu-mask", pmu_mask);
>  }
>
>  static void prop_pmu_num_get(Object *obj, Visitor *v, const char *name,
> @@ -1462,16 +1503,54 @@ static void prop_pmu_num_get(Object *obj, Visitor *v, 
> const char *name,
>      visit_type_uint8(v, name, &pmu_num, errp);
>  }
>
> -const PropertyInfo prop_pmu_num = {
> +static const PropertyInfo prop_pmu_num = {
>      .name = "pmu-num",
>      .get = prop_pmu_num_get,
>      .set = prop_pmu_num_set,
>  };
>
> -Property riscv_cpu_options[] = {
> -    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 
> MAKE_64BIT_MASK(3, 16)),
> -    {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
> +static void prop_pmu_mask_set(Object *obj, Visitor *v, const char *name,
> +                             void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint32_t value;
> +    uint8_t pmu_num;
> +
> +    visit_type_uint32(v, name, &value, errp);
> +
> +    if (value != cpu->cfg.pmu_mask && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %x\n",
> +                          name, cpu->cfg.pmu_mask);
> +        return;
> +    }
> +
> +    pmu_num = ctpop32(value);
> +
> +    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> +        error_setg(errp, "Number of counters exceeds maximum available");
> +        return;
> +    }
>
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.pmu_mask = value;
> +}
> +
> +static void prop_pmu_mask_get(Object *obj, Visitor *v, const char *name,
> +                             void *opaque, Error **errp)
> +{
> +    uint8_t pmu_mask = RISCV_CPU(obj)->cfg.pmu_mask;
> +
> +    visit_type_uint8(v, name, &pmu_mask, errp);
> +}
> +
> +static const PropertyInfo prop_pmu_mask = {
> +    .name = "pmu-mask",
> +    .get = prop_pmu_mask_get,
> +    .set = prop_pmu_mask_set,
> +};
> +
> +Property riscv_cpu_options[] = {
>      DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
>      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
>
> @@ -1490,6 +1569,9 @@ Property riscv_cpu_options[] = {
>  static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>
> +    {.name = "pmu-mask", .info = &prop_pmu_mask},
> +    {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> --
> 2.43.0
>
>