Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 11/15/18 12:35 AM, Stefano Stabellini wrote:
> On Wed, 14 Nov 2018, Julien Grall wrote:
> > On 14/11/2018 22:45, Stefano Stabellini wrote:
> > > On Wed, 14 Nov 2018, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 13/11/2018 20:44, Stefano Stabellini wrote:
> > > > > On Mon, 12 Nov 2018, Julien Grall wrote:
> > > > > > (+ Andre)
> > > > > >
> > > > > > On 11/12/18 5:42 PM, Mirela Simonovic wrote:
> > > > > > > Hi Julien,
> > > > > > >
> > > > > > > On Mon, Nov 12, 2018 at 6:00 PM Julien Grall <julien.grall@xxxxxxx>
> > > > > > > wrote:
> > > > > > > > On 11/12/18 4:52 PM, Mirela Simonovic wrote:
> > > > > > > > > Hi Julien,
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > > Thanks for the feedback.
> > > > > > > > >
> > > > > > > > > On Mon, Nov 12, 2018 at 4:36 PM Julien Grall <julien.grall@xxxxxxx>
> > > > > > > > > wrote:
> > > > > > > > > > Hi Mirela,
> > > > > > > > > >
> > > > > > > > > > On 11/12/18 11:30 AM, Mirela Simonovic wrote:
> > > > > > > > > > > GIC and virtual timer context must be saved when the domain
> > > > > > > > > > > suspends.
> > > > > > > > > > Please provide the rationale for this. Is it required by the spec?
> > > > > > > > > This is required for GIC because a guest leaves enabled interrupts
> > > > > > > > > in
> > > > > > > > > the GIC that could wake it up, and on resume it should be able to
> > > > > > > > > detect which interrupt woke it up. Without saving/restoring the
> > > > > > > > > state
> > > > > > > > > of GIC this would not be possible.
> > > > > > > > I am confused. In patch #10, you save the GIC host because the GIC can
> > > > > > > > be powered-down. Linux is also saving the GIC state. So how the
> > > > > > > > interrupt can come up if the GIC is powered down?
> > > > > > > After Xen (or Linux in the config without Xen) hands over the control
> > > > > > > to EL3 on suspend (calls system suspend PSCI to EL3), it leaves
> > > > > > > enabled interrupts that are its wake-up sources. Saving a GIC state
> > > > > > > only means that its current configuration will be remembered somewhere
> > > > > > > in data structures, but the configuration is not changed on suspend.
> > > > > > > Everything that happens with interrupt configuration from this point
> > > > > > > on is platform specific. Now there are 2 options: 1) EL3 will never
> > > > > > > allow CPU0 to be powered down and the wake-up interrupt will indeed
> > > > > > > propagate via GIC;
> > > > > > > or 2) CPU0 will be powered down and the GIC may be
> > > > > > > powered down as well, so an external help is needed to deal with
> > > > > > > wake-up and resume (e.g. in order to react to a wake-up interrupt
> > > > > > > while the GIC is down, and power up CPU0). This external help is
> > > > > > > provided by some low-power processor outside of the Cortex-A cluster.
> > > > > > >
> > > > > > > So the platform firmware is responsible for properly configuring a
> > > > > > > wake-up path if GIC goes down. This is commonly handled by EL3
> > > > > > > communicating with low-power processor. When the GIC power comes up,
> > > > > > > the interrupt triggered by a peripheral is still active and the
> > > > > > > software on Cortex-A cluster should be able to observe it once the GIC
> > > > > > > state is restored, i.e. interrupts get enabled at GIC.
> > > > > > Thank you for the explanation.  Now the question is why can't we reset at
> > > > > > least the GIC CPU interface?
> > > > > >
> > > > > > AFAIU, the guest should restore them in any case. The only things we need
> > > > > > to
> > > > > > know is the interrupt was received for a given guest. We can then queue it
> > > > > > and
> > > > > > wake-up the domain.
> > > > > >
> > > > > > This seems to fit with the description on top of gic_dist_save() in Linux
> > > > > > GICv2 driver.
> > > > > Can we rely on all PSCI compliant OSes to restore their own GIC again at
> > > > > resume? The PSCI spec is not very clear in that regard (at the the
> > > > > version I am looking at...) I am just asking so that we don't come up
> > > > > with a solution that only works with Linux.
> > > > See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context
> > > > because the PSCI implementations is allowed to shutdown the GIC.
> > > Great, in that case we should be able to skip saving some of the GICD
> > > registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR,
> > > but we should be able to skip the others (GICD_ISENABLER,
> > > GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to
> > > re-initialize them as we do in gicv2_dist_init.
> > You are assuming a domain will handle properly the suspend/resume. I
> > don't think we can promise that as we call freeze/thaw.
> Dho! That would break every single guest that has been forcefully
> suspended :-/
>
> Right, we can't do that (FYI I tested today the series with an unaware
> domU and it all resumed correctly.)
>
> But given that we only suspend/resume GICC_CTLR, GICC_PMR, GICC_BPR of
> the GICC interface, it should be fine to re-initialize that. We do need
> to be careful because the current implementation of gicv2_cpu_init
> touches a bunch of GICD registers that we'll have to save separately for
> suspend/resume.
See my review on patch #10. I suggested to move out GICC_CTLR, GICC_PMR, 
GICC_BPR in a separate helpers that could be called by gicv2_cpu_init 
and the new function.
A good name for that helper would be gicv2_cpu_if_up().

Cheers,


--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel