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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Fri, May 8, 2026 at 2:31 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> 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?

Yes, fair point.

That wording is too broad for this patch. It describes the wider GICv3
suspend/resume flow in which the ITS handling is invoked, rather than the
ITS-specific part added here.

The CPU interface, distributor and redistributor handling are covered by
the related GICv3 suspend/resume patches, while this patch itself adds the
ITS state save/restore.

I will tighten the commit message in the next version so it only describes
the ITS-specific suspend/resume handling done by this patch.

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

Ack.

Best regards,
Mykola

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