[PATCH] clarify SHUTDOWN_suspend additional argument

[PATCH] clarify SHUTDOWN_suspend additional argument

[Stefano Stabellini]

SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
guests. Those guests pass 0 instead. Clarify the behaviour in the
hypercall description.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
index a30b11d..c170556 100644
--- a/xen/include/public/sched.h
+++ b/xen/include/public/sched.h
@@ -77,8 +77,9 @@
  * @arg == pointer to sched_shutdown_t structure.
  *
  * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
- * hypercall takes an additional extra argument which should be the
- * MFN of the guest's start_info_t.
+ * hypercall takes an additional extra argument which should be:
+ *  - the MFN of the guest's start_info_t for x86 PV guests;
+ *  - 0 for x86 HVM guests and arm and arm64 guests.
  *
  * In addition, which reason is SHUTDOWN_suspend this hypercall
  * returns 1 if suspend was cancelled or the domain was merely

Re: [PATCH] clarify SHUTDOWN_suspend additional argument

[Ian Campbell]

On Wed, 2014-05-07 at 14:05 +0100, Stefano Stabellini wrote:
> SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
> guests. Those guests pass 0 instead. Clarify the behaviour in the
> hypercall description.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

But:
> diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
> index a30b11d..c170556 100644
> --- a/xen/include/public/sched.h
> +++ b/xen/include/public/sched.h
> @@ -77,8 +77,9 @@
>   * @arg == pointer to sched_shutdown_t structure.
>   *
>   * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
> - * hypercall takes an additional extra argument which should be the
> - * MFN of the guest's start_info_t.
> + * hypercall takes an additional extra argument which should be:
> + *  - the MFN of the guest's start_info_t for x86 PV guests;
> + *  - 0 for x86 HVM guests and arm and arm64 guests.

Is this strictly true or is the argument ignored for those guests?
(Requiring it to be zero doesn't conflict with that hence the ack)

>   *
>   * In addition, which reason is SHUTDOWN_suspend this hypercall
>   * returns 1 if suspend was cancelled or the domain was merely

Re: [PATCH] clarify SHUTDOWN_suspend additional argument

[Andrew Cooper]

On 08/05/14 11:11, Ian Campbell wrote:
> On Wed, 2014-05-07 at 14:05 +0100, Stefano Stabellini wrote:
>> SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
>> guests. Those guests pass 0 instead. Clarify the behaviour in the
>> hypercall description.
>>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> But:
>> diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
>> index a30b11d..c170556 100644
>> --- a/xen/include/public/sched.h
>> +++ b/xen/include/public/sched.h
>> @@ -77,8 +77,9 @@
>>   * @arg == pointer to sched_shutdown_t structure.
>>   *
>>   * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
>> - * hypercall takes an additional extra argument which should be the
>> - * MFN of the guest's start_info_t.
>> + * hypercall takes an additional extra argument which should be:
>> + *  - the MFN of the guest's start_info_t for x86 PV guests;
>> + *  - 0 for x86 HVM guests and arm and arm64 guests.
> Is this strictly true or is the argument ignored for those guests?
> (Requiring it to be zero doesn't conflict with that hence the ack)

Having recently played with code here, I believe it is as follows.

* Xen does absolutely nothing with the value whatsoever.
* PV Migration *must* check the value (and indeed converts it to a pfn
for transit)
* HVM guests (including arm for these purposes) don't have easy access
to the register, and just transmit the Xen architectural state blob as-is.

Therefore, the only requirement I can see is that PV guests must point
it at the start_info_t mfn.

~Andrew

>
>>   *
>>   * In addition, which reason is SHUTDOWN_suspend this hypercall
>>   * returns 1 if suspend was cancelled or the domain was merely
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] clarify SHUTDOWN_suspend additional argument

[Ian Campbell]

On Thu, 2014-05-08 at 11:21 +0100, Andrew Cooper wrote:
> On 08/05/14 11:11, Ian Campbell wrote:
> > On Wed, 2014-05-07 at 14:05 +0100, Stefano Stabellini wrote:
> >> SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
> >> guests. Those guests pass 0 instead. Clarify the behaviour in the
> >> hypercall description.
> >>
> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > But:
> >> diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
> >> index a30b11d..c170556 100644
> >> --- a/xen/include/public/sched.h
> >> +++ b/xen/include/public/sched.h
> >> @@ -77,8 +77,9 @@
> >>   * @arg == pointer to sched_shutdown_t structure.
> >>   *
> >>   * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
> >> - * hypercall takes an additional extra argument which should be the
> >> - * MFN of the guest's start_info_t.
> >> + * hypercall takes an additional extra argument which should be:
> >> + *  - the MFN of the guest's start_info_t for x86 PV guests;
> >> + *  - 0 for x86 HVM guests and arm and arm64 guests.
> > Is this strictly true or is the argument ignored for those guests?
> > (Requiring it to be zero doesn't conflict with that hence the ack)
> 
> Having recently played with code here, I believe it is as follows.
> 
> * Xen does absolutely nothing with the value whatsoever.
> * PV Migration *must* check the value (and indeed converts it to a pfn
> for transit)
> * HVM guests (including arm for these purposes) don't have easy access
> to the register, and just transmit the Xen architectural state blob as-is.
> 
> Therefore, the only requirement I can see is that PV guests must point
> it at the start_info_t mfn.

This matched my understanding too, thanks for confirming.

There's no harm in mandating it be zero I suppose, except perhaps we've
just made some OSes undetectably buggy, which would only be a problem if
in the future someone decided "oh, this must be 0, so we can now extend
the interface safely".

Ian.

Re: [PATCH] clarify SHUTDOWN_suspend additional argument

[David Vrabel]

On 07/05/14 14:05, Stefano Stabellini wrote:
> SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
> guests. Those guests pass 0 instead. Clarify the behaviour in the
> hypercall description.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
> index a30b11d..c170556 100644
> --- a/xen/include/public/sched.h
> +++ b/xen/include/public/sched.h
> @@ -77,8 +77,9 @@
>   * @arg == pointer to sched_shutdown_t structure.
>   *
>   * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
> - * hypercall takes an additional extra argument which should be the
> - * MFN of the guest's start_info_t.
> + * hypercall takes an additional extra argument which should be:
> + *  - the MFN of the guest's start_info_t for x86 PV guests;
> + *  - 0 for x86 HVM guests and arm and arm64 guests.

Perhaps word this to reflect what the toolstack actually does?

* x86 PV guest must also set RDX (EDX for 32-bit guests) to the MFN
* of the guest's start info page.  RDX/EDX is the third hypercall
* argument.

David

Re: [PATCH] clarify SHUTDOWN_suspend additional argument

[Andrew Cooper]

On 08/05/14 11:40, Ian Campbell wrote:
> On Thu, 2014-05-08 at 11:21 +0100, Andrew Cooper wrote:
>> On 08/05/14 11:11, Ian Campbell wrote:
>>> On Wed, 2014-05-07 at 14:05 +0100, Stefano Stabellini wrote:
>>>> SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
>>>> guests. Those guests pass 0 instead. Clarify the behaviour in the
>>>> hypercall description.
>>>>
>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> But:
>>>> diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
>>>> index a30b11d..c170556 100644
>>>> --- a/xen/include/public/sched.h
>>>> +++ b/xen/include/public/sched.h
>>>> @@ -77,8 +77,9 @@
>>>>   * @arg == pointer to sched_shutdown_t structure.
>>>>   *
>>>>   * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
>>>> - * hypercall takes an additional extra argument which should be the
>>>> - * MFN of the guest's start_info_t.
>>>> + * hypercall takes an additional extra argument which should be:
>>>> + *  - the MFN of the guest's start_info_t for x86 PV guests;
>>>> + *  - 0 for x86 HVM guests and arm and arm64 guests.
>>> Is this strictly true or is the argument ignored for those guests?
>>> (Requiring it to be zero doesn't conflict with that hence the ack)
>> Having recently played with code here, I believe it is as follows.
>>
>> * Xen does absolutely nothing with the value whatsoever.
>> * PV Migration *must* check the value (and indeed converts it to a pfn
>> for transit)
>> * HVM guests (including arm for these purposes) don't have easy access
>> to the register, and just transmit the Xen architectural state blob as-is.
>>
>> Therefore, the only requirement I can see is that PV guests must point
>> it at the start_info_t mfn.
> This matched my understanding too, thanks for confirming.
>
> There's no harm in mandating it be zero I suppose, except perhaps we've
> just made some OSes undetectably buggy, which would only be a problem if
> in the future someone decided "oh, this must be 0, so we can now extend
> the interface safely".
>
> Ian.
>

Yes - retroactively mandating it to be 0 will 'break' current HVM
guests, which will have whatever value was in %rdx.  For a very long
time this hypercall was mis-documented as a 2-argument hypercall, making
it more likely that something non-0 was in there.

The best that can be safely stated in the comment is that the 3rd
parameter is free for VM use for x86 HVM and ARM guests.  (Unless I
suppose ARM want to restrict this before migration is supported?)

~Andrew

Re: [PATCH] clarify SHUTDOWN_suspend additional argument

[Stefano Stabellini]

On Thu, 8 May 2014, David Vrabel wrote:
> On 07/05/14 14:05, Stefano Stabellini wrote:
> > SCHEDOP_shutdown has a third argument that is unused on HVM and ARM
> > guests. Those guests pass 0 instead. Clarify the behaviour in the
> > hypercall description.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
> > index a30b11d..c170556 100644
> > --- a/xen/include/public/sched.h
> > +++ b/xen/include/public/sched.h
> > @@ -77,8 +77,9 @@
> >   * @arg == pointer to sched_shutdown_t structure.
> >   *
> >   * If the sched_shutdown_t reason is SHUTDOWN_suspend then this
> > - * hypercall takes an additional extra argument which should be the
> > - * MFN of the guest's start_info_t.
> > + * hypercall takes an additional extra argument which should be:
> > + *  - the MFN of the guest's start_info_t for x86 PV guests;
> > + *  - 0 for x86 HVM guests and arm and arm64 guests.
> 
> Perhaps word this to reflect what the toolstack actually does?
> 
> * x86 PV guest must also set RDX (EDX for 32-bit guests) to the MFN
> * of the guest's start info page.  RDX/EDX is the third hypercall
> * argument.
 
That is better. It also the advantage on keeping the sched_op hypercall
always to 2 arguments on ARM, making the Linux side implementation
cleaner.
I'll respin both this patch and the corresponding Linux side.