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

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-10-14 at 16:41 +0100, Stefano Stabellini wrote:
> 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@citrix.com>
> > > > ---
> > > > 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.

Part of the reason for xenforeignmemory_unmap is so that we can make things
like valgrind more aware of what is going on, so having this happen even if
things are about to exit is still useful.

I came up with the following patch which I will insert at the front of v4,
but I have one doubt, namely are the calls to qemu_chr_add_handler and
qemu_chr_fe_release not useful here too?

Obviously the call to xen_be_unbind_evtchn is not useful as is, but I do
wonder where the evtchn which the primary console must have somewhere
actually is then...

Maybe the reasoning here is the same "we are going to exit anyway", and
maybe this cleanup isn't the sort of thing valgrind will complain about
(i.e. neither of them free any memory), in which case they may as well be
left as is.

Ian.

commit 8d706e73b799efe0dd8268e085e572f31a6245d3
Author: Ian Campbell <ian.campbell@citrix.com>
Date:   Wed Oct 14 16:49:25 2015 +0100

    xen_console: correctly unmap primary console on teardown.
    
    The ->dev and ->gnttabdev handles are either both set or neither. This
    means that we never reach the existing unmap.
    
    Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
    ---
    v4: New patch based on feedback to "xen: Switch uses of
    xc_map_foreign_bulk to use libxenforeignmemory API."

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index eb7f450..b6f0029 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -266,6 +266,10 @@ static void con_disconnect(struct XenDevice *xendev)
     struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
 
     if (!xendev->dev) {
+        if (con->string) {
+            munmap(con->sring, XC_PAGE_SIZE);
+            con->sring = NULL;
+        }
         return;
     }
     if (con->chr) {
@@ -275,11 +279,7 @@ static void con_disconnect(struct XenDevice *xendev)
     xen_be_unbind_evtchn(&con->xendev);
 
     if (con->sring) {
-        if (!xendev->gnttabdev) {
-            munmap(con->sring, XC_PAGE_SIZE);
-        } else {
-            xc_gnttab_munmap(xendev->gnttabdev, con->sring, 1);
-        }
+        xc_gnttab_munmap(xendev->gnttabdev, con->sring, 1);
 	con->sring = NULL;
     }
 }