| Message ID | 1921604dc4cd820363ccf728ade6508e0987e082.1564580090.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] perf/smmuv3: Validate group size | expand |
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
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...)
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
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;
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(+)