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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

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

apologies for spotting these only now, in case gicv2_info.nr_lines is 1020,
here and below for GICD_ITARGETSR we are going to save also IDs 1020-1023
which are reserved.

Could we assume irqs->ipriorityr and irqs->itargetsr have the same size and 
implement
some cap logic which might cap the last loop (eventually):

    for ( i = 0; i < blocks; i++ )
    {
        struct irq_block *irqs = gic_ctx.dist.irqs + i;
        size_t j, off = i * sizeof(irqs->isenabler);
        size_t nr_regs = ARRAY_SIZE(irqs->ipriorityr);

        if ( i == blocks - 1 )
            nr_regs = DIV_ROUND_UP(gicv2_info.nr_lines - i * 32, 4);

        […]

        off = i * sizeof(irqs->ipriorityr);
        for ( j = 0; j < nr_regs; j++ )
            writel_gicd(irqs->ipriorityr[j], GICD_IPRIORITYR + off + j * 4);

        /*
         * GICD_ITARGETSR0..7 cover SGIs/PPIs and hold no state to save:
         * they are read-only on multiprocessor implementations and RAZ/WI
         * on uniprocessor implementations.
         */
        if ( i )
        {
            off = i * sizeof(irqs->itargetsr);
            for ( j = 0; j < nr_regs; j++ )
                writel_gicd(irqs->itargetsr[j], GICD_ITARGETSR + off + j * 4);
        }

        […]
    }

> +
> +        /*
> +         * GICD_ITARGETSR0..7 cover SGIs/PPIs and hold no state to save:
> +         * they are read-only on multiprocessor implementations and RAZ/WI
> +         * on uniprocessor implementations.
> +         */
> +        if ( i )
> +        {
> +            off = i * sizeof(irqs->itargetsr);
> +            for ( j = 0; j < ARRAY_SIZE(irqs->itargetsr); j++ )
> +                writel_gicd(irqs->itargetsr[j], GICD_ITARGETSR + off + j * 
> 4);
> +        }
> +
> +        off = i * sizeof(irqs->icfgr);
> +        for ( j = 0; j < ARRAY_SIZE(irqs->icfgr); j++ )
> +            writel_gicd(irqs->icfgr[j], GICD_ICFGR + off + j * 4);

in the GICv2 specs the usage constraints
of GICD_ICFGR says: “Before changing the value of a programmable Int_config 
field,
software must disable the corresponding interrupt, otherwise GIC behavior is
UNPREDICTABLE"

ARM IHI 0048B.b, 4.3.13.

I think we should move this restore just after GICD_ICENABLER write, before 
writing
GICD_ISENABLER.

And also the section says that the GICD_ICFGR0 is read-only.

Let me know your thoughts on that.

> +    }
> +
> +    /* 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);
> +}
> +
> +static void __init gicv2_alloc_context(void)
> +{
> +    uint32_t blocks = DIV_ROUND_UP(gicv2_info.nr_lines, 32);
> +
> +    gic_ctx.dist.irqs = xzalloc_array(struct irq_block, blocks);
> +    if ( !gic_ctx.dist.irqs )
> +        panic("Failed to allocate memory for GICv2 suspend context\n");
> +}
> +
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
> #ifdef CONFIG_ACPI
> static unsigned long gicv2_get_hwdom_extra_madt_size(const struct domain *d)
> {
> @@ -1312,6 +1484,11 @@ static int __init gicv2_init(void)
> 
>     spin_unlock(&gicv2.lock);
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +    /* Allocate memory to be used for saving GIC context during the suspend 
> */
> +    gicv2_alloc_context();
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> +
>     return 0;
> }
> 
> @@ -1355,6 +1532,10 @@ static const struct gic_hw_operations gicv2_ops = {
>     .map_hwdom_extra_mappings = gicv2_map_hwdom_extra_mappings,
>     .iomem_deny_access   = gicv2_iomem_deny_access,
>     .do_LPI              = gicv2_do_LPI,
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +    .suspend             = gicv2_suspend,
> +    .resume              = gicv2_resume,
> +#endif /* CONFIG_SYSTEM_SUSPEND */
> };
> 

I don’t have any other findings for this patch.

Cheers,
Luca