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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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.

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

Cheers,
Luca