[v3] Assorted fixes in RISC-V PMU driver

[PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Atish Patra 1 year, 5 months ago

From: Samuel Holland <samuel.holland@sifive.com>

Currently, we stop all the counters while a new cpu is brought online.
However, the hpmevent to counter mappings are not reset. The firmware may
have some stale encoding in their mapping structure which may lead to
undesirable results. We have not encountered such scenario though.

Signed-off-by: Atish Patra <atishp@rivosinc.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 a2e4005e1fd0..94bc369a3454 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -762,7 +762,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, pmu->cmask, 0, 0, 0, 0);
+		  0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
 }
 
 static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)

-- 
2.34.1

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Samuel Holland 1 year, 5 months ago

On 2024-06-26 2:23 AM, Atish Patra wrote:
> From: Samuel Holland <samuel.holland@sifive.com>
> 
> Currently, we stop all the counters while a new cpu is brought online.
> However, the hpmevent to counter mappings are not reset. The firmware may
> have some stale encoding in their mapping structure which may lead to
> undesirable results. We have not encountered such scenario though.
> 

This needs:

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

otherwise your commit message looks fine to me.

> Signed-off-by: Atish Patra <atishp@rivosinc.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 a2e4005e1fd0..94bc369a3454 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -762,7 +762,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, pmu->cmask, 0, 0, 0, 0);
> +		  0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
>  }
>  
>  static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
>

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Atish Kumar Patra 1 year, 5 months ago

On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2024-06-26 2:23 AM, Atish Patra wrote:
> > From: Samuel Holland <samuel.holland@sifive.com>
> >
> > Currently, we stop all the counters while a new cpu is brought online.
> > However, the hpmevent to counter mappings are not reset. The firmware may
> > have some stale encoding in their mapping structure which may lead to
> > undesirable results. We have not encountered such scenario though.
> >
>
> This needs:
>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>

Oops. Sorry I missed that.

@Alexandre Ghiti @Palmer Dabbelt : Can you add that while picking up
the patch or should I respin a v4 ?

> otherwise your commit message looks fine to me.
>
> > Signed-off-by: Atish Patra <atishp@rivosinc.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 a2e4005e1fd0..94bc369a3454 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -762,7 +762,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, pmu->cmask, 0, 0, 0, 0);
> > +               0, pmu->cmask, SBI_PMU_STOP_FLAG_RESET, 0, 0, 0);
> >  }
> >
> >  static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
> >
>

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Conor Dooley 1 year, 5 months ago

On Wed, Jun 26, 2024 at 09:18:46AM -0700, Atish Kumar Patra wrote:
> On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
> <samuel.holland@sifive.com> wrote:
> >
> > On 2024-06-26 2:23 AM, Atish Patra wrote:
> > > From: Samuel Holland <samuel.holland@sifive.com>
> > >
> > > Currently, we stop all the counters while a new cpu is brought online.
> > > However, the hpmevent to counter mappings are not reset. The firmware may
> > > have some stale encoding in their mapping structure which may lead to
> > > undesirable results. We have not encountered such scenario though.
> > >
> >
> > This needs:
> >
> > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> >
> 
> Oops. Sorry I missed that.
> 
> @Alexandre Ghiti

What's Alex going to be able to do?

> @Palmer Dabbelt : Can you add that while picking up
> the patch or should I respin a v4 ?

b4 should pick the signoff up though. "perf: RISC-V: Check standard
event availability" seems to be missing your signoff though...

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Conor Dooley 1 year, 5 months ago

On Wed, Jun 26, 2024 at 05:37:07PM +0100, Conor Dooley wrote:
> On Wed, Jun 26, 2024 at 09:18:46AM -0700, Atish Kumar Patra wrote:
> > On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
> > <samuel.holland@sifive.com> wrote:
> > >
> > > On 2024-06-26 2:23 AM, Atish Patra wrote:
> > > > From: Samuel Holland <samuel.holland@sifive.com>
> > > >
> > > > Currently, we stop all the counters while a new cpu is brought online.
> > > > However, the hpmevent to counter mappings are not reset. The firmware may
> > > > have some stale encoding in their mapping structure which may lead to
> > > > undesirable results. We have not encountered such scenario though.
> > > >
> > >
> > > This needs:
> > >
> > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > >
> > 
> > Oops. Sorry I missed that.
> > 
> > @Alexandre Ghiti
> 
> What's Alex going to be able to do?
> 
> > @Palmer Dabbelt : Can you add that while picking up
> > the patch or should I respin a v4 ?
> 
> b4 should pick the signoff up though. "perf: RISC-V: Check standard
> event availability" seems to be missing your signoff though...

Huh, this doesn't really make sense. I meant:
	b4 should pick the signoff up, though "perf: RISC-V: Check standard
	event availability" seems to be missing your signoff...

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Atish Kumar Patra 1 year, 5 months ago

On Wed, Jun 26, 2024 at 9:39 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Jun 26, 2024 at 05:37:07PM +0100, Conor Dooley wrote:
> > On Wed, Jun 26, 2024 at 09:18:46AM -0700, Atish Kumar Patra wrote:
> > > On Wed, Jun 26, 2024 at 6:24 AM Samuel Holland
> > > <samuel.holland@sifive.com> wrote:
> > > >
> > > > On 2024-06-26 2:23 AM, Atish Patra wrote:
> > > > > From: Samuel Holland <samuel.holland@sifive.com>
> > > > >
> > > > > Currently, we stop all the counters while a new cpu is brought online.
> > > > > However, the hpmevent to counter mappings are not reset. The firmware may
> > > > > have some stale encoding in their mapping structure which may lead to
> > > > > undesirable results. We have not encountered such scenario though.
> > > > >
> > > >
> > > > This needs:
> > > >
> > > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > > >
> > >
> > > Oops. Sorry I missed that.
> > >
> > > @Alexandre Ghiti
> >
> > What's Alex going to be able to do?
> >

He is collecting the fixes patches in the RISC-V tree and pinged for
revision for this patch last week.

> > > @Palmer Dabbelt : Can you add that while picking up
> > > the patch or should I respin a v4 ?
> >
> > b4 should pick the signoff up though. "perf: RISC-V: Check standard
> > event availability" seems to be missing your signoff though...
>
> Huh, this doesn't really make sense. I meant:
>         b4 should pick the signoff up, though "perf: RISC-V: Check standard
>         event availability" seems to be missing your signoff...

Strange. I modified and sent the patch using b4 as well. It's missing
my sign off too.

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Conor Dooley 1 year, 5 months ago

On Wed, Jun 26, 2024 at 01:40:52PM -0700, Atish Kumar Patra wrote:

> > > > @Palmer Dabbelt : Can you add that while picking up
> > > > the patch or should I respin a v4 ?
> > >
> > > b4 should pick the signoff up though. "perf: RISC-V: Check standard
> > > event availability" seems to be missing your signoff though...
> >
> > Huh, this doesn't really make sense. I meant:
> >         b4 should pick the signoff up, though "perf: RISC-V: Check standard
> >         event availability" seems to be missing your signoff...
> 
> Strange. I modified and sent the patch using b4 as well. It's missing
> my sign off too.

`b4 shazam` should pick up trailers provided on the list, signoffs
included. `b4 shazam -s` will add yours. TBH, I am not sure why that is
not the default behaviour.

Cheers,
Conor.

Re: [PATCH v3 2/3] drivers/perf: riscv: Reset the counter to hpmevent mapping while starting cpus

Posted by Konstantin Ryabitsev 1 year, 5 months ago

On Wed, Jun 26, 2024 at 11:11:54PM GMT, Conor Dooley wrote:
> > Strange. I modified and sent the patch using b4 as well. It's missing
> > my sign off too.
> 
> `b4 shazam` should pick up trailers provided on the list, signoffs
> included. `b4 shazam -s` will add yours. TBH, I am not sure why that is
> not the default behaviour.

Some projects don't use the DCO model, so they don't require Signed-off-by
trailers.

-K