Re: [PATCH v7 04/12] xen/arm: gic-v3: add ITS suspend/resume support

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

Thanks for the review.

On Sat, Jan 31, 2026 at 7:00 PM Oleksandr Tyshchenko
<olekstysh@xxxxxxxxx> wrote:
>
>
>
> On 11.12.25 20:43, Mykola Kvach wrote:
>
> Hello Mykola
>
> > 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, 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>
> > ---
> >   xen/arch/arm/gic-v3-its.c             | 91 +++++++++++++++++++++++++++
> >   xen/arch/arm/gic-v3.c                 | 15 ++++-
> >   xen/arch/arm/include/asm/gic_v3_its.h | 23 +++++++
> >   xen/include/xen/list.h                | 14 +++++
> >   4 files changed, 140 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> > index 34833166ad..08a3d8d1ef 100644
> > --- a/xen/arch/arm/gic-v3-its.c
> > +++ b/xen/arch/arm/gic-v3-its.c
> > @@ -1209,6 +1209,97 @@ int gicv3_its_init(void)
> >       return 0;
> >   }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +int gicv3_its_suspend(void)
> > +{
> > +    struct host_its *its;
> > +    int ret;
> > +
> > +    list_for_each_entry(its, &host_its_list, entry)
> > +    {
> > +        unsigned int i;
> > +        void __iomem *base = its->its_base;
> > +
> > +        its->suspend_ctx.ctlr = readl_relaxed(base + GITS_CTLR);
> > +        ret = gicv3_disable_its(its);
> > +        if ( ret )
> > +        {
> > +            writel_relaxed(its->suspend_ctx.ctlr, base + GITS_CTLR);
> > +            goto err;
> > +        }
> > +
> > +        its->suspend_ctx.cbaser = readq_relaxed(base + GITS_CBASER);
> > +
> > +        for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> > +            uint64_t baser = readq_relaxed(base + GITS_BASER0 + i * 8);
> > +
> > +            if ( !(baser & GITS_VALID_BIT) )
> > +                continue;
> > +
> > +            its->suspend_ctx.baser[i] = baser;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +
> > + err:
> > +    list_for_each_entry_continue_reverse(its, &host_its_list, entry)
> > +        writel_relaxed(its->suspend_ctx.ctlr, its->its_base + GITS_CTLR);
> > +
> > +    return ret;
> > +}
> > +
> > +void gicv3_its_resume(void)
> > +{
> > +    struct host_its *its;
> > +    int ret;
> > +
> > +    list_for_each_entry(its, &host_its_list, entry)
> > +    {
> > +        void __iomem *base;
> > +        unsigned int i;
> > +
> > +        base = its->its_base;
> > +
> > +        /*
> > +         * Make sure that the ITS is disabled. If it fails to quiesce,
> > +         * don't restore it since writing to CBASER or BASER<n>
> > +         * registers is undefined according to the GIC v3 ITS
> > +         * Specification.
> > +         *
> > +         * Firmware resuming with the ITS enabled is terminally broken.
> > +         */
> > +        WARN_ON(readl_relaxed(base + GITS_CTLR) & GITS_CTLR_ENABLE);
> > +        ret = gicv3_disable_its(its);
> > +        if ( ret )
> > +            continue;
>
> If ITS fails to disable (quiesce), the code skips restoration and ITS
> remains in an unconfigured state. However, immediately after the loop ...
>
>
> > +
> > +        writeq_relaxed(its->suspend_ctx.cbaser, base + GITS_CBASER);
> > +
> > +        /*
> > +         * Writing CBASER resets CREADR to 0, so make CWRITER and
> > +         * cmd_write line up with it.
>
> NIT: The variable "cmd_write" does not exist in the Xen driver. As I
> understand, this comment was ported from the Linux kernel driver as is,
> which maintains a software shadow copy of the write pointer named
> "cmd_write".

Good catch, thanks.
You’re right - cmd_write doesn’t exist in the Xen driver; this comment
was ported from Linux. I’ll drop the cmd_write mention and keep the
comment focused on aligning CWRITER with the reset CREADR.

>
>
> > +         */
> > +        writeq_relaxed(0, base + GITS_CWRITER);
> > +
> > +        /* Restore GITS_BASER from the value cache. */
> > +        for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> > +            uint64_t baser = its->suspend_ctx.baser[i];
> > +
> > +            if ( !(baser & GITS_VALID_BIT) )
> > +                continue;
> > +
> > +            writeq_relaxed(baser, base + GITS_BASER0 + i * 8);
> > +        }
> > +        writel_relaxed(its->suspend_ctx.ctlr, base + GITS_CTLR);
> > +    }
> > +
> > +    ret = gicv3_its_setup_collection(smp_processor_id());
>
>   ... this function iterates over all host ITS instances (including the
> one we skipped) and attempts to send MAPC commands. I am afraid, that
> attempting to write to the command queue of an uninitialized/unrestored
> ITS might have bad consequences.

Good catch, thanks.
Since gicv3_its_setup_collection() iterates over all host ITS instances,
it may send MAPC commands through an ITS that we intentionally skipped or
failed to restore. That’s unsafe. I’ll fix this by invoking collection
setup only for ITS instances successfully restored in this loop.

Best regards,
Mykola



>
> > +    if ( ret )
> > +        panic("GICv3: ITS: resume setup collection failed: %d\n", ret);
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> >
>
> [snip]
>