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,

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.
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.
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 ..."

>
> >       if ( p->domain->is_shut_down &&
> >           (p->domain->shutdown_code == SHUTDOWN_suspend) )
> > @@ -115,10 +122,6 @@ static void ctxt_switch_from(struct vcpu *p)
> >       p->arch.tpidrro_el0 = READ_SYSREG(TPIDRRO_EL0);
> >       p->arch.tpidr_el1 = READ_SYSREG(TPIDR_EL1);
> >
> > -    /* Arch timer */
> > -    p->arch.cntkctl = READ_SYSREG32(CNTKCTL_EL1);
> > -    virt_timer_save(p);
> > -
> >       if ( is_32bit_domain(p->domain) && cpu_has_thumbee )
> >       {
> >           p->arch.teecr = READ_SYSREG32(TEECR32_EL1);
> > @@ -170,9 +173,6 @@ static void ctxt_switch_from(struct vcpu *p)
> >       /* VFP */
> >       vfp_save_state(p);
> >
> > -    /* VGIC */
> > -    gic_save_state(p);
> > -
> >       isb();
> >   }
> >
> >
>
> Cheers,
>
> --
> Julien Grall

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