Re: [PATCH v11 0/5] arm64 userspace counter support

[Mark Rutland]

On Tue, Oct 26, 2021 at 08:34:35AM -0500, Rob Herring wrote:
> On Tue, Oct 26, 2021 at 8:10 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> >
> > On Tue, Oct 19, 2021 at 06:19:02PM -0500, Rob Herring wrote:
> > > Another version of arm64 userspace counter access support.
> > >
> > > The arm64 support departs from the x86 implementation by requiring the user
> > > to explicitly request user access (via attr.config1) and only enables access
> > > for task bound events. Since usage is explicitly requested, access is
> > > enabled at perf_event_open() rather than on mmap() as that greatly
> > > simplifies the implementation. Rather than trying to lock down the access
> > > as the x86 implementation has been doing, we can start with only a limited
> > > use case enabled and later expand it if needed.
> > >
> > > I've run this version thru Vince's perf tests[13] with arm64 support added.
> > > I wish I'd found these tests sooner...
> >
> > When you say "with arm64 support added", do you mean with patches not
> > yet upstreamed?
> 
> Correct.

Did that require significant changes? I'd like to be able to run the
tests locally if possible.

> > I took a look at the upstream repo, and there's some existing RDPMC
> > support even though upstream never previously supported userspace
> > access. That support code uses PMSELR_EL0, which this series adds no
> > provisions for.
> >
> > Kernel-side, we'll need to either:
> >
> > * Document that PMSELR_EL0 is unreliable, and explcitly zero it within
> >   the kernel such that it cnanot be used as a covert channel. Get the
> >   tests updated to not rely on the never-previously-supported use of
> >   PMSELR_EL0.
> >
> > * Context switch PMSELR_EL0 (which'll IIUC is unreliable for big.LITTLE,
> >   even where the registers exist on each CPU).
> 
> Whether we support userspace using PMSELR_EL0 or not, we just need to
> zero it when userspace access is enabled (like the dirty counters).

Sure; doing that in armv8pmu_enable_user_access() makes sense to me,
given it's not exposed to userspace otherwise, and the kernel itself
doesn't care.

If we do that, I think that we should document that it may be zero'd
arbitrarily from userspace's PoV. That should only need a sentence or
two in the documentation added in the final patch.

Thanks,
Mark.