Re: [PATCH v12 02/13] xen/arm: gic-v2: Implement GIC suspend/resume functions

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

> On 25 Sep 2026, at 00:23, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> Thank you for the review.
> 
> On Wed, Sep 23, 2026 at 6:28 PM Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx> wrote:
>> 
>> Hi Mykola,
>> 
>> Sorry for the delay to review this serie.
>> 
>>> On 27 Aug 2026, at 16:31, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
>>> 
>>> From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>>> 
>>> System suspend may lead to a state where GIC would be powered down.
>>> Therefore, Xen should save/restore the context of GIC on suspend/resume.
>>> 
>>> Note that the context consists of states of registers which are
>>> controlled by the hypervisor. Other GIC registers which are accessible
>>> by guests are saved/restored on context switch.
>>> 
>>> Transient physical SGI pending state (GICD_CPENDSGIRn/GICD_SPENDSGIRn)
>>> is intentionally excluded. CPU-interface active-priority state is also
>>> not restored across suspend/resume. Xen reaches the final suspend path
>>> at a quiescent point, so there is no active-priority execution context
>>> to replay after resume. Enforce this with a runtime check after
>>> disabling the CPU interface: if any implemented GICC_APRn word is still
>>> non-zero, restore GICC_CTLR and abort suspend with -EBUSY.
>> 
>> You mention SGI pending state but you do not say what would happen for 
>> PPI/SPI
>> pending state, and the patch does not look at or save/restore GICD_ISPENDR.
>> 
>> Can you clarify what is expected for those?
> 
> This patch was originally based on the Linux GICv2 suspend/resume
> code, which also does not save PPI/SPI pending state. The comment
> above gic_dist_restore() explains that level interrupts still
> asserted after resume will be handled, while edge events during
> suspend need to be handled by the platform-specific wakeup
> mechanism.
> 
> Saving GICD_ISPENDR would preserve the pending state at the time of
> each read. However, an interrupt could become pending after that
> read and before the GIC loses power. Saving pending state alone
> therefore does not cover the whole suspend transition.
> 
> The assumption here is that device drivers have stopped normal I/O
> and quiesced non-wakeup interrupt sources before Xen suspends the
> GIC. Earlier events must already have been handled, or their state
> must be preserved outside the GIC. Only configured wakeup sources
> are expected to generate new events at this point.
> 
> We rely on the platform wakeup mechanism throughout suspend entry
> and sleep. If the GIC loses power, this mechanism must capture
> wakeup events outside the GIC and keep them observable after
> resume.
> 
> Not saving pending state depends on these assumptions. The race
> after a register read does not, by itself, justify losing an event
> that is already pending.

I think this would deserve a bit more explanation in the commit message
and maybe a comment in the code so the next one does not wonder if
something is needed here or not and why.

> ---
> 
> While checking the pending-state question, I also noticed a related
> issue with disabling the Distributor during suspend entry.
> 
> My earlier reasoning relied on the Distributor being powered down
> during system suspend. Not every platform is required to follow
> BSA.
> 
> PSCI requires us to save the state that could be lost. Section 6.8
> explicitly discusses Distributor power-down as a feature of some
> systems. This does not require Xen to disable its interrupt group
> before calling SYSTEM_SUSPEND.
> 
> The GICv2 pseudocode in section 3.7.2 shows that irq_wake and
> fiq_wake depend on the Distributor group enables, but not on the
> CPU interface group enables. Clearing the group enable in
> GICD_CTLR can therefore block that group's wakeup path on a
> platform that uses these signals.
> 
> I therefore propose leaving the Distributor group enabled during
> suspend entry, while still saving its configuration in case it
> loses power. The platform would handle any further shutdown and
> the required wakeup configuration. The Distributor would still
> be disabled while restoring its registers on resume.
> 
> Linux also leaves the GICv2 CPU interface enabled before the PSCI
> call. Our early gicv2_cpu_disable() is another difference: it can
> hide that group's interrupts from TF-A's early ISR_EL1 check.
> I propose leaving that shutdown to firmware as well.

Yes that would make sense.
I will let you do that on next version once i will be done with this
version of the serie.

Cheers
Bertrand

> 
> Best regards,
> Mykola