|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH QEMU-XEN v4 6/9] xen: Switch uses of xc_map_foreign_bulk to use libxenforeignmemory API.
On Wed, 21 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
^ implicit
> 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>
Reviewed-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 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?
>
> 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 "*".
> ---
> hw/char/xen_console.c | 8 ++++----
> hw/display/xenfb.c | 15 ++++++++-------
> hw/xen/xen_backend.c | 3 ++-
> include/hw/xen/xen_backend.h | 1 +
> include/hw/xen/xen_common.h | 12 ++++++++++++
> xen-common.c | 6 ++++++
> xen-hvm.c | 18 +++++++++---------
> xen-mapcache.c | 6 +++---
> 8 files changed, 45 insertions(+), 24 deletions(-)
>
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index 11c6472..24f3a40 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -230,9 +230,9 @@ static int con_initialise(struct XenDevice *xendev)
>
> if (!xendev->dev) {
> xen_pfn_t mfn = con->ring_ref;
> - con->sring = xc_map_foreign_bulk(xen_xc, con->xendev.dom,
> - PROT_READ|PROT_WRITE,
> - &mfn, &err, 1);
> + con->sring = xenforeignmemory_map(xen_fmem, con->xendev.dom,
> + PROT_READ|PROT_WRITE,
> + &mfn, &err, 1);
> } else {
> con->sring = xengnttab_map_grant_ref(xendev->gnttabdev,
> con->xendev.dom,
> con->ring_ref,
> @@ -274,7 +274,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_munmap(xendev->gnttabdev, con->sring, 1);
> }
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index b0ac1e6..a5ddb60 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -103,8 +103,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_bulk(xen_xc, c->xendev.dom,
> - PROT_READ | PROT_WRITE, &mfn, &err, 1);
> + c->page = xenforeignmemory_map(xen_fmem, c->xendev.dom,
> + PROT_READ | PROT_WRITE, &mfn, &err, 1);
> if (c->page == NULL)
> return -1;
>
> @@ -119,7 +119,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;
> }
> }
> @@ -491,14 +491,14 @@ static int xenfb_map_fb(struct XenFB *xenfb)
> errs = g_malloc0(sizeof(int) * n_fbdirs);
>
> xenfb_copy_mfns(mode, n_fbdirs, pgmfns, pd);
> - map = xc_map_foreign_bulk(xen_xc, xenfb->c.xendev.dom,
> - PROT_READ, pgmfns, errs, n_fbdirs);
> + map = xenforeignmemory_map(xen_fmem, xenfb->c.xendev.dom,
> + PROT_READ, pgmfns, errs, n_fbdirs);
> 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_bulk(xen_xc, xenfb->c.xendev.dom,
> + xenfb->pixels = xenforeignmemory_map(xen_fmem, xenfb->c.xendev.dom,
> PROT_READ, fbmfns, errs, xenfb->fbpages);
> if (xenfb->pixels == NULL)
> goto out;
> @@ -907,6 +907,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 f988b95..30ffcb4 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;
>
> @@ -719,7 +720,7 @@ int xen_be_init(void)
> goto err;
> }
>
> - 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 db62df4..2a5f27a 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,11 @@ static inline XenXC xen_xc_interface_open(void *logger,
> void *dombuild_logger,
> return xc_interface_open();
> }
>
> +#define xenforeignmemory_open(l, f) &xen_xc
> +#define xenforeignmemory_map(h, d, p, a, e, n) \
> + xc_map_foreign_bulk(*h, d, p, a, e, n)
> +#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
> +
> static inline int xc_fd(int xen_xc)
> {
> return xen_xc;
> @@ -149,6 +155,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 +185,11 @@ static inline XenXC xen_xc_interface_open(void *logger,
> void *dombuild_logger,
> return xc_interface_open(logger, dombuild_logger, open_flags);
> }
>
> +#define xenforeignmemory_open(l, f) &xen_xc
> +#define xenforeignmemory_map(h, d, p, a, e, n) \
> + xc_map_foreign_bulk(*h, d, p, a, e, n)
> +#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
> +
> /* FIXME There is now way to have the xen fd */
> static inline int xc_fd(xc_interface *xen_xc)
> {
> diff --git a/xen-common.c b/xen-common.c
> index 56359ca..dd8619b 100644
> --- a/xen-common.c
> +++ b/xen-common.c
> @@ -117,6 +117,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);
>
> return 0;
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 0c84e30..8fe7397 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -1214,9 +1214,9 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size,
> ram_addr_t *above_4g_mem_size,
> 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_bulk(xen_xc, xen_domid,
> - PROT_READ|PROT_WRITE,
> - &ioreq_pfn, &map_err, 1);
> + state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid,
> + PROT_READ|PROT_WRITE,
> + &ioreq_pfn, &map_err, 1);
> if (state->shared_page == NULL) {
> hw_error(
> "map shared IO page returned error %d(%d) handle="
> XC_INTERFACE_FMT,
> @@ -1227,8 +1227,8 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size,
> ram_addr_t *above_4g_mem_size,
> if (!rc) {
> DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn);
> state->shared_vmport_page =
> - xc_map_foreign_bulk(xen_xc, xen_domid, PROT_READ|PROT_WRITE,
> - &ioreq_pfn, &map_err, 1);
> + xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> + &ioreq_pfn, &map_err, 1);
> if (state->shared_vmport_page == NULL) {
> hw_error("map shared vmport IO page returned error %d (%d)
> handle="
> XC_INTERFACE_FMT, errno, map_err, xen_xc);
> @@ -1237,10 +1237,10 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size,
> ram_addr_t *above_4g_mem_size,
> hw_error("get vmport regs pfn returned error %d, rc=%d", errno, rc);
> }
>
> - state->buffered_io_page = xc_map_foreign_bulk(xen_xc, xen_domid,
> - PROT_READ|PROT_WRITE,
> - &bufioreq_pfn,
> - &map_err, 1);
> + state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid,
> + PROT_READ|PROT_WRITE,
> + &bufioreq_pfn,
> + &map_err, 1);
> if (state->buffered_io_page == NULL) {
> hw_error("map buffered IO page returned error %d (%d)", errno,
> map_err);
> }
> diff --git a/xen-mapcache.c b/xen-mapcache.c
> index 66da1a6..7d4953c 100644
> --- a/xen-mapcache.c
> +++ b/xen-mapcache.c
> @@ -169,10 +169,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,
> + pfns, err, nb_pfn);
> if (vaddr_base == NULL) {
> - perror("xc_map_foreign_bulk");
> + perror("xenforeignmemory_map");
> exit(-1);
> }
>
> --
> 2.1.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |