Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)

[Julien Grall <julien.grall@xxxxxxx>]

(+ Andre)

On 11/15/18 12:47 AM, Andrew Cooper wrote:
> On 14/11/2018 12:49, Julien Grall wrote:
> > Hi Mirela,
> >
> > On 14/11/2018 12:08, Mirela Simonovic wrote:
> > > On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> > > > On 12/11/2018 19:56, Julien Grall wrote:
> > > > > Hi Andrew,
> > > > >
> > > > > On 11/12/18 4:41 PM, Andrew Cooper wrote:
> > > > > > On 12/11/18 16:35, Mirela Simonovic wrote:
> > > > > > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > > > > > > > index e594b48d81..7f8105465c 100644
> > > > > > > > > --- a/xen/arch/arm/domain.c
> > > > > > > > > +++ b/xen/arch/arm/domain.c
> > > > > > > > > @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
> > > > > > > > >          if ( is_idle_vcpu(p) )
> > > > > > > > >              return;
> > > > > > > > >
> > > > > > > > > +    /* VCPU's context should not be saved if its domain is
> > > > > > > > > suspended */
> > > > > > > > > +    if ( p->domain->is_shut_down &&
> > > > > > > > > +        (p->domain->shutdown_code == SHUTDOWN_suspend) )
> > > > > > > > > +        return;
> > > > > > > > SHUTDOWN_suspend is used in Xen for other purpose (see
> > > > > > > > SCHEDOP_shutdown). The other user of that code relies on all the
> > > > > > > > state
> > > > > > > > to be saved on suspend.
> > > > > > > We just need a flag to mark a domain as suspended, and I do believe
> > > > > > > SHUTDOWN_suspend is not used anywhere else.
> > > > > > > Let's come back on this.
> > > > > > SHUTDOWN_suspend is used for migration.  Grep for it through the Xen
> > > > > > tree and you'll find several pieces of documentation, including the
> > > > > > description of what this shutdown code means.
> > > > > >
> > > > > > What you are introducing here is not a shutdown - it is a suspend
> > > > > > with
> > > > > > the intent to resume executing later.  As such, it shouldn't use
> > > > > > Xen's
> > > > > > shutdown infrastructure, which exists mainly to communicate with the
> > > > > > toolstack.
> > > > > Would domain pause/unpause be a better solution?
> > > > Actually yes - that sounds like a very neat solution.
> > > I believe domain pause will not work here - a domain cannot pause
> > > itself, i.e. current domain cannot be paused. This functionality
> > > seems to assume that a domain is pausing another domain. Or I missed
> > > the point.
> > Yes domain pause/unpause will not work. However, you can introduce a
> > boolean to tell you whether the domain was suspend.
> >
> > I actually quite like how suspend work for x86 HVM. This is based on
> > pause/unpause. Have a look at hvm_s3_{suspend/resume}.
> That only exists because the ACPI controller is/was implemented in
> QEMU.  I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from Arm 
here.
> Having spent some more time thinking about this problem, what properties
> does the PSCI call have?
>
> I gather from other parts of this thread that there may be a partial
> reset of state?  Beyond that, what else?
>
> I ask, because conceptually, domU suspend to RAM is literally just
> "de-schedule until we want to wake up", and in this case, it appears to
> be "wake up on any of the still-active interrupts".  We've already got a
> scheduler API for that, and its called vcpu_block().  This already
> exists with "wait until a new event occurs" semantics.
>
> Is there anything else I've missed?
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also, only 
events on that vCPU can trigger a resume. All the other event should not 
be taken into account.
My worry with vcpu_block() is we don't prevent the other vCPUs to run. 
Technically they should be off, but I would like some safety to avoid 
any potential corner case (i.e other way to turn a vCPU on).
That's why I think domain_pause() is more suitable in that case. We 
could unpause the domain either when receiving an interrupt or when 
requested by the toolstack).
Cheers,

--
Julien Grall

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