linux-kernel - Re: [GICv3 ITS]S2IDLE framework does not invoke syscore

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: <CAPDyKFrxK3Mx055hx+a4SP3CWDpWP+CEHxz+WJfT+RficK0_Ag@mail.gmail.com>
Date: Tue, 1 Apr 2025 14:45:43 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Youngmin Nam <youngmin.nam@...sung.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Saravana Kannan <saravanak@...gle.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	kernel-team@...roid.com, hajun.sung@...sung.com, d7271.choe@...sung.com, 
	joonki.min@...sung.com
Subject: Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3
 ITS driver

On Thu, 27 Mar 2025 at 09:25, Marc Zyngier <maz@...nel.org> wrote:
>
> On Thu, 27 Mar 2025 03:22:19 +0000,
> Youngmin Nam <youngmin.nam@...sung.com> wrote:
> >
> > [1  <text/plain; utf-8 (8bit)>]
> > On Wed, Mar 26, 2025 at 08:59:02AM +0000, Marc Zyngier wrote:
> > > On Wed, 26 Mar 2025 03:09:37 +0000,
> > > Youngmin Nam <youngmin.nam@...sung.com> wrote:
> > > >
> > > > Hi.
> > > >
> > > > On our SoC, we are using S2IDLE instead of S2R as a system suspend mode.
> > > > However, when I try to enable ARM GICv3 ITS driver (drivers/irqchip/irq-gic-v3-its.c),
> > > > I noticed that there is no proper way to invoke suspend/resume callback,
> > > > because it only uses syscore_ops, which is not called in an s2idle scenario.
> > >
> > > This is *by design*.
>
> [...]
>
> > > > How should we handle this situation ?
> > >
> > > By implementing anything related to GIC power-management in your EL3
> > > firmware. Only your firmware knows whether you are going into a state
> > > where the GIC (and the ITS) is going to lose its state (because power
> > > is going to be removed) or if the sleep period is short enough that
> > > you can come back from idle without loss of context.
> > >
> > > Furthermore, there is a lot of things that non-secure cannot do when
> > > it comes to GIC power management (most the controls are secure only),
> > > so it is pretty clear that the kernel is the wrong place for this.
> > >
> > > I'd suggest you look at what TF-A provides, because this is not
> > > exactly a new problem (it has been solved several years ago).
> > >
> > >     M.
> > >
> > > --
> > > Without deviation from the norm, progress is not possible.
> > >
> >
> > Hi Marc,
> >
> > First of all, I’d like to distinguish between the GICv3 driver (irq-gic-v3.c)
> > and the ITS driver (irq-gic-v3-its.c).
> >
> > I now understand why the GICv3 driver doesn’t implement suspend and resume functions.
> > However, unlike the GICv3 driver, the ITS driver currently provides
> > suspend and resume functions via syscore_ops in the kernel.
>
> For *suspend*. The real suspend. Not a glorified WFI. And that's only
> for situations where we know for sure that we are going to suspend.
>
> > And AFAIK, LPIs are always treated as non-secure. (Please correct me If I'm wrong).
> >
> > The problem is that syscore_ops is not invoked during the S2IDLE scenario,
> > so we cannot rely on it in that context.
> > We would like to use these suspend/resume functions during S2IDLE as well.
>
> Again, this is *by design*. There is no semantic difference between
> s2idle and normal idle. They are the same thing. Do you really want to
> save/restore the whole ITS state on each and every call into idle?
> Absolutely not.

I agree that we don't want to save/restore for every call to idle,
that would simply be unnecessary and add latencies.

Instead, I think the save/restore could depend on what idlestate we
enter and whether it's a system-wide state (s2idle/s2ram) or just
regular cpuidle-state.

Today, we are pointing the callbacks for cpuidle and s2idle to the
same functions (at least for PSCI PC mode), but it's easy to change
that *if* we need some differentiation between s2idle and cpuidle.

>
> Only your firmware knows how deep you will be suspended, and how long
> you will be suspended for, and this is the right place for to perform
> save/restore of the ITS state. Not in generic code that runs on every
> arm64 platform on the planet.

Assuming we can make the code for saving/restoring generic (not in FW)
and that we are able to make sure the code is only executed for those
platforms and states that really need it. Do you think there would
there be any other drawback for doing this?

Kind regards
Uffe