[PATCH v2 0/2] qemu-qdisk: Implementation of grant copy operation.

[PATCH v2 0/2] qemu-qdisk: Implementation of grant copy operation.

[Paulina Szubarczyk]

Hi,

It is a proposition for implementation of grant copy operation in qemu-qdisk and 
interface in libxc/libs. 

Changes since v1:
Interface:
- changed the interface to call grant copy operation to match ioctl
	int xengnttab_grant_copy(xengnttab_handle *xgt,
                         	 uint32_t count,
                         	 xengnttab_grant_copy_segment_t* segs)

- added a struct 'xengnttab_copy_grant_segment' definition to tools/libs	
  /gnttab/private.h, tools/libxc/include/xenctrl_compat.h

- changed the function 'osdep_gnttab_grant_copy' which right now just
  call the ioctl

- added a new VER1.1 to tools/libs/gnttab/libxengnttab.map 

qemu-qdisk:
- removed the 'ioreq_write','ioreq_read_init','ioreq_read' functions 
- implemented 'ioreq_init_copy_buffers', 'ioreq_copy' 
- reverted the removal of grant map and introduced conditional invoking
  grant copy or grant map
- resigned from caching the local buffers on behalf of allocating the 
  required amount of pages at once. The cached structure would require 
  to have an lock guard and I suppose that the performance improvement 
  would degraded. 

For the functional test I attached the device with a qdisk backend to the guest, 
mounted, performed some reads and writes.

I run fio tests[1] with different iodepth and size of the block. The test can be 
accessed on my github[2] but mainly after the warm up I run for 60 seconds:
    fio --time_based \
		--clocksource=clock_gettime \
		--rw=randread \
		--random_distribution=pareto:0.9 \
		--size=10g \
	    --direct='1' \
	    --ioengine=libaio \
		--filename=$DEV \
		--iodepth=$IODEPTH \
		--bs=$BS \
		--name=$NAME \
		--runtime=$RUNTIME >> $FILENAME
The test were repeated at least three times. 

[1] https://docs.google.com/spreadsheets/d/1E6AMiB8ceJpExL6jWpH9u2yy6DZxzhmDUyFf-eUuJ0c/edit?usp=sharing

[2] https://github.com/paulina-szubarczyk/xen-benchmark
    - multitest_with_iodepth.sh


Thanks and regards, 
Paulina

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

[PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Paulina Szubarczyk]

Implentation of interface for grant copy operation in libs and
libxc.

In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
system call is invoked. In mini-os the operation is yet not
implemented. For other OSs there is a dummy implementation.

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>

---
Changes since v1:
- changed the interface to call grant copy operation to match ioctl
  int xengnttab_grant_copy(xengnttab_handle *xgt,
                           uint32_t count,
                           xengnttab_grant_copy_segment_t* segs)
- added a struct 'xengnttab_copy_grant_segment' definition to tools/libs  
  /gnttab/private.h, tools/libxc/include/xenctrl_compat.h
- changed the function osdep_gnttab_grant_copy which right now just call
  the ioctl
- added a new VER1.1 to tools/libs/gnttab/libxengnttab.map 

 tools/include/xen-sys/Linux/gntdev.h  | 21 +++++++++++++++++++++
 tools/libs/gnttab/gnttab_core.c       |  6 ++++++
 tools/libs/gnttab/gnttab_unimp.c      |  6 ++++++
 tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++
 tools/libs/gnttab/libxengnttab.map    |  4 ++++
 tools/libs/gnttab/linux.c             | 18 ++++++++++++++++++
 tools/libs/gnttab/minios.c            |  6 ++++++
 tools/libs/gnttab/private.h           | 18 ++++++++++++++++++
 tools/libxc/include/xenctrl_compat.h  | 23 +++++++++++++++++++++++
 tools/libxc/xc_gnttab_compat.c        |  7 +++++++
 10 files changed, 123 insertions(+)

diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
index caf6fb4..0ca07c9 100644
--- a/tools/include/xen-sys/Linux/gntdev.h
+++ b/tools/include/xen-sys/Linux/gntdev.h
@@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
 /* Send an interrupt on the indicated event channel */
 #define UNMAP_NOTIFY_SEND_EVENT 0x2
 
+struct ioctl_gntdev_grant_copy_segment {
+    union {
+        void *virt;
+        struct {
+            uint32_t ref;
+            uint16_t offset;
+            uint16_t domid;
+        } foreign;
+    } source, dest;
+    uint16_t len;
+    uint16_t flags;
+    int16_t status;
+};
+
+#define IOCTL_GNTDEV_GRANT_COPY \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
+struct ioctl_gntdev_grant_copy {
+    unsigned int count;
+    struct ioctl_gntdev_grant_copy_segment *segments;
+};
+
 #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 5d0474d..9badc58 100644
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
     return osdep_gnttab_unmap(xgt, start_address, count);
 }
 
+int xengnttab_grant_copy(xengnttab_handle *xgt,
+                         uint32_t count,
+                         xengnttab_grant_copy_segment_t* segs)
+{
+    return osdep_gnttab_grant_copy(xgt, count, segs);
+}
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c
index b3a4a20..79a5c88 100644
--- a/tools/libs/gnttab/gnttab_unimp.c
+++ b/tools/libs/gnttab/gnttab_unimp.c
@@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
     abort();
 }
 
+int xengnttab_copy_grant(xengnttab_handle *xgt,
+                         uint32_t count,
+                         xengnttab_copy_grant_segment_t* segs)
+{
+    return -1;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/gnttab/include/xengnttab.h b/tools/libs/gnttab/include/xengnttab.h
index 0431dcf..a9fa1fe 100644
--- a/tools/libs/gnttab/include/xengnttab.h
+++ b/tools/libs/gnttab/include/xengnttab.h
@@ -258,6 +258,20 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count);
 int xengnttab_set_max_grants(xengnttab_handle *xgt,
                              uint32_t nr_grants);
 
+typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
+
+/**
+ * Copy memory from or to grant references. The information of each operations
+ * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
+ * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
+ *
+ * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
+ * should not exceed XEN_PAGE_SIZE
+ */
+int xengnttab_grant_copy(xengnttab_handle *xgt,
+                         uint32_t count,
+                         xengnttab_grant_copy_segment_t* segs);
+
 /*
  * Grant Sharing Interface (allocating and granting pages to others)
  */
diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
index dc737ac..0928963 100644
--- a/tools/libs/gnttab/libxengnttab.map
+++ b/tools/libs/gnttab/libxengnttab.map
@@ -21,3 +21,7 @@ VERS_1.0 {
 		xengntshr_unshare;
 	local: *; /* Do not expose anything by default */
 };
+
+VERS_1.1 {
+		xengnttab_grant_copy;
+} VERS_1.0;
\ No newline at end of file
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 7b0fba4..26bfbdc 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
     return 0;
 }
 
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+                            uint32_t count,
+                            xengnttab_grant_copy_segment_t* segs)
+{
+    int fd = xgt->fd;
+    struct ioctl_gntdev_grant_copy copy;
+
+    copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs;
+    copy.count = count;
+
+    if (ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, &copy)) {
+        GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
+        return -1;
+    }
+
+    return 0;
+}
+
 int osdep_gntshr_open(xengntshr_handle *xgs)
 {
     int fd = open(DEVXEN "gntalloc", O_RDWR);
diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
index 7e04174..93e4f89 100644
--- a/tools/libs/gnttab/minios.c
+++ b/tools/libs/gnttab/minios.c
@@ -106,6 +106,12 @@ int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
     return ret;
 }
 
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+                            uint32_t count,
+                            xengnttab_grant_copy_segment_t* segs)
+{
+    return -1;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index d286c86..22ad53a 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -9,6 +9,20 @@ struct xengntdev_handle {
     int fd;
 };
 
+struct xengnttab_copy_grant_segment {
+    union xengnttab_copy_ptr {
+        void *virt;
+        struct {
+            uint32_t ref;
+            uint16_t offset;
+            uint16_t domid;
+        } foreign;
+    } source, dest;
+    uint16_t len;
+    uint16_t flags;
+    int16_t status;
+};
+
 int osdep_gnttab_open(xengnttab_handle *xgt);
 int osdep_gnttab_close(xengnttab_handle *xgt);
 
@@ -23,6 +37,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int osdep_gnttab_unmap(xengnttab_handle *xgt,
                        void *start_address,
                        uint32_t count);
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+                            uint32_t count,
+                            xengnttab_grant_copy_segment_t* segs);
+
 int osdep_gntshr_open(xengntshr_handle *xgs);
 int osdep_gntshr_close(xengntshr_handle *xgs);
 
diff --git a/tools/libxc/include/xenctrl_compat.h b/tools/libxc/include/xenctrl_compat.h
index 93ccadb..7bb24a4 100644
--- a/tools/libxc/include/xenctrl_compat.h
+++ b/tools/libxc/include/xenctrl_compat.h
@@ -105,6 +105,29 @@ int xc_gnttab_munmap(xc_gnttab *xcg,
 int xc_gnttab_set_max_grants(xc_gnttab *xcg,
                              uint32_t count);
 
+union xengnttab_grant_copy_ptr {
+    void *virt;
+    struct {
+        uint32_t ref;
+        uint16_t offset;
+        uint16_t domid;
+    } foreign;
+};
+
+struct xengnttab_grant_copy_segment {
+    union xengnttab_grant_copy_ptr source, dest;
+    uint16_t len;
+    uint16_t flags;
+    int16_t status;
+};
+
+typedef struct xengnttab_grant_copy_segment xc_gnttab_grant_copy_segment_t;
+typedef union xengnttab_grant_copy_ptr xc_gnttab_grant_copy_ptr_t;
+
+int xc_gnttab_grant_copy(xc_gnttab *xgt,
+                         uint32_t count,
+                         xc_gnttab_grant_copy_segment_t* segs);
+
 typedef struct xengntdev_handle xc_gntshr;
 
 xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
diff --git a/tools/libxc/xc_gnttab_compat.c b/tools/libxc/xc_gnttab_compat.c
index 6f036d8..c265bb0 100644
--- a/tools/libxc/xc_gnttab_compat.c
+++ b/tools/libxc/xc_gnttab_compat.c
@@ -69,6 +69,13 @@ int xc_gnttab_set_max_grants(xc_gnttab *xcg,
     return xengnttab_set_max_grants(xcg, count);
 }
 
+int xc_gnttab_grant_copy(xc_gnttab *xgt,
+                         uint32_t count,
+                         xc_gnttab_grant_copy_segment_t* segs)
+{
+    return xengnttab_grant_copy(xgt, count, segs);
+}
+
 xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
                           unsigned open_flags)
 {
-- 
1.9.1


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

[PATCH v2 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Paulina Szubarczyk]

Copy data operated on during request from/to local buffers to/from 
the grant references. 

Before grant copy operation local buffers must be allocated what is 
done by calling ioreq_init_copy_buffers. For the 'read' operation, 
first, the qemu device invokes the read operation on local buffers 
and on the completion grant copy is called and buffers are freed. 
For the 'write' operation grant copy is performed before invoking 
write by qemu device. 

A new value 'feature_grant_copy' is added to recognize when the 
grant copy operation is supported by a guest. 
The body of the function 'ioreq_runio_qemu_aio' is moved to 
'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
on the support for grant copy according checks, initialization, grant 
operation are made, then the 'ioreq_runio_qemu_aio_blk' function is 
called. 

Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v1:
- removed the 'ioreq_write','ioreq_read_init','ioreq_read' functions 
- implemented 'ioreq_init_copy_buffers', 'ioreq_copy' 
- reverted the removal of grant map and introduced conditional invoking
  grant copy or grant map
- resigned from caching the local buffers on behalf of allocating the 
  required amount of pages at once. The cached structure would require 
  to have an lock guard and I suppose that the performance improvement 
  would degraded. 

 hw/block/xen_disk.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 163 insertions(+), 12 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 37e14d1..af6b8c7 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -131,6 +131,9 @@ struct XenBlkDev {
     unsigned int        persistent_gnt_count;
     unsigned int        max_grants;
 
+    /* Grant copy */
+    gboolean            feature_grant_copy;
+
     /* qemu block driver */
     DriveInfo           *dinfo;
     BlockBackend        *blk;
@@ -500,6 +503,100 @@ static int ioreq_map(struct ioreq *ioreq)
     return 0;
 }
 
+static void* get_buffer(int count) 
+{
+    return xc_memalign(xen_xc, XC_PAGE_SIZE, count*XC_PAGE_SIZE);
+}
+
+static void free_buffers(struct ioreq *ioreq) 
+{
+    int i;
+
+    for (i = 0; i < ioreq->v.niov; i++) { 
+        ioreq->page[i] = NULL;
+    }
+
+    free(ioreq->pages);
+}
+
+static int ioreq_init_copy_buffers(struct ioreq *ioreq) {
+    int i;
+
+    if (ioreq->v.niov == 0) {
+        return 0;
+    }
+
+    ioreq->pages = get_buffer(ioreq->v.niov);
+    if (!ioreq->pages) { 
+        return -1; 
+    }
+
+    for (i = 0; i < ioreq->v.niov; i++) {
+        ioreq->page[i] = ioreq->pages + i*XC_PAGE_SIZE;
+        ioreq->v.iov[i].iov_base += (uintptr_t)ioreq->page[i];
+    }
+
+    return 0;
+}
+
+static int ioreq_copy(struct ioreq *ioreq) 
+{
+    XenGnttab gnt = ioreq->blkdev->xendev.gnttabdev;
+    xc_gnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+    int i, count = 0, r, rc;
+    int64_t file_blk = ioreq->blkdev->file_blk;
+
+    if (ioreq->v.niov == 0) {
+        r = 0; goto out;
+    }
+
+    count = ioreq->v.niov;
+
+    for (i = 0; i < count; i++) {
+
+        xc_gnttab_grant_copy_ptr_t *from, *to;
+
+        if (ioreq->req.operation == BLKIF_OP_READ) {
+            segs[i].flags = GNTCOPY_dest_gref;
+            from = &(segs[i].dest);
+            to = &(segs[i].source);
+        } else {
+            segs[i].flags = GNTCOPY_source_gref;
+            from = &(segs[i].source);
+            to = &(segs[i].dest);
+        }
+        segs[i].len = (ioreq->req.seg[i].last_sect 
+                       - ioreq->req.seg[i].first_sect + 1) * file_blk;
+        from->foreign.ref = ioreq->refs[i];
+        from->foreign.domid = ioreq->domids[i];
+        from->foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
+        to->virt = ioreq->v.iov[i].iov_base;
+    }
+
+    rc = xc_gnttab_grant_copy(gnt, count, segs);
+
+    if (rc) {
+        xen_be_printf(&ioreq->blkdev->xendev, 0, 
+                      "failed to copy data %d \n", rc);
+        ioreq->aio_errors++;
+        r = -1; goto out;
+    } else {
+        r = 0;
+    }
+
+    for (i = 0; i < count; i++) {
+        if (segs[i].status != GNTST_okay) {
+            xen_be_printf(&ioreq->blkdev->xendev, 0, 
+                          "failed to copy data %d for gref %d, domid %d\n", rc, 
+                          ioreq->refs[i], ioreq->domids[i]); 
+            ioreq->aio_errors++;
+            r = -1;
+        }
+    }
+out:
+    return r;
+}
+
 static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
 
 static void qemu_aio_complete(void *opaque, int ret)
@@ -521,6 +618,7 @@ static void qemu_aio_complete(void *opaque, int ret)
     if (ioreq->aio_inflight > 0) {
         return;
     }
+
     if (ioreq->postsync) {
         ioreq->postsync = 0;
         ioreq->aio_inflight++;
@@ -528,8 +626,32 @@ static void qemu_aio_complete(void *opaque, int ret)
         return;
     }
 
+    if (ioreq->blkdev->feature_grant_copy) {
+        switch (ioreq->req.operation) {
+        case BLKIF_OP_READ:
+            /* in case of failure ioreq->aio_errors is increased
+             * and it is logged */
+            ioreq_copy(ioreq);
+            free_buffers(ioreq);
+            break;
+        case BLKIF_OP_WRITE:
+        case BLKIF_OP_FLUSH_DISKCACHE:
+            if (!ioreq->req.nr_segments) {
+                break;
+            }
+            free_buffers(ioreq);
+            break;
+        default:
+            break;
+        }
+    }
+
     ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
-    ioreq_unmap(ioreq);
+    
+    if (!ioreq->blkdev->feature_grant_copy) {
+        ioreq_unmap(ioreq);
+    }
+
     ioreq_finish(ioreq);
     switch (ioreq->req.operation) {
     case BLKIF_OP_WRITE:
@@ -547,14 +669,42 @@ static void qemu_aio_complete(void *opaque, int ret)
     qemu_bh_schedule(ioreq->blkdev->bh);
 }
 
+static int ioreq_runio_qemu_aio_blk(struct ioreq *ioreq);
+
 static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 {
-    struct XenBlkDev *blkdev = ioreq->blkdev;
+    if (ioreq->blkdev->feature_grant_copy) {
+
+        ioreq_init_copy_buffers(ioreq);
+        if (ioreq->req.nr_segments && (ioreq->req.operation == BLKIF_OP_WRITE ||
+            ioreq->req.operation == BLKIF_OP_FLUSH_DISKCACHE)) {
+            if (ioreq_copy(ioreq)) {
+                free_buffers(ioreq);
+                goto err;
+            }
+        }
+        if (ioreq_runio_qemu_aio_blk(ioreq)) goto err;
 
-    if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
-        goto err_no_map;
+    } else {
+        
+        if (ioreq->req.nr_segments && ioreq_map(ioreq)) goto err;
+        if (ioreq_runio_qemu_aio_blk(ioreq)) {
+            ioreq_unmap(ioreq);
+            goto err;
+        }
     }
 
+    return 0;
+err:
+    ioreq_finish(ioreq);
+    ioreq->status = BLKIF_RSP_ERROR;
+    return -1;
+}
+
+static int ioreq_runio_qemu_aio_blk(struct ioreq *ioreq) 
+{
+    struct XenBlkDev *blkdev = ioreq->blkdev;
+
     ioreq->aio_inflight++;
     if (ioreq->presync) {
         blk_aio_flush(ioreq->blkdev->blk, qemu_aio_complete, ioreq);
@@ -594,19 +744,12 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
     }
     default:
         /* unknown operation (shouldn't happen -- parse catches this) */
-        goto err;
+        return -1;
     }
 
     qemu_aio_complete(ioreq, 0);
 
     return 0;
-
-err:
-    ioreq_unmap(ioreq);
-err_no_map:
-    ioreq_finish(ioreq);
-    ioreq->status = BLKIF_RSP_ERROR;
-    return -1;
 }
 
 static int blk_send_response_one(struct ioreq *ioreq)
@@ -1020,10 +1163,18 @@ static int blk_connect(struct XenDevice *xendev)
 
     xen_be_bind_evtchn(&blkdev->xendev);
 
+    xc_gnttab_grant_copy_segment_t seg;
+    blkdev->feature_grant_copy = 
+                (xc_gnttab_grant_copy(blkdev->xendev.gnttabdev, 0, &seg) == 0);
+
+    xen_be_printf(&blkdev->xendev, 3, "GRANT COPY %s\n", 
+                  blkdev->feature_grant_copy ? "ENABLED" : "DISABLED");
+
     xen_be_printf(&blkdev->xendev, 1, "ok: proto %s, ring-ref %d, "
                   "remote port %d, local port %d\n",
                   blkdev->xendev.protocol, blkdev->ring_ref,
                   blkdev->xendev.remote_port, blkdev->xendev.local_port);
+
     return 0;
 }
 
-- 
1.9.1


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

Re: [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[David Vrabel]

On 13/06/16 10:43, Paulina Szubarczyk wrote:
> Implentation of interface for grant copy operation in libs and
> libxc.
> 
> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> system call is invoked. In mini-os the operation is yet not
> implemented. For other OSs there is a dummy implementation.

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David


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

Re: [PATCH v2 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[David Vrabel]

On 13/06/16 10:43, Paulina Szubarczyk wrote:
> Copy data operated on during request from/to local buffers to/from 
> the grant references. 
> 
> Before grant copy operation local buffers must be allocated what is 
> done by calling ioreq_init_copy_buffers. For the 'read' operation, 
> first, the qemu device invokes the read operation on local buffers 
> and on the completion grant copy is called and buffers are freed. 
> For the 'write' operation grant copy is performed before invoking 
> write by qemu device. 
> 
> A new value 'feature_grant_copy' is added to recognize when the 
> grant copy operation is supported by a guest. 
> The body of the function 'ioreq_runio_qemu_aio' is moved to 
> 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
> on the support for grant copy according checks, initialization, grant 
> operation are made, then the 'ioreq_runio_qemu_aio_blk' function is 
> called. 

I think you should add an option to force the use of grant mapping even
if copy support is detected.  If future changes to the grant map
infrastructure makes it faster or if grant map scales better in some
systems, then it would be useful to be able to use it.

> +    rc = xc_gnttab_grant_copy(gnt, count, segs);
> +
> +    if (rc) {
> +        xen_be_printf(&ioreq->blkdev->xendev, 0, 
> +                      "failed to copy data %d \n", rc);

I don't think you want to log anything here.  A guest could spam the
logs by repeatedly submitting requests with (for example) bad grant
references.

> +        ioreq->aio_errors++;
> +        r = -1; goto out;

return -1;

> @@ -1020,10 +1163,18 @@ static int blk_connect(struct XenDevice *xendev)
>  
>      xen_be_bind_evtchn(&blkdev->xendev);
>  
> +    xc_gnttab_grant_copy_segment_t seg;
> +    blkdev->feature_grant_copy = 
> +                (xc_gnttab_grant_copy(blkdev->xendev.gnttabdev, 0, &seg) == 0);

You can pass NULL for the segments here.

> +
> +    xen_be_printf(&blkdev->xendev, 3, "GRANT COPY %s\n", 
> +                  blkdev->feature_grant_copy ? "ENABLED" : "DISABLED");
> +
>      xen_be_printf(&blkdev->xendev, 1, "ok: proto %s, ring-ref %d, "
>                    "remote port %d, local port %d\n",
>                    blkdev->xendev.protocol, blkdev->ring_ref,
>                    blkdev->xendev.remote_port, blkdev->xendev.local_port);
> +
>      return 0;
>  }

David


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

Re: [PATCH v2 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Paulina Szubarczyk]

On Mon, 2016-06-13 at 11:15 +0100, David Vrabel wrote:
> On 13/06/16 10:43, Paulina Szubarczyk wrote:
> > Copy data operated on during request from/to local buffers to/from 
> > the grant references. 
> > 
> > Before grant copy operation local buffers must be allocated what is 
> > done by calling ioreq_init_copy_buffers. For the 'read' operation, 
> > first, the qemu device invokes the read operation on local buffers 
> > and on the completion grant copy is called and buffers are freed. 
> > For the 'write' operation grant copy is performed before invoking 
> > write by qemu device. 
> > 
> > A new value 'feature_grant_copy' is added to recognize when the 
> > grant copy operation is supported by a guest. 
> > The body of the function 'ioreq_runio_qemu_aio' is moved to 
> > 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
> > on the support for grant copy according checks, initialization, grant 
> > operation are made, then the 'ioreq_runio_qemu_aio_blk' function is 
> > called. 
> 
> I think you should add an option to force the use of grant mapping even
> if copy support is detected.  If future changes to the grant map
> infrastructure makes it faster or if grant map scales better in some
> systems, then it would be useful to be able to use it.

The 'feature_grant_copy' is a boolean and could be set to false in such case.
There could be added a node in XenStore, for example
'feature-force-grant-map', which when set by frontend will be read
during a connection and changed the value to false forcing the grant map
operation. 

> > +    rc = xc_gnttab_grant_copy(gnt, count, segs);
> > +
> > +    if (rc) {
> > +        xen_be_printf(&ioreq->blkdev->xendev, 0, 
> > +                      "failed to copy data %d \n", rc);
> 
> I don't think you want to log anything here.  A guest could spam the
> logs by repeatedly submitting requests with (for example) bad grant
> references.

I might removed that log or change the level, though when the mapping
fails for grant map it is logged in a similar manner.

> > +        ioreq->aio_errors++;
> > +        r = -1; goto out;
> 
> return -1;
> 
> > @@ -1020,10 +1163,18 @@ static int blk_connect(struct XenDevice *xendev)
> >  
> >      xen_be_bind_evtchn(&blkdev->xendev);
> >  
> > +    xc_gnttab_grant_copy_segment_t seg;
> > +    blkdev->feature_grant_copy = 
> > +                (xc_gnttab_grant_copy(blkdev->xendev.gnttabdev, 0, &seg) == 0);
> 
> You can pass NULL for the segments here.

Yes, thank you.
> 
> > +
> > +    xen_be_printf(&blkdev->xendev, 3, "GRANT COPY %s\n", 
> > +                  blkdev->feature_grant_copy ? "ENABLED" : "DISABLED");
> > +
> >      xen_be_printf(&blkdev->xendev, 1, "ok: proto %s, ring-ref %d, "
> >                    "remote port %d, local port %d\n",
> >                    blkdev->xendev.protocol, blkdev->ring_ref,
> >                    blkdev->xendev.remote_port, blkdev->xendev.local_port);
> > +
> >      return 0;
> >  }
> 
> David
> 
Paulina



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

Re: [PATCH v2 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[David Vrabel]

On 13/06/16 11:44, Paulina Szubarczyk wrote:
> On Mon, 2016-06-13 at 11:15 +0100, David Vrabel wrote:
>> On 13/06/16 10:43, Paulina Szubarczyk wrote:
>>> Copy data operated on during request from/to local buffers to/from 
>>> the grant references. 
>>>
>>> Before grant copy operation local buffers must be allocated what is 
>>> done by calling ioreq_init_copy_buffers. For the 'read' operation, 
>>> first, the qemu device invokes the read operation on local buffers 
>>> and on the completion grant copy is called and buffers are freed. 
>>> For the 'write' operation grant copy is performed before invoking 
>>> write by qemu device. 
>>>
>>> A new value 'feature_grant_copy' is added to recognize when the 
>>> grant copy operation is supported by a guest. 
>>> The body of the function 'ioreq_runio_qemu_aio' is moved to 
>>> 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
>>> on the support for grant copy according checks, initialization, grant 
>>> operation are made, then the 'ioreq_runio_qemu_aio_blk' function is 
>>> called. 
>>
>> I think you should add an option to force the use of grant mapping even
>> if copy support is detected.  If future changes to the grant map
>> infrastructure makes it faster or if grant map scales better in some
>> systems, then it would be useful to be able to use it.
> 
> The 'feature_grant_copy' is a boolean and could be set to false in such case.
> There could be added a node in XenStore, for example
> 'feature-force-grant-map', which when set by frontend will be read
> during a connection and changed the value to false forcing the grant map
> operation.

This option should not be exposed/controlled by the frontend.  I was
thinking of a command line option to qemu or similar.

David

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

Re: [PATCH v2 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Paulina Szubarczyk]

On Mon, 2016-06-13 at 11:58 +0100, David Vrabel wrote:
> On 13/06/16 11:44, Paulina Szubarczyk wrote:
> > On Mon, 2016-06-13 at 11:15 +0100, David Vrabel wrote:
> >> On 13/06/16 10:43, Paulina Szubarczyk wrote:
> >>> Copy data operated on during request from/to local buffers to/from 
> >>> the grant references. 
> >>>
> >>> Before grant copy operation local buffers must be allocated what is 
> >>> done by calling ioreq_init_copy_buffers. For the 'read' operation, 
> >>> first, the qemu device invokes the read operation on local buffers 
> >>> and on the completion grant copy is called and buffers are freed. 
> >>> For the 'write' operation grant copy is performed before invoking 
> >>> write by qemu device. 
> >>>
> >>> A new value 'feature_grant_copy' is added to recognize when the 
> >>> grant copy operation is supported by a guest. 
> >>> The body of the function 'ioreq_runio_qemu_aio' is moved to 
> >>> 'ioreq_runio_qemu_aio_blk' and in the 'ioreq_runio_qemu_aio' depending
> >>> on the support for grant copy according checks, initialization, grant 
> >>> operation are made, then the 'ioreq_runio_qemu_aio_blk' function is 
> >>> called. 
> >>
> >> I think you should add an option to force the use of grant mapping even
> >> if copy support is detected.  If future changes to the grant map
> >> infrastructure makes it faster or if grant map scales better in some
> >> systems, then it would be useful to be able to use it.
> > 
> > The 'feature_grant_copy' is a boolean and could be set to false in such case.
> > There could be added a node in XenStore, for example
> > 'feature-force-grant-map', which when set by frontend will be read
> > during a connection and changed the value to false forcing the grant map
> > operation.
> 
> This option should not be exposed/controlled by the frontend.  I was
> thinking of a command line option to qemu or similar.

I think then there should be possibility for setting this in 
xl block-attach <disk-spec-component(s)> and in the config file that 
defines the domain in the corresponding fields. But if there is 
a need for such feature I would rather do it in a different patch.

Paulina


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

Re: [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Wei Liu]

On Mon, Jun 13, 2016 at 11:43:55AM +0200, Paulina Szubarczyk wrote:
> Implentation of interface for grant copy operation in libs and
> libxc.
> 
> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> system call is invoked. In mini-os the operation is yet not
> implemented. For other OSs there is a dummy implementation.
> 
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> 
> ---
> Changes since v1:
> - changed the interface to call grant copy operation to match ioctl
>   int xengnttab_grant_copy(xengnttab_handle *xgt,
>                            uint32_t count,
>                            xengnttab_grant_copy_segment_t* segs)
> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs  
>   /gnttab/private.h, tools/libxc/include/xenctrl_compat.h
> - changed the function osdep_gnttab_grant_copy which right now just call
>   the ioctl
> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map 
> 
>  tools/include/xen-sys/Linux/gntdev.h  | 21 +++++++++++++++++++++
>  tools/libs/gnttab/gnttab_core.c       |  6 ++++++
>  tools/libs/gnttab/gnttab_unimp.c      |  6 ++++++
>  tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++
>  tools/libs/gnttab/libxengnttab.map    |  4 ++++
>  tools/libs/gnttab/linux.c             | 18 ++++++++++++++++++
>  tools/libs/gnttab/minios.c            |  6 ++++++
>  tools/libs/gnttab/private.h           | 18 ++++++++++++++++++
>  tools/libxc/include/xenctrl_compat.h  | 23 +++++++++++++++++++++++
>  tools/libxc/xc_gnttab_compat.c        |  7 +++++++
>  10 files changed, 123 insertions(+)
> 
> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
> index caf6fb4..0ca07c9 100644
> --- a/tools/include/xen-sys/Linux/gntdev.h
> +++ b/tools/include/xen-sys/Linux/gntdev.h
> @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
>  /* Send an interrupt on the indicated event channel */
>  #define UNMAP_NOTIFY_SEND_EVENT 0x2
>  
> +struct ioctl_gntdev_grant_copy_segment {
> +    union {
> +        void *virt;
> +        struct {
> +            uint32_t ref;
> +            uint16_t offset;
> +            uint16_t domid;
> +        } foreign;
> +    } source, dest;
> +    uint16_t len;
> +    uint16_t flags;
> +    int16_t status;
> +};
> +
> +#define IOCTL_GNTDEV_GRANT_COPY \
> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
> +struct ioctl_gntdev_grant_copy {
> +    unsigned int count;
> +    struct ioctl_gntdev_grant_copy_segment *segments;
> +};
> +

This is ok.

>  #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
> index 5d0474d..9badc58 100644
> --- a/tools/libs/gnttab/gnttab_core.c
> +++ b/tools/libs/gnttab/gnttab_core.c
> @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
>      return osdep_gnttab_unmap(xgt, start_address, count);
>  }
>  
> +int xengnttab_grant_copy(xengnttab_handle *xgt,
> +                         uint32_t count,
> +                         xengnttab_grant_copy_segment_t* segs)
> +{
> +    return osdep_gnttab_grant_copy(xgt, count, segs);
> +}
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c
> index b3a4a20..79a5c88 100644
> --- a/tools/libs/gnttab/gnttab_unimp.c
> +++ b/tools/libs/gnttab/gnttab_unimp.c
> @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
>      abort();
>  }
>  
> +int xengnttab_copy_grant(xengnttab_handle *xgt,
> +                         uint32_t count,
> +                         xengnttab_copy_grant_segment_t* segs)

Coding style. Should be " *segs" instead of "* segs".

Please also fix other instances of this nit.

> +{
> +    return -1;

Please use abort() here instead.

> +}
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libs/gnttab/include/xengnttab.h b/tools/libs/gnttab/include/xengnttab.h
> index 0431dcf..a9fa1fe 100644
> --- a/tools/libs/gnttab/include/xengnttab.h
> +++ b/tools/libs/gnttab/include/xengnttab.h
> @@ -258,6 +258,20 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count);
>  int xengnttab_set_max_grants(xengnttab_handle *xgt,
>                               uint32_t nr_grants);
>  
> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> +
> +/**
> + * Copy memory from or to grant references. The information of each operations
> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
> + *
> + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
> + * should not exceed XEN_PAGE_SIZE
> + */
> +int xengnttab_grant_copy(xengnttab_handle *xgt,
> +                         uint32_t count,
> +                         xengnttab_grant_copy_segment_t* segs);
> +
>  /*
>   * Grant Sharing Interface (allocating and granting pages to others)
>   */
> diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
> index dc737ac..0928963 100644
> --- a/tools/libs/gnttab/libxengnttab.map
> +++ b/tools/libs/gnttab/libxengnttab.map
> @@ -21,3 +21,7 @@ VERS_1.0 {
>  		xengntshr_unshare;
>  	local: *; /* Do not expose anything by default */
>  };
> +
> +VERS_1.1 {

Missing "global:" here?

> +		xengnttab_grant_copy;
> +} VERS_1.0;

You also need to modify the "MINOR" field in gnttab/Makefile so that the
generate so file contains the correct version number.

> \ No newline at end of file
> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> index 7b0fba4..26bfbdc 100644
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
>      return 0;
>  }
>  
> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> index d286c86..22ad53a 100644
> --- a/tools/libs/gnttab/private.h
> +++ b/tools/libs/gnttab/private.h
> @@ -9,6 +9,20 @@ struct xengntdev_handle {
>      int fd;
>  };
>  
> +struct xengnttab_copy_grant_segment {
> +    union xengnttab_copy_ptr {
> +        void *virt;
> +        struct {
> +            uint32_t ref;
> +            uint16_t offset;
> +            uint16_t domid;
> +        } foreign;
> +    } source, dest;
> +    uint16_t len;
> +    uint16_t flags;
> +    int16_t status;
> +};
> +

The struct is more or less a direct copy of Linux structure. It is
probably fine as-is, but I don't want to risk making this library Linux
centric. If you look at other functions, they accept a bunch of discrete
arguments then assemble those arguments into OS dependent structure in
osdep functions. I know having discrete arguments for the API you want
to introduce seems cumbersome, but I want to at least tell you all the
background information needed for a thorough discussion. I would be
interested in Roger's view on this.

I apologise for not having commented on your series earlier.

>  int osdep_gnttab_open(xengnttab_handle *xgt);
>  int osdep_gnttab_close(xengnttab_handle *xgt);
>  
> @@ -23,6 +37,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
>  int osdep_gnttab_unmap(xengnttab_handle *xgt,
>                         void *start_address,
>                         uint32_t count);
> +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> +                            uint32_t count,
> +                            xengnttab_grant_copy_segment_t* segs);
> +
>  int osdep_gntshr_open(xengntshr_handle *xgs);
>  int osdep_gntshr_close(xengntshr_handle *xgs);
>  
> diff --git a/tools/libxc/include/xenctrl_compat.h b/tools/libxc/include/xenctrl_compat.h
> index 93ccadb..7bb24a4 100644
> --- a/tools/libxc/include/xenctrl_compat.h
> +++ b/tools/libxc/include/xenctrl_compat.h
> @@ -105,6 +105,29 @@ int xc_gnttab_munmap(xc_gnttab *xcg,
>  int xc_gnttab_set_max_grants(xc_gnttab *xcg,
>                               uint32_t count);
>  
> +union xengnttab_grant_copy_ptr {
> +    void *virt;
> +    struct {
> +        uint32_t ref;
> +        uint16_t offset;
> +        uint16_t domid;
> +    } foreign;
> +};
> +
> +struct xengnttab_grant_copy_segment {
> +    union xengnttab_grant_copy_ptr source, dest;
> +    uint16_t len;
> +    uint16_t flags;
> +    int16_t status;
> +};
> +
> +typedef struct xengnttab_grant_copy_segment xc_gnttab_grant_copy_segment_t;
> +typedef union xengnttab_grant_copy_ptr xc_gnttab_grant_copy_ptr_t;
> +
> +int xc_gnttab_grant_copy(xc_gnttab *xgt,
> +                         uint32_t count,
> +                         xc_gnttab_grant_copy_segment_t* segs);
> +

The purpose of compact header is to make old application code that uses
old APIs (the xc_* functions) continue to compile by providing necessary
stubs.

This is new API, so it doesn't belong to the compat header. Anyone who
wants to use this new API should be using libxengnttab instead. We won't
be adding more xc_ functions.

I think you can just drop these changes completely.

This also means the compatibility should be on QEMU side. I think QEMU
already knows how to link against libxengnttab. What you need to do is
to actually test if the API exists and take appropriate action there.

If I'm not clear enough, feel free to ask.

Wei.

>  typedef struct xengntdev_handle xc_gntshr;
>  
>  xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
> diff --git a/tools/libxc/xc_gnttab_compat.c b/tools/libxc/xc_gnttab_compat.c
> index 6f036d8..c265bb0 100644
> --- a/tools/libxc/xc_gnttab_compat.c
> +++ b/tools/libxc/xc_gnttab_compat.c
> @@ -69,6 +69,13 @@ int xc_gnttab_set_max_grants(xc_gnttab *xcg,
>      return xengnttab_set_max_grants(xcg, count);
>  }
>  
> +int xc_gnttab_grant_copy(xc_gnttab *xgt,
> +                         uint32_t count,
> +                         xc_gnttab_grant_copy_segment_t* segs)
> +{
> +    return xengnttab_grant_copy(xgt, count, segs);
> +}
> +
>  xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
>                            unsigned open_flags)
>  {
> -- 
> 1.9.1
> 

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

Re: [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[David Vrabel]

On 16/06/16 13:16, Wei Liu wrote:
> On Mon, Jun 13, 2016 at 11:43:55AM +0200, Paulina Szubarczyk wrote:
>> Implentation of interface for grant copy operation in libs and
>> libxc.
>>
>> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
>> system call is invoked. In mini-os the operation is yet not
>> implemented. For other OSs there is a dummy implementation.
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>>
>> ---
>> Changes since v1:
>> - changed the interface to call grant copy operation to match ioctl
>>   int xengnttab_grant_copy(xengnttab_handle *xgt,
>>                            uint32_t count,
>>                            xengnttab_grant_copy_segment_t* segs)
>> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs  
>>   /gnttab/private.h, tools/libxc/include/xenctrl_compat.h
>> - changed the function osdep_gnttab_grant_copy which right now just call
>>   the ioctl
>> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map 
>>
>>  tools/include/xen-sys/Linux/gntdev.h  | 21 +++++++++++++++++++++
>>  tools/libs/gnttab/gnttab_core.c       |  6 ++++++
>>  tools/libs/gnttab/gnttab_unimp.c      |  6 ++++++
>>  tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++
>>  tools/libs/gnttab/libxengnttab.map    |  4 ++++
>>  tools/libs/gnttab/linux.c             | 18 ++++++++++++++++++
>>  tools/libs/gnttab/minios.c            |  6 ++++++
>>  tools/libs/gnttab/private.h           | 18 ++++++++++++++++++
>>  tools/libxc/include/xenctrl_compat.h  | 23 +++++++++++++++++++++++
>>  tools/libxc/xc_gnttab_compat.c        |  7 +++++++
>>  10 files changed, 123 insertions(+)
>>
>> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
>> index caf6fb4..0ca07c9 100644
>> --- a/tools/include/xen-sys/Linux/gntdev.h
>> +++ b/tools/include/xen-sys/Linux/gntdev.h
>> @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
>>  /* Send an interrupt on the indicated event channel */
>>  #define UNMAP_NOTIFY_SEND_EVENT 0x2
>>  
>> +struct ioctl_gntdev_grant_copy_segment {
>> +    union {
>> +        void *virt;
>> +        struct {
>> +            uint32_t ref;
>> +            uint16_t offset;
>> +            uint16_t domid;
>> +        } foreign;
>> +    } source, dest;
>> +    uint16_t len;
>> +    uint16_t flags;
>> +    int16_t status;
>> +};
>> +
>> +#define IOCTL_GNTDEV_GRANT_COPY \
>> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
>> +struct ioctl_gntdev_grant_copy {
>> +    unsigned int count;
>> +    struct ioctl_gntdev_grant_copy_segment *segments;
>> +};
>> +
> 
> This is ok.
> 
>>  #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
>> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
>> index 5d0474d..9badc58 100644
>> --- a/tools/libs/gnttab/gnttab_core.c
>> +++ b/tools/libs/gnttab/gnttab_core.c
>> @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
>>      return osdep_gnttab_unmap(xgt, start_address, count);
>>  }
>>  
>> +int xengnttab_grant_copy(xengnttab_handle *xgt,
>> +                         uint32_t count,
>> +                         xengnttab_grant_copy_segment_t* segs)
>> +{
>> +    return osdep_gnttab_grant_copy(xgt, count, segs);
>> +}
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c
>> index b3a4a20..79a5c88 100644
>> --- a/tools/libs/gnttab/gnttab_unimp.c
>> +++ b/tools/libs/gnttab/gnttab_unimp.c
>> @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
>>      abort();
>>  }
>>  
>> +int xengnttab_copy_grant(xengnttab_handle *xgt,
>> +                         uint32_t count,
>> +                         xengnttab_copy_grant_segment_t* segs)
> 
> Coding style. Should be " *segs" instead of "* segs".
> 
> Please also fix other instances of this nit.
> 
>> +{
>> +    return -1;
> 
> Please use abort() here instead.

This function is used to test whether grant copy is supported so cannot
abort().  It is correctly returning an error.

>> \ No newline at end of file
>> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
>> index 7b0fba4..26bfbdc 100644
>> --- a/tools/libs/gnttab/linux.c
>> +++ b/tools/libs/gnttab/linux.c
>> @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
>>      return 0;
>>  }
>>  
>> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
>> index d286c86..22ad53a 100644
>> --- a/tools/libs/gnttab/private.h
>> +++ b/tools/libs/gnttab/private.h
>> @@ -9,6 +9,20 @@ struct xengntdev_handle {
>>      int fd;
>>  };
>>  
>> +struct xengnttab_copy_grant_segment {
>> +    union xengnttab_copy_ptr {
>> +        void *virt;
>> +        struct {
>> +            uint32_t ref;
>> +            uint16_t offset;
>> +            uint16_t domid;
>> +        } foreign;
>> +    } source, dest;
>> +    uint16_t len;
>> +    uint16_t flags;
>> +    int16_t status;
>> +};
>> +
> 
> The struct is more or less a direct copy of Linux structure.

Not really.  It's a copy of the hypercall structure, adjusted to have a
virt address instead of gfn/offset for local buffers.

The Linux structure is also a similar copy which is why they happen to
look the same.

The previous threads explain why it is like this, but in summary this
allows the library to present the same functionality as the underlying
hypercall.

I would also suggest you look at the previous version of this series to
compare the user of this API.  The one using this structure is nicer.

David

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

Re: [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Wei Liu]

On Thu, Jun 16, 2016 at 01:36:32PM +0100, David Vrabel wrote:
> On 16/06/16 13:16, Wei Liu wrote:
> > On Mon, Jun 13, 2016 at 11:43:55AM +0200, Paulina Szubarczyk wrote:
> >> Implentation of interface for grant copy operation in libs and
> >> libxc.
> >>
> >> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> >> system call is invoked. In mini-os the operation is yet not
> >> implemented. For other OSs there is a dummy implementation.
> >>
> >> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> >>
> >> ---
> >> Changes since v1:
> >> - changed the interface to call grant copy operation to match ioctl
> >>   int xengnttab_grant_copy(xengnttab_handle *xgt,
> >>                            uint32_t count,
> >>                            xengnttab_grant_copy_segment_t* segs)
> >> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs  
> >>   /gnttab/private.h, tools/libxc/include/xenctrl_compat.h
> >> - changed the function osdep_gnttab_grant_copy which right now just call
> >>   the ioctl
> >> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map 
> >>
> >>  tools/include/xen-sys/Linux/gntdev.h  | 21 +++++++++++++++++++++
> >>  tools/libs/gnttab/gnttab_core.c       |  6 ++++++
> >>  tools/libs/gnttab/gnttab_unimp.c      |  6 ++++++
> >>  tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++
> >>  tools/libs/gnttab/libxengnttab.map    |  4 ++++
> >>  tools/libs/gnttab/linux.c             | 18 ++++++++++++++++++
> >>  tools/libs/gnttab/minios.c            |  6 ++++++
> >>  tools/libs/gnttab/private.h           | 18 ++++++++++++++++++
> >>  tools/libxc/include/xenctrl_compat.h  | 23 +++++++++++++++++++++++
> >>  tools/libxc/xc_gnttab_compat.c        |  7 +++++++
> >>  10 files changed, 123 insertions(+)
> >>
> >> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
> >> index caf6fb4..0ca07c9 100644
> >> --- a/tools/include/xen-sys/Linux/gntdev.h
> >> +++ b/tools/include/xen-sys/Linux/gntdev.h
> >> @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
> >>  /* Send an interrupt on the indicated event channel */
> >>  #define UNMAP_NOTIFY_SEND_EVENT 0x2
> >>  
> >> +struct ioctl_gntdev_grant_copy_segment {
> >> +    union {
> >> +        void *virt;
> >> +        struct {
> >> +            uint32_t ref;
> >> +            uint16_t offset;
> >> +            uint16_t domid;
> >> +        } foreign;
> >> +    } source, dest;
> >> +    uint16_t len;
> >> +    uint16_t flags;
> >> +    int16_t status;
> >> +};
> >> +
> >> +#define IOCTL_GNTDEV_GRANT_COPY \
> >> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
> >> +struct ioctl_gntdev_grant_copy {
> >> +    unsigned int count;
> >> +    struct ioctl_gntdev_grant_copy_segment *segments;
> >> +};
> >> +
> > 
> > This is ok.
> > 
> >>  #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
> >> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
> >> index 5d0474d..9badc58 100644
> >> --- a/tools/libs/gnttab/gnttab_core.c
> >> +++ b/tools/libs/gnttab/gnttab_core.c
> >> @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
> >>      return osdep_gnttab_unmap(xgt, start_address, count);
> >>  }
> >>  
> >> +int xengnttab_grant_copy(xengnttab_handle *xgt,
> >> +                         uint32_t count,
> >> +                         xengnttab_grant_copy_segment_t* segs)
> >> +{
> >> +    return osdep_gnttab_grant_copy(xgt, count, segs);
> >> +}
> >>  /*
> >>   * Local variables:
> >>   * mode: C
> >> diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c
> >> index b3a4a20..79a5c88 100644
> >> --- a/tools/libs/gnttab/gnttab_unimp.c
> >> +++ b/tools/libs/gnttab/gnttab_unimp.c
> >> @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
> >>      abort();
> >>  }
> >>  
> >> +int xengnttab_copy_grant(xengnttab_handle *xgt,
> >> +                         uint32_t count,
> >> +                         xengnttab_copy_grant_segment_t* segs)
> > 
> > Coding style. Should be " *segs" instead of "* segs".
> > 
> > Please also fix other instances of this nit.
> > 
> >> +{
> >> +    return -1;
> > 
> > Please use abort() here instead.
> 
> This function is used to test whether grant copy is supported so cannot
> abort().  It is correctly returning an error.
> 

No. For the "unimplemented" implementation, the code shouldn't call this
function in the first place because the attempt to open a handle would
have already failed.

This is an impossible state for the "unimplemented" implementation. Any
application uses the API like this deserves to be aborted.

> >> \ No newline at end of file
> >> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> >> index 7b0fba4..26bfbdc 100644
> >> --- a/tools/libs/gnttab/linux.c
> >> +++ b/tools/libs/gnttab/linux.c
> >> @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
> >>      return 0;
> >>  }
> >>  
> >> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> >> index d286c86..22ad53a 100644
> >> --- a/tools/libs/gnttab/private.h
> >> +++ b/tools/libs/gnttab/private.h
> >> @@ -9,6 +9,20 @@ struct xengntdev_handle {
> >>      int fd;
> >>  };
> >>  
> >> +struct xengnttab_copy_grant_segment {
> >> +    union xengnttab_copy_ptr {
> >> +        void *virt;
> >> +        struct {
> >> +            uint32_t ref;
> >> +            uint16_t offset;
> >> +            uint16_t domid;
> >> +        } foreign;
> >> +    } source, dest;
> >> +    uint16_t len;
> >> +    uint16_t flags;
> >> +    int16_t status;
> >> +};
> >> +
> > 
> > The struct is more or less a direct copy of Linux structure.
> 
> Not really.  It's a copy of the hypercall structure, adjusted to have a
> virt address instead of gfn/offset for local buffers.
> 
> The Linux structure is also a similar copy which is why they happen to
> look the same.
> 
> The previous threads explain why it is like this, but in summary this
> allows the library to present the same functionality as the underlying
> hypercall.
> 
> I would also suggest you look at the previous version of this series to
> compare the user of this API.  The one using this structure is nicer.
> 

Right. I will go back and read the thread.

Wei.

> David

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

Re: [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Wei Liu]

On Thu, Jun 16, 2016 at 01:16:54PM +0100, Wei Liu wrote:
[...]
[...]
> > diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> > index d286c86..22ad53a 100644
> > --- a/tools/libs/gnttab/private.h
> > +++ b/tools/libs/gnttab/private.h
> > @@ -9,6 +9,20 @@ struct xengntdev_handle {
> >      int fd;
> >  };
> >
> > +struct xengnttab_copy_grant_segment {
> > +    union xengnttab_copy_ptr {
> > +        void *virt;
> > +        struct {
> > +            uint32_t ref;
> > +            uint16_t offset;
> > +            uint16_t domid;
> > +        } foreign;
> > +    } source, dest;
> > +    uint16_t len;
> > +    uint16_t flags;
> > +    int16_t status;
> > +};
> >
>
> The struct is more or less a direct copy of Linux structure. It is
> probably fine as-is, but I don't want to risk making this library Linux
> centric. If you look at other functions, they accept a bunch of discrete
> arguments then assemble those arguments into OS dependent structure in
> osdep functions. I know having discrete arguments for the API you want
> to introduce seems cumbersome, but I want to at least tell you all the
> background information needed for a thorough discussion. I would be
> interested in Roger's view on this.
>
> I apologise for not having commented on your series earlier.
>

After checking various places I'm convinced that this structure is fine
as-is.

BSDes have not yet had a user space grant table driver, so I don't
really worry about that at this point.

As I have asked you to removed all the stuff in xenctrl_compat.h, you
will need to move this to libs/gnttab/xengnttab.h.

Also I have one further comment for code:

> +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> +                            uint32_t count,
> +                            xengnttab_grant_copy_segment_t* segs)
> +{
> +    int fd = xgt->fd;
> +    struct ioctl_gntdev_grant_copy copy;
> +
> +    copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs;

Please create an array of ioctl structure and use explicit field by
field assignment here.

Casting like this can easily introduce bug -- just imagine ioctl gets
changed, or the segment structure gets changed.

Wei.

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

Re: [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Paulina Szubarczyk]

On Fri, 2016-06-17 at 17:43 +0100, Wei Liu wrote:
> On Thu, Jun 16, 2016 at 01:16:54PM +0100, Wei Liu wrote:
> [...]
> [...]
> > > diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> > > index d286c86..22ad53a 100644
> > > --- a/tools/libs/gnttab/private.h
> > > +++ b/tools/libs/gnttab/private.h
> > > @@ -9,6 +9,20 @@ struct xengntdev_handle {
> > >      int fd;
> > >  };
> > >
> > > +struct xengnttab_copy_grant_segment {
> > > +    union xengnttab_copy_ptr {
> > > +        void *virt;
> > > +        struct {
> > > +            uint32_t ref;
> > > +            uint16_t offset;
> > > +            uint16_t domid;
> > > +        } foreign;
> > > +    } source, dest;
> > > +    uint16_t len;
> > > +    uint16_t flags;
> > > +    int16_t status;
> > > +};
> > >
> >
> > The struct is more or less a direct copy of Linux structure. It is
> > probably fine as-is, but I don't want to risk making this library Linux
> > centric. If you look at other functions, they accept a bunch of discrete
> > arguments then assemble those arguments into OS dependent structure in
> > osdep functions. I know having discrete arguments for the API you want
> > to introduce seems cumbersome, but I want to at least tell you all the
> > background information needed for a thorough discussion. I would be
> > interested in Roger's view on this.
> >
> > I apologise for not having commented on your series earlier.
> >
> 
> After checking various places I'm convinced that this structure is fine
> as-is.
> 
> BSDes have not yet had a user space grant table driver, so I don't
> really worry about that at this point.
> 
> As I have asked you to removed all the stuff in xenctrl_compat.h, you
> will need to move this to libs/gnttab/xengnttab.h.
> 
> Also I have one further comment for code:
> 
> > +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> > +                            uint32_t count,
> > +                            xengnttab_grant_copy_segment_t* segs)
> > +{
> > +    int fd = xgt->fd;
> > +    struct ioctl_gntdev_grant_copy copy;
> > +
> > +    copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs;
> 
> Please create an array of ioctl structure and use explicit field by
> field assignment here.
> 
> Casting like this can easily introduce bug -- just imagine ioctl gets
> changed, or the segment structure gets changed.
> 
> Wei.

Hi Wei, 

Thank you for all the remarks. I am working on correcting the patch.

Paulina



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