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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> 
>> 
>>> 
>>> 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?
> 
> Good questions.
> 
> Yes, the distinction I should have made clearer is between CPU-interface
> active-priority state and distributor active state.
> 
> For GICC_APRn/GICC_NSAPRn, I expect the state to be quiesced at this point.
> Those registers track active priorities in the CPU interface. Xen reaches
> gic_suspend() with local interrupts disabled, and for the guest-routed
> interrupt case that can leave a distributor active bit behind, Xen has
> already performed the physical EOI, so the CPU-interface priority has been
> dropped.
> There is no CPU-interface active-priority context that we can meaningfully
> replay after resume.
> 
> That is different from GICD_ISACTIVERn. In EOImode==1, EOIR only drops the
> priority. The interrupt remains active in the distributor until the separate
> deactivation step. For a guest-routed interrupt Xen's GICv2 guest end path 
> does
> only the physical EOI; deactivation is completed later by the virtual GIC/GICV
> path when the guest completes the interrupt.
> 
> This is why APR/NSAPR and ISACTIVERn are treated differently. For example:
> 
>  1. A physical IRQ routed to a guest is acknowledged by Xen.
>  2. The GIC marks the interrupt active in the distributor.
>  3. Xen EOIs it, which drops the physical priority.
>  4. Xen queues/injects the interrupt to the vGIC.
>  5. The guest has not yet run, or the virtual interrupt is not yet deliverable
>     because of guest PMR/priority/local IRQ masking/vGIC state.
>  6. Therefore the guest-side deactivate has not happened yet, and the physical
>     distributor active bit remains set.
> 
> There is also a late suspend window in the current Xen path: domains are
> suspended and the scheduler is disabled before local IRQs are disabled.
> A guest-routed IRQ can therefore be taken by Xen after the guest is already
> suspended, but before gic_suspend(). Xen can EOI/priority-drop it and queue
> it for the guest, while the guest cannot run and deactivate it before the
> GIC state is saved.
> 
> This is the same class of issue handled by Linux for GIC EOImode==1. Linux
> saves/restores the active state because forwarded interrupts can remain active
> while passed to a VM [1].
> 
> So I don't think GICD_ISACTIVERn should be treated as "must be zero" unless we
> also add an explicit suspend-abort/quiesce policy for in-flight guest
> interrupts. That would be a different design: detect non-zero active/in-flight
> state, unwind suspend, thaw domains, let the guest drain/deactivate the
> interrupts, and retry later. This series does not implement that policy. Given
> the current flow, preserving GICD_ISACTIVERn avoids losing architectural
> interrupt-controller state across suspend/resume.
> 
> I am not opposed to such a policy as a follow-up if we want stricter suspend
> quiescence rules, but I think it should be designed explicitly rather than
> inferred from the GIC save/restore code.
> 
> Best regards,
> Mykola
> 
> [1] 
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/1447701208-18150-5-git-send-email-marc.zyngier@xxxxxxx/

Right, yes I agree! I have another question though, since GICC_APRn state 
should be
quiesced in the suspend path (allimplemented active-priority bits should read 
as zero),
should we have a runtime check just after disabling the CPU interface?

Cheers,
Luca