Message-ID: <586dc43d-74cd-413b-86e2-545384ad796f@rivosinc.com>
Date: Fri, 7 Feb 2025 11:29:08 +0100
From: Clément Léger <cleger@...osinc.com>
To: Atish Patra <atishp@...osinc.com>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Anup Patel <anup@...infault.org>,
 Atish Patra <atishp@...shpatra.org>, Will Deacon <will@...nel.org>,
 Mark Rutland <mark.rutland@....com>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>, weilin.wang@...el.com
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Conor Dooley <conor@...nel.org>, devicetree@...r.kernel.org,
 kvm@...r.kernel.org, kvm-riscv@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v4 12/21] RISC-V: perf: Modify the counter discovery
 mechanism



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@...osinc.com>
> ---
>  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",
> 

Powered by blists - more mailing lists