Re: [PATCH v2] arm64: Implement prctl(PR_{G,S}ET_TSC)

[Will Deacon <will@kernel.org>]

On Fri, Aug 16, 2024 at 12:04:14PM -0700, Peter Collingbourne wrote:
> On Fri, Aug 16, 2024 at 3:20 AM Will Deacon <will@kernel.org> wrote:
> >
> > Hi Peter,
> >
> > On Mon, Jun 10, 2024 at 01:24:51PM -0700, Peter Collingbourne wrote:
> > > On Fri, May 17, 2024 at 2:25 PM Peter Collingbourne <pcc@google.com> wrote:
> > > >
> > > > On arm64, this prctl controls access to CNTVCT_EL0, CNTVCTSS_EL0 and
> > > > CNTFRQ_EL0 via CNTKCTL_EL1.EL0VCTEN. Since this bit is also used to
> > > > implement various erratum workarounds, check whether the CPU needs
> > > > a workaround whenever we potentially need to change it.
> > >
> > > Ping.
> >
> > Chatting with Marc and Catalin about this, we're a little uneasy about
> > exposing a prctl() for this. Ptrace does seem like a much better fit.
> 
> I still disagree for the consistency and ease of use reasons
> previously mentioned. But I suppose that a ptrace() API would be
> better than nothing.
> 
> > Can you add a new regset for the counter instead?
> 
> Is your idea that setting the regset to a specific CNTVCT_EL0 value
> will cause the kernel to trap and automatically return the previously
> set CNTVCT_EL0 without notifying the tracer? I don't think it is
> suitable for rr. This would force rr to set CNTVCT_EL0 to a specific
> value and keep it constant (or at least it can change it when the
> process is stopped for some other reason), whereas what rr wants to do
> is handle a CNTVCT_EL0 read at record time by reading CNTVCT_EL0 and
> recording the value that was read, and replaying those values at
> replay time. To give one example of where a constant CNTVCT_EL0 can
> cause problems, imagine that we have a program such as a delay loop
> that reads CNTVCT_EL0 repeatedly and compares its value to a
> previously read one and only makes progress if the value changes. Such
> a program will not make progress if there is no way to make CNTVCT_EL0
> change between reads. So if it has to be a ptrace() API, then I think
> it needs to be an opt-in to putting the process into a stopped state
> upon reading CNTVCT_EL0.

Thanks, makes sense. I replied on the main patch.

Will