Re: [RFC PATCH 0/2] target/riscv: Add support for machine specific pmu's events

[Alistair Francis]

On Wed, Sep 11, 2024 at 3:49 AM Alexei Filippov
<alexei.filippov@syntacore.com> wrote:
>
> Following original patch [1] here's a patch with support of machine
> specific pmu events and PoC with initial support for sifive_u's HPM.

Thanks for the patch.

I'm hesitate to support these callback functions as I feel they
(callbacks in the CPU to the machine in general) are clunky.

I think the cover letter, code and commit messages need more details here.

First can you link to the exact spec you are trying to implement
(RISC-V has a habit of creating multiple "ratified" specs that are all
incompatible). It's really handy to point to the exact PDF in the
cover letter or commit message to just be really clear what you are
supporting.

Secondly, can you describe why this is useful? What is the point of
machine specific PMU events? Why do we want to support this in QEMU?

The callbacks should also have some documentation in the code base so
others can implement the functionality.

It might also be helpful to split this patch up a little bit more. A
quick read through and it seems like the patches could be a little
smaller, making it easier to review.

Finally, for the next version CC @Atish Patra  who has ended up being
the PMU person :)

Alistair

>
> == Test scenarios ==
>
> So, I tested this patches on current Linux master with perf.
> something like `perf stat -e branch-misses perf bench mem memcpy` works
> just fine, also 'perf record -e branch-misses perf bench mem memcpy'
> collect samples just fine and `perf report` works.
>
> == ToDos / Limitations ==
>
> Second patch is only inital sifive_u's HPM support, without any
> filtering, events combining features or differrent counting
> algorithm for different events. There are also no tests, but if you
> have any suggestions about where I need to look to implement them, please
> point me to.
>
> == Changes since original patch ==
>
> - Rebased to current master
>
> [1] 
> https://lore.kernel.org/all/20240625144643.34733-1-alexei.filippov@syntacore.com/
>
> Alexei Filippov (2):
>   target/riscv: Add support for machine specific pmu's events
>   hw/riscv/sifive_u.c: Add initial HPM support
>
>  hw/misc/meson.build            |   1 +
>  hw/misc/sifive_u_pmu.c         | 384 +++++++++++++++++++++++++++++++++
>  hw/riscv/sifive_u.c            |  14 ++
>  include/hw/misc/sifive_u_pmu.h |  24 +++
>  target/riscv/cpu.c             |  20 +-
>  target/riscv/cpu.h             |   9 +
>  target/riscv/csr.c             |  93 +++++---
>  target/riscv/pmu.c             | 138 ++++++------
>  target/riscv/pmu.h             |  19 +-
>  9 files changed, 599 insertions(+), 103 deletions(-)
>  create mode 100644 hw/misc/sifive_u_pmu.c
>  create mode 100644 include/hw/misc/sifive_u_pmu.h
>
> --
> 2.34.1
>
>