| Message ID | 20220825-arm-spe-v8-7-v3-7-87682f78caac@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: Arm SPEv1.2 support | expand |
On Fri, Nov 04, 2022 at 10:55:07AM -0500, Rob Herring wrote: > Arm SPEv1.2 adds another 64-bits of event filtering control. As the > existing perf_event_attr::configN fields are all used up for SPE PMU, an > additional field is needed. Add a new 'config3' field. > > Tested-by: James Clark <james.clark@arm.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > v3: > - No change > v2: > - Drop tools/ side update > --- > include/uapi/linux/perf_event.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > index 85be78e0e7f6..b2b1d7b54097 100644 > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -374,6 +374,7 @@ enum perf_event_read_format { > #define PERF_ATTR_SIZE_VER5 112 /* add: aux_watermark */ > #define PERF_ATTR_SIZE_VER6 120 /* add: aux_sample_size */ > #define PERF_ATTR_SIZE_VER7 128 /* add: sig_data */ > +#define PERF_ATTR_SIZE_VER8 136 /* add: config3 */ > > /* > * Hardware event_id to monitor via a performance monitoring event: > @@ -515,6 +516,8 @@ struct perf_event_attr { > * truncated accordingly on 32 bit architectures. > */ > __u64 sig_data; > + > + __u64 config3; /* extension of config2 */ I need an ack from the perf core maintainers before I can take this. Will
On Fri, Nov 18, 2022 at 10:49 AM Will Deacon <will@kernel.org> wrote: > > On Fri, Nov 04, 2022 at 10:55:07AM -0500, Rob Herring wrote: > > Arm SPEv1.2 adds another 64-bits of event filtering control. As the > > existing perf_event_attr::configN fields are all used up for SPE PMU, an > > additional field is needed. Add a new 'config3' field. > > > > Tested-by: James Clark <james.clark@arm.com> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > v3: > > - No change > > v2: > > - Drop tools/ side update > > --- > > include/uapi/linux/perf_event.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > > index 85be78e0e7f6..b2b1d7b54097 100644 > > --- a/include/uapi/linux/perf_event.h > > +++ b/include/uapi/linux/perf_event.h > > @@ -374,6 +374,7 @@ enum perf_event_read_format { > > #define PERF_ATTR_SIZE_VER5 112 /* add: aux_watermark */ > > #define PERF_ATTR_SIZE_VER6 120 /* add: aux_sample_size */ > > #define PERF_ATTR_SIZE_VER7 128 /* add: sig_data */ > > +#define PERF_ATTR_SIZE_VER8 136 /* add: config3 */ > > > > /* > > * Hardware event_id to monitor via a performance monitoring event: > > @@ -515,6 +516,8 @@ struct perf_event_attr { > > * truncated accordingly on 32 bit architectures. > > */ > > __u64 sig_data; > > + > > + __u64 config3; /* extension of config2 */ > > I need an ack from the perf core maintainers before I can take this. Peter, Arnaldo, Ingo, Can I get an ack on this please. Rob
Rob Herring <robh@kernel.org> writes: > On Fri, Nov 18, 2022 at 10:49 AM Will Deacon <will@kernel.org> wrote: >> >> On Fri, Nov 04, 2022 at 10:55:07AM -0500, Rob Herring wrote: >> > @@ -515,6 +516,8 @@ struct perf_event_attr { >> > * truncated accordingly on 32 bit architectures. >> > */ >> > __u64 sig_data; >> > + >> > + __u64 config3; /* extension of config2 */ >> >> I need an ack from the perf core maintainers before I can take this. > > Peter, Arnaldo, Ingo, > > Can I get an ack on this please. It appears that PMUs that don't use config{1,2} and now config3 allow them to be whatever without any validation, whereas in reality we should probably -EINVAL in those cases. Should something be done about that? Regards, -- Alex
On Mon, Nov 28, 2022 at 10:36 AM Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote: > > Rob Herring <robh@kernel.org> writes: > > > On Fri, Nov 18, 2022 at 10:49 AM Will Deacon <will@kernel.org> wrote: > >> > >> On Fri, Nov 04, 2022 at 10:55:07AM -0500, Rob Herring wrote: > >> > @@ -515,6 +516,8 @@ struct perf_event_attr { > >> > * truncated accordingly on 32 bit architectures. > >> > */ > >> > __u64 sig_data; > >> > + > >> > + __u64 config3; /* extension of config2 */ > >> > >> I need an ack from the perf core maintainers before I can take this. > > > > Peter, Arnaldo, Ingo, > > > > Can I get an ack on this please. > > It appears that PMUs that don't use config{1,2} and now config3 allow > them to be whatever without any validation, whereas in reality we should > probably -EINVAL in those cases. Should something be done about that? Always the 3rd occurrence that gets to clean-up things. ;) I think we'd have to add some capability flags for PMU drivers to set to enable configN usage and then use those to validate configN is 0. Wouldn't be too hard to do for config3 as we know there's exactly 1 user, but for 1,2 there's about 80 PMU drivers to check. Rob
Peter, it looks like this series is blocked on the below now; what would you
prefer out of:
(a) Take this as is, and look add adding additional validation on top.
(b) Add some flag to indicate a PMU driver supports config3, and have the core
code check that, but leave the existing fields as-is for now (and hopefully
follow up with further validation later for the existing fields).
(c) Go audit all the existing drivers, add flags to indicate support for
existing fields, and have the core code check that. Atop that, add support
for config3 with the same sort of flag check.
I suspect that'd end up needing to go check more than config1/config2 given
all the filter controls and so on that drivers aren't great at checking,
and that might being fairly invasive.
(d) Something else?
I think we want to get to a point where drivers indicate what they actually
support and the core code rejects stuff drivers don't support or recognise, but
I think it'd be a little unreasonable to delay this series on cleaning up all
the existing issues.
I'm tempted to say (b) as that shouldn't introduce any regressions, should be a
relatively simple change to this series, and doesn't precluse making the rest
stricter as a follow-up. I'm happy to take a look at that (and IIUC Rob is
too).
What's your preference?
Thanks,
Mark.
On Mon, Nov 28, 2022 at 11:15:21AM -0600, Rob Herring wrote:
> On Mon, Nov 28, 2022 at 10:36 AM Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
> >
> > Rob Herring <robh@kernel.org> writes:
> >
> > > On Fri, Nov 18, 2022 at 10:49 AM Will Deacon <will@kernel.org> wrote:
> > >>
> > >> On Fri, Nov 04, 2022 at 10:55:07AM -0500, Rob Herring wrote:
> > >> > @@ -515,6 +516,8 @@ struct perf_event_attr {
> > >> > * truncated accordingly on 32 bit architectures.
> > >> > */
> > >> > __u64 sig_data;
> > >> > +
> > >> > + __u64 config3; /* extension of config2 */
> > >>
> > >> I need an ack from the perf core maintainers before I can take this.
> > >
> > > Peter, Arnaldo, Ingo,
> > >
> > > Can I get an ack on this please.
> >
> > It appears that PMUs that don't use config{1,2} and now config3 allow
> > them to be whatever without any validation, whereas in reality we should
> > probably -EINVAL in those cases. Should something be done about that?
>
> Always the 3rd occurrence that gets to clean-up things. ;)
>
> I think we'd have to add some capability flags for PMU drivers to set
> to enable configN usage and then use those to validate configN is 0.
> Wouldn't be too hard to do for config3 as we know there's exactly 1
> user, but for 1,2 there's about 80 PMU drivers to check.
>
> Rob
>
On Tue, Dec 6, 2022 at 10:28 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Peter, it looks like this series is blocked on the below now; what would you > prefer out of: > > (a) Take this as is, and look add adding additional validation on top. > > (b) Add some flag to indicate a PMU driver supports config3, and have the core > code check that, but leave the existing fields as-is for now (and hopefully > follow up with further validation later for the existing fields). That looks something like this: diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index 853f64b6c8c2..845162b152ea 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -286,6 +286,7 @@ struct perf_event; #define PERF_PMU_CAP_NO_EXCLUDE 0x0080 #define PERF_PMU_CAP_AUX_OUTPUT 0x0100 #define PERF_PMU_CAP_EXTENDED_HW_TYPE 0x0200 +#define PERF_PMU_CAP_CONFIG3 0x0400 struct perf_output_handle; diff --git a/kernel/events/core.c b/kernel/events/core.c index aefc1e08e015..4414ae64432a 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -11314,6 +11314,9 @@ static int perf_try_init_event(struct pmu *pmu, struct perf_event *event) event_has_any_exclude_flag(event)) ret = -EINVAL; + if (!(pmu->capabilities & PERF_PMU_CAP_CONFIG3) && event->attr.config3) + ret = -EINVAL; + if (ret && event->destroy) event->destroy(event); }
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index 85be78e0e7f6..b2b1d7b54097 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -374,6 +374,7 @@ enum perf_event_read_format { #define PERF_ATTR_SIZE_VER5 112 /* add: aux_watermark */ #define PERF_ATTR_SIZE_VER6 120 /* add: aux_sample_size */ #define PERF_ATTR_SIZE_VER7 128 /* add: sig_data */ +#define PERF_ATTR_SIZE_VER8 136 /* add: config3 */ /* * Hardware event_id to monitor via a performance monitoring event: @@ -515,6 +516,8 @@ struct perf_event_attr { * truncated accordingly on 32 bit architectures. */ __u64 sig_data; + + __u64 config3; /* extension of config2 */ }; /*