Re: [PATCH v12 02/13] xen/arm: gic-v2: Implement GIC suspend/resume functions

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykola,

Sorry for the delay to review this serie.

> On 27 Aug 2026, at 16:31, Mykola Kvach <mykola_kvach@xxxxxxxx> wrote:
> 
> From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> 
> System suspend may lead to a state where GIC would be powered down.
> Therefore, Xen should save/restore the context of GIC on suspend/resume.
> 
> Note that the context consists of states of registers which are
> controlled by the hypervisor. Other GIC registers which are accessible
> by guests are saved/restored on context switch.
> 
> Transient physical SGI pending state (GICD_CPENDSGIRn/GICD_SPENDSGIRn)
> is intentionally excluded. CPU-interface active-priority state is also
> not restored across suspend/resume. Xen reaches the final suspend path
> at a quiescent point, so there is no active-priority execution context
> to replay after resume. Enforce this with a runtime check after
> disabling the CPU interface: if any implemented GICC_APRn word is still
> non-zero, restore GICC_CTLR and abort suspend with -EBUSY.

You mention SGI pending state but you do not say what would happen for PPI/SPI
pending state, and the patch does not look at or save/restore GICD_ISPENDR.

Can you clarify what is expected for those?

Cheers
Bertrand

> 
> This does not apply to distributor active state. With GICv2 EOImode==1,
> EOIR only drops the interrupt priority; final deactivation is a separate
> step. For guest-routed interrupts, Xen can have already EOIed the physical
> IRQ while deactivation is still pending on the vGIC/GICV path. Therefore
> GICD_ISACTIVER is preserved as architectural in-flight interrupt state.
> 
> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> Changes in V10:
> - Limit GICC_APR<n> active-priority checks to APR bits visible from
>  the Xen CPU-interface view.
> - Avoid touching reserved GICD_IPRIORITYR/GICD_ITARGETSR words when the
>  last implemented interrupt block is partial.
> - Restore distributor configuration before restoring interrupt enable
>  state, so GICD_ICFGR is written while the corresponding interrupts are
>  disabled.
> 
> Changes in V9:
> - Skip saving/restoring GICD_ITARGETSR0..7 because SGI/PPI target
>  registers hold no state (read-only on MP, RAZ/WI on UP).
> - Add a runtime GICC_APRn quiescence check after disabling the CPU
>  interface, and restore GICC_CTLR before returning -EBUSY.
> 
> Changes in V8:
> - disable cpu interface + distributor before suspend
> - change 0xffffffff to GENMASK;
> - cosmetic changes;
> 
> Changes in V7:
> - Allocate one contiguous memory block for the GICv2 dist suspend context.
> - gicv2_resume() no longer unconditionally re-enables the distributor/CPU
>  interface; it now writes back the saved CTLR values as-is.
> - gicv2_alloc_context() now returns 0 on success and panics on failure,
>  since suspend context allocation is not recoverable.
> ---
> xen/arch/arm/gic-v2.c          | 226 +++++++++++++++++++++++++++++++++
> xen/arch/arm/gic.c             |  29 +++++
> xen/arch/arm/include/asm/gic.h |  12 ++
> 3 files changed, 267 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 43a379fdda..a0ef6ffc7f 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1108,6 +1108,223 @@ static int gicv2_iomem_deny_access(struct domain *d)
>     return iomem_deny_access(d, mfn, mfn + nr - 1);
> }
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +
> +/* This struct represents block of 32 IRQs */
> +struct irq_block {
> +    uint32_t icfgr[2]; /* 2 registers of 16 IRQs each */
> +    uint32_t ipriorityr[8];
> +    uint32_t isenabler;
> +    uint32_t isactiver;
> +    uint32_t itargetsr[8];
> +};
> +
> +/* GICv2 registers to be saved/restored on system suspend/resume */
> +struct gicv2_context {
> +    /* GICC context */
> +    struct cpu_ctx {
> +        uint32_t ctlr;
> +        uint32_t pmr;
> +        uint32_t bpr;
> +    } cpu;
> +
> +    /* GICD context */
> +    struct dist_ctx {
> +        uint32_t ctlr;
> +        /* Includes banked SGI/PPI state for the boot CPU. */
> +        struct irq_block *irqs;
> +    } dist;
> +};
> +
> +static struct gicv2_context gic_ctx;
> +
> +#define GICV2_NR_APRS          4
> +#define GICV2_APR_BITS_PER_REG 32U
> +
> +static int gicv2_check_active_priorities(uint32_t bpr)
> +{
> +    unsigned int i, apr_bits, nr_aprs;
> +
> +    /*
> +     * Xen writes GICC_BPR to 0 during CPU init and does not change it. Per
> +     * IHI0048B.b, a write below the implementation minimum reads back as the
> +     * minimum supported BPR value. Table 4-47 maps that Xen-visible BPR 
> value
> +     * to the visible GICC_APR<n> bits. Avoid reading APR registers outside
> +     * that visible range.
> +     *
> +     * This covers both GICv2 with and without Security Extensions.
> +     */
> +    apr_bits = 1U << (7 - (bpr & 0x7));
> +    nr_aprs = DIV_ROUND_UP(apr_bits, GICV2_APR_BITS_PER_REG);
> +
> +    ASSERT(nr_aprs <= GICV2_NR_APRS);
> +
> +    for ( i = 0; i < nr_aprs; i++ )
> +    {
> +        unsigned int bits = min(GICV2_APR_BITS_PER_REG,
> +                                apr_bits - i * GICV2_APR_BITS_PER_REG);
> +        uint32_t mask = GENMASK(bits - 1, 0);
> +        uint32_t apr = readl_gicc(GICC_APR + i * 4) & mask;
> +
> +        if ( !apr )
> +            continue;
> +
> +        printk(XENLOG_ERR "GICv2: suspend aborted: GICC_APR%u=%#08x\n",
> +               i, apr);
> +        return -EBUSY;
> +    }
> +
> +    return 0;
> +}
> +
> +static int gicv2_suspend(void)
> +{
> +    unsigned int i, blocks = DIV_ROUND_UP(gicv2_info.nr_lines, 32);
> +    int ret;
> +
> +    /* Save GICC_CTLR configuration. */
> +    gic_ctx.cpu.ctlr = readl_gicc(GICC_CTLR);
> +
> +    /* Quiesce the GIC CPU interface before suspend. */
> +    gicv2_cpu_disable();
> +
> +    gic_ctx.cpu.bpr = readl_gicc(GICC_BPR);
> +
> +    /*
> +     * Check the active-priority state for the group Xen drives through the
> +     * CPU interface. GICC_CTL_ENABLE enables Group 0 without SecurityExtn 
> and
> +     * Group 1 in Xen's Non-secure view with SecurityExtn, and in both cases
> +     * the relevant state is visible through GICC_APRn. The APR layout is
> +     * implementation-defined, so only test the bits visible from Xen's CPU
> +     * interface view instead of reading every possible APR register.
> +     */
> +    ret = gicv2_check_active_priorities(gic_ctx.cpu.bpr);
> +    if ( ret )
> +    {
> +        writel_gicc(gic_ctx.cpu.ctlr, GICC_CTLR);
> +        return ret;
> +    }
> +
> +    gic_ctx.cpu.pmr = readl_gicc(GICC_PMR);
> +
> +    /* Save GICD configuration */
> +    gic_ctx.dist.ctlr = readl_gicd(GICD_CTLR);
> +    writel_gicd(0, GICD_CTLR);
> +
> +    for ( i = 0; i < blocks; i++ )
> +    {
> +        struct irq_block *irqs = gic_ctx.dist.irqs + i;
> +        size_t j, off = i * sizeof(irqs->isenabler);
> +        size_t nr_regs = ARRAY_SIZE(irqs->ipriorityr);
> +
> +        if ( i == blocks - 1 )
> +            nr_regs = DIV_ROUND_UP(gicv2_info.nr_lines - i * 32, 4);
> +
> +        irqs->isenabler = readl_gicd(GICD_ISENABLER + off);
> +
> +        /*
> +         * Save distributor active state as part of the hypervisor-owned
> +         * physical interrupt state. In GICv2 EOImode==1, EOIR only drops the
> +         * priority; final deactivation is separate. For guest-routed
> +         * interrupts, Xen may have EOIed the physical IRQ while the 
> guest/vGIC
> +         * side still owns the deactivate step. Therefore GICD_ISACTIVER can
> +         * legitimately remain set even though transient SGI pending state 
> and
> +         * CPU-interface active-priority state are expected to be quiesced 
> here.
> +         */
> +        irqs->isactiver = readl_gicd(GICD_ISACTIVER + off);
> +
> +        off = i * sizeof(irqs->ipriorityr);
> +        for ( j = 0; j < nr_regs; j++ )
> +            irqs->ipriorityr[j] = readl_gicd(GICD_IPRIORITYR + off + j * 4);
> +
> +        /*
> +         * GICD_ITARGETSR0..7 cover SGIs/PPIs and hold no state to save:
> +         * they are read-only on multiprocessor implementations and RAZ/WI
> +         * on uniprocessor implementations.
> +         */
> +        if ( i )
> +        {
> +            off = i * sizeof(irqs->itargetsr);
> +            for ( j = 0; j < nr_regs; j++ )
> +                irqs->itargetsr[j] = readl_gicd(GICD_ITARGETSR + off + j * 
> 4);
> +        }
> +
> +        off = i * sizeof(irqs->icfgr);
> +        for ( j = 0; j < ARRAY_SIZE(irqs->icfgr); j++ )
> +            irqs->icfgr[j] = readl_gicd(GICD_ICFGR + off + j * 4);
> +    }
> +
> +    return 0;
> +}
> +
> +static void gicv2_resume(void)
> +{
> +    unsigned int i, blocks = DIV_ROUND_UP(gicv2_info.nr_lines, 32);
> +
> +    gicv2_cpu_disable();
> +    /* Disable distributor */
> +    writel_gicd(0, GICD_CTLR);
> +
> +    for ( i = 0; i < blocks; i++ )
> +    {
> +        struct irq_block *irqs = gic_ctx.dist.irqs + i;
> +        size_t j, off = i * sizeof(irqs->isenabler);
> +        size_t nr_regs = ARRAY_SIZE(irqs->ipriorityr);
> +
> +        if ( i == blocks - 1 )
> +            nr_regs = DIV_ROUND_UP(gicv2_info.nr_lines - i * 32, 4);
> +
> +        writel_gicd(GENMASK(31, 0), GICD_ICENABLER + off);
> +
> +        off = i * sizeof(irqs->icfgr);
> +        for ( j = 0; j < ARRAY_SIZE(irqs->icfgr); j++ )
> +            writel_gicd(irqs->icfgr[j], GICD_ICFGR + off + j * 4);
> +
> +        off = i * sizeof(irqs->ipriorityr);
> +        for ( j = 0; j < nr_regs; j++ )
> +            writel_gicd(irqs->ipriorityr[j], GICD_IPRIORITYR + off + j * 4);
> +
> +        /*
> +         * GICD_ITARGETSR0..7 cover SGIs/PPIs and hold no state to save:
> +         * they are read-only on multiprocessor implementations and RAZ/WI
> +         * on uniprocessor implementations.
> +         */
> +        if ( i )
> +        {
> +            off = i * sizeof(irqs->itargetsr);
> +            for ( j = 0; j < nr_regs; j++ )
> +                writel_gicd(irqs->itargetsr[j], GICD_ITARGETSR + off + j * 
> 4);
> +        }
> +
> +        off = i * sizeof(irqs->isenabler);
> +        writel_gicd(irqs->isenabler, GICD_ISENABLER + off);
> +
> +        writel_gicd(GENMASK(31, 0), GICD_ICACTIVER + off);
> +        writel_gicd(irqs->isactiver, GICD_ISACTIVER + off);
> +    }
> +
> +    /* Restore distributor control state. */
> +    writel_gicd(gic_ctx.dist.ctlr, GICD_CTLR);
> +
> +    /* Restore GIC CPU interface configuration */
> +    writel_gicc(gic_ctx.cpu.pmr, GICC_PMR);
> +    writel_gicc(gic_ctx.cpu.bpr, GICC_BPR);
> +
> +    /* Enable GIC CPU interface */
> +    writel_gicc(gic_ctx.cpu.ctlr, GICC_CTLR);
> +}
> +
> +static void __init gicv2_alloc_context(void)
> +{
> +    uint32_t blocks = DIV_ROUND_UP(gicv2_info.nr_lines, 32);
> +
> +    gic_ctx.dist.irqs = xzalloc_array(struct irq_block, blocks);
> +    if ( !gic_ctx.dist.irqs )
> +        panic("Failed to allocate memory for GICv2 suspend context\n");
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
> #ifdef CONFIG_ACPI
> static unsigned long gicv2_get_hwdom_extra_madt_size(const struct domain *d)
> {
> @@ -1312,6 +1529,11 @@ static int __init gicv2_init(void)
> 
>     spin_unlock(&gicv2.lock);
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +    /* Allocate memory to be used for saving GIC context during the suspend 
> */
> +    gicv2_alloc_context();
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
>     return 0;
> }
> 
> @@ -1355,6 +1577,10 @@ static const struct gic_hw_operations gicv2_ops = {
>     .map_hwdom_extra_mappings = gicv2_map_hwdom_extra_mappings,
>     .iomem_deny_access   = gicv2_iomem_deny_access,
>     .do_LPI              = gicv2_do_LPI,
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +    .suspend             = gicv2_suspend,
> +    .resume              = gicv2_resume,
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> };
> 
> /* Set up the GIC */
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 078049e741..ffc11f36a1 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -438,6 +438,35 @@ int gic_iomem_deny_access(struct domain *d)
>     return gic_hw_ops->iomem_deny_access(d);
> }
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +
> +int gic_suspend(void)
> +{
> +    /* Must be called by boot CPU#0 with interrupts disabled */
> +    ASSERT(!local_irq_is_enabled());
> +    ASSERT(!smp_processor_id());
> +
> +    if ( !gic_hw_ops->suspend || !gic_hw_ops->resume )
> +        return -ENOSYS;
> +
> +    return gic_hw_ops->suspend();
> +}
> +
> +void gic_resume(void)
> +{
> +    /*
> +     * Must be called by boot CPU#0 with interrupts disabled after 
> gic_suspend
> +     * has returned successfully.
> +     */
> +    ASSERT(!local_irq_is_enabled());
> +    ASSERT(!smp_processor_id());
> +    ASSERT(gic_hw_ops->resume);
> +
> +    gic_hw_ops->resume();
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
> static int cpu_gic_callback(struct notifier_block *nfb,
>                             unsigned long action,
>                             void *hcpu)
> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> index ee2c26adb4..29bb9a89a4 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -301,6 +301,12 @@ extern int gicv_setup(struct domain *d);
> extern void gic_save_state(struct vcpu *v);
> extern void gic_restore_state(struct vcpu *v);
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +/* Suspend/resume */
> +extern int gic_suspend(void);
> +extern void gic_resume(void);
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
> /* SGI (AKA IPIs) */
> enum gic_sgi {
>     GIC_SGI_EVENT_CHECK,
> @@ -444,6 +450,12 @@ struct gic_hw_operations {
>     int (*iomem_deny_access)(struct domain *d);
>     /* Handle LPIs, which require special handling */
>     void (*do_LPI)(unsigned int lpi);
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +    /* Save GIC configuration due to the system suspend */
> +    int (*suspend)(void);
> +    /* Restore GIC configuration due to the system resume */
> +    void (*resume)(void);
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> };
> 
> extern const struct gic_hw_operations *gic_hw_ops;
> -- 
> 2.43.0
>