[PATCH RFC 00/10] Allow platform specific PMU event encoding

Atish Patra posted 10 patches 1 month, 2 weeks ago
hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
target/riscv/cpu.h        |  52 ++++++++++--
target/riscv/cpu_bits.h   |   4 +-
target/riscv/cpu_helper.c |  21 ++---
target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
target/riscv/pmu.h        |   3 +-
6 files changed, 246 insertions(+), 134 deletions(-)
[PATCH RFC 00/10] Allow platform specific PMU event encoding
Posted by Atish Patra 1 month, 2 weeks ago
Currently, the pmu implementation is virt machine specific that
implements the SBI PMU encodings. In the future, individual machines
would want to implement their own custom event encodings as there
is no standard event encoding defined by the ISA. There is a performance
events TG which is working on defining standard set of events but not
encodings. That allows flexibility for platforms to choose whatever
encoding scheme they want. However, that means the generic pmu code
should be flexible enough to allow that in Qemu as well.

This series aims to solve that problem by first disassociating the
common pmu implementation and event encoding. The event encoding is
specific to a platform and should be defined in the platform specific
machine or cpu implementation. The newly defined callbacks can be invoked
from machine specific cpu implementation or machine code itself depending
on the requirement.

The first 5 patches in the series are generic improvements and cleanups
where as the last 5 actually implements the disassociation for the virt
machine. The current series can also be found at[2].

I recently found that Alexei has done a similar effort for SiFive FU740[1]
but the implementation differs from this one based on how the cpu callbacks
are invoked. For example, Alexei's series implements a single callback for
all the events and has defined machine specific counter read/write callbacks.
However, it just defaults to get_ticks() for every event. IMO, that is
confusing to the users unless we can actually emulate more generic events or
machine specific events.

I have separate callbacks for each type of events that we currently support
in Qemu (cycle, instruction, TLB events). Separate callbacks seems a better
approach to avoid ambiguity as we have very limited event capability in qemu.
I am open to converging them to one callback as well if we think we will
be extending set of events in the future.

Once we converge on the approaches, we can consolidate the patches
so that both SiFive FU740 and virt machine can use the same abstraction.

Cc: alexei.filippov@syntacore.com

[1] https://lore.kernel.org/all/20240910174747.148141-1-alexei.filippov@syntacore.com/T/
[2] https://github.com/atishp04/qemu/tree/b4/pmu_event_machine

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (10):
      target/riscv: Fix the hpmevent mask
      target/riscv: Introduce helper functions for pmu hashtable lookup
      target/riscv: Protect the hashtable modifications with a lock
      target/riscv: Use uint64 instead of uint as key
      target/riscv: Rename the PMU events
      target/riscv: Define PMU event related structures
      hw/riscv/virt.c : Disassociate virt PMU events
      target/riscv: Update event mapping hashtable for invalid events
      target/riscv : Use the new tlb fill event functions
      hw/riscv/virt.c: Generate the PMU node from the machine

 hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
 target/riscv/cpu.h        |  52 ++++++++++--
 target/riscv/cpu_bits.h   |   4 +-
 target/riscv/cpu_helper.c |  21 ++---
 target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
 target/riscv/pmu.h        |   3 +-
 6 files changed, 246 insertions(+), 134 deletions(-)
---
base-commit: 19a9809808a51291008f72d051d0f9b3499fc223
change-id: 20241008-pmu_event_machine-b87c58104e61
--
Regards,
Atish patra
Re: [PATCH RFC 00/10] Allow platform specific PMU event encoding
Posted by Alexei Filippov 1 month, 2 weeks ago

On 10.10.2024 02:08, Atish Patra wrote:
> Currently, the pmu implementation is virt machine specific that
> implements the SBI PMU encodings. In the future, individual machines
> would want to implement their own custom event encodings as there
> is no standard event encoding defined by the ISA. There is a performance
> events TG which is working on defining standard set of events but not
> encodings. That allows flexibility for platforms to choose whatever
> encoding scheme they want. However, that means the generic pmu code
> should be flexible enough to allow that in Qemu as well.
> 
> This series aims to solve that problem by first disassociating the
> common pmu implementation and event encoding. The event encoding is
> specific to a platform and should be defined in the platform specific
> machine or cpu implementation. The newly defined callbacks can be invoked
> from machine specific cpu implementation or machine code itself depending
> on the requirement.
> 
> The first 5 patches in the series are generic improvements and cleanups
> where as the last 5 actually implements the disassociation for the virt
> machine. The current series can also be found at[2].
> 
> I recently found that Alexei has done a similar effort for SiFive FU740[1]
> but the implementation differs from this one based on how the cpu callbacks
> are invoked. For example, Alexei's series implements a single callback for
> all the events and has defined machine specific counter read/write callbacks.
> However, it just defaults to get_ticks() for every event. IMO, that is
> confusing to the users unless we can actually emulate more generic events or
> machine specific events.
> 
> I have separate callbacks for each type of events that we currently support
> in Qemu (cycle, instruction, TLB events). Separate callbacks seems a better
> approach to avoid ambiguity as we have very limited event capability in qemu.
> I am open to converging them to one callback as well if we think we will
> be extending set of events in the future.
> 
> Once we converge on the approaches, we can consolidate the patches
> so that both SiFive FU740 and virt machine can use the same abstraction.
> 
> Cc: alexei.filippov@syntacore.com
>
Thanks for CCing me and your patch. Your done a great work, but still I 
do not think this approach with per event callback are scalable enough. 
I'll suggest to collaborate to work your and mine solution to unite them 
to one patch set. Let me know what do you think.
> [1] https://lore.kernel.org/all/20240910174747.148141-1-alexei.filippov@syntacore.com/T/
> [2] https://github.com/atishp04/qemu/tree/b4/pmu_event_machine
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> Atish Patra (10):
>        target/riscv: Fix the hpmevent mask
>        target/riscv: Introduce helper functions for pmu hashtable lookup
>        target/riscv: Protect the hashtable modifications with a lock
>        target/riscv: Use uint64 instead of uint as key
>        target/riscv: Rename the PMU events
>        target/riscv: Define PMU event related structures
>        hw/riscv/virt.c : Disassociate virt PMU events
>        target/riscv: Update event mapping hashtable for invalid events
>        target/riscv : Use the new tlb fill event functions
>        hw/riscv/virt.c: Generate the PMU node from the machine
> 
>   hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
>   target/riscv/cpu.h        |  52 ++++++++++--
>   target/riscv/cpu_bits.h   |   4 +-
>   target/riscv/cpu_helper.c |  21 ++---
>   target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
>   target/riscv/pmu.h        |   3 +-
>   6 files changed, 246 insertions(+), 134 deletions(-)
> ---
> base-commit: 19a9809808a51291008f72d051d0f9b3499fc223
> change-id: 20241008-pmu_event_machine-b87c58104e61
> --
> Regards,
> Atish patra
>
Re: [PATCH RFC 00/10] Allow platform specific PMU event encoding
Posted by Atish Kumar Patra 1 month, 1 week ago
On Thu, Oct 10, 2024 at 5:51 AM Alexei Filippov
<alexei.filippov@yadro.com> wrote:
>
>
>
> On 10.10.2024 02:08, Atish Patra wrote:
> > Currently, the pmu implementation is virt machine specific that
> > implements the SBI PMU encodings. In the future, individual machines
> > would want to implement their own custom event encodings as there
> > is no standard event encoding defined by the ISA. There is a performance
> > events TG which is working on defining standard set of events but not
> > encodings. That allows flexibility for platforms to choose whatever
> > encoding scheme they want. However, that means the generic pmu code
> > should be flexible enough to allow that in Qemu as well.
> >
> > This series aims to solve that problem by first disassociating the
> > common pmu implementation and event encoding. The event encoding is
> > specific to a platform and should be defined in the platform specific
> > machine or cpu implementation. The newly defined callbacks can be invoked
> > from machine specific cpu implementation or machine code itself depending
> > on the requirement.
> >
> > The first 5 patches in the series are generic improvements and cleanups
> > where as the last 5 actually implements the disassociation for the virt
> > machine. The current series can also be found at[2].
> >
> > I recently found that Alexei has done a similar effort for SiFive FU740[1]
> > but the implementation differs from this one based on how the cpu callbacks
> > are invoked. For example, Alexei's series implements a single callback for
> > all the events and has defined machine specific counter read/write callbacks.
> > However, it just defaults to get_ticks() for every event. IMO, that is
> > confusing to the users unless we can actually emulate more generic events or
> > machine specific events.
> >
> > I have separate callbacks for each type of events that we currently support
> > in Qemu (cycle, instruction, TLB events). Separate callbacks seems a better
> > approach to avoid ambiguity as we have very limited event capability in qemu.
> > I am open to converging them to one callback as well if we think we will
> > be extending set of events in the future.
> >
> > Once we converge on the approaches, we can consolidate the patches
> > so that both SiFive FU740 and virt machine can use the same abstraction.
> >
> > Cc: alexei.filippov@syntacore.com
> >
> Thanks for CCing me and your patch. Your done a great work, but still I
> do not think this approach with per event callback are scalable enough.
> I'll suggest to collaborate to work your and mine solution to unite them
> to one patch set. Let me know what do you think.

Yes. We should definitely collaborate and send a single series to support both
virt and sifive machines. You had a question about widening the
hpmevent in your series.

(Answering here to keep the discussion in 1 place)

As per the section 18.1. Count Overflow Control, only top 8 bits are reserved.
Thus, a platform can implement their event encoding to upto 56 bit wide.

> > [1] https://lore.kernel.org/all/20240910174747.148141-1-alexei.filippov@syntacore.com/T/
> > [2] https://github.com/atishp04/qemu/tree/b4/pmu_event_machine
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> > Atish Patra (10):
> >        target/riscv: Fix the hpmevent mask
> >        target/riscv: Introduce helper functions for pmu hashtable lookup
> >        target/riscv: Protect the hashtable modifications with a lock
> >        target/riscv: Use uint64 instead of uint as key
> >        target/riscv: Rename the PMU events
> >        target/riscv: Define PMU event related structures
> >        hw/riscv/virt.c : Disassociate virt PMU events
> >        target/riscv: Update event mapping hashtable for invalid events
> >        target/riscv : Use the new tlb fill event functions
> >        hw/riscv/virt.c: Generate the PMU node from the machine
> >
> >   hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
> >   target/riscv/cpu.h        |  52 ++++++++++--
> >   target/riscv/cpu_bits.h   |   4 +-
> >   target/riscv/cpu_helper.c |  21 ++---
> >   target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
> >   target/riscv/pmu.h        |   3 +-
> >   6 files changed, 246 insertions(+), 134 deletions(-)
> > ---
> > base-commit: 19a9809808a51291008f72d051d0f9b3499fc223
> > change-id: 20241008-pmu_event_machine-b87c58104e61
> > --
> > Regards,
> > Atish patra
> >