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.
> {
> 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
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.
> 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 ?
> +
> + /* 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
> + } 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",
>
