linux-kernel - Re: [PATCH] coresight: trbe: Save/restore state across CPU low power state

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: <CAJ9a7VjhCRk9BxGvE3ZyyZzq__+A1PpD=oRtFQOcD8BwXKa27Q@mail.gmail.com>
Date: Mon, 28 Apr 2025 13:55:29 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: Yabin Cui <yabinc@...gle.com>, Anshuman Khandual <anshuman.khandual@....com>, 
	Suzuki K Poulose <suzuki.poulose@....com>, James Clark <james.clark@...aro.org>, 
	Jie Gan <quic_jiegan@...cinc.com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, coresight@...ts.linaro.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] coresight: trbe: Save/restore state across CPU low power state

Hi Leo

On Mon, 28 Apr 2025 at 13:24, Leo Yan <leo.yan@....com> wrote:
>
> Hi all,
>
> On Mon, Apr 28, 2025 at 11:53:26AM +0100, Mike Leach wrote:
>
> [...]
>
> > > > > @@ -362,6 +362,10 @@ enum cs_mode {
> > > > >   * @alloc_buffer:    initialises perf's ring buffer for trace collection.
> > > > >   * @free_buffer:     release memory allocated in @get_config.
> > > > >   * @update_buffer:   update buffer pointers after a trace session.
> > > > > + * @percpu_save:     saves state when CPU enters idle state.
> > > > > + *                   Only set for percpu sink.
> > > > > + * @percpu_restore:  restores state when CPU exits idle state.
> > > > > + *                   only set for percpu sink.
> > > > >   */
> > > > >  struct coresight_ops_sink {
> > > > >       int (*enable)(struct coresight_device *csdev, enum cs_mode mode,
> > > > > @@ -374,6 +378,8 @@ struct coresight_ops_sink {
> > > > >       unsigned long (*update_buffer)(struct coresight_device *csdev,
> > > > >                             struct perf_output_handle *handle,
> > > > >                             void *sink_config);
> > > > > +     void (*percpu_save)(struct coresight_device *csdev);
> > > > > +     void (*percpu_restore)(struct coresight_device *csdev);
> > > >
> > > > Again - why this percpu_* prefix ?
> > > >
> > > > >  };
> > > > >
> > > > >  /**
> >
> > I do not think this is the best approach.
> >
> > The TRBE driver has its own power management registration functions,
> > so is it not possible for the save and restore should be handled
> > there, through a PM notifier, just as the ETM/ETE is?
> >
> > Drop the unnecessary DT entry - TRBE is a per cpu architectural device
> > - if a TRBE is present, we know it will power down with the PE.
> >
> > The CoreSight architecture permits an ETE to drive trace to an
> > external sink - so the TRBE might be present but unused, therefore
> > hooking into a source driver that may not be driving trace into the
> > device does not seem wise..
>
> Sorry I jumped in a bit late (I saw the patch at last week and it is
> on my review list).
>
> I would suggest to hold on for this patch.  I am refactoring CPU PM and
> hotplug in CoreSight based on the CoreSight path.
>
> The idea is when we do CPU power management for CoreSight devices, we
> cannot simply control individual devices.  Alternatively, we need to
> control logics based on the linkages on CoreSight path as it can reflect
> dependencies crossing the components.  For example, for a CoreSight
> path, when running into CPU low power state, we need firstly disable
> tracer, then links, and at the end sinks.  When CPU resumes back, we
> need to use the reversed ordering for turning on devices.
>
> As a result, except the tracers (ETM / ETE) should be saved and
> restored contexts during CPU power states, other components on the
> path will be controlled by traversing CoreSight paths.  The benefit is
> if a component (e.g., a link or a sink module) is shared by multiple
> CoreSight paths, then the component can be disabled or enabled based on
> reference counter.
>
> I know I am a bit lagged - as I also need to support the locking on
> CoreSight paths.  Please expect in next 1~2 weeks I will send out
> patches for public review.
>
> > The TRBE PM can follow the model of the ETE / ETMv4 and save and
> > restore if currently in use.
>
> If TRBE PM is registered as a seperate PM notifier, a prominent issue is
> it cannot promise the depedency between ETE and TRBE when execute CPU
> power management.  E.g., when entering CPU idle states, ETE should be
> disabled prior to switch off TRBE, otherwise, it might cause lockup
> issue in hardware.  If ETE and TRBE register PM notifier separately,
> we cannot ensure the sequence between ETE and TRBE, this is why we need
> to do the operations based on CoreSight paths.
>

I believe that the architecture requires that if the disabled TRBE
cannot receive trace then the ETE should regard the trace as having
been output (A1.4 ETE spec). Incorrect sequencing should only result
in missing trace, not a core lockup - unless the implementation is not
compliant.

Mike

> Thanks,
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK