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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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, 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>
> ---
> 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             | 126 ++++++++++++++++++++++++--
> 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, 166 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 9ba068c46f..fe2865eac9 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;
>     }
> @@ -1209,6 +1217,106 @@ 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)

NIT: codestyle, spaces after and before the parenthesis

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


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

> +            goto err;
> +        }
> +
> +        its->suspend_ctx.cbaser = readq_relaxed(base + GITS_CBASER);
> +
> +        for (i = 0; i < GITS_BASER_NR_REGS; i++)

NIT: codestyle on the spaces and parenthesis

> +        {
> +            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
> +     * 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);
> +}
> +
> +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 d182a71478..ef8318dd50 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -862,7 +862,7 @@ static bool gicv3_enable_lpis(void)
>     return true;
> }
> 
> -static int __init gicv3_populate_rdist(void)
> +static int gicv3_populate_rdist(void)
> {
>     int i;
>     uint32_t aff;
> @@ -932,7 +932,7 @@ static int __init gicv3_populate_rdist(void)
>                     ret = gicv3_lpi_init_rdist(ptr);
>                     if ( ret && ret != -ENODEV && ret != -EBUSY )
>                     {
> -                        printk("GICv3: CPU%d: Cannot initialize LPIs: %u\n",
> +                        printk("GICv3: CPU%d: Cannot initialize LPIs: %d\n",

this is to fix the mistake of a patch before, 

Cheers,
Luca