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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

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.

> > For timer, my understanding is that vtimer accounts for how long the
> > guest was executing and this tracking should not be affected by the
> > suspend/resume sequence. Without saving/restoring the state of vtimer,
> > the time tracking would be affected by the suspend/resume.
>
> Currently, vtimer does not take such things into account. It only setup
> a timer in Xen to wake up the guest later on.
>
> > Hope this is ok, I'll add it to the commit message.
> >
> >>> This is done by moving the respective code in ctxt_switch_from()
> >>> before the return that happens if the domain suspended.
> >>>
> >>> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >>> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> >>> ---
> >>>    xen/arch/arm/domain.c | 14 +++++++-------
> >>>    1 file changed, 7 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >>> index 7f8105465c..bebe3238e8 100644
> >>> --- a/xen/arch/arm/domain.c
> >>> +++ b/xen/arch/arm/domain.c
> >>> @@ -97,6 +97,13 @@ static void ctxt_switch_from(struct vcpu *p)
> >>>        if ( is_idle_vcpu(p) )
> >>>            return;
> >>>
> >>> +    /* VGIC */
> >>> +    gic_save_state(p);
> >>> +
> >>> +    /* Arch timer */
> >>> +    p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
> >>> +    virt_timer_save(p);
> >>> +
> >>>        /* VCPU's context should not be saved if its domain is suspended */
> >>
> >> The GIC and timer are part of the vCPU context. So this comment looks a
> >> bit stale.
> >
> > It's not stale, but incorrect in that perspective. The comment should
> > be updated to "VCPU architecture specific context should ..."
>
> But the timer is part of the architecture...
>
> Cheers,
>
> --
> Julien Grall

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