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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the feedback.

On Tue, Apr 21, 2026 at 4:26 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index b23e72a3d0..dbff470962 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -1098,6 +1098,129 @@ static int gicv2_iomem_deny_access(struct domain *d)
> >     return iomem_deny_access(d, mfn, mfn + nr);
> > }
> >
> > +#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;
> > +
> > +static int gicv2_suspend(void)
> > +{
> > +    unsigned int i, blocks = DIV_ROUND_UP(gicv2_info.nr_lines, 32);
> > +
> > +    /* Save GICC_CTLR configuration. */
> > +    gic_ctx.cpu.ctlr = readl_gicc(GICC_CTLR);
> > +
> > +    /* Quiesce the GIC CPU interface before suspend. */
> > +    gicv2_cpu_disable();
> > +
> > +    /* Save GICD configuration */
> > +    gic_ctx.dist.ctlr = readl_gicd(GICD_CTLR);
> > +    writel_gicd(0, GICD_CTLR);
> > +
> > +    gic_ctx.cpu.pmr = readl_gicc(GICC_PMR);
> > +    gic_ctx.cpu.bpr = readl_gicc(GICC_BPR);
> > +
> > +    for ( i = 0; i < blocks; i++ )
> > +    {
> > +        struct irq_block *irqs = gic_ctx.dist.irqs + i;
> > +        size_t j, off = i * sizeof(irqs->isenabler);
> > +
> > +        irqs->isenabler = readl_gicd(GICD_ISENABLER + off);
> > +        irqs->isactiver = readl_gicd(GICD_ISACTIVER + off);
> > +
> > +        off = i * sizeof(irqs->ipriorityr);
> > +        for ( j = 0; j < ARRAY_SIZE(irqs->ipriorityr); j++ )
> > +        {
> > +            irqs->ipriorityr[j] = readl_gicd(GICD_IPRIORITYR + off + j * 
> > 4);
> > +            irqs->itargetsr[j] = readl_gicd(GICD_ITARGETSR + off + j * 4);
>
> regarding GICD_ITARGETSR ...
>
> > +        }
> > +
> > +        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);
> > +
> > +        writel_gicd(GENMASK(31, 0), GICD_ICENABLER + off);
> > +        writel_gicd(irqs->isenabler, GICD_ISENABLER + off);
> > +
> > +        writel_gicd(GENMASK(31, 0), GICD_ICACTIVER + off);
> > +        writel_gicd(irqs->isactiver, GICD_ISACTIVER + off);
> > +
> > +        off = i * sizeof(irqs->ipriorityr);
> > +        for ( j = 0; j < ARRAY_SIZE(irqs->ipriorityr); j++ )
> > +        {
> > +            writel_gicd(irqs->ipriorityr[j], GICD_IPRIORITYR + off + j * 
> > 4);
> > +            writel_gicd(irqs->itargetsr[j], GICD_ITARGETSR + off + j * 4);
>
> … please let me know if I read correctly this loop, but here GICD_ITARGETSR0 
> … 7
> are restored when i=0, but the specificaitons says that this block is read 
> only on
> multiprocessor, so we should skip the restore part.
> Also saving it could be skipped because each field returns a value that 
> corresponds
> only to the processor reading the register.
>
> 4.3.12 User constraints [1]

You are right, thanks for pointing this out.
I will skip saving/restoring the read-only GICD_ITARGETSR0..7 block in v9.

>
> > +        }
> > +
> > +        off = i * sizeof(irqs->icfgr);
> > +        for ( j = 0; j < ARRAY_SIZE(irqs->icfgr); j++ )
> > +            writel_gicd(irqs->icfgr[j], GICD_ICFGR + off + j * 4);
> > +    }
> > +
> > +    /* Make sure all registers are restored and enable distributor */
> > +    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);
> > +}
> > +
>
> I also see that we don’t save pending SGIs state (by 
> GICD_CPENDSGIRn/GICD_SPENDSGIRn) or Active Priorities registers
> state (GICC_APRn/GICC_NSAPRn [latter if security extension are there]) as 
> written in [1] “4.5 Preserving and restoring GIC state”,
> was it intentional?

Yes, this was intentional.

The GICv2 suspend callback is called at a quiescent point in the
SYSTEM_SUSPEND path: all domains are already shut down for suspend, guest
execution is quiesced, the scheduler is disabled, non-boot CPUs have been
offlined, and CPU0 enters gic_suspend() with local interrupts disabled.

For SGIs, I don't consider GICD_CPENDSGIRn/GICD_SPENDSGIRn part of the saved
host GIC context. Xen uses physical SGIs as IPIs, and IPI delivery is an
internal synchronization mechanism, not architectural state that should be
replayed after SYSTEM_SUSPEND. Guest SGI state is virtual GIC state and is not
represented by these physical GICD SGI pending registers.

For GICC_APRn/GICC_NSAPRn, those registers describe active priority state for
interrupts already acknowledged by the CPU interface. The final suspend path is
not expected to run with an active physical interrupt context. If those
registers were non-zero there, restoring only APR/NSAPR would not make the
corresponding interrupt handling context valid after resume, and could instead
leave the CPU interface with stale active priority state.

So I did not add save/restore for GICD_CPENDSGIRn/GICD_SPENDSGIRn or
GICC_APRn/GICC_NSAPRn in this patch. I can add a short comment in v9 to make
this scope explicit.

Please let me know if you think there is a suspend/resume path where this
state still needs to be preserved.

Best regards,
Mykola

>
> [1] https://developer.arm.com/documentation/ihi0048/bb/?lang=en
>
> Cheers,
> Luca
>
>