Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Atish Kumar Patra <atishp@rivosinc.com>]

On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
<alexei.filippov@yadro.com> wrote:
>
>
>
> On 10.10.2024 02:09, Atish Patra wrote:
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >   target/riscv/cpu.h | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 2ac391a7cf74..53426710f73e 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
> >           uint64_t counter_virt_prev[2];
> >   } PMUFixedCtrState;
> >
> > +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
> > +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
> > +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
> > +
> > +typedef struct PMUEventInfo {
> > +    /* Event ID (BIT [0:55] valid) */
> > +    uint64_t event_id;
> > +    /* Supported hpmcounters for this event */
> > +    uint32_t counter_mask;
> > +    /* Bitmask of valid event bits */
> > +    uint64_t event_mask;
> > +} PMUEventInfo;
> > +
> > +typedef struct PMUEventFunc {
> > +    /* Get the ID of the event that can monitor cycles */
> > +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
> > +    /* Get the ID of the event that can monitor cycles */
> > +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
> > +    /* Get the ID of the event that can monitor TLB events*/
> > +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
> Ok, this is kinda interesting decision, but it's not scalable. AFAIU

Yes it is not scalable if there is a need to scale as mentioned earlier.
Do you have any other events that can be emulated in Qemu ?

Having said that, I am okay with single call back though but not too sure
about read/write callback unless there is a use case to support those.

> none spec provide us full enum of existing events. So anytime when
> somebody will try to implement their own pmu events they would have to
> add additional callbacks, and this structure never will be fulled
> properly. And then we ended up with structure 1000+ callback with only 5
> machines wich supports pmu events. I suggest my approach with only
> read/write callbacks, where machine can decide by itself how to handle
> any of machine specific events.

Lot of these events are microarchitectural events which can't be
supported in Qemu.
I don't think it's a good idea at all to add dummy values for all the
events defined in a platform
which is harder to debug for a user.


> > +} PMUEventFunc;
> > +
> >   struct CPUArchState {
> >       target_ulong gpr[32];
> >       target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> > @@ -386,6 +408,9 @@ struct CPUArchState {
> >       target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >
> >       PMUFixedCtrState pmu_fixed_ctrs[2];
> > +    PMUEventInfo *pmu_events;
> > +    PMUEventFunc pmu_efuncs;
> > +    int num_pmu_events;
> >
> >       target_ulong sscratch;
> >       target_ulong mscratch;
> >