[Qemu-devel] [PATCH QEMU-XEN v4 6/9] xen: Switch uses of xc_map_foreign_bulk to use libxenforeignmemory API.

[Ian Campbell]

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 <address@hidden>
---
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