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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

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?

~Andrew

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