From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] tools: libxl: do not set the PoD target on ARM Date: Tue, 28 Jan 2014 15:24:40 +0000 Message-ID: <52E7CBB8.7040904@eu.citrix.com> References: <1389886079-6855-1-git-send-email-ian.campbell@citrix.com> <1390906058.7753.37.camel@kazak.uk.xensource.com> <1390919466.7753.97.camel@kazak.uk.xensource.com> <52E7C320.7080402@eu.citrix.com> <1390921417.7753.106.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1390921417.7753.106.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Stefano Stabellini , Ian Jackson , Julien Grall , Tim Deegan , George Dunlap , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 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 >>>>>> 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