| Message ID | 20240910174747.148141-1-alexei.filippov@syntacore.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/riscv: Add support for machine specific pmu's events | expand |
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 > >
On Mon, Oct 7, 2024 at 7:52 PM Alistair Francis <alistair23@gmail.com> wrote: > > 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. > This patch is trying to implement SiFive specific event encodings. There is no standard RISC-V ISA involved here. > 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? > I happen to work on a similar implementation as well. Apologies for not seeing this patch earlier. Here is the link to the series that I have been working on to implement a similar feature. https://github.com/atishp04/qemu/tree/b4/pmu_event_machine I will send it to the mailing list tomorrow after some checkpatch fixes. Regarding the motivation, RISC-V ISA doesn't define any standard event encodings. The virt machine implemented event encodings defined in the SBI PMU extension because there was nothing else available. There is an active performance events TG who is working on defining the standard events for RISC-V but not the encodings. The goal is provide flexibility for the platforms while allowing a minimum set of events that would work across platforms. However, any platform would define their own event encodings and want to support those in their Qemu machine implementation. That's why, we should disassociate the event encodings in the pmu.c to make it more generic and usable across machines. > 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 :) > Thanks for Ccing me. I completely missed this patch earlier. Few thoughts by looking at this series. @Alexei: 1. Event encoding needs to be widened to 64 bits. That's what I tried to achieve with my implementation along with a bunch of other cleanups. 2. Why do we need machine specific counter write/read functions ? If we really need it, we should definitely have that as a separate patch as my implementation only focussed on disassociating the events and pmu implementation. Please take a look at the patches shared above or the mailing list (should land tomorrow) and let me know your thoughts. I am happy to collaborate on your patches so that we have more than just a virt machine that we can test with this series. > 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 > > > >
On 09.10.2024 06:51, Atish Kumar Patra wrote: > On Mon, Oct 7, 2024 at 7:52 PM Alistair Francis <alistair23@gmail.com> wrote: >> >> 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. >> > > This patch is trying to implement SiFive specific event encodings. > There is no standard > RISC-V ISA involved here. > >> 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? >> > > I happen to work on a similar implementation as well. Apologies for > not seeing this patch earlier. > Here is the link to the series that I have been working on to > implement a similar feature. > https://github.com/atishp04/qemu/tree/b4/pmu_event_machine > I will send it to the mailing list tomorrow after some checkpatch fixes. > > Regarding the motivation, RISC-V ISA doesn't define any standard > event encodings. > The virt machine implemented event encodings defined in the SBI PMU > extension because > there was nothing else available. There is an active performance > events TG who is working on defining > the standard events for RISC-V but not the encodings. The goal is > provide flexibility for the platforms while > allowing a minimum set of events that would work across platforms. > > However, any platform would define their own event encodings and want > to support those in their Qemu > machine implementation. That's why, we should disassociate the event > encodings in the pmu.c to make it > more generic and usable across machines. > >> 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 :) >> > > Thanks for Ccing me. I completely missed this patch earlier. Few > thoughts by looking at this series. > > @Alexei: > 1. Event encoding needs to be widened to 64 bits. That's what I tried Hi, Atish, thanks for the review. Does we really need to wide up? Can you please share why? > to achieve with my implementation > along with a bunch of other cleanups. > > 2. Why do we need machine specific counter write/read functions ? If > we really need it, we should definitely have that > as a separate patch as my implementation only focussed on > disassociating the events and pmu implementation. Ok, I saw your path and I think we should have this. Just because it's more scalable solution. Any event could count differently, but every 1 of those must count something, as described in their own specs. This will make life of perf folks much easier, cz they will be able to debug perf on qemu. Same to sbi folks i guess. > > Please take a look at the patches shared above or the mailing list > (should land tomorrow) and let me know your thoughts. > I am happy to collaborate on your patches so that we have more than > just a virt machine that we can test with this series. Thanks for your series, I have some thoughts about it, I'll describe them on your patchset. > >> 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 >>> >>>