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 14:48:00 +0000 Message-ID: <52E7C320.7080402@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1390919466.7753.97.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 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. (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