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

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

On Thu, 22 Jun 2023, Besar Wicaksono wrote:

>> -----Original Message-----
>> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> Sent: Thursday, June 22, 2023 3:34 PM
>> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
>> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-
>> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> 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@os.amperecomputing.com> 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.

That's a good idea. I do that.

Cheers, Ilkka

>
>> Otherwise one question inline.
>>>
>>> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>>  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);
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel