Re: [PATCH v3 7/8] perf: Add perf_event_attr::config3

From: Mark Rutland <mark.rutland@arm.com>
To: Rob Herring <robh@kernel.org>, Peter Zijlstra <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Will Deacon <will@kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ingo Molnar <mingo@redhat.com>,
	Namhyung Kim <namhyung@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	kvmarm@lists.linux.dev, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, James Clark <james.clark@arm.com>,
	Mark Brown <broonie@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v3 7/8] perf: Add perf_event_attr::config3
Date: Tue, 6 Dec 2022 16:28:38 +0000	[thread overview]
Message-ID: <Y49ttrv6W5k3ZNYw@FVFF77S0Q05N.cambridge.arm.com> (raw)
In-Reply-To: <CAL_JsqKJYrEUzkzNMKMOGvzJN_EqacHZvBR6eVt35bRhhtRo=g@mail.gmail.com>

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
> 