| Message ID | 20220408203330.4014015-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm_pmu: Validate single/group leader events | expand |
Hi Rob, On Fri, Apr 08, 2022 at 03:33:30PM -0500, Rob Herring wrote: > In the case where there is only a cycle counter available (i.e. > PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open > should fail as the event can never possibly be scheduled. However, the > event validation when an event is opened is skipped when the group > leader is opened. Fix this by always validating the group leader events. > > Reported-by: Al Grant <al.grant@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Rob Herring <robh@kernel.org> This looks obviously correct to me, so FWIW: Acked-by: Mark Rutland <mark.rutland@arm.com> Just to check, have you tested this (e.g. by running on a platform with PMCR_EL0.N == 0, or hacking the PMU probing to report just the cycle counter) Thanks, Mark. > --- > drivers/perf/arm_pmu.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > index 9694370651fa..59d3980b8ca2 100644 > --- a/drivers/perf/arm_pmu.c > +++ b/drivers/perf/arm_pmu.c > @@ -400,6 +400,9 @@ validate_group(struct perf_event *event) > if (!validate_event(event->pmu, &fake_pmu, leader)) > return -EINVAL; > > + if (event == leader) > + return 0; > + > for_each_sibling_event(sibling, leader) { > if (!validate_event(event->pmu, &fake_pmu, sibling)) > return -EINVAL; > @@ -489,12 +492,7 @@ __hw_perf_event_init(struct perf_event *event) > local64_set(&hwc->period_left, hwc->sample_period); > } > > - if (event->group_leader != event) { > - if (validate_group(event) != 0) > - return -EINVAL; > - } > - > - return 0; > + return validate_group(event); > } > > static int armpmu_event_init(struct perf_event *event) > -- > 2.32.0 >
On Mon, Apr 11, 2022 at 11:04:26AM +0100, Mark Rutland wrote: > Hi Rob, > > On Fri, Apr 08, 2022 at 03:33:30PM -0500, Rob Herring wrote: > > In the case where there is only a cycle counter available (i.e. > > PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open > > should fail as the event can never possibly be scheduled. However, the > > event validation when an event is opened is skipped when the group > > leader is opened. Fix this by always validating the group leader events. > > > > Reported-by: Al Grant <al.grant@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Signed-off-by: Rob Herring <robh@kernel.org> > > This looks obviously correct to me, so FWIW: > > Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks. > Just to check, have you tested this (e.g. by running on a platform with > PMCR_EL0.N == 0, or hacking the PMU probing to report just the cycle counter) Yes, tested on FVP with 0 counters running the libperf evsel userspace access tests as that tries both the cycle counter and instruction counts. Rob
On Mon, Apr 11, 2022 at 09:14:03AM -0500, Rob Herring wrote: > On Mon, Apr 11, 2022 at 11:04:26AM +0100, Mark Rutland wrote: > > Hi Rob, > > > > On Fri, Apr 08, 2022 at 03:33:30PM -0500, Rob Herring wrote: > > > In the case where there is only a cycle counter available (i.e. > > > PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open > > > should fail as the event can never possibly be scheduled. However, the > > > event validation when an event is opened is skipped when the group > > > leader is opened. Fix this by always validating the group leader events. > > > > > > Reported-by: Al Grant <al.grant@arm.com> > > > Cc: Will Deacon <will@kernel.org> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > This looks obviously correct to me, so FWIW: > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > Thanks. > > > Just to check, have you tested this (e.g. by running on a platform with > > PMCR_EL0.N == 0, or hacking the PMU probing to report just the cycle counter) > > Yes, tested on FVP with 0 counters running the libperf evsel userspace > access tests as that tries both the cycle counter and instruction > counts. Perfect, thanks! Will, I assume that you'll pick this up. Mark.
On Fri, 8 Apr 2022 15:33:30 -0500, Rob Herring wrote: > In the case where there is only a cycle counter available (i.e. > PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open > should fail as the event can never possibly be scheduled. However, the > event validation when an event is opened is skipped when the group > leader is opened. Fix this by always validating the group leader events. > > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm_pmu: Validate single/group leader events https://git.kernel.org/arm64/c/e5c23779f93d Cheers,
diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c index 9694370651fa..59d3980b8ca2 100644 --- a/drivers/perf/arm_pmu.c +++ b/drivers/perf/arm_pmu.c @@ -400,6 +400,9 @@ validate_group(struct perf_event *event) if (!validate_event(event->pmu, &fake_pmu, leader)) return -EINVAL; + if (event == leader) + return 0; + for_each_sibling_event(sibling, leader) { if (!validate_event(event->pmu, &fake_pmu, sibling)) return -EINVAL; @@ -489,12 +492,7 @@ __hw_perf_event_init(struct perf_event *event) local64_set(&hwc->period_left, hwc->sample_period); } - if (event->group_leader != event) { - if (validate_group(event) != 0) - return -EINVAL; - } - - return 0; + return validate_group(event); } static int armpmu_event_init(struct perf_event *event)
In the case where there is only a cycle counter available (i.e. PMCR_EL0.N is 0) and an event other than CPU cycles is opened, the open should fail as the event can never possibly be scheduled. However, the event validation when an event is opened is skipped when the group leader is opened. Fix this by always validating the group leader events. Reported-by: Al Grant <al.grant@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/perf/arm_pmu.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)