[PATCH] tools: libxl: do not set the PoD target on ARM

[PATCH] tools: libxl: do not set the PoD target on ARM

[Ian Campbell]

ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.

The correct solution here would be to check for ENOSYS in libxl, unfortunately
xc_domain_set_pod_target suffers from the same broken error reporting as the
rest of libxc and throws away the errno.

So for now conditionally define xc_domain_set_pod_target to return success
(which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
errno==-1 and returns -1, which matches the broken error reporting of the
existing function. It appears to have no in tree callers in any case.

The conditional should be removed once libxc has been fixed.

This makes ballooning (xl mem-set) work for ARM domains.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: george.dunlap@citrix.com
---
I'd be generally wary of modifying the error handling in a piecemeal way, but
certainly doing so for 4.4 now would be inapropriate.

IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
frame, at which point this conditional stuff could be dropped.

In terms of the 4.4 release, obviously ballooning would be very nice to have
for ARM guests, on the other hand I'm aware that while the patch is fairly
small/contained and safe it is also pretty skanky and likely wouldn't be
accepted outside of the rc period.
---
 tools/libxc/xc_domain.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index c2fdd74..e1d1bec 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -986,6 +986,12 @@ out:
     return rc;
 }
 
+/* Currently only implemented on x86. This cannot be handled in the
+ * caller, e.g. by looking for errno==ENOSYS because of the broken
+ * error reporting style. Once this is fixed then this condition can
+ * be removed.
+ */
+#if defined(__i386__)||defined(__x86_64__)
 static int xc_domain_pod_target(xc_interface *xch,
                                 int op,
                                 uint32_t domid,
@@ -1055,6 +1061,28 @@ int xc_domain_get_pod_target(xc_interface *xch,
                                 pod_cache_pages,
                                 pod_entries);
 }
+#else
+int xc_domain_set_pod_target(xc_interface *xch,
+                             uint32_t domid,
+                             uint64_t target_pages,
+                             uint64_t *tot_pages,
+                             uint64_t *pod_cache_pages,
+                             uint64_t *pod_entries)
+{
+    return 0;
+}
+int xc_domain_get_pod_target(xc_interface *xch,
+                             uint32_t domid,
+                             uint64_t *tot_pages,
+                             uint64_t *pod_cache_pages,
+                             uint64_t *pod_entries)
+{
+    /* On x86 (above) xc_domain_pod_target will incorrectly return -1
+     * with errno==-1 on error. Do the same for least surprise. */
+    errno = -1;
+    return -1;
+}
+#endif
 
 int xc_domain_max_vcpus(xc_interface *xch, uint32_t domid, unsigned int max)
 {
-- 
1.7.10.4

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[Ian Campbell]

On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
> ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.
> 
> The correct solution here would be to check for ENOSYS in libxl, unfortunately
> xc_domain_set_pod_target suffers from the same broken error reporting as the
> rest of libxc and throws away the errno.
> 
> So for now conditionally define xc_domain_set_pod_target to return success
> (which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
> errno==-1 and returns -1, which matches the broken error reporting of the
> existing function. It appears to have no in tree callers in any case.
> 
> The conditional should be removed once libxc has been fixed.
> 
> This makes ballooning (xl mem-set) work for ARM domains.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: george.dunlap@citrix.com
> ---
> I'd be generally wary of modifying the error handling in a piecemeal way, but
> certainly doing so for 4.4 now would be inapropriate.
> 
> IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
> frame, at which point this conditional stuff could be dropped.
> 
> In terms of the 4.4 release, obviously ballooning would be very nice to have
> for ARM guests, on the other hand I'm aware that while the patch is fairly
> small/contained and safe it is also pretty skanky and likely wouldn't be
> accepted outside of the rc period.

George -- what do you think of this?

> ---
>  tools/libxc/xc_domain.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index c2fdd74..e1d1bec 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -986,6 +986,12 @@ out:
>      return rc;
>  }
>  
> +/* Currently only implemented on x86. This cannot be handled in the
> + * caller, e.g. by looking for errno==ENOSYS because of the broken
> + * error reporting style. Once this is fixed then this condition can
> + * be removed.
> + */
> +#if defined(__i386__)||defined(__x86_64__)
>  static int xc_domain_pod_target(xc_interface *xch,
>                                  int op,
>                                  uint32_t domid,
> @@ -1055,6 +1061,28 @@ int xc_domain_get_pod_target(xc_interface *xch,
>                                  pod_cache_pages,
>                                  pod_entries);
>  }
> +#else
> +int xc_domain_set_pod_target(xc_interface *xch,
> +                             uint32_t domid,
> +                             uint64_t target_pages,
> +                             uint64_t *tot_pages,
> +                             uint64_t *pod_cache_pages,
> +                             uint64_t *pod_entries)
> +{
> +    return 0;
> +}
> +int xc_domain_get_pod_target(xc_interface *xch,
> +                             uint32_t domid,
> +                             uint64_t *tot_pages,
> +                             uint64_t *pod_cache_pages,
> +                             uint64_t *pod_entries)
> +{
> +    /* On x86 (above) xc_domain_pod_target will incorrectly return -1
> +     * with errno==-1 on error. Do the same for least surprise. */
> +    errno = -1;
> +    return -1;
> +}
> +#endif
>  
>  int xc_domain_max_vcpus(xc_interface *xch, uint32_t domid, unsigned int max)
>  {

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[George Dunlap]

On Tue, Jan 28, 2014 at 10:47 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
>> ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.
>>
>> The correct solution here would be to check for ENOSYS in libxl, unfortunately
>> xc_domain_set_pod_target suffers from the same broken error reporting as the
>> rest of libxc and throws away the errno.
>>
>> So for now conditionally define xc_domain_set_pod_target to return success
>> (which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
>> errno==-1 and returns -1, which matches the broken error reporting of the
>> existing function. It appears to have no in tree callers in any case.
>>
>> The conditional should be removed once libxc has been fixed.
>>
>> This makes ballooning (xl mem-set) work for ARM domains.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Cc: george.dunlap@citrix.com
>> ---
>> I'd be generally wary of modifying the error handling in a piecemeal way, but
>> certainly doing so for 4.4 now would be inapropriate.
>>
>> IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
>> frame, at which point this conditional stuff could be dropped.
>>
>> In terms of the 4.4 release, obviously ballooning would be very nice to have
>> for ARM guests, on the other hand I'm aware that while the patch is fairly
>> small/contained and safe it is also pretty skanky and likely wouldn't be
>> accepted outside of the rc period.
>
> George -- what do you think of this?

So is this actually called in the arm domain build code at the moment?

 -George

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[Ian Campbell]

On Tue, 2014-01-28 at 14:28 +0000, George Dunlap wrote:
> On Tue, Jan 28, 2014 at 10:47 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
> >> ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.
> >>
> >> The correct solution here would be to check for ENOSYS in libxl, unfortunately
> >> xc_domain_set_pod_target suffers from the same broken error reporting as the
> >> rest of libxc and throws away the errno.
> >>
> >> So for now conditionally define xc_domain_set_pod_target to return success
> >> (which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
> >> errno==-1 and returns -1, which matches the broken error reporting of the
> >> existing function. It appears to have no in tree callers in any case.
> >>
> >> The conditional should be removed once libxc has been fixed.
> >>
> >> This makes ballooning (xl mem-set) work for ARM domains.
> >>
> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >> Cc: george.dunlap@citrix.com
> >> ---
> >> I'd be generally wary of modifying the error handling in a piecemeal way, but
> >> certainly doing so for 4.4 now would be inapropriate.
> >>
> >> IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
> >> frame, at which point this conditional stuff could be dropped.
> >>
> >> In terms of the 4.4 release, obviously ballooning would be very nice to have
> >> for ARM guests, on the other hand I'm aware that while the patch is fairly
> >> small/contained and safe it is also pretty skanky and likely wouldn't be
> >> accepted outside of the rc period.
> >
> > George -- what do you think of this?
> 
> So is this actually called in the arm domain build code at the moment?

It is common code in libxl which calls into it. I originally had the
ifdef there instead.

(I've just noticed that I forgot to update $subject when I moved the
#ifdef from libxl to libxc)

Ian.

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[George Dunlap]

On 01/28/2014 02:31 PM, Ian Campbell wrote:
> On Tue, 2014-01-28 at 14:28 +0000, George Dunlap wrote:
>> On Tue, Jan 28, 2014 at 10:47 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
>>>> ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.
>>>>
>>>> The correct solution here would be to check for ENOSYS in libxl, unfortunately
>>>> xc_domain_set_pod_target suffers from the same broken error reporting as the
>>>> rest of libxc and throws away the errno.
>>>>
>>>> So for now conditionally define xc_domain_set_pod_target to return success
>>>> (which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
>>>> errno==-1 and returns -1, which matches the broken error reporting of the
>>>> existing function. It appears to have no in tree callers in any case.
>>>>
>>>> The conditional should be removed once libxc has been fixed.
>>>>
>>>> This makes ballooning (xl mem-set) work for ARM domains.
>>>>
>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>> Cc: george.dunlap@citrix.com
>>>> ---
>>>> I'd be generally wary of modifying the error handling in a piecemeal way, but
>>>> certainly doing so for 4.4 now would be inapropriate.
>>>>
>>>> IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
>>>> frame, at which point this conditional stuff could be dropped.
>>>>
>>>> In terms of the 4.4 release, obviously ballooning would be very nice to have
>>>> for ARM guests, on the other hand I'm aware that while the patch is fairly
>>>> small/contained and safe it is also pretty skanky and likely wouldn't be
>>>> accepted outside of the rc period.
>>>
>>> George -- what do you think of this?
>>
>> So is this actually called in the arm domain build code at the moment?
>
> It is common code in libxl which calls into it. I originally had the
> ifdef there instead.
>
> (I've just noticed that I forgot to update $subject when I moved the
> #ifdef from libxl to libxc)

Oh, right -- yes, you normally need to call set_pod_target() every time 
you update the balloon target, just in case PoD mode was activated on 
boot; if it wasn't (or if all the PoD entries have gone away) this will 
be a noop.

The only conceptual issue with putting it here is that 
xc_domain_set_pod_target() is also called during domain creation to fill 
the PoD "cache" with the domain's memory, from which to populate the p2m 
on-demand.  So if "someone" were to try to add PoD to the ARM guest 
creation, and forgot about this hack, they might spend a bit of time 
figuring out why the initial call to fill the PoD cache was succeeding 
but the guest was crashing with "PoD empty cache" anyway.

(xc_domain_set_target behaves differently if there are no entries in the 
p2m than if there are: if the p2m is empty, it will respond to this by 
filling the cache; if it's non-empty, it will ignore changes if there 
are no outstanding p2m entries.  That made sense at the time, but now it 
looks like a bit of an interface trap for the unwary...)

Was there a reason to put this in libxc rather than libxc?  We don't 
expect anyone to call libxc, so having it libxc isn't a big deal, but 
conceptually it would probably be safer in libxl.

  -George

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[George Dunlap]

On Tue, Jan 28, 2014 at 2:48 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Was there a reason to put this in libxc rather than libxc?  We don't expect
> anyone to call libxc, so having it libxc isn't a big deal, but conceptually
> it would probably be safer in libxl.

Er, change the 2nd "libxc" to "libxl" in this sentence, and it will
probably make a bit more sense...

 -George

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[Ian Campbell]

On Tue, 2014-01-28 at 14:48 +0000, George Dunlap wrote:
> On 01/28/2014 02:31 PM, Ian Campbell wrote:
> > On Tue, 2014-01-28 at 14:28 +0000, George Dunlap wrote:
> >> On Tue, Jan 28, 2014 at 10:47 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >>> On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
> >>>> ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.
> >>>>
> >>>> The correct solution here would be to check for ENOSYS in libxl, unfortunately
> >>>> xc_domain_set_pod_target suffers from the same broken error reporting as the
> >>>> rest of libxc and throws away the errno.
> >>>>
> >>>> So for now conditionally define xc_domain_set_pod_target to return success
> >>>> (which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
> >>>> errno==-1 and returns -1, which matches the broken error reporting of the
> >>>> existing function. It appears to have no in tree callers in any case.
> >>>>
> >>>> The conditional should be removed once libxc has been fixed.
> >>>>
> >>>> This makes ballooning (xl mem-set) work for ARM domains.
> >>>>
> >>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>>> Cc: george.dunlap@citrix.com
> >>>> ---
> >>>> I'd be generally wary of modifying the error handling in a piecemeal way, but
> >>>> certainly doing so for 4.4 now would be inapropriate.
> >>>>
> >>>> IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
> >>>> frame, at which point this conditional stuff could be dropped.
> >>>>
> >>>> In terms of the 4.4 release, obviously ballooning would be very nice to have
> >>>> for ARM guests, on the other hand I'm aware that while the patch is fairly
> >>>> small/contained and safe it is also pretty skanky and likely wouldn't be
> >>>> accepted outside of the rc period.
> >>>
> >>> George -- what do you think of this?
> >>
> >> So is this actually called in the arm domain build code at the moment?
> >
> > It is common code in libxl which calls into it. I originally had the
> > ifdef there instead.
> >
> > (I've just noticed that I forgot to update $subject when I moved the
> > #ifdef from libxl to libxc)
> 
> Oh, right -- yes, you normally need to call set_pod_target() every time 
> you update the balloon target, just in case PoD mode was activated on 
> boot; if it wasn't (or if all the PoD entries have gone away) this will 
> be a noop.
> 
> The only conceptual issue with putting it here is that 
> xc_domain_set_pod_target() is also called during domain creation to fill 
> the PoD "cache" with the domain's memory, from which to populate the p2m 
> on-demand.  So if "someone" were to try to add PoD to the ARM guest 
> creation, and forgot about this hack, they might spend a bit of time 
> figuring out why the initial call to fill the PoD cache was succeeding 
> but the guest was crashing with "PoD empty cache" anyway.

My hope is that this will get cleaned up in the 4.5 timeframe, as part
of the overdue cleanup of libxc error handling. After that then this
will properly report ENOSYS so that libxl can just DTRT. My hope is that
this will happen before anyone gets to implementing PoD on ARM.

Anyway, if this is the biggest stumbling block someone has while adding
PoD to ARM then they will have done pretty well...

> (xc_domain_set_target behaves differently if there are no entries in the 
> p2m than if there are: if the p2m is empty, it will respond to this by 
> filling the cache; if it's non-empty, it will ignore changes if there 
> are no outstanding p2m entries.  That made sense at the time, but now it 
> looks like a bit of an interface trap for the unwary...)
> 
> Was there a reason to put this in libxc rather than libxc?  We don't 
> expect anyone to call libxc, so having it libxc isn't a big deal, but 
> conceptually it would probably be safer in libxl.

I just thought the hack was more contained in libxc is all. Also having
it in libxc means that when the error handling cleanup I mentioned
occurs this will cleaned up at the same time, whereas if it was an ifdef
in libxl it might get missed. Not a big deal until someone implements
PoD on ARM though...

Ian.

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[George Dunlap]

On 01/28/2014 03:03 PM, Ian Campbell wrote:
> On Tue, 2014-01-28 at 14:48 +0000, George Dunlap wrote:
>> On 01/28/2014 02:31 PM, Ian Campbell wrote:
>>> On Tue, 2014-01-28 at 14:28 +0000, George Dunlap wrote:
>>>> On Tue, Jan 28, 2014 at 10:47 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>>> On Thu, 2014-01-16 at 15:27 +0000, Ian Campbell wrote:
>>>>>> ARM does not implemented PoD and so returns ENOSYS from XENMEM_set_pod_target.
>>>>>>
>>>>>> The correct solution here would be to check for ENOSYS in libxl, unfortunately
>>>>>> xc_domain_set_pod_target suffers from the same broken error reporting as the
>>>>>> rest of libxc and throws away the errno.
>>>>>>
>>>>>> So for now conditionally define xc_domain_set_pod_target to return success
>>>>>> (which is what PoD does if nothing needs doing). xc_domain_get_pod_target sets
>>>>>> errno==-1 and returns -1, which matches the broken error reporting of the
>>>>>> existing function. It appears to have no in tree callers in any case.
>>>>>>
>>>>>> The conditional should be removed once libxc has been fixed.
>>>>>>
>>>>>> This makes ballooning (xl mem-set) work for ARM domains.
>>>>>>
>>>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>>>> Cc: george.dunlap@citrix.com
>>>>>> ---
>>>>>> I'd be generally wary of modifying the error handling in a piecemeal way, but
>>>>>> certainly doing so for 4.4 now would be inapropriate.
>>>>>>
>>>>>> IIRC Ian J was planning a thorough sweep of the libxc error paths in 4.5 time
>>>>>> frame, at which point this conditional stuff could be dropped.
>>>>>>
>>>>>> In terms of the 4.4 release, obviously ballooning would be very nice to have
>>>>>> for ARM guests, on the other hand I'm aware that while the patch is fairly
>>>>>> small/contained and safe it is also pretty skanky and likely wouldn't be
>>>>>> accepted outside of the rc period.
>>>>>
>>>>> George -- what do you think of this?
>>>>
>>>> So is this actually called in the arm domain build code at the moment?
>>>
>>> It is common code in libxl which calls into it. I originally had the
>>> ifdef there instead.
>>>
>>> (I've just noticed that I forgot to update $subject when I moved the
>>> #ifdef from libxl to libxc)
>>
>> Oh, right -- yes, you normally need to call set_pod_target() every time
>> you update the balloon target, just in case PoD mode was activated on
>> boot; if it wasn't (or if all the PoD entries have gone away) this will
>> be a noop.
>>
>> The only conceptual issue with putting it here is that
>> xc_domain_set_pod_target() is also called during domain creation to fill
>> the PoD "cache" with the domain's memory, from which to populate the p2m
>> on-demand.  So if "someone" were to try to add PoD to the ARM guest
>> creation, and forgot about this hack, they might spend a bit of time
>> figuring out why the initial call to fill the PoD cache was succeeding
>> but the guest was crashing with "PoD empty cache" anyway.
>
> My hope is that this will get cleaned up in the 4.5 timeframe, as part
> of the overdue cleanup of libxc error handling. After that then this
> will properly report ENOSYS so that libxl can just DTRT. My hope is that
> this will happen before anyone gets to implementing PoD on ARM.
>
> Anyway, if this is the biggest stumbling block someone has while adding
> PoD to ARM then they will have done pretty well...
>
>> (xc_domain_set_target behaves differently if there are no entries in the
>> p2m than if there are: if the p2m is empty, it will respond to this by
>> filling the cache; if it's non-empty, it will ignore changes if there
>> are no outstanding p2m entries.  That made sense at the time, but now it
>> looks like a bit of an interface trap for the unwary...)
>>
>> Was there a reason to put this in libxc rather than libxc?  We don't
>> expect anyone to call libxc, so having it libxc isn't a big deal, but
>> conceptually it would probably be safer in libxl.
>
> I just thought the hack was more contained in libxc is all. Also having
> it in libxc means that when the error handling cleanup I mentioned
> occurs this will cleaned up at the same time, whereas if it was an ifdef
> in libxl it might get missed. Not a big deal until someone implements
> PoD on ARM though...

OK -- well I guess whatever you want to do then.  It's a bit of a 
bikeshed issue -- I've expressed my preference, go ahead and paint it 
whatever color you think best. :-)

If you want to check in this one:

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH] tools: libxl: do not set the PoD target on ARM"):
> If you want to check in this one:
> 
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(from a tools PoV)

Ian.

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[Julien Grall]

On 02/04/2014 03:04 PM, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH] tools: libxl: do not set the PoD target on ARM"):
>> If you want to check in this one:
>>
>> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Julien Grall <julien.grall@linaro.org>


-- 
Julien Grall

Re: [PATCH] tools: libxl: do not set the PoD target on ARM

[Ian Campbell]

On Tue, 2014-02-04 at 15:04 +0000, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH] tools: libxl: do not set the PoD target on ARM"):
> > If you want to check in this one:
> > 
> > Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Between that, Julien's ack  and George's Release-ack: Applied.

Thanks.