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: <20250408221147.GA2187207@tiffany>
Date: Wed, 9 Apr 2025 07:11:47 +0900
From: Donghyeok Choe <d7271.choe@...sung.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: Youngmin Nam <youngmin.nam@...sung.com>, Marc Zyngier <maz@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>, Saravana Kannan
	<saravanak@...gle.com>, Sudeep Holla <sudeep.holla@....com>, Ulf Hansson
	<ulf.hansson@...aro.org>, 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, joonki.min@...sung.com,
	ne.yoo@...sung.com, Donghyeok Choe <d7271.choe@...sung.com>
Subject: Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in
 GICv3 ITS driver

On Tue, Apr 08, 2025 at 11:46:40AM +0100, Sudeep Holla wrote:
> On Tue, Apr 08, 2025 at 07:51:46AM +0900, Donghyeok Choe wrote:
> > On Mon, Apr 07, 2025 at 10:17:43AM +0100, Sudeep Holla wrote:
> > > On Fri, Apr 04, 2025 at 01:13:23PM +0900, Donghyeok Choe wrote:
> > > > On Thu, Apr 03, 2025 at 10:18:54AM +0100, Sudeep Holla wrote:
> > > > > /me more confused.
> > > > > 
> > > > > Are you saying you have some cpuidle platform specific logic inside
> > > > > trace_android_vh_cpuidle_psci_enter(). I would assume it was just to
> > > > > trace the entry into the state and nothing more.
> > > > 
> > > > If you have any further questions, feel free to reach out.
> > > > 
> > > 
> > > I was trying to understand the difference in behaviour between normal
> > > cpuidle entering the same deepest state that is entered in s2idle state.
> > > I assume GIC doesn't loose power and no need for GIC ITS save/restore
> > > in normal cpuidle path ?
> > >
> > > If so, what triggers the GIC suspend in s2idle path if syscore_ops is
> > > not getting called ?
> > >
> > > Why would the firmware pull the plug on GIC ?
> > 
> > The GIC loses power. It is powered down to the same level as during suspend.
> > Therefore, it became necessary to perform GIC ITS save/restore through
> > a method other than the GIC ITS syscore path.
> > To help with better understanding, I will write a pseudo code.
> > 
> > void mimic_syscore_suspend()
> > {
> > 	/* Perform the actions required to power off all cores. */
> > 	...
> > 	its_save_disable();
> > }
> > 
> > void android_vh_cpuidle_psci_enter_handler(... bool s2idle)
> > {
> > 
> > 	if (!s2idle)
> > 		return;
> > 
> > 	set_cpu_powerdown_mark();
> > 
> > 	if (cpu != booting core)
> > 		return;
> > 
> > 	/* only booting core here */
> > 	mimic_syscore_suspend()
> > }
> > 
> > void mimic_syscore_resume()
> > {
> > 	...
> > 	its_restore_enable();
> > }
> > 
> > void android_vh_cpuidle_psci_exit_handler(... bool s2idle)
> > {
> > 	if (!s2idle)
> > 		return;
> > 
> > 	if (cpu == booting core)
> > 		mimic_syscore_resume();
> > 
> > 	set_cpu_poweron_mark();
> > }
> > 
> > All cores will be marked as powered down when the HVC/SMC call for
> > CPU suspend is invoked. When all cores call the suspend function,
> > the firmware will recognize the powerdown mark and transition
> > the system into suspend. At this point, the entire GIC will also
> > be powered off.
> > In a cpuidle situation that is not s2idle, the cores do not mark
> > CPU powerdown, so the GIC ITS save/restore operation is neither
> > performed nor necessary.
> > 
> 
> OK, I understood. In short, you create problems by hacking up or misusing
> your trace handlers in ways it shouldn't be, and now you are t/crying to
> solve those problems.
> 
> > > Do you use any suspend/resume logic in drivers/irqchip/irq-gic-pm.c ?
> > No, there are parts of the GIC that require secure access, so the
> > GIC save/restore is performed by the firmware.
> > Since the GIC-ITS is entirely controlled as a non-secure IP,
> > I think it is more efficient to perform save/restore in the kernel.
> > 
> 
> I can understand that part, but my hacking up things the way you have
> shown above, though you may think you have achieved some feature very
> smartly, you have just dug up the hole with issues you are facing now.
> 
> The only reason IIUC s2idle info is used is to identify when the RPM
> is disabled. You are using that info to manage GIC power state.
> 
> The CPU deepest idle states entered in the normal and s2idle must be
> same. If you want to still achieve extra power save with GIC powerdown
> make it completely transparent to the OS.

First of all, thank you.
You're clearly pointing out the painful truth.
I’m well aware that this approach isn’t ideal.

Following your advice, I’ll do my best to keep things
as clear and transparent as possible.
My goal was to explain the situation we’re facing, and I feel
like I’ve managed to do that — so I’m glad about that.

Best regards,  
Donghyeok Choe