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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?
>
> 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).

Why not have the SYSTEM_SUSPEND check that all other vCPUs are DOWN
first, and fail the call if not?

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

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