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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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.
(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



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