2023-06-01 03:11:23

by Ilkka Koskinen

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

Some platforms may use e.g. different filtering mechanism and, thus,
may need different way to validate the events.

Signed-off-by: Ilkka Koskinen <[email protected]>
---
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;
+
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);
--
2.40.1

2023-06-01 15:18:00

by Robin Murphy

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

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 <[email protected]>
> ---
> 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).

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?

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);

2023-06-02 07:39:20

by Ilkka Koskinen

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

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 <[email protected]>
>> ---
>> 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);
>