* [PATCH 1/2] perf/smmuv3: Validate group size
@ 2019-07-31 13:37 Robin Murphy
2019-07-31 13:37 ` [PATCH 2/2] perf/smmuv3: Validate groups for global filtering Robin Murphy
2019-08-01 11:32 ` [PATCH 1/2] perf/smmuv3: Validate group size Will Deacon
0 siblings, 2 replies; 6+ messages in thread
From: Robin Murphy @ 2019-07-31 13:37 UTC (permalink / raw)
To: will, mark.rutland; +Cc: linux-arm-kernel, shameerali.kolothum.thodi
Ensure that a group will actually fit into the available counters.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/perf/arm_smmuv3_pmu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index da71c741cb46..dd40df2ac895 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -323,6 +323,7 @@ static int smmu_pmu_event_init(struct perf_event *event)
struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
struct device *dev = smmu_pmu->dev;
struct perf_event *sibling;
+ int group_num_events = 1;
u16 event_id;
if (event->attr.type != event->pmu->type)
@@ -359,6 +360,9 @@ static int smmu_pmu_event_init(struct perf_event *event)
dev_dbg(dev, "Can't create mixed PMU group\n");
return -EINVAL;
}
+
+ if (++group_num_events >= smmu_pmu->num_counters)
+ return -EINVAL;
}
hwc->idx = -1;
--
2.21.0.dirty
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] perf/smmuv3: Validate groups for global filtering
2019-07-31 13:37 [PATCH 1/2] perf/smmuv3: Validate group size Robin Murphy
@ 2019-07-31 13:37 ` Robin Murphy
2019-08-01 10:56 ` Will Deacon
2019-08-01 11:32 ` [PATCH 1/2] perf/smmuv3: Validate group size Will Deacon
1 sibling, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2019-07-31 13:37 UTC (permalink / raw)
To: will, mark.rutland; +Cc: linux-arm-kernel, shameerali.kolothum.thodi
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;
+ }
}
hwc->idx = -1;
--
2.21.0.dirty
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf/smmuv3: Validate groups for global filtering
2019-07-31 13:37 ` [PATCH 2/2] perf/smmuv3: Validate groups for global filtering Robin Murphy
@ 2019-08-01 10:56 ` Will Deacon
2019-08-01 11:17 ` Robin Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2019-08-01 10:56 UTC (permalink / raw)
To: Robin Murphy; +Cc: mark.rutland, linux-arm-kernel, shameerali.kolothum.thodi
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.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf/smmuv3: Validate groups for global filtering
2019-08-01 10:56 ` Will Deacon
@ 2019-08-01 11:17 ` Robin Murphy
2019-08-01 11:45 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2019-08-01 11:17 UTC (permalink / raw)
To: Will Deacon; +Cc: mark.rutland, linux-arm-kernel, shameerali.kolothum.thodi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Validate group size
2019-07-31 13:37 [PATCH 1/2] perf/smmuv3: Validate group size Robin Murphy
2019-07-31 13:37 ` [PATCH 2/2] perf/smmuv3: Validate groups for global filtering Robin Murphy
@ 2019-08-01 11:32 ` Will Deacon
1 sibling, 0 replies; 6+ messages in thread
From: Will Deacon @ 2019-08-01 11:32 UTC (permalink / raw)
To: Robin Murphy; +Cc: mark.rutland, linux-arm-kernel, shameerali.kolothum.thodi
On Wed, Jul 31, 2019 at 02:37:41PM +0100, Robin Murphy wrote:
> Ensure that a group will actually fit into the available counters.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/perf/arm_smmuv3_pmu.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index da71c741cb46..dd40df2ac895 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -323,6 +323,7 @@ static int smmu_pmu_event_init(struct perf_event *event)
> struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
> struct device *dev = smmu_pmu->dev;
> struct perf_event *sibling;
> + int group_num_events = 1;
> u16 event_id;
>
> if (event->attr.type != event->pmu->type)
> @@ -359,6 +360,9 @@ static int smmu_pmu_event_init(struct perf_event *event)
> dev_dbg(dev, "Can't create mixed PMU group\n");
> return -EINVAL;
> }
> +
> + if (++group_num_events >= smmu_pmu->num_counters)
> + return -EINVAL;
Did you forget to update the condition when you changed your mind about the
count?
;)
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf/smmuv3: Validate groups for global filtering
2019-08-01 11:17 ` Robin Murphy
@ 2019-08-01 11:45 ` Will Deacon
0 siblings, 0 replies; 6+ messages in thread
From: Will Deacon @ 2019-08-01 11:45 UTC (permalink / raw)
To: Robin Murphy; +Cc: mark.rutland, linux-arm-kernel, shameerali.kolothum.thodi
On Thu, Aug 01, 2019 at 12:17:20PM +0100, Robin Murphy wrote:
> 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.
Oh yes, performing the actual register writes could be catastrophic!
All I'm trying to get to is a situation where we don't have to update both
the event_init() and smmu_pmu_get_event_idx() callchains if we gain
additional validation requirements in future.
> (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...)
I noticed it looked weird, but assumed I was being thick and then forgot
about it.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-01 11:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 13:37 [PATCH 1/2] perf/smmuv3: Validate group size Robin Murphy
2019-07-31 13:37 ` [PATCH 2/2] perf/smmuv3: Validate groups for global filtering Robin Murphy
2019-08-01 10:56 ` Will Deacon
2019-08-01 11:17 ` Robin Murphy
2019-08-01 11:45 ` Will Deacon
2019-08-01 11:32 ` [PATCH 1/2] perf/smmuv3: Validate group size Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).