From: "Clément Léger" <cleger@rivosinc.com>
To: Atish Patra <atishp@rivosinc.com>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Anup Patel <anup@brainfault.org>,
Atish Patra <atishp@atishpatra.org>,
Will Deacon <will@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
Adrian Hunter <adrian.hunter@intel.com>,
weilin.wang@intel.com
Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
Conor Dooley <conor@kernel.org>,
devicetree@vger.kernel.org, kvm@vger.kernel.org,
kvm-riscv@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-perf-users@vger.kernel.org
Subject: Re: [PATCH v4 12/21] RISC-V: perf: Modify the counter discovery mechanism
Date: Fri, 7 Feb 2025 11:29:08 +0100 [thread overview]
Message-ID: <586dc43d-74cd-413b-86e2-545384ad796f@rivosinc.com> (raw)
In-Reply-To: <20250205-counter_delegation-v4-12-835cfa88e3b1@rivosinc.com>
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@rivosinc.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",
>
next prev parent reply other threads:[~2025-02-07 10:29 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-06 7:23 [PATCH v4 00/21] Add Counter delegation ISA extension support Atish Patra
2025-02-06 7:23 ` [PATCH v4 01/21] perf pmu-events: Add functions in jevent.py to parse counter and event info for hardware aware grouping Atish Patra
2025-02-06 7:23 ` [PATCH v4 02/21] RISC-V: Add Sxcsrind ISA extension CSR definitions Atish Patra
2025-02-07 7:57 ` Clément Léger
2025-02-06 7:23 ` [PATCH v4 03/21] RISC-V: Add Sxcsrind ISA extension definition and parsing Atish Patra
2025-02-06 7:23 ` [PATCH v4 04/21] dt-bindings: riscv: add Sxcsrind ISA extension description Atish Patra
2025-02-19 14:09 ` Rob Herring (Arm)
2025-02-06 7:23 ` [PATCH v4 05/21] RISC-V: Define indirect CSR access helpers Atish Patra
2025-02-06 7:23 ` [PATCH v4 06/21] RISC-V: Add Smcntrpmf extension parsing Atish Patra
2025-02-07 9:21 ` Clément Léger
2025-02-06 7:23 ` [PATCH v4 07/21] dt-bindings: riscv: add Smcntrpmf ISA extension description Atish Patra
2025-02-19 14:09 ` Rob Herring (Arm)
2025-02-06 7:23 ` [PATCH v4 08/21] RISC-V: Add Sscfg extension CSR definition Atish Patra
2025-02-07 9:30 ` Clément Léger
2025-02-27 0:03 ` Atish Kumar Patra
2025-02-06 7:23 ` [PATCH v4 09/21] RISC-V: Add Ssccfg ISA extension definition and parsing Atish Patra
2025-02-07 8:08 ` Clément Léger
2025-02-07 8:13 ` Clément Léger
2025-02-27 0:06 ` Atish Kumar Patra
2025-02-06 7:23 ` [PATCH v4 10/21] dt-bindings: riscv: add Counter delegation ISA extensions description Atish Patra
2025-02-06 8:39 ` Rob Herring (Arm)
2025-02-06 7:23 ` [PATCH v4 11/21] RISC-V: perf: Restructure the SBI PMU code Atish Patra
2025-02-06 10:51 ` Will Deacon
2025-02-07 16:53 ` Atish Kumar Patra
2025-02-07 9:59 ` Clément Léger
2025-02-06 7:23 ` [PATCH v4 12/21] RISC-V: perf: Modify the counter discovery mechanism Atish Patra
2025-02-07 10:29 ` Clément Léger [this message]
2025-02-27 1:05 ` Atish Kumar Patra
2025-02-06 7:23 ` [PATCH v4 13/21] RISC-V: perf: Add a mechanism to defined legacy event encoding Atish Patra
2025-02-06 7:23 ` [PATCH v4 14/21] RISC-V: perf: Implement supervisor counter delegation support Atish Patra
2025-02-06 7:23 ` [PATCH v4 15/21] RISC-V: perf: Skip PMU SBI extension when not implemented Atish Patra
2025-02-06 7:23 ` [PATCH v4 16/21] RISC-V: perf: Use config2/vendor table for event to counter mapping Atish Patra
2025-02-06 7:23 ` [PATCH v4 17/21] RISC-V: perf: Add legacy event encodings via sysfs Atish Patra
2025-02-06 7:23 ` [PATCH v4 18/21] RISC-V: perf: Add Qemu virt machine events Atish Patra
2025-02-06 7:23 ` [PATCH v4 19/21] tools/perf: Support event code for arch standard events Atish Patra
2025-02-06 7:23 ` [PATCH v4 20/21] tools/perf: Pass the Counter constraint values in the pmu events Atish Patra
2025-02-06 7:23 ` [PATCH v4 21/21] Sync empty-pmu-events.c with autogenerated one Atish Patra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=586dc43d-74cd-413b-86e2-545384ad796f@rivosinc.com \
--to=cleger@rivosinc.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=anup@brainfault.org \
--cc=atishp@atishpatra.org \
--cc=atishp@rivosinc.com \
--cc=conor+dt@kernel.org \
--cc=conor@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=kvm-riscv@lists.infradead.org \
--cc=kvm@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=peterz@infradead.org \
--cc=robh@kernel.org \
--cc=weilin.wang@intel.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).