Re: [PATCH v8 04/13] xen/arm: gic-v3: Implement GICv3 suspend/resume functions

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the review.

On Thu, Apr 23, 2026 at 2:29 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index 34fb065afc..d182a71478 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1072,12 +1072,12 @@ out:
> >     return res;
> > }
> >
> > -static void gicv3_hyp_disable(void)
> > +static void gicv3_hyp_enable(bool enable)
> > {
> >     register_t hcr;
> >
> >     hcr = READ_SYSREG(ICH_HCR_EL2);
> > -    hcr &= ~GICH_HCR_EN;
> > +    hcr = enable ? (hcr | GICH_HCR_EN) : (hcr & ~GICH_HCR_EN);
> >     WRITE_SYSREG(hcr, ICH_HCR_EL2);
> >     isb();
> > }
> > @@ -1184,7 +1184,7 @@ static void gicv3_disable_interface(void)
> >     spin_lock(&gicv3.lock);
> >
> >     gicv3_cpu_disable();
> > -    gicv3_hyp_disable();
> > +    gicv3_hyp_enable(false);
> >
> >     spin_unlock(&gicv3.lock);
> > }
> > @@ -1920,6 +1920,313 @@ static bool gic_dist_supports_lpis(void)
> >     return (readl_relaxed(GICD + GICD_TYPER) & GICD_TYPE_LPIS);
> > }
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +
> > +/* This struct represent block of 32 IRQs */
>
> NIT: s/represent/represents

Ack.

>
> > +struct dist_irq_block {
> > +    uint32_t icfgr[2];
> > +    uint32_t ipriorityr[8];
> > +    uint64_t irouter[32];
> > +    uint32_t isactiver;
> > +    uint32_t isenabler;
> > +};
> > +
> > +struct redist_ctx {
> > +    uint32_t ctlr;
> > +    uint32_t icfgr; /* only PPIs stored */
> > +    uint32_t igroupr;
>
> I think Xen writes also GICD_IGROUPR<n>E, we are not saving it so in case of 
> a reset
> we would have GICD_IGROUPR<n>E containing the reset value which is zero.
> Or we could decide to re-initialise it in the same way Xen does (all 1s).

Yes, good point.

For the normal SPI range I re-initialise GICD_IGROUPR to all 1s during resume,
but I missed doing the same for the eSPI range. I will add the corresponding
GICD_IGROUPR<n>E re-initialisation, matching the normal Xen initialisation path.

>
> > +    uint32_t ipriorityr[8];
> > +    uint32_t isactiver;
> > +    uint32_t isenabler;
> > +
> > +    uint64_t pendbase;
> > +    uint64_t propbase;
> > +};
> > +
> > +/* GICv3 registers to be saved/restored on system suspend/resume */
> > +struct gicv3_ctx {
> > +    struct dist_ctx {
> > +        uint32_t ctlr;
> > +        struct dist_irq_block *irqs, *espi_irqs;
>
> NIT: I would declare them one after the other and not in the same line, but 
> this is a matter of taste
> maybe so I will leave the decision to the maintainers.

Ack.

>
> > +    } dist;
> > +
> > +    /* have only one rdist structure for last running CPU during suspend */
> > +    struct redist_ctx rdist;
> > +
> > +    struct cpu_ctx {
> > +        uint32_t ctlr;
> > +        uint32_t pmr;
> > +        uint32_t bpr;
> > +        uint32_t sre_el2;
> > +        uint32_t grpen;
> > +    } cpu;
> > +};
> > +
> > +static struct gicv3_ctx gicv3_ctx;
> > +
> > +static void __init gicv3_alloc_context(void)
> > +{
> > +    uint32_t blocks = DIV_ROUND_UP(gicv3_info.nr_lines, 32);
> > +
> > +    /* The spec allows for systems without any SPIs */
> > +    if ( blocks > 1 )
> > +    {
> > +        gicv3_ctx.dist.irqs = xzalloc_array(struct dist_irq_block, blocks 
> > - 1);
> > +        if ( !gicv3_ctx.dist.irqs )
> > +            panic("Failed to allocate memory for GICv3 suspend context\n");
> > +    }
> > +
> > +#ifdef CONFIG_GICV3_ESPI
> > +    if ( !gic_number_espis() )
> > +        return;
> > +
> > +    blocks = gic_number_espis() / 32;
> > +    gicv3_ctx.dist.espi_irqs = xzalloc_array(struct dist_irq_block, 
> > blocks);
> > +    if ( !gicv3_ctx.dist.espi_irqs )
> > +        panic("Failed to allocate memory for GICv3 eSPI suspend 
> > context\n");
> > +#endif
> > +}
> > +
> > +static int gicv3_disable_redist(void)
> > +{
> > +    void __iomem *waker = GICD_RDIST_BASE + GICR_WAKER;
> > +    s_time_t deadline;
> > +
> > +    /*
> > +     * Avoid infinite loop if Non-secure does not have access to 
> > GICR_WAKER.
> > +     * See Arm IHI 0069H.b, 12.11.42 GICR_WAKER:
> > +     *     When GICD_CTLR.DS == 0 and an access is Non-secure accesses to 
> > this
> > +     *     register are RAZ/WI.
> > +     */
> > +    if ( !(readl_relaxed(GICD + GICD_CTLR) & GICD_CTLR_DS) )
> > +        return 0;
> > +
> > +    deadline = NOW() + MILLISECS(1000);
> > +
> > +    writel_relaxed(readl_relaxed(waker) | GICR_WAKER_ProcessorSleep, 
> > waker);
> > +    while ( (readl_relaxed(waker) & GICR_WAKER_ChildrenAsleep) == 0 )
> > +    {
> > +        if ( NOW() > deadline )
> > +        {
> > +            printk("GICv3: Timeout waiting for redistributor to sleep\n");
> > +            return -ETIMEDOUT;
> > +        }
> > +        cpu_relax();
> > +        udelay(10);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +#define GET_SPI_REG_OFFSET(name, is_espi) \
> > +    ((is_espi) ? GICD_##name##nE : GICD_##name)
> > +
> > +static void gicv3_store_spi_irq_block(struct dist_irq_block *irqs,
> > +                                      unsigned int i, bool is_espi)
> > +{
> > +    void __iomem *base;
> > +    unsigned int irq;
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ICFGR, is_espi) + i * 
> > sizeof(irqs->icfgr);
> > +    irqs->icfgr[0] = readl_relaxed(base);
> > +    irqs->icfgr[1] = readl_relaxed(base + 4);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(IPRIORITYR, is_espi);
> > +    base += i * sizeof(irqs->ipriorityr);
> > +    for ( irq = 0; irq < ARRAY_SIZE(irqs->ipriorityr); irq++ )
> > +        irqs->ipriorityr[irq] = readl_relaxed(base + 4 * irq);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(IROUTER, is_espi);
> > +    base += i * sizeof(irqs->irouter);
> > +    for ( irq = 0; irq < ARRAY_SIZE(irqs->irouter); irq++ )
> > +        irqs->irouter[irq] = readq_relaxed_non_atomic(base + 8 * irq);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ISACTIVER, is_espi);
> > +    base += i * sizeof(irqs->isactiver);
> > +    irqs->isactiver = readl_relaxed(base);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ISENABLER, is_espi);
> > +    base += i * sizeof(irqs->isenabler);
> > +    irqs->isenabler = readl_relaxed(base);
> > +}
> > +
> > +static void gicv3_restore_spi_irq_block(struct dist_irq_block *irqs,
> > +                                        unsigned int i, bool is_espi)
> > +{
> > +    void __iomem *base;
> > +    unsigned int irq;
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ICFGR, is_espi) + i * 
> > sizeof(irqs->icfgr);
> > +    writel_relaxed(irqs->icfgr[0], base);
> > +    writel_relaxed(irqs->icfgr[1], base + 4);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(IPRIORITYR, is_espi);
> > +    base += i * sizeof(irqs->ipriorityr);
> > +    for ( irq = 0; irq < ARRAY_SIZE(irqs->ipriorityr); irq++ )
> > +        writel_relaxed(irqs->ipriorityr[irq], base + 4 * irq);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(IROUTER, is_espi);
> > +    base += i * sizeof(irqs->irouter);
> > +    for ( irq = 0; irq < ARRAY_SIZE(irqs->irouter); irq++ )
> > +        writeq_relaxed_non_atomic(irqs->irouter[irq], base + 8 * irq);
>
>
> The [1] 12.9.22 GICD_IROUTER<n> says "these registers are used only when 
> affinity routing is enabled.
> When affinity routing is not enabled: These registers are RES0. An 
> implementation is permitted to make
> the register RAZ/WI in this case”
>
> So I think these needs to be written after we set GICD_CTLR or we are going 
> to loose anything written there
> and also the configuration won’t be restored.

You are right. Restoring IROUTER before restoring the affinity-routing state is
not safe, because these registers are only meaningful when affinity routing is
enabled.

I will fix the restore ordering in the next version.

>
>
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ICENABLER, is_espi) + i * 4;
> > +    writel_relaxed(GENMASK(31, 0), base);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ISENABLER, is_espi);
> > +    base += i * sizeof(irqs->isenabler);
> > +    writel_relaxed(irqs->isenabler, base);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ICACTIVER, is_espi) + i * 4;
> > +    writel_relaxed(GENMASK(31, 0), base);
> > +
> > +    base = GICD + GET_SPI_REG_OFFSET(ISACTIVER, is_espi);
> > +    base += i * sizeof(irqs->isactiver);
> > +    writel_relaxed(irqs->isactiver, base);
> > +}
> > +
> > +static int gicv3_suspend(void)
> > +{
> > +    unsigned int i;
> > +    void __iomem *base;
> > +    int ret;
> > +    struct redist_ctx *rdist = &gicv3_ctx.rdist;
> > +
> > +    /* Save GICC configuration */
> > +    gicv3_ctx.cpu.ctlr     = READ_SYSREG(ICC_CTLR_EL1);
> > +    gicv3_ctx.cpu.pmr      = READ_SYSREG(ICC_PMR_EL1);
> > +    gicv3_ctx.cpu.bpr      = READ_SYSREG(ICC_BPR1_EL1);
> > +    gicv3_ctx.cpu.sre_el2  = READ_SYSREG(ICC_SRE_EL2);
> > +    gicv3_ctx.cpu.grpen    = READ_SYSREG(ICC_IGRPEN1_EL1);
> > +
> > +    gicv3_disable_interface();
>
> this one is calling also gicv3_cpu_disable() that will zero ICC_IGRPEN1_EL1 
> ...
>
> > +
> > +    ret = gicv3_disable_redist();
> > +    if ( ret )
> > +        goto out_enable_iface;
>
> … but when we fail here ...
>
> > +
> > +    /* Save GICR configuration */
> > +    gicv3_redist_wait_for_rwp();
> > +
> > +    base = GICD_RDIST_BASE;
> > +
> > +    rdist->ctlr = readl_relaxed(base + GICR_CTLR);
> > +
> > +    rdist->propbase = readq_relaxed(base + GICR_PROPBASER);
> > +    rdist->pendbase = readq_relaxed(base + GICR_PENDBASER);
> > +
> > +    base = GICD_RDIST_SGI_BASE;
> > +
> > +    /* Save priority on PPI and SGI interrupts */
> > +    for ( i = 0; i < NR_GIC_LOCAL_IRQS / 4; i++ )
> > +        rdist->ipriorityr[i] = readl_relaxed(base + GICR_IPRIORITYR0 + 4 * 
> > i);
> > +
> > +    rdist->isactiver = readl_relaxed(base + GICR_ISACTIVER0);
> > +    rdist->isenabler = readl_relaxed(base + GICR_ISENABLER0);
> > +    rdist->igroupr   = readl_relaxed(base + GICR_IGROUPR0);
> > +    rdist->icfgr     = readl_relaxed(base + GICR_ICFGR1);
> > +
> > +    /* Save GICD configuration */
> > +    gicv3_dist_wait_for_rwp();
> > +    gicv3_ctx.dist.ctlr = readl_relaxed(GICD + GICD_CTLR);
> > +
> > +    for ( i = 1; i < DIV_ROUND_UP(gicv3_info.nr_lines, 32); i++ )
> > +        gicv3_store_spi_irq_block(gicv3_ctx.dist.irqs + i - 1, i, false);
> > +
> > +#ifdef CONFIG_GICV3_ESPI
> > +    for ( i = 0; i < gic_number_espis() / 32; i++ )
> > +        gicv3_store_spi_irq_block(gicv3_ctx.dist.espi_irqs + i, i, true);
> > +#endif
> > +
> > +    return 0;
> > +
> > + out_enable_iface:
> > +    gicv3_hyp_enable(true);
> > +    WRITE_SYSREG(gicv3_ctx.cpu.ctlr, ICC_CTLR_EL1);
>
> we don’t recover ICC_IGRPEN1_EL1

Yes, you are right.

This series missed the change introduced by commit 18b718b6af3d ("xen/arm:
gic-v3: disable Group 1 before CPU power-down"). Since gicv3_cpu_disable() now
disables ICC_IGRPEN1_EL1, the error path needs to restore it before returning.

I will fix this in the next version.

Best regards,
Mykola

>
> > +    isb();
> > +
> > +    return ret;
> > +}
> > +
> > +static void gicv3_resume(void)
> > +{
> > +    int ret;
> > +    unsigned int i;
> > +    void __iomem *base;
> > +    struct redist_ctx *rdist = &gicv3_ctx.rdist;
> > +
> > +    writel_relaxed(0, GICD + GICD_CTLR);
> > +
> > +    for ( i = NR_GIC_LOCAL_IRQS; i < gicv3_info.nr_lines; i += 32 )
> > +        writel_relaxed(GENMASK(31, 0), GICD + GICD_IGROUPR + (i / 32) * 4);
> > +
> > +    for ( i = 1; i < DIV_ROUND_UP(gicv3_info.nr_lines, 32); i++ )
> > +        gicv3_restore_spi_irq_block(gicv3_ctx.dist.irqs + i - 1, i, false);
> > +
> > +#ifdef CONFIG_GICV3_ESPI
> > +    for ( i = 0; i < gic_number_espis() / 32; i++ )
> > +        gicv3_restore_spi_irq_block(gicv3_ctx.dist.espi_irqs + i, i, true);
> > +#endif
> > +
> > +    writel_relaxed(gicv3_ctx.dist.ctlr, GICD + GICD_CTLR);
> > +    gicv3_dist_wait_for_rwp();
> > +
> > +    ret = gicv3_lpi_init_rdist(GICD_RDIST_BASE);
> > +    /*
> > +     * If LPIs are already enabled, assume firmware or the still-powered
> > +     * redistributor has valid PROPBASER/PENDBASER and skip reprogramming.
> > +     * Return -EBUSY so callers can ignore this case.
> > +     */
> > +    if ( ret && ret != -ENODEV && ret != -EBUSY )
> > +        panic("GICv3: Failed to re-initialize LPIs during resume\n");
> > +    else if ( ret == -EBUSY ) /* extra checks, just to be sure */
> > +    {
> > +        base = GICD_RDIST_BASE;
> > +        if ( readq_relaxed(base + GICR_PROPBASER) != rdist->propbase ||
> > +             readq_relaxed(base + GICR_PENDBASER) != rdist->pendbase )
> > +        {
> > +            panic("GICv3: LPIs already enabled with unexpected 
> > PROPBASER/PENDBASER during resume\n");
> > +        }
> > +    }
> > +
> > +    /* Restore GICR (Redistributor) configuration */
> > +    if ( gicv3_enable_redist() )
> > +        panic("GICv3: Failed to re-enable redistributor during resume\n");
> > +
> > +    base = GICD_RDIST_SGI_BASE;
> > +
> > +    writel_relaxed(GENMASK(31, 0), base + GICR_ICENABLER0);
> > +    gicv3_redist_wait_for_rwp();
> > +
> > +    for ( i = 0; i < NR_GIC_LOCAL_IRQS / 4; i++ )
> > +        writel_relaxed(rdist->ipriorityr[i], base + GICR_IPRIORITYR0 + i * 
> > 4);
> > +
> > +    writel_relaxed(rdist->isactiver, base + GICR_ISACTIVER0);
> > +    writel_relaxed(rdist->igroupr,   base + GICR_IGROUPR0);
> > +    writel_relaxed(rdist->icfgr,     base + GICR_ICFGR1);
> > +
> > +    gicv3_redist_wait_for_rwp();
> > +
> > +    writel_relaxed(rdist->isenabler, base + GICR_ISENABLER0);
> > +    writel_relaxed(rdist->ctlr, GICD_RDIST_BASE + GICR_CTLR);
> > +
> > +    gicv3_redist_wait_for_rwp();
> > +
> > +    WRITE_SYSREG(gicv3_ctx.cpu.sre_el2, ICC_SRE_EL2);
> > +    isb();
> > +
> > +    /* Restore CPU interface (System registers) */
> > +    WRITE_SYSREG(gicv3_ctx.cpu.pmr,   ICC_PMR_EL1);
> > +    WRITE_SYSREG(gicv3_ctx.cpu.bpr,   ICC_BPR1_EL1);
> > +    WRITE_SYSREG(gicv3_ctx.cpu.ctlr,  ICC_CTLR_EL1);
> > +    WRITE_SYSREG(gicv3_ctx.cpu.grpen, ICC_IGRPEN1_EL1);
> > +    isb();
> > +
> > +    gicv3_hyp_init();
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> > /* Set up the GIC */
> > static int __init gicv3_init(void)
> > {
> > @@ -1994,6 +2301,10 @@ static int __init gicv3_init(void)
> >
> >     gicv3_hyp_init();
> >
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    gicv3_alloc_context();
> > +#endif
> > +
> > out:
> >     spin_unlock(&gicv3.lock);
> >
> > @@ -2033,6 +2344,10 @@ static const struct gic_hw_operations gicv3_ops = {
> > #endif
> >     .iomem_deny_access   = gicv3_iomem_deny_access,
> >     .do_LPI              = gicv3_do_LPI,
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    .suspend             = gicv3_suspend,
> > +    .resume              = gicv3_resume,
> > +#endif
> > };
> >
> > static int __init gicv3_dt_preinit(struct dt_device_node *node, const void 
> > *data)
> > diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h 
> > b/xen/arch/arm/include/asm/gic_v3_defs.h
> > index c373b94d19..992c8f9c2f 100644
> > --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> > +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> > @@ -94,6 +94,7 @@
> > #define GICD_TYPE_LPIS               (1U << 17)
> >
> > #define GICD_CTLR_RWP                (1UL << 31)
> > +#define GICD_CTLR_DS                 (1U << 6)
> > #define GICD_CTLR_ARE_NS             (1U << 4)
> > #define GICD_CTLR_ENABLE_G1A         (1U << 1)
> > #define GICD_CTLR_ENABLE_G1          (1U << 0)
> >
>
> Cheers,
> Luca
>
>
>
>