perf: riscv: Fix selecting counters in legacy mode

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

Posted by Shifrin Dmitry 1 year, 6 months ago

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

Posted by Samuel Holland 1 year, 6 months ago

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

Posted by Dmitry Shifrin 1 year, 6 months ago

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;
>