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

[George Dunlap <george.dunlap@eu.citrix.com>]

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