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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the detailed review.

On Wed, May 13, 2026 at 7:12 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > +}
> > +
> > +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");
>
> I think here we should clear GICR_WAKER_ProcessorSleep, the Arm IHI 0069H.b, 
> section
> 11.1 says that
>
> “”"
> When GICR_WAKER.ProcessorSleep == 1 or GICR_WAKER.ChildrenAsleep == 1 then a 
> write to any GICC_*,
> GICV_*, GICH_*, ICC_*, ICV_*, or ICH_* registers, other than those in the 
> following list, is unpredictable:
> • ICC_SRE_EL1.
> • ICC_SRE_EL2.
> • ICC_SRE_EL3
> “""
> But in the error path used in gicv3_suspend() we are writing ICH_HCR_EL2 and 
> ICC_IGRPEN1_EL1.

Agreed. I will fix the abort path by calling gicv3_enable_redist() before
restoring the CPU/virtual interface state.

>
> > +            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_config(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);
> > +}
> > +
> > +static void gicv3_restore_spi_irq_routing(struct dist_irq_block *irqs,
> > +                                          unsigned int i, bool is_espi)
> > +{
> > +    void __iomem *base;
> > +    unsigned int 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);
> > +}
> > +
> > +static void gicv3_restore_spi_irq_state(struct dist_irq_block *irqs,
> > +                                        unsigned int i, bool is_espi)
> > +{
> > +    void __iomem *base;
> > +
> > +    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();
>
> Should we check that ICC_AP1R<n>_EL1 == 0 before continuing our
> suspend? Like we do in the GICv2?

Yes, I agree. This is the GICv3 equivalent of the GICv2
GICC_APR<n> check for the physical CPU interface active-priority
state.

I will use ICH_VTR_EL2.PREbits to decide which AP1R registers are
implemented, so we do not read an unimplemented ICC_AP1R<n>_EL1
register.

>
> > +
> > +    ret = gicv3_disable_redist();
> > +    if ( ret )
> > +        goto out_enable_iface;
> > +
> > +    /* 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.grpen, ICC_IGRPEN1_EL1);
> > +    isb();
> > +
> > +    return ret;
> > +}
> > +
> > +static void gicv3_resume(void)
> > +{
> > +    int ret;
> > +    unsigned int i;
> > +    uint32_t dist_ctlr;
> > +    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_config(gicv3_ctx.dist.irqs + i - 1, i, 
> > false);
> > +
> > +#ifdef CONFIG_GICV3_ESPI
> > +    for ( i = 0; i < gic_number_espis() / 32; i++ )
> > +    {
> > +        writel_relaxed(GENMASK(31, 0), GICD + GICD_IGROUPRnE + i * 4);
> > +        gicv3_restore_spi_irq_config(gicv3_ctx.dist.espi_irqs + i, i, 
> > true);
> > +    }
> > +#endif
> > +
> > +    dist_ctlr = gicv3_ctx.dist.ctlr & GICD_CTLR_ARE_NS;
> > +    if ( dist_ctlr )
> > +    {
> > +        writel_relaxed(dist_ctlr, GICD + GICD_CTLR);
> > +        gicv3_dist_wait_for_rwp();
> > +
> > +        for ( i = 1; i < DIV_ROUND_UP(gicv3_info.nr_lines, 32); i++ )
> > +            gicv3_restore_spi_irq_routing(gicv3_ctx.dist.irqs + i - 1, i,
> > +                                          false);
>
> I think we have an issue in this loop as we are accessing 
> GICD_IROUTER<1020…1023>
> and GICD_IPRIORITYR255, while the specs says 1020…1023 are reserved and
> GICD_IPRIORITYR<n> goes from 0 to 254. here and in the gicv3_suspend()

Ack. I will fix this.

Best regards,
Mykola