Re: [PATCH v4 1/6] target/riscv: Propagate error from PMU setup

[Atish Kumar Patra]

On Wed, Oct 18, 2023 at 8:44 AM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> More closely follow the QEMU style by returning an Error and propagating
> it there is an error relating to the PMU setup.
>
> Further simplify the function by removing the num_counters parameter as
> this is available from the passed in cpu pointer.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>  target/riscv/pmu.c         | 19 +++++++++----------
>  target/riscv/pmu.h         |  3 ++-
>  target/riscv/tcg/tcg-cpu.c |  8 +++++++-
>  3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 36f6307d28..13801ccb78 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -434,22 +434,21 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t 
> value, uint32_t ctr_idx)
>  }
>
>
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters)
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
>  {
> -    if (num_counters > (RV_MAX_MHPMCOUNTERS - 3)) {
> -        return -1;
> +    uint8_t pmu_num = cpu->cfg.pmu_num;
> +
> +    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> +        error_setg(errp, "Number of counters exceeds maximum available");
> +        return;
>      }
>
>      cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
>      if (!cpu->pmu_event_ctr_map) {
> -        /* PMU support can not be enabled */
> -        qemu_log_mask(LOG_UNIMP, "PMU events can't be supported\n");
> -        cpu->cfg.pmu_num = 0;
> -        return -1;
> +        error_setg(errp, "Unable to allocate PMU event hash table");
> +        return;
>      }
>
>      /* Create a bitmask of available programmable counters */
> -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, num_counters);
> -
> -    return 0;
> +    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
>  }
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 2bfb71ba87..88e0713296 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -17,13 +17,14 @@
>   */
>
>  #include "cpu.h"
> +#include "qapi/error.h"
>
>  bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>                                          uint32_t target_ctr);
>  bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
>                                    uint32_t target_ctr);
>  void riscv_pmu_timer_cb(void *priv);
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
>  int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>                                 uint32_t ctr_idx);
>  int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index a28918ab30..ed3eb991c0 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -614,7 +614,13 @@ static bool tcg_cpu_realize(CPUState *cs, Error **errp)
>      }
>
>      if (cpu->cfg.pmu_num) {
> -        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) 
> {
> +        riscv_pmu_init(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return false;
> +        }
> +
> +        if (cpu->cfg.ext_sscofpmf) {
>              cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                            riscv_pmu_timer_cb, cpu);
>          }
> --
> 2.41.0
>


Reviewed-by: Atish Patra <atishp@rivosinc.com>