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 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.
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. Then we can
say whether those events should wake-up the guest or not.

Thanks,
Mirela

> Cheers,
>
> --
> Julien Grall

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