Re: [Xen-devel] [PATCH QEMU-XEN v3 5/8] xen: Switch uses of xc_map_foreign_bulk to use libxenforeignmemory API.

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 14 Oct 2015, Ian Campbell wrote:
> On Wed, 2015-10-14 at 15:10 +0100, Stefano Stabellini wrote:
> > On Wed, 7 Oct 2015, Ian Campbell wrote:
> > > In Xen 4.7 we are refactoring parts libxenctrl into a number of
> > > separate libraries which will provide backward and forward API and ABI
> > > compatiblity.
> > > 
> > > One such library will be libxenforeignmemory which provides access to
> > > privileged foreign mappings and which will provide an interface
> > > equivalent to xc_map_foreign_bulk.
> > > 
> > > In preparation for adding support for libxenforeignmemory add support
> > > to the <=4.0 and <=4.6 compat code in xen_common.h to allow us to
> > > switch to using the new API. These shims will disappear for versions
> > > of Xen which include libxenforeignmemory.
> > > 
> > > Since libxenforeignmemory will have its own handle type but for <= 4.6
> > > the functionality is provided by using a libxenctrl handle we
> > > introduce a new global xen_fmem alongside the existing xen_xc. In fact
> > > we make xen_fmem a pointer to the existing xen_xc, which then works
> > > correctly with both <=4.0 (xc handle is an int) and <=4.6 (xc handle
> > > is a pointer). In the latter case xen_fmem is actually a double
> > > indirect pointer, but it all falls out in the wash.
> > > 
> > > Unlike libxenctrl libxenforeignmemory has an explicit unmap function,
> > > rather than just specifying that munmap should be used, so the unmap
> > > paths are updated to use xenforeignmemory_unmap, which is a shim for
> > > munmap on these versions of xen. The mappings in xen-hvm.c do not
> > > appear to be unmapped (which makes sense for a qemu-dm process)
> > > 
> > > In fb_disconnect this results in a change from simply mmap over the
> > > existing mapping (with an implciit munmap) to expliclty unmapping with
> > > xenforeignmemory_unmap and then mapping the required anonymous memory
> > > in the same hole. I don't think this is a problem since any other
> > > thread which was racily touching this region would already be running
> > > the risk of hitting the mapping halfway through the call. If this is
> > > thought to be a problem then we could consider adding an extra API to
> > > the libxenforeignmemory interface to replace a foreign mapping with
> > > anonymous shared memory, but I'd prefer not to.
> > > 
> > > Build tested with 4.0 and 4.5.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > ---
> > > I noticed in xen_console.c that the decision to use a foreign
> > > privileged memory mapping vs a grant dev is made using different
> > > variables in con_initialise vs con_disconnect. The former uses
> > > xendev->dev while the latter uses xendev->gnttabdev. Is this a latent
> > > bug?
> > 
> > The code in con_disconnect is superfluous: the initial check
> > 
> >     if (!xendev->dev) {
> >         return;
> >     }
> > 
> > makes sure the rest of the function only deals with dev != 0.
> > I guess it should be
> > 
> >     if (!xendev->dev) {
> 
> Did you mean xendev->gnttabdev here? Since this is what the code is
> touching.
>
> Should ->dev and ->gnttabdev be either both set or neither then?

That's right


> Since con_connect uses the former to decide it would seem logical for
> teardown to use the same condition, but I don't know if I'm missing
> something. In particular given the initial check you point to how is the
> foreign mapping not already leaked today if the lifecycle of ->dev and 
> ->gnttabdev are strictly aligned?
> 
> >         munmap(con->sring, XC_PAGE_SIZE);
> 
> And this is how the code should be _now_, i.e. with this patch it should
> become xenforeignmemory_unmap?

Yes, I think that today it is leaked, but in practice there is one QEMU
process per VM and the VM never disconnect/reconnect the primary console
(I don't think there is a way to do that). In other words, after
con_disconnect, QEMU is killed.

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