Re: [PATCH v8 05/13] xen/arm: gic-v3: add ITS suspend/resume support

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> 
> On Fri, Apr 24, 2026 at 1:54 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>> 
>> Hi Mykola,
>> 
>>> On 2 Apr 2026, at 11:45, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
>>> 
>>> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
>>> 
>>> Handle system suspend/resume for GICv3 with an ITS present so LPIs keep
>>> working after firmware powers the GIC down. Snapshot the CPU interface,
>>> distributor and last-CPU redistributor state,

“Snapshot the CPU interface, distributor and last-CPU redistributor state” 
happened in the commit before?

>>> disable the ITS to cache its
>>> CTLR/CBASER/BASER registers, then restore everything and re-arm the
>>> collection on resume.
>>> 
>>> Add list_for_each_entry_continue_reverse() in list.h for the ITS suspend
>>> error path that needs to roll back partially saved state.
>>> 
>>> Based on Linux commit dba0bc7b76dc ("irqchip/gic-v3-its: Add ability to 
>>> save/restore ITS state")
>>> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
>>> ---
[…]
> 
>> 
>>> +    {
>>> +        unsigned int i;
>>> +        void __iomem *base = its->its_base;
>>> +
>>> +        its->suspend_ctx.ctlr = readl_relaxed(base + GITS_CTLR);
>>> +        ret = gicv3_disable_its(its);
>> 
>> This is called from system_suspend(), along the path iommu_suspend and
>> console_suspend() are called, finally reaching gic_suspend() and this one.
>> 
>> In the IHI 0069H.b, 5.6.2 Disabling an ITS, it says:
>> “Ensure that all interrupts that target the ITS that is being powered down 
>> are
>> either redirected or disabled”, is it correct to assume all the ITS 
>> targeting source
>> at this point are disabled because domains should be already suspended?
> 
> Yes, that is the assumption here.
> 
> Before Xen reaches this path, each domain must already have entered
> SHUTDOWN_suspend. In other words, the guest OS has already requested
> SYSTEM_SUSPEND only after completing its own suspend flow, so the
> ITS-targeting interrupt sources owned by that OS are expected to be
> quiesced at this point.
> 
> So this code relies on the owning OS having disabled or otherwise
> quiesced those sources before issuing SYSTEM_SUSPEND, rather than Xen
> explicitly doing that in gicv3_its_suspend().

Ok! I would be for a comment stating this assumption, unless the maintainers 
disagree

> 
>> 
>> 
>>> +        if ( ret )
>>> +        {
>>> +            writel_relaxed(its->suspend_ctx.ctlr, base + GITS_CTLR);
>> 
>> here and in the other places we write GITS_CTLR, this reg has Quiescent as 
>> RO,
>> maybe we should mask the write to only the other bits that are writable?
> 
> Yes, this was inherited from the Linux ITS suspend/resume code, which restores
> the saved GITS_CTLR value directly.
> 
> That said, masking the write to the writable bits is cleaner, and I will do
> that in the next version.

ok

Cheers,
Luca