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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

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

> +}
> +
> +void gicv3_its_resume(void)
> +{
> +    struct host_its *its;
> +    unsigned int cpu = smp_processor_id();
> +    int ret;
> +
> +    list_for_each_entry( its, &host_its_list, entry )
> +    {
> +        ret = gicv3_its_resume_single(its, cpu);
> +        if ( ret )
> +            panic("GICv3: ITS@%"PRIpaddr": failed to restore during resume: 
> %d\n",
> +                   its->addr, ret);
> +    }
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> 

Cheers,
Luca