[PATCH v2 0/2] perf: RISC-V: fix access beyond allocated array

[PATCH v2 0/2] perf: RISC-V: fix access beyond allocated array

[Sergey Matyukevich]

Hi all,

These patches suggest some fixes and cleanups for the handling of pmu
counters. The first patch fixes access beyond the allocated pmu_ctr_list
array. The second patch fixes the counters mask sent to SBI firmware: it
excludes counters that were not fully specified by SBI firmware on init.

Initial attempt to fix access to the highest available has been reworked.
Now it is handled in the OpenSBI, see the following patch:
- https://patchwork.ozlabs.org/project/opensbi/patch/20220624110330.452640-1-geomatsi@gmail.com/

Regards,
Sergey

v1 -> v2:
- drop changes for access to the highest available counter as they are
  now handled on the OpenSBI side
- drop switch to IDR: in fact there is no need to handle non-contiguous
  counter ranges

Sergey Matyukevich (2):
  perf: RISC-V: fix access beyond allocated array
  perf: RISC-V: exclude invalid pmu counters from SBI calls

 drivers/perf/riscv_pmu_legacy.c |  4 ++--
 drivers/perf/riscv_pmu_sbi.c    | 24 ++++++++++++++----------
 include/linux/perf/riscv_pmu.h  |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 1/2] perf: RISC-V: fix access beyond allocated array

[Sergey Matyukevich]

From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

The root cause could be related to the interpretation of the number of
counters reported by SBI firmware. For instance, if we assume that unused
timer counter with index 1 is not reported, then the range is correct
and larger array needs to be allocated.

This is not the case though since SBI firmware is supposed to report the
total number of firmware and hardware counters including special or
unused ones like the timer counter. So just fix the range in for-loop.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
---
 drivers/perf/riscv_pmu_sbi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index dca3537a8dcc..a5d25b51beac 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -457,7 +457,7 @@ static int pmu_sbi_get_ctrinfo(int nctr)
 	if (!pmu_ctr_list)
 		return -ENOMEM;
 
-	for (i = 0; i <= nctr; i++) {
+	for (i = 0; i < nctr; 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 */
-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] perf: RISC-V: fix access beyond allocated array

[Sergey Matyukevich]

> The root cause could be related to the interpretation of the number of
> counters reported by SBI firmware. For instance, if we assume that unused
> timer counter with index 1 is not reported, then the range is correct
> and larger array needs to be allocated.
> 
> This is not the case though since SBI firmware is supposed to report the
> total number of firmware and hardware counters including special or
> unused ones like the timer counter. So just fix the range in for-loop.

Hi Atish,

Just a friendly reminder.

Regards,
Sergey

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] perf: RISC-V: fix access beyond allocated array

[Atish Patra]

On Sun, Jul 10, 2022 at 11:43 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> > The root cause could be related to the interpretation of the number of
> > counters reported by SBI firmware. For instance, if we assume that unused
> > timer counter with index 1 is not reported, then the range is correct
> > and larger array needs to be allocated.
> >
> > This is not the case though since SBI firmware is supposed to report the
> > total number of firmware and hardware counters including special or
> > unused ones like the timer counter. So just fix the range in for-loop.
>
> Hi Atish,
>
> Just a friendly reminder.
>

Sorry. I thought I already sent the RB tag.

> Regards,
> Sergey



-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 1/2] perf: RISC-V: fix access beyond allocated array

[Atish Patra]

On Fri, Jun 24, 2022 at 6:59 AM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
>
> The root cause could be related to the interpretation of the number of
> counters reported by SBI firmware. For instance, if we assume that unused
> timer counter with index 1 is not reported, then the range is correct
> and larger array needs to be allocated.
>
> This is not the case though since SBI firmware is supposed to report the
> total number of firmware and hardware counters including special or
> unused ones like the timer counter. So just fix the range in for-loop.
>
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> ---
>  drivers/perf/riscv_pmu_sbi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index dca3537a8dcc..a5d25b51beac 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -457,7 +457,7 @@ static int pmu_sbi_get_ctrinfo(int nctr)
>         if (!pmu_ctr_list)
>                 return -ENOMEM;
>
> -       for (i = 0; i <= nctr; i++) {
> +       for (i = 0; i < nctr; 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 */
> --
> 2.36.1
>
LGTM.

Reviewed-by: Atish Patra <atishp@rivosinc.com>

-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2 2/2] perf: RISC-V: exclude invalid pmu counters from SBI calls

[Sergey Matyukevich]

From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

SBI firmware may not provide information for some counters in response
to SBI_EXT_PMU_COUNTER_GET_INFO call. Exclude such counters from the
subsequent SBI requests. For this purpose use global mask to keep track
of fully specified counters.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
---
 drivers/perf/riscv_pmu_legacy.c |  4 ++--
 drivers/perf/riscv_pmu_sbi.c    | 22 +++++++++++++---------
 include/linux/perf/riscv_pmu.h  |  2 +-
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/perf/riscv_pmu_legacy.c b/drivers/perf/riscv_pmu_legacy.c
index 342778782359..7d7131c47bc0 100644
--- a/drivers/perf/riscv_pmu_legacy.c
+++ b/drivers/perf/riscv_pmu_legacy.c
@@ -14,7 +14,6 @@
 
 #define RISCV_PMU_LEGACY_CYCLE		0
 #define RISCV_PMU_LEGACY_INSTRET	1
-#define RISCV_PMU_LEGACY_NUM_CTR	2
 
 static bool pmu_init_done;
 
@@ -83,7 +82,8 @@ static void pmu_legacy_init(struct riscv_pmu *pmu)
 {
 	pr_info("Legacy PMU implementation is available\n");
 
-	pmu->num_counters = RISCV_PMU_LEGACY_NUM_CTR;
+	pmu->cmask = BIT(RISCV_PMU_LEGACY_CYCLE) |
+		BIT(RISCV_PMU_LEGACY_INSTRET);
 	pmu->ctr_start = pmu_legacy_ctr_start;
 	pmu->ctr_stop = NULL;
 	pmu->event_map = pmu_legacy_event_map;
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index a5d25b51beac..70a3db29427a 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -265,7 +265,6 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
 	struct sbiret ret;
 	int idx;
 	uint64_t cbase = 0;
-	uint64_t cmask = GENMASK_ULL(rvpmu->num_counters - 1, 0);
 	unsigned long cflags = 0;
 
 	if (event->attr.exclude_kernel)
@@ -274,8 +273,8 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
 		cflags |= SBI_PMU_CFG_FLAG_SET_UINH;
 
 	/* retrieve the available counter index */
-	ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, cbase, cmask,
-			cflags, hwc->event_base, hwc->config, 0);
+	ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH, cbase,
+			rvpmu->cmask, cflags, hwc->event_base, hwc->config, 0);
 	if (ret.error) {
 		pr_debug("Not able to find a counter for event %lx config %llx\n",
 			hwc->event_base, hwc->config);
@@ -283,7 +282,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
 	}
 
 	idx = ret.value;
-	if (idx >= rvpmu->num_counters || !pmu_ctr_list[idx].value)
+	if (!test_bit(idx, &rvpmu->cmask) || !pmu_ctr_list[idx].value)
 		return -ENOENT;
 
 	/* Additional sanity check for the counter id */
@@ -447,7 +446,7 @@ static int pmu_sbi_find_num_ctrs(void)
 		return sbi_err_map_linux_errno(ret.error);
 }
 
-static int pmu_sbi_get_ctrinfo(int nctr)
+static int pmu_sbi_get_ctrinfo(int nctr, unsigned long *mask)
 {
 	struct sbiret ret;
 	int i, num_hw_ctr = 0, num_fw_ctr = 0;
@@ -462,6 +461,9 @@ static int pmu_sbi_get_ctrinfo(int nctr)
 		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++;
@@ -482,7 +484,7 @@ static inline void pmu_sbi_stop_all(struct riscv_pmu *pmu)
 	 * which may include counters that are not enabled yet.
 	 */
 	sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP,
-		  0, GENMASK_ULL(pmu->num_counters - 1, 0), 0, 0, 0, 0);
+		  0, pmu->cmask, 0, 0, 0, 0);
 }
 
 static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
@@ -696,8 +698,9 @@ static int pmu_sbi_setup_irqs(struct riscv_pmu *pmu, struct platform_device *pde
 static int pmu_sbi_device_probe(struct platform_device *pdev)
 {
 	struct riscv_pmu *pmu = NULL;
-	int num_counters;
+	unsigned long cmask = 0;
 	int ret = -ENODEV;
+	int num_counters;
 
 	pr_info("SBI PMU extension is available\n");
 	pmu = riscv_pmu_alloc();
@@ -711,7 +714,7 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
 	}
 
 	/* cache all the information about counters now */
-	if (pmu_sbi_get_ctrinfo(num_counters))
+	if (pmu_sbi_get_ctrinfo(num_counters, &cmask))
 		goto out_free;
 
 	ret = pmu_sbi_setup_irqs(pmu, pdev);
@@ -720,7 +723,8 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
 		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
 		pmu->pmu.capabilities |= PERF_PMU_CAP_NO_EXCLUDE;
 	}
-	pmu->num_counters = num_counters;
+
+	pmu->cmask = cmask;
 	pmu->ctr_start = pmu_sbi_ctr_start;
 	pmu->ctr_stop = pmu_sbi_ctr_stop;
 	pmu->event_map = pmu_sbi_event_map;
diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h
index 46f9b6fe306e..60f4373fe581 100644
--- a/include/linux/perf/riscv_pmu.h
+++ b/include/linux/perf/riscv_pmu.h
@@ -45,7 +45,7 @@ struct riscv_pmu {
 
 	irqreturn_t	(*handle_irq)(int irq_num, void *dev);
 
-	int		num_counters;
+	unsigned long	cmask;
 	u64		(*ctr_read)(struct perf_event *event);
 	int		(*ctr_get_idx)(struct perf_event *event);
 	int		(*ctr_get_width)(int idx);
-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2 0/2] perf: RISC-V: fix access beyond allocated array

[Sergey Matyukevich]

Hi Atish, Anup

> These patches suggest some fixes and cleanups for the handling of pmu
> counters. The first patch fixes access beyond the allocated pmu_ctr_list
> array. The second patch fixes the counters mask sent to SBI firmware: it
> excludes counters that were not fully specified by SBI firmware on init.
> 
> Initial attempt to fix access to the highest available has been reworked.
> Now it is handled in the OpenSBI, see the following patch:
> - https://patchwork.ozlabs.org/project/opensbi/patch/20220624110330.452640-1-geomatsi@gmail.com/
> 
> Regards,
> Sergey
> 
> v1 -> v2:
> - drop changes for access to the highest available counter as they are
>   now handled on the OpenSBI side
> - drop switch to IDR: in fact there is no need to handle non-contiguous
>   counter ranges
> 
> Sergey Matyukevich (2):
>   perf: RISC-V: fix access beyond allocated array
>   perf: RISC-V: exclude invalid pmu counters from SBI calls
> 
>  drivers/perf/riscv_pmu_legacy.c |  4 ++--
>  drivers/perf/riscv_pmu_sbi.c    | 24 ++++++++++++++----------
>  include/linux/perf/riscv_pmu.h  |  2 +-
>  3 files changed, 17 insertions(+), 13 deletions(-)

Friendly ping.  I have already received RB tag from Atish for the first
patch. Do you have any concerns with the second one ?

Besides, what is the appropriate merge path for RISC-V PMU driver ?
Are they usually merged via risc-v kernel trees ?

Regards,
Sergey

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv