RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Besar Wicaksono]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Sent: Thursday, June 22, 2023 3:34 PM
> To: Ilkka Koskinen <ilkka@xxxxxxxxxxxxxxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>; Robin Murphy <robin.murphy@xxxxxxx>;
> Besar Wicaksono <bwicaksono@xxxxxxxxxx>; Suzuki K Poulose
> <suzuki.poulose@xxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; linux-
> arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
> filters
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 21 Jun 2023 18:11:39 -0700
> Ilkka Koskinen <ilkka@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > Generic filters aren't used in all the platforms. Instead, the platforms
> > may use different means to filter events. Add support for implementation
> > specific filters.
> 
> If the specification allows explicitly for non standard ways of controlling filters
> it would be good to add a specification reference to this.
> 

Want to point out that the spec considers PMEVTYPER and PMEVFILTR* registers as optional,
please refer to section 2.1 in the spec. The spec also defines PMIMPDEF registers (starting
from offset 0xD80), which is intended for implementation defined extension. My interpretation
to this is implementer can have other methods to configure event selection and filtering, although
maybe not clear of how much freedom is given to the implementer. 

> Otherwise one question inline.
> >
> > Reviewed-by: Besar Wicaksono <bwicaksono@xxxxxxxxxx>
> > Signed-off-by: Ilkka Koskinen <ilkka@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
> >  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> > index 0f517152cb4e..fafd734c3218 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> > @@ -117,6 +117,9 @@
> >
> >  static unsigned long arm_cspmu_cpuhp_state;
> >
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +                                 struct hw_perf_event *hwc, u32 filter);
> > +
> >  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
> >  {
> >       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> > @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct
> arm_cspmu *cspmu)
> >       CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
> >       CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
> >       CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
> > +     CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
> >
> >       return 0;
> >  }
> > @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct
> arm_cspmu *cspmu,
> >       writel(hwc->config, cspmu->base0 + offset);
> >  }
> >
> > -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> >                                          struct hw_perf_event *hwc,
> >                                          u32 filter)
> >  {
> > @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event
> *event, int pmu_flags)
> >               arm_cspmu_set_cc_filter(cspmu, filter);
> >       } else {
> >               arm_cspmu_set_event(cspmu, hwc);
> > -             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> > +             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
> 
> Optional callback so don't you need either provide a default, or check
> it isn't null?
> 

Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallback
to default if ops.set_ev_filter is null.

Regards,
Besar

> >       }
> >
> >       hwc->state = 0;
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
> b/drivers/perf/arm_cspmu/arm_cspmu.h
> > index 83df53d1c132..d6d88c246a23 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> > @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
> >       u32 (*event_type)(const struct perf_event *event);
> >       /* Decode filter value from configs */
> >       u32 (*event_filter)(const struct perf_event *event);
> > +     /* Set event filter */
> > +     void (*set_ev_filter)(struct arm_cspmu *cspmu,
> > +                           struct hw_perf_event *hwc, u32 filter);
> >       /* Hide/show unsupported events */
> >       umode_t (*event_attr_is_visible)(struct kobject *kobj,
> >                                        struct attribute *attr, int unused);