[PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[jennifer.herbert]

From: Jennifer Herbert <Jennifer.Herbert@citrix.com>

No functional change.

Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
--
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd..fb4bcec 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -25,6 +25,13 @@
 
 #include <xsm/xsm.h>
 
+struct dmop_args {
+    domid_t domid;
+    unsigned int nr_bufs;
+    /* Reserve enough buf elements for all current hypercalls. */
+    struct xen_dm_op_buf buf[2];
+};
+
 static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
                                 unsigned int nr_bufs, void *dst,
                                 unsigned int idx, size_t dst_size)
@@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
     return 0;
 }
 
-static int dm_op(domid_t domid,
-                 unsigned int nr_bufs,
-                 xen_dm_op_buf_t bufs[])
+static int dm_op(struct dmop_args *op_args)
 {
     struct domain *d;
     struct xen_dm_op op;
     bool const_op = true;
     long rc;
 
-    rc = rcu_lock_remote_domain_by_id(domid, &d);
+    rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
     if ( rc )
         return rc;
 
@@ -307,7 +312,7 @@ static int dm_op(domid_t domid,
     if ( rc )
         goto out;
 
-    if ( !copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op)) )
+    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0, sizeof(op)) )
     {
         rc = -EFAULT;
         goto out;
@@ -466,10 +471,10 @@ static int dm_op(domid_t domid,
         if ( data->pad )
             break;
 
-        if ( nr_bufs < 2 )
+        if ( op_args->nr_bufs < 2 )
             break;
 
-        rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs[1]);
+        rc = track_dirty_vram(d, data->first_pfn, data->nr, &op_args->buf[1]);
         break;
     }
 
@@ -564,7 +569,7 @@ static int dm_op(domid_t domid,
 
     if ( (!rc || rc == -ERESTART) &&
          !const_op &&
-         !copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op)) )
+         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op, sizeof(op)) )
         rc = -EFAULT;
 
  out:
@@ -587,20 +592,21 @@ CHECK_dm_op_set_mem_type;
 CHECK_dm_op_inject_event;
 CHECK_dm_op_inject_msi;
 
-#define MAX_NR_BUFS 2
-
 int compat_dm_op(domid_t domid,
                  unsigned int nr_bufs,
                  XEN_GUEST_HANDLE_PARAM(void) bufs)
 {
-    struct xen_dm_op_buf nat[MAX_NR_BUFS];
+    struct dmop_args args;
     unsigned int i;
     int rc;
 
-    if ( nr_bufs > MAX_NR_BUFS )
+    if ( nr_bufs > ARRAY_SIZE(args.buf) )
         return -E2BIG;
 
-    for ( i = 0; i < nr_bufs; i++ )
+    args.domid = domid;
+    args.nr_bufs = nr_bufs;
+
+    for ( i = 0; i < args.nr_bufs; i++ )
     {
         struct compat_dm_op_buf cmp;
 
@@ -610,12 +616,12 @@ int compat_dm_op(domid_t domid,
 #define XLAT_dm_op_buf_HNDL_h(_d_, _s_) \
         guest_from_compat_handle((_d_)->h, (_s_)->h)
 
-        XLAT_dm_op_buf(&nat[i], &cmp);
+        XLAT_dm_op_buf(&args.buf[i], &cmp);
 
 #undef XLAT_dm_op_buf_HNDL_h
     }
 
-    rc = dm_op(domid, nr_bufs, nat);
+    rc = dm_op(&args);
 
     if ( rc == -ERESTART )
         rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
@@ -628,16 +634,19 @@ long do_dm_op(domid_t domid,
               unsigned int nr_bufs,
               XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
 {
-    struct xen_dm_op_buf nat[MAX_NR_BUFS];
+    struct dmop_args args;
     int rc;
 
-    if ( nr_bufs > MAX_NR_BUFS )
+    if ( nr_bufs > ARRAY_SIZE(args.buf) )
         return -E2BIG;
 
-    if ( copy_from_guest_offset(nat, bufs, 0, nr_bufs) )
+    args.domid = domid;
+    args.nr_bufs = nr_bufs;
+
+    if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) )
         return -EFAULT;
 
-    rc = dm_op(domid, nr_bufs, nat);
+    rc = dm_op(&args);
 
     if ( rc == -ERESTART )
         rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
-- 
2.1.4


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

[PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[jennifer.herbert]

From: Jennifer Herbert <Jennifer.Herbert@citrix.com>

This also allows the usual cases to be simplified, by omitting an unnecessary
buf parameters, and because the macros can appropriately size the object.

This makes copying to or from a buf that isn't big enough an error.
If the buffer isnt big enough, trying to carry on regardless
can only cause trouble later on.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
--
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/dm.c | 47 +++++++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index fb4bcec..3607ddb 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -32,36 +32,47 @@ struct dmop_args {
     struct xen_dm_op_buf buf[2];
 };
 
-static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
-                                unsigned int nr_bufs, void *dst,
-                                unsigned int idx, size_t dst_size)
+static bool _raw_copy_from_guest_buf(void *dst,
+                                     const struct dmop_args *args,
+                                     unsigned int buf_idx,
+                                     size_t dst_bytes)
 {
-    size_t size;
+    size_t buf_bytes;
 
-    if ( idx >= nr_bufs )
+    if ( buf_idx >= args->nr_bufs )
         return false;
 
-    memset(dst, 0, dst_size);
+    buf_bytes =  args->buf[buf_idx].size;
 
-    size = min_t(size_t, dst_size, bufs[idx].size);
+    if ( dst_bytes > buf_bytes )
+        return false;
 
-    return !copy_from_guest(dst, bufs[idx].h, size);
+    return !copy_from_guest(dst, args->buf[buf_idx].h, buf_bytes);
 }
 
-static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
-                              unsigned int nr_bufs, unsigned int idx,
-                              const void *src, size_t src_size)
+static bool _raw_copy_to_guest_buf(struct dmop_args *args,
+                                   unsigned int buf_idx,
+                                   const void *src, size_t src_bytes)
 {
-    size_t size;
+    size_t buf_bytes;
 
-    if ( idx >= nr_bufs )
+    if ( buf_idx >= args->nr_bufs )
         return false;
 
-    size = min_t(size_t, bufs[idx].size, src_size);
+    buf_bytes = args->buf[buf_idx].size;
+
+    if ( src_bytes > buf_bytes )
+        return false;
 
-    return !copy_to_guest(bufs[idx].h, src, size);
+    return !copy_to_guest(args->buf[buf_idx].h, src, buf_bytes);
 }
 
+#define copy_from_guest_buf(dst, args, buf_idx) \
+    _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst)))
+
+#define copy_to_guest_buf(args, buf_idx, src) \
+    _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src)))
+
 static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
                             unsigned int nr, struct xen_dm_op_buf *buf)
 {
@@ -312,7 +323,7 @@ static int dm_op(struct dmop_args *op_args)
     if ( rc )
         goto out;
 
-    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0, sizeof(op)) )
+    if ( !copy_from_guest_buf(&op, op_args, 0) );
     {
         rc = -EFAULT;
         goto out;
@@ -568,8 +579,8 @@ static int dm_op(struct dmop_args *op_args)
     }
 
     if ( (!rc || rc == -ERESTART) &&
-         !const_op &&
-         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op, sizeof(op)) )
+         !const_op && !copy_to_guest_buf(op_args, 0, &op) )
+
         rc = -EFAULT;
 
  out:
-- 
2.1.4


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

[PATCH 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers

[jennifer.herbert]

From: Jennifer Herbert <Jennifer.Herbert@citrix.com>

copy_{to,from}_guest_buf() are now implemented using an offset of 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
--
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/dm.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 3607ddb..6990725 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -32,10 +32,11 @@ struct dmop_args {
     struct xen_dm_op_buf buf[2];
 };
 
-static bool _raw_copy_from_guest_buf(void *dst,
-                                     const struct dmop_args *args,
-                                     unsigned int buf_idx,
-                                     size_t dst_bytes)
+static bool _raw_copy_from_guest_buf_offset(void *dst,
+                                            const struct dmop_args *args,
+                                            unsigned int buf_idx,
+                                            size_t offset_bytes,
+                                            size_t dst_bytes)
 {
     size_t buf_bytes;
 
@@ -44,15 +45,20 @@ static bool _raw_copy_from_guest_buf(void *dst,
 
     buf_bytes =  args->buf[buf_idx].size;
 
-    if ( dst_bytes > buf_bytes )
+    if ( offset_bytes >= buf_bytes ||
+         (offset_bytes + dst_bytes) < offset_bytes ||
+         (offset_bytes + dst_bytes) > buf_bytes )
         return false;
 
-    return !copy_from_guest(dst, args->buf[buf_idx].h, buf_bytes);
+    return !copy_from_guest_offset(dst, args->buf[buf_idx].h,
+                                   offset_bytes, dst_bytes);
 }
 
-static bool _raw_copy_to_guest_buf(struct dmop_args *args,
-                                   unsigned int buf_idx,
-                                   const void *src, size_t src_bytes)
+static bool _raw_copy_to_guest_buf_offset(struct dmop_args *args,
+                                          unsigned int buf_idx,
+                                          size_t offset_bytes,
+                                          const void *src,
+                                          size_t src_bytes)
 {
     size_t buf_bytes;
 
@@ -61,17 +67,29 @@ static bool _raw_copy_to_guest_buf(struct dmop_args *args,
 
     buf_bytes = args->buf[buf_idx].size;
 
-    if ( src_bytes > buf_bytes )
+
+    if ( offset_bytes >= buf_bytes ||
+         (offset_bytes + src_bytes) < offset_bytes ||
+         (offset_bytes + src_bytes) > buf_bytes )
         return false;
 
-    return !copy_to_guest(args->buf[buf_idx].h, src, buf_bytes);
+    return !copy_to_guest_offset(args->buf[buf_idx].h, offset_bytes,
+                                 src, src_bytes);
 }
 
-#define copy_from_guest_buf(dst, args, buf_idx) \
-    _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst)))
+#define copy_from_guest_buf_offset(dst, bufs, buf_idx, offset_bytes) \
+    _raw_copy_from_guest_buf_offset(dst, bufs, buf_idx, offset_bytes, \
+                                    sizeof(*(dst)))
+
+#define copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, src) \
+    _raw_copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, \
+                                  src, sizeof(*(src)))
+
+#define copy_from_guest_buf(dst, bufs, buf_idx) \
+    copy_from_guest_buf_offset(dst, bufs, buf_idx, 0)
 
-#define copy_to_guest_buf(args, buf_idx, src) \
-    _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src)))
+#define copy_to_guest_buf(bufs, buf_idx, src) \
+    copy_to_guest_buf_offset(bufs, buf_idx, 0, src)
 
 static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
                             unsigned int nr, struct xen_dm_op_buf *buf)
-- 
2.1.4


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

[PATCH 4/4] dmop: Add xendevicemodel_modified_memory_bulk()

[jennifer.herbert]

From: Jennifer Herbert <Jennifer.Herbert@citrix.com>

This new lib devicemodel call allows multiple extents of pages to be
marked as modified in a single call.  This is something needed for a
usecase I'm working on.

The xen side of the modified_memory call has been modified to accept
an array of extents.  The devicemodel library either provides an array
of length 1, to support the original library function, or a second
function allows an array to be provided.

Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
--
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libs/devicemodel/core.c                   |  30 ++++--
 tools/libs/devicemodel/include/xendevicemodel.h |  19 +++-
 xen/arch/x86/hvm/dm.c                           | 117 ++++++++++++++++--------
 xen/include/public/hvm/dm_op.h                  |  19 +++-
 4 files changed, 134 insertions(+), 51 deletions(-)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index ff09819..d7c6476 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -459,22 +459,36 @@ int xendevicemodel_track_dirty_vram(
                              dirty_bitmap, (size_t)(nr + 7) / 8);
 }
 
-int xendevicemodel_modified_memory(
-    xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
-    uint32_t nr)
+int xendevicemodel_modified_memory_bulk(
+    xendevicemodel_handle *dmod, domid_t domid,
+    struct xen_dm_op_modified_memory_extent *extents, uint32_t nr)
 {
     struct xen_dm_op op;
-    struct xen_dm_op_modified_memory *data;
+    struct xen_dm_op_modified_memory *header;
+    size_t extents_size = nr * sizeof(struct xen_dm_op_modified_memory_extent);
 
     memset(&op, 0, sizeof(op));
 
     op.op = XEN_DMOP_modified_memory;
-    data = &op.u.modified_memory;
+    header = &op.u.modified_memory;
 
-    data->first_pfn = first_pfn;
-    data->nr = nr;
+    header->nr_extents = nr;
+    header->opaque = 0;
 
-    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+    return xendevicemodel_op(dmod, domid, 2, &op, sizeof(op),
+                             extents, extents_size);
+}
+
+int xendevicemodel_modified_memory(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
+    uint32_t nr)
+{
+    struct xen_dm_op_modified_memory_extent extent;
+
+    extent.first_pfn = first_pfn;
+    extent.nr = nr;
+
+    return xendevicemodel_modified_memory_bulk(dmod, domid, &extent, 1);
 }
 
 int xendevicemodel_set_mem_type(
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h
index 1da216f..580fad2 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -254,8 +254,8 @@ int xendevicemodel_track_dirty_vram(
     uint32_t nr, unsigned long *dirty_bitmap);
 
 /**
- * This function notifies the hypervisor that a set of domain pages
- * have been modified.
+ * This function notifies the hypervisor that a set of contiguous
+ * domain pages have been modified.
  *
  * @parm dmod a handle to an open devicemodel interface.
  * @parm domid the domain id to be serviced
@@ -268,6 +268,21 @@ int xendevicemodel_modified_memory(
     uint32_t nr);
 
 /**
+ * This function notifies the hypervisor that a set of discontiguous
+ * domain pages have been modified.
+ *
+ * @parm dmod a handle to an open devicemodel interface.
+ * @parm domid the domain id to be serviced
+ * @parm extents an array of extent structs, which each hold
+                 a start_pfn and nr (number of pfns).
+ * @parm nr the number of extents in the array
+ * @return 0 on success, -1 on failure.
+ */
+int xendevicemodel_modified_memory_bulk(
+    xendevicemodel_handle *dmod, domid_t domid,
+    struct xen_dm_op_modified_memory_extent extents[], uint32_t nr);
+
+/**
  * This function notifies the hypervisor that a set of domain pages
  * are to be treated in a specific way. (See the definition of
  * hvmmem_type_t).
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 6990725..61df3cf 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -155,56 +155,102 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
 }
 
 static int modified_memory(struct domain *d,
-                           struct xen_dm_op_modified_memory *data)
+                           struct dmop_args *bufs,
+                           struct xen_dm_op_modified_memory *header)
 {
-    xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
-    unsigned int iter = 0;
-    int rc = 0;
+#define EXTENTS_BUFFER 1
 
-    if ( (data->first_pfn > last_pfn) ||
-         (last_pfn > domain_get_maximum_gpfn(d)) )
-        return -EINVAL;
+    /* Process maximum of 256 pfns before checking for continuation. */
+    const unsigned int cont_check_interval = 0x100;
+    unsigned int *rem_extents =  &header->nr_extents;
+    unsigned int batch_rem_pfns = cont_check_interval;
+    /* Used for continuation. */
+    unsigned int *pfns_done = &header->opaque;
 
     if ( !paging_mode_log_dirty(d) )
         return 0;
 
-    while ( iter < data->nr )
+    if ( (bufs->buf[EXTENTS_BUFFER].size /
+          sizeof(struct xen_dm_op_modified_memory_extent)) <
+         *rem_extents )
+        return -EINVAL;
+
+    while ( *rem_extents > 0 )
     {
-        unsigned long pfn = data->first_pfn + iter;
-        struct page_info *page;
+        struct xen_dm_op_modified_memory_extent extent;
+        unsigned int batch_nr;
+        xen_pfn_t pfn, end_pfn;
+        int rc;
+
+        rc = copy_from_guest_buf_offset(&extent,
+            bufs, EXTENTS_BUFFER, (*rem_extents - 1) * sizeof(extent));
+        if ( rc )
+            return -EFAULT;
+
+        if ( extent.pad )
+            return -EINVAL;
+
+        end_pfn = extent.first_pfn + extent.nr;
+
+        if ( end_pfn <= extent.first_pfn ||
+             end_pfn > domain_get_maximum_gpfn(d) )
+            return -EINVAL;
 
-        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
-        if ( page )
+        if ( *pfns_done >= extent.nr )
+            return -EINVAL;
+
+        pfn = extent.first_pfn + *pfns_done;
+        batch_nr = extent.nr - *pfns_done;
+
+        if ( batch_nr > batch_rem_pfns )
+        {
+            batch_nr = batch_rem_pfns;
+            *pfns_done += batch_nr;
+            end_pfn = pfn + batch_nr;
+        }
+        else
         {
-            mfn_t gmfn = _mfn(page_to_mfn(page));
-
-            paging_mark_dirty(d, gmfn);
-            /*
-             * These are most probably not page tables any more
-             * don't take a long time and don't die either.
-             */
-            sh_remove_shadows(d, gmfn, 1, 0);
-            put_page(page);
+            (*rem_extents)--;
+            *pfns_done = 0;
         }
 
-        iter++;
+        batch_rem_pfns -= batch_nr;
+
+        for ( ; pfn < end_pfn; pfn++ )
+        {
+            struct page_info *page;
+
+            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
+            if ( page )
+            {
+                mfn_t gmfn = _mfn(page_to_mfn(page));
+
+                paging_mark_dirty(d, gmfn);
+                /*
+                 * These are most probably not page tables any more
+                 * don't take a long time and don't die either.
+                 */
+                sh_remove_shadows(d, gmfn, 1, 0);
+                put_page(page);
+            }
+        }
 
         /*
-         * Check for continuation every 256th iteration and if the
-         * iteration is not the last.
+         * After a full batch of cont_check_interval pfns
+         * have been processed, and there are still extents
+         * remaining to process, check for continuation.
          */
-        if ( (iter < data->nr) && ((iter & 0xff) == 0) &&
-             hypercall_preempt_check() )
+        if ( (batch_rem_pfns == 0) && (*rem_extents > 0) )
         {
-            data->first_pfn += iter;
-            data->nr -= iter;
+            if ( hypercall_preempt_check() )
+                return -ERESTART;
 
-            rc = -ERESTART;
-            break;
+            batch_rem_pfns = cont_check_interval;
         }
     }
+    return 0;
 
-    return rc;
+#undef EXTENTS_BUFFER
 }
 
 static bool allow_p2m_type_change(p2m_type_t old, p2m_type_t new)
@@ -541,13 +587,8 @@ static int dm_op(struct dmop_args *op_args)
         struct xen_dm_op_modified_memory *data =
             &op.u.modified_memory;
 
-        const_op = false;
-
-        rc = -EINVAL;
-        if ( data->pad )
-            break;
-
-        rc = modified_memory(d, data);
+        rc = modified_memory(d, op_args, data);
+        const_op = !rc;
         break;
     }
 
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index 5ea79ef..20c21b6 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -237,13 +237,26 @@ struct xen_dm_op_set_pci_link_route {
  * XEN_DMOP_modified_memory: Notify that a set of pages were modified by
  *                           an emulator.
  *
- * NOTE: In the event of a continuation, the @first_pfn is set to the
- *       value of the pfn of the remaining set of pages and @nr reduced
- *       to the size of the remaining set.
+ * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
+ * @nr_extents entries.
+ *
+ * On error, @nr_extents will contain the index+1 of the extent that
+ * had the error.  It is not defined if or which pages may have been
+ * marked as dirty, in this event.
  */
 #define XEN_DMOP_modified_memory 11
 
 struct xen_dm_op_modified_memory {
+    /*
+     * IN - Number of extents to be processed
+     * OUT -returns n+1 for failing extent
+     */
+    uint32_t nr_extents;
+    /* IN/OUT - Must be set to 0 */
+    uint32_t opaque;
+};
+
+struct xen_dm_op_modified_memory_extent {
     /* IN - number of contiguous pages modified */
     uint32_t nr;
     uint32_t pad;
-- 
2.1.4


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

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Jan Beulich]

>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> No functional change.
> 
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

As already given for v5
Reviewed-by: Jan Beulich <jbeulich@suse.com>

As a general note, though: You've lost the version indicator, and
there's no summary of the changes done from the previous one.

Jan


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>

Is this correct, considering that iirc the patch was new in v5 and ...

> This also allows the usual cases to be simplified, by omitting an 
> unnecessary
> buf parameters, and because the macros can appropriately size the object.
> 
> This makes copying to or from a buf that isn't big enough an error.
> If the buffer isnt big enough, trying to carry on regardless
> can only cause trouble later on.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>

... this sequence of S-o-b-s?

> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -32,36 +32,47 @@ struct dmop_args {
>      struct xen_dm_op_buf buf[2];
>  };
>  
> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> -                                unsigned int nr_bufs, void *dst,
> -                                unsigned int idx, size_t dst_size)
> +static bool _raw_copy_from_guest_buf(void *dst,
> +                                     const struct dmop_args *args,
> +                                     unsigned int buf_idx,
> +                                     size_t dst_bytes)
>  {
> -    size_t size;
> +    size_t buf_bytes;
>  
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
>  
> -    memset(dst, 0, dst_size);
> +    buf_bytes =  args->buf[buf_idx].size;
>  
> -    size = min_t(size_t, dst_size, bufs[idx].size);
> +    if ( dst_bytes > buf_bytes )
> +        return false;

While this behavioral change is now being mentioned in the
description, I'm not sure I buy the argument of basically being
guaranteed to cause trouble down the road. Did you consider the
forward compatibility aspect here, allowing us to extend interface
structures by adding fields to their ends without breaking old
callers? Paul, what are your thoughts here?

Jan


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

Re: [PATCH 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers

[Jan Beulich]

>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
> @@ -44,15 +45,20 @@ static bool _raw_copy_from_guest_buf(void *dst,
>  
>      buf_bytes =  args->buf[buf_idx].size;
>  
> -    if ( dst_bytes > buf_bytes )
> +    if ( offset_bytes >= buf_bytes ||
> +         (offset_bytes + dst_bytes) < offset_bytes ||
> +         (offset_bytes + dst_bytes) > buf_bytes )
>          return false;

Looks like the first of these checks is redundant with the third
one, especially since - afaics - we don't even need to consider
the special case of dst_bytes being zero (and if we still wanted
to, this could be folded into the second check by making it <=
instead of < ).

Other than that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 4/4] dmop: Add xendevicemodel_modified_memory_bulk()

[Jan Beulich]

>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> This new lib devicemodel call allows multiple extents of pages to be
> marked as modified in a single call.  This is something needed for a
> usecase I'm working on.
> 
> The xen side of the modified_memory call has been modified to accept
> an array of extents.  The devicemodel library either provides an array
> of length 1, to support the original library function, or a second
> function allows an array to be provided.
> 
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Paul Durrant]

> -----Original Message-----
> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> Sent: 20 April 2017 19:00
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> multiple parameters around
> 
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> No functional change.
> 
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> --
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++------------
> -------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d72b7bd..fb4bcec 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -25,6 +25,13 @@
> 
>  #include <xsm/xsm.h>
> 
> +struct dmop_args {
> +    domid_t domid;
> +    unsigned int nr_bufs;
> +    /* Reserve enough buf elements for all current hypercalls. */
> +    struct xen_dm_op_buf buf[2];
> +};
> +
>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>                                  unsigned int nr_bufs, void *dst,
>                                  unsigned int idx, size_t dst_size)
> @@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
>      return 0;
>  }
> 
> -static int dm_op(domid_t domid,
> -                 unsigned int nr_bufs,
> -                 xen_dm_op_buf_t bufs[])
> +static int dm_op(struct dmop_args *op_args)

Shouldn't this be a const pointer?

Apart from that...

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

>  {
>      struct domain *d;
>      struct xen_dm_op op;
>      bool const_op = true;
>      long rc;
> 
> -    rc = rcu_lock_remote_domain_by_id(domid, &d);
> +    rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
>      if ( rc )
>          return rc;
> 
> @@ -307,7 +312,7 @@ static int dm_op(domid_t domid,
>      if ( rc )
>          goto out;
> 
> -    if ( !copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op)) )
> +    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0,
> sizeof(op)) )
>      {
>          rc = -EFAULT;
>          goto out;
> @@ -466,10 +471,10 @@ static int dm_op(domid_t domid,
>          if ( data->pad )
>              break;
> 
> -        if ( nr_bufs < 2 )
> +        if ( op_args->nr_bufs < 2 )
>              break;
> 
> -        rc = track_dirty_vram(d, data->first_pfn, data->nr, &bufs[1]);
> +        rc = track_dirty_vram(d, data->first_pfn, data->nr, &op_args->buf[1]);
>          break;
>      }
> 
> @@ -564,7 +569,7 @@ static int dm_op(domid_t domid,
> 
>      if ( (!rc || rc == -ERESTART) &&
>           !const_op &&
> -         !copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op)) )
> +         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op,
> sizeof(op)) )
>          rc = -EFAULT;
> 
>   out:
> @@ -587,20 +592,21 @@ CHECK_dm_op_set_mem_type;
>  CHECK_dm_op_inject_event;
>  CHECK_dm_op_inject_msi;
> 
> -#define MAX_NR_BUFS 2
> -
>  int compat_dm_op(domid_t domid,
>                   unsigned int nr_bufs,
>                   XEN_GUEST_HANDLE_PARAM(void) bufs)
>  {
> -    struct xen_dm_op_buf nat[MAX_NR_BUFS];
> +    struct dmop_args args;
>      unsigned int i;
>      int rc;
> 
> -    if ( nr_bufs > MAX_NR_BUFS )
> +    if ( nr_bufs > ARRAY_SIZE(args.buf) )
>          return -E2BIG;
> 
> -    for ( i = 0; i < nr_bufs; i++ )
> +    args.domid = domid;
> +    args.nr_bufs = nr_bufs;
> +
> +    for ( i = 0; i < args.nr_bufs; i++ )
>      {
>          struct compat_dm_op_buf cmp;
> 
> @@ -610,12 +616,12 @@ int compat_dm_op(domid_t domid,
>  #define XLAT_dm_op_buf_HNDL_h(_d_, _s_) \
>          guest_from_compat_handle((_d_)->h, (_s_)->h)
> 
> -        XLAT_dm_op_buf(&nat[i], &cmp);
> +        XLAT_dm_op_buf(&args.buf[i], &cmp);
> 
>  #undef XLAT_dm_op_buf_HNDL_h
>      }
> 
> -    rc = dm_op(domid, nr_bufs, nat);
> +    rc = dm_op(&args);
> 
>      if ( rc == -ERESTART )
>          rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> @@ -628,16 +634,19 @@ long do_dm_op(domid_t domid,
>                unsigned int nr_bufs,
>                XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
>  {
> -    struct xen_dm_op_buf nat[MAX_NR_BUFS];
> +    struct dmop_args args;
>      int rc;
> 
> -    if ( nr_bufs > MAX_NR_BUFS )
> +    if ( nr_bufs > ARRAY_SIZE(args.buf) )
>          return -E2BIG;
> 
> -    if ( copy_from_guest_offset(nat, bufs, 0, nr_bufs) )
> +    args.domid = domid;
> +    args.nr_bufs = nr_bufs;
> +
> +    if ( copy_from_guest_offset(&args.buf[0], bufs, 0, args.nr_bufs) )
>          return -EFAULT;
> 
> -    rc = dm_op(domid, nr_bufs, nat);
> +    rc = dm_op(&args);
> 
>      if ( rc == -ERESTART )
>          rc = hypercall_create_continuation(__HYPERVISOR_dm_op, "iih",
> --
> 2.1.4


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Paul Durrant]

> -----Original Message-----
> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> Sent: 20 April 2017 19:00
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in
> terms of raw accessors
> 
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> This also allows the usual cases to be simplified, by omitting an unnecessary
> buf parameters, and because the macros can appropriately size the object.
> 
> This makes copying to or from a buf that isn't big enough an error.
> If the buffer isnt big enough, trying to carry on regardless
> can only cause trouble later on.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> --
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/dm.c | 47 +++++++++++++++++++++++++++++----------
> --------
>  1 file changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index fb4bcec..3607ddb 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -32,36 +32,47 @@ struct dmop_args {
>      struct xen_dm_op_buf buf[2];
>  };
> 
> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> -                                unsigned int nr_bufs, void *dst,
> -                                unsigned int idx, size_t dst_size)
> +static bool _raw_copy_from_guest_buf(void *dst,
> +                                     const struct dmop_args *args,
> +                                     unsigned int buf_idx,
> +                                     size_t dst_bytes)
>  {
> -    size_t size;
> +    size_t buf_bytes;
> 
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
> 
> -    memset(dst, 0, dst_size);
> +    buf_bytes =  args->buf[buf_idx].size;
> 
> -    size = min_t(size_t, dst_size, bufs[idx].size);
> +    if ( dst_bytes > buf_bytes )
> +        return false;
> 
> -    return !copy_from_guest(dst, bufs[idx].h, size);
> +    return !copy_from_guest(dst, args->buf[buf_idx].h, buf_bytes);
>  }
> 
> -static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
> -                              unsigned int nr_bufs, unsigned int idx,
> -                              const void *src, size_t src_size)
> +static bool _raw_copy_to_guest_buf(struct dmop_args *args,

I think this should be const, same as in the copy-from case above.

> +                                   unsigned int buf_idx,
> +                                   const void *src, size_t src_bytes)
>  {
> -    size_t size;
> +    size_t buf_bytes;
> 
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
> 
> -    size = min_t(size_t, bufs[idx].size, src_size);
> +    buf_bytes = args->buf[buf_idx].size;
> +
> +    if ( src_bytes > buf_bytes )
> +        return false;
> 
> -    return !copy_to_guest(bufs[idx].h, src, size);
> +    return !copy_to_guest(args->buf[buf_idx].h, src, buf_bytes);
>  }
> 
> +#define copy_from_guest_buf(dst, args, buf_idx) \
> +    _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst)))
> +
> +#define copy_to_guest_buf(args, buf_idx, src) \
> +    _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src)))
> +

Not sure I like the use of sizeof(*<thing>) in a macro. If someone was to use these macros and pass a pointer to allocated memory rather than &<thing-on-stack> then they would not have the desired effect. Clearly such use would be very naïve but I wonder whether having something like:

#define copy_to_guest_buf(args, buf_idx, src) \
    _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))

would be safer.

  Paul

>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
>                              unsigned int nr, struct xen_dm_op_buf *buf)
>  {
> @@ -312,7 +323,7 @@ static int dm_op(struct dmop_args *op_args)
>      if ( rc )
>          goto out;
> 
> -    if ( !copy_buf_from_guest(&op_args->buf[0], op_args->nr_bufs, &op, 0,
> sizeof(op)) )
> +    if ( !copy_from_guest_buf(&op, op_args, 0) );
>      {
>          rc = -EFAULT;
>          goto out;
> @@ -568,8 +579,8 @@ static int dm_op(struct dmop_args *op_args)
>      }
> 
>      if ( (!rc || rc == -ERESTART) &&
> -         !const_op &&
> -         !copy_buf_to_guest(&op_args->buf[0], op_args->nr_bufs, 0, &op,
> sizeof(op)) )
> +         !const_op && !copy_to_guest_buf(op_args, 0, &op) )
> +
>          rc = -EFAULT;
> 
>   out:
> --
> 2.1.4


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

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Andrew Cooper]

On 21/04/2017 08:54, Paul Durrant wrote:
>> -----Original Message-----
>> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
>> Sent: 20 April 2017 19:00
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
>> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>> multiple parameters around
>>
>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>
>> No functional change.
>>
>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> --
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++------------
>> -------
>>  1 file changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>> index d72b7bd..fb4bcec 100644
>> --- a/xen/arch/x86/hvm/dm.c
>> +++ b/xen/arch/x86/hvm/dm.c
>> @@ -25,6 +25,13 @@
>>
>>  #include <xsm/xsm.h>
>>
>> +struct dmop_args {
>> +    domid_t domid;
>> +    unsigned int nr_bufs;
>> +    /* Reserve enough buf elements for all current hypercalls. */
>> +    struct xen_dm_op_buf buf[2];
>> +};
>> +
>>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>                                  unsigned int nr_bufs, void *dst,
>>                                  unsigned int idx, size_t dst_size)
>> @@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
>>      return 0;
>>  }
>>
>> -static int dm_op(domid_t domid,
>> -                 unsigned int nr_bufs,
>> -                 xen_dm_op_buf_t bufs[])
>> +static int dm_op(struct dmop_args *op_args)
> Shouldn't this be a const pointer?

No.  copy_to_guest_buf() uses a non const reference of op_args->buf[$IDX].

~Andrew

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

Re: [PATCH 3/4] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers

[Paul Durrant]

> -----Original Message-----
> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> Sent: 20 April 2017 19:00
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 3/4] hvm/dmop: Implement copy_{to,
> from}_guest_buf_offset() helpers
> 
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> copy_{to,from}_guest_buf() are now implemented using an offset of 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> --
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/dm.c | 48 +++++++++++++++++++++++++++++++++----
> -----------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 3607ddb..6990725 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -32,10 +32,11 @@ struct dmop_args {
>      struct xen_dm_op_buf buf[2];
>  };
> 
> -static bool _raw_copy_from_guest_buf(void *dst,
> -                                     const struct dmop_args *args,
> -                                     unsigned int buf_idx,
> -                                     size_t dst_bytes)
> +static bool _raw_copy_from_guest_buf_offset(void *dst,
> +                                            const struct dmop_args *args,
> +                                            unsigned int buf_idx,
> +                                            size_t offset_bytes,
> +                                            size_t dst_bytes)
>  {
>      size_t buf_bytes;
> 
> @@ -44,15 +45,20 @@ static bool _raw_copy_from_guest_buf(void *dst,
> 
>      buf_bytes =  args->buf[buf_idx].size;
> 
> -    if ( dst_bytes > buf_bytes )
> +    if ( offset_bytes >= buf_bytes ||
> +         (offset_bytes + dst_bytes) < offset_bytes ||
> +         (offset_bytes + dst_bytes) > buf_bytes )
>          return false;
> 
> -    return !copy_from_guest(dst, args->buf[buf_idx].h, buf_bytes);
> +    return !copy_from_guest_offset(dst, args->buf[buf_idx].h,
> +                                   offset_bytes, dst_bytes);
>  }
> 
> -static bool _raw_copy_to_guest_buf(struct dmop_args *args,
> -                                   unsigned int buf_idx,
> -                                   const void *src, size_t src_bytes)
> +static bool _raw_copy_to_guest_buf_offset(struct dmop_args *args,
> +                                          unsigned int buf_idx,
> +                                          size_t offset_bytes,
> +                                          const void *src,
> +                                          size_t src_bytes)
>  {
>      size_t buf_bytes;
> 
> @@ -61,17 +67,29 @@ static bool _raw_copy_to_guest_buf(struct
> dmop_args *args,
> 
>      buf_bytes = args->buf[buf_idx].size;
> 
> -    if ( src_bytes > buf_bytes )
> +
> +    if ( offset_bytes >= buf_bytes ||
> +         (offset_bytes + src_bytes) < offset_bytes ||
> +         (offset_bytes + src_bytes) > buf_bytes )
>          return false;
> 
> -    return !copy_to_guest(args->buf[buf_idx].h, src, buf_bytes);
> +    return !copy_to_guest_offset(args->buf[buf_idx].h, offset_bytes,
> +                                 src, src_bytes);
>  }
> 
> -#define copy_from_guest_buf(dst, args, buf_idx) \
> -    _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst)))
> +#define copy_from_guest_buf_offset(dst, bufs, buf_idx, offset_bytes) \
> +    _raw_copy_from_guest_buf_offset(dst, bufs, buf_idx, offset_bytes, \
> +                                    sizeof(*(dst)))
> +
> +#define copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, src) \
> +    _raw_copy_to_guest_buf_offset(bufs, buf_idx, offset_bytes, \
> +                                  src, sizeof(*(src)))
> +
> +#define copy_from_guest_buf(dst, bufs, buf_idx) \
> +    copy_from_guest_buf_offset(dst, bufs, buf_idx, 0)
> 
> -#define copy_to_guest_buf(args, buf_idx, src) \
> -    _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src)))
> +#define copy_to_guest_buf(bufs, buf_idx, src) \
> +    copy_to_guest_buf_offset(bufs, buf_idx, 0, src)
> 
>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
>                              unsigned int nr, struct xen_dm_op_buf *buf)
> --
> 2.1.4


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

Re: [PATCH 4/4] dmop: Add xendevicemodel_modified_memory_bulk()

[Paul Durrant]

> -----Original Message-----
> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> Sent: 20 April 2017 19:00
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Andrew
> Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Julien Grall
> <julien.grall@arm.com>
> Subject: [PATCH 4/4] dmop: Add xendevicemodel_modified_memory_bulk()
> 
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> This new lib devicemodel call allows multiple extents of pages to be
> marked as modified in a single call.  This is something needed for a
> usecase I'm working on.
> 
> The xen side of the modified_memory call has been modified to accept
> an array of extents.  The devicemodel library either provides an array
> of length 1, to support the original library function, or a second
> function allows an array to be provided.
> 
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> --
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  tools/libs/devicemodel/core.c                   |  30 ++++--
>  tools/libs/devicemodel/include/xendevicemodel.h |  19 +++-
>  xen/arch/x86/hvm/dm.c                           | 117 ++++++++++++++++--------
>  xen/include/public/hvm/dm_op.h                  |  19 +++-
>  4 files changed, 134 insertions(+), 51 deletions(-)
> 
> diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> index ff09819..d7c6476 100644
> --- a/tools/libs/devicemodel/core.c
> +++ b/tools/libs/devicemodel/core.c
> @@ -459,22 +459,36 @@ int xendevicemodel_track_dirty_vram(
>                               dirty_bitmap, (size_t)(nr + 7) / 8);
>  }
> 
> -int xendevicemodel_modified_memory(
> -    xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
> -    uint32_t nr)
> +int xendevicemodel_modified_memory_bulk(
> +    xendevicemodel_handle *dmod, domid_t domid,
> +    struct xen_dm_op_modified_memory_extent *extents, uint32_t nr)
>  {
>      struct xen_dm_op op;
> -    struct xen_dm_op_modified_memory *data;
> +    struct xen_dm_op_modified_memory *header;
> +    size_t extents_size = nr * sizeof(struct
> xen_dm_op_modified_memory_extent);
> 
>      memset(&op, 0, sizeof(op));
> 
>      op.op = XEN_DMOP_modified_memory;
> -    data = &op.u.modified_memory;
> +    header = &op.u.modified_memory;
> 
> -    data->first_pfn = first_pfn;
> -    data->nr = nr;
> +    header->nr_extents = nr;
> +    header->opaque = 0;
> 
> -    return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +    return xendevicemodel_op(dmod, domid, 2, &op, sizeof(op),
> +                             extents, extents_size);
> +}
> +
> +int xendevicemodel_modified_memory(
> +    xendevicemodel_handle *dmod, domid_t domid, uint64_t first_pfn,
> +    uint32_t nr)
> +{
> +    struct xen_dm_op_modified_memory_extent extent;
> +
> +    extent.first_pfn = first_pfn;
> +    extent.nr = nr;
> +
> +    return xendevicemodel_modified_memory_bulk(dmod, domid, &extent,
> 1);
>  }
> 
>  int xendevicemodel_set_mem_type(
> diff --git a/tools/libs/devicemodel/include/xendevicemodel.h
> b/tools/libs/devicemodel/include/xendevicemodel.h
> index 1da216f..580fad2 100644
> --- a/tools/libs/devicemodel/include/xendevicemodel.h
> +++ b/tools/libs/devicemodel/include/xendevicemodel.h
> @@ -254,8 +254,8 @@ int xendevicemodel_track_dirty_vram(
>      uint32_t nr, unsigned long *dirty_bitmap);
> 
>  /**
> - * This function notifies the hypervisor that a set of domain pages
> - * have been modified.
> + * This function notifies the hypervisor that a set of contiguous
> + * domain pages have been modified.
>   *
>   * @parm dmod a handle to an open devicemodel interface.
>   * @parm domid the domain id to be serviced
> @@ -268,6 +268,21 @@ int xendevicemodel_modified_memory(
>      uint32_t nr);
> 
>  /**
> + * This function notifies the hypervisor that a set of discontiguous
> + * domain pages have been modified.
> + *
> + * @parm dmod a handle to an open devicemodel interface.
> + * @parm domid the domain id to be serviced
> + * @parm extents an array of extent structs, which each hold
> +                 a start_pfn and nr (number of pfns).
> + * @parm nr the number of extents in the array
> + * @return 0 on success, -1 on failure.
> + */
> +int xendevicemodel_modified_memory_bulk(
> +    xendevicemodel_handle *dmod, domid_t domid,
> +    struct xen_dm_op_modified_memory_extent extents[], uint32_t nr);
> +
> +/**
>   * This function notifies the hypervisor that a set of domain pages
>   * are to be treated in a specific way. (See the definition of
>   * hvmmem_type_t).
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 6990725..61df3cf 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -155,56 +155,102 @@ static int set_isa_irq_level(struct domain *d,
> uint8_t isa_irq,
>  }
> 
>  static int modified_memory(struct domain *d,
> -                           struct xen_dm_op_modified_memory *data)
> +                           struct dmop_args *bufs,
> +                           struct xen_dm_op_modified_memory *header)
>  {
> -    xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
> -    unsigned int iter = 0;
> -    int rc = 0;
> +#define EXTENTS_BUFFER 1
> 
> -    if ( (data->first_pfn > last_pfn) ||
> -         (last_pfn > domain_get_maximum_gpfn(d)) )
> -        return -EINVAL;
> +    /* Process maximum of 256 pfns before checking for continuation. */
> +    const unsigned int cont_check_interval = 0x100;
> +    unsigned int *rem_extents =  &header->nr_extents;
> +    unsigned int batch_rem_pfns = cont_check_interval;
> +    /* Used for continuation. */
> +    unsigned int *pfns_done = &header->opaque;
> 
>      if ( !paging_mode_log_dirty(d) )
>          return 0;
> 
> -    while ( iter < data->nr )
> +    if ( (bufs->buf[EXTENTS_BUFFER].size /
> +          sizeof(struct xen_dm_op_modified_memory_extent)) <
> +         *rem_extents )
> +        return -EINVAL;
> +
> +    while ( *rem_extents > 0 )
>      {
> -        unsigned long pfn = data->first_pfn + iter;
> -        struct page_info *page;
> +        struct xen_dm_op_modified_memory_extent extent;
> +        unsigned int batch_nr;
> +        xen_pfn_t pfn, end_pfn;
> +        int rc;
> +
> +        rc = copy_from_guest_buf_offset(&extent,
> +            bufs, EXTENTS_BUFFER, (*rem_extents - 1) * sizeof(extent));
> +        if ( rc )
> +            return -EFAULT;
> +
> +        if ( extent.pad )
> +            return -EINVAL;
> +
> +        end_pfn = extent.first_pfn + extent.nr;
> +
> +        if ( end_pfn <= extent.first_pfn ||
> +             end_pfn > domain_get_maximum_gpfn(d) )
> +            return -EINVAL;
> 
> -        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> -        if ( page )
> +        if ( *pfns_done >= extent.nr )
> +            return -EINVAL;
> +
> +        pfn = extent.first_pfn + *pfns_done;
> +        batch_nr = extent.nr - *pfns_done;
> +
> +        if ( batch_nr > batch_rem_pfns )
> +        {
> +            batch_nr = batch_rem_pfns;
> +            *pfns_done += batch_nr;
> +            end_pfn = pfn + batch_nr;
> +        }
> +        else
>          {
> -            mfn_t gmfn = _mfn(page_to_mfn(page));
> -
> -            paging_mark_dirty(d, gmfn);
> -            /*
> -             * These are most probably not page tables any more
> -             * don't take a long time and don't die either.
> -             */
> -            sh_remove_shadows(d, gmfn, 1, 0);
> -            put_page(page);
> +            (*rem_extents)--;
> +            *pfns_done = 0;
>          }
> 
> -        iter++;
> +        batch_rem_pfns -= batch_nr;
> +
> +        for ( ; pfn < end_pfn; pfn++ )
> +        {
> +            struct page_info *page;
> +
> +            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> +            if ( page )
> +            {
> +                mfn_t gmfn = _mfn(page_to_mfn(page));
> +
> +                paging_mark_dirty(d, gmfn);
> +                /*
> +                 * These are most probably not page tables any more
> +                 * don't take a long time and don't die either.
> +                 */
> +                sh_remove_shadows(d, gmfn, 1, 0);
> +                put_page(page);
> +            }
> +        }
> 
>          /*
> -         * Check for continuation every 256th iteration and if the
> -         * iteration is not the last.
> +         * After a full batch of cont_check_interval pfns
> +         * have been processed, and there are still extents
> +         * remaining to process, check for continuation.
>           */
> -        if ( (iter < data->nr) && ((iter & 0xff) == 0) &&
> -             hypercall_preempt_check() )
> +        if ( (batch_rem_pfns == 0) && (*rem_extents > 0) )
>          {
> -            data->first_pfn += iter;
> -            data->nr -= iter;
> +            if ( hypercall_preempt_check() )
> +                return -ERESTART;
> 
> -            rc = -ERESTART;
> -            break;
> +            batch_rem_pfns = cont_check_interval;
>          }
>      }
> +    return 0;
> 
> -    return rc;
> +#undef EXTENTS_BUFFER
>  }
> 
>  static bool allow_p2m_type_change(p2m_type_t old, p2m_type_t new)
> @@ -541,13 +587,8 @@ static int dm_op(struct dmop_args *op_args)
>          struct xen_dm_op_modified_memory *data =
>              &op.u.modified_memory;
> 
> -        const_op = false;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;
> -
> -        rc = modified_memory(d, data);
> +        rc = modified_memory(d, op_args, data);
> +        const_op = !rc;
>          break;
>      }
> 
> diff --git a/xen/include/public/hvm/dm_op.h
> b/xen/include/public/hvm/dm_op.h
> index 5ea79ef..20c21b6 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -237,13 +237,26 @@ struct xen_dm_op_set_pci_link_route {
>   * XEN_DMOP_modified_memory: Notify that a set of pages were modified
> by
>   *                           an emulator.
>   *
> - * NOTE: In the event of a continuation, the @first_pfn is set to the
> - *       value of the pfn of the remaining set of pages and @nr reduced
> - *       to the size of the remaining set.
> + * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent
> with
> + * @nr_extents entries.
> + *
> + * On error, @nr_extents will contain the index+1 of the extent that
> + * had the error.  It is not defined if or which pages may have been
> + * marked as dirty, in this event.
>   */
>  #define XEN_DMOP_modified_memory 11
> 
>  struct xen_dm_op_modified_memory {
> +    /*
> +     * IN - Number of extents to be processed
> +     * OUT -returns n+1 for failing extent
> +     */
> +    uint32_t nr_extents;
> +    /* IN/OUT - Must be set to 0 */
> +    uint32_t opaque;
> +};
> +
> +struct xen_dm_op_modified_memory_extent {
>      /* IN - number of contiguous pages modified */
>      uint32_t nr;
>      uint32_t pad;
> --
> 2.1.4


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

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
> Andrew Cooper
> Sent: 21 April 2017 09:04
> To: Paul Durrant <Paul.Durrant@citrix.com>; Jennifer Herbert
> <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> multiple parameters around
> 
> On 21/04/2017 08:54, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> >> Sent: 20 April 2017 19:00
> >> To: Xen-devel <xen-devel@lists.xen.org>
> >> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> >> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> >> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> >> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> >> multiple parameters around
> >>
> >> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>
> >> No functional change.
> >>
> >> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> --
> >> CC: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> ---
> >>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++--------
> ----
> >> -------
> >>  1 file changed, 28 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >> index d72b7bd..fb4bcec 100644
> >> --- a/xen/arch/x86/hvm/dm.c
> >> +++ b/xen/arch/x86/hvm/dm.c
> >> @@ -25,6 +25,13 @@
> >>
> >>  #include <xsm/xsm.h>
> >>
> >> +struct dmop_args {
> >> +    domid_t domid;
> >> +    unsigned int nr_bufs;
> >> +    /* Reserve enough buf elements for all current hypercalls. */
> >> +    struct xen_dm_op_buf buf[2];
> >> +};
> >> +
> >>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> >>                                  unsigned int nr_bufs, void *dst,
> >>                                  unsigned int idx, size_t dst_size)
> >> @@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
> >>      return 0;
> >>  }
> >>
> >> -static int dm_op(domid_t domid,
> >> -                 unsigned int nr_bufs,
> >> -                 xen_dm_op_buf_t bufs[])
> >> +static int dm_op(struct dmop_args *op_args)
> > Shouldn't this be a const pointer?
> 
> No.  copy_to_guest_buf() uses a non const reference of op_args-
> >buf[$IDX].
> 

Can't that be const too (as I commented in the relevant patch)?

  Paul

> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Andrew Cooper]

On 21/04/2017 09:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
>> Andrew Cooper
>> Sent: 21 April 2017 09:04
>> To: Paul Durrant <Paul.Durrant@citrix.com>; Jennifer Herbert
>> <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
>> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>> multiple parameters around
>>
>> On 21/04/2017 08:54, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
>>>> Sent: 20 April 2017 19:00
>>>> To: Xen-devel <xen-devel@lists.xen.org>
>>>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
>>>> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
>>>> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>>>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>>>> multiple parameters around
>>>>
>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> --
>>>> CC: Paul Durrant <paul.durrant@citrix.com>
>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++--------
>> ----
>>>> -------
>>>>  1 file changed, 28 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>>>> index d72b7bd..fb4bcec 100644
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -25,6 +25,13 @@
>>>>
>>>>  #include <xsm/xsm.h>
>>>>
>>>> +struct dmop_args {
>>>> +    domid_t domid;
>>>> +    unsigned int nr_bufs;
>>>> +    /* Reserve enough buf elements for all current hypercalls. */
>>>> +    struct xen_dm_op_buf buf[2];
>>>> +};
>>>> +
>>>>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>                                  unsigned int nr_bufs, void *dst,
>>>>                                  unsigned int idx, size_t dst_size)
>>>> @@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
>>>>      return 0;
>>>>  }
>>>>
>>>> -static int dm_op(domid_t domid,
>>>> -                 unsigned int nr_bufs,
>>>> -                 xen_dm_op_buf_t bufs[])
>>>> +static int dm_op(struct dmop_args *op_args)
>>> Shouldn't this be a const pointer?
>> No.  copy_to_guest_buf() uses a non const reference of op_args-
>>> buf[$IDX].
> Can't that be const too (as I commented in the relevant patch)?

No.  That is not legal in the C typesystem.

copy_to_guest_offset(args->buf[buf_idx].h, ...) really really uses a non
constant .h here.

The broken quirk of the of the C typesystem which loses const when
following pointers doesn't apply here, because buf[] is an embedded
array and properly inherits the constness of the args pointer.

~Andrew

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

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
> Andrew Cooper
> Sent: 21 April 2017 09:29
> To: Paul Durrant <Paul.Durrant@citrix.com>; Jennifer Herbert
> <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> multiple parameters around
> 
> On 21/04/2017 09:10, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
> >> Andrew Cooper
> >> Sent: 21 April 2017 09:04
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; Jennifer Herbert
> >> <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> >> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> >> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> >> multiple parameters around
> >>
> >> On 21/04/2017 08:54, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: jennifer.herbert@citrix.com
> [mailto:jennifer.herbert@citrix.com]
> >>>> Sent: 20 April 2017 19:00
> >>>> To: Xen-devel <xen-devel@lists.xen.org>
> >>>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
> >>>> <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>;
> >>>> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> >>>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
> >>>> multiple parameters around
> >>>>
> >>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>>>
> >>>> No functional change.
> >>>>
> >>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> --
> >>>> CC: Paul Durrant <paul.durrant@citrix.com>
> >>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> CC: Jan Beulich <JBeulich@suse.com>
> >>>> CC: Julien Grall <julien.grall@arm.com>
> >>>> ---
> >>>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++-----
> ---
> >> ----
> >>>> -------
> >>>>  1 file changed, 28 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >>>> index d72b7bd..fb4bcec 100644
> >>>> --- a/xen/arch/x86/hvm/dm.c
> >>>> +++ b/xen/arch/x86/hvm/dm.c
> >>>> @@ -25,6 +25,13 @@
> >>>>
> >>>>  #include <xsm/xsm.h>
> >>>>
> >>>> +struct dmop_args {
> >>>> +    domid_t domid;
> >>>> +    unsigned int nr_bufs;
> >>>> +    /* Reserve enough buf elements for all current hypercalls. */
> >>>> +    struct xen_dm_op_buf buf[2];
> >>>> +};
> >>>> +
> >>>>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
> >>>>                                  unsigned int nr_bufs, void *dst,
> >>>>                                  unsigned int idx, size_t dst_size)
> >>>> @@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
> >>>>      return 0;
> >>>>  }
> >>>>
> >>>> -static int dm_op(domid_t domid,
> >>>> -                 unsigned int nr_bufs,
> >>>> -                 xen_dm_op_buf_t bufs[])
> >>>> +static int dm_op(struct dmop_args *op_args)
> >>> Shouldn't this be a const pointer?
> >> No.  copy_to_guest_buf() uses a non const reference of op_args-
> >>> buf[$IDX].
> > Can't that be const too (as I commented in the relevant patch)?
> 
> No.  That is not legal in the C typesystem.
> 
> copy_to_guest_offset(args->buf[buf_idx].h, ...) really really uses a non
> constant .h here.
> 
> The broken quirk of the of the C typesystem which loses const when
> following pointers doesn't apply here, because buf[] is an embedded
> array and properly inherits the constness of the args pointer.

That's a shame since nothing in the buf array changes... only what it points at.

  Paul

> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 10:02, <Paul.Durrant@citrix.com> wrote:
>> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
>> Sent: 20 April 2017 19:00
>> +#define copy_from_guest_buf(dst, args, buf_idx) \
>> +    _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst)))
>> +
>> +#define copy_to_guest_buf(args, buf_idx, src) \
>> +    _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src)))
>> +
> 
> Not sure I like the use of sizeof(*<thing>) in a macro. If someone was to use 
> these macros and pass a pointer to allocated memory rather than &<thing-on-stack> 
> then they would not have the desired effect. Clearly such use would be very 
> naïve but I wonder whether having something like:
> 
> #define copy_to_guest_buf(args, buf_idx, src) \
>     _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
> 
> would be safer.

You mean in the case the allocated memory was an array? If it's a
pointer to a singular object, I don't think there would be anything
wrong.

Jan

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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 April 2017 09:48
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Jennifer Herbert
> <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Subject: RE: [PATCH 2/4] hvm/dmop: Implement copy_{to,
> from}_guest_buf() in terms of raw accessors
> 
> >>> On 21.04.17 at 10:02, <Paul.Durrant@citrix.com> wrote:
> >> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> >> Sent: 20 April 2017 19:00
> >> +#define copy_from_guest_buf(dst, args, buf_idx) \
> >> +    _raw_copy_from_guest_buf(dst, args, buf_idx, sizeof(*(dst)))
> >> +
> >> +#define copy_to_guest_buf(args, buf_idx, src) \
> >> +    _raw_copy_to_guest_buf(args, buf_idx, src, sizeof(*(src)))
> >> +
> >
> > Not sure I like the use of sizeof(*<thing>) in a macro. If someone was to
> use
> > these macros and pass a pointer to allocated memory rather than &<thing-
> on-stack>
> > then they would not have the desired effect. Clearly such use would be
> very
> > naïve but I wonder whether having something like:
> >
> > #define copy_to_guest_buf(args, buf_idx, src) \
> >     _raw_copy_to_guest_buf(args, buf_idx, &src, sizeof(src))
> >
> > would be safer.
> 
> You mean in the case the allocated memory was an array? If it's a
> pointer to a singular object, I don't think there would be anything
> wrong.

Yes, or someone erroneously passing a void *. As I said, such code would be very naïve, but changing the macro as I suggest would rule it out.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Andrew Cooper]

On 21/04/2017 08:27, Jan Beulich wrote:
>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> Is this correct, considering that iirc the patch was new in v5 and ...
>
>> This also allows the usual cases to be simplified, by omitting an 
>> unnecessary
>> buf parameters, and because the macros can appropriately size the object.
>>
>> This makes copying to or from a buf that isn't big enough an error.
>> If the buffer isnt big enough, trying to carry on regardless
>> can only cause trouble later on.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> ... this sequence of S-o-b-s?
>
>> --- a/xen/arch/x86/hvm/dm.c
>> +++ b/xen/arch/x86/hvm/dm.c
>> @@ -32,36 +32,47 @@ struct dmop_args {
>>      struct xen_dm_op_buf buf[2];
>>  };
>>  
>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>> -                                unsigned int nr_bufs, void *dst,
>> -                                unsigned int idx, size_t dst_size)
>> +static bool _raw_copy_from_guest_buf(void *dst,
>> +                                     const struct dmop_args *args,
>> +                                     unsigned int buf_idx,
>> +                                     size_t dst_bytes)
>>  {
>> -    size_t size;
>> +    size_t buf_bytes;
>>  
>> -    if ( idx >= nr_bufs )
>> +    if ( buf_idx >= args->nr_bufs )
>>          return false;
>>  
>> -    memset(dst, 0, dst_size);
>> +    buf_bytes =  args->buf[buf_idx].size;
>>  
>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>> +    if ( dst_bytes > buf_bytes )
>> +        return false;
> While this behavioral change is now being mentioned in the
> description, I'm not sure I buy the argument of basically being
> guaranteed to cause trouble down the road. Did you consider the
> forward compatibility aspect here, allowing us to extend interface
> structures by adding fields to their ends without breaking old
> callers? Paul, what are your thoughts here?

DMOP is a stable ABI.  There is no legal extending of any objects.

The previous semantics are guaranteed to break the ABI with future
subops, which is why I removed it.  In the case that the guest
originally passed an overly long buffer, and someone tried be "clever"
here passing the same object here expecting _raw_copy_from_guest_buf()
to DTRT, the function will end up copying too much data from the guest,
and you will end up with something the guest wasn't intending to be part
of the structure replacing the zero extension.

New subops which take a longer object should use a brand new object.

$MAGIC compatibility logic like you want has no business living in the
copy helper.  Had I spotted the intention during the original dmop
series, I would have rejected it during review.

~Andrew

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

Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Jan Beulich]

>>> On 21.04.17 at 10:29, <andrew.cooper3@citrix.com> wrote:
> On 21/04/2017 09:10, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
>>> Andrew Cooper
>>> Sent: 21 April 2017 09:04
>>> To: Paul Durrant <Paul.Durrant@citrix.com>; Jennifer Herbert
>>> <jennifer.herbert@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
>>> Cc: Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>>> Subject: Re: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>>> multiple parameters around
>>>
>>> On 21/04/2017 08:54, Paul Durrant wrote:
>>>>> -----Original Message-----
>>>>> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
>>>>> Sent: 20 April 2017 19:00
>>>>> To: Xen-devel <xen-devel@lists.xen.org>
>>>>> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Andrew Cooper
>>>>> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
>>>>> Jan Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
>>>>> Subject: [PATCH 1/4] hvm/dmop: Box dmop_args rather than passing
>>>>> multiple parameters around
>>>>>
>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> --
>>>>> CC: Paul Durrant <paul.durrant@citrix.com>
>>>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: Julien Grall <julien.grall@arm.com>
>>>>> ---
>>>>>  xen/arch/x86/hvm/dm.c | 47 ++++++++++++++++++++++++++++--------
>>> ----
>>>>> -------
>>>>>  1 file changed, 28 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>>>>> index d72b7bd..fb4bcec 100644
>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>> @@ -25,6 +25,13 @@
>>>>>
>>>>>  #include <xsm/xsm.h>
>>>>>
>>>>> +struct dmop_args {
>>>>> +    domid_t domid;
>>>>> +    unsigned int nr_bufs;
>>>>> +    /* Reserve enough buf elements for all current hypercalls. */
>>>>> +    struct xen_dm_op_buf buf[2];
>>>>> +};
>>>>> +
>>>>>  static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>>                                  unsigned int nr_bufs, void *dst,
>>>>>                                  unsigned int idx, size_t dst_size)
>>>>> @@ -287,16 +294,14 @@ static int inject_event(struct domain *d,
>>>>>      return 0;
>>>>>  }
>>>>>
>>>>> -static int dm_op(domid_t domid,
>>>>> -                 unsigned int nr_bufs,
>>>>> -                 xen_dm_op_buf_t bufs[])
>>>>> +static int dm_op(struct dmop_args *op_args)
>>>> Shouldn't this be a const pointer?
>>> No.  copy_to_guest_buf() uses a non const reference of op_args-
>>>> buf[$IDX].
>> Can't that be const too (as I commented in the relevant patch)?
> 
> No.  That is not legal in the C typesystem.
> 
> copy_to_guest_offset(args->buf[buf_idx].h, ...) really really uses a non
> constant .h here.
> 
> The broken quirk of the of the C typesystem which loses const when
> following pointers doesn't apply here, because buf[] is an embedded
> array and properly inherits the constness of the args pointer.

But the type of what .h refers to isn't affected by this - it'll remain
a handle of void, the const-ness doesn't propagate there. After
all this is just a pointer equivalent, i.e.

struct xen_dm_op_buf {
    void *h;
    ...
};

The const Paul suggests to apply would change this to
"void *const" rather than "const void *", and that should be fine.

Jan


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 10:54, <andrew.cooper3@citrix.com> wrote:
> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>> Is this correct, considering that iirc the patch was new in v5 and ...
>>
>>> This also allows the usual cases to be simplified, by omitting an 
>>> unnecessary
>>> buf parameters, and because the macros can appropriately size the object.
>>>
>>> This makes copying to or from a buf that isn't big enough an error.
>>> If the buffer isnt big enough, trying to carry on regardless
>>> can only cause trouble later on.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>> ... this sequence of S-o-b-s?
>>
>>> --- a/xen/arch/x86/hvm/dm.c
>>> +++ b/xen/arch/x86/hvm/dm.c
>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>      struct xen_dm_op_buf buf[2];
>>>  };
>>>  
>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>> -                                unsigned int nr_bufs, void *dst,
>>> -                                unsigned int idx, size_t dst_size)
>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>> +                                     const struct dmop_args *args,
>>> +                                     unsigned int buf_idx,
>>> +                                     size_t dst_bytes)
>>>  {
>>> -    size_t size;
>>> +    size_t buf_bytes;
>>>  
>>> -    if ( idx >= nr_bufs )
>>> +    if ( buf_idx >= args->nr_bufs )
>>>          return false;
>>>  
>>> -    memset(dst, 0, dst_size);
>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>  
>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>> +    if ( dst_bytes > buf_bytes )
>>> +        return false;
>> While this behavioral change is now being mentioned in the
>> description, I'm not sure I buy the argument of basically being
>> guaranteed to cause trouble down the road. Did you consider the
>> forward compatibility aspect here, allowing us to extend interface
>> structures by adding fields to their ends without breaking old
>> callers? Paul, what are your thoughts here?
> 
> DMOP is a stable ABI.  There is no legal extending of any objects.

I disagree: We have various pad fields which we check to be zero.
Putting flags there indicating presence of extended fields would be
quite fine, without breaking the ABI in any way.

> The previous semantics are guaranteed to break the ABI with future
> subops, which is why I removed it.  In the case that the guest
> originally passed an overly long buffer, and someone tried be "clever"
> here passing the same object here expecting _raw_copy_from_guest_buf()
> to DTRT, the function will end up copying too much data from the guest,
> and you will end up with something the guest wasn't intending to be part
> of the structure replacing the zero extension.

Good point, but taken care of with the flags semantics assignable
to padding fields.

> New subops which take a longer object should use a brand new object.
> 
> $MAGIC compatibility logic like you want has no business living in the
> copy helper.  Had I spotted the intention during the original dmop
> series, I would have rejected it during review.

I'm surprised you didn't notice, because iirc the behavior had
even been changed back and forth between revisions.

Jan


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Andrew Cooper]

On 21/04/2017 09:54, Andrew Cooper wrote:
> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>> Is this correct, considering that iirc the patch was new in v5 and ...
>>
>>> This also allows the usual cases to be simplified, by omitting an 
>>> unnecessary
>>> buf parameters, and because the macros can appropriately size the object.
>>>
>>> This makes copying to or from a buf that isn't big enough an error.
>>> If the buffer isnt big enough, trying to carry on regardless
>>> can only cause trouble later on.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>> ... this sequence of S-o-b-s?
>>
>>> --- a/xen/arch/x86/hvm/dm.c
>>> +++ b/xen/arch/x86/hvm/dm.c
>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>      struct xen_dm_op_buf buf[2];
>>>  };
>>>  
>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>> -                                unsigned int nr_bufs, void *dst,
>>> -                                unsigned int idx, size_t dst_size)
>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>> +                                     const struct dmop_args *args,
>>> +                                     unsigned int buf_idx,
>>> +                                     size_t dst_bytes)
>>>  {
>>> -    size_t size;
>>> +    size_t buf_bytes;
>>>  
>>> -    if ( idx >= nr_bufs )
>>> +    if ( buf_idx >= args->nr_bufs )
>>>          return false;
>>>  
>>> -    memset(dst, 0, dst_size);
>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>  
>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>> +    if ( dst_bytes > buf_bytes )
>>> +        return false;
>> While this behavioral change is now being mentioned in the
>> description, I'm not sure I buy the argument of basically being
>> guaranteed to cause trouble down the road. Did you consider the
>> forward compatibility aspect here, allowing us to extend interface
>> structures by adding fields to their ends without breaking old
>> callers? Paul, what are your thoughts here?
> DMOP is a stable ABI.  There is no legal extending of any objects.
>
> The previous semantics are guaranteed to break the ABI with future
> subops, which is why I removed it.  In the case that the guest
> originally passed an overly long buffer, and someone tried be "clever"
> here passing the same object here expecting _raw_copy_from_guest_buf()
> to DTRT, the function will end up copying too much data from the guest,
> and you will end up with something the guest wasn't intending to be part
> of the structure replacing the zero extension.
>
> New subops which take a longer object should use a brand new object.
>
> $MAGIC compatibility logic like you want has no business living in the
> copy helper.  Had I spotted the intention during the original dmop
> series, I would have rejected it during review.

Actually, the existing behaviour is already broken.  If a guest passes
an overly short buf[0], the dmop logic won't get a failure, and instead
get a truncated structure which has been zero extended.

This is very definitely the wrong thing to do, because such a truncated
structure might actually contain some legitimate operations.

I reiterate my statement that $MAGIC like this has no place living in
the copy helpers.  All it does is introduce bugs.

~Andrew

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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
> On 21/04/2017 09:54, Andrew Cooper wrote:
>> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>> Is this correct, considering that iirc the patch was new in v5 and ...
>>>
>>>> This also allows the usual cases to be simplified, by omitting an 
>>>> unnecessary
>>>> buf parameters, and because the macros can appropriately size the object.
>>>>
>>>> This makes copying to or from a buf that isn't big enough an error.
>>>> If the buffer isnt big enough, trying to carry on regardless
>>>> can only cause trouble later on.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>> ... this sequence of S-o-b-s?
>>>
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>>      struct xen_dm_op_buf buf[2];
>>>>  };
>>>>  
>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>> -                                unsigned int nr_bufs, void *dst,
>>>> -                                unsigned int idx, size_t dst_size)
>>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>>> +                                     const struct dmop_args *args,
>>>> +                                     unsigned int buf_idx,
>>>> +                                     size_t dst_bytes)
>>>>  {
>>>> -    size_t size;
>>>> +    size_t buf_bytes;
>>>>  
>>>> -    if ( idx >= nr_bufs )
>>>> +    if ( buf_idx >= args->nr_bufs )
>>>>          return false;
>>>>  
>>>> -    memset(dst, 0, dst_size);
>>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>>  
>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>>> +    if ( dst_bytes > buf_bytes )
>>>> +        return false;
>>> While this behavioral change is now being mentioned in the
>>> description, I'm not sure I buy the argument of basically being
>>> guaranteed to cause trouble down the road. Did you consider the
>>> forward compatibility aspect here, allowing us to extend interface
>>> structures by adding fields to their ends without breaking old
>>> callers? Paul, what are your thoughts here?
>> DMOP is a stable ABI.  There is no legal extending of any objects.
>>
>> The previous semantics are guaranteed to break the ABI with future
>> subops, which is why I removed it.  In the case that the guest
>> originally passed an overly long buffer, and someone tried be "clever"
>> here passing the same object here expecting _raw_copy_from_guest_buf()
>> to DTRT, the function will end up copying too much data from the guest,
>> and you will end up with something the guest wasn't intending to be part
>> of the structure replacing the zero extension.
>>
>> New subops which take a longer object should use a brand new object.
>>
>> $MAGIC compatibility logic like you want has no business living in the
>> copy helper.  Had I spotted the intention during the original dmop
>> series, I would have rejected it during review.
> 
> Actually, the existing behaviour is already broken.  If a guest passes
> an overly short buf[0], the dmop logic won't get a failure, and instead
> get a truncated structure which has been zero extended.
> 
> This is very definitely the wrong thing to do, because such a truncated
> structure might actually contain some legitimate operations.

So what, if that's what the caller meant to happen?

Considering this is a controversial change, I think it is a bad idea
to merge this into the otherwise uncontroversial change here.

Jan


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jennifer Herbert]

On 21/04/17 10:46, Jan Beulich wrote:
>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
>> On 21/04/2017 09:54, Andrew Cooper wrote:
>>> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>> Is this correct, considering that iirc the patch was new in v5 and ...
>>>>
>>>>> This also allows the usual cases to be simplified, by omitting an
>>>>> unnecessary
>>>>> buf parameters, and because the macros can appropriately size the object.
>>>>>
>>>>> This makes copying to or from a buf that isn't big enough an error.
>>>>> If the buffer isnt big enough, trying to carry on regardless
>>>>> can only cause trouble later on.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>> ... this sequence of S-o-b-s?
>>>>
>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>>>       struct xen_dm_op_buf buf[2];
>>>>>   };
>>>>>   
>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>> -                                unsigned int nr_bufs, void *dst,
>>>>> -                                unsigned int idx, size_t dst_size)
>>>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>>>> +                                     const struct dmop_args *args,
>>>>> +                                     unsigned int buf_idx,
>>>>> +                                     size_t dst_bytes)
>>>>>   {
>>>>> -    size_t size;
>>>>> +    size_t buf_bytes;
>>>>>   
>>>>> -    if ( idx >= nr_bufs )
>>>>> +    if ( buf_idx >= args->nr_bufs )
>>>>>           return false;
>>>>>   
>>>>> -    memset(dst, 0, dst_size);
>>>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>>>   
>>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>>>> +    if ( dst_bytes > buf_bytes )
>>>>> +        return false;
>>>> While this behavioral change is now being mentioned in the
>>>> description, I'm not sure I buy the argument of basically being
>>>> guaranteed to cause trouble down the road. Did you consider the
>>>> forward compatibility aspect here, allowing us to extend interface
>>>> structures by adding fields to their ends without breaking old
>>>> callers? Paul, what are your thoughts here?
>>> DMOP is a stable ABI.  There is no legal extending of any objects.
>>>
>>> The previous semantics are guaranteed to break the ABI with future
>>> subops, which is why I removed it.  In the case that the guest
>>> originally passed an overly long buffer, and someone tried be "clever"
>>> here passing the same object here expecting _raw_copy_from_guest_buf()
>>> to DTRT, the function will end up copying too much data from the guest,
>>> and you will end up with something the guest wasn't intending to be part
>>> of the structure replacing the zero extension.
>>>
>>> New subops which take a longer object should use a brand new object.
>>>
>>> $MAGIC compatibility logic like you want has no business living in the
>>> copy helper.  Had I spotted the intention during the original dmop
>>> series, I would have rejected it during review.
>> Actually, the existing behaviour is already broken.  If a guest passes
>> an overly short buf[0], the dmop logic won't get a failure, and instead
>> get a truncated structure which has been zero extended.
>>
>> This is very definitely the wrong thing to do, because such a truncated
>> structure might actually contain some legitimate operations.
> So what, if that's what the caller meant to happen?
>
> Considering this is a controversial change, I think it is a bad idea
> to merge this into the otherwise uncontroversial change here.

How about we move the min(..) and memset out of the helper function, and 
into dm_op?


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote:
> On 21/04/17 10:46, Jan Beulich wrote:
>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
>>> On 21/04/2017 09:54, Andrew Cooper wrote:
>>>> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>> Is this correct, considering that iirc the patch was new in v5 and ...
>>>>>
>>>>>> This also allows the usual cases to be simplified, by omitting an
>>>>>> unnecessary
>>>>>> buf parameters, and because the macros can appropriately size the object.
>>>>>>
>>>>>> This makes copying to or from a buf that isn't big enough an error.
>>>>>> If the buffer isnt big enough, trying to carry on regardless
>>>>>> can only cause trouble later on.
>>>>>>
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>> ... this sequence of S-o-b-s?
>>>>>
>>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>>>>       struct xen_dm_op_buf buf[2];
>>>>>>   };
>>>>>>   
>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>>> -                                unsigned int nr_bufs, void *dst,
>>>>>> -                                unsigned int idx, size_t dst_size)
>>>>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>>>>> +                                     const struct dmop_args *args,
>>>>>> +                                     unsigned int buf_idx,
>>>>>> +                                     size_t dst_bytes)
>>>>>>   {
>>>>>> -    size_t size;
>>>>>> +    size_t buf_bytes;
>>>>>>   
>>>>>> -    if ( idx >= nr_bufs )
>>>>>> +    if ( buf_idx >= args->nr_bufs )
>>>>>>           return false;
>>>>>>   
>>>>>> -    memset(dst, 0, dst_size);
>>>>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>>>>   
>>>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>>>>> +    if ( dst_bytes > buf_bytes )
>>>>>> +        return false;
>>>>> While this behavioral change is now being mentioned in the
>>>>> description, I'm not sure I buy the argument of basically being
>>>>> guaranteed to cause trouble down the road. Did you consider the
>>>>> forward compatibility aspect here, allowing us to extend interface
>>>>> structures by adding fields to their ends without breaking old
>>>>> callers? Paul, what are your thoughts here?
>>>> DMOP is a stable ABI.  There is no legal extending of any objects.
>>>>
>>>> The previous semantics are guaranteed to break the ABI with future
>>>> subops, which is why I removed it.  In the case that the guest
>>>> originally passed an overly long buffer, and someone tried be "clever"
>>>> here passing the same object here expecting _raw_copy_from_guest_buf()
>>>> to DTRT, the function will end up copying too much data from the guest,
>>>> and you will end up with something the guest wasn't intending to be part
>>>> of the structure replacing the zero extension.
>>>>
>>>> New subops which take a longer object should use a brand new object.
>>>>
>>>> $MAGIC compatibility logic like you want has no business living in the
>>>> copy helper.  Had I spotted the intention during the original dmop
>>>> series, I would have rejected it during review.
>>> Actually, the existing behaviour is already broken.  If a guest passes
>>> an overly short buf[0], the dmop logic won't get a failure, and instead
>>> get a truncated structure which has been zero extended.
>>>
>>> This is very definitely the wrong thing to do, because such a truncated
>>> structure might actually contain some legitimate operations.
>> So what, if that's what the caller meant to happen?
>>
>> Considering this is a controversial change, I think it is a bad idea
>> to merge this into the otherwise uncontroversial change here.
> 
> How about we move the min(..) and memset out of the helper function, and 
> into dm_op?

Wouldn't that result in different buffers being treated differently,
which I'd rather not see happening?

Jan

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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Andrew Cooper]

On 21/04/17 10:46, Jan Beulich wrote:
>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
>> On 21/04/2017 09:54, Andrew Cooper wrote:
>>> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>> Is this correct, considering that iirc the patch was new in v5 and ...
>>>>
>>>>> This also allows the usual cases to be simplified, by omitting an 
>>>>> unnecessary
>>>>> buf parameters, and because the macros can appropriately size the object.
>>>>>
>>>>> This makes copying to or from a buf that isn't big enough an error.
>>>>> If the buffer isnt big enough, trying to carry on regardless
>>>>> can only cause trouble later on.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>> ... this sequence of S-o-b-s?
>>>>
>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>>>      struct xen_dm_op_buf buf[2];
>>>>>  };
>>>>>  
>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>> -                                unsigned int nr_bufs, void *dst,
>>>>> -                                unsigned int idx, size_t dst_size)
>>>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>>>> +                                     const struct dmop_args *args,
>>>>> +                                     unsigned int buf_idx,
>>>>> +                                     size_t dst_bytes)
>>>>>  {
>>>>> -    size_t size;
>>>>> +    size_t buf_bytes;
>>>>>  
>>>>> -    if ( idx >= nr_bufs )
>>>>> +    if ( buf_idx >= args->nr_bufs )
>>>>>          return false;
>>>>>  
>>>>> -    memset(dst, 0, dst_size);
>>>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>>>  
>>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>>>> +    if ( dst_bytes > buf_bytes )
>>>>> +        return false;
>>>> While this behavioral change is now being mentioned in the
>>>> description, I'm not sure I buy the argument of basically being
>>>> guaranteed to cause trouble down the road. Did you consider the
>>>> forward compatibility aspect here, allowing us to extend interface
>>>> structures by adding fields to their ends without breaking old
>>>> callers? Paul, what are your thoughts here?
>>> DMOP is a stable ABI.  There is no legal extending of any objects.
>>>
>>> The previous semantics are guaranteed to break the ABI with future
>>> subops, which is why I removed it.  In the case that the guest
>>> originally passed an overly long buffer, and someone tried be "clever"
>>> here passing the same object here expecting _raw_copy_from_guest_buf()
>>> to DTRT, the function will end up copying too much data from the guest,
>>> and you will end up with something the guest wasn't intending to be part
>>> of the structure replacing the zero extension.
>>>
>>> New subops which take a longer object should use a brand new object.
>>>
>>> $MAGIC compatibility logic like you want has no business living in the
>>> copy helper.  Had I spotted the intention during the original dmop
>>> series, I would have rejected it during review.
>> Actually, the existing behaviour is already broken.  If a guest passes
>> an overly short buf[0], the dmop logic won't get a failure, and instead
>> get a truncated structure which has been zero extended.
>>
>> This is very definitely the wrong thing to do, because such a truncated
>> structure might actually contain some legitimate operations.
> So what, if that's what the caller meant to happen?

If the caller doesn't provide all the information the hypervisor half of
the interface expects, it should get a hard error, not a truncated
attempt to fulful the request.

The existing behaviour is simply broken.

~Andrew

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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jennifer Herbert]

On 21/04/17 11:38, Jan Beulich wrote:
>>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote:
>> On 21/04/17 10:46, Jan Beulich wrote:
>>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
>>>> On 21/04/2017 09:54, Andrew Cooper wrote:
>>>>> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>>> Is this correct, considering that iirc the patch was new in v5 and ...
>>>>>>
>>>>>>> This also allows the usual cases to be simplified, by omitting an
>>>>>>> unnecessary
>>>>>>> buf parameters, and because the macros can appropriately size the object.
>>>>>>>
>>>>>>> This makes copying to or from a buf that isn't big enough an error.
>>>>>>> If the buffer isnt big enough, trying to carry on regardless
>>>>>>> can only cause trouble later on.
>>>>>>>
>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>>> ... this sequence of S-o-b-s?
>>>>>>
>>>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>>>>>        struct xen_dm_op_buf buf[2];
>>>>>>>    };
>>>>>>>    
>>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>>>> -                                unsigned int nr_bufs, void *dst,
>>>>>>> -                                unsigned int idx, size_t dst_size)
>>>>>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>>>>>> +                                     const struct dmop_args *args,
>>>>>>> +                                     unsigned int buf_idx,
>>>>>>> +                                     size_t dst_bytes)
>>>>>>>    {
>>>>>>> -    size_t size;
>>>>>>> +    size_t buf_bytes;
>>>>>>>    
>>>>>>> -    if ( idx >= nr_bufs )
>>>>>>> +    if ( buf_idx >= args->nr_bufs )
>>>>>>>            return false;
>>>>>>>    
>>>>>>> -    memset(dst, 0, dst_size);
>>>>>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>>>>>    
>>>>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>>>>>> +    if ( dst_bytes > buf_bytes )
>>>>>>> +        return false;
>>>>>> While this behavioral change is now being mentioned in the
>>>>>> description, I'm not sure I buy the argument of basically being
>>>>>> guaranteed to cause trouble down the road. Did you consider the
>>>>>> forward compatibility aspect here, allowing us to extend interface
>>>>>> structures by adding fields to their ends without breaking old
>>>>>> callers? Paul, what are your thoughts here?
>>>>> DMOP is a stable ABI.  There is no legal extending of any objects.
>>>>>
>>>>> The previous semantics are guaranteed to break the ABI with future
>>>>> subops, which is why I removed it.  In the case that the guest
>>>>> originally passed an overly long buffer, and someone tried be "clever"
>>>>> here passing the same object here expecting _raw_copy_from_guest_buf()
>>>>> to DTRT, the function will end up copying too much data from the guest,
>>>>> and you will end up with something the guest wasn't intending to be part
>>>>> of the structure replacing the zero extension.
>>>>>
>>>>> New subops which take a longer object should use a brand new object.
>>>>>
>>>>> $MAGIC compatibility logic like you want has no business living in the
>>>>> copy helper.  Had I spotted the intention during the original dmop
>>>>> series, I would have rejected it during review.
>>>> Actually, the existing behaviour is already broken.  If a guest passes
>>>> an overly short buf[0], the dmop logic won't get a failure, and instead
>>>> get a truncated structure which has been zero extended.
>>>>
>>>> This is very definitely the wrong thing to do, because such a truncated
>>>> structure might actually contain some legitimate operations.
>>> So what, if that's what the caller meant to happen?
>>>
>>> Considering this is a controversial change, I think it is a bad idea
>>> to merge this into the otherwise uncontroversial change here.
>> How about we move the min(..) and memset out of the helper function, and
>> into dm_op?
> Wouldn't that result in different buffers being treated differently,
> which I'd rather not see happening?

I think in the general case, its wrong to assume that its ok to truncate 
a buffer.
In specific cases, such as buf[0], we don't really know how big the buf 
needs to be until we start parsing it. In this case, we want to say 
'give me what you have, upto this maximum'.  This is the special case, 
not the general one, an as such does not belong in the general case 
accesser helper.
Certainly when we get to updating the accesser helper to deal with 
offsets, its get more confused.  If we give it an offset greater then 
the size of the buffer, do we just memset?  That would seem wrong.  With 
an offset of 0, we want it to do the memset thing, to deal with buf[0].  
But what about if we have an offset.  I'd think the calling function 
would really want to know if its got nonsense.

Another solution would be to have two variants of the helper function. A 
'normal' one, and the best effort one.

-jenny


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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 13:03, <Jennifer.Herbert@citrix.com> wrote:

> 
> On 21/04/17 11:38, Jan Beulich wrote:
>>>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote:
>>> On 21/04/17 10:46, Jan Beulich wrote:
>>>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
>>>>> On 21/04/2017 09:54, Andrew Cooper wrote:
>>>>>> On 21/04/2017 08:27, Jan Beulich wrote:
>>>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
>>>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>>>> Is this correct, considering that iirc the patch was new in v5 and ...
>>>>>>>
>>>>>>>> This also allows the usual cases to be simplified, by omitting an
>>>>>>>> unnecessary
>>>>>>>> buf parameters, and because the macros can appropriately size the object.
>>>>>>>>
>>>>>>>> This makes copying to or from a buf that isn't big enough an error.
>>>>>>>> If the buffer isnt big enough, trying to carry on regardless
>>>>>>>> can only cause trouble later on.
>>>>>>>>
>>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
>>>>>>> ... this sequence of S-o-b-s?
>>>>>>>
>>>>>>>> --- a/xen/arch/x86/hvm/dm.c
>>>>>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>>>>>> @@ -32,36 +32,47 @@ struct dmop_args {
>>>>>>>>        struct xen_dm_op_buf buf[2];
>>>>>>>>    };
>>>>>>>>    
>>>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t bufs[],
>>>>>>>> -                                unsigned int nr_bufs, void *dst,
>>>>>>>> -                                unsigned int idx, size_t dst_size)
>>>>>>>> +static bool _raw_copy_from_guest_buf(void *dst,
>>>>>>>> +                                     const struct dmop_args *args,
>>>>>>>> +                                     unsigned int buf_idx,
>>>>>>>> +                                     size_t dst_bytes)
>>>>>>>>    {
>>>>>>>> -    size_t size;
>>>>>>>> +    size_t buf_bytes;
>>>>>>>>    
>>>>>>>> -    if ( idx >= nr_bufs )
>>>>>>>> +    if ( buf_idx >= args->nr_bufs )
>>>>>>>>            return false;
>>>>>>>>    
>>>>>>>> -    memset(dst, 0, dst_size);
>>>>>>>> +    buf_bytes =  args->buf[buf_idx].size;
>>>>>>>>    
>>>>>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
>>>>>>>> +    if ( dst_bytes > buf_bytes )
>>>>>>>> +        return false;
>>>>>>> While this behavioral change is now being mentioned in the
>>>>>>> description, I'm not sure I buy the argument of basically being
>>>>>>> guaranteed to cause trouble down the road. Did you consider the
>>>>>>> forward compatibility aspect here, allowing us to extend interface
>>>>>>> structures by adding fields to their ends without breaking old
>>>>>>> callers? Paul, what are your thoughts here?
>>>>>> DMOP is a stable ABI.  There is no legal extending of any objects.
>>>>>>
>>>>>> The previous semantics are guaranteed to break the ABI with future
>>>>>> subops, which is why I removed it.  In the case that the guest
>>>>>> originally passed an overly long buffer, and someone tried be "clever"
>>>>>> here passing the same object here expecting _raw_copy_from_guest_buf()
>>>>>> to DTRT, the function will end up copying too much data from the guest,
>>>>>> and you will end up with something the guest wasn't intending to be part
>>>>>> of the structure replacing the zero extension.
>>>>>>
>>>>>> New subops which take a longer object should use a brand new object.
>>>>>>
>>>>>> $MAGIC compatibility logic like you want has no business living in the
>>>>>> copy helper.  Had I spotted the intention during the original dmop
>>>>>> series, I would have rejected it during review.
>>>>> Actually, the existing behaviour is already broken.  If a guest passes
>>>>> an overly short buf[0], the dmop logic won't get a failure, and instead
>>>>> get a truncated structure which has been zero extended.
>>>>>
>>>>> This is very definitely the wrong thing to do, because such a truncated
>>>>> structure might actually contain some legitimate operations.
>>>> So what, if that's what the caller meant to happen?
>>>>
>>>> Considering this is a controversial change, I think it is a bad idea
>>>> to merge this into the otherwise uncontroversial change here.
>>> How about we move the min(..) and memset out of the helper function, and
>>> into dm_op?
>> Wouldn't that result in different buffers being treated differently,
>> which I'd rather not see happening?
> 
> I think in the general case, its wrong to assume that its ok to truncate 
> a buffer.
> In specific cases, such as buf[0], we don't really know how big the buf 
> needs to be until we start parsing it.

How that? Just like with domctl and sysctl the caller is supposed to
be handing a full struct xen_dm_op, so I'd rather view this as the
specific case where zero-extension is of no use.

> Certainly when we get to updating the accesser helper to deal with 
> offsets, its get more confused.  If we give it an offset greater then 
> the size of the buffer, do we just memset?  That would seem wrong.  With 
> an offset of 0, we want it to do the memset thing, to deal with buf[0].  

Why? It's the natural extension of the zero-extending model.

> But what about if we have an offset.  I'd think the calling function 
> would really want to know if its got nonsense.

Zero-extended input is not nonsense to me.

> Another solution would be to have two variants of the helper function. A 
> 'normal' one, and the best effort one.

I don't think we want that. Andrew being wholeheartedly opposed
to this zero-extension approach, he wouldn't agree to that either
afaict. Considering that I don't expect him to change his mind, I
guess I'll give up opposition to the change done, provided it is
being done as a separate patch. Of course I'd be interested to
know Paul's position here, as back then he appeared to agree
with using this model (or maybe it also was just giving in)...

Jan

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

Re: [PATCH 4/4] dmop: Add xendevicemodel_modified_memory_bulk()

[Wei Liu]

On Thu, Apr 20, 2017 at 05:59:49PM +0000, jennifer.herbert@citrix.com wrote:
> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> 
> This new lib devicemodel call allows multiple extents of pages to be
> marked as modified in a single call.  This is something needed for a
> usecase I'm working on.
> 
> The xen side of the modified_memory call has been modified to accept
> an array of extents.  The devicemodel library either provides an array
> of length 1, to support the original library function, or a second
> function allows an array to be provided.
> 
> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 2/4] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 April 2017 13:05
> To: Jennifer Herbert <jennifer.herbert@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Xen-devel <xen-devel@lists.xen.org>
> Subject: Re: [Xen-devel] [PATCH 2/4] hvm/dmop: Implement copy_{to,
> from}_guest_buf() in terms of raw accessors
> 
> >>> On 21.04.17 at 13:03, <Jennifer.Herbert@citrix.com> wrote:
> 
> >
> > On 21/04/17 11:38, Jan Beulich wrote:
> >>>>> On 21.04.17 at 12:11, <Jennifer.Herbert@citrix.com> wrote:
> >>> On 21/04/17 10:46, Jan Beulich wrote:
> >>>>>>> On 21.04.17 at 11:11, <andrew.cooper3@citrix.com> wrote:
> >>>>> On 21/04/2017 09:54, Andrew Cooper wrote:
> >>>>>> On 21/04/2017 08:27, Jan Beulich wrote:
> >>>>>>>>>> On 20.04.17 at 19:59, <jennifer.herbert@citrix.com> wrote:
> >>>>>>>> From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>>>>>> Is this correct, considering that iirc the patch was new in v5 and ...
> >>>>>>>
> >>>>>>>> This also allows the usual cases to be simplified, by omitting an
> >>>>>>>> unnecessary
> >>>>>>>> buf parameters, and because the macros can appropriately size
> the object.
> >>>>>>>>
> >>>>>>>> This makes copying to or from a buf that isn't big enough an error.
> >>>>>>>> If the buffer isnt big enough, trying to carry on regardless
> >>>>>>>> can only cause trouble later on.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>>>>>> Signed-off-by: Jennifer Herbert <Jennifer.Herbert@citrix.com>
> >>>>>>> ... this sequence of S-o-b-s?
> >>>>>>>
> >>>>>>>> --- a/xen/arch/x86/hvm/dm.c
> >>>>>>>> +++ b/xen/arch/x86/hvm/dm.c
> >>>>>>>> @@ -32,36 +32,47 @@ struct dmop_args {
> >>>>>>>>        struct xen_dm_op_buf buf[2];
> >>>>>>>>    };
> >>>>>>>>
> >>>>>>>> -static bool copy_buf_from_guest(const xen_dm_op_buf_t
> bufs[],
> >>>>>>>> -                                unsigned int nr_bufs, void *dst,
> >>>>>>>> -                                unsigned int idx, size_t dst_size)
> >>>>>>>> +static bool _raw_copy_from_guest_buf(void *dst,
> >>>>>>>> +                                     const struct dmop_args *args,
> >>>>>>>> +                                     unsigned int buf_idx,
> >>>>>>>> +                                     size_t dst_bytes)
> >>>>>>>>    {
> >>>>>>>> -    size_t size;
> >>>>>>>> +    size_t buf_bytes;
> >>>>>>>>
> >>>>>>>> -    if ( idx >= nr_bufs )
> >>>>>>>> +    if ( buf_idx >= args->nr_bufs )
> >>>>>>>>            return false;
> >>>>>>>>
> >>>>>>>> -    memset(dst, 0, dst_size);
> >>>>>>>> +    buf_bytes =  args->buf[buf_idx].size;
> >>>>>>>>
> >>>>>>>> -    size = min_t(size_t, dst_size, bufs[idx].size);
> >>>>>>>> +    if ( dst_bytes > buf_bytes )
> >>>>>>>> +        return false;
> >>>>>>> While this behavioral change is now being mentioned in the
> >>>>>>> description, I'm not sure I buy the argument of basically being
> >>>>>>> guaranteed to cause trouble down the road. Did you consider the
> >>>>>>> forward compatibility aspect here, allowing us to extend interface
> >>>>>>> structures by adding fields to their ends without breaking old
> >>>>>>> callers? Paul, what are your thoughts here?
> >>>>>> DMOP is a stable ABI.  There is no legal extending of any objects.
> >>>>>>
> >>>>>> The previous semantics are guaranteed to break the ABI with future
> >>>>>> subops, which is why I removed it.  In the case that the guest
> >>>>>> originally passed an overly long buffer, and someone tried be
> "clever"
> >>>>>> here passing the same object here expecting
> _raw_copy_from_guest_buf()
> >>>>>> to DTRT, the function will end up copying too much data from the
> guest,
> >>>>>> and you will end up with something the guest wasn't intending to be
> part
> >>>>>> of the structure replacing the zero extension.
> >>>>>>
> >>>>>> New subops which take a longer object should use a brand new
> object.
> >>>>>>
> >>>>>> $MAGIC compatibility logic like you want has no business living in the
> >>>>>> copy helper.  Had I spotted the intention during the original dmop
> >>>>>> series, I would have rejected it during review.
> >>>>> Actually, the existing behaviour is already broken.  If a guest passes
> >>>>> an overly short buf[0], the dmop logic won't get a failure, and instead
> >>>>> get a truncated structure which has been zero extended.
> >>>>>
> >>>>> This is very definitely the wrong thing to do, because such a truncated
> >>>>> structure might actually contain some legitimate operations.
> >>>> So what, if that's what the caller meant to happen?
> >>>>
> >>>> Considering this is a controversial change, I think it is a bad idea
> >>>> to merge this into the otherwise uncontroversial change here.
> >>> How about we move the min(..) and memset out of the helper function,
> and
> >>> into dm_op?
> >> Wouldn't that result in different buffers being treated differently,
> >> which I'd rather not see happening?
> >
> > I think in the general case, its wrong to assume that its ok to truncate
> > a buffer.
> > In specific cases, such as buf[0], we don't really know how big the buf
> > needs to be until we start parsing it.
> 
> How that? Just like with domctl and sysctl the caller is supposed to
> be handing a full struct xen_dm_op, so I'd rather view this as the
> specific case where zero-extension is of no use.
> 
> > Certainly when we get to updating the accesser helper to deal with
> > offsets, its get more confused.  If we give it an offset greater then
> > the size of the buffer, do we just memset?  That would seem wrong.  With
> > an offset of 0, we want it to do the memset thing, to deal with buf[0].
> 
> Why? It's the natural extension of the zero-extending model.
> 
> > But what about if we have an offset.  I'd think the calling function
> > would really want to know if its got nonsense.
> 
> Zero-extended input is not nonsense to me.
> 
> > Another solution would be to have two variants of the helper function. A
> > 'normal' one, and the best effort one.
> 
> I don't think we want that. Andrew being wholeheartedly opposed
> to this zero-extension approach, he wouldn't agree to that either
> afaict. Considering that I don't expect him to change his mind, I
> guess I'll give up opposition to the change done, provided it is
> being done as a separate patch. Of course I'd be interested to
> know Paul's position here, as back then he appeared to agree
> with using this model (or maybe it also was just giving in)...
> 

I've lost track of all the various arguments but my position is that zero extending makes no sense; the caller must always supply sufficient data. The length check in the helper function should not be an identity check though since not all the data from a particular buffer may be required in one go. Thus the patch, with the macro changes, should be ok.

  Paul

> Jan

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