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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

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".

> +         */
> +        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.
> +    if ( ret )
> +        panic("GICv3: ITS: resume setup collection failed: %d\n", ret);
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
>   
[snip]