| Message ID | alpine.DEB.2.02.1405071402550.14596@kaball.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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
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.
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
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>