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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx> 
> >> 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@xxxxxxxxxx>
> >>>> Cc: george.dunlap@xxxxxxxxxx
> >>>> ---
> >>>> 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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel