* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.