Re: [PATCH 1/1] Return EINVAL when precise_ip perf events are requested on Arm

From: Will Deacon
Date: Fri Jan 17 2020 - 07:39:29 EST


Hi James,

On Wed, Jan 15, 2020 at 10:58:55AM +0000, James Clark wrote:
> ARM PMU events can be delivered with arbitrary skid, and there's
> nothing the kernel can do to prevent this. Given that, the PMU
> cannot support precise_ip != 0.
>
> Also update comment to state that attr.config field is used to
> set the event type rather than event_id which doesn't exist.

"Also..." is usually a good sign that you should split up the patch. In
this case, you're touching a UAPI header with a questionable clarification,
so I'd definitely rather see that handled separately.

> Signed-off-by: James Clark <james.clark@xxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> Cc: Tan Xiaojun <tanxiaojun@xxxxxxxxxx>
> Cc: Al Grant <al.grant@xxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> drivers/perf/arm_pmu.c | 3 +++
> include/uapi/linux/perf_event.h | 4 ++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index df352b334ea7..4ddbdb93b3b6 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -102,6 +102,9 @@ armpmu_map_event(struct perf_event *event,
> u64 config = event->attr.config;
> int type = event->attr.type;
>
> + if (event->attr.precise)
> + return -EINVAL;

You're right that this is a user-visible change, and I'm pretty nervous
about it to be honest with you.

Perhaps a better way would be to expose something under sysfs, a bit like
the caps directory for the SPE PMU, which identifies the fields of the attr
structure that the driver does not ignore. I think doing this as an Arm-PMU
specific thing initially would be fine, but it would be even better to have
something where a driver can tell perf core about the parts it responds to
and have this stuff populated automatically. The current design makes it
inevitable that PMU drivers will have issues like the one you point out in
the cover letter.

Thoughts?

Will