[GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

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.
Please refer to the codes below.

<drivers/irqchip/irq-gic-v3-its.c>
5028 static struct syscore_ops its_syscore_ops = {
5029         .suspend = its_save_disable,
5030         .resume = its_restore_enable,
5031 };
...
5803         register_syscore_ops(&its_syscore_ops);

<kernel/power/suspend.c>
444         if (state == PM_SUSPEND_TO_IDLE) {
445                 s2idle_loop();
446                 goto Platform_wake;
447         }
448
449         error = pm_sleep_disable_secondary_cpus();
450         if (error || suspend_test(TEST_CPUS)) {
451                 log_suspend_abort_reason("Disabling non-boot cpus failed");
452                 goto Enable_cpus;
453         }
454
455         arch_suspend_disable_irqs();
456         BUG_ON(!irqs_disabled());
457
458         system_state = SYSTEM_SUSPEND;
459
460         error = syscore_suspend();

How should we handle this situation ?

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Marc Zyngier]

On Wed, 26 Mar 2025 03:09:37 +0000,
Youngmin Nam <youngmin.nam@samsung.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*.

> Please refer to the codes below.
> 
> <drivers/irqchip/irq-gic-v3-its.c>
> 5028 static struct syscore_ops its_syscore_ops = {
> 5029         .suspend = its_save_disable,
> 5030         .resume = its_restore_enable,
> 5031 };
> ...
> 5803         register_syscore_ops(&its_syscore_ops);
> 
> <kernel/power/suspend.c>
> 444         if (state == PM_SUSPEND_TO_IDLE) {
> 445                 s2idle_loop();
> 446                 goto Platform_wake;
> 447         }
> 448
> 449         error = pm_sleep_disable_secondary_cpus();
> 450         if (error || suspend_test(TEST_CPUS)) {
> 451                 log_suspend_abort_reason("Disabling non-boot cpus failed");
> 452                 goto Enable_cpus;
> 453         }
> 454
> 455         arch_suspend_disable_irqs();
> 456         BUG_ON(!irqs_disabled());
> 457
> 458         system_state = SYSTEM_SUSPEND;
> 459
> 460         error = syscore_suspend();
> 
> 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.

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]

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@samsung.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*.
> 
> > Please refer to the codes below.
> > 
> > <drivers/irqchip/irq-gic-v3-its.c>
> > 5028 static struct syscore_ops its_syscore_ops = {
> > 5029         .suspend = its_save_disable,
> > 5030         .resume = its_restore_enable,
> > 5031 };
> > ...
> > 5803         register_syscore_ops(&its_syscore_ops);
> > 
> > <kernel/power/suspend.c>
> > 444         if (state == PM_SUSPEND_TO_IDLE) {
> > 445                 s2idle_loop();
> > 446                 goto Platform_wake;
> > 447         }
> > 448
> > 449         error = pm_sleep_disable_secondary_cpus();
> > 450         if (error || suspend_test(TEST_CPUS)) {
> > 451                 log_suspend_abort_reason("Disabling non-boot cpus failed");
> > 452                 goto Enable_cpus;
> > 453         }
> > 454
> > 455         arch_suspend_disable_irqs();
> > 456         BUG_ON(!irqs_disabled());
> > 457
> > 458         system_state = SYSTEM_SUSPEND;
> > 459
> > 460         error = syscore_suspend();
> > 
> > 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.
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.

Thanks.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Marc Zyngier]

On Thu, 27 Mar 2025 03:22:19 +0000,
Youngmin Nam <youngmin.nam@samsung.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@samsung.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.

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.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

On Thu, Mar 27, 2025 at 08:25:14AM +0000, Marc Zyngier wrote:
> On Thu, 27 Mar 2025 03:22:19 +0000,
> Youngmin Nam <youngmin.nam@samsung.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@samsung.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.
> 
> 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.
> 
> 	M.

Thank you for the clear explanation. I completely understand now.

> 
> -- 
> Without deviation from the norm, progress is not possible.
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Ulf Hansson]

On Thu, 27 Mar 2025 at 09:25, Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 27 Mar 2025 03:22:19 +0000,
> Youngmin Nam <youngmin.nam@samsung.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@samsung.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

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Marc Zyngier]

On Tue, 01 Apr 2025 13:45:43 +0100,
Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> On Thu, 27 Mar 2025 at 09:25, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 27 Mar 2025 03:22:19 +0000,
> > Youngmin Nam <youngmin.nam@samsung.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@samsung.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?

Yes. We'd end-up having to implement all sort of split PM schemes
depending on the GIC implementation, what the firmware does, the
various braindead assumptions that the integration makes, and other
parameters I don't even want to consider.

The GIC power management is, for better or worse, *outside* of the
scope of the architecture. Most of it is implementation defined,
because each and every implementer/vendor sees it as added value to
invent their own particular flavour of crap. For example, there is no
provision for wake-up interrupts, because nobody can agree on how
that's supposed to work.

Do we want to deal with this in the various GIC drivers? No. It is the
job of firmware to manage this mess, because this clearly delineates
where the responsibilities lie.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Ulf Hansson]

On Tue, 1 Apr 2025 at 15:11, Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 01 Apr 2025 13:45:43 +0100,
> Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 27 Mar 2025 at 09:25, Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Thu, 27 Mar 2025 03:22:19 +0000,
> > > Youngmin Nam <youngmin.nam@samsung.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@samsung.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?
>
> Yes. We'd end-up having to implement all sort of split PM schemes
> depending on the GIC implementation, what the firmware does, the
> various braindead assumptions that the integration makes, and other
> parameters I don't even want to consider.

I don't think it needs to be that complicated, at all. But let's not
discuss the solution at this point, at least for me, that's too early.

However, I do understand your concerns and share them.

>
> The GIC power management is, for better or worse, *outside* of the
> scope of the architecture. Most of it is implementation defined,
> because each and every implementer/vendor sees it as added value to
> invent their own particular flavour of crap. For example, there is no
> provision for wake-up interrupts, because nobody can agree on how
> that's supposed to work.

Right. I guess it falls in the SoC specific area and we need to live
with it, for now.

Anyway, the main reason why I joined the discussion is exactly because
of this. I have been working on enabling the same deep state for
s2idle as the one that corresponds to s2ram for some legacy arm64
platforms. To allow the system to wake up properly from this deep
state, I needed to save/restore these types of GIC registers.

I intend to post a complete series for this soonish. It should show
what is needed for a particular SoC in this regard. I will keep you
posted.

>
> Do we want to deal with this in the various GIC drivers? No. It is the
> job of firmware to manage this mess, because this clearly delineates
> where the responsibilities lie.

The FW could deal with this, but that would only work for platforms
with new or upgrade-able FW, which is not the case for these legacy
platforms that I am working on.

Moreover, we already implement the save/restore for some GIC variants
- and in some cases using different ways to do it. In my opinion it
would be nice to have a common solution that would only be enabled for
the states/platforms that really need it. In the series above I will
try to implement this, let's see if I can make it.

Kind regards
Uffe

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Marc Zyngier]

On Wed, 02 Apr 2025 11:57:31 +0100,
Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> On Tue, 1 Apr 2025 at 15:11, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 01 Apr 2025 13:45:43 +0100,
> > Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > 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?
> >
> > Yes. We'd end-up having to implement all sort of split PM schemes
> > depending on the GIC implementation, what the firmware does, the
> > various braindead assumptions that the integration makes, and other
> > parameters I don't even want to consider.
> 
> I don't think it needs to be that complicated, at all. But let's not
> discuss the solution at this point, at least for me, that's too early.
> 
> However, I do understand your concerns and share them.
> 
> >
> > The GIC power management is, for better or worse, *outside* of the
> > scope of the architecture. Most of it is implementation defined,
> > because each and every implementer/vendor sees it as added value to
> > invent their own particular flavour of crap. For example, there is no
> > provision for wake-up interrupts, because nobody can agree on how
> > that's supposed to work.
> 
> Right. I guess it falls in the SoC specific area and we need to live
> with it, for now.
>
> Anyway, the main reason why I joined the discussion is exactly because
> of this. I have been working on enabling the same deep state for
> s2idle as the one that corresponds to s2ram for some legacy arm64
> platforms. To allow the system to wake up properly from this deep
> state, I needed to save/restore these types of GIC registers.

Or not. I will *NOT* entertain SoC-specific code in the GIC drivers
for anything that isn't a workaround for a functional erratum.

> 
> I intend to post a complete series for this soonish. It should show
> what is needed for a particular SoC in this regard. I will keep you
> posted.
> 
> >
> > Do we want to deal with this in the various GIC drivers? No. It is the
> > job of firmware to manage this mess, because this clearly delineates
> > where the responsibilities lie.
> 
> The FW could deal with this, but that would only work for platforms
> with new or upgrade-able FW, which is not the case for these legacy
> platforms that I am working on.

Then these platforms can die and be pruned from the tree, or live with
a sub-par power management.

> Moreover, we already implement the save/restore for some GIC variants
> - and in some cases using different ways to do it. In my opinion it
> would be nice to have a common solution that would only be enabled for
> the states/platforms that really need it. In the series above I will
> try to implement this, let's see if I can make it.

The solution is firmware. It's been advertised as such for over 10
years, and GICv5 doesn't change this. We spent years *removing* that
crap from the irqchip subsystem so that we could have something
manageable. I'm not going to go back in time for the sake of shit HW.

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Sudeep Holla]

(Failed to find the original email, so reply on this instead)

On Thu, Mar 27, 2025 at 12:22:19PM +0900, Youngmin Nam wrote:
> 
> 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.

I have one orthogonal question. The s2idle will just use the deepest
cpuidle state registered. So if s2idle needs this save/restore of GICv3
ITS, how does that work when all the CPUs enter that idle state.

With respect to the PSCI CPU_SUSPEND call, it doesn't change. So I am
bit confused as how it can work fine in normal cpuidle paths but no in
s2idle path. What am I missing ? I do psci_enter_domain_idle_state handles
s2idle little different but nothing to change this GICv3 ITS save/restore
requirement between cpuidle and s2idle.

-- 
Regards,
Sudeep

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

On Wed, Apr 02, 2025 at 12:56:53PM +0100, Sudeep Holla wrote:
> (Failed to find the original email, so reply on this instead)
> 
> On Thu, Mar 27, 2025 at 12:22:19PM +0900, Youngmin Nam wrote:
> > 
> > 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.
> 
> I have one orthogonal question. The s2idle will just use the deepest
> cpuidle state registered. So if s2idle needs this save/restore of GICv3
> ITS, how does that work when all the CPUs enter that idle state.
> 
> With respect to the PSCI CPU_SUSPEND call, it doesn't change. So I am
> bit confused as how it can work fine in normal cpuidle paths but no in
> s2idle path. What am I missing ? I do psci_enter_domain_idle_state handles
> s2idle little different but nothing to change this GICv3 ITS save/restore
> requirement between cpuidle and s2idle.
> 
> -- 
> Regards,
> Sudeep
> 
Hi Sudeep,

Thanks for asking.
As a SoC vendor, we are using the Android kernel, which includes a vendor hook like the one below.

In this function,
a vendor-specific handler attached to trace_android_vh_cpuidle_psci_enter is called.

54 static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
55                                                     struct cpuidle_driver *drv, int idx,
56                                                     bool s2idle)
57 {
58         struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
59         u32 *states = data->psci_states;
60         struct device *pd_dev = data->dev;
61         u32 state;
62         int ret;
63
64         ret = cpu_pm_enter();
65         if (ret)
66                 return -1;
67
68         /* Do runtime PM to manage a hierarchical CPU toplogy. */
69         trace_android_vh_cpuidle_psci_enter(dev, s2idle);

Within the vendor-specific handler, if the current mode is S2IDLE and the CPU logical number is 0,
the GIC ITS suspend function is executed.

Thanks.
Youngmin

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Sudeep Holla]

On Thu, Apr 03, 2025 at 10:30:42AM +0900, Youngmin Nam wrote:
> On Wed, Apr 02, 2025 at 12:56:53PM +0100, Sudeep Holla wrote:
> > (Failed to find the original email, so reply on this instead)
> > 
> > On Thu, Mar 27, 2025 at 12:22:19PM +0900, Youngmin Nam wrote:
> > > 
> > > 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.
> > 
> > I have one orthogonal question. The s2idle will just use the deepest
> > cpuidle state registered. So if s2idle needs this save/restore of GICv3
> > ITS, how does that work when all the CPUs enter that idle state.
> > 
> > With respect to the PSCI CPU_SUSPEND call, it doesn't change. So I am
> > bit confused as how it can work fine in normal cpuidle paths but no in
> > s2idle path. What am I missing ? I do psci_enter_domain_idle_state handles
> > s2idle little different but nothing to change this GICv3 ITS save/restore
> > requirement between cpuidle and s2idle.
> > 
> Hi Sudeep,
> 
> Thanks for asking.
> As a SoC vendor, we are using the Android kernel, which includes a vendor
> hook like the one below.
> 
> In this function, a vendor-specific handler attached to
> trace_android_vh_cpuidle_psci_enter is called.
> 
> 54 static __cpuidle int __psci_enter_domain_idle_state(struct cpuidle_device *dev,
> 55                                                     struct cpuidle_driver *drv, int idx,
> 56                                                     bool s2idle)
> 57 {
> 58         struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> 59         u32 *states = data->psci_states;
> 60         struct device *pd_dev = data->dev;
> 61         u32 state;
> 62         int ret;
> 63
> 64         ret = cpu_pm_enter();
> 65         if (ret)
> 66                 return -1;
> 67
> 68         /* Do runtime PM to manage a hierarchical CPU toplogy. */
> 69         trace_android_vh_cpuidle_psci_enter(dev, s2idle);
> 
> Within the vendor-specific handler, if the current mode is S2IDLE and the
> CPU logical number is 0, the GIC ITS suspend function is executed.
> 

/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.

In fact, it was recently added upstream as well.
Commit 7b7644831e72 ("cpuidle: psci: Add trace for PSCI domain idle")

Further you didn't explicitly answer my question. IIUC are you calling
GIC ITS suspend function unconditionally if its boot cpu ? Or is it
done only for s2idle ? If done only for s2idle, how does it work for
normal cpuidle entry to deepest idle state that matches the one entered
during s2idle.

-- 
Regards,
Sudeep

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Donghyeok Choe]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

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.

Hi, I am a Samsung developer working with Youngmin Nam.

Yes, you are correct. The platform-specific logic runs, and only
the booting core performs the GIC ITS save/restore. I understand that
using trace in this way might have surprised you.
However, this kind of trace usage is actually quite common
within the Android kernel.

The Android kernel aims to ensure that all SoC vendors use
the same kernel code, aiming for the benefits of the GKI project.
Since vendors often want to control kernel behavior, they utilize
traces like this to achieve that flexibility.

> In fact, it was recently added upstream as well.
> Commit 7b7644831e72 ("cpuidle: psci: Add trace for PSCI domain idle")

Then it seems that this will be used instead of the Android kernel specific
vendor hook(trace_android_vh_cpuidle_psci_enter).
We can eliminate the diff between the mainline code and the ACK.

> Further you didn't explicitly answer my question. IIUC are you calling
> GIC ITS suspend function unconditionally if its boot cpu ? Or is it
> done only for s2idle ? If done only for s2idle, how does it work for
> normal cpuidle entry to deepest idle state that matches the one entered
> during s2idle.

In the s2idle situation, only the booting core performs
the GIC ITS suspend/resume. The difference from normal cpuidle
is that in the case of s2idle, the entire system enters suspend
at a level equivalent to kernel suspend.

Through that trace(trace_android_vh_cpuidle_psci_enter),
we don't directly call syscore suspend, but rather mimic
the actions that syscore suspend would perform.

You might be concerned about the following:
While kernel suspend hotplugs out the remaining cores except for
the booting core and performs syscore suspend, in the s2idle scenario,
cores enter cpuidle in parallel when going into suspend.
I understand that you may worry about the synchronization issues
that could arise when cores enter cpuidle in parallel during
the s2idle situation.
There were many exceptional cases, and we resolved them all together
with my colleagues. We paid the price for not using the mainline kernel
code as intended.

If you have any further questions, feel free to reach out.

Best regards,  
Donghyeok Choe

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Sudeep Holla]

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 ?

Do you use any suspend/resume logic in drivers/irqchip/irq-gic-pm.c ?
I am still missing something in this flow.

-- 
Regards,
Sudeep

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Donghyeok Choe]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

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.

> 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 am still missing something in this flow.
Feel free to ask questions until everything is clear!
As I look for answers to your questions, I appreciate your insights
and find it helpful to review the s2idle sequence we are using.

Best regards,  
Donghyeok Choe

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Marc Zyngier]

On Mon, 07 Apr 2025 23:51:46 +0100,
Donghyeok Choe <d7271.choe@samsung.com> wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Mon, Apr 07, 2025 at 10:17:43AM +0100, Sudeep Holla wrote:

> > 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.

More efficient? Give me *one* aspect of this save/restore sequence
that is done in a more efficiently way in the kernel. Dumping MMIO
accesses into memory has the exact same cost at EL1, El2 or EL3, and
splitting things along an arbitrary line to paper over bad firmware is
not a valid argument.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Donghyeok Choe]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

On Tue, Apr 08, 2025 at 07:51:32AM +0100, Marc Zyngier wrote:
> On Mon, 07 Apr 2025 23:51:46 +0100,
> Donghyeok Choe <d7271.choe@samsung.com> wrote:
> > 
> > [1  <text/plain; us-ascii (7bit)>]
> > On Mon, Apr 07, 2025 at 10:17:43AM +0100, Sudeep Holla wrote:
> 
> > > 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.
> 
> More efficient? Give me *one* aspect of this save/restore sequence
> that is done in a more efficiently way in the kernel. Dumping MMIO
> accesses into memory has the exact same cost at EL1, El2 or EL3, and
> splitting things along an arbitrary line to paper over bad firmware is
> not a valid argument.

If I had to highlight just one efficiency gain — it's that I get to
reuse well-tested and well-written save/restore code without
reinventing the wheel (or worse, reinventing a square one).
And as for bad firmware... I guess it deserves the punishment
of handling GIC ITS save/restore.

That said, I truly appreciate your insight, and I’ll make an effort
to avoid relying on such suboptimal kernel usage going forward.

Best regards,  
Donghyeok Choe

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

Re: [GICv3 ITS]S2IDLE framework does not invoke syscore_ops in GICv3 ITS driver

[Donghyeok Choe]

[-- Attachment #1: Type: text/plain, Size: 4023 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]