* [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
@ 2016-09-07 10:41 ` Paulina Szubarczyk
0 siblings, 0 replies; 18+ messages in thread
From: Paulina Szubarczyk @ 2016-09-07 10:41 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, Paulina Szubarczyk, ian.jackson,
qemu-devel, david.vrabel, anthony.perard, roger.pau
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.
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v5:
-added checking of every interface in the configure file. Based on
the Roger's comment that xengnttab_map_grant_ref was added prior
xengnttab_grant_copy, thus do not need to be check again here
I dropped this check.
---
configure | 55 ++++++++++++++++
hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
include/hw/xen/xen_common.h | 14 ++++
3 files changed, 221 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index 4b808f9..3f44d38 100755
--- a/configure
+++ b/configure
@@ -1956,6 +1956,61 @@ EOF
/*
* If we have stable libs the we don't want the libxc compat
* layers, regardless of what CFLAGS we may have been given.
+ *
+ * Also, check if xengnttab_grant_copy_segment_t is defined and
+ * grant copy operation is implemented.
+ */
+#undef XC_WANT_COMPAT_EVTCHN_API
+#undef XC_WANT_COMPAT_GNTTAB_API
+#undef XC_WANT_COMPAT_MAP_FOREIGN_API
+#include <xenctrl.h>
+#include <xenstore.h>
+#include <xenevtchn.h>
+#include <xengnttab.h>
+#include <xenforeignmemory.h>
+#include <stdint.h>
+#include <xen/hvm/hvm_info_table.h>
+#if !defined(HVM_MAX_VCPUS)
+# error HVM_MAX_VCPUS not defined
+#endif
+int main(void) {
+ xc_interface *xc = NULL;
+ xenforeignmemory_handle *xfmem;
+ xenevtchn_handle *xe;
+ xengnttab_handle *xg;
+ xen_domain_handle_t handle;
+ xengnttab_grant_copy_segment_t* seg = NULL;
+
+ xs_daemon_open();
+
+ xc = xc_interface_open(0, 0, 0);
+ xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+ xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
+ xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
+ xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
+ xc_domain_create(xc, 0, handle, 0, NULL, NULL);
+
+ xfmem = xenforeignmemory_open(0, 0);
+ xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
+
+ xe = xenevtchn_open(0, 0);
+ xenevtchn_fd(xe);
+
+ xg = xengnttab_open(0, 0);
+ xengnttab_grant_copy(xg, 0, seg);
+
+ return 0;
+}
+EOF
+ compile_prog "" "$xen_libs $xen_stable_libs"
+ then
+ xen_ctrl_version=480
+ xen=yes
+ elif
+ cat > $TMPC <<EOF &&
+/*
+ * If we have stable libs the we don't want the libxc compat
+ * layers, regardless of what CFLAGS we may have been given.
*/
#undef XC_WANT_COMPAT_EVTCHN_API
#undef XC_WANT_COMPAT_GNTTAB_API
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 3b8ad33..3739e13 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -119,6 +119,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;
@@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
return 0;
}
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
+
+static void free_buffers(struct ioreq *ioreq)
+{
+ int i;
+
+ for (i = 0; i < ioreq->v.niov; i++) {
+ ioreq->page[i] = NULL;
+ }
+
+ qemu_vfree(ioreq->pages);
+}
+
+static int ioreq_init_copy_buffers(struct ioreq *ioreq)
+{
+ int i;
+
+ if (ioreq->v.niov == 0) {
+ return 0;
+ }
+
+ ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
+
+ for (i = 0; i < ioreq->v.niov; i++) {
+ ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
+ ioreq->v.iov[i].iov_base = ioreq->page[i];
+ }
+
+ return 0;
+}
+
+static int ioreq_copy(struct ioreq *ioreq)
+{
+ xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
+ xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+ int i, count, rc;
+ int64_t file_blk = ioreq->blkdev->file_blk;
+
+ if (ioreq->v.niov == 0) {
+ return 0;
+ }
+
+ count = ioreq->v.niov;
+
+ for (i = 0; i < count; i++) {
+
+ if (ioreq->req.operation == BLKIF_OP_READ) {
+ segs[i].flags = GNTCOPY_dest_gref;
+ segs[i].dest.foreign.ref = ioreq->refs[i];
+ segs[i].dest.foreign.domid = ioreq->domids[i];
+ segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
+ segs[i].source.virt = ioreq->v.iov[i].iov_base;
+ } else {
+ segs[i].flags = GNTCOPY_source_gref;
+ segs[i].source.foreign.ref = ioreq->refs[i];
+ segs[i].source.foreign.domid = ioreq->domids[i];
+ segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
+ segs[i].dest.virt = ioreq->v.iov[i].iov_base;
+ }
+ segs[i].len = (ioreq->req.seg[i].last_sect
+ - ioreq->req.seg[i].first_sect + 1) * file_blk;
+
+ }
+
+ rc = xengnttab_grant_copy(gnt, count, segs);
+
+ if (rc) {
+ xen_be_printf(&ioreq->blkdev->xendev, 0,
+ "failed to copy data %d\n", rc);
+ ioreq->aio_errors++;
+ return -1;
+ }
+
+ for (i = 0; i < count; i++) {
+ if (segs[i].status != GNTST_okay) {
+ xen_be_printf(&ioreq->blkdev->xendev, 3,
+ "failed to copy data %d for gref %d, domid %d\n",
+ segs[i].status, ioreq->refs[i], ioreq->domids[i]);
+ ioreq->aio_errors++;
+ rc = -1;
+ }
+ }
+
+ return rc;
+}
+#else
+static void free_buffers(struct ioreq *ioreq)
+{
+ abort();
+}
+
+static int ioreq_init_copy_buffers(struct ioreq *ioreq)
+{
+ abort();
+}
+
+static int ioreq_copy(struct ioreq *ioreq)
+{
+ abort();
+}
+#endif
+
static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
static void qemu_aio_complete(void *opaque, int ret)
@@ -511,8 +616,31 @@ 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 */
+ if (ret == 0) {
+ 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:
@@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
{
struct XenBlkDev *blkdev = ioreq->blkdev;
- if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
- goto err_no_map;
+ 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;
+ }
+ }
+
+ } else {
+ if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
+ goto err;
+ }
}
ioreq->aio_inflight++;
@@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
}
default:
/* unknown operation (shouldn't happen -- parse catches this) */
+ if (!ioreq->blkdev->feature_grant_copy) {
+ ioreq_unmap(ioreq);
+ }
goto err;
}
@@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
return 0;
err:
- ioreq_unmap(ioreq);
-err_no_map:
ioreq_finish(ioreq);
ioreq->status = BLKIF_RSP_ERROR;
return -1;
@@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
xen_be_bind_evtchn(&blkdev->xendev);
+ blkdev->feature_grant_copy =
+ (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
+
+ xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index bd39287..8e1580d 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
#endif
#endif
+/* Xen before 4.8 */
+
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
+
+
+typedef void *xengnttab_grant_copy_segment_t;
+
+static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return -ENOSYS;
+}
+#endif
+
#endif /* QEMU_HW_XEN_COMMON_H */
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 10:41 ` Paulina Szubarczyk
(?)
@ 2016-09-07 17:05 ` Anthony PERARD
-1 siblings, 0 replies; 18+ messages in thread
From: Anthony PERARD @ 2016-09-07 17:05 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
xen-devel, roger.pau
On Wed, Sep 07, 2016 at 12:41:00PM +0200, 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.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 10:41 ` Paulina Szubarczyk
(?)
(?)
@ 2016-09-07 17:05 ` Anthony PERARD
-1 siblings, 0 replies; 18+ messages in thread
From: Anthony PERARD @ 2016-09-07 17:05 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
sstabellini, qemu-devel
On Wed, Sep 07, 2016 at 12:41:00PM +0200, 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.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 10:41 ` Paulina Szubarczyk
` (2 preceding siblings ...)
(?)
@ 2016-09-07 20:56 ` Stefano Stabellini
-1 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2016-09-07 20:56 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, xen-devel, roger.pau
On Wed, 7 Sep 2016, 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.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
> Changes since v5:
> -added checking of every interface in the configure file. Based on
> the Roger's comment that xengnttab_map_grant_ref was added prior
> xengnttab_grant_copy, thus do not need to be check again here
> I dropped this check.
>
Thank you Paulina, the patch is good. Thanks for your work! Sorry for
coming in late in the review; I have a couple of minor suggestions
below.
In addition to Anthony's ack, it would be also nice to have Roger's ack
on this patch.
> configure | 55 ++++++++++++++++
> hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 14 ++++
> 3 files changed, 221 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index 4b808f9..3f44d38 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,61 @@ EOF
> /*
> * If we have stable libs the we don't want the libxc compat
> * layers, regardless of what CFLAGS we may have been given.
> + *
> + * Also, check if xengnttab_grant_copy_segment_t is defined and
> + * grant copy operation is implemented.
> + */
> +#undef XC_WANT_COMPAT_EVTCHN_API
> +#undef XC_WANT_COMPAT_GNTTAB_API
> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <xenctrl.h>
> +#include <xenstore.h>
> +#include <xenevtchn.h>
> +#include <xengnttab.h>
> +#include <xenforeignmemory.h>
> +#include <stdint.h>
> +#include <xen/hvm/hvm_info_table.h>
> +#if !defined(HVM_MAX_VCPUS)
> +# error HVM_MAX_VCPUS not defined
> +#endif
> +int main(void) {
> + xc_interface *xc = NULL;
> + xenforeignmemory_handle *xfmem;
> + xenevtchn_handle *xe;
> + xengnttab_handle *xg;
> + xen_domain_handle_t handle;
> + xengnttab_grant_copy_segment_t* seg = NULL;
> +
> + xs_daemon_open();
> +
> + xc = xc_interface_open(0, 0, 0);
> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
> +
> + xfmem = xenforeignmemory_open(0, 0);
> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
> +
> + xe = xenevtchn_open(0, 0);
> + xenevtchn_fd(xe);
> +
> + xg = xengnttab_open(0, 0);
> + xengnttab_grant_copy(xg, 0, seg);
> +
> + return 0;
> +}
> +EOF
> + compile_prog "" "$xen_libs $xen_stable_libs"
> + then
> + xen_ctrl_version=480
> + xen=yes
> + elif
> + cat > $TMPC <<EOF &&
> +/*
> + * If we have stable libs the we don't want the libxc compat
> + * layers, regardless of what CFLAGS we may have been given.
> */
> #undef XC_WANT_COMPAT_EVTCHN_API
> #undef XC_WANT_COMPAT_GNTTAB_API
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3b8ad33..3739e13 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -119,6 +119,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;
> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
see that Anthony suggested it for a good reason. The only alternartive I
can think of would be to introduce two static inline functions in
xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
also OK.
> +static void free_buffers(struct ioreq *ioreq)
Please name this function ioreq_free_copy_buffers to make it clearer
that has to do with the same buffers initialized below.
> +{
> + int i;
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = NULL;
> + }
> +
> + qemu_vfree(ioreq->pages);
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + int i;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = ioreq->page[i];
> + }
> +
> + return 0;
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
Please name this function in a way that makes it clear that it has
something to do with grant copies. Like for example ioreq_grant_copy.
> +{
> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> + int i, count, rc;
> + int64_t file_blk = ioreq->blkdev->file_blk;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + count = ioreq->v.niov;
> +
> + for (i = 0; i < count; i++) {
> +
> + if (ioreq->req.operation == BLKIF_OP_READ) {
> + segs[i].flags = GNTCOPY_dest_gref;
> + segs[i].dest.foreign.ref = ioreq->refs[i];
> + segs[i].dest.foreign.domid = ioreq->domids[i];
> + segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
> + } else {
> + segs[i].flags = GNTCOPY_source_gref;
> + segs[i].source.foreign.ref = ioreq->refs[i];
> + segs[i].source.foreign.domid = ioreq->domids[i];
> + segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
> + }
> + segs[i].len = (ioreq->req.seg[i].last_sect
> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
> +
> + }
> +
> + rc = xengnttab_grant_copy(gnt, count, segs);
> +
> + if (rc) {
> + xen_be_printf(&ioreq->blkdev->xendev, 0,
> + "failed to copy data %d\n", rc);
> + ioreq->aio_errors++;
> + return -1;
> + }
> +
> + for (i = 0; i < count; i++) {
> + if (segs[i].status != GNTST_okay) {
> + xen_be_printf(&ioreq->blkdev->xendev, 3,
> + "failed to copy data %d for gref %d, domid %d\n",
> + segs[i].status, ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + rc = -1;
> + }
> + }
> +
> + return rc;
> +}
> +#else
> +static void free_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +#endif
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +616,31 @@ 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 */
> + if (ret == 0) {
> + 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:
> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
>
> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
> - goto err_no_map;
> + 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;
> + }
> + }
> +
Please remove blank this line (it looks a bit odd)
> + } else {
> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
> + goto err;
> + }
> }
>
> ioreq->aio_inflight++;
> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> }
> default:
> /* unknown operation (shouldn't happen -- parse catches this) */
> + if (!ioreq->blkdev->feature_grant_copy) {
> + ioreq_unmap(ioreq);
> + }
Why are you adding this? Is it a bug fix? If so, please explain in the
commit message.
> goto err;
> }
>
> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> return 0;
>
> err:
> - ioreq_unmap(ioreq);
> -err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>
> xen_be_bind_evtchn(&blkdev->xendev);
>
> + blkdev->feature_grant_copy =
> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
> +
> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index bd39287..8e1580d 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
> #endif
> #endif
>
> +/* Xen before 4.8 */
> +
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> +
> +
> +typedef void *xengnttab_grant_copy_segment_t;
> +
> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> + xengnttab_grant_copy_segment_t *segs)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +
> #endif /* QEMU_HW_XEN_COMMON_H */
> --
> 1.9.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 10:41 ` Paulina Szubarczyk
` (3 preceding siblings ...)
(?)
@ 2016-09-07 20:56 ` Stefano Stabellini
2016-09-07 22:00 ` Paulina Szubarczyk
2016-09-07 22:00 ` Paulina Szubarczyk
-1 siblings, 2 replies; 18+ messages in thread
From: Stefano Stabellini @ 2016-09-07 20:56 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
sstabellini, anthony.perard, qemu-devel
On Wed, 7 Sep 2016, 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.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
> Changes since v5:
> -added checking of every interface in the configure file. Based on
> the Roger's comment that xengnttab_map_grant_ref was added prior
> xengnttab_grant_copy, thus do not need to be check again here
> I dropped this check.
>
Thank you Paulina, the patch is good. Thanks for your work! Sorry for
coming in late in the review; I have a couple of minor suggestions
below.
In addition to Anthony's ack, it would be also nice to have Roger's ack
on this patch.
> configure | 55 ++++++++++++++++
> hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 14 ++++
> 3 files changed, 221 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index 4b808f9..3f44d38 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,61 @@ EOF
> /*
> * If we have stable libs the we don't want the libxc compat
> * layers, regardless of what CFLAGS we may have been given.
> + *
> + * Also, check if xengnttab_grant_copy_segment_t is defined and
> + * grant copy operation is implemented.
> + */
> +#undef XC_WANT_COMPAT_EVTCHN_API
> +#undef XC_WANT_COMPAT_GNTTAB_API
> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <xenctrl.h>
> +#include <xenstore.h>
> +#include <xenevtchn.h>
> +#include <xengnttab.h>
> +#include <xenforeignmemory.h>
> +#include <stdint.h>
> +#include <xen/hvm/hvm_info_table.h>
> +#if !defined(HVM_MAX_VCPUS)
> +# error HVM_MAX_VCPUS not defined
> +#endif
> +int main(void) {
> + xc_interface *xc = NULL;
> + xenforeignmemory_handle *xfmem;
> + xenevtchn_handle *xe;
> + xengnttab_handle *xg;
> + xen_domain_handle_t handle;
> + xengnttab_grant_copy_segment_t* seg = NULL;
> +
> + xs_daemon_open();
> +
> + xc = xc_interface_open(0, 0, 0);
> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
> +
> + xfmem = xenforeignmemory_open(0, 0);
> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
> +
> + xe = xenevtchn_open(0, 0);
> + xenevtchn_fd(xe);
> +
> + xg = xengnttab_open(0, 0);
> + xengnttab_grant_copy(xg, 0, seg);
> +
> + return 0;
> +}
> +EOF
> + compile_prog "" "$xen_libs $xen_stable_libs"
> + then
> + xen_ctrl_version=480
> + xen=yes
> + elif
> + cat > $TMPC <<EOF &&
> +/*
> + * If we have stable libs the we don't want the libxc compat
> + * layers, regardless of what CFLAGS we may have been given.
> */
> #undef XC_WANT_COMPAT_EVTCHN_API
> #undef XC_WANT_COMPAT_GNTTAB_API
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3b8ad33..3739e13 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -119,6 +119,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;
> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
see that Anthony suggested it for a good reason. The only alternartive I
can think of would be to introduce two static inline functions in
xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
also OK.
> +static void free_buffers(struct ioreq *ioreq)
Please name this function ioreq_free_copy_buffers to make it clearer
that has to do with the same buffers initialized below.
> +{
> + int i;
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = NULL;
> + }
> +
> + qemu_vfree(ioreq->pages);
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + int i;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = ioreq->page[i];
> + }
> +
> + return 0;
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
Please name this function in a way that makes it clear that it has
something to do with grant copies. Like for example ioreq_grant_copy.
> +{
> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> + int i, count, rc;
> + int64_t file_blk = ioreq->blkdev->file_blk;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + count = ioreq->v.niov;
> +
> + for (i = 0; i < count; i++) {
> +
> + if (ioreq->req.operation == BLKIF_OP_READ) {
> + segs[i].flags = GNTCOPY_dest_gref;
> + segs[i].dest.foreign.ref = ioreq->refs[i];
> + segs[i].dest.foreign.domid = ioreq->domids[i];
> + segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
> + } else {
> + segs[i].flags = GNTCOPY_source_gref;
> + segs[i].source.foreign.ref = ioreq->refs[i];
> + segs[i].source.foreign.domid = ioreq->domids[i];
> + segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
> + }
> + segs[i].len = (ioreq->req.seg[i].last_sect
> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
> +
> + }
> +
> + rc = xengnttab_grant_copy(gnt, count, segs);
> +
> + if (rc) {
> + xen_be_printf(&ioreq->blkdev->xendev, 0,
> + "failed to copy data %d\n", rc);
> + ioreq->aio_errors++;
> + return -1;
> + }
> +
> + for (i = 0; i < count; i++) {
> + if (segs[i].status != GNTST_okay) {
> + xen_be_printf(&ioreq->blkdev->xendev, 3,
> + "failed to copy data %d for gref %d, domid %d\n",
> + segs[i].status, ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + rc = -1;
> + }
> + }
> +
> + return rc;
> +}
> +#else
> +static void free_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +#endif
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +616,31 @@ 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 */
> + if (ret == 0) {
> + 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:
> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
>
> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
> - goto err_no_map;
> + 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;
> + }
> + }
> +
Please remove blank this line (it looks a bit odd)
> + } else {
> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
> + goto err;
> + }
> }
>
> ioreq->aio_inflight++;
> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> }
> default:
> /* unknown operation (shouldn't happen -- parse catches this) */
> + if (!ioreq->blkdev->feature_grant_copy) {
> + ioreq_unmap(ioreq);
> + }
Why are you adding this? Is it a bug fix? If so, please explain in the
commit message.
> goto err;
> }
>
> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> return 0;
>
> err:
> - ioreq_unmap(ioreq);
> -err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>
> xen_be_bind_evtchn(&blkdev->xendev);
>
> + blkdev->feature_grant_copy =
> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
> +
> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index bd39287..8e1580d 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
> #endif
> #endif
>
> +/* Xen before 4.8 */
> +
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> +
> +
> +typedef void *xengnttab_grant_copy_segment_t;
> +
> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> + xengnttab_grant_copy_segment_t *segs)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +
> #endif /* QEMU_HW_XEN_COMMON_H */
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 20:56 ` [Qemu-devel] " Stefano Stabellini
@ 2016-09-07 22:00 ` Paulina Szubarczyk
2016-09-08 18:40 ` Stefano Stabellini
` (2 more replies)
2016-09-07 22:00 ` Paulina Szubarczyk
1 sibling, 3 replies; 18+ messages in thread
From: Paulina Szubarczyk @ 2016-09-07 22:00 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
anthony.perard, qemu-devel
On 09/07/2016 10:56 PM, Stefano Stabellini wrote:
> On Wed, 7 Sep 2016, 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.
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>> Changes since v5:
>> -added checking of every interface in the configure file. Based on
>> the Roger's comment that xengnttab_map_grant_ref was added prior
>> xengnttab_grant_copy, thus do not need to be check again here
>> I dropped this check.
>>
>
> Thank you Paulina, the patch is good. Thanks for your work! Sorry for
> coming in late in the review; I have a couple of minor suggestions
> below.
>
It had not been possible without all the help I have got. I am very
grateful for it.
> In addition to Anthony's ack, it would be also nice to have Roger's ack
> on this patch.
>
>
>> configure | 55 ++++++++++++++++
>> hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/xen/xen_common.h | 14 ++++
>> 3 files changed, 221 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 4b808f9..3f44d38 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1956,6 +1956,61 @@ EOF
>> /*
>> * If we have stable libs the we don't want the libxc compat
>> * layers, regardless of what CFLAGS we may have been given.
>> + *
>> + * Also, check if xengnttab_grant_copy_segment_t is defined and
>> + * grant copy operation is implemented.
>> + */
>> +#undef XC_WANT_COMPAT_EVTCHN_API
>> +#undef XC_WANT_COMPAT_GNTTAB_API
>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include <xenctrl.h>
>> +#include <xenstore.h>
>> +#include <xenevtchn.h>
>> +#include <xengnttab.h>
>> +#include <xenforeignmemory.h>
>> +#include <stdint.h>
>> +#include <xen/hvm/hvm_info_table.h>
>> +#if !defined(HVM_MAX_VCPUS)
>> +# error HVM_MAX_VCPUS not defined
>> +#endif
>> +int main(void) {
>> + xc_interface *xc = NULL;
>> + xenforeignmemory_handle *xfmem;
>> + xenevtchn_handle *xe;
>> + xengnttab_handle *xg;
>> + xen_domain_handle_t handle;
>> + xengnttab_grant_copy_segment_t* seg = NULL;
>> +
>> + xs_daemon_open();
>> +
>> + xc = xc_interface_open(0, 0, 0);
>> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
>> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
>> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
>> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
>> +
>> + xfmem = xenforeignmemory_open(0, 0);
>> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
>> +
>> + xe = xenevtchn_open(0, 0);
>> + xenevtchn_fd(xe);
>> +
>> + xg = xengnttab_open(0, 0);
>> + xengnttab_grant_copy(xg, 0, seg);
>> +
>> + return 0;
>> +}
>> +EOF
>> + compile_prog "" "$xen_libs $xen_stable_libs"
>> + then
>> + xen_ctrl_version=480
>> + xen=yes
>> + elif
>> + cat > $TMPC <<EOF &&
>> +/*
>> + * If we have stable libs the we don't want the libxc compat
>> + * layers, regardless of what CFLAGS we may have been given.
>> */
>> #undef XC_WANT_COMPAT_EVTCHN_API
>> #undef XC_WANT_COMPAT_GNTTAB_API
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 3b8ad33..3739e13 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -119,6 +119,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;
>> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
>> return 0;
>> }
>>
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
>
> In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
> see that Anthony suggested it for a good reason. The only alternartive I
> can think of would be to introduce two static inline functions in
> xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
> also OK.
>
The functions take as parameters pointers to struct ioreq defined in
xen_disk.c which members are used to fill the
xengnttab_grant_copy_segment_t. There would be some overhead to move the
functions to the header.
>
>> +static void free_buffers(struct ioreq *ioreq)
>
> Please name this function ioreq_free_copy_buffers to make it clearer
> that has to do with the same buffers initialized below.
>
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = NULL;
>> + }
>> +
>> + qemu_vfree(ioreq->pages);
>> +}
>> +
>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>> +{
>> + int i;
>> +
>> + if (ioreq->v.niov == 0) {
>> + return 0;
>> + }
>> +
>> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>> + ioreq->v.iov[i].iov_base = ioreq->page[i];
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ioreq_copy(struct ioreq *ioreq)
>
> Please name this function in a way that makes it clear that it has
> something to do with grant copies. Like for example ioreq_grant_copy.
>
I will change the names of both functions and remove the blank line
pointed below.
>
>> +{
>> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
>> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>> + int i, count, rc;
>> + int64_t file_blk = ioreq->blkdev->file_blk;
>> +
>> + if (ioreq->v.niov == 0) {
>> + return 0;
>> + }
>> +
>> + count = ioreq->v.niov;
>> +
>> + for (i = 0; i < count; i++) {
>> +
>> + if (ioreq->req.operation == BLKIF_OP_READ) {
>> + segs[i].flags = GNTCOPY_dest_gref;
>> + segs[i].dest.foreign.ref = ioreq->refs[i];
>> + segs[i].dest.foreign.domid = ioreq->domids[i];
>> + segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
>> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
>> + } else {
>> + segs[i].flags = GNTCOPY_source_gref;
>> + segs[i].source.foreign.ref = ioreq->refs[i];
>> + segs[i].source.foreign.domid = ioreq->domids[i];
>> + segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
>> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
>> + }
>> + segs[i].len = (ioreq->req.seg[i].last_sect
>> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
>> +
>> + }
>> +
>> + rc = xengnttab_grant_copy(gnt, count, segs);
>> +
>> + if (rc) {
>> + xen_be_printf(&ioreq->blkdev->xendev, 0,
>> + "failed to copy data %d\n", rc);
>> + ioreq->aio_errors++;
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < count; i++) {
>> + if (segs[i].status != GNTST_okay) {
>> + xen_be_printf(&ioreq->blkdev->xendev, 3,
>> + "failed to copy data %d for gref %d, domid %d\n",
>> + segs[i].status, ioreq->refs[i], ioreq->domids[i]);
>> + ioreq->aio_errors++;
>> + rc = -1;
>> + }
>> + }
>> +
>> + return rc;
>> +}
>> +#else
>> +static void free_buffers(struct ioreq *ioreq)
>> +{
>> + abort();
>> +}
>> +
>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>> +{
>> + abort();
>> +}
>> +
>> +static int ioreq_copy(struct ioreq *ioreq)
>> +{
>> + abort();
>> +}
>> +#endif
>> +
>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>
>> static void qemu_aio_complete(void *opaque, int ret)
>> @@ -511,8 +616,31 @@ 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 */
>> + if (ret == 0) {
>> + 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:
>> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>> {
>> struct XenBlkDev *blkdev = ioreq->blkdev;
>>
>> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
>> - goto err_no_map;
>> + 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;
>> + }
>> + }
>> +
>
> Please remove blank this line (it looks a bit odd)
>
>
>> + } else {
>> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
>> + goto err;
>> + }
>> }
>>
>> ioreq->aio_inflight++;
>> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>> }
>> default:
>> /* unknown operation (shouldn't happen -- parse catches this) */
>> + if (!ioreq->blkdev->feature_grant_copy) {
>> + ioreq_unmap(ioreq);
>> + }
>
> Why are you adding this? Is it a bug fix? If so, please explain in the
> commit message.
>
It is not a bug fix. In the ioreq_runio_qemu_aio there were two error
labels 'err_no_map' and 'err' each of them used once. So before the
patch the labels are looking this way:
err:
ioreq_unmap(ioreq);
err_no_map:
ioreq_finish(ioreq);
ioreq->status = BLKIF_RSP_ERROR;
return -1;
I removed the 'err_no_map' label and from the 'err' section I removed
the ioreq_unmap. The 'err' label was previously used in that default
section of the switch because the grant_map is called regardless the
ioreq->req.operation.
In the patch, there is no need for any special behavior for grant_copy,
because buffers were not allocated in this case, so there is only jump
to 'err'.
An advantage of this change is one unified error path for both grant
operations.
>
>> goto err;
>> }
>>
>> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>> return 0;
>>
>> err:
>> - ioreq_unmap(ioreq);
>> -err_no_map:
>> ioreq_finish(ioreq);
>> ioreq->status = BLKIF_RSP_ERROR;
>> return -1;
>> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>>
>> xen_be_bind_evtchn(&blkdev->xendev);
>>
>> + blkdev->feature_grant_copy =
>> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
>> +
>> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
>> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
>> index bd39287..8e1580d 100644
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
>> #endif
>> #endif
>>
>> +/* Xen before 4.8 */
>> +
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
>> +
>> +
>> +typedef void *xengnttab_grant_copy_segment_t;
>> +
>> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
>> + xengnttab_grant_copy_segment_t *segs)
>> +{
>> + return -ENOSYS;
>> +}
>> +#endif
>> +
>> #endif /* QEMU_HW_XEN_COMMON_H */
>> --
>> 1.9.1
>>
Paulina
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 22:00 ` Paulina Szubarczyk
@ 2016-09-08 18:40 ` Stefano Stabellini
2016-09-08 18:40 ` [Qemu-devel] " Stefano Stabellini
2016-09-09 12:32 ` Paulina Szubarczyk
2 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2016-09-08 18:40 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: Stefano Stabellini, wei.liu2, ian.jackson, qemu-devel,
david.vrabel, anthony.perard, xen-devel, roger.pau
On Thu, 8 Sep 2016, Paulina Szubarczyk wrote:
> > > @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> > > }
> > > default:
> > > /* unknown operation (shouldn't happen -- parse catches this) */
> > > + if (!ioreq->blkdev->feature_grant_copy) {
> > > + ioreq_unmap(ioreq);
> > > + }
> >
> > Why are you adding this? Is it a bug fix? If so, please explain in the
> > commit message.
> >
> It is not a bug fix. In the ioreq_runio_qemu_aio there were two error labels
> 'err_no_map' and 'err' each of them used once. So before the patch the labels
> are looking this way:
> err:
> ioreq_unmap(ioreq);
> err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
>
> I removed the 'err_no_map' label and from the 'err' section I removed the
> ioreq_unmap. The 'err' label was previously used in that default section of
> the switch because the grant_map is called regardless the
> ioreq->req.operation.
>
> In the patch, there is no need for any special behavior for grant_copy,
> because buffers were not allocated in this case, so there is only jump to
> 'err'.
>
> An advantage of this change is one unified error path for both grant
> operations.
Thanks for the explanation, it looks fine.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 22:00 ` Paulina Szubarczyk
2016-09-08 18:40 ` Stefano Stabellini
@ 2016-09-08 18:40 ` Stefano Stabellini
2016-09-09 12:32 ` Paulina Szubarczyk
2 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2016-09-08 18:40 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: Stefano Stabellini, xen-devel, roger.pau, wei.liu2, ian.jackson,
david.vrabel, anthony.perard, qemu-devel
On Thu, 8 Sep 2016, Paulina Szubarczyk wrote:
> > > @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> > > }
> > > default:
> > > /* unknown operation (shouldn't happen -- parse catches this) */
> > > + if (!ioreq->blkdev->feature_grant_copy) {
> > > + ioreq_unmap(ioreq);
> > > + }
> >
> > Why are you adding this? Is it a bug fix? If so, please explain in the
> > commit message.
> >
> It is not a bug fix. In the ioreq_runio_qemu_aio there were two error labels
> 'err_no_map' and 'err' each of them used once. So before the patch the labels
> are looking this way:
> err:
> ioreq_unmap(ioreq);
> err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
>
> I removed the 'err_no_map' label and from the 'err' section I removed the
> ioreq_unmap. The 'err' label was previously used in that default section of
> the switch because the grant_map is called regardless the
> ioreq->req.operation.
>
> In the patch, there is no need for any special behavior for grant_copy,
> because buffers were not allocated in this case, so there is only jump to
> 'err'.
>
> An advantage of this change is one unified error path for both grant
> operations.
Thanks for the explanation, it looks fine.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 22:00 ` Paulina Szubarczyk
@ 2016-09-09 12:32 ` Paulina Szubarczyk
2016-09-08 18:40 ` [Qemu-devel] " Stefano Stabellini
2016-09-09 12:32 ` Paulina Szubarczyk
2 siblings, 0 replies; 18+ messages in thread
From: Paulina Szubarczyk @ 2016-09-09 12:32 UTC (permalink / raw)
To: roger.pau
Cc: Stefano Stabellini, xen-devel, wei.liu2, ian.jackson,
david.vrabel, anthony.perard, qemu-devel
On 09/08/2016 12:00 AM, Paulina Szubarczyk wrote:
>
> On 09/07/2016 10:56 PM, Stefano Stabellini wrote:
>> On Wed, 7 Sep 2016, 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.
>>>
>>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>>> ---
>>> Changes since v5:
>>> -added checking of every interface in the configure file. Based on
>>> the Roger's comment that xengnttab_map_grant_ref was added prior
>>> xengnttab_grant_copy, thus do not need to be check again here
>>> I dropped this check.
>>>
>>
>> Thank you Paulina, the patch is good. Thanks for your work! Sorry for
>> coming in late in the review; I have a couple of minor suggestions
>> below.
>>
> It had not been possible without all the help I have got. I am very
> grateful for it.
>
>> In addition to Anthony's ack, it would be also nice to have Roger's ack
>> on this patch.
>>
Roger, will you need more time to take a look at the patch or may I send
a new version with applied Stefano comments?
>>
>>> configure | 55 ++++++++++++++++
>>> hw/block/xen_disk.c | 157
>>> ++++++++++++++++++++++++++++++++++++++++++--
>>> include/hw/xen/xen_common.h | 14 ++++
>>> 3 files changed, 221 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 4b808f9..3f44d38 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -1956,6 +1956,61 @@ EOF
>>> /*
>>> * If we have stable libs the we don't want the libxc compat
>>> * layers, regardless of what CFLAGS we may have been given.
>>> + *
>>> + * Also, check if xengnttab_grant_copy_segment_t is defined and
>>> + * grant copy operation is implemented.
>>> + */
>>> +#undef XC_WANT_COMPAT_EVTCHN_API
>>> +#undef XC_WANT_COMPAT_GNTTAB_API
>>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>>> +#include <xenctrl.h>
>>> +#include <xenstore.h>
>>> +#include <xenevtchn.h>
>>> +#include <xengnttab.h>
>>> +#include <xenforeignmemory.h>
>>> +#include <stdint.h>
>>> +#include <xen/hvm/hvm_info_table.h>
>>> +#if !defined(HVM_MAX_VCPUS)
>>> +# error HVM_MAX_VCPUS not defined
>>> +#endif
>>> +int main(void) {
>>> + xc_interface *xc = NULL;
>>> + xenforeignmemory_handle *xfmem;
>>> + xenevtchn_handle *xe;
>>> + xengnttab_handle *xg;
>>> + xen_domain_handle_t handle;
>>> + xengnttab_grant_copy_segment_t* seg = NULL;
>>> +
>>> + xs_daemon_open();
>>> +
>>> + xc = xc_interface_open(0, 0, 0);
>>> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>>> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
>>> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
>>> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC,
>>> NULL);
>>> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
>>> +
>>> + xfmem = xenforeignmemory_open(0, 0);
>>> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
>>> +
>>> + xe = xenevtchn_open(0, 0);
>>> + xenevtchn_fd(xe);
>>> +
>>> + xg = xengnttab_open(0, 0);
>>> + xengnttab_grant_copy(xg, 0, seg);
>>> +
>>> + return 0;
>>> +}
>>> +EOF
>>> + compile_prog "" "$xen_libs $xen_stable_libs"
>>> + then
>>> + xen_ctrl_version=480
>>> + xen=yes
>>> + elif
>>> + cat > $TMPC <<EOF &&
>>> +/*
>>> + * If we have stable libs the we don't want the libxc compat
>>> + * layers, regardless of what CFLAGS we may have been given.
>>> */
>>> #undef XC_WANT_COMPAT_EVTCHN_API
>>> #undef XC_WANT_COMPAT_GNTTAB_API
>>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>>> index 3b8ad33..3739e13 100644
>>> --- a/hw/block/xen_disk.c
>>> +++ b/hw/block/xen_disk.c
>>> @@ -119,6 +119,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;
>>> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
>>> return 0;
>>> }
>>>
>>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
>>
>> In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
>> see that Anthony suggested it for a good reason. The only alternartive I
>> can think of would be to introduce two static inline functions in
>> xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
>> also OK.
>>
> The functions take as parameters pointers to struct ioreq defined in
> xen_disk.c which members are used to fill the
> xengnttab_grant_copy_segment_t. There would be some overhead to move the
> functions to the header.
>>
>>> +static void free_buffers(struct ioreq *ioreq)
>>
>> Please name this function ioreq_free_copy_buffers to make it clearer
>> that has to do with the same buffers initialized below.
>>
>>
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ioreq->v.niov; i++) {
>>> + ioreq->page[i] = NULL;
>>> + }
>>> +
>>> + qemu_vfree(ioreq->pages);
>>> +}
>>> +
>>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>>> +{
>>> + int i;
>>> +
>>> + if (ioreq->v.niov == 0) {
>>> + return 0;
>>> + }
>>> +
>>> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov *
>>> XC_PAGE_SIZE);
>>> +
>>> + for (i = 0; i < ioreq->v.niov; i++) {
>>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>>> + ioreq->v.iov[i].iov_base = ioreq->page[i];
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ioreq_copy(struct ioreq *ioreq)
>>
>> Please name this function in a way that makes it clear that it has
>> something to do with grant copies. Like for example ioreq_grant_copy.
>>
> I will change the names of both functions and remove the blank line
> pointed below.
>>
>>> +{
>>> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
>>> + xengnttab_grant_copy_segment_t
>>> segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>>> + int i, count, rc;
>>> + int64_t file_blk = ioreq->blkdev->file_blk;
>>> +
>>> + if (ioreq->v.niov == 0) {
>>> + return 0;
>>> + }
>>> +
>>> + count = ioreq->v.niov;
>>> +
>>> + for (i = 0; i < count; i++) {
>>> +
>>> + if (ioreq->req.operation == BLKIF_OP_READ) {
>>> + segs[i].flags = GNTCOPY_dest_gref;
>>> + segs[i].dest.foreign.ref = ioreq->refs[i];
>>> + segs[i].dest.foreign.domid = ioreq->domids[i];
>>> + segs[i].dest.foreign.offset =
>>> ioreq->req.seg[i].first_sect * file_blk;
>>> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
>>> + } else {
>>> + segs[i].flags = GNTCOPY_source_gref;
>>> + segs[i].source.foreign.ref = ioreq->refs[i];
>>> + segs[i].source.foreign.domid = ioreq->domids[i];
>>> + segs[i].source.foreign.offset =
>>> ioreq->req.seg[i].first_sect * file_blk;
>>> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
>>> + }
>>> + segs[i].len = (ioreq->req.seg[i].last_sect
>>> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
>>> +
>>> + }
>>> +
>>> + rc = xengnttab_grant_copy(gnt, count, segs);
>>> +
>>> + if (rc) {
>>> + xen_be_printf(&ioreq->blkdev->xendev, 0,
>>> + "failed to copy data %d\n", rc);
>>> + ioreq->aio_errors++;
>>> + return -1;
>>> + }
>>> +
>>> + for (i = 0; i < count; i++) {
>>> + if (segs[i].status != GNTST_okay) {
>>> + xen_be_printf(&ioreq->blkdev->xendev, 3,
>>> + "failed to copy data %d for gref %d, domid
>>> %d\n",
>>> + segs[i].status, ioreq->refs[i],
>>> ioreq->domids[i]);
>>> + ioreq->aio_errors++;
>>> + rc = -1;
>>> + }
>>> + }
>>> +
>>> + return rc;
>>> +}
>>> +#else
>>> +static void free_buffers(struct ioreq *ioreq)
>>> +{
>>> + abort();
>>> +}
>>> +
>>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>>> +{
>>> + abort();
>>> +}
>>> +
>>> +static int ioreq_copy(struct ioreq *ioreq)
>>> +{
>>> + abort();
>>> +}
>>> +#endif
>>> +
>>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>>
>>> static void qemu_aio_complete(void *opaque, int ret)
>>> @@ -511,8 +616,31 @@ 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 */
>>> + if (ret == 0) {
>>> + 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:
>>> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq
>>> *ioreq)
>>> {
>>> struct XenBlkDev *blkdev = ioreq->blkdev;
>>>
>>> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
>>> - goto err_no_map;
>>> + 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;
>>> + }
>>> + }
>>> +
>>
>> Please remove blank this line (it looks a bit odd)
>>
>>
>>> + } else {
>>> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
>>> + goto err;
>>> + }
>>> }
>>>
>>> ioreq->aio_inflight++;
>>> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>>> }
>>> default:
>>> /* unknown operation (shouldn't happen -- parse catches
>>> this) */
>>> + if (!ioreq->blkdev->feature_grant_copy) {
>>> + ioreq_unmap(ioreq);
>>> + }
>>
>> Why are you adding this? Is it a bug fix? If so, please explain in the
>> commit message.
>>
> It is not a bug fix. In the ioreq_runio_qemu_aio there were two error
> labels 'err_no_map' and 'err' each of them used once. So before the
> patch the labels are looking this way:
> err:
> ioreq_unmap(ioreq);
> err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
>
> I removed the 'err_no_map' label and from the 'err' section I removed
> the ioreq_unmap. The 'err' label was previously used in that default
> section of the switch because the grant_map is called regardless the
> ioreq->req.operation.
>
> In the patch, there is no need for any special behavior for grant_copy,
> because buffers were not allocated in this case, so there is only jump
> to 'err'.
>
> An advantage of this change is one unified error path for both grant
> operations.
>>
>>> goto err;
>>> }
>>>
>>> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>>> return 0;
>>>
>>> err:
>>> - ioreq_unmap(ioreq);
>>> -err_no_map:
>>> ioreq_finish(ioreq);
>>> ioreq->status = BLKIF_RSP_ERROR;
>>> return -1;
>>> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>>>
>>> xen_be_bind_evtchn(&blkdev->xendev);
>>>
>>> + blkdev->feature_grant_copy =
>>> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0,
>>> NULL) == 0);
>>> +
>>> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
>>> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
>>> index bd39287..8e1580d 100644
>>> --- a/include/hw/xen/xen_common.h
>>> +++ b/include/hw/xen/xen_common.h
>>> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface
>>> *xc, uint32_t ssidref,
>>> #endif
>>> #endif
>>>
>>> +/* Xen before 4.8 */
>>> +
>>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
>>> +
>>> +
>>> +typedef void *xengnttab_grant_copy_segment_t;
>>> +
>>> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt,
>>> uint32_t count,
>>> +
>>> xengnttab_grant_copy_segment_t *segs)
>>> +{
>>> + return -ENOSYS;
>>> +}
>>> +#endif
>>> +
>>> #endif /* QEMU_HW_XEN_COMMON_H */
>>> --
>>> 1.9.1
>>>
>
> Paulina
Paulina
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
@ 2016-09-09 12:32 ` Paulina Szubarczyk
0 siblings, 0 replies; 18+ messages in thread
From: Paulina Szubarczyk @ 2016-09-09 12:32 UTC (permalink / raw)
To: roger.pau
Cc: Stefano Stabellini, wei.liu2, ian.jackson, qemu-devel,
david.vrabel, anthony.perard, xen-devel
On 09/08/2016 12:00 AM, Paulina Szubarczyk wrote:
>
> On 09/07/2016 10:56 PM, Stefano Stabellini wrote:
>> On Wed, 7 Sep 2016, 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.
>>>
>>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>>> ---
>>> Changes since v5:
>>> -added checking of every interface in the configure file. Based on
>>> the Roger's comment that xengnttab_map_grant_ref was added prior
>>> xengnttab_grant_copy, thus do not need to be check again here
>>> I dropped this check.
>>>
>>
>> Thank you Paulina, the patch is good. Thanks for your work! Sorry for
>> coming in late in the review; I have a couple of minor suggestions
>> below.
>>
> It had not been possible without all the help I have got. I am very
> grateful for it.
>
>> In addition to Anthony's ack, it would be also nice to have Roger's ack
>> on this patch.
>>
Roger, will you need more time to take a look at the patch or may I send
a new version with applied Stefano comments?
>>
>>> configure | 55 ++++++++++++++++
>>> hw/block/xen_disk.c | 157
>>> ++++++++++++++++++++++++++++++++++++++++++--
>>> include/hw/xen/xen_common.h | 14 ++++
>>> 3 files changed, 221 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 4b808f9..3f44d38 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -1956,6 +1956,61 @@ EOF
>>> /*
>>> * If we have stable libs the we don't want the libxc compat
>>> * layers, regardless of what CFLAGS we may have been given.
>>> + *
>>> + * Also, check if xengnttab_grant_copy_segment_t is defined and
>>> + * grant copy operation is implemented.
>>> + */
>>> +#undef XC_WANT_COMPAT_EVTCHN_API
>>> +#undef XC_WANT_COMPAT_GNTTAB_API
>>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>>> +#include <xenctrl.h>
>>> +#include <xenstore.h>
>>> +#include <xenevtchn.h>
>>> +#include <xengnttab.h>
>>> +#include <xenforeignmemory.h>
>>> +#include <stdint.h>
>>> +#include <xen/hvm/hvm_info_table.h>
>>> +#if !defined(HVM_MAX_VCPUS)
>>> +# error HVM_MAX_VCPUS not defined
>>> +#endif
>>> +int main(void) {
>>> + xc_interface *xc = NULL;
>>> + xenforeignmemory_handle *xfmem;
>>> + xenevtchn_handle *xe;
>>> + xengnttab_handle *xg;
>>> + xen_domain_handle_t handle;
>>> + xengnttab_grant_copy_segment_t* seg = NULL;
>>> +
>>> + xs_daemon_open();
>>> +
>>> + xc = xc_interface_open(0, 0, 0);
>>> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>>> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
>>> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
>>> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC,
>>> NULL);
>>> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
>>> +
>>> + xfmem = xenforeignmemory_open(0, 0);
>>> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
>>> +
>>> + xe = xenevtchn_open(0, 0);
>>> + xenevtchn_fd(xe);
>>> +
>>> + xg = xengnttab_open(0, 0);
>>> + xengnttab_grant_copy(xg, 0, seg);
>>> +
>>> + return 0;
>>> +}
>>> +EOF
>>> + compile_prog "" "$xen_libs $xen_stable_libs"
>>> + then
>>> + xen_ctrl_version=480
>>> + xen=yes
>>> + elif
>>> + cat > $TMPC <<EOF &&
>>> +/*
>>> + * If we have stable libs the we don't want the libxc compat
>>> + * layers, regardless of what CFLAGS we may have been given.
>>> */
>>> #undef XC_WANT_COMPAT_EVTCHN_API
>>> #undef XC_WANT_COMPAT_GNTTAB_API
>>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>>> index 3b8ad33..3739e13 100644
>>> --- a/hw/block/xen_disk.c
>>> +++ b/hw/block/xen_disk.c
>>> @@ -119,6 +119,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;
>>> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
>>> return 0;
>>> }
>>>
>>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
>>
>> In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
>> see that Anthony suggested it for a good reason. The only alternartive I
>> can think of would be to introduce two static inline functions in
>> xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
>> also OK.
>>
> The functions take as parameters pointers to struct ioreq defined in
> xen_disk.c which members are used to fill the
> xengnttab_grant_copy_segment_t. There would be some overhead to move the
> functions to the header.
>>
>>> +static void free_buffers(struct ioreq *ioreq)
>>
>> Please name this function ioreq_free_copy_buffers to make it clearer
>> that has to do with the same buffers initialized below.
>>
>>
>>> +{
>>> + int i;
>>> +
>>> + for (i = 0; i < ioreq->v.niov; i++) {
>>> + ioreq->page[i] = NULL;
>>> + }
>>> +
>>> + qemu_vfree(ioreq->pages);
>>> +}
>>> +
>>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>>> +{
>>> + int i;
>>> +
>>> + if (ioreq->v.niov == 0) {
>>> + return 0;
>>> + }
>>> +
>>> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov *
>>> XC_PAGE_SIZE);
>>> +
>>> + for (i = 0; i < ioreq->v.niov; i++) {
>>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>>> + ioreq->v.iov[i].iov_base = ioreq->page[i];
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int ioreq_copy(struct ioreq *ioreq)
>>
>> Please name this function in a way that makes it clear that it has
>> something to do with grant copies. Like for example ioreq_grant_copy.
>>
> I will change the names of both functions and remove the blank line
> pointed below.
>>
>>> +{
>>> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
>>> + xengnttab_grant_copy_segment_t
>>> segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>>> + int i, count, rc;
>>> + int64_t file_blk = ioreq->blkdev->file_blk;
>>> +
>>> + if (ioreq->v.niov == 0) {
>>> + return 0;
>>> + }
>>> +
>>> + count = ioreq->v.niov;
>>> +
>>> + for (i = 0; i < count; i++) {
>>> +
>>> + if (ioreq->req.operation == BLKIF_OP_READ) {
>>> + segs[i].flags = GNTCOPY_dest_gref;
>>> + segs[i].dest.foreign.ref = ioreq->refs[i];
>>> + segs[i].dest.foreign.domid = ioreq->domids[i];
>>> + segs[i].dest.foreign.offset =
>>> ioreq->req.seg[i].first_sect * file_blk;
>>> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
>>> + } else {
>>> + segs[i].flags = GNTCOPY_source_gref;
>>> + segs[i].source.foreign.ref = ioreq->refs[i];
>>> + segs[i].source.foreign.domid = ioreq->domids[i];
>>> + segs[i].source.foreign.offset =
>>> ioreq->req.seg[i].first_sect * file_blk;
>>> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
>>> + }
>>> + segs[i].len = (ioreq->req.seg[i].last_sect
>>> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
>>> +
>>> + }
>>> +
>>> + rc = xengnttab_grant_copy(gnt, count, segs);
>>> +
>>> + if (rc) {
>>> + xen_be_printf(&ioreq->blkdev->xendev, 0,
>>> + "failed to copy data %d\n", rc);
>>> + ioreq->aio_errors++;
>>> + return -1;
>>> + }
>>> +
>>> + for (i = 0; i < count; i++) {
>>> + if (segs[i].status != GNTST_okay) {
>>> + xen_be_printf(&ioreq->blkdev->xendev, 3,
>>> + "failed to copy data %d for gref %d, domid
>>> %d\n",
>>> + segs[i].status, ioreq->refs[i],
>>> ioreq->domids[i]);
>>> + ioreq->aio_errors++;
>>> + rc = -1;
>>> + }
>>> + }
>>> +
>>> + return rc;
>>> +}
>>> +#else
>>> +static void free_buffers(struct ioreq *ioreq)
>>> +{
>>> + abort();
>>> +}
>>> +
>>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>>> +{
>>> + abort();
>>> +}
>>> +
>>> +static int ioreq_copy(struct ioreq *ioreq)
>>> +{
>>> + abort();
>>> +}
>>> +#endif
>>> +
>>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>>
>>> static void qemu_aio_complete(void *opaque, int ret)
>>> @@ -511,8 +616,31 @@ 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 */
>>> + if (ret == 0) {
>>> + 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:
>>> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq
>>> *ioreq)
>>> {
>>> struct XenBlkDev *blkdev = ioreq->blkdev;
>>>
>>> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
>>> - goto err_no_map;
>>> + 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;
>>> + }
>>> + }
>>> +
>>
>> Please remove blank this line (it looks a bit odd)
>>
>>
>>> + } else {
>>> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
>>> + goto err;
>>> + }
>>> }
>>>
>>> ioreq->aio_inflight++;
>>> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>>> }
>>> default:
>>> /* unknown operation (shouldn't happen -- parse catches
>>> this) */
>>> + if (!ioreq->blkdev->feature_grant_copy) {
>>> + ioreq_unmap(ioreq);
>>> + }
>>
>> Why are you adding this? Is it a bug fix? If so, please explain in the
>> commit message.
>>
> It is not a bug fix. In the ioreq_runio_qemu_aio there were two error
> labels 'err_no_map' and 'err' each of them used once. So before the
> patch the labels are looking this way:
> err:
> ioreq_unmap(ioreq);
> err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
>
> I removed the 'err_no_map' label and from the 'err' section I removed
> the ioreq_unmap. The 'err' label was previously used in that default
> section of the switch because the grant_map is called regardless the
> ioreq->req.operation.
>
> In the patch, there is no need for any special behavior for grant_copy,
> because buffers were not allocated in this case, so there is only jump
> to 'err'.
>
> An advantage of this change is one unified error path for both grant
> operations.
>>
>>> goto err;
>>> }
>>>
>>> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>>> return 0;
>>>
>>> err:
>>> - ioreq_unmap(ioreq);
>>> -err_no_map:
>>> ioreq_finish(ioreq);
>>> ioreq->status = BLKIF_RSP_ERROR;
>>> return -1;
>>> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>>>
>>> xen_be_bind_evtchn(&blkdev->xendev);
>>>
>>> + blkdev->feature_grant_copy =
>>> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0,
>>> NULL) == 0);
>>> +
>>> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
>>> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
>>> index bd39287..8e1580d 100644
>>> --- a/include/hw/xen/xen_common.h
>>> +++ b/include/hw/xen/xen_common.h
>>> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface
>>> *xc, uint32_t ssidref,
>>> #endif
>>> #endif
>>>
>>> +/* Xen before 4.8 */
>>> +
>>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
>>> +
>>> +
>>> +typedef void *xengnttab_grant_copy_segment_t;
>>> +
>>> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt,
>>> uint32_t count,
>>> +
>>> xengnttab_grant_copy_segment_t *segs)
>>> +{
>>> + return -ENOSYS;
>>> +}
>>> +#endif
>>> +
>>> #endif /* QEMU_HW_XEN_COMMON_H */
>>> --
>>> 1.9.1
>>>
>
> Paulina
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 20:56 ` [Qemu-devel] " Stefano Stabellini
2016-09-07 22:00 ` Paulina Szubarczyk
@ 2016-09-07 22:00 ` Paulina Szubarczyk
1 sibling, 0 replies; 18+ messages in thread
From: Paulina Szubarczyk @ 2016-09-07 22:00 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2, ian.jackson, qemu-devel, david.vrabel, anthony.perard,
xen-devel, roger.pau
On 09/07/2016 10:56 PM, Stefano Stabellini wrote:
> On Wed, 7 Sep 2016, 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.
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>> Changes since v5:
>> -added checking of every interface in the configure file. Based on
>> the Roger's comment that xengnttab_map_grant_ref was added prior
>> xengnttab_grant_copy, thus do not need to be check again here
>> I dropped this check.
>>
>
> Thank you Paulina, the patch is good. Thanks for your work! Sorry for
> coming in late in the review; I have a couple of minor suggestions
> below.
>
It had not been possible without all the help I have got. I am very
grateful for it.
> In addition to Anthony's ack, it would be also nice to have Roger's ack
> on this patch.
>
>
>> configure | 55 ++++++++++++++++
>> hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/xen/xen_common.h | 14 ++++
>> 3 files changed, 221 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 4b808f9..3f44d38 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1956,6 +1956,61 @@ EOF
>> /*
>> * If we have stable libs the we don't want the libxc compat
>> * layers, regardless of what CFLAGS we may have been given.
>> + *
>> + * Also, check if xengnttab_grant_copy_segment_t is defined and
>> + * grant copy operation is implemented.
>> + */
>> +#undef XC_WANT_COMPAT_EVTCHN_API
>> +#undef XC_WANT_COMPAT_GNTTAB_API
>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include <xenctrl.h>
>> +#include <xenstore.h>
>> +#include <xenevtchn.h>
>> +#include <xengnttab.h>
>> +#include <xenforeignmemory.h>
>> +#include <stdint.h>
>> +#include <xen/hvm/hvm_info_table.h>
>> +#if !defined(HVM_MAX_VCPUS)
>> +# error HVM_MAX_VCPUS not defined
>> +#endif
>> +int main(void) {
>> + xc_interface *xc = NULL;
>> + xenforeignmemory_handle *xfmem;
>> + xenevtchn_handle *xe;
>> + xengnttab_handle *xg;
>> + xen_domain_handle_t handle;
>> + xengnttab_grant_copy_segment_t* seg = NULL;
>> +
>> + xs_daemon_open();
>> +
>> + xc = xc_interface_open(0, 0, 0);
>> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
>> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
>> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
>> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
>> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
>> +
>> + xfmem = xenforeignmemory_open(0, 0);
>> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
>> +
>> + xe = xenevtchn_open(0, 0);
>> + xenevtchn_fd(xe);
>> +
>> + xg = xengnttab_open(0, 0);
>> + xengnttab_grant_copy(xg, 0, seg);
>> +
>> + return 0;
>> +}
>> +EOF
>> + compile_prog "" "$xen_libs $xen_stable_libs"
>> + then
>> + xen_ctrl_version=480
>> + xen=yes
>> + elif
>> + cat > $TMPC <<EOF &&
>> +/*
>> + * If we have stable libs the we don't want the libxc compat
>> + * layers, regardless of what CFLAGS we may have been given.
>> */
>> #undef XC_WANT_COMPAT_EVTCHN_API
>> #undef XC_WANT_COMPAT_GNTTAB_API
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 3b8ad33..3739e13 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -119,6 +119,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;
>> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
>> return 0;
>> }
>>
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
>
> In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
> see that Anthony suggested it for a good reason. The only alternartive I
> can think of would be to introduce two static inline functions in
> xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
> also OK.
>
The functions take as parameters pointers to struct ioreq defined in
xen_disk.c which members are used to fill the
xengnttab_grant_copy_segment_t. There would be some overhead to move the
functions to the header.
>
>> +static void free_buffers(struct ioreq *ioreq)
>
> Please name this function ioreq_free_copy_buffers to make it clearer
> that has to do with the same buffers initialized below.
>
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = NULL;
>> + }
>> +
>> + qemu_vfree(ioreq->pages);
>> +}
>> +
>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>> +{
>> + int i;
>> +
>> + if (ioreq->v.niov == 0) {
>> + return 0;
>> + }
>> +
>> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>> + ioreq->v.iov[i].iov_base = ioreq->page[i];
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ioreq_copy(struct ioreq *ioreq)
>
> Please name this function in a way that makes it clear that it has
> something to do with grant copies. Like for example ioreq_grant_copy.
>
I will change the names of both functions and remove the blank line
pointed below.
>
>> +{
>> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
>> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
>> + int i, count, rc;
>> + int64_t file_blk = ioreq->blkdev->file_blk;
>> +
>> + if (ioreq->v.niov == 0) {
>> + return 0;
>> + }
>> +
>> + count = ioreq->v.niov;
>> +
>> + for (i = 0; i < count; i++) {
>> +
>> + if (ioreq->req.operation == BLKIF_OP_READ) {
>> + segs[i].flags = GNTCOPY_dest_gref;
>> + segs[i].dest.foreign.ref = ioreq->refs[i];
>> + segs[i].dest.foreign.domid = ioreq->domids[i];
>> + segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
>> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
>> + } else {
>> + segs[i].flags = GNTCOPY_source_gref;
>> + segs[i].source.foreign.ref = ioreq->refs[i];
>> + segs[i].source.foreign.domid = ioreq->domids[i];
>> + segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
>> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
>> + }
>> + segs[i].len = (ioreq->req.seg[i].last_sect
>> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
>> +
>> + }
>> +
>> + rc = xengnttab_grant_copy(gnt, count, segs);
>> +
>> + if (rc) {
>> + xen_be_printf(&ioreq->blkdev->xendev, 0,
>> + "failed to copy data %d\n", rc);
>> + ioreq->aio_errors++;
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < count; i++) {
>> + if (segs[i].status != GNTST_okay) {
>> + xen_be_printf(&ioreq->blkdev->xendev, 3,
>> + "failed to copy data %d for gref %d, domid %d\n",
>> + segs[i].status, ioreq->refs[i], ioreq->domids[i]);
>> + ioreq->aio_errors++;
>> + rc = -1;
>> + }
>> + }
>> +
>> + return rc;
>> +}
>> +#else
>> +static void free_buffers(struct ioreq *ioreq)
>> +{
>> + abort();
>> +}
>> +
>> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
>> +{
>> + abort();
>> +}
>> +
>> +static int ioreq_copy(struct ioreq *ioreq)
>> +{
>> + abort();
>> +}
>> +#endif
>> +
>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>
>> static void qemu_aio_complete(void *opaque, int ret)
>> @@ -511,8 +616,31 @@ 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 */
>> + if (ret == 0) {
>> + 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:
>> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>> {
>> struct XenBlkDev *blkdev = ioreq->blkdev;
>>
>> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
>> - goto err_no_map;
>> + 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;
>> + }
>> + }
>> +
>
> Please remove blank this line (it looks a bit odd)
>
>
>> + } else {
>> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
>> + goto err;
>> + }
>> }
>>
>> ioreq->aio_inflight++;
>> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>> }
>> default:
>> /* unknown operation (shouldn't happen -- parse catches this) */
>> + if (!ioreq->blkdev->feature_grant_copy) {
>> + ioreq_unmap(ioreq);
>> + }
>
> Why are you adding this? Is it a bug fix? If so, please explain in the
> commit message.
>
It is not a bug fix. In the ioreq_runio_qemu_aio there were two error
labels 'err_no_map' and 'err' each of them used once. So before the
patch the labels are looking this way:
err:
ioreq_unmap(ioreq);
err_no_map:
ioreq_finish(ioreq);
ioreq->status = BLKIF_RSP_ERROR;
return -1;
I removed the 'err_no_map' label and from the 'err' section I removed
the ioreq_unmap. The 'err' label was previously used in that default
section of the switch because the grant_map is called regardless the
ioreq->req.operation.
In the patch, there is no need for any special behavior for grant_copy,
because buffers were not allocated in this case, so there is only jump
to 'err'.
An advantage of this change is one unified error path for both grant
operations.
>
>> goto err;
>> }
>>
>> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>> return 0;
>>
>> err:
>> - ioreq_unmap(ioreq);
>> -err_no_map:
>> ioreq_finish(ioreq);
>> ioreq->status = BLKIF_RSP_ERROR;
>> return -1;
>> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>>
>> xen_be_bind_evtchn(&blkdev->xendev);
>>
>> + blkdev->feature_grant_copy =
>> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
>> +
>> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
>> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
>> index bd39287..8e1580d 100644
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
>> #endif
>> #endif
>>
>> +/* Xen before 4.8 */
>> +
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
>> +
>> +
>> +typedef void *xengnttab_grant_copy_segment_t;
>> +
>> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
>> + xengnttab_grant_copy_segment_t *segs)
>> +{
>> + return -ENOSYS;
>> +}
>> +#endif
>> +
>> #endif /* QEMU_HW_XEN_COMMON_H */
>> --
>> 1.9.1
>>
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 10:41 ` Paulina Szubarczyk
` (4 preceding siblings ...)
(?)
@ 2016-09-12 16:06 ` Roger Pau Monné
-1 siblings, 0 replies; 18+ messages in thread
From: Roger Pau Monné @ 2016-09-12 16:06 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel
On Wed, Sep 07, 2016 at 12:41:00PM +0200, 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.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Just a couple of minor comments below, but the implementation looks fine to
me.
> ---
> Changes since v5:
> -added checking of every interface in the configure file. Based on
> the Roger's comment that xengnttab_map_grant_ref was added prior
> xengnttab_grant_copy, thus do not need to be check again here
> I dropped this check.
>
> ---
> configure | 55 ++++++++++++++++
> hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 14 ++++
> 3 files changed, 221 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index 4b808f9..3f44d38 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,61 @@ EOF
> /*
> * If we have stable libs the we don't want the libxc compat
> * layers, regardless of what CFLAGS we may have been given.
> + *
> + * Also, check if xengnttab_grant_copy_segment_t is defined and
> + * grant copy operation is implemented.
> + */
> +#undef XC_WANT_COMPAT_EVTCHN_API
> +#undef XC_WANT_COMPAT_GNTTAB_API
> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <xenctrl.h>
> +#include <xenstore.h>
> +#include <xenevtchn.h>
> +#include <xengnttab.h>
> +#include <xenforeignmemory.h>
> +#include <stdint.h>
> +#include <xen/hvm/hvm_info_table.h>
> +#if !defined(HVM_MAX_VCPUS)
> +# error HVM_MAX_VCPUS not defined
> +#endif
> +int main(void) {
> + xc_interface *xc = NULL;
> + xenforeignmemory_handle *xfmem;
> + xenevtchn_handle *xe;
> + xengnttab_handle *xg;
> + xen_domain_handle_t handle;
> + xengnttab_grant_copy_segment_t* seg = NULL;
> +
> + xs_daemon_open();
> +
> + xc = xc_interface_open(0, 0, 0);
> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
> +
> + xfmem = xenforeignmemory_open(0, 0);
> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
> +
> + xe = xenevtchn_open(0, 0);
> + xenevtchn_fd(xe);
> +
> + xg = xengnttab_open(0, 0);
> + xengnttab_grant_copy(xg, 0, seg);
> +
> + return 0;
> +}
> +EOF
> + compile_prog "" "$xen_libs $xen_stable_libs"
> + then
> + xen_ctrl_version=480
> + xen=yes
> + elif
> + cat > $TMPC <<EOF &&
> +/*
> + * If we have stable libs the we don't want the libxc compat
> + * layers, regardless of what CFLAGS we may have been given.
> */
> #undef XC_WANT_COMPAT_EVTCHN_API
> #undef XC_WANT_COMPAT_GNTTAB_API
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3b8ad33..3739e13 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -119,6 +119,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;
> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
> +
> +static void free_buffers(struct ioreq *ioreq)
> +{
> + int i;
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = NULL;
> + }
> +
> + qemu_vfree(ioreq->pages);
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + int i;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = ioreq->page[i];
> + }
> +
> + return 0;
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
> +{
> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> + int i, count, rc;
> + int64_t file_blk = ioreq->blkdev->file_blk;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + count = ioreq->v.niov;
> +
> + for (i = 0; i < count; i++) {
This newline here...
> +
> + if (ioreq->req.operation == BLKIF_OP_READ) {
> + segs[i].flags = GNTCOPY_dest_gref;
> + segs[i].dest.foreign.ref = ioreq->refs[i];
> + segs[i].dest.foreign.domid = ioreq->domids[i];
> + segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
> + } else {
> + segs[i].flags = GNTCOPY_source_gref;
> + segs[i].source.foreign.ref = ioreq->refs[i];
> + segs[i].source.foreign.domid = ioreq->domids[i];
> + segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
> + }
> + segs[i].len = (ioreq->req.seg[i].last_sect
> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
... and here, are not needed (unless I'm missing something from QEMU coding
style).
> + }
> +
> + rc = xengnttab_grant_copy(gnt, count, segs);
> +
> + if (rc) {
> + xen_be_printf(&ioreq->blkdev->xendev, 0,
> + "failed to copy data %d\n", rc);
> + ioreq->aio_errors++;
> + return -1;
> + }
> +
> + for (i = 0; i < count; i++) {
> + if (segs[i].status != GNTST_okay) {
> + xen_be_printf(&ioreq->blkdev->xendev, 3,
> + "failed to copy data %d for gref %d, domid %d\n",
> + segs[i].status, ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + rc = -1;
> + }
> + }
> +
> + return rc;
> +}
> +#else
> +static void free_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +#endif
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +616,31 @@ 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 */
> + if (ret == 0) {
> + 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:
> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
>
> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
> - goto err_no_map;
> + 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)) {
This looks odd, can't you join this inner if with it's parent? (using &&,
like you did below)
> + free_buffers(ioreq);
> + goto err;
> + }
> + }
> +
> + } else {
> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
> + goto err;
> + }
> }
>
> ioreq->aio_inflight++;
> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> }
> default:
> /* unknown operation (shouldn't happen -- parse catches this) */
> + if (!ioreq->blkdev->feature_grant_copy) {
> + ioreq_unmap(ioreq);
> + }
> goto err;
> }
>
> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> return 0;
>
> err:
> - ioreq_unmap(ioreq);
> -err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>
> xen_be_bind_evtchn(&blkdev->xendev);
>
> + blkdev->feature_grant_copy =
> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
> +
> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index bd39287..8e1580d 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
> #endif
> #endif
>
> +/* Xen before 4.8 */
> +
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> +
> +
> +typedef void *xengnttab_grant_copy_segment_t;
> +
> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> + xengnttab_grant_copy_segment_t *segs)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +
> #endif /* QEMU_HW_XEN_COMMON_H */
> --
> 1.9.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-09-07 10:41 ` Paulina Szubarczyk
` (5 preceding siblings ...)
(?)
@ 2016-09-12 16:06 ` Roger Pau Monné
-1 siblings, 0 replies; 18+ messages in thread
From: Roger Pau Monné @ 2016-09-12 16:06 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, xen-devel
On Wed, Sep 07, 2016 at 12:41:00PM +0200, 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.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Just a couple of minor comments below, but the implementation looks fine to
me.
> ---
> Changes since v5:
> -added checking of every interface in the configure file. Based on
> the Roger's comment that xengnttab_map_grant_ref was added prior
> xengnttab_grant_copy, thus do not need to be check again here
> I dropped this check.
>
> ---
> configure | 55 ++++++++++++++++
> hw/block/xen_disk.c | 157 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 14 ++++
> 3 files changed, 221 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index 4b808f9..3f44d38 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,61 @@ EOF
> /*
> * If we have stable libs the we don't want the libxc compat
> * layers, regardless of what CFLAGS we may have been given.
> + *
> + * Also, check if xengnttab_grant_copy_segment_t is defined and
> + * grant copy operation is implemented.
> + */
> +#undef XC_WANT_COMPAT_EVTCHN_API
> +#undef XC_WANT_COMPAT_GNTTAB_API
> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <xenctrl.h>
> +#include <xenstore.h>
> +#include <xenevtchn.h>
> +#include <xengnttab.h>
> +#include <xenforeignmemory.h>
> +#include <stdint.h>
> +#include <xen/hvm/hvm_info_table.h>
> +#if !defined(HVM_MAX_VCPUS)
> +# error HVM_MAX_VCPUS not defined
> +#endif
> +int main(void) {
> + xc_interface *xc = NULL;
> + xenforeignmemory_handle *xfmem;
> + xenevtchn_handle *xe;
> + xengnttab_handle *xg;
> + xen_domain_handle_t handle;
> + xengnttab_grant_copy_segment_t* seg = NULL;
> +
> + xs_daemon_open();
> +
> + xc = xc_interface_open(0, 0, 0);
> + xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
> + xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
> + xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
> + xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC, NULL);
> + xc_domain_create(xc, 0, handle, 0, NULL, NULL);
> +
> + xfmem = xenforeignmemory_open(0, 0);
> + xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
> +
> + xe = xenevtchn_open(0, 0);
> + xenevtchn_fd(xe);
> +
> + xg = xengnttab_open(0, 0);
> + xengnttab_grant_copy(xg, 0, seg);
> +
> + return 0;
> +}
> +EOF
> + compile_prog "" "$xen_libs $xen_stable_libs"
> + then
> + xen_ctrl_version=480
> + xen=yes
> + elif
> + cat > $TMPC <<EOF &&
> +/*
> + * If we have stable libs the we don't want the libxc compat
> + * layers, regardless of what CFLAGS we may have been given.
> */
> #undef XC_WANT_COMPAT_EVTCHN_API
> #undef XC_WANT_COMPAT_GNTTAB_API
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 3b8ad33..3739e13 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -119,6 +119,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;
> @@ -489,6 +492,108 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480
> +
> +static void free_buffers(struct ioreq *ioreq)
> +{
> + int i;
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = NULL;
> + }
> +
> + qemu_vfree(ioreq->pages);
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + int i;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov * XC_PAGE_SIZE);
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = ioreq->page[i];
> + }
> +
> + return 0;
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
> +{
> + xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
> + xengnttab_grant_copy_segment_t segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
> + int i, count, rc;
> + int64_t file_blk = ioreq->blkdev->file_blk;
> +
> + if (ioreq->v.niov == 0) {
> + return 0;
> + }
> +
> + count = ioreq->v.niov;
> +
> + for (i = 0; i < count; i++) {
This newline here...
> +
> + if (ioreq->req.operation == BLKIF_OP_READ) {
> + segs[i].flags = GNTCOPY_dest_gref;
> + segs[i].dest.foreign.ref = ioreq->refs[i];
> + segs[i].dest.foreign.domid = ioreq->domids[i];
> + segs[i].dest.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].source.virt = ioreq->v.iov[i].iov_base;
> + } else {
> + segs[i].flags = GNTCOPY_source_gref;
> + segs[i].source.foreign.ref = ioreq->refs[i];
> + segs[i].source.foreign.domid = ioreq->domids[i];
> + segs[i].source.foreign.offset = ioreq->req.seg[i].first_sect * file_blk;
> + segs[i].dest.virt = ioreq->v.iov[i].iov_base;
> + }
> + segs[i].len = (ioreq->req.seg[i].last_sect
> + - ioreq->req.seg[i].first_sect + 1) * file_blk;
... and here, are not needed (unless I'm missing something from QEMU coding
style).
> + }
> +
> + rc = xengnttab_grant_copy(gnt, count, segs);
> +
> + if (rc) {
> + xen_be_printf(&ioreq->blkdev->xendev, 0,
> + "failed to copy data %d\n", rc);
> + ioreq->aio_errors++;
> + return -1;
> + }
> +
> + for (i = 0; i < count; i++) {
> + if (segs[i].status != GNTST_okay) {
> + xen_be_printf(&ioreq->blkdev->xendev, 3,
> + "failed to copy data %d for gref %d, domid %d\n",
> + segs[i].status, ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + rc = -1;
> + }
> + }
> +
> + return rc;
> +}
> +#else
> +static void free_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_init_copy_buffers(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +
> +static int ioreq_copy(struct ioreq *ioreq)
> +{
> + abort();
> +}
> +#endif
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +616,31 @@ 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 */
> + if (ret == 0) {
> + 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:
> @@ -538,8 +666,20 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> {
> struct XenBlkDev *blkdev = ioreq->blkdev;
>
> - if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
> - goto err_no_map;
> + 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)) {
This looks odd, can't you join this inner if with it's parent? (using &&,
like you did below)
> + free_buffers(ioreq);
> + goto err;
> + }
> + }
> +
> + } else {
> + if (ioreq->req.nr_segments && ioreq_map(ioreq)) {
> + goto err;
> + }
> }
>
> ioreq->aio_inflight++;
> @@ -582,6 +722,9 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> }
> default:
> /* unknown operation (shouldn't happen -- parse catches this) */
> + if (!ioreq->blkdev->feature_grant_copy) {
> + ioreq_unmap(ioreq);
> + }
> goto err;
> }
>
> @@ -590,8 +733,6 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
> return 0;
>
> err:
> - ioreq_unmap(ioreq);
> -err_no_map:
> ioreq_finish(ioreq);
> ioreq->status = BLKIF_RSP_ERROR;
> return -1;
> @@ -1032,6 +1173,12 @@ static int blk_connect(struct XenDevice *xendev)
>
> xen_be_bind_evtchn(&blkdev->xendev);
>
> + blkdev->feature_grant_copy =
> + (xengnttab_grant_copy(blkdev->xendev.gnttabdev, 0, NULL) == 0);
> +
> + xen_be_printf(&blkdev->xendev, 3, "grant copy operation %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,
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index bd39287..8e1580d 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -424,4 +424,18 @@ static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
> #endif
> #endif
>
> +/* Xen before 4.8 */
> +
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> +
> +
> +typedef void *xengnttab_grant_copy_segment_t;
> +
> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> + xengnttab_grant_copy_segment_t *segs)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +
> #endif /* QEMU_HW_XEN_COMMON_H */
> --
> 1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread