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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 15 Nov 2018, Mirela Simonovic wrote:
> Hi Julien,
> 
> On Thu, Nov 15, 2018 at 12:38 PM Julien Grall <julien.grall@xxxxxxx> wrote:
> >
> > Hi,
> >
> > On 11/15/18 11:10 AM, Mirela Simonovic wrote:
> > > Hi Julien,
> > >
> > > On Thu, Nov 15, 2018 at 11:59 AM Julien Grall <julien.grall@xxxxxxx> 
> > > wrote:
> > >>
> > >> Hi Mirela,
> > >>
> > >> On 11/15/18 10:33 AM, Mirela Simonovic wrote:
> > >>> On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
> > >>> <andrew.cooper3@xxxxxxxxxx> wrote:
> > >>>>
> > >>>> On 15/11/2018 10:13, Julien Grall wrote:
> > >>>>> (+ 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.
> > >>>>
> > >>>> If you observe, that can only be actioned by a hypercall from qemu.  It
> > >>>> can't be actioned by an ACPI controller emulated by Xen.
> > >>>>
> > >>>>>
> > >>>>>>
> > >>>>>> 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?
> > >>>>>
> > >>>
> > >>> That's correct, and I agree. BTW that is what's implemented in this 
> > >>> series.
> > >>>
> > >>>>> 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.
> > >>>>>
> > >>>
> > >>> What other events are talking about here?
> > >>
> > >> vcpu_unblock is not only called when you receive interrupts. It can be
> > >> called in other place when you receive an events.
> > >>
> > >>   From the Arm Arm, an event can be anything. So do we really want to
> > >> wake-up on any events?
> > >>
> > >>>
> > >>>>> 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).
> > >>>>
> > >>>
> > >>> Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
> > >>> the case, SYSTEM_SUSPEND will return an error.
> > >>> Could you please clarify what a potential corner case would be?
> > >>
> > >> PSCI CPU_ON is not the only way to online a vCPU. I merely want to
> > >> prevent other path to play with the vCPU when it is not necessary.
> > >>
> > >> [...]
> > >>
> > >>>> If instead of waiting for any event, you need to wait for a specific
> > >>>> event, there is also vcpu_poll() which is a related scheduler API.
> > >>>>
> > >>>> ~Andrew
> > >>>
> > >>> Some content disappeared, so I'll write here to avoid thread branching.
> > >>>
> > >>> The semantic of vCPU block and domain_pause is not the same. When
> > >>> guest suspends the domain is not (and should not be) paused, instead
> > >>> its last online vCPU is blocked waiting on an interrupt. That's it.

From the scheduler's point of view, a suspended domain is not running,
so it is basically "paused". We are "pausing the scheduling" of it. Also
looking at the implementation of hvm_s3_suspend, I think it is fine to
call domain_pause() on ARM too.

However, also like hvm_s3_suspend, we need to set an additional special
flag (such as d->arch.hvm.is_s3_suspended) to make sure we know how to
differentiate a paused domain from a suspended domain: a user should not
be able to resume a domain with "xl unpause", they should use something
like xl trigger s3resume. We should not lose the differentiation between
suspend and pause in our internal state tracking.

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