Re: [PATCH v2 4/5] perf: arm_cspmu: Support implementation specific event validation

[Ilkka Koskinen]

Hi Robin,

On Thu, 1 Jun 2023, Robin Murphy wrote:
> On 2023-06-01 04:01, Ilkka Koskinen wrote:
> > Some platforms may use e.g. different filtering mechanism and, thus,
> > may need different way to validate the events.
> >
> > Signed-off-by: Ilkka Koskinen <ilkka@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/perf/arm_cspmu/arm_cspmu.c | 4 ++++
> >   drivers/perf/arm_cspmu/arm_cspmu.h | 2 ++
> >   2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c 
> > b/drivers/perf/arm_cspmu/arm_cspmu.c
> > index b4c4ef81c719..a26f484e06b1 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> > @@ -593,6 +593,10 @@ static int arm_cspmu_get_event_idx(struct 
> > arm_cspmu_hw_events *hw_events,
> >   	if (idx >= cspmu->num_logical_ctrs)
> >   		return -EAGAIN;
> >   +	if (cspmu->impl.ops.validate_event &&
> > +	    !cspmu->impl.ops.validate_event(cspmu, event))
> > +		return -EAGAIN;
>
> Seems like this should be -EINVAL, or maybe the callback should return int so 
> it can make its own distinction (yes, I know the outer logic doesn't actually 
> propagate it, but there's no reason that couldn't improve at some point as 
> well).

Makes sense to me.

> Another thought is that once we get into imp-def conditions for whether an 
> event is valid in itself, we presumably also need to consider imp-def 
> conditions for whether a given pair of events are compatible to be grouped?

That's a good point. I'll take a look at it.

Cheers, Ilkka

> Thanks,
> Robin.
>
> > +
> >   	set_bit(idx, hw_events->used_ctrs);
> >     	return idx;
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h 
> > b/drivers/perf/arm_cspmu/arm_cspmu.h
> > index 4a29b921f7e8..0e5c316c96f9 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> > @@ -106,6 +106,8 @@ struct arm_cspmu_impl_ops {
> >   	void (*set_ev_filter)(struct arm_cspmu *cspmu,
> >   			      struct hw_perf_event *hwc,
> >   			      u32 filter);
> > +	/* Implementation specific event validation */
> > +	bool (*validate_event)(struct arm_cspmu *cspmu, struct perf_event 
> > *new);
> >   	/* Hide/show unsupported events */
> >   	umode_t (*event_attr_is_visible)(struct kobject *kobj,
> >   					 struct attribute *attr, int unused);