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 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@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.
> 
> 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?

Yes, it looks like they might be useful.


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

Actually I think that xen_be_unbind_evtchn might be useful too, I think
that is the primary console evtchn. I wonder what specific bug I was
trying to fix when I introduced that if (!xendev->dev) check.


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

I don't know, but you have a valid point valgrid.


> commit 8d706e73b799efe0dd8268e085e572f31a6245d3
> Author: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 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@xxxxxxxxxx>
>     ---
>     v4: New patch based on feedback to "xen: Switch uses of
>     xc_map_foreign_bulk to use libxenforeignmemory API."

Makes sense


> 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;
>      }
>  }
> 

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