[PATCH v8 for-4.9 1/5] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[PATCH v8 for-4.9 1/5] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.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>
---
No change.
---
 xen/arch/x86/hvm/dm.c | 49 +++++++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd..e583e41 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)
@@ -56,7 +63,7 @@ static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
 }
 
 static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
-                            unsigned int nr, struct xen_dm_op_buf *buf)
+                            unsigned int nr, const struct xen_dm_op_buf *buf)
 {
     if ( nr > (GB(1) >> PAGE_SHIFT) )
         return -EINVAL;
@@ -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(const 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 v8 for-4.9 2/5] hvm/dmop: Make copy_buf_{from, to}_guest for a buffer not big enough an error.

[jennifer.herbert]

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

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: 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>
---
Switch buf_bytes to {dst, src}_bytes for copy_{from,to}_guest
---
 xen/arch/x86/hvm/dm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index e583e41..89186d2 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -36,30 +36,32 @@ 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)
 {
-    size_t size;
+    size_t buf_bytes;
 
     if ( idx >= nr_bufs )
         return false;
 
-    memset(dst, 0, dst_size);
-
-    size = min_t(size_t, dst_size, bufs[idx].size);
+    buf_bytes = bufs[idx].size;
+    if ( dst_size > buf_bytes )
+        return false;
 
-    return !copy_from_guest(dst, bufs[idx].h, size);
+    return !copy_from_guest(dst, bufs[idx].h, dst_size);
 }
 
 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)
 {
-    size_t size;
+    size_t buf_bytes;
 
     if ( idx >= nr_bufs )
         return false;
 
-    size = min_t(size_t, bufs[idx].size, src_size);
+    buf_bytes = bufs[idx].size;
+    if ( src_size > buf_bytes )
+        return false;
 
-    return !copy_to_guest(bufs[idx].h, src, size);
+    return !copy_to_guest(bufs[idx].h, src, src_size);
 }
 
 static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
-- 
2.1.4


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

[PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[jennifer.herbert]

From: Andrew Cooper <andrew.cooper3@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.

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>
---
Rebased.
---
 xen/arch/x86/hvm/dm.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index 89186d2..b31c252 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -32,38 +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 buf_bytes;
 
-    if ( idx >= nr_bufs )
+    if ( buf_idx >= args->nr_bufs )
         return false;
 
-    buf_bytes = bufs[idx].size;
-    if ( dst_size > buf_bytes )
+    buf_bytes =  args->buf[buf_idx].size;
+
+    if ( dst_bytes > buf_bytes )
         return false;
 
-    return !copy_from_guest(dst, bufs[idx].h, dst_size);
+    return !copy_from_guest(dst, args->buf[buf_idx].h, dst_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(const struct dmop_args *args,
+                                   unsigned int buf_idx,
+                                   const void *src, size_t src_bytes)
 {
     size_t buf_bytes;
 
-    if ( idx >= nr_bufs )
+    if ( buf_idx >= args->nr_bufs )
         return false;
 
-    buf_bytes = bufs[idx].size;
-    if ( src_size > buf_bytes )
+    buf_bytes = args->buf[buf_idx].size;
+
+    if ( src_bytes > buf_bytes )
         return false;
 
-    return !copy_to_guest(bufs[idx].h, src, src_size);
+    return !copy_to_guest(args->buf[buf_idx].h, 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_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, const struct xen_dm_op_buf *buf)
 {
@@ -314,7 +323,7 @@ static int dm_op(const 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;
@@ -570,8 +579,8 @@ static int dm_op(const 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 v8 for-4.9 4/5] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers

[jennifer.herbert]

From: Andrew Cooper <andrew.cooper3@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>
Reviewed-by: Jan Beulich <jbeulich@suse.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>

--
Rebased
---
 xen/arch/x86/hvm/dm.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index b31c252..c91895f 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,19 @@ static bool _raw_copy_from_guest_buf(void *dst,
 
     buf_bytes =  args->buf[buf_idx].size;
 
-    if ( dst_bytes > buf_bytes )
+    if ( (offset_bytes + dst_bytes) < offset_bytes ||
+         (offset_bytes + dst_bytes) > buf_bytes )
         return false;
 
-    return !copy_from_guest(dst, args->buf[buf_idx].h, dst_bytes);
+    return !copy_from_guest_offset(dst, args->buf[buf_idx].h,
+                                   offset_bytes, dst_bytes);
 }
 
-static bool _raw_copy_to_guest_buf(const struct dmop_args *args,
-                                   unsigned int buf_idx,
-                                   const void *src, size_t src_bytes)
+static bool _raw_copy_to_guest_buf_offset(const 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 +66,28 @@ static bool _raw_copy_to_guest_buf(const struct dmop_args *args,
 
     buf_bytes = args->buf[buf_idx].size;
 
-    if ( src_bytes > buf_bytes )
+
+    if ( (offset_bytes + src_bytes) < offset_bytes ||
+         (offset_bytes + src_bytes) > buf_bytes )
         return false;
 
-    return !copy_to_guest(args->buf[buf_idx].h, src, src_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, const 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 v8 for-4.9 5/5] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
--
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

--
No change
---
 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 c91895f..b1f547b 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -153,56 +153,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)
+                           const 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)
@@ -539,13 +585,8 @@ static int dm_op(const 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 v8 for-4.9 2/5] hvm/dmop: Make copy_buf_{from, to}_guest for a buffer not big enough an error.

[Paul Durrant]

> -----Original Message-----
> From: jennifer.herbert@citrix.com [mailto:jennifer.herbert@citrix.com]
> Sent: 21 April 2017 15:06
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Jennifer Herbert <jennifer.herbert@citrix.com>; Jennifer Herbert
> <jennifer.herbert@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH v8 for-4.9 2/5] hvm/dmop: Make copy_buf_{from,
> to}_guest for a buffer not big enough an error.
> 
> From: Jennifer Herbert <jennifer.herbert@citrix.com>
> 
> 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: 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>
> ---
> Switch buf_bytes to {dst, src}_bytes for copy_{from,to}_guest
> ---
>  xen/arch/x86/hvm/dm.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index e583e41..89186d2 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -36,30 +36,32 @@ 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)
>  {
> -    size_t size;
> +    size_t buf_bytes;
> 
>      if ( idx >= nr_bufs )
>          return false;
> 
> -    memset(dst, 0, dst_size);
> -
> -    size = min_t(size_t, dst_size, bufs[idx].size);
> +    buf_bytes = bufs[idx].size;
> +    if ( dst_size > buf_bytes )
> +        return false;
> 
> -    return !copy_from_guest(dst, bufs[idx].h, size);
> +    return !copy_from_guest(dst, bufs[idx].h, dst_size);
>  }
> 
>  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)
>  {
> -    size_t size;
> +    size_t buf_bytes;
> 
>      if ( idx >= nr_bufs )
>          return false;
> 
> -    size = min_t(size_t, bufs[idx].size, src_size);
> +    buf_bytes = bufs[idx].size;
> +    if ( src_size > buf_bytes )
> +        return false;
> 
> -    return !copy_to_guest(bufs[idx].h, src, size);
> +    return !copy_to_guest(bufs[idx].h, src, src_size);
>  }
> 
>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
> --
> 2.1.4


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

Re: [PATCH v8 for-4.9 3/5] 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: 21 April 2017 15:06
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jennifer Herbert
> <jennifer.herbert@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan
> Beulich <JBeulich@suse.com>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to,
> from}_guest_buf() in terms of raw accessors
> 
> From: Andrew Cooper <andrew.cooper3@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.
> 
> 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>
> ---
> Rebased.
> ---
>  xen/arch/x86/hvm/dm.c | 43 ++++++++++++++++++++++++++---------------
> --
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index 89186d2..b31c252 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -32,38 +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 buf_bytes;
> 
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
> 
> -    buf_bytes = bufs[idx].size;
> -    if ( dst_size > buf_bytes )
> +    buf_bytes =  args->buf[buf_idx].size;
> +
> +    if ( dst_bytes > buf_bytes )
>          return false;
> 
> -    return !copy_from_guest(dst, bufs[idx].h, dst_size);
> +    return !copy_from_guest(dst, args->buf[buf_idx].h, dst_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(const struct dmop_args *args,
> +                                   unsigned int buf_idx,
> +                                   const void *src, size_t src_bytes)
>  {
>      size_t buf_bytes;
> 
> -    if ( idx >= nr_bufs )
> +    if ( buf_idx >= args->nr_bufs )
>          return false;
> 
> -    buf_bytes = bufs[idx].size;
> -    if ( src_size > buf_bytes )
> +    buf_bytes = args->buf[buf_idx].size;
> +
> +    if ( src_bytes > buf_bytes )
>          return false;
> 
> -    return !copy_to_guest(bufs[idx].h, src, src_size);
> +    return !copy_to_guest(args->buf[buf_idx].h, 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_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, const struct xen_dm_op_buf *buf)
>  {
> @@ -314,7 +323,7 @@ static int dm_op(const 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;
> @@ -570,8 +579,8 @@ static int dm_op(const 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 v8 for-4.9 1/5] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Julien Grall]

Hi Jennifer,

I don't see any cover letter for this series, so I will answer here.

Looking at the code, it looks like a new feature rather than a bug fix. 
Am I right?

Could you explain what would be the benefits and risks to get this code 
in the release?

I also like to hear the opinion of the x86 maintainers about getting 
this code in Xen 4.9.

Cheers,

On 21/04/17 15:05, 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.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>
> ---
> No change.
> ---
>  xen/arch/x86/hvm/dm.c | 49 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d72b7bd..e583e41 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)
> @@ -56,7 +63,7 @@ static bool copy_buf_to_guest(const xen_dm_op_buf_t bufs[],
>  }
>
>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
> -                            unsigned int nr, struct xen_dm_op_buf *buf)
> +                            unsigned int nr, const struct xen_dm_op_buf *buf)
>  {
>      if ( nr > (GB(1) >> PAGE_SHIFT) )
>          return -EINVAL;
> @@ -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(const 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",
>

-- 
Julien Grall

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

Re: [PATCH v8 for-4.9 1/5] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Andrew Cooper]

On 21/04/17 15:17, Julien Grall wrote:
> Hi Jennifer,
>
> I don't see any cover letter for this series, so I will answer here.
>
> Looking at the code, it looks like a new feature rather than a bug
> fix. Am I right?
>
> Could you explain what would be the benefits and risks to get this
> code in the release?
>
> I also like to hear the opinion of the x86 maintainers about getting
> this code in Xen 4.9.

Patch 1 is a bug in the existing implementation, which absolutely needs
fixing.

Patch 4 it is a correction to the DM OP ABI (which is still modifiable,
before becoming properly stable when 4.9 releases).  If that were
pospond to 4.10, We'd have to burn the existing modified_memory subop
and introduce a new corrected one.

The intermediate patches are fallout from previous rounds of review.


One item on my TODO list is to re-review all dmops for proper
continuability before 4.9 release, so I make no promises that there wont
be further bugfixes needing to get into 4.9.

~Andrew

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

Re: [PATCH v8 for-4.9 1/5] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Jennifer Herbert]

Hi Julien,

This is extending an existing feature.
Once 4.9 is released, the existing feature will be frozen, and the only 
way to later get the
extra functionality would be to created a completely new dm_op, which 
does something very similar
to an existing one.  Although not the end of the world, this wouldnt 
look so nice.

The benefits of the feature are that a VM can request multiple extents 
to be marked as modified at once,
without having to loop though them, calling the existing call many many 
times.  This will be more efficient and faster.
As an extra, additional accessors have been created for dm_op 
operations, which new dm_ops can take advantage of.

The benefits of introducing the feature for 4.9 as opposed to later is 
that we wont' have to support the same feature, with multiple dm_opts 
with varying parameters - which as well as looking less good, also 
unnesseserily bloats the code.

I think risks are low, with a minor, affecting dm_op operations only.  
The core change, in 5/5 will only affect the modified memory call, which 
has been tested.  The remaining patches are to tidy up and fix existing 
behaviour.

-jenny

On 21/04/17 15:17, Julien Grall wrote:
> Hi Jennifer,
>
> I don't see any cover letter for this series, so I will answer here.
>
> Looking at the code, it looks like a new feature rather than a bug 
> fix. Am I right?
>
> Could you explain what would be the benefits and risks to get this 
> code in the release?
>
> I also like to hear the opinion of the x86 maintainers about getting 
> this code in Xen 4.9.
>
> Cheers,
>
> On 21/04/17 15:05, 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>
>> Reviewed-by: Jan Beulich <jbeulich@suse.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>
>> ---
>> No change.
>> ---
>>  xen/arch/x86/hvm/dm.c | 49 
>> +++++++++++++++++++++++++++++--------------------
>>  1 file changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>> index d72b7bd..e583e41 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)
>> @@ -56,7 +63,7 @@ static bool copy_buf_to_guest(const xen_dm_op_buf_t 
>> bufs[],
>>  }
>>
>>  static int track_dirty_vram(struct domain *d, xen_pfn_t first_pfn,
>> -                            unsigned int nr, struct xen_dm_op_buf *buf)
>> +                            unsigned int nr, const struct 
>> xen_dm_op_buf *buf)
>>  {
>>      if ( nr > (GB(1) >> PAGE_SHIFT) )
>>          return -EINVAL;
>> @@ -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(const 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",
>>
>


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

Re: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
> +#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))

Why all caps all of the sudden? With this undone
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 v8 for-4.9 4/5] hvm/dmop: Implement copy_{to, from}_guest_buf_offset() helpers

[Jan Beulich]

>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
> From: Andrew Cooper <andrew.cooper3@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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Same thing here regarding the use of capitals in the macro names,
i.e. with that undone
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 v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Andrew Cooper]

On 21/04/17 16:45, Jan Beulich wrote:
>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
>> +#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))
> Why all caps all of the sudden?

This is the start of some code improvements, given the fallout from XSA-212.

This macro is not a C function and doesn't behave like one
(specifically, capturing src by name rather than value).  Therefore, it
gets ALL_CAPS() (which is actually traditional for any macro in C) to
make it more obvious to people reading the code that it *is not* a C
function and doesn't behave like one.

It is getting embarrassing how many security vulnerability we are seeing
because macros look like they are doing one thing, yet actually do
something else, and improving the quality of the code is the only way
this is going to get better.

Therefore, I am going to insist that they stay like this.

~Andrew

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

Re: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 18:10, <andrew.cooper3@citrix.com> wrote:
> On 21/04/17 16:45, Jan Beulich wrote:
>>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
>>> +#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))

(Side note: src also isn't properly parenthesized, and the title went
out of sync with the implementation.)

>> Why all caps all of the sudden?
> 
> This is the start of some code improvements, given the fallout from XSA-212.

I don't think making the names shout is an improvement in any way.
The #define-s above may still look fine, but the code using them is
now looking plain ugly (even more so with the yet longer names
introduced in patch 4).

> This macro is not a C function and doesn't behave like one
> (specifically, capturing src by name rather than value).  Therefore, it
> gets ALL_CAPS() (which is actually traditional for any macro in C) to

I disagree - manifest constants may indeed be traditionally all
caps, but not macros. Just look in the Library section of the C
standard. In various cases it's not even enforced whether a
given identifier is a macro or a variable/function.

> make it more obvious to people reading the code that it *is not* a C
> function and doesn't behave like one.
> 
> It is getting embarrassing how many security vulnerability we are seeing
> because macros look like they are doing one thing, yet actually do
> something else, and improving the quality of the code is the only way
> this is going to get better.

Considering the "how many" you use, mind giving three examples
where using all caps macro names would have made a difference?
I sincerely doubt that the case used in identifiers would make a
whole lot of a difference.

> Therefore, I am going to insist that they stay like this.

Well, how about I insist on names being made consistent again
with what we have elsewhere, as they were in earlier versions?
My conditional R-b stands in any event (plus the correction of
the parenthesization issue mentioned above, also for patch 4).

As a possible alternative, was it considered to pass pointers
here as before, using __builtin_object_size() on them instead of
the sizeof() above, and making the macros inline functions?

Jan


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

Re: [PATCH v8 for-4.9 1/5] hvm/dmop: Box dmop_args rather than passing multiple parameters around

[Julien Grall]

On 21/04/17 15:44, Jennifer Herbert wrote:
> Hi Julien,

Hello Jenny,

> This is extending an existing feature.
> Once 4.9 is released, the existing feature will be frozen, and the only
> way to later get the
> extra functionality would be to created a completely new dm_op, which
> does something very similar
> to an existing one.  Although not the end of the world, this wouldnt
> look so nice.
>
> The benefits of the feature are that a VM can request multiple extents
> to be marked as modified at once,
> without having to loop though them, calling the existing call many many
> times.  This will be more efficient and faster.
> As an extra, additional accessors have been created for dm_op
> operations, which new dm_ops can take advantage of.
>
> The benefits of introducing the feature for 4.9 as opposed to later is
> that we wont' have to support the same feature, with multiple dm_opts
> with varying parameters - which as well as looking less good, also
> unnesseserily bloats the code.
>
> I think risks are low, with a minor, affecting dm_op operations only.
> The core change, in 5/5 will only affect the modified memory call, which
> has been tested.  The remaining patches are to tidy up and fix existing
> behaviour.

It would have been useful to have a cover letter explaining that.

Anyway, I think I agree it would be better to get the DM OP ABI in shape 
for continuability before it gets stable. Although, it would be nice if 
we can get that done in early RCs.

Release-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Andrew Cooper]

On 24/04/17 09:19, Jan Beulich wrote:
>>>> On 21.04.17 at 18:10, <andrew.cooper3@citrix.com> wrote:
>> On 21/04/17 16:45, Jan Beulich wrote:
>>>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
>>>> +#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))
> (Side note: src also isn't properly parenthesized, and the title went
> out of sync with the implementation.)
>
>>> Why all caps all of the sudden?
>> This is the start of some code improvements, given the fallout from XSA-212.
> I don't think making the names shout is an improvement in any way.
> The #define-s above may still look fine, but the code using them is
> now looking plain ugly (even more so with the yet longer names
> introduced in patch 4).

That is a matter of opinion which I don't share, but ok.

As an alternative, how else do you suggest making it obvious to the
reader of the code that this thing which looks like a function doesn't
have function semantics?  (This is the purpose I am trying to get across.)

>> make it more obvious to people reading the code that it *is not* a C
>> function and doesn't behave like one.
>>
>> It is getting embarrassing how many security vulnerability we are seeing
>> because macros look like they are doing one thing, yet actually do
>> something else, and improving the quality of the code is the only way
>> this is going to get better.
> Considering the "how many" you use, mind giving three examples
> where using all caps macro names would have made a difference?
> I sincerely doubt that the case used in identifiers would make a
> whole lot of a difference.

You have missed my point then.  We have many security vulnerabilities
because we have deceptive code, and fix for that is to prevent the code
being deceptive.  This is going to positive code quality effort on our
behalf, because the status quo is currently terrible.

Most notably, XSA-212 just gone, where the root of the vulnerability is
that "guest_handle_okay(base_ptr, array_element)" doesn't consider its
second parameter, and degrades to checking just base_ptr.

I accept that, in this case, capitalising the macro wouldn't help, but
that is because its deceptive nature is in its naming, not because it
behaves in a way contrary to a C function.

> As a possible alternative, was it considered to pass pointers
> here as before, using __builtin_object_size() on them instead of
> the sizeof() above, and making the macros inline functions?

I have never tried using it in anger, but looking into it, it degrades
to (size_t)-1 in the case the compiler can't statically work out the
correct value.  As a result, you'd end up with a function which has
gets() semantics (in the case that the compiler can't work out what's
going on).  I don't recommend we use any constructs like this.

~Andrew

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

Re: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 25.04.17 at 22:03, <andrew.cooper3@citrix.com> wrote:
> On 24/04/17 09:19, Jan Beulich wrote:
>>>>> On 21.04.17 at 18:10, <andrew.cooper3@citrix.com> wrote:
>>> On 21/04/17 16:45, Jan Beulich wrote:
>>>>>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
>>>>> +#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))
>> (Side note: src also isn't properly parenthesized, and the title went
>> out of sync with the implementation.)
>>
>>>> Why all caps all of the sudden?
>>> This is the start of some code improvements, given the fallout from XSA-212.
>> I don't think making the names shout is an improvement in any way.
>> The #define-s above may still look fine, but the code using them is
>> now looking plain ugly (even more so with the yet longer names
>> introduced in patch 4).
> 
> That is a matter of opinion which I don't share, but ok.
> 
> As an alternative, how else do you suggest making it obvious to the
> reader of the code that this thing which looks like a function doesn't
> have function semantics?  (This is the purpose I am trying to get across.)

I do understand the intention, but I don't think capitalization of the
names helps this in any way. This is not the least because there are
compiler builtins which don't really have function semantics either
(e.g. __builtin_constant_p(), __builtin_expect(),
__builtin_types_compatible_p(), or __builtin_unreachable()). You
won't be able to do anything about these, other than perhaps
encapsulating them wrapper macros, but that still won't make the
compiler mandated name all caps.

Or take offsetof(), which following your argumentation then should
be - contrary to the C standard - OFFSETOF(), while what we have
as ASSERT() should then be - in line with the C standard, albeit our
macro has slightly different behavior - assert().

>>> make it more obvious to people reading the code that it *is not* a C
>>> function and doesn't behave like one.
>>>
>>> It is getting embarrassing how many security vulnerability we are seeing
>>> because macros look like they are doing one thing, yet actually do
>>> something else, and improving the quality of the code is the only way
>>> this is going to get better.
>> Considering the "how many" you use, mind giving three examples
>> where using all caps macro names would have made a difference?
>> I sincerely doubt that the case used in identifiers would make a
>> whole lot of a difference.
> 
> You have missed my point then.  We have many security vulnerabilities
> because we have deceptive code, and fix for that is to prevent the code
> being deceptive.  This is going to positive code quality effort on our
> behalf, because the status quo is currently terrible.
> 
> Most notably, XSA-212 just gone, where the root of the vulnerability is
> that "guest_handle_okay(base_ptr, array_element)" doesn't consider its
> second parameter, and degrades to checking just base_ptr.
> 
> I accept that, in this case, capitalising the macro wouldn't help, but
> that is because its deceptive nature is in its naming, not because it
> behaves in a way contrary to a C function.

A function could easily (and - by default - without warning) ignore
one or more of the arguments passed to it, too.

And as you say - XSA-212 is not an suitable example for your
argumentation here. Once again - I don't think name capitalization
helps in any way in what you want to ensure. And no, I don't think
I've missed your point here, I'm merely questioning the measure
you want to take to address your concerns. Therefore I'm still
lacking any examples of XSAs supporting your position regarding
identifier naming.

>> As a possible alternative, was it considered to pass pointers
>> here as before, using __builtin_object_size() on them instead of
>> the sizeof() above, and making the macros inline functions?
> 
> I have never tried using it in anger, but looking into it, it degrades
> to (size_t)-1 in the case the compiler can't statically work out the
> correct value.  As a result, you'd end up with a function which has
> gets() semantics (in the case that the compiler can't work out what's
> going on).  I don't recommend we use any constructs like this.

Well, I wouldn't rule it out without a little bit of experimenting.
While it can't be used in BUILD_BUG_ON(), it can very well be used
in ways similar to e.g. {read,write}_atomic(), adding a reference
to an always undefined symbol in the case the builtin produces 0
or -1.

On the grounds of wanting to not delay this series any further,
I'm about to commit it (albeit I note patch 2 is lacking a proper
maintainer ack, and I'm not going to add mine), but I'll make my
disagreement with the choice of macro names explicit, so these
names can't later be used as a reference to support naming
other macros in similarly ugly ways.

Jan

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

Re: [PATCH v8 for-4.9 3/5] hvm/dmop: Implement copy_{to, from}_guest_buf() in terms of raw accessors

[Jan Beulich]

>>> On 21.04.17 at 16:05, <jennifer.herbert@citrix.com> wrote:
> @@ -314,7 +323,7 @@ static int dm_op(const 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) );

Btw, I also had to remove the bogus semicolon here, causing a build
failure, and ...

> @@ -570,8 +579,8 @@ static int dm_op(const 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;

... I've taken the liberty to remove this stray blank line.

Both are likely a sign of this having been put together in too much
of a hurry, and the former suggests this version hasn't been tested
at all.

Jan


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