Re: [PATCH 2/2] perf/smmuv3: Validate groups for global filtering

[Robin Murphy <robin.murphy@arm.com>]

On 2019-08-01 11:56 am, Will Deacon wrote:
> On Wed, Jul 31, 2019 at 02:37:42PM +0100, Robin Murphy wrote:
>> With global filtering, it becomes possible for users to construct
>> self-contradictory groups with conflicting filters. Make sure we
>> cover that when initially validating events.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/perf/arm_smmuv3_pmu.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>> index dd40df2ac895..e1e41d2fea94 100644
>> --- a/drivers/perf/arm_smmuv3_pmu.c
>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>> @@ -324,6 +324,8 @@ static int smmu_pmu_event_init(struct perf_event *event)
>>   	struct device *dev = smmu_pmu->dev;
>>   	struct perf_event *sibling;
>>   	int group_num_events = 1;
>> +	bool has_filter;
>> +	u32 filter_span, filter_sid;
>>   	u16 event_id;
>>   
>>   	if (event->attr.type != event->pmu->type)
>> @@ -354,6 +356,10 @@ static int smmu_pmu_event_init(struct perf_event *event)
>>   		return -EINVAL;
>>   	}
>>   
>> +	has_filter = get_filter_enable(event);
>> +	filter_span = get_filter_span(event);
>> +	filter_sid = get_filter_stream_id(event);
>> +
>>   	for_each_sibling_event(sibling, event->group_leader) {
>>   		if (sibling->pmu != event->pmu &&
>>   		    !is_software_event(sibling)) {
>> @@ -363,6 +369,16 @@ static int smmu_pmu_event_init(struct perf_event *event)
>>   
>>   		if (++group_num_events >= smmu_pmu->num_counters)
>>   			return -EINVAL;
>> +
>> +		if (smmu_pmu->global_filter) {
>> +			if (has_filter != (bool)get_filter_enable(sibling))
>> +				return -EINVAL;
>> +
>> +			if (has_filter &&
>> +			    (filter_span != get_filter_span(sibling) ||
>> +			    filter_sid != get_filter_stream_id (sibling)))
>> +				return -EINVAL;
>> +		}
> 
> Can we avoid duplicating the validation logic from
> smmu_pmu_apply_event_filter() by adding the group to a fake PMU, like we
> do for the CPU PMU and the DSU PMU? You'll probably need to do a bit
> of surgery on 'struct smmu_pmu' to move out the bits you need, but I
> think it would be better that way.

Given that apply_event_filter() includes the actual register writes, I 
have a strong feeling that down that road lies madness. However, on a 
second look, I see no reason not to factor out the global filter 
validation part for reuse, and in fact I think we can even save 
explicitly tracking global_filter_{span,sid} that way. I'll give it a try.

Robin.

(and hopefully the promise of a respin will let us all overlook the 
obvious "forgot to update the condition when I changed my mind about the 
count" error in patch #1...)

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