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

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3953 bytes --]

On Tue, 13 Nov 2018, Julien Grall 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.
> > 
> > That is true, but it is not only used for migration. It is also used
> > for suspending a guest and saving its state to file with the intention
> > of resuming it later from file.
> 
> Which is some sort of migration at the end. However, they don't have the same
> semantics as suspend/resume regarding the state of the vCPU.
 
Right

 
> > > 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.
> > 
> > Future work will need toolstack support for suspending/resuming guests.
> > SHUTDOWN_suspend is the most natural fit today; we don't want to hijack
> > domain pause, because if we do, then we can't normally pause a domain
> > anymore. 
> 
> Why? suspend/resume is like pausing the domain but will be resumed on event
> (e.g interrupts) rather than user request.

I meant to say two things.

1) Which domain state should we use for suspended guests?

This patch is using SHUTDOWN_suspend. Taken on its own, a regular "pause
state" sounds like an option, in fact it is necessary to set the domain
as paused otherwise the scheduler will schedule it. But in addition we
need to distinguish a normal paused state from a PSCI system suspend
state. We need to know that the domain has been system-suspended with
PSCI, not just paused.


2) This ties into the discussion of what xl commands we want to use to
request a domU to suspend. We don't need to talk about it now, but at
some point we'll want something equivalent to "xl save" or "xl trigger
sleep" and "xl restore" or "xl trigger s3resume" for this suspended
state.

If we mark the domU simply as "paused" it will be difficult to implement
correctly "xl restore" / "xl trigger s3resume". We should be able to
distinguish a domain which is simply not running or paused (as in "xl
pause") from one that has been put to sleep.  Otherwise we won't be able
to use "xl pause" in its original sense anymore. "xl pause" will become
effectively "xl trigger sleep" on ARM. Which is something to consider
but I don't think that is what we want.

The most similar feature is ACPI S3 in x86-land but the current calls
are so ACPI/x86 specific that don't make much sense on a ACPI-less ARM
implementation. If I am not mistaken, on x86 there is a special struct
domain flag for this: d->arch.hvm.is_s3_suspended. 


Let's leave aside the "which commands should we use" discussion for now
because it doesn't related to this patch series. It seems to me that the
best option is to introduce a new ARM specific struct domain flag,
something akin to d->arch.hvm.is_s3_suspended but ARM PSCI specific.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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