[PATCH v23 0/2] guest resource mapping (reprise)

[PATCH v23 0/2] guest resource mapping (reprise)

[Paul Durrant]

These patches are the patches from my original resource mapping series
that did not make it into 4.11.

Paul Durrant (2):
  common: add a new mappable resource type: XENMEM_resource_grant_table
  tools/libxenctrl: use new xenforeignmemory API to seed grant table

 tools/libxc/include/xc_dom.h        |  12 +--
 tools/libxc/xc_dom_boot.c           | 166 +++++++++++++++++++++++-------------
 tools/libxc/xc_sr_restore_x86_hvm.c |  10 +--
 tools/libxc/xc_sr_restore_x86_pv.c  |   2 +-
 tools/libxl/libxl_dom.c             |   1 -
 tools/python/xen/lowlevel/xc/xc.c   |   6 +-
 xen/common/grant_table.c            | 116 ++++++++++++++++++++++---
 xen/common/memory.c                 |  56 +++++++++++-
 xen/include/public/memory.h         |   6 ++
 xen/include/xen/grant_table.h       |   4 +
 10 files changed, 287 insertions(+), 92 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v23 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant]

This patch allows grant table frames to be mapped using the
XENMEM_acquire_resource memory op.

NOTE: This patch expands the on-stack mfn_list array in acquire_resource()
      but it is still small enough to remain on-stack.

NOTE: This patch also removes a bogus comment above the
      grant_to_status_frames() function.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

v23:
 - Fix overflow check.
 - Return EACCES rather than EOPNOTSUPP if foreign map is unsafe.

v22:
 - Remove bogus comment and update commit message accordingly.
 - Add ASSERTion that an invalid MFN is not passed back to the caller of
   XENMEM_acquire_resource.
 - Re-code the idx to nr calculation to try to make it more obvious and
   add explicit overflow checks.

v21:
 - Prevent PVH/HVM tools domains from mapping any non-caller-owned resource
   unless the tools domain is also the hardware domain.
 - Grow the grant table appropriately whether it is a shared frame or
   a status frame that is being mapped.
 - Fix comment style breakage in memory.h.
 - Move implicit version setting to gnttab_get_shared_frame().

v19:
 - Add test to prevent PVH/HVM tools domains mapping grant status frames
   this way as the mapping infrastructure in Xen does not yet implement the
   necessary reference counting to make this safe.
 - Make sure grant table version is set before any attempt to grow the table.

v18:
 - Non-trivial re-base of grant table code.
 - Dropped Jan's R-b because of the grant table changes.

v13:
 - Re-work the internals to avoid using the XENMAPIDX_grant_table_status
   hack.

v12:
 - Dropped limit checks as requested by Jan.

v10:
 - Addressed comments from Jan.

v8:
 - The functionality was originally incorporated into the earlier patch
   "x86/mm: add HYPERVISOR_memory_op to acquire guest resources".
---
 xen/common/grant_table.c      | 116 +++++++++++++++++++++++++++++++++++++-----
 xen/common/memory.c           |  56 +++++++++++++++++++-
 xen/include/public/memory.h   |   6 +++
 xen/include/xen/grant_table.h |   4 ++
 4 files changed, 167 insertions(+), 15 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d9ec711c73..bc54535982 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -352,12 +352,17 @@ static inline void active_entry_release(struct active_grant_entry *act)
 
 #define GRANT_STATUS_PER_PAGE (PAGE_SIZE / sizeof(grant_status_t))
 #define GRANT_PER_PAGE (PAGE_SIZE / sizeof(grant_entry_v2_t))
-/* Number of grant table status entries. Caller must hold d's gr. table lock.*/
+
 static inline unsigned int grant_to_status_frames(unsigned int grant_frames)
 {
     return DIV_ROUND_UP(grant_frames * GRANT_PER_PAGE, GRANT_STATUS_PER_PAGE);
 }
 
+static inline unsigned int status_to_grant_frames(unsigned int status_frames)
+{
+    return DIV_ROUND_UP(status_frames * GRANT_STATUS_PER_PAGE, GRANT_PER_PAGE);
+}
+
 /* Check if the page has been paged out, or needs unsharing.
    If rc == GNTST_okay, *page contains the page struct with a ref taken.
    Caller must do put_page(*page).
@@ -3860,6 +3865,67 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
 }
 #endif
 
+/* caller must hold write lock */
+static int gnttab_get_status_frame_mfn(struct domain *d,
+                                       unsigned long idx, mfn_t *mfn)
+{
+    const struct grant_table *gt = d->grant_table;
+
+    ASSERT(gt->gt_version == 2);
+
+    if ( idx >= nr_status_frames(gt) )
+    {
+        unsigned long nr_status;
+        unsigned long nr_grant;
+
+        nr_status = idx + 1; /* sufficient frames to make idx valid */
+
+        if ( nr_status <= nr_status_frames(gt) ) /* overflow check */
+            return -EINVAL;
+
+        nr_grant = status_to_grant_frames(nr_status);
+
+        if ( nr_grant <= gt->max_grant_frames )
+            gnttab_grow_table(d, nr_grant);
+
+        /* check whether gnttab_grow_table() succeeded */
+        if ( idx >= nr_status_frames(gt) )
+            return -EINVAL;
+    }
+
+    *mfn = _mfn(virt_to_mfn(gt->status[idx]));
+    return 0;
+}
+
+/* caller must hold write lock */
+static int gnttab_get_shared_frame_mfn(struct domain *d,
+                                       unsigned long idx, mfn_t *mfn)
+{
+    const struct grant_table *gt = d->grant_table;
+
+    ASSERT(gt->gt_version != 0);
+
+    if ( idx >= nr_grant_frames(gt) )
+    {
+        unsigned long nr_grant;
+
+        nr_grant = idx + 1; /* sufficient frames to make idx valid */
+
+        if ( nr_grant <= nr_grant_frames(gt) ) /* overflow check */
+            return -EINVAL;
+
+        if ( nr_grant <= gt->max_grant_frames )
+            gnttab_grow_table(d, nr_grant);
+
+        /* check whether gnttab_grow_table() succeeded */
+        if ( idx >= nr_grant_frames(gt) )
+            return -EINVAL;
+    }
+
+    *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
+    return 0;
+}
+
 int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
                      mfn_t *mfn)
 {
@@ -3877,21 +3943,11 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
     {
         idx &= ~XENMAPIDX_grant_table_status;
         status = true;
-        if ( idx < nr_status_frames(gt) )
-            *mfn = _mfn(virt_to_mfn(gt->status[idx]));
-        else
-            rc = -EINVAL;
-    }
-    else
-    {
-        if ( (idx >= nr_grant_frames(gt)) && (idx < gt->max_grant_frames) )
-            gnttab_grow_table(d, idx + 1);
 
-        if ( idx < nr_grant_frames(gt) )
-            *mfn = _mfn(virt_to_mfn(gt->shared_raw[idx]));
-        else
-            rc = -EINVAL;
+        rc = gnttab_get_status_frame_mfn(d, idx, mfn);
     }
+    else
+        rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
 
     if ( !rc && paging_mode_translate(d) &&
          !gfn_eq(gnttab_get_frame_gfn(gt, status, idx), INVALID_GFN) )
@@ -3906,6 +3962,38 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
     return rc;
 }
 
+int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
+                            mfn_t *mfn)
+{
+    struct grant_table *gt = d->grant_table;
+    int rc;
+
+    grant_write_lock(gt);
+
+    if ( gt->gt_version == 0 )
+        gt->gt_version = 1;
+
+    rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
+
+    grant_write_unlock(gt);
+
+    return rc;
+}
+
+int gnttab_get_status_frame(struct domain *d, unsigned long idx,
+                            mfn_t *mfn)
+{
+    struct grant_table *gt = d->grant_table;
+    int rc;
+
+    grant_write_lock(gt);
+    rc = (gt->gt_version == 2) ?
+        gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
+    grant_write_unlock(gt);
+
+    return rc;
+}
+
 static void gnttab_usage_print(struct domain *rd)
 {
     int first = 1;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index e29d596727..996f94b103 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -23,6 +23,7 @@
 #include <xen/numa.h>
 #include <xen/mem_access.h>
 #include <xen/trace.h>
+#include <xen/grant_table.h>
 #include <asm/current.h>
 #include <asm/hardirq.h>
 #include <asm/p2m.h>
@@ -982,6 +983,44 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
     return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
 }
 
+static int acquire_grant_table(struct domain *d, unsigned int id,
+                               unsigned long frame,
+                               unsigned int nr_frames,
+                               xen_pfn_t mfn_list[])
+{
+    unsigned int i = nr_frames;
+
+    /* Iterate backwards in case table needs to grow */
+    while ( i-- != 0 )
+    {
+        mfn_t mfn = INVALID_MFN;
+        int rc;
+
+        switch ( id )
+        {
+        case XENMEM_resource_grant_table_id_shared:
+            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
+            break;
+
+        case XENMEM_resource_grant_table_id_status:
+            rc = gnttab_get_status_frame(d, frame + i, &mfn);
+            break;
+
+        default:
+            rc = -EINVAL;
+            break;
+        }
+
+        if ( rc )
+            return rc;
+
+        ASSERT(!mfn_eq(mfn, INVALID_MFN));
+        mfn_list[i] = mfn_x(mfn);
+    }
+
+    return 0;
+}
+
 static int acquire_resource(
     XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
 {
@@ -992,7 +1031,7 @@ static int acquire_resource(
      * moment since they are small, but if they need to grow in future
      * use-cases then per-CPU arrays or heap allocations may be required.
      */
-    xen_pfn_t mfn_list[2];
+    xen_pfn_t mfn_list[32];
     int rc;
 
     if ( copy_from_guest(&xmar, arg, 1) )
@@ -1027,6 +1066,11 @@ static int acquire_resource(
 
     switch ( xmar.type )
     {
+    case XENMEM_resource_grant_table:
+        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
+                                 mfn_list);
+        break;
+
     default:
         rc = arch_acquire_resource(d, xmar.type, xmar.id, xmar.frame,
                                    xmar.nr_frames, mfn_list, &xmar.flags);
@@ -1046,6 +1090,16 @@ static int acquire_resource(
         xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
         unsigned int i;
 
+        /*
+         * FIXME: Until foreign pages inserted into the P2M are properly
+         *        reference counted, it is unsafe to allow mapping of
+         *        non-caller-owned resource pages unless the caller is
+         *        the hardware domain.
+         */
+        if ( !(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
+             !is_hardware_domain(currd) )
+            return -EACCES;
+
         if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
             rc = -EFAULT;
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index bf2f81faae..8fc27ceeab 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -611,14 +611,20 @@ struct xen_mem_acquire_resource {
     uint16_t type;
 
 #define XENMEM_resource_ioreq_server 0
+#define XENMEM_resource_grant_table 1
 
     /*
      * IN - a type-specific resource identifier, which must be zero
      *      unless stated otherwise.
      *
      * type == XENMEM_resource_ioreq_server -> id == ioreq server id
+     * type == XENMEM_resource_grant_table -> id defined below
      */
     uint32_t id;
+
+#define XENMEM_resource_grant_table_id_shared 0
+#define XENMEM_resource_grant_table_id_status 1
+
     /*
      * IN/OUT - As an IN parameter number of frames of the resource
      *          to be mapped. However, if the specified value is 0 and
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 0286ba33dd..c881414e5b 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -58,6 +58,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
 
 int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
                      mfn_t *mfn);
+int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
+                            mfn_t *mfn);
+int gnttab_get_status_frame(struct domain *d, unsigned long idx,
+                            mfn_t *mfn);
 
 unsigned int gnttab_dom0_frames(void);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v23 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table

[Paul Durrant]

A previous patch added support for priv-mapping guest resources directly
(rather than having to foreign-map, which requires P2M modification for
HVM guests).

This patch makes use of the new API to seed the guest grant table unless
the underlying infrastructure (i.e. privcmd) doesn't support it, in which
case the old scheme is used.

NOTE: The call to xc_dom_gnttab_hvm_seed() in hvm_build_set_params() was
      actually unnecessary, as the grant table has already been seeded
      by a prior call to xc_dom_gnttab_init() made by libxl__build_dom().

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>

v22:
 - Addressed comments from Andrew (cosmetic changes only).

v18:
 - Trivial re-base.

v13:
 - Re-base.

v10:
 - Use new id constant for grant table.

v4:
 - Minor cosmetic fix suggested by Roger.

v3:
 - Introduced xc_dom_set_gnttab_entry() to avoid duplicated code.
---
 tools/libxc/include/xc_dom.h        |  12 +--
 tools/libxc/xc_dom_boot.c           | 166 +++++++++++++++++++++++-------------
 tools/libxc/xc_sr_restore_x86_hvm.c |  10 +--
 tools/libxc/xc_sr_restore_x86_pv.c  |   2 +-
 tools/libxl/libxl_dom.c             |   1 -
 tools/python/xen/lowlevel/xc/xc.c   |   6 +-
 6 files changed, 120 insertions(+), 77 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 8a66889c68..0b5a632d3c 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -337,14 +337,10 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, xen_pfn_t pfn,
 int xc_dom_boot_image(struct xc_dom_image *dom);
 int xc_dom_compat_check(struct xc_dom_image *dom);
 int xc_dom_gnttab_init(struct xc_dom_image *dom);
-int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
-                           xen_pfn_t console_gmfn,
-                           xen_pfn_t xenstore_gmfn,
-                           uint32_t console_domid,
-                           uint32_t xenstore_domid);
-int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
-                       xen_pfn_t console_gmfn,
-                       xen_pfn_t xenstore_gmfn,
+int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
+                       bool is_hvm,
+                       xen_pfn_t console_gfn,
+                       xen_pfn_t xenstore_gfn,
                        uint32_t console_domid,
                        uint32_t xenstore_domid);
 bool xc_dom_translated(const struct xc_dom_image *dom);
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 2e5681dc5d..0f852237ee 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -256,71 +256,81 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, uint32_t domid)
     return gmfn;
 }
 
-int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
-                       xen_pfn_t console_gmfn,
-                       xen_pfn_t xenstore_gmfn,
-                       uint32_t console_domid,
-                       uint32_t xenstore_domid)
+static void xc_dom_set_gnttab_entry(xc_interface *xch,
+                                    grant_entry_v1_t *gnttab,
+                                    unsigned int idx,
+                                    uint32_t guest_domid,
+                                    uint32_t backend_domid,
+                                    xen_pfn_t guest_gfn)
 {
+    if ( guest_domid == backend_domid || guest_gfn == -1 )
+        return;
+
+    xc_dom_printf(xch, "%s: [%u] -> 0x%"PRI_xen_pfn,
+                  __func__, idx, guest_gfn);
+
+    gnttab[idx].flags = GTF_permit_access;
+    gnttab[idx].domid = backend_domid;
+    gnttab[idx].frame = guest_gfn;
+}
 
-    xen_pfn_t gnttab_gmfn;
+static int compat_gnttab_seed(xc_interface *xch, uint32_t domid,
+                              xen_pfn_t console_gfn,
+                              xen_pfn_t xenstore_gfn,
+                              uint32_t console_domid,
+                              uint32_t xenstore_domid)
+{
+
+    xen_pfn_t gnttab_gfn;
     grant_entry_v1_t *gnttab;
 
-    gnttab_gmfn = xc_dom_gnttab_setup(xch, domid);
-    if ( gnttab_gmfn == -1 )
+    gnttab_gfn = xc_dom_gnttab_setup(xch, domid);
+    if ( gnttab_gfn == -1 )
         return -1;
 
     gnttab = xc_map_foreign_range(xch,
                                   domid,
                                   PAGE_SIZE,
                                   PROT_READ|PROT_WRITE,
-                                  gnttab_gmfn);
+                                  gnttab_gfn);
     if ( gnttab == NULL )
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to map domU grant table "
+                     "%s: failed to map d%d grant table "
                      "[errno=%d]\n",
-                     __FUNCTION__, errno);
+                     __func__, domid, errno);
         return -1;
     }
 
-    if ( domid != console_domid  && console_gmfn != -1)
-    {
-        gnttab[GNTTAB_RESERVED_CONSOLE].flags = GTF_permit_access;
-        gnttab[GNTTAB_RESERVED_CONSOLE].domid = console_domid;
-        gnttab[GNTTAB_RESERVED_CONSOLE].frame = console_gmfn;
-    }
-    if ( domid != xenstore_domid && xenstore_gmfn != -1)
-    {
-        gnttab[GNTTAB_RESERVED_XENSTORE].flags = GTF_permit_access;
-        gnttab[GNTTAB_RESERVED_XENSTORE].domid = xenstore_domid;
-        gnttab[GNTTAB_RESERVED_XENSTORE].frame = xenstore_gmfn;
-    }
+    xc_dom_set_gnttab_entry(xch, gnttab, GNTTAB_RESERVED_CONSOLE,
+                            domid, console_domid, console_gfn);
+    xc_dom_set_gnttab_entry(xch, gnttab, GNTTAB_RESERVED_XENSTORE,
+                            domid, xenstore_domid, xenstore_gfn);
 
     if ( munmap(gnttab, PAGE_SIZE) == -1 )
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to unmap domU grant table "
+                     "%s: failed to unmap d%d grant table "
                      "[errno=%d]\n",
-                     __FUNCTION__, errno);
+                     __func__, domid, errno);
         return -1;
     }
 
     /* Guest shouldn't really touch its grant table until it has
      * enabled its caches. But lets be nice. */
-    xc_domain_cacheflush(xch, domid, gnttab_gmfn, 1);
+    xc_domain_cacheflush(xch, domid, gnttab_gfn, 1);
 
     return 0;
 }
 
-int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
-                           xen_pfn_t console_gpfn,
-                           xen_pfn_t xenstore_gpfn,
-                           uint32_t console_domid,
-                           uint32_t xenstore_domid)
+static int compat_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
+                                  xen_pfn_t console_gfn,
+                                  xen_pfn_t xenstore_gfn,
+                                  uint32_t console_domid,
+                                  uint32_t xenstore_domid)
 {
     int rc;
-    xen_pfn_t scratch_gpfn;
+    xen_pfn_t scratch_gfn;
     struct xen_add_to_physmap xatp = {
         .domid = domid,
         .space = XENMAPSPACE_grant_table,
@@ -330,41 +340,41 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
         .domid = domid,
     };
 
-    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gpfn);
+    rc = xc_core_arch_get_scratch_gpfn(xch, domid, &scratch_gfn);
     if ( rc < 0 )
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to get a scratch gfn "
+                     "%s: failed to get a scratch gfn from d%d"
                      "[errno=%d]\n",
-                     __FUNCTION__, errno);
+                     __func__, domid, errno);
         return -1;
     }
-    xatp.gpfn = scratch_gpfn;
-    xrfp.gpfn = scratch_gpfn;
-
-    xc_dom_printf(xch, "%s: called, pfn=0x%"PRI_xen_pfn, __FUNCTION__,
-                  scratch_gpfn);
+    xatp.gpfn = scratch_gfn;
+    xrfp.gpfn = scratch_gfn;
 
+    xc_dom_printf(xch, "%s: d%d: pfn=0x%"PRI_xen_pfn, __func__,
+                  domid, scratch_gfn);
 
     rc = do_memory_op(xch, XENMEM_add_to_physmap, &xatp, sizeof(xatp));
     if ( rc != 0 )
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to add gnttab to physmap "
+                     "%s: failed to add gnttab to d%d physmap "
                      "[errno=%d]\n",
-                     __FUNCTION__, errno);
+                     __func__, domid, errno);
         return -1;
     }
 
-    rc = xc_dom_gnttab_seed(xch, domid,
-                            console_gpfn, xenstore_gpfn,
+    rc = compat_gnttab_seed(xch, domid,
+                            console_gfn, xenstore_gfn,
                             console_domid, xenstore_domid);
     if (rc != 0)
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to seed gnttab entries\n",
-                     __FUNCTION__);
-        (void) do_memory_op(xch, XENMEM_remove_from_physmap, &xrfp, sizeof(xrfp));
+                     "%s: failed to seed gnttab entries for d%d\n",
+                     __func__, domid);
+        (void) do_memory_op(xch, XENMEM_remove_from_physmap, &xrfp,
+                            sizeof(xrfp));
         return -1;
     }
 
@@ -372,27 +382,65 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
     if (rc != 0)
     {
         xc_dom_panic(xch, XC_INTERNAL_ERROR,
-                     "%s: failed to remove gnttab from physmap "
+                     "%s: failed to remove gnttab from d%d physmap "
                      "[errno=%d]\n",
-                     __FUNCTION__, errno);
+                     __func__, domid, errno);
         return -1;
     }
 
     return 0;
 }
 
-int xc_dom_gnttab_init(struct xc_dom_image *dom)
+int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
+                       bool is_hvm, xen_pfn_t console_gfn,
+                       xen_pfn_t xenstore_gfn, uint32_t console_domid,
+                       uint32_t xenstore_domid)
 {
-    if ( xc_dom_translated(dom) ) {
-        return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
-                                      dom->console_pfn, dom->xenstore_pfn,
-                                      dom->console_domid, dom->xenstore_domid);
-    } else {
-        return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
-                                  xc_dom_p2m(dom, dom->console_pfn),
-                                  xc_dom_p2m(dom, dom->xenstore_pfn),
-                                  dom->console_domid, dom->xenstore_domid);
+    xenforeignmemory_handle* fmem = xch->fmem;
+    xenforeignmemory_resource_handle *fres;
+    void *addr = NULL;
+
+    fres = xenforeignmemory_map_resource(
+        fmem, guest_domid, XENMEM_resource_grant_table,
+        XENMEM_resource_grant_table_id_shared, 0, 1, &addr,
+        PROT_READ | PROT_WRITE, 0);
+    if ( !fres )
+    {
+        if ( errno == EOPNOTSUPP )
+            return is_hvm ?
+                compat_gnttab_hvm_seed(xch, guest_domid,
+                                       console_gfn, xenstore_gfn,
+                                       console_domid, xenstore_domid) :
+                compat_gnttab_seed(xch, guest_domid,
+                                   console_gfn, xenstore_gfn,
+                                   console_domid, xenstore_domid);
+
+        xc_dom_panic(xch, XC_INTERNAL_ERROR,
+                     "%s: failed to acquire d%d grant table "
+                     "[errno=%d]\n",
+                     __func__, guest_domid, errno);
+        return -1;
     }
+
+    xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_CONSOLE,
+                            guest_domid, console_domid, console_gfn);
+    xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_XENSTORE,
+                            guest_domid, xenstore_domid, xenstore_gfn);
+
+    xenforeignmemory_unmap_resource(fmem, fres);
+
+    return 0;
+}
+
+int xc_dom_gnttab_init(struct xc_dom_image *dom)
+{
+    bool is_hvm = xc_dom_translated(dom);
+    xen_pfn_t console_gfn = xc_dom_p2m(dom, dom->console_pfn);
+    xen_pfn_t xenstore_gfn = xc_dom_p2m(dom, dom->xenstore_pfn);
+
+    return xc_dom_gnttab_seed(dom->xch, dom->guest_domid, is_hvm,
+                              console_gfn, xenstore_gfn,
+                              dom->console_domid, dom->xenstore_domid);
 }
 
 /*
diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c b/tools/libxc/xc_sr_restore_x86_hvm.c
index 227c48553e..4765a52f33 100644
--- a/tools/libxc/xc_sr_restore_x86_hvm.c
+++ b/tools/libxc/xc_sr_restore_x86_hvm.c
@@ -216,11 +216,11 @@ static int x86_hvm_stream_complete(struct xc_sr_context *ctx)
         return rc;
     }
 
-    rc = xc_dom_gnttab_hvm_seed(xch, ctx->domid,
-                                ctx->restore.console_gfn,
-                                ctx->restore.xenstore_gfn,
-                                ctx->restore.console_domid,
-                                ctx->restore.xenstore_domid);
+    rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
+                            ctx->restore.console_gfn,
+                            ctx->restore.xenstore_gfn,
+                            ctx->restore.console_domid,
+                            ctx->restore.xenstore_domid);
     if ( rc )
     {
         PERROR("Failed to seed grant table");
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index d81dfdcca6..a2dbf85157 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -1105,7 +1105,7 @@ static int x86_pv_stream_complete(struct xc_sr_context *ctx)
     if ( rc )
         return rc;
 
-    rc = xc_dom_gnttab_seed(xch, ctx->domid,
+    rc = xc_dom_gnttab_seed(xch, ctx->domid, false,
                             ctx->restore.console_gfn,
                             ctx->restore.xenstore_gfn,
                             ctx->restore.console_domid,
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 3cfe0d4808..c8a1dc7fd5 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -905,7 +905,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
     *store_mfn = str_mfn;
     *console_mfn = cons_mfn;
 
-    xc_dom_gnttab_hvm_seed(handle, domid, *console_mfn, *store_mfn, console_domid, store_domid);
     return 0;
 }
 
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index fc19ee0741..5ade12762a 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -800,9 +800,9 @@ static PyObject *pyxc_gnttab_hvm_seed(XcObject *self,
 				      &console_domid, &xenstore_domid) )
         return NULL;
 
-    if ( xc_dom_gnttab_hvm_seed(self->xc_handle, dom,
-				console_gmfn, xenstore_gmfn,
-				console_domid, xenstore_domid) != 0 )
+    if ( xc_dom_gnttab_seed(self->xc_handle, dom, true,
+                            console_gmfn, xenstore_gmfn,
+                            console_domid, xenstore_domid) != 0 )
         return pyxc_error_to_exception(self->xc_handle);
 
     return Py_None;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v23 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Jan Beulich]

>>> On 09.08.18 at 11:59, <paul.durrant@citrix.com> wrote:
> +static int gnttab_get_status_frame_mfn(struct domain *d,
> +                                       unsigned long idx, mfn_t *mfn)
> +{
> +    const struct grant_table *gt = d->grant_table;
> +
> +    ASSERT(gt->gt_version == 2);
> +
> +    if ( idx >= nr_status_frames(gt) )
> +    {
> +        unsigned long nr_status;
> +        unsigned long nr_grant;
> +
> +        nr_status = idx + 1; /* sufficient frames to make idx valid */
> +
> +        if ( nr_status <= nr_status_frames(gt) ) /* overflow check */
> +            return -EINVAL;

Still pretty odd a check, even if now at least correct. Why not simply
check nr_status to be zero? Let me know if you're fine with me making
this adjustment while committing:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

That said though - idx being -1UL is not really "invalid". In an abstract
world it simply means a fully populated table of maximum size. But of
course the table can't grow this large in practice, because each entry
is more than one byte (i.e. we'd still get -EINVAL further down).

> +        nr_grant = status_to_grant_frames(nr_status);

Irrespective of the R-b above: This is the real source of possible
overflows, as here nr_status gets multiplied by a value larger than 1.
I therefore wonder whether it wouldn't be better to check here
that the reverse translation yields nr_status again. Once again I'd
be fine adding this while committing, provided you agree.

Otoh I'm not convinced all this overflow checking does much good
here anyway: Anyone setting the maximum table size so absurdly
high that this would start to matter is going to have bigger trouble
anyway afaict.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v23 1/2] common: add a new mappable resource type: XENMEM_resource_grant_table

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 09 August 2018 11:24
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: Re: [PATCH v23 1/2] common: add a new mappable resource type:
> XENMEM_resource_grant_table
> 
> >>> On 09.08.18 at 11:59, <paul.durrant@citrix.com> wrote:
> > +static int gnttab_get_status_frame_mfn(struct domain *d,
> > +                                       unsigned long idx, mfn_t *mfn)
> > +{
> > +    const struct grant_table *gt = d->grant_table;
> > +
> > +    ASSERT(gt->gt_version == 2);
> > +
> > +    if ( idx >= nr_status_frames(gt) )
> > +    {
> > +        unsigned long nr_status;
> > +        unsigned long nr_grant;
> > +
> > +        nr_status = idx + 1; /* sufficient frames to make idx valid */
> > +
> > +        if ( nr_status <= nr_status_frames(gt) ) /* overflow check */
> > +            return -EINVAL;
> 
> Still pretty odd a check, even if now at least correct. Why not simply
> check nr_status to be zero? Let me know if you're fine with me making
> this adjustment while committing:

Yes, I'm happy for you to adjust overflow checking as you see fit. (I thought just doing a similar check as the outer if was kind of more obvious, but given it's a + 1 then clearly checking against 0 would be fine too).

> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> That said though - idx being -1UL is not really "invalid". In an abstract
> world it simply means a fully populated table of maximum size. But of
> course the table can't grow this large in practice, because each entry
> is more than one byte (i.e. we'd still get -EINVAL further down).
> 
> > +        nr_grant = status_to_grant_frames(nr_status);
> 
> Irrespective of the R-b above: This is the real source of possible
> overflows, as here nr_status gets multiplied by a value larger than 1.
> I therefore wonder whether it wouldn't be better to check here
> that the reverse translation yields nr_status again. Once again I'd
> be fine adding this while committing, provided you agree.
> 

Yes, that sounds like a worthwhile check.

> Otoh I'm not convinced all this overflow checking does much good
> here anyway: Anyone setting the maximum table size so absurdly
> high that this would start to matter is going to have bigger trouble
> anyway afaict.

True.

  Paul

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel