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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

> On 27 Aug 2026, at 16:31, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
> 
> 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. On resume,
> re-establish the collection mapping only when the collection is held in
> the ITS itself. Memory-backed collections are restored through the
> restored GITS_BASER tables and must not be remapped unconditionally.
> 
> 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>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>

Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx> # Arm code

Changes to list.h will need a review from the Others group but they look
good to me.


Cheers
Bertrand

> ---
> Changes in V10:
> - Replay MAPC on resume only for collections held in the ITS itself, as
>  indicated by GITS_TYPER.HCC. Memory-backed collections are restored
>  through GITS_BASER and are no longer remapped unconditionally.
> - Make the current Xen col_id == cpu assumption explicit in the ITS
>  resume path.
> - Use "unpredictable" instead of "undefined" in the CBASER/BASER restore
>  comment.
> 
> 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             | 146 ++++++++++++++++++++++++--
> xen/arch/arm/gic-v3.c                 |  11 +-
> xen/arch/arm/include/asm/gic_v3_its.h |  28 +++++
> xen/include/xen/list.h                |  14 +++
> 4 files changed, 189 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 7560d46c6d..dd53209865 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;
>     }
> @@ -1211,6 +1219,126 @@ 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;
> +    uint64_t typer;
> +    unsigned int col_id = cpu; /* Xen currently uses col_id == cpu. */
> +
> +    /*
> +     * 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 unpredictable according to the GIC v3 ITS
> +     * 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);
> +
> +    typer = readq_relaxed(base + GITS_TYPER);
> +
> +    /*
> +     * Only collections with IDs below HCC are held in the ITS itself
> +     * and lose their state across an ITS reset/power loss. Memory-backed
> +     * collections are restored by restoring GITS_BASER and must not be
> +     * remapped here.
> +     */
> +    if ( col_id < GITS_TYPER_HCC(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 */
> 
> /*
>  * Local variables:
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 038bf41142..6b025c023d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -2186,10 +2186,14 @@ static int gicv3_suspend(void)
>     if ( ret )
>         goto out_enable_iface;
> 
> -    ret = gicv3_disable_redist();
> +    ret = gicv3_its_suspend();
>     if ( ret )
>         goto out_enable_iface;
> 
> +    ret = gicv3_disable_redist();
> +    if ( ret )
> +        goto out_its_resume;
> +
>     /* Save GICR configuration */
>     gicv3_redist_wait_for_rwp();
> 
> @@ -2229,6 +2233,9 @@ static int gicv3_suspend(void)
> 
>     return 0;
> 
> + out_its_resume:
> +    gicv3_its_resume();
> +
>  out_enable_iface:
>     if ( gicv3_enable_redist() )
>         panic("GICv3: Failed to re-enable redistributor after suspend 
> abort\n");
> @@ -2355,6 +2362,8 @@ static void gicv3_resume(void)
> 
>     gicv3_redist_wait_for_rwp();
> 
> +    gicv3_its_resume();
> +
>     WRITE_SYSREG(gicv3_ctx.cpu.sre_el2, ICC_SRE_EL2);
>     isb();
> 
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h 
> b/xen/arch/arm/include/asm/gic_v3_its.h
> index fc5a84892c..0f8cb16e41 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -43,6 +43,11 @@
> #define GITS_CTLR_QUIESCENT             BIT(31, UL)
> #define GITS_CTLR_ENABLE                BIT(0, UL)
> 
> +#define GITS_TYPER_HCC_SHIFT            24
> +#define GITS_TYPER_HCC_MASK             0xffUL
> +#define GITS_TYPER_HCC(r)               (((r) >> GITS_TYPER_HCC_SHIFT) & \
> +                                                 GITS_TYPER_HCC_MASK)
> +
> #define GITS_TYPER_PTA                  BIT(19, UL)
> #define GITS_TYPER_DEVIDS_SHIFT         13
> #define GITS_TYPER_DEVIDS_MASK          (0x1fUL << GITS_TYPER_DEVIDS_SHIFT)
> @@ -129,6 +134,13 @@ struct host_its {
>     spinlock_t cmd_lock;
>     void *cmd_buf;
>     unsigned int flags;
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +    struct suspend_ctx {
> +        uint32_t ctlr;
> +        uint64_t cbaser;
> +        uint64_t baser[GITS_BASER_NR_REGS];
> +    } suspend_ctx;
> +#endif
> };
> 
> /* Map a collection for this host CPU to each host ITS. */
> @@ -204,6 +216,11 @@ uint64_t gicv3_its_get_cacheability(void);
> uint64_t gicv3_its_get_shareability(void);
> unsigned int gicv3_its_get_memflags(void);
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +int gicv3_its_suspend(void);
> +void gicv3_its_resume(void);
> +#endif
> +
> #else
> 
> #ifdef CONFIG_ACPI
> @@ -271,6 +288,17 @@ static inline int gicv3_its_make_hwdom_dt_nodes(const 
> struct domain *d,
>     return 0;
> }
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +static inline int gicv3_its_suspend(void)
> +{
> +    return 0;
> +}
> +
> +static inline void gicv3_its_resume(void)
> +{
> +}
> +#endif
> +
> #endif /* CONFIG_HAS_ITS */
> 
> #endif
> diff --git a/xen/include/xen/list.h b/xen/include/xen/list.h
> index 98d8482dab..2aab274157 100644
> --- a/xen/include/xen/list.h
> +++ b/xen/include/xen/list.h
> @@ -535,6 +535,20 @@ static inline void list_splice_init(struct list_head 
> *list,
>          &(pos)->member != (head);                                        \
>          (pos) = list_entry((pos)->member.next, typeof(*(pos)), member))
> 
> +/**
> + * list_for_each_entry_continue_reverse - iterate backwards from the given 
> point
> + * @pos:    the type * to use as a loop cursor.
> + * @head:   the head for your list.
> + * @member: the name of the list_head within the struct.
> + *
> + * Iterate over list of given type backwards, starting from the element 
> previous
> + * to the current one in list order.
> + */
> +#define list_for_each_entry_continue_reverse(pos, head, member)           \
> +    for ((pos) = list_entry((pos)->member.prev, typeof(*(pos)), member);  \
> +         &(pos)->member != (head);                                        \
> +         (pos) = list_entry((pos)->member.prev, typeof(*(pos)), member))
> +
> /**
>  * list_for_each_entry_from - iterate over list of given type from the
>  *                            current point
> -- 
> 2.43.0
>