perf: arm_pmuv3: Fix chained event check for cycle counter

[PATCH] perf: arm_pmuv3: Fix chained event check for cycle counter

Posted by Rob Herring (Arm) 1 year, 5 months ago

Since commit b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to
counter remapping"), armv8pmu_event_is_chained() is incorrectly
returning that the cycle counter is chained, but the cycle counter has
always been 64-bit. The result is trying to configure counter #30 which
typically doesn't exist.

As ARMV8_PMU_MAX_GENERAL_COUNTERS is the number of counters (31), the
comparison to the counter index needs to be '<' rather than '<='.

Fixes: b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to counter remapping")
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/perf/arm_pmuv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
index 3b3a3334cc3f..0e22c68fb804 100644
--- a/drivers/perf/arm_pmuv3.c
+++ b/drivers/perf/arm_pmuv3.c
@@ -482,7 +482,7 @@ static bool armv8pmu_event_is_chained(struct perf_event *event)
 	return !armv8pmu_event_has_user_read(event) &&
 	       armv8pmu_event_is_64bit(event) &&
 	       !armv8pmu_has_long_event(cpu_pmu) &&
-	       (idx <= ARMV8_PMU_MAX_GENERAL_COUNTERS);
+	       (idx < ARMV8_PMU_MAX_GENERAL_COUNTERS);
 }
 
 /*
-- 
2.43.0

Re: [PATCH] perf: arm_pmuv3: Fix chained event check for cycle counter

Posted by Will Deacon 1 year, 4 months ago

On Wed, Jul 10, 2024 at 12:23:56PM -0600, Rob Herring (Arm) wrote:
> Since commit b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to
> counter remapping"), armv8pmu_event_is_chained() is incorrectly
> returning that the cycle counter is chained, but the cycle counter has
> always been 64-bit. The result is trying to configure counter #30 which
> typically doesn't exist.
> 
> As ARMV8_PMU_MAX_GENERAL_COUNTERS is the number of counters (31), the
> comparison to the counter index needs to be '<' rather than '<='.
> 
> Fixes: b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to counter remapping")
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/perf/arm_pmuv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 3b3a3334cc3f..0e22c68fb804 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -482,7 +482,7 @@ static bool armv8pmu_event_is_chained(struct perf_event *event)
>  	return !armv8pmu_event_has_user_read(event) &&
>  	       armv8pmu_event_is_64bit(event) &&
>  	       !armv8pmu_has_long_event(cpu_pmu) &&
> -	       (idx <= ARMV8_PMU_MAX_GENERAL_COUNTERS);
> +	       (idx < ARMV8_PMU_MAX_GENERAL_COUNTERS);
>  }

Acked-by: Will Deacon <will@kernel.org>

Catalin -- please can you pick this up as a fix?

Cheers,

Will

Re: [PATCH] perf: arm_pmuv3: Fix chained event check for cycle counter

Posted by Rob Herring 1 year, 4 months ago

On Thu, Aug 1, 2024 at 5:29 AM Will Deacon <will@kernel.org> wrote:
>
> On Wed, Jul 10, 2024 at 12:23:56PM -0600, Rob Herring (Arm) wrote:
> > Since commit b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to
> > counter remapping"), armv8pmu_event_is_chained() is incorrectly
> > returning that the cycle counter is chained, but the cycle counter has
> > always been 64-bit. The result is trying to configure counter #30 which
> > typically doesn't exist.
> >
> > As ARMV8_PMU_MAX_GENERAL_COUNTERS is the number of counters (31), the
> > comparison to the counter index needs to be '<' rather than '<='.
> >
> > Fixes: b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to counter remapping")
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  drivers/perf/arm_pmuv3.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> > index 3b3a3334cc3f..0e22c68fb804 100644
> > --- a/drivers/perf/arm_pmuv3.c
> > +++ b/drivers/perf/arm_pmuv3.c
> > @@ -482,7 +482,7 @@ static bool armv8pmu_event_is_chained(struct perf_event *event)
> >       return !armv8pmu_event_has_user_read(event) &&
> >              armv8pmu_event_is_64bit(event) &&
> >              !armv8pmu_has_long_event(cpu_pmu) &&
> > -            (idx <= ARMV8_PMU_MAX_GENERAL_COUNTERS);
> > +            (idx < ARMV8_PMU_MAX_GENERAL_COUNTERS);
> >  }
>
> Acked-by: Will Deacon <will@kernel.org>
>
> Catalin -- please can you pick this up as a fix?

No, this is the fix for what Will dropped and said to resend on the
v9.4 fixed instruction counter series. Here's the series with the fix
in it[1].

Rob

[1] https://lore.kernel.org/all/20240731-arm-pmu-3-9-icntr-v3-0-280a8d7ff465@kernel.org/

Re: [PATCH] perf: arm_pmuv3: Fix chained event check for cycle counter

Posted by Will Deacon 1 year, 4 months ago

On Thu, Aug 01, 2024 at 11:19:26AM -0600, Rob Herring wrote:
> On Thu, Aug 1, 2024 at 5:29 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Wed, Jul 10, 2024 at 12:23:56PM -0600, Rob Herring (Arm) wrote:
> > > Since commit b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to
> > > counter remapping"), armv8pmu_event_is_chained() is incorrectly
> > > returning that the cycle counter is chained, but the cycle counter has
> > > always been 64-bit. The result is trying to configure counter #30 which
> > > typically doesn't exist.
> > >
> > > As ARMV8_PMU_MAX_GENERAL_COUNTERS is the number of counters (31), the
> > > comparison to the counter index needs to be '<' rather than '<='.
> > >
> > > Fixes: b7e89b0f5bd7 ("perf: arm_pmu: Remove event index to counter remapping")
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > >  drivers/perf/arm_pmuv3.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> > > index 3b3a3334cc3f..0e22c68fb804 100644
> > > --- a/drivers/perf/arm_pmuv3.c
> > > +++ b/drivers/perf/arm_pmuv3.c
> > > @@ -482,7 +482,7 @@ static bool armv8pmu_event_is_chained(struct perf_event *event)
> > >       return !armv8pmu_event_has_user_read(event) &&
> > >              armv8pmu_event_is_64bit(event) &&
> > >              !armv8pmu_has_long_event(cpu_pmu) &&
> > > -            (idx <= ARMV8_PMU_MAX_GENERAL_COUNTERS);
> > > +            (idx < ARMV8_PMU_MAX_GENERAL_COUNTERS);
> > >  }
> >
> > Acked-by: Will Deacon <will@kernel.org>
> >
> > Catalin -- please can you pick this up as a fix?
> 
> No, this is the fix for what Will dropped and said to resend on the
> v9.4 fixed instruction counter series. Here's the series with the fix
> in it[1].

Urgh, sorry about that, and thanks for pointing it out.

I'd initially marked this as read, but then I was pointed at it the
other day and thought I'd missed a fix since "git show b7e89b0f5bd7"
still shows the patch, despite pruning the remotes.

I'll queue up the other series for 6.12.

Will