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

[Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>]

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.
> >> Well no, you will block until you receive an event. Interrupts are one
> >> of them.
> >>
> >> Do we want to consider all events as wakeup event?
> >>
> >
> > I think we need to assume that events are not triggered via toolstack,
> > Andrew made a good point.
>
> I don't think we are discussing the same thing. The discussion was
> around other vCPUs, not the vCPU calling SYSTEM_SUSPEND.
>
> Most likely in the future, we would want to allow the toolstack to
> request resuming the domain. This can be considered as an event.
>

Yes, such an event will unblock the vcpu and cause the domain to
resume. So from this perspective it's not only an interrupt targeted
to the guest.

> > Given the assumption, my understanding is that Xen itself will not
> > unblock vCPU, except due to an interrupt targeted to the guest.
> > Am I missing something? An example would be appreciated.
>
> At least on Arm, the current semantics of vcpu_block/vcpu_unblock is to
> block until you receive an events.
>
> I don't much want to restrict the definition of events to only
> interrupts.  To clarify my point, if you want to wake-up for any events
> then fine. But this needs to be understood that it may not be only
> interrupts.
>

In the context described above this is fine - events are not only interrupts.

> Cheers,
>
> --
> Julien Grall

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