MIME-Version: 1.0
References: <20230127182558.2416400-1-atishp@rivosinc.com> <20230127182558.2416400-2-atishp@rivosinc.com>
In-Reply-To: <20230127182558.2416400-2-atishp@rivosinc.com>
From:   Anup Patel <anup@brainfault.org>
Date:   Sat, 28 Jan 2023 20:14:38 +0530
Message-ID: <CAAhSdy3U0sgZG6gT3i6vOaupmycHBwALmr_0DHVjuRR7U0Tr6w@mail.gmail.com>
Subject: Re: [PATCH v3 01/14] perf: RISC-V: Define helper functions expose hpm
 counter width and count
To:     Atish Patra <atishp@rivosinc.com>
Cc:     linux-kernel@vger.kernel.org,
        Andrew Jones <ajones@ventanamicro.com>,
        Atish Patra <atishp@atishpatra.org>,
        Guo Ren <guoren@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
        kvm-riscv@lists.infradead.org, kvm@vger.kernel.org,
        linux-riscv@lists.infradead.org,
        Mark Rutland <mark.rutland@arm.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Sergey Matyukevich <sergey.matyukevich@syntacore.com>,
        Will Deacon <will@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 27, 2023 at 11:56 PM Atish Patra <atishp@rivosinc.com> wrote:
>
> KVM module needs to know how many hardware counters and the counter
> width that the platform supports. Otherwise, it will not be able to show
> optimal value of virtual counters to the guest. The virtual hardware
> counters also need to have the same width as the logical hardware
> counters for simplicity. However, there shouldn't be mapping between
> virtual hardware counters and logical hardware counters. As we don't
> support hetergeneous harts or counters with different width as of now,
> the implementation relies on the counter width of the first available
> programmable counter.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  drivers/perf/riscv_pmu_sbi.c   | 37 ++++++++++++++++++++++++++++++++--
>  include/linux/perf/riscv_pmu.h |  3 +++
>  2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index f6507ef..6b53adc 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -44,7 +44,7 @@ static const struct attribute_group *riscv_pmu_attr_groups[] = {
>  };
>
>  /*
> - * RISC-V doesn't have hetergenous harts yet. This need to be part of
> + * RISC-V doesn't have heterogeneous harts yet. This need to be part of
>   * per_cpu in case of harts with different pmu counters
>   */
>  static union sbi_pmu_ctr_info *pmu_ctr_list;
> @@ -52,6 +52,9 @@ static bool riscv_pmu_use_irq;
>  static unsigned int riscv_pmu_irq_num;
>  static unsigned int riscv_pmu_irq;
>
> +/* Cache the available counters in a bitmask */
> +static unsigned long cmask;
> +
>  struct sbi_pmu_event_data {
>         union {
>                 union {
> @@ -267,6 +270,37 @@ static bool pmu_sbi_ctr_is_fw(int cidx)
>         return (info->type == SBI_PMU_CTR_TYPE_FW) ? true : false;
>  }
>
> +/*
> + * Returns the counter width of a programmable counter and number of hardware
> + * counters. As we don't support heterogeneous CPUs yet, it is okay to just
> + * return the counter width of the first programmable counter.
> + */
> +int riscv_pmu_get_hpm_info(u32 *hw_ctr_width, u32 *num_hw_ctr)
> +{
> +       int i;
> +       union sbi_pmu_ctr_info *info;
> +       u32 hpm_width = 0, hpm_count = 0;
> +
> +       if (!cmask)
> +               return -EINVAL;
> +
> +       for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) {
> +               info = &pmu_ctr_list[i];
> +               if (!info)
> +                       continue;
> +               if (!hpm_width && info->csr != CSR_CYCLE && info->csr != CSR_INSTRET)
> +                       hpm_width = info->width;
> +               if (info->type == SBI_PMU_CTR_TYPE_HW)
> +                       hpm_count++;
> +       }
> +
> +       *hw_ctr_width = hpm_width;
> +       *num_hw_ctr = hpm_count;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(riscv_pmu_get_hpm_info);
> +
>  static int pmu_sbi_ctr_get_idx(struct perf_event *event)
>  {
>         struct hw_perf_event *hwc = &event->hw;
> @@ -812,7 +846,6 @@ static void riscv_pmu_destroy(struct riscv_pmu *pmu)
>  static int pmu_sbi_device_probe(struct platform_device *pdev)
>  {
>         struct riscv_pmu *pmu = NULL;
> -       unsigned long cmask = 0;
>         int ret = -ENODEV;
>         int num_counters;
>
> diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h
> index e17e86a..a1c3f77 100644
> --- a/include/linux/perf/riscv_pmu.h
> +++ b/include/linux/perf/riscv_pmu.h
> @@ -73,6 +73,9 @@ void riscv_pmu_legacy_skip_init(void);
>  static inline void riscv_pmu_legacy_skip_init(void) {};
>  #endif
>  struct riscv_pmu *riscv_pmu_alloc(void);
> +#ifdef CONFIG_RISCV_PMU_SBI
> +int riscv_pmu_get_hpm_info(u32 *hw_ctr_width, u32 *num_hw_ctr);
> +#endif
>
>  #endif /* CONFIG_RISCV_PMU */
>
> --
> 2.25.1
>