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

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

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>