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

[Julien Grall <julien.grall@xxxxxxx>]

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?
> 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