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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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.

> +            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?

> +
> +    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()

Cheers,
Luca