linux-kernel - Re: [PATCH v3 4/6] coresight: perf: Support AUX trace pause and resume

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: <20250402123151.GH115840@e132581.arm.com>
Date: Wed, 2 Apr 2025 13:31:51 +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 4/6] coresight: perf: Support AUX trace pause and
 resume

Hi Mike,

On Wed, Apr 02, 2025 at 09:45:20AM +0100, Mike Leach wrote:

[...]

> > > >  static void etm_event_start(struct perf_event *event, int flags)
> > > >  {
> > > >         int cpu = smp_processor_id();
> > > > @@ -463,6 +484,14 @@ static void etm_event_start(struct perf_event *event, int flags)
> > > >         if (!csdev)
> > > >                 goto fail;
> > > >
> > >
> > > Is it possible here that the first call to etm_event_start() also has
> > > the PERF_EF_RESUME flag set?
> >
> > The first call has a flow below, using flag 0 but not PERF_EF_RESUME.
> >
> >   etm_event_add()
> >     `>  etm_event_start(event, 0);
> >
> 
> When I looked at the vague comments in the perf source - it seemed to
> imply that ->start() calls could overlap - so the associated event
> that resumes trace could occur at the same time as the initialising
> start from paused for the trace operations.

Good point.  A subtle but important thing is the 'cs_etm' event must be
an event group leader, otherwise, the tool reports error:

  # perf record -m,64M -e cycles/aux-action=pause,period=10000000/ \
      -e cycles/aux-action=resume,period=10/ \
      -e cs_etm/aux-action=start-paused/u -- /mnt/sort
  Events with aux-action must have AUX area event group leader

If the 'cs_etm' event is the event group leader, it will be always
enabled ahead other PMU events.  So etm_event_start(event, 0) is
invoked prior to PMU events enabling.  As a result, this can avoid the
race condition you mentioned.

I confirmed with ftrace log:

  sort-901     [005] d..3.  1033.827186: etm_event_add <-event_sched_in
  sort-901     [005] d..3.  1033.827187: etm_event_start <-etm_event_add
  sort-901     [005] d..3.  1033.827283: armpmu_add <-event_sched_in
  sort-901     [005] d..3.  1033.827287: armpmu_start <-armpmu_add
  sort-901     [005] d..3.  1033.827288: armpmu_event_set_period <-armpmu_start
  sort-901     [005] d..3.  1033.827292: armpmu_add <-event_sched_in
  sort-901     [005] d..3.  1033.827293: armpmu_start <-armpmu_add
  sort-901     [005] d..3.  1033.827294: armpmu_event_set_period <-armpmu_start
  sort-901     [005] d..3.  1033.827298: armpmu_filter <-visit_groups_merge.constprop.0.isra.0
  sort-901     [005] d..3.  1033.827298: armpmu_enable <-perf_pmu_enable
  sort-901     [005] d..3.  1033.827301: armpmu_enable <-perf_pmu_enable
  sort-901     [005] d.h1.  1033.827304: armpmu_dispatch_irq <-__handle_irq_event_percpu
  sort-901     [005] d.h1.  1033.827306: armpmu_event_update <-armv8pmu_handle_irq
  sort-901     [005] d.h1.  1033.827308: armpmu_event_set_period <-armv8pmu_handle_irq
  sort-901     [005] d.h..  1033.827322: perf_event_aux_pause: event=ffff000207503e40 pause=0

> If we are guaranteed this cannot happen then we are good to go!

Now I think we are safe, right?  ;)

Thanks,
Leo