diff mbox series

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

Message ID 20230601030144.3458136-5-ilkka@os.amperecomputing.com
State New
Headers
Series perf: ampere: Add support for Ampere SoC PMUs |

Commit Message

Ilkka Koskinen June 1, 2023, 3:01 a.m. UTC 
  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@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 4 ++++
 drivers/perf/arm_cspmu/arm_cspmu.h | 2 ++
 2 files changed, 6 insertions(+)

Comments

Robin Murphy June 1, 2023, 3:09 p.m. UTC | #1 
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@os.amperecomputing.com>
> ---
>   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);
Ilkka Koskinen June 2, 2023, 7:09 a.m. UTC | #2 
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@os.amperecomputing.com>
>> ---
>>   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);
>
diff mbox series

Patch

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