[PATCH 0/3] Assorted fixes for PMU

Atish Patra posted 3 patches 2 weeks, 4 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240429-countinhibit._5Ffix-v1-0-802ec1e99133@rivosinc.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bmeng.cn@gmail.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
target/riscv/cpu.h     |   1 -
target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
target/riscv/machine.c |   1 -
3 files changed, 68 insertions(+), 45 deletions(-)
[PATCH 0/3] Assorted fixes for PMU
Posted by Atish Patra 2 weeks, 4 days ago
This series contains few miscallenous fixes related to hpmcounters
and related code. The first patch fixes an issue with cycle/instret
counters overcouting while the remaining two are more for specification
compliance.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (3):
      target/riscv: Save counter values during countinhibit update
      target/riscv: Enforce WARL behavior for scounteren/hcounteren
      target/riscv: Fix the predicate functions for mhpmeventhX CSRs

 target/riscv/cpu.h     |   1 -
 target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
 target/riscv/machine.c |   1 -
 3 files changed, 68 insertions(+), 45 deletions(-)
---
base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
change-id: 20240428-countinhibit_fix-c6a1c11f4375
--
Regards,
Atish patra
Re: [PATCH 0/3] Assorted fixes for PMU
Posted by Peter Maydell 3 days, 19 hours ago
On Mon, 29 Apr 2024 at 20:29, Atish Patra <atishp@rivosinc.com> wrote:
>
> This series contains few miscallenous fixes related to hpmcounters
> and related code. The first patch fixes an issue with cycle/instret
> counters overcouting while the remaining two are more for specification
> compliance.

I've noticed a number of riscv patchsets from various people
recently where the subject line for the cover letter doesn't
include any indication that it's a riscv related patchset.
For instance this one is just "Assorted fixes for PMU", which
could easily be an Arm PMU series. Could you all try to make sure
that cover letter subject lines indicate the architecture or
other subcomponent they affect, please? This helps people who
are skimming over the mailing list looking for patches relevant
to them.

thanks
-- PMM
Re: [PATCH 0/3] Assorted fixes for PMU
Posted by Atish Patra 3 days, 11 hours ago
On Tue, May 14, 2024 at 2:16 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 29 Apr 2024 at 20:29, Atish Patra <atishp@rivosinc.com> wrote:
> >
> > This series contains few miscallenous fixes related to hpmcounters
> > and related code. The first patch fixes an issue with cycle/instret
> > counters overcouting while the remaining two are more for specification
> > compliance.
>
> I've noticed a number of riscv patchsets from various people
> recently where the subject line for the cover letter doesn't
> include any indication that it's a riscv related patchset.
> For instance this one is just "Assorted fixes for PMU", which
> could easily be an Arm PMU series. Could you all try to make sure
> that cover letter subject lines indicate the architecture or
> other subcomponent they affect, please? This helps people who
> are skimming over the mailing list looking for patches relevant
> to them.
>

Makes sense. I will include RISC-V in the series cover letter as well.
Thanks for the feedback.

> thanks
> -- PMM
>


-- 
Regards,
Atish
Re: [PATCH 0/3] Assorted fixes for PMU
Posted by Alistair Francis 3 days, 22 hours ago
On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> This series contains few miscallenous fixes related to hpmcounters
> and related code. The first patch fixes an issue with cycle/instret
> counters overcouting while the remaining two are more for specification
> compliance.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> Atish Patra (3):
>       target/riscv: Save counter values during countinhibit update
>       target/riscv: Enforce WARL behavior for scounteren/hcounteren
>       target/riscv: Fix the predicate functions for mhpmeventhX CSRs

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu.h     |   1 -
>  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
>  target/riscv/machine.c |   1 -
>  3 files changed, 68 insertions(+), 45 deletions(-)
> ---
> base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> change-id: 20240428-countinhibit_fix-c6a1c11f4375
> --
> Regards,
> Atish patra
>
>
Re: [PATCH 0/3] Assorted fixes for PMU
Posted by Atish Kumar Patra 3 days, 21 hours ago
On Mon, May 13, 2024 at 11:29 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > This series contains few miscallenous fixes related to hpmcounters
> > and related code. The first patch fixes an issue with cycle/instret
> > counters overcouting while the remaining two are more for specification
> > compliance.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> > Atish Patra (3):
> >       target/riscv: Save counter values during countinhibit update
> >       target/riscv: Enforce WARL behavior for scounteren/hcounteren
> >       target/riscv: Fix the predicate functions for mhpmeventhX CSRs
>
> Thanks!
>
> Applied to riscv-to-apply.next
>

Hi Alistair,
Thanks for your review. But the patch 1 had some comments about
vmstate which needs updating.
We also found a few more fixes that I was planning to include in v2.

I can send a separate fixes series on top riscv-to-apply.next or this
series can be dropped for the time being.
You can queue it v2 later. Let me know what you prefer.


> Alistair
>
> >
> >  target/riscv/cpu.h     |   1 -
> >  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
> >  target/riscv/machine.c |   1 -
> >  3 files changed, 68 insertions(+), 45 deletions(-)
> > ---
> > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> > change-id: 20240428-countinhibit_fix-c6a1c11f4375
> > --
> > Regards,
> > Atish patra
> >
> >
Re: [PATCH 0/3] Assorted fixes for PMU
Posted by Alistair Francis 3 days, 18 hours ago
On Tue, May 14, 2024 at 5:15 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Mon, May 13, 2024 at 11:29 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
> > >
> > > This series contains few miscallenous fixes related to hpmcounters
> > > and related code. The first patch fixes an issue with cycle/instret
> > > counters overcouting while the remaining two are more for specification
> > > compliance.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > > Atish Patra (3):
> > >       target/riscv: Save counter values during countinhibit update
> > >       target/riscv: Enforce WARL behavior for scounteren/hcounteren
> > >       target/riscv: Fix the predicate functions for mhpmeventhX CSRs
> >
> > Thanks!
> >
> > Applied to riscv-to-apply.next
> >
>
> Hi Alistair,
> Thanks for your review. But the patch 1 had some comments about
> vmstate which needs updating.

Ah, I did read the comments but forgot that you were going to bump the version.

> We also found a few more fixes that I was planning to include in v2.

I found that patch `target/riscv: Save counter values during
countinhibit update` gives me this error as well

../target/riscv/csr.c: In function ‘write_mcountinhibit’:
../target/riscv/csr.c:1981:44: error: too few arguments to function ‘get_ticks’
1981 |                 counter->mhpmcounter_val = get_ticks(false) -
     |                                            ^~~~~~~~~
../target/riscv/csr.c:765:21: note: declared here
 765 | static target_ulong get_ticks(bool shift, bool instructions)
     |                     ^~~~~~~~~
../target/riscv/csr.c:1985:49: error: too few arguments to function ‘get_ticks’
1985 |                     counter->mhpmcounterh_val = get_ticks(false) -
     |                                                 ^~~~~~~~~
../target/riscv/csr.c:765:21: note: declared here
 765 | static target_ulong get_ticks(bool shift, bool instructions)
     |                     ^~~~~~~~~



>
> I can send a separate fixes series on top riscv-to-apply.next or this
> series can be dropped for the time being.

I'm going to drop it due to the build error above

Alistair

> You can queue it v2 later. Let me know what you prefer.
>
>
> > Alistair
> >
> > >
> > >  target/riscv/cpu.h     |   1 -
> > >  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
> > >  target/riscv/machine.c |   1 -
> > >  3 files changed, 68 insertions(+), 45 deletions(-)
> > > ---
> > > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> > > change-id: 20240428-countinhibit_fix-c6a1c11f4375
> > > --
> > > Regards,
> > > Atish patra
> > >
> > >
Re: [PATCH 0/3] Assorted fixes for PMU
Posted by Atish Patra 3 days, 11 hours ago
On Tue, May 14, 2024 at 3:18 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, May 14, 2024 at 5:15 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Mon, May 13, 2024 at 11:29 PM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Tue, Apr 30, 2024 at 5:29 AM Atish Patra <atishp@rivosinc.com> wrote:
> > > >
> > > > This series contains few miscallenous fixes related to hpmcounters
> > > > and related code. The first patch fixes an issue with cycle/instret
> > > > counters overcouting while the remaining two are more for specification
> > > > compliance.
> > > >
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > ---
> > > > Atish Patra (3):
> > > >       target/riscv: Save counter values during countinhibit update
> > > >       target/riscv: Enforce WARL behavior for scounteren/hcounteren
> > > >       target/riscv: Fix the predicate functions for mhpmeventhX CSRs
> > >
> > > Thanks!
> > >
> > > Applied to riscv-to-apply.next
> > >
> >
> > Hi Alistair,
> > Thanks for your review. But the patch 1 had some comments about
> > vmstate which needs updating.
>
> Ah, I did read the comments but forgot that you were going to bump the version.
>
> > We also found a few more fixes that I was planning to include in v2.
>
> I found that patch `target/riscv: Save counter values during
> countinhibit update` gives me this error as well
>
> ../target/riscv/csr.c: In function ‘write_mcountinhibit’:
> ../target/riscv/csr.c:1981:44: error: too few arguments to function ‘get_ticks’
> 1981 |                 counter->mhpmcounter_val = get_ticks(false) -
>      |                                            ^~~~~~~~~
> ../target/riscv/csr.c:765:21: note: declared here
>  765 | static target_ulong get_ticks(bool shift, bool instructions)
>      |                     ^~~~~~~~~
> ../target/riscv/csr.c:1985:49: error: too few arguments to function ‘get_ticks’
> 1985 |                     counter->mhpmcounterh_val = get_ticks(false) -
>      |                                                 ^~~~~~~~~
> ../target/riscv/csr.c:765:21: note: declared here
>  765 | static target_ulong get_ticks(bool shift, bool instructions)
>      |                     ^~~~~~~~~
>

Yeah. Clement's patch got in. I will rebase and update the series
based on the riscv-to-apply.next.

>
>
> >
> > I can send a separate fixes series on top riscv-to-apply.next or this
> > series can be dropped for the time being.
>
> I'm going to drop it due to the build error above
>
> Alistair
>
> > You can queue it v2 later. Let me know what you prefer.
> >
> >
> > > Alistair
> > >
> > > >
> > > >  target/riscv/cpu.h     |   1 -
> > > >  target/riscv/csr.c     | 111 ++++++++++++++++++++++++++++++-------------------
> > > >  target/riscv/machine.c |   1 -
> > > >  3 files changed, 68 insertions(+), 45 deletions(-)
> > > > ---
> > > > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a
> > > > change-id: 20240428-countinhibit_fix-c6a1c11f4375
> > > > --
> > > > Regards,
> > > > Atish patra
> > > >
> > > >
>


-- 
Regards,
Atish