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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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]

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

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

Cheers,
Luca