On Fri, Feb 7, 2025 at 2:29 AM Clément Léger <cleger@xxxxxxxxxxxx> wrote:
>
>
>
> On 06/02/2025 08:23, Atish Patra wrote:
> > If both counter delegation and SBI PMU is present, the counter
> > delegation will be used for hardware pmu counters while the SBI PMU
> > will be used for firmware counters. Thus, the driver has to probe
> > the counters info via SBI PMU to distinguish the firmware counters.
> >
> > The hybrid scheme also requires improvements of the informational
> > logging messages to indicate the user about underlying interface
> > used for each use case.
> >
> > Signed-off-by: Atish Patra <atishp@xxxxxxxxxxxx>
> > ---
> > drivers/perf/riscv_pmu_dev.c | 118 ++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 88 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_dev.c b/drivers/perf/riscv_pmu_dev.c
> > index 6b43d844eaea..5ddf4924c5b3 100644
> > --- a/drivers/perf/riscv_pmu_dev.c
> > +++ b/drivers/perf/riscv_pmu_dev.c
> > @@ -66,6 +66,10 @@ static bool sbi_v2_available;
> > static DEFINE_STATIC_KEY_FALSE(sbi_pmu_snapshot_available);
> > #define sbi_pmu_snapshot_available() \
> > static_branch_unlikely(&sbi_pmu_snapshot_available)
> > +static DEFINE_STATIC_KEY_FALSE(riscv_pmu_sbi_available);
> > +static DEFINE_STATIC_KEY_FALSE(riscv_pmu_cdeleg_available);
> > +static bool cdeleg_available;
> > +static bool sbi_available;
> >
> > static struct attribute *riscv_arch_formats_attr[] = {
> > &format_attr_event.attr,
> > @@ -88,7 +92,8 @@ static int sysctl_perf_user_access __read_mostly = SYSCTL_USER_ACCESS;
> >
> > /*
> > * This structure is SBI specific but counter delegation also require counter
> > - * width, csr mapping. Reuse it for now.
> > + * width, csr mapping. Reuse it for now we can have firmware counters for
> > + * platfroms with counter delegation support.
> > * RISC-V doesn't have heterogeneous harts yet. This need to be part of
> > * per_cpu in case of harts with different pmu counters
> > */
> > @@ -100,6 +105,8 @@ static unsigned int riscv_pmu_irq;
> >
> > /* Cache the available counters in a bitmask */
> > static unsigned long cmask;
> > +/* Cache the available firmware counters in another bitmask */
> > +static unsigned long firmware_cmask;
> >
> > struct sbi_pmu_event_data {
> > union {
> > @@ -778,35 +785,49 @@ static int rvpmu_sbi_find_num_ctrs(void)
> > return sbi_err_map_linux_errno(ret.error);
> > }
> >
> > -static int rvpmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
> > +static int rvpmu_deleg_find_ctrs(void)
> > +{
> > + /* TODO */
> > + return -1;
> > +}
> > +
> > +static int rvpmu_sbi_get_ctrinfo(int nsbi_ctr, int ndeleg_ctr)
>
> Hi Atish,
>
> These parameters could be unsigned I think.
>
sure. Changed it to u32.
> > {
> > struct sbiret ret;
> > - int i, num_hw_ctr = 0, num_fw_ctr = 0;
> > + int i, num_hw_ctr = 0, num_fw_ctr = 0, num_ctr = 0;
> > union sbi_pmu_ctr_info cinfo;
> >
> > - pmu_ctr_list = kcalloc(nctr, sizeof(*pmu_ctr_list), GFP_KERNEL);
> > - if (!pmu_ctr_list)
> > - return -ENOMEM;
> > -
> > - for (i = 0; i < nctr; i++) {
> > + for (i = 0; i < nsbi_ctr; i++) {
> > ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_INFO, i, 0, 0, 0, 0, 0);
> > if (ret.error)
> > /* The logical counter ids are not expected to be contiguous */
> > continue;
> >
> > - *mask |= BIT(i);
> > -
> > cinfo.value = ret.value;
> > if (cinfo.type == SBI_PMU_CTR_TYPE_FW)
> > num_fw_ctr++;
> > - else
> > +
> > + if (!cdeleg_available) {
>
> What is the rationale for using additional boolean identical to the
> static keys ? Reducing the amount of code patch site in cold path ? If
yes.
> so, I guess you can use static_key_enabled(&riscv_pmu_cdeleg_available).
> But your solution is fine as well, it just duplicates two identical values.
>
good point. I will change it. Thanks!
> > num_hw_ctr++;
> > - pmu_ctr_list[i].value = cinfo.value;
> > + cmask |= BIT(i);
> > + pmu_ctr_list[i].value = cinfo.value;
> > + } else if (cinfo.type == SBI_PMU_CTR_TYPE_FW) {
> > + /* Track firmware counters in a different mask */
> > + firmware_cmask |= BIT(i);
> > + pmu_ctr_list[i].value = cinfo.value;
> > + }
> > +
> > }
> >
> > - pr_info("%d firmware and %d hardware counters\n", num_fw_ctr, num_hw_ctr);
> > + if (cdeleg_available) {
> > + pr_info("%d firmware and %d hardware counters\n", num_fw_ctr, ndeleg_ctr);
> > + num_ctr = num_fw_ctr + ndeleg_ctr;
> > + } else {
> > + pr_info("%d firmware and %d hardware counters\n", num_fw_ctr, num_hw_ctr);
> > + num_ctr = nsbi_ctr;
> > + }
> >
> > - return 0;
> > + return num_ctr;
> > }
> >
> > static inline void rvpmu_sbi_stop_all(struct riscv_pmu *pmu)
> > @@ -1067,16 +1088,33 @@ static void rvpmu_ctr_stop(struct perf_event *event, unsigned long flag)
> > /* TODO: Counter delegation implementation */
> > }
> >
> > -static int rvpmu_find_num_ctrs(void)
> > +static int rvpmu_find_ctrs(void)
> > {
> > - return rvpmu_sbi_find_num_ctrs();
> > - /* TODO: Counter delegation implementation */
> > -}
> > + int num_sbi_counters = 0, num_deleg_counters = 0, num_counters = 0;
> >
> > -static int rvpmu_get_ctrinfo(int nctr, unsigned long *mask)
> > -{
> > - return rvpmu_sbi_get_ctrinfo(nctr, mask);
> > - /* TODO: Counter delegation implementation */
> > + /*
> > + * We don't know how many firmware counters available. Just allocate
> > + * for maximum counters driver can support. The default is 64 anyways.
> > + */
> > + pmu_ctr_list = kcalloc(RISCV_MAX_COUNTERS, sizeof(*pmu_ctr_list),
> > + GFP_KERNEL);
> > + if (!pmu_ctr_list)
> > + return -ENOMEM;
> > +
> > + if (cdeleg_available)
> > + num_deleg_counters = rvpmu_deleg_find_ctrs();
> > +
> > + /* This is required for firmware counters even if the above is true */
> > + if (sbi_available)
> > + num_sbi_counters = rvpmu_sbi_find_num_ctrs();
> > +
> > + if (num_sbi_counters >= RISCV_MAX_COUNTERS || num_deleg_counters >= RISCV_MAX_COUNTERS)
> > + return -ENOSPC;
>
> Why is this using '>=' ? You allocated space for RISCV_MAX_COUNTERS, so
> RISCV_MAX_COUNTERS should fit right ?
>
Yeah. That's a typo. Thanks for catching it.
> > +
> > + /* cache all the information about counters now */
> > + num_counters = rvpmu_sbi_get_ctrinfo(num_sbi_counters, num_deleg_counters);
> > +
> > + return num_counters;
>
> return rvpmu_sbi_get_ctrinfo(num_sbi_counters, num_deleg_counters);
>
> > }
> >
> > static int rvpmu_event_map(struct perf_event *event, u64 *econfig)
> > @@ -1377,12 +1415,21 @@ static int rvpmu_device_probe(struct platform_device *pdev)
> > int ret = -ENODEV;
> > int num_counters;
> >
> > - pr_info("SBI PMU extension is available\n");
> > + if (cdeleg_available) {
> > + pr_info("hpmcounters will use the counter delegation ISA extension\n");
> > + if (sbi_available)
> > + pr_info("Firmware counters will be use SBI PMU extension\n");
>
> s/will be use/will use
>
> > + else
> > + pr_info("Firmware counters will be not available as SBI PMU extension is not present\n");
>
> s/will be not/will not
>
Fixed.
> > + } else if (sbi_available) {
> > + pr_info("Both hpmcounters and firmware counters will use SBI PMU extension\n");
> > + }
> > +
> > pmu = riscv_pmu_alloc();
> > if (!pmu)
> > return -ENOMEM;
> >
> > - num_counters = rvpmu_find_num_ctrs();
> > + num_counters = rvpmu_find_ctrs();
> > if (num_counters < 0) {
> > pr_err("SBI PMU extension doesn't provide any counters\n");
> > goto out_free;
> > @@ -1394,9 +1441,6 @@ static int rvpmu_device_probe(struct platform_device *pdev)
> > pr_info("SBI returned more than maximum number of counters. Limiting the number of counters to %d\n", num_counters);
> > }
> >
> > - /* cache all the information about counters now */
> > - if (rvpmu_get_ctrinfo(num_counters, &cmask))
> > - goto out_free;
> >
> > ret = rvpmu_setup_irqs(pmu, pdev);
> > if (ret < 0) {
> > @@ -1486,13 +1530,27 @@ static int __init rvpmu_devinit(void)
> > int ret;
> > struct platform_device *pdev;
> >
> > - if (sbi_spec_version < sbi_mk_version(0, 3) ||
> > - !sbi_probe_extension(SBI_EXT_PMU)) {
> > - return 0;
> > + if (sbi_spec_version >= sbi_mk_version(0, 3) &&
> > + sbi_probe_extension(SBI_EXT_PMU)) {
> > + static_branch_enable(&riscv_pmu_sbi_available);
> > + sbi_available = true;
> > }
> >
> > if (sbi_spec_version >= sbi_mk_version(2, 0))
> > sbi_v2_available = true;
> > + /*
> > + * We need all three extensions to be present to access the counters
> > + * in S-mode via Supervisor Counter delegation.
> > + */
> > + if (riscv_isa_extension_available(NULL, SSCCFG) &&
> > + riscv_isa_extension_available(NULL, SMCDELEG) &&
> > + riscv_isa_extension_available(NULL, SSCSRIND)) {
> > + static_branch_enable(&riscv_pmu_cdeleg_available);
> > + cdeleg_available = true;
> > + }
> > +
> > + if (!(sbi_available || cdeleg_available))
> > + return 0;
> >
> > ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING,
> > "perf/riscv/pmu:starting",
> >
>
