linux-kernel - Re: [PATCH v3 5/6] coresight: perf: Update buffer on AUX pause

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250401150857.GD115840@e132581.arm.com>
Date: Tue, 1 Apr 2025 16:08:57 +0100
From: Leo Yan <leo.yan@....com>
To: Mike Leach <mike.leach@...aro.org>
Cc: Suzuki K Poulose <suzuki.poulose@....com>,
	James Clark <james.clark@...aro.org>,
	Jonathan Corbet <corbet@....net>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] coresight: perf: Update buffer on AUX pause

On Tue, Apr 01, 2025 at 03:35:44PM +0100, Mike Leach wrote:

[...]

> > > -static void etm_event_pause(struct coresight_device *csdev,
> > > +static void etm_event_pause(struct perf_event *event,
> > > +                         struct coresight_device *csdev,
> > >                           struct etm_ctxt *ctxt)
> > >   {
> > > +     int cpu = smp_processor_id();
> > > +     struct coresight_device *sink;
> > > +     struct perf_output_handle *handle = &ctxt->handle;
> > > +     struct coresight_path *path;
> > > +     unsigned long size;
> > > +
> > >       if (!ctxt->event_data)
> > >               return;
> > >
> > >       /* Stop tracer */
> > >       coresight_pause_source(csdev);
> > > +
> > > +     path = etm_event_cpu_path(ctxt->event_data, cpu);
> > > +     sink = coresight_get_sink(path);
> > > +     if (WARN_ON_ONCE(!sink))
> > > +             return;
> > > +
> > > +     /*
> > > +      * The per CPU sink has own interrupt handling, it might have
> > > +      * race condition with updating buffer on AUX trace pause if
> > > +      * it is invoked from NMI.  To avoid the race condition,
> > > +      * disallows updating buffer for the per CPU sink case.
> > > +      */
> > > +     if (coresight_is_percpu_sink(sink))
> > > +             return;
> > > +
> > > +     if (WARN_ON_ONCE(handle->event != event))
> > > +             return;
> > > +
> > > +     if (!sink_ops(sink)->update_buffer)
> > > +             return;
> > > +
> > > +     size = sink_ops(sink)->update_buffer(sink, handle,
> > > +                                          ctxt->event_data->snk_config);
> >
> > I believe we keep the sink disabled/stopped in update_buffer. We need to
> > turn it back ON after the "buffer update". May be we could use a flag
> > to update_buffer() to indicate this is "pause" triggered update.

Thanks for pointing out, Suzuki.

I will fix it in next spin.

> The sink is stopped to read data, but also important is the
> enable/disable refcount. The only time that "update_buffer" will read
> is if the sink has a refcount == 1. These are ultimately controlled by
> enable/disable path - which will not occur during pause/resume
> operations.

Hi Mike,

My understanding is: if a sink is shared by multiple CPUs, e.g. a Perf
session with CPU mode or system wide mode, then we cannot arbitrarily
disable sink and update buffer, otherwise, it might cause hardware
lockup issue.

For per-thread mode, the refcount == 1, we can take chance to update
buffer, right?

Thanks,
Leo