From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH QEMU-XEN v8 5/8] xen: Switch uses of xc_map_foreign_{pages, bulk} to use libxenforeignmemory API. Date: Fri, 15 Jan 2016 14:43:17 +0000 Message-ID: References: <1452864168.32341.97.camel@citrix.com> <1452864224-2554-1-git-send-email-ian.campbell@citrix.com> <1452864224-2554-6-git-send-email-ian.campbell@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1452864224-2554-6-git-send-email-ian.campbell@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, 15 Jan 2016, 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_{pages,bulk}. > > The new xenforeignmemory_map() function behaves like > xc_map_foreign_pages() when the err argument is NULL and like > xc_map_foreign_bulk() when err is non-NULL, which maps into the shim > here onto checking err == NULL and calling the appropriate old > function. > > Note that xenforeignmemory_map() takes the number of pages before the > arrays themselves, in order to support potentially future use of > variable-length-arrays in the prototype (in the future, when Xen's > baseline toolchain requirements are new enough to ensure VLAs are > supported). > > 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 implicit 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. > > Signed-off-by: Ian Campbell Reviewed-by: Stefano Stabellini > --- > v8: Move two copies of compat xenforeignmemory_{open,unmap} to the > common location. Guard with version 471 (which will be added in > the next patch) > v7: Move two copies of compat xenforeignmemory_map to a common location. > Note that the existing xen_domain_create #ifdef will be covered by > a CONFIG_XEN_PV_DOMAIN_BUILD in a later patch, and so is not a > suitable place to put this function. > v6: Handle _pages as well as _bulk, due to changes in the previous > patches, including the dropping of "xen: Switch uses of > xc_map_foreign_pages into xc_map_foreign_bulk" which previous preceded > this patch and the change of "xen: Switch uses of > xc_map_foreign_range into xc_map_foreign_bulk" into "xen: Switch > uses of xc_map_foreign_range into xc_map_foreign_pages". > > Handle reordering of arguments to xenforeignmemory_map(). > > Dropped Stefano's Reviewed-by due to these changes. > > v4: Rebase onto "xen_console: correctly cleanup primary console on > teardown." > > xenforeignmemory_unmap takes pages not bytes > > Compat wrapper for xenforeignmemory_open instead of ifdef in code. > > Run check patch and fix most issues. I did not fix: > > ERROR: do not initialise globals to 0 or NULL > +xenforeignmemory_handle *xen_fmem = NULL; > > => This is consistent with all of the existing declarations. > > ERROR: need consistent spacing around '*' (ctx:WxV) > +typedef xc_interface *xenforeignmemory_handle; > > => I think this is a false +ve since this is a pointer "*" not a multiple "*". > > reorder args to xenforeignmemory_map > > --- > > hw/char/xen_console.c | 8 ++++---- > hw/display/xenfb.c | 17 +++++++++-------- > hw/xen/xen_backend.c | 3 ++- > include/hw/xen/xen_backend.h | 1 + > include/hw/xen/xen_common.h | 25 +++++++++++++++++++++++++ > xen-common.c | 6 ++++++ > xen-hvm.c | 12 ++++++------ > xen-mapcache.c | 6 +++--- > 8 files changed, 56 insertions(+), 22 deletions(-) > > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c > index 3e8a57b..b92d0c6 100644 > --- a/hw/char/xen_console.c > +++ b/hw/char/xen_console.c > @@ -229,9 +229,9 @@ static int con_initialise(struct XenDevice *xendev) > > if (!xendev->dev) { > xen_pfn_t mfn = con->ring_ref; > - con->sring = xc_map_foreign_pages(xen_xc, con->xendev.dom, > - PROT_READ|PROT_WRITE, > - &mfn, 1); > + con->sring = xenforeignmemory_map(xen_fmem, con->xendev.dom, > + PROT_READ|PROT_WRITE, > + 1, &mfn, NULL); > } else { > con->sring = xengnttab_map_grant_ref(xendev->gnttabdev, con->xendev.dom, > con->ring_ref, > @@ -273,7 +273,7 @@ static void con_disconnect(struct XenDevice *xendev) > > if (con->sring) { > if (!xendev->dev) { > - munmap(con->sring, XC_PAGE_SIZE); > + xenforeignmemory_unmap(xen_fmem, con->sring, 1); > } else { > xengnttab_unmap(xendev->gnttabdev, con->sring, 1); > } > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > index bb8f6b3..a42971c 100644 > --- a/hw/display/xenfb.c > +++ b/hw/display/xenfb.c > @@ -106,8 +106,8 @@ static int common_bind(struct common *c) > if (xenstore_read_fe_int(&c->xendev, "event-channel", &c->xendev.remote_port) == -1) > return -1; > > - c->page = xc_map_foreign_pages(xen_xc, c->xendev.dom, > - PROT_READ | PROT_WRITE, &mfn, 1); > + c->page = xenforeignmemory_map(xen_fmem, c->xendev.dom, > + PROT_READ | PROT_WRITE, 1, &mfn, NULL); > if (c->page == NULL) > return -1; > > @@ -122,7 +122,7 @@ static void common_unbind(struct common *c) > { > xen_be_unbind_evtchn(&c->xendev); > if (c->page) { > - munmap(c->page, XC_PAGE_SIZE); > + xenforeignmemory_unmap(xen_fmem, c->page, 1); > c->page = NULL; > } > } > @@ -495,15 +495,15 @@ static int xenfb_map_fb(struct XenFB *xenfb) > fbmfns = g_malloc0(sizeof(xen_pfn_t) * xenfb->fbpages); > > xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd); > - map = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom, > - PROT_READ, pgmfns, n_fbdirs); > + map = xenforeignmemory_map(xen_fmem, xenfb->c.xendev.dom, > + PROT_READ, n_fbdirs, pgmfns, NULL); > if (map == NULL) > goto out; > xenfb_copy_mfns(mode, xenfb->fbpages, fbmfns, map); > - munmap(map, n_fbdirs * XC_PAGE_SIZE); > + xenforeignmemory_unmap(xen_fmem, map, n_fbdirs); > > - xenfb->pixels = xc_map_foreign_pages(xen_xc, xenfb->c.xendev.dom, > - PROT_READ, fbmfns, xenfb->fbpages); > + xenfb->pixels = xenforeignmemory_map(xen_fmem, xenfb->c.xendev.dom, > + PROT_READ, xenfb->fbpages, fbmfns, NULL); > if (xenfb->pixels == NULL) > goto out; > > @@ -912,6 +912,7 @@ static void fb_disconnect(struct XenDevice *xendev) > * Replacing the framebuffer with anonymous shared memory > * instead. This releases the guest pages and keeps qemu happy. > */ > + xenforeignmemory_unmap(xen_fmem, fb->pixels, fb->fbpages); > fb->pixels = mmap(fb->pixels, fb->fbpages * XC_PAGE_SIZE, > PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANON, > -1, 0); > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c > index 966e34f..caa459c 100644 > --- a/hw/xen/xen_backend.c > +++ b/hw/xen/xen_backend.c > @@ -45,6 +45,7 @@ > > /* public */ > XenXC xen_xc = XC_HANDLER_INITIAL_VALUE; > +xenforeignmemory_handle *xen_fmem = NULL; > struct xs_handle *xenstore = NULL; > const char *xen_protocol; > > @@ -717,7 +718,7 @@ int xen_be_init(void) > > qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, NULL); > > - if (xen_xc == XC_HANDLER_INITIAL_VALUE) { > + if (xen_xc == XC_HANDLER_INITIAL_VALUE || xen_fmem == NULL) { > /* Check if xen_init() have been called */ > goto err; > } > diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h > index 8e8857b..e0d52ee 100644 > --- a/include/hw/xen/xen_backend.h > +++ b/include/hw/xen/xen_backend.h > @@ -57,6 +57,7 @@ struct XenDevice { > > /* variables */ > extern XenXC xen_xc; > +extern xenforeignmemory_handle *xen_fmem; > extern struct xs_handle *xenstore; > extern const char *xen_protocol; > > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h > index 8f38310..95275b3 100644 > --- a/include/hw/xen/xen_common.h > +++ b/include/hw/xen/xen_common.h > @@ -41,6 +41,7 @@ static inline void *xc_map_foreign_bulk(int xc_handle, uint32_t dom, int prot, > typedef int XenXC; > typedef int xenevtchn_handle; > typedef int xengnttab_handle; > +typedef int xenforeignmemory_handle; > > # define XC_INTERFACE_FMT "%i" > # define XC_HANDLER_INITIAL_VALUE -1 > @@ -104,6 +105,8 @@ static inline XenXC xen_xc_interface_open(void *logger, void *dombuild_logger, > return xc_interface_open(); > } > > +/* See below for xenforeignmemory_* APIs */ > + > static inline int xc_fd(int xen_xc) > { > return xen_xc; > @@ -149,6 +152,7 @@ static inline void xs_close(struct xs_handle *xsh) > #else > > typedef xc_interface *XenXC; > +typedef xc_interface *xenforeignmemory_handle; > typedef xc_evtchn xenevtchn_handle; > typedef xc_gnttab xengnttab_handle; > > @@ -178,6 +182,8 @@ static inline XenXC xen_xc_interface_open(void *logger, void *dombuild_logger, > return xc_interface_open(logger, dombuild_logger, open_flags); > } > > +/* See below for xenforeignmemory_* APIs */ > + > /* FIXME There is now way to have the xen fd */ > static inline int xc_fd(xc_interface *xen_xc) > { > @@ -501,4 +507,23 @@ static inline int xen_domain_create(XenXC xc, uint32_t ssidref, > } > #endif > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 471 > + > +#define xenforeignmemory_open(l, f) &xen_xc > + > +static inline void *xenforeignmemory_map(XenXC *h, uint32_t dom, > + int prot, size_t pages, > + const xen_pfn_t arr[/*pages*/], > + int err[/*pages*/]) > +{ > + if (err) > + return xc_map_foreign_bulk(*h, dom, prot, arr, err, pages); > + else > + return xc_map_foreign_pages(*h, dom, prot, arr, pages); > +} > + > +#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE) > + > +#endif > + > #endif /* QEMU_HW_XEN_COMMON_H */ > diff --git a/xen-common.c b/xen-common.c > index 0dcdbc3..6cd2959 100644 > --- a/xen-common.c > +++ b/xen-common.c > @@ -118,6 +118,12 @@ static int xen_init(MachineState *ms) > xen_be_printf(NULL, 0, "can't open xen interface\n"); > return -1; > } > + xen_fmem = xenforeignmemory_open(0, 0); > + if (xen_fmem == NULL) { > + xen_be_printf(NULL, 0, "can't open xen fmem interface\n"); > + xc_interface_close(xen_xc); > + return -1; > + } > qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); > > global_state_set_optional(); > diff --git a/xen-hvm.c b/xen-hvm.c > index 1f729f6..66ee835 100644 > --- a/xen-hvm.c > +++ b/xen-hvm.c > @@ -1247,9 +1247,9 @@ int xen_hvm_init(PCMachineState *pcms, > DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn); > DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn); > > - state->shared_page = xc_map_foreign_pages(xen_xc, xen_domid, > + state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid, > PROT_READ|PROT_WRITE, > - &ioreq_pfn, 1); > + 1, &ioreq_pfn, NULL); > if (state->shared_page == NULL) { > hw_error("map shared IO page returned error %d handle=" XC_INTERFACE_FMT, > errno, xen_xc); > @@ -1259,8 +1259,8 @@ int xen_hvm_init(PCMachineState *pcms, > if (!rc) { > DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn); > state->shared_vmport_page = > - xc_map_foreign_pages(xen_xc, xen_domid, PROT_READ|PROT_WRITE, > - &ioreq_pfn, 1); > + xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE, > + 1, &ioreq_pfn, NULL); > if (state->shared_vmport_page == NULL) { > hw_error("map shared vmport IO page returned error %d handle=" > XC_INTERFACE_FMT, errno, xen_xc); > @@ -1269,9 +1269,9 @@ int xen_hvm_init(PCMachineState *pcms, > hw_error("get vmport regs pfn returned error %d, rc=%d", errno, rc); > } > > - state->buffered_io_page = xc_map_foreign_pages(xen_xc, xen_domid, > + state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid, > PROT_READ|PROT_WRITE, > - &bufioreq_pfn, 1); > + 1, &bufioreq_pfn, NULL); > if (state->buffered_io_page == NULL) { > hw_error("map buffered IO page returned error %d", errno); > } > diff --git a/xen-mapcache.c b/xen-mapcache.c > index 97fece2..4a04378 100644 > --- a/xen-mapcache.c > +++ b/xen-mapcache.c > @@ -176,10 +176,10 @@ static void xen_remap_bucket(MapCacheEntry *entry, > pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i; > } > > - vaddr_base = xc_map_foreign_bulk(xen_xc, xen_domid, PROT_READ|PROT_WRITE, > - pfns, err, nb_pfn); > + vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE, > + nb_pfn, pfns, err); > if (vaddr_base == NULL) { > - perror("xc_map_foreign_bulk"); > + perror("xenforeignmemory_map"); > exit(-1); > } > > -- > 2.1.4 >