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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On 15/01/2026 12:00, Mykola Kvach wrote:
> Hi Julien,
>
> Thanks for the review.
>
> On Fri, Dec 26, 2025 at 2:29 PM Julien Grall <julien@xxxxxxx> wrote:
> > Hi Mykola,
> >
> > On 11/12/2025 18:43, Mykola Kvach wrote:
> > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > > index b23e72a3d0..0b2f7b3862 100644
> > > --- a/xen/arch/arm/gic-v2.c
> > > +++ b/xen/arch/arm/gic-v2.c
> > > @@ -1098,6 +1098,123 @@ static int gicv2_iomem_deny_access(struct domain *d)
> > >        return iomem_deny_access(d, mfn, mfn + nr);
> > >    }
> > >
> > > +#ifdef CONFIG_SYSTEM_SUSPEND
> > > +
> > > +/* This struct represent 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;
> > > +        struct irq_block *irqs;
> > To confirm my understanding, this will also include the PPIs, SGIs for
> > the boot CPU, am I correct? If so, I would suggest to add a comment.
> Yes, correct. I’ll add a comment to make it explicit that this includes
> SGIs/PPIs for the boot CPU.
>
> > > +    } 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 configuration */
> > > +    gic_ctx.cpu.ctlr = readl_gicc(GICC_CTLR);
> > > +    gic_ctx.cpu.pmr = readl_gicc(GICC_PMR);
> > > +    gic_ctx.cpu.bpr = readl_gicc(GICC_BPR);
> > > +
> > > +    /* Save GICD configuration */
> > > +    gic_ctx.dist.ctlr = readl_gicd(GICD_CTLR);
> > Do we want to disable the GIC distributor and CPU interface on suspend?
> I think we should quiesce the CPU interface after saving state,
> but not disable the distributor globally.
>
> I still prefer not to disable GICD globally for safety on platforms
> where the wake request is routed from the GIC to the PMU/SCP (e.g. via
> nIRQOUT/nFIQOUT). So I’d quiesce GICC, keep GICD enabled.
>
> Are you OK with this approach?
I have revisited the documentation and could not find a
normative PSCI requirement to disable the GIC distributor.

PSCI 1.3, section 6.8 ("Preserving the execution context"),
only says that some systems may power down the GIC
distributor as part of SYSTEM_SUSPEND or a deep
CPU_SUSPEND, in which case the relevant GIC state must be
saved and restored. So PSCI acknowledges this as a possible
platform behavior, but it does not appear to mandate it.

The stronger guidance seems to come from BSA. In Arm BSA 1.2
(DEN0094E), section 3.10, semantic E allows the GIC to be
powered off after system software has saved its state.
Table 8 shows semantic E with the GIC distributor Off, and
Table 9 states that the GIC Distributor must be Off if all
PEs are in the Off state.


Best regards,
Mykola

> > > +
> > > +    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);
> > > +        }
> > > +
> > > +        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(0xffffffffU, GICD_ICENABLER + off);
> > NIT: Can we use GENMASK? This will make easier to confirm we have the
> > correct number of bits.
> Sure, I'll change it to GENMASK
>
> Best regards,
> Mykola
>
>
> > [...]
> >
> > Cheers,
> >
> > --
> > Julien Grall