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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the detailed review.

On Wed, May 13, 2026 at 5:09 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> 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);
>         }
>
>         […]
>     }

This was intentional to keep the logic simpler.

For the 1020-interrupt case, the extra word would correspond to
interrupt IDs 1020-1023. My reading of ARM IHI 0048B.b is that this
is architecturally harmless: section 4.1.2 says that reserved
Distributor register addresses are RAZ/WI, and Table 4-1 marks
GICD_IPRIORITYR offset 0x7fc and GICD_ITARGETSR offset 0xbfc as
Reserved.

Would you be OK with keeping this as-is, or would you prefer me to add
the cap logic anyway?

>
> > +
> > +        /*
> > +         * 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.

Good catch, I agree.

I will move the GICD_ICFGR restore after the GICD_ICENABLER writes
and before restoring GICD_ISENABLER, so that programmable Int_config
fields are restored while the corresponding interrupts are disabled.

I will also check whether it makes sense to move the other
configuration restores before GICD_ISENABLER as well. The spec does
not seem to impose the same strict requirement there, but keeping all
configuration restores before re-enabling the interrupts might make
the ordering clearer.

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

For GICD_ICFGR0, my intention was to keep the restore loop uniform.
There should be no useful SGI state to restore here: section 4.3.13
says that SGI Int_config[1] is not programmable and RAO/WI, while
Int_config[0] is reserved. Also, the value written is the value
previously read from the same register.

So I do not expect this write to affect the architected SGI
configuration. However, if you prefer avoiding the write to
GICD_ICFGR0 explicitly, I will skip it in the next version of
this series.

Best regards,
Mykola