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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Fri, May 15, 2026 at 12:53 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 15 May 2026, at 08:59, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> >
> > 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?
>
> I think this would be the only part in the driver that does that, also Linux 
> is avoiding
> to touch these reserved parts
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic.c?h=v7.1-rc3#n582)
> My preference would be to be consistent.

Ack. I'll add the cap logic.

>
> >
> >>
> >>> +
> >>> +        /*
> >>> +         * 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.
>
> I think also Linux “restores” it, so I’m ok to keep the code as it is, in 
> fact it’s read-only
> and not marked as reserved, my bad!

Ack. I'll keep this part as-is here.

Best regards,
Mykola

>
> Cheers,
> Luca
>