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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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?

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?

Ian.

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