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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> 
>> 
>>> +        }
>>> +
>>> +        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?
> 
> Yes, this was intentional.
> 
> The GICv2 suspend callback is called at a quiescent point in the
> SYSTEM_SUSPEND path: all domains are already shut down for suspend, guest
> execution is quiesced, the scheduler is disabled, non-boot CPUs have been
> offlined, and CPU0 enters gic_suspend() with local interrupts disabled.
> 
> For SGIs, I don't consider GICD_CPENDSGIRn/GICD_SPENDSGIRn part of the saved
> host GIC context. Xen uses physical SGIs as IPIs, and IPI delivery is an
> internal synchronization mechanism, not architectural state that should be
> replayed after SYSTEM_SUSPEND. Guest SGI state is virtual GIC state and is not
> represented by these physical GICD SGI pending registers.

ack, I would maybe mention in the commit message that we exclude transient 
IPI/active-priority
state at the suspend quiescent point.

> 
> For GICC_APRn/GICC_NSAPRn, those registers describe active priority state for
> interrupts already acknowledged by the CPU interface. The final suspend path 
> is
> not expected to run with an active physical interrupt context. If those
> registers were non-zero there, restoring only APR/NSAPR would not make the
> corresponding interrupt handling context valid after resume, and could instead
> leave the CPU interface with stale active priority state.

Ok I understand now, but if we are expecting here GICD_ISACTIVERn zeroed, why 
are
we saving/restoring it? Shouldn’t we instead have a runtime check that it’s 
zero and in case
it’s not bail out? And in the resume path we would only zero it.

Am I missing something?

> 
> So I did not add save/restore for GICD_CPENDSGIRn/GICD_SPENDSGIRn or
> GICC_APRn/GICC_NSAPRn in this patch. I can add a short comment in v9 to make
> this scope explicit.
> 
> Please let me know if you think there is a suspend/resume path where this
> state still needs to be preserved.
> 
> Best regards,
> Mykola

Cheers,
Luca