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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the detailed review.

On Thu, May 14, 2026 at 5:46 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 12 May 2026, at 18:07, 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. Save and restore the ITS
> > CTLR, CBASER and BASER registers, and re-establish the collection mapping
> > 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>
> > ---
> > Changes in V9:
> > - fix the ITS suspend/resume coding-style nits;
> > - preserve the saved GITS_CTLR state while masking the read-only
> >  QUIESCENT bit.
> >
> > Changes in V8:
> > - Reword the CBASER/CWRITER comment to match Xen and drop the stale Linux
> >  cmd_write reference.
> > - Clarify the list_for_each_entry_continue_reverse() comment.
> > - Factor out per-ITS helpers for collection setup and resume.
> > - Restore each ITS and re-establish its collection mapping in the same
> >  loop, so a failed ITS resume is not followed by MAPC/SYNC on that
> >  un-restored instance.
> > - panic in case when resume of an ITS failed
> > - cleanup baser cache during suspend
> > ---
> > xen/arch/arm/gic-v3-its.c             | 133 ++++++++++++++++++++++++--
> > xen/arch/arm/gic-v3.c                 |  11 ++-
> > xen/arch/arm/include/asm/gic_v3_its.h |  23 +++++
> > xen/include/xen/list.h                |  14 +++
> > 4 files changed, 171 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> > index 9005ce8ce5..582c26d964 100644
> > --- a/xen/arch/arm/gic-v3-its.c
> > +++ b/xen/arch/arm/gic-v3-its.c
> > @@ -335,6 +335,22 @@ static int its_send_cmd_inv(struct host_its *its,
> >     return its_send_command(its, cmd);
> > }
> >
> > +static int gicv3_its_setup_collection_single(struct host_its *its,
> > +                                             unsigned int cpu)
> > +{
> > +    int ret;
> > +
> > +    ret = its_send_cmd_mapc(its, cpu, cpu);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    ret = its_send_cmd_sync(its, cpu);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    return gicv3_its_wait_commands(its);
> > +}
> > +
> > /* Set up the (1:1) collection mapping for the given host CPU. */
> > int gicv3_its_setup_collection(unsigned int cpu)
> > {
> > @@ -343,15 +359,7 @@ int gicv3_its_setup_collection(unsigned int cpu)
> >
> >     list_for_each_entry(its, &host_its_list, entry)
> >     {
> > -        ret = its_send_cmd_mapc(its, cpu, cpu);
> > -        if ( ret )
> > -            return ret;
> > -
> > -        ret = its_send_cmd_sync(its, cpu);
> > -        if ( ret )
> > -            return ret;
> > -
> > -        ret = gicv3_its_wait_commands(its);
> > +        ret = gicv3_its_setup_collection_single(its, cpu);
> >         if ( ret )
> >             return ret;
> >     }
> > @@ -1210,6 +1218,113 @@ 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;
> > +
> > +        /*
> > +         * By the time Xen reaches gic_suspend(), every domain is already 
> > in
> > +         * SHUTDOWN_suspend, so ITS-targeting interrupt sources are 
> > expected
> > +         * to have been quiesced by the owning OS before SYSTEM_SUSPEND.
> > +         */
> > +        /* Preserve saved GITS_CTLR state, excluding read-only QUIESCENT. 
> > */
> > +        its->suspend_ctx.ctlr = readl_relaxed(base + GITS_CTLR) &
> > +                                ~GITS_CTLR_QUIESCENT;
> > +        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);
> > +
> > +            its->suspend_ctx.baser[i] = 0;
> > +
> > +            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;
> > +}
> > +
> > +static int gicv3_its_resume_single(struct host_its *its, unsigned int cpu)
> > +{
> > +    void __iomem *base = its->its_base;
> > +    unsigned int i;
> > +    int ret;
> > +
> > +    /*
> > +     * 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
>
> s/undefined/unpredictable/ ?

Ack.

>
> > +     * Specification.
> > +     */
> > +    WARN_ON(readl_relaxed(base + GITS_CTLR) & GITS_CTLR_ENABLE);
> > +    ret = gicv3_disable_its(its);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    writeq_relaxed(its->suspend_ctx.cbaser, base + GITS_CBASER);
> > +
> > +    /*
> > +     * Writing CBASER resets CREADR to 0, so reset CWRITER to
> > +     * keep the command queue pointers aligned.
> > +     */
> > +    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);
> > +
> > +    return gicv3_its_setup_collection_single(its, cpu);
>
> This will always issue a MAPC V=1, in the section 5.3.9 it sais it’s 
> "unpredictable
> if there are interrupts that are mapped to the specified collection and the
> collection is currently mapped to a Redistributor, unless MAPC is followed by 
> MOVALL”,
> in this case the redistributor is the same but the specs don’t say anything 
> about this case,
> it’s generally unpredictable if we are remapping an already-live collection.
>
> I see Linux reply the MAPC V=1 only if the collection is stored in the ITS 
> (not memory backed),
> our col_id is `cpu`, which I believe that for the suspend path is always zero 
> (?), so by looking into
> HCC we could check if we need to issue the MAPC or not.
>
> if ( cpu < GITS_TYPER_HCC(readq_relaxed(base + GITS_TYPER)) )
>     return gicv3_its_setup_collection_single(its, cpu);
>
> return 0;
>

Good point, I agree.

Replaying MAPC unconditionally is not needed here and may be unsafe for
memory-backed collections. Since Xen currently uses col_id == cpu, I will
add the HCC check before calling gicv3_its_setup_collection_single(), as
you suggested.

I will also add a short comment to make clear that the check is about the
CollectionID being ITS-held.

Best regards,
Mykola