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

[Paul Durrant <paul.durrant@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel