[PATCH] perf: riscv: Fix selecting counters in legacy mode

[PATCH] perf: riscv: Fix selecting counters in legacy mode

[Shifrin Dmitry]

It is required to check event type before checking event config.
Events with the different types can have the same config.
This check is missed for legacy mode code

For such perf usage:
    sysctl -w kernel.perf_user_access=2
    perf stat -e cycles,L1-dcache-loads --
driver will try to force both events to CYCLE counter.

This commit implements event type check before forcing
events on the special counters.

Signed-off-by: Shifrin Dmitry <dmitry.shifrin@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 36d128ff166f..bf14ab282e11 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
 	 * but not in the user access mode as we want to use the other counters
 	 * that support sampling/filtering.
 	 */
-	if (hwc->flags & PERF_EVENT_FLAG_LEGACY) {
+	if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) {
 		if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) {
 			cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH;
 			cmask = 1;
-- 
2.34.1

Re: [PATCH] perf: riscv: Fix selecting counters in legacy mode

[Samuel Holland]

Hi Dmitry,

On 2024-07-29 7:58 AM, Shifrin Dmitry wrote:
> It is required to check event type before checking event config.
> Events with the different types can have the same config.
> This check is missed for legacy mode code
> 
> For such perf usage:
>     sysctl -w kernel.perf_user_access=2
>     perf stat -e cycles,L1-dcache-loads --
> driver will try to force both events to CYCLE counter.
> 
> This commit implements event type check before forcing
> events on the special counters.

It looks like pmu_sbi_event_mapped() and pmu_sbi_event_mapped() have a similar
bug where they do not check event->attr.type.

Regards,
Samuel

> 
> Signed-off-by: Shifrin Dmitry <dmitry.shifrin@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 36d128ff166f..bf14ab282e11 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
>  	 * but not in the user access mode as we want to use the other counters
>  	 * that support sampling/filtering.
>  	 */
> -	if (hwc->flags & PERF_EVENT_FLAG_LEGACY) {
> +	if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) {
>  		if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) {
>  			cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH;
>  			cmask = 1;

Re: [PATCH] perf: riscv: Fix selecting counters in legacy mode

[Dmitry Shifrin]

Hi Samuel,

On Wed, 2024-07-31 at 02:37 -0500, Samuel Holland wrote:
> Hi Dmitry,
> 
> On 2024-07-29 7:58 AM, Shifrin Dmitry wrote:
> > It is required to check event type before checking event config.
> > Events with the different types can have the same config.
> > This check is missed for legacy mode code
> > 
> > For such perf usage:
> >     sysctl -w kernel.perf_user_access=2
> >     perf stat -e cycles,L1-dcache-loads --
> > driver will try to force both events to CYCLE counter.
> > 
> > This commit implements event type check before forcing
> > events on the special counters.
> 
> It looks like pmu_sbi_event_mapped() and pmu_sbi_event_mapped() have a similar
> bug where they do not check event->attr.type.

I am not sure that pmu_sbi_event_unmapped() and pmu_sbi_event_mapped() should be changed in the same way.
These functions in legacy mode just set/clear PERF_EVENT_FLAG_USER_READ_CNT for legacy events without any
mapping/unmapping. However in pmu_sbi_starting_cpu() CYCLE, TIME and INSTRET counters are always mapped to
user space in legacy mode. So it looks like in legacy mode we can just use

: if (event->hw.flags & PERF_EVENT_FLAG_LEGACY)
:	return;

Thanks, Dmitry

> 
> Regards,
> Samuel
> 
> > 
> > Signed-off-by: Shifrin Dmitry <dmitry.shifrin@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 36d128ff166f..bf14ab282e11 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -416,7 +416,7 @@ static int pmu_sbi_ctr_get_idx(struct perf_event *event)
> >          * but not in the user access mode as we want to use the other counters
> >          * that support sampling/filtering.
> >          */
> > -       if (hwc->flags & PERF_EVENT_FLAG_LEGACY) {
> > +       if ((hwc->flags & PERF_EVENT_FLAG_LEGACY) && (event->attr.type == PERF_TYPE_HARDWARE)) {
> >                 if (event->attr.config == PERF_COUNT_HW_CPU_CYCLES) {
> >                         cflags |= SBI_PMU_CFG_FLAG_SKIP_MATCH;
> >                         cmask = 1;
> 

Re: [PATCH] perf: riscv: Fix selecting counters in legacy mode

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Mon, 29 Jul 2024 15:58:58 +0300 you wrote:
> It is required to check event type before checking event config.
> Events with the different types can have the same config.
> This check is missed for legacy mode code
> 
> For such perf usage:
>     sysctl -w kernel.perf_user_access=2
>     perf stat -e cycles,L1-dcache-loads --
> driver will try to force both events to CYCLE counter.
> 
> [...]

Here is the summary with links:
  - perf: riscv: Fix selecting counters in legacy mode
    https://git.kernel.org/riscv/c/941a8e9b7a86

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html