* [Qemu-devel] [PATCH v4 0/2] qemu-qdisk: Implementation of grant copy operation.
@ 2016-08-02 14:06 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-02 14:06 UTC (permalink / raw)
To: xen-devel
Cc: roger.pau, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel, Paulina Szubarczyk
Hi,
It is a proposition for implementation of grant copy operation in qemu-qdisk and interface in libxc/libs.
Changes since v3:
Interface:
- revert to cast from xengnttab_grant_copy_segment_t
to ioctl_gntdev_grant_copy.
- added compile-time check to compare the libs
xengnttab_grant_copy_segment_t with the ioctl structure.
The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON in libs/gnttab.
qemu-qdisk:
- qemu_memalign/qemu_free is used instead function allocating
memory from xc.
- removed the get_buffer function instead there is a direct call
to qemu_memalign.
- moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
- added struct xengnttab_grant_copy_segment_t and stub in
xen_common.h for version of Xen earlier then 480.
- added checking for version 480 to configure. The test repeats
all the operation that are required for version < 480 and
checks if xengnttab_grant_copy() is implemented.
Changes since v2:
Interface:
- dropped the changes in libxc/include/xenctrl_compat
- changed the MINOR version in Makefile
- replaced 'return -1' -> 'abort()'in libs/gnttab/gnttab_unimp.c
- moved the struct 'xengnttab_copy_grant_segment' to
libs/gnttab/include/xengnttab.h
- added explicit assingment to ioctl_gntdev_grant_copy_segment
to the linux part
qemu-qdisk:
- to use the xengnttab_* function directly added -lxengnttab to configure
and include <xengnttab.h> in include/hw/xen/xen_common.h
- in ioreq_copy removed an out path, changed a log level, made explicit
assignement to 'xengnttab_copy_grant_segment'
* I did not change the way of testing if grant_copy operation is implemented.
As far as I understand if the code from gnttab_unimp.c is used then the gnttab
device is unavailable and the handler to gntdev would be invalid. But
if the handler is valid then the ioctl should return operation unimplemented
if the gntdev does not implement the operation.
Changes since v1:
Interface:
- changed the interface to call grant copy operation to match ioctl
int xengnttab_grant_copy(xengnttab_handle *xgt,
uint32_t count,
xengnttab_grant_copy_segment_t* segs)
- added a struct 'xengnttab_copy_grant_segment' definition to tools/libs
/gnttab/private.h, tools/libxc/include/xenctrl_compat.h
- changed the function 'osdep_gnttab_grant_copy' which right now just
call the ioctl
- added a new VER1.1 to tools/libs/gnttab/libxengnttab.map
qemu-qdisk:
- removed the 'ioreq_write','ioreq_read_init','ioreq_read' functions
- implemented 'ioreq_init_copy_buffers', 'ioreq_copy'
- reverted the removal of grant map and introduced conditional invoking
grant copy or grant map
- resigned from caching the local buffers on behalf of allocating the
required amount of pages at once. The cached structure would require
to have an lock guard and I suppose that the performance improvement
would degraded.
For the functional test I attached the device with a qdisk backend to the guest,
mounted, performed some reads and writes.
I run fio tests[1] with different iodepth and size of the block. The test can be
accessed on my github[2] but mainly after the warm up I run for 60 seconds:
fio --time_based \
--clocksource=clock_gettime \
--rw=randread \
--random_distribution=pareto:0.9 \
--size=10g \
--direct='1' \
--ioengine=libaio \
--filename=$DEV \
--iodepth=$IODEPTH \
--bs=$BS \
--name=$NAME \
--runtime=$RUNTIME >> $FILENAME
The test were repeated at least three times.
[1] https://docs.google.com/spreadsheets/d/1E6AMiB8ceJpExL6jWpH9u2yy6DZxzhmDUyFf-eUuJ0c/edit?usp=sharing
[2] https://github.com/paulina-szubarczyk/xen-benchmark
- multitest_with_iodepth.sh
Thanks and regards,
Paulina
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 0/2] qemu-qdisk: Implementation of grant copy operation.
@ 2016-08-02 14:06 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-02 14:06 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, Paulina Szubarczyk, ian.jackson,
qemu-devel, david.vrabel, anthony.perard, roger.pau
Hi,
It is a proposition for implementation of grant copy operation in qemu-qdisk and interface in libxc/libs.
Changes since v3:
Interface:
- revert to cast from xengnttab_grant_copy_segment_t
to ioctl_gntdev_grant_copy.
- added compile-time check to compare the libs
xengnttab_grant_copy_segment_t with the ioctl structure.
The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON in libs/gnttab.
qemu-qdisk:
- qemu_memalign/qemu_free is used instead function allocating
memory from xc.
- removed the get_buffer function instead there is a direct call
to qemu_memalign.
- moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
- added struct xengnttab_grant_copy_segment_t and stub in
xen_common.h for version of Xen earlier then 480.
- added checking for version 480 to configure. The test repeats
all the operation that are required for version < 480 and
checks if xengnttab_grant_copy() is implemented.
Changes since v2:
Interface:
- dropped the changes in libxc/include/xenctrl_compat
- changed the MINOR version in Makefile
- replaced 'return -1' -> 'abort()'in libs/gnttab/gnttab_unimp.c
- moved the struct 'xengnttab_copy_grant_segment' to
libs/gnttab/include/xengnttab.h
- added explicit assingment to ioctl_gntdev_grant_copy_segment
to the linux part
qemu-qdisk:
- to use the xengnttab_* function directly added -lxengnttab to configure
and include <xengnttab.h> in include/hw/xen/xen_common.h
- in ioreq_copy removed an out path, changed a log level, made explicit
assignement to 'xengnttab_copy_grant_segment'
* I did not change the way of testing if grant_copy operation is implemented.
As far as I understand if the code from gnttab_unimp.c is used then the gnttab
device is unavailable and the handler to gntdev would be invalid. But
if the handler is valid then the ioctl should return operation unimplemented
if the gntdev does not implement the operation.
Changes since v1:
Interface:
- changed the interface to call grant copy operation to match ioctl
int xengnttab_grant_copy(xengnttab_handle *xgt,
uint32_t count,
xengnttab_grant_copy_segment_t* segs)
- added a struct 'xengnttab_copy_grant_segment' definition to tools/libs
/gnttab/private.h, tools/libxc/include/xenctrl_compat.h
- changed the function 'osdep_gnttab_grant_copy' which right now just
call the ioctl
- added a new VER1.1 to tools/libs/gnttab/libxengnttab.map
qemu-qdisk:
- removed the 'ioreq_write','ioreq_read_init','ioreq_read' functions
- implemented 'ioreq_init_copy_buffers', 'ioreq_copy'
- reverted the removal of grant map and introduced conditional invoking
grant copy or grant map
- resigned from caching the local buffers on behalf of allocating the
required amount of pages at once. The cached structure would require
to have an lock guard and I suppose that the performance improvement
would degraded.
For the functional test I attached the device with a qdisk backend to the guest,
mounted, performed some reads and writes.
I run fio tests[1] with different iodepth and size of the block. The test can be
accessed on my github[2] but mainly after the warm up I run for 60 seconds:
fio --time_based \
--clocksource=clock_gettime \
--rw=randread \
--random_distribution=pareto:0.9 \
--size=10g \
--direct='1' \
--ioengine=libaio \
--filename=$DEV \
--iodepth=$IODEPTH \
--bs=$BS \
--name=$NAME \
--runtime=$RUNTIME >> $FILENAME
The test were repeated at least three times.
[1] https://docs.google.com/spreadsheets/d/1E6AMiB8ceJpExL6jWpH9u2yy6DZxzhmDUyFf-eUuJ0c/edit?usp=sharing
[2] https://github.com/paulina-szubarczyk/xen-benchmark
- multitest_with_iodepth.sh
Thanks and regards,
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-02 14:06 ` Paulina Szubarczyk
@ 2016-08-02 14:06 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-02 14:06 UTC (permalink / raw)
To: xen-devel
Cc: roger.pau, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel, Paulina Szubarczyk
In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
system call is invoked. In mini-os the operation is yet not
implemented. For the OSs that does not implement gnttab the
call of the grant copy operation causes abort.
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v3:
- revert to cast from xengnttab_grant_copy_segment_t
to ioctl_gntdev_grant_copy.
- added compile-time check to compare the libs
xengnttab_grant_copy_segment_t with the ioctl structure.
The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON
in libs/gnttab.
---
tools/include/xen-sys/Linux/gntdev.h | 21 ++++++++++
tools/libs/gnttab/Makefile | 2 +-
tools/libs/gnttab/gnttab_core.c | 6 +++
tools/libs/gnttab/gnttab_unimp.c | 6 +++
tools/libs/gnttab/include/xengnttab.h | 28 +++++++++++++
tools/libs/gnttab/libxengnttab.map | 5 +++
tools/libs/gnttab/linux.c | 79 +++++++++++++++++++++++++++++++++++
tools/libs/gnttab/minios.c | 6 +++
tools/libs/gnttab/private.h | 4 ++
9 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
index caf6fb4..0ca07c9 100644
--- a/tools/include/xen-sys/Linux/gntdev.h
+++ b/tools/include/xen-sys/Linux/gntdev.h
@@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
/* Send an interrupt on the indicated event channel */
#define UNMAP_NOTIFY_SEND_EVENT 0x2
+struct ioctl_gntdev_grant_copy_segment {
+ union {
+ void *virt;
+ struct {
+ uint32_t ref;
+ uint16_t offset;
+ uint16_t domid;
+ } foreign;
+ } source, dest;
+ uint16_t len;
+ uint16_t flags;
+ int16_t status;
+};
+
+#define IOCTL_GNTDEV_GRANT_COPY \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
+struct ioctl_gntdev_grant_copy {
+ unsigned int count;
+ struct ioctl_gntdev_grant_copy_segment *segments;
+};
+
#endif /* __LINUX_PUBLIC_GNTDEV_H__ */
diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index af64542..95c2cd8 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
include $(XEN_ROOT)/tools/Rules.mk
MAJOR = 1
-MINOR = 0
+MINOR = 1
SHLIB_LDFLAGS += -Wl,--version-script=libxengnttab.map
CFLAGS += -Werror -Wmissing-prototypes
diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 5d0474d..968c833 100644
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
return osdep_gnttab_unmap(xgt, start_address, count);
}
+int xengnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return osdep_gnttab_grant_copy(xgt, count, segs);
+}
/*
* Local variables:
* mode: C
diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c
index b3a4a20..829eced 100644
--- a/tools/libs/gnttab/gnttab_unimp.c
+++ b/tools/libs/gnttab/gnttab_unimp.c
@@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
abort();
}
+int xengnttab_copy_grant(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_copy_grant_segment_t *segs)
+{
+ abort();
+}
/*
* Local variables:
* mode: C
diff --git a/tools/libs/gnttab/include/xengnttab.h b/tools/libs/gnttab/include/xengnttab.h
index 0431dcf..949fd9e 100644
--- a/tools/libs/gnttab/include/xengnttab.h
+++ b/tools/libs/gnttab/include/xengnttab.h
@@ -258,6 +258,34 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count);
int xengnttab_set_max_grants(xengnttab_handle *xgt,
uint32_t nr_grants);
+struct xengnttab_grant_copy_segment {
+ union xengnttab_copy_ptr {
+ void *virt;
+ struct {
+ uint32_t ref;
+ uint16_t offset;
+ uint16_t domid;
+ } foreign;
+ } source, dest;
+ uint16_t len;
+ uint16_t flags;
+ int16_t status;
+};
+
+typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
+
+/**
+ * Copy memory from or to grant references. The information of each operations
+ * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
+ * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
+ *
+ * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
+ * should not exceed XEN_PAGE_SIZE
+ */
+int xengnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs);
+
/*
* Grant Sharing Interface (allocating and granting pages to others)
*/
diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
index dc737ac..f78da22 100644
--- a/tools/libs/gnttab/libxengnttab.map
+++ b/tools/libs/gnttab/libxengnttab.map
@@ -21,3 +21,8 @@ VERS_1.0 {
xengntshr_unshare;
local: *; /* Do not expose anything by default */
};
+
+VERS_1.1 {
+ global:
+ xengnttab_grant_copy;
+} VERS_1.0;
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 7b0fba4..56dff57 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -23,6 +23,7 @@
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
+#include <stddef.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
@@ -235,6 +236,84 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
return 0;
}
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ int rc;
+ int fd = xgt->fd;
+ struct ioctl_gntdev_grant_copy copy;
+
+ XENGNTTAB_BUILD_BUG_ON(sizeof(struct ioctl_gntdev_grant_copy_segment) !=
+ sizeof(xengnttab_grant_copy_segment_t));
+
+ XENGNTTAB_BUILD_BUG_ON(__alignof__(struct ioctl_gntdev_grant_copy_segment) !=
+ __alignof__(xengnttab_grant_copy_segment_t));
+
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.virt) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.virt));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign.ref) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign.offset) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign.offset));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign.domid) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign.domid));
+
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.virt) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.virt));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign.ref) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign.offset) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign.offset));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign.domid) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign.domid));
+
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ len) !=
+ offsetof(xengnttab_grant_copy_segment_t, len));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ flags) !=
+ offsetof(xengnttab_grant_copy_segment_t, flags));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ status) !=
+ offsetof(xengnttab_grant_copy_segment_t, status));
+
+ copy.segments = (struct ioctl_gntdev_grant_copy_segment *)segs;
+ copy.count = count;
+
+ rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©);
+ if (rc)
+ {
+ GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
+ }
+
+ return rc;
+}
+
int osdep_gntshr_open(xengntshr_handle *xgs)
{
int fd = open(DEVXEN "gntalloc", O_RDWR);
diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
index 7e04174..0951bc9 100644
--- a/tools/libs/gnttab/minios.c
+++ b/tools/libs/gnttab/minios.c
@@ -106,6 +106,12 @@ int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
return ret;
}
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return -1;
+}
/*
* Local variables:
* mode: C
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index 2bdc0f2..f8d234a 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -30,6 +30,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
int osdep_gnttab_unmap(xengnttab_handle *xgt,
void *start_address,
uint32_t count);
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs);
+
int osdep_gntshr_open(xengntshr_handle *xgs);
int osdep_gntshr_close(xengntshr_handle *xgs);
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 1/2] Interface for grant copy operation in libs.
@ 2016-08-02 14:06 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-02 14:06 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, Paulina Szubarczyk, ian.jackson,
qemu-devel, david.vrabel, anthony.perard, roger.pau
In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
system call is invoked. In mini-os the operation is yet not
implemented. For the OSs that does not implement gnttab the
call of the grant copy operation causes abort.
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v3:
- revert to cast from xengnttab_grant_copy_segment_t
to ioctl_gntdev_grant_copy.
- added compile-time check to compare the libs
xengnttab_grant_copy_segment_t with the ioctl structure.
The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON
in libs/gnttab.
---
tools/include/xen-sys/Linux/gntdev.h | 21 ++++++++++
tools/libs/gnttab/Makefile | 2 +-
tools/libs/gnttab/gnttab_core.c | 6 +++
tools/libs/gnttab/gnttab_unimp.c | 6 +++
tools/libs/gnttab/include/xengnttab.h | 28 +++++++++++++
tools/libs/gnttab/libxengnttab.map | 5 +++
tools/libs/gnttab/linux.c | 79 +++++++++++++++++++++++++++++++++++
tools/libs/gnttab/minios.c | 6 +++
tools/libs/gnttab/private.h | 4 ++
9 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
index caf6fb4..0ca07c9 100644
--- a/tools/include/xen-sys/Linux/gntdev.h
+++ b/tools/include/xen-sys/Linux/gntdev.h
@@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
/* Send an interrupt on the indicated event channel */
#define UNMAP_NOTIFY_SEND_EVENT 0x2
+struct ioctl_gntdev_grant_copy_segment {
+ union {
+ void *virt;
+ struct {
+ uint32_t ref;
+ uint16_t offset;
+ uint16_t domid;
+ } foreign;
+ } source, dest;
+ uint16_t len;
+ uint16_t flags;
+ int16_t status;
+};
+
+#define IOCTL_GNTDEV_GRANT_COPY \
+_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
+struct ioctl_gntdev_grant_copy {
+ unsigned int count;
+ struct ioctl_gntdev_grant_copy_segment *segments;
+};
+
#endif /* __LINUX_PUBLIC_GNTDEV_H__ */
diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index af64542..95c2cd8 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
include $(XEN_ROOT)/tools/Rules.mk
MAJOR = 1
-MINOR = 0
+MINOR = 1
SHLIB_LDFLAGS += -Wl,--version-script=libxengnttab.map
CFLAGS += -Werror -Wmissing-prototypes
diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 5d0474d..968c833 100644
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
return osdep_gnttab_unmap(xgt, start_address, count);
}
+int xengnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return osdep_gnttab_grant_copy(xgt, count, segs);
+}
/*
* Local variables:
* mode: C
diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c
index b3a4a20..829eced 100644
--- a/tools/libs/gnttab/gnttab_unimp.c
+++ b/tools/libs/gnttab/gnttab_unimp.c
@@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count)
abort();
}
+int xengnttab_copy_grant(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_copy_grant_segment_t *segs)
+{
+ abort();
+}
/*
* Local variables:
* mode: C
diff --git a/tools/libs/gnttab/include/xengnttab.h b/tools/libs/gnttab/include/xengnttab.h
index 0431dcf..949fd9e 100644
--- a/tools/libs/gnttab/include/xengnttab.h
+++ b/tools/libs/gnttab/include/xengnttab.h
@@ -258,6 +258,34 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count);
int xengnttab_set_max_grants(xengnttab_handle *xgt,
uint32_t nr_grants);
+struct xengnttab_grant_copy_segment {
+ union xengnttab_copy_ptr {
+ void *virt;
+ struct {
+ uint32_t ref;
+ uint16_t offset;
+ uint16_t domid;
+ } foreign;
+ } source, dest;
+ uint16_t len;
+ uint16_t flags;
+ int16_t status;
+};
+
+typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
+
+/**
+ * Copy memory from or to grant references. The information of each operations
+ * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
+ * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
+ *
+ * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
+ * should not exceed XEN_PAGE_SIZE
+ */
+int xengnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs);
+
/*
* Grant Sharing Interface (allocating and granting pages to others)
*/
diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
index dc737ac..f78da22 100644
--- a/tools/libs/gnttab/libxengnttab.map
+++ b/tools/libs/gnttab/libxengnttab.map
@@ -21,3 +21,8 @@ VERS_1.0 {
xengntshr_unshare;
local: *; /* Do not expose anything by default */
};
+
+VERS_1.1 {
+ global:
+ xengnttab_grant_copy;
+} VERS_1.0;
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 7b0fba4..56dff57 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -23,6 +23,7 @@
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
+#include <stddef.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
@@ -235,6 +236,84 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
return 0;
}
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ int rc;
+ int fd = xgt->fd;
+ struct ioctl_gntdev_grant_copy copy;
+
+ XENGNTTAB_BUILD_BUG_ON(sizeof(struct ioctl_gntdev_grant_copy_segment) !=
+ sizeof(xengnttab_grant_copy_segment_t));
+
+ XENGNTTAB_BUILD_BUG_ON(__alignof__(struct ioctl_gntdev_grant_copy_segment) !=
+ __alignof__(xengnttab_grant_copy_segment_t));
+
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.virt) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.virt));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign.ref) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign.offset) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign.offset));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ source.foreign.domid) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ source.foreign.domid));
+
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.virt) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.virt));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign.ref) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign.offset) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign.offset));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ dest.foreign.domid) !=
+ offsetof(xengnttab_grant_copy_segment_t,
+ dest.foreign.domid));
+
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ len) !=
+ offsetof(xengnttab_grant_copy_segment_t, len));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ flags) !=
+ offsetof(xengnttab_grant_copy_segment_t, flags));
+ XENGNTTAB_BUILD_BUG_ON(offsetof(struct ioctl_gntdev_grant_copy_segment,
+ status) !=
+ offsetof(xengnttab_grant_copy_segment_t, status));
+
+ copy.segments = (struct ioctl_gntdev_grant_copy_segment *)segs;
+ copy.count = count;
+
+ rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©);
+ if (rc)
+ {
+ GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
+ }
+
+ return rc;
+}
+
int osdep_gntshr_open(xengntshr_handle *xgs)
{
int fd = open(DEVXEN "gntalloc", O_RDWR);
diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
index 7e04174..0951bc9 100644
--- a/tools/libs/gnttab/minios.c
+++ b/tools/libs/gnttab/minios.c
@@ -106,6 +106,12 @@ int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
return ret;
}
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return -1;
+}
/*
* Local variables:
* mode: C
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index 2bdc0f2..f8d234a 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -30,6 +30,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
int osdep_gnttab_unmap(xengnttab_handle *xgt,
void *start_address,
uint32_t count);
+int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
+ uint32_t count,
+ xengnttab_grant_copy_segment_t *segs);
+
int osdep_gntshr_open(xengntshr_handle *xgs);
int osdep_gntshr_close(xengntshr_handle *xgs);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-02 14:06 ` Paulina Szubarczyk
@ 2016-08-02 14:06 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-02 14:06 UTC (permalink / raw)
To: xen-devel
Cc: roger.pau, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel, Paulina Szubarczyk
Copy data operated on during request from/to local buffers to/from
the grant references.
Before grant copy operation local buffers must be allocated what is
done by calling ioreq_init_copy_buffers. For the 'read' operation,
first, the qemu device invokes the read operation on local buffers
and on the completion grant copy is called and buffers are freed.
For the 'write' operation grant copy is performed before invoking
write by qemu device.
A new value 'feature_grant_copy' is added to recognize when the
grant copy operation is supported by a guest.
Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
---
Changes since v3:
- qemu_memalign/qemu_free is used instead function allocating
memory from xc.
- removed the get_buffer function instead there is a direct call
to qemu_memalign.
- moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
- added struct xengnttab_grant_copy_segment_t and stub in
xen_common.h for version of xen earlier then 480.
- added checking for version 480 to configure. The test repeats
all the operation that are required for version < 480 and
checks if xengnttab_grant_copy() is implemented.
* I did not change the way of testing if grant_copy operation is
implemented. As far as I understand if the code from
gnttab_unimp.c is used then the gnttab device is unavailable
and the handler to gntdev would be invalid. But if the handler
is valid then the ioctl should return operation unimplemented
if the gntdev does not implement the operation.
---
configure | 56 +++++++++++++++++
hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
include/hw/xen/xen_common.h | 25 ++++++++
3 files changed, 218 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index f57fcc6..b5bf7d4 100755
--- a/configure
+++ b/configure
@@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
return 0;
}
+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);
+ if (!ioreq->pages) {
+ return -1;
+ }
+
+ for (i = 0; i < ioreq->v.niov; i++) {
+ ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
+ ioreq->v.iov[i].iov_base = 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 = 0, r, 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;
+ } else {
+ r = 0;
+ }
+
+ 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", rc,
+ ioreq->refs[i], ioreq->domids[i]);
+ ioreq->aio_errors++;
+ r = -1;
+ }
+ }
+
+ return r;
+}
+
static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
static void qemu_aio_complete(void *opaque, int ret)
@@ -511,8 +603,29 @@ 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 */
+ 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 +651,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 +707,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 +718,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 +1158,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 640c31e..e80c61f 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -25,6 +25,31 @@
*/
/* Xen 4.2 through 4.6 */
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
+
+struct xengnttab_grant_copy_segment {
+ union xengnttab_copy_ptr {
+ void *virt;
+ struct {
+ uint32_t ref;
+ uint16_t offset;
+ uint16_t domid;
+ } foreign;
+ } source, dest;
+ uint16_t len;
+ uint16_t flags;
+ int16_t status;
+};
+typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
+
+static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return -1;
+}
+
+#endif
+
#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 471
typedef xc_interface xenforeignmemory_handle;
--
1.9.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
@ 2016-08-02 14:06 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-02 14:06 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 v3:
- qemu_memalign/qemu_free is used instead function allocating
memory from xc.
- removed the get_buffer function instead there is a direct call
to qemu_memalign.
- moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
- added struct xengnttab_grant_copy_segment_t and stub in
xen_common.h for version of xen earlier then 480.
- added checking for version 480 to configure. The test repeats
all the operation that are required for version < 480 and
checks if xengnttab_grant_copy() is implemented.
* I did not change the way of testing if grant_copy operation is
implemented. As far as I understand if the code from
gnttab_unimp.c is used then the gnttab device is unavailable
and the handler to gntdev would be invalid. But if the handler
is valid then the ioctl should return operation unimplemented
if the gntdev does not implement the operation.
---
configure | 56 +++++++++++++++++
hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
include/hw/xen/xen_common.h | 25 ++++++++
3 files changed, 218 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index f57fcc6..b5bf7d4 100755
--- a/configure
+++ b/configure
@@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
return 0;
}
+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);
+ if (!ioreq->pages) {
+ return -1;
+ }
+
+ for (i = 0; i < ioreq->v.niov; i++) {
+ ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
+ ioreq->v.iov[i].iov_base = 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 = 0, r, 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;
+ } else {
+ r = 0;
+ }
+
+ 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", rc,
+ ioreq->refs[i], ioreq->domids[i]);
+ ioreq->aio_errors++;
+ r = -1;
+ }
+ }
+
+ return r;
+}
+
static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
static void qemu_aio_complete(void *opaque, int ret)
@@ -511,8 +603,29 @@ 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 */
+ 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 +651,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 +707,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 +718,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 +1158,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 640c31e..e80c61f 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -25,6 +25,31 @@
*/
/* Xen 4.2 through 4.6 */
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
+
+struct xengnttab_grant_copy_segment {
+ union xengnttab_copy_ptr {
+ void *virt;
+ struct {
+ uint32_t ref;
+ uint16_t offset;
+ uint16_t domid;
+ } foreign;
+ } source, dest;
+ uint16_t len;
+ uint16_t flags;
+ int16_t status;
+};
+typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
+
+static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
+ xengnttab_grant_copy_segment_t *segs)
+{
+ return -1;
+}
+
+#endif
+
#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 471
typedef xc_interface xenforeignmemory_handle;
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-02 14:06 ` Paulina Szubarczyk
(?)
(?)
@ 2016-08-03 14:36 ` David Vrabel
2016-08-04 9:42 ` David Vrabel
2016-08-04 9:42 ` David Vrabel
-1 siblings, 2 replies; 27+ messages in thread
From: David Vrabel @ 2016-08-03 14:36 UTC (permalink / raw)
To: Paulina Szubarczyk, xen-devel
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, roger.pau
On 02/08/16 15:06, Paulina Szubarczyk wrote:
>
> +/**
> + * Copy memory from or to grant references. The information of each operations
> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
> + *
> + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
> + * should not exceed XEN_PAGE_SIZE
"For each segment, @virt may cross a page boundary but @offset + @len
must be less than XEN_PAGE_SIZE." might be better.
With or without the above change:
Reviewed-by: David Vrabel <david.vrabel@citrix.com>
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-02 14:06 ` Paulina Szubarczyk
(?)
@ 2016-08-03 14:36 ` David Vrabel
-1 siblings, 0 replies; 27+ messages in thread
From: David Vrabel @ 2016-08-03 14:36 UTC (permalink / raw)
To: Paulina Szubarczyk, xen-devel
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, roger.pau
On 02/08/16 15:06, Paulina Szubarczyk wrote:
>
> +/**
> + * Copy memory from or to grant references. The information of each operations
> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
> + *
> + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
> + * should not exceed XEN_PAGE_SIZE
"For each segment, @virt may cross a page boundary but @offset + @len
must be less than XEN_PAGE_SIZE." might be better.
With or without the above change:
Reviewed-by: David Vrabel <david.vrabel@citrix.com>
David
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-02 14:06 ` Paulina Szubarczyk
` (3 preceding siblings ...)
(?)
@ 2016-08-04 9:38 ` Wei Liu
2016-08-04 10:27 ` Paulina Szubarczyk
-1 siblings, 1 reply; 27+ messages in thread
From: Wei Liu @ 2016-08-04 9:38 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
sstabellini, anthony.perard, qemu-devel
The code looks ok. I have two minor suggestions below.
I would suggest changing the subject line to:
libs/gnttab: introduce grant copy interface
On Tue, Aug 02, 2016 at 04:06:29PM +0200, Paulina Szubarczyk wrote:
> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> system call is invoked. In mini-os the operation is yet not
> implemented. For the OSs that does not implement gnttab the
> call of the grant copy operation causes abort.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
> Changes since v3:
> - revert to cast from xengnttab_grant_copy_segment_t
> to ioctl_gntdev_grant_copy.
> - added compile-time check to compare the libs
> xengnttab_grant_copy_segment_t with the ioctl structure.
> The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON
> in libs/gnttab.
I should resubmit that one soon.
> ---
[...]
> + rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©);
> + if (rc)
> + {
> + GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
> + }
Normally for a single statement you don't need {} around it.
No need to resubmit just because of this patch. I can handle the subject
line change, fix up the style issue and change the comment according to
David's suggestion while committing if you don't object to any of them.
I won't commit this patch right away though. I will wait until the QEMU
patch is acked because I would avoid committing things that have no
users.
If you end up submitting another version you can make those changes
yourself.
Wei.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-02 14:06 ` Paulina Szubarczyk
` (2 preceding siblings ...)
(?)
@ 2016-08-04 9:38 ` Wei Liu
-1 siblings, 0 replies; 27+ messages in thread
From: Wei Liu @ 2016-08-04 9:38 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, xen-devel, roger.pau
The code looks ok. I have two minor suggestions below.
I would suggest changing the subject line to:
libs/gnttab: introduce grant copy interface
On Tue, Aug 02, 2016 at 04:06:29PM +0200, Paulina Szubarczyk wrote:
> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> system call is invoked. In mini-os the operation is yet not
> implemented. For the OSs that does not implement gnttab the
> call of the grant copy operation causes abort.
>
> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
> ---
> Changes since v3:
> - revert to cast from xengnttab_grant_copy_segment_t
> to ioctl_gntdev_grant_copy.
> - added compile-time check to compare the libs
> xengnttab_grant_copy_segment_t with the ioctl structure.
> The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON
> in libs/gnttab.
I should resubmit that one soon.
> ---
[...]
> + rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©);
> + if (rc)
> + {
> + GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
> + }
Normally for a single statement you don't need {} around it.
No need to resubmit just because of this patch. I can handle the subject
line change, fix up the style issue and change the comment according to
David's suggestion while committing if you don't object to any of them.
I won't commit this patch right away though. I will wait until the QEMU
patch is acked because I would avoid committing things that have no
users.
If you end up submitting another version you can make those changes
yourself.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-03 14:36 ` [Qemu-devel] [Xen-devel] " David Vrabel
@ 2016-08-04 9:42 ` David Vrabel
2016-08-04 9:42 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2016-08-04 9:42 UTC (permalink / raw)
To: Paulina Szubarczyk, xen-devel
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, anthony.perard,
roger.pau
On 03/08/16 15:36, David Vrabel wrote:
> On 02/08/16 15:06, Paulina Szubarczyk wrote:
>>
>> +/**
>> + * Copy memory from or to grant references. The information of each operations
>> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
>> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
>> + *
>> + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
>> + * should not exceed XEN_PAGE_SIZE
>
> "For each segment, @virt may cross a page boundary but @offset + @len
> must be less than XEN_PAGE_SIZE." might be better.
This should be:
"For each segment, @virt may cross a page boundary but @offset + @len
must not exceed XEN_PAGE_SIZE."
David
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-03 14:36 ` [Qemu-devel] [Xen-devel] " David Vrabel
2016-08-04 9:42 ` David Vrabel
@ 2016-08-04 9:42 ` David Vrabel
1 sibling, 0 replies; 27+ messages in thread
From: David Vrabel @ 2016-08-04 9:42 UTC (permalink / raw)
To: Paulina Szubarczyk, xen-devel
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, anthony.perard,
roger.pau
On 03/08/16 15:36, David Vrabel wrote:
> On 02/08/16 15:06, Paulina Szubarczyk wrote:
>>
>> +/**
>> + * Copy memory from or to grant references. The information of each operations
>> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate
>> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
>> + *
>> + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t'
>> + * should not exceed XEN_PAGE_SIZE
>
> "For each segment, @virt may cross a page boundary but @offset + @len
> must be less than XEN_PAGE_SIZE." might be better.
This should be:
"For each segment, @virt may cross a page boundary but @offset + @len
must not exceed XEN_PAGE_SIZE."
David
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] Interface for grant copy operation in libs.
2016-08-04 9:38 ` [Qemu-devel] " Wei Liu
@ 2016-08-04 10:27 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-04 10:27 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel, roger.pau, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel
On 08/04/2016 11:38 AM, Wei Liu wrote:
> The code looks ok. I have two minor suggestions below.
>
> I would suggest changing the subject line to:
>
> libs/gnttab: introduce grant copy interface
>
> On Tue, Aug 02, 2016 at 04:06:29PM +0200, Paulina Szubarczyk wrote:
>> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
>> system call is invoked. In mini-os the operation is yet not
>> implemented. For the OSs that does not implement gnttab the
>> call of the grant copy operation causes abort.
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>> Changes since v3:
>> - revert to cast from xengnttab_grant_copy_segment_t
>> to ioctl_gntdev_grant_copy.
>> - added compile-time check to compare the libs
>> xengnttab_grant_copy_segment_t with the ioctl structure.
>> The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON
>> in libs/gnttab.
>
> I should resubmit that one soon.
>
>> ---
> [...]
>> + rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©);
>> + if (rc)
>> + {
>> + GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
>> + }
>
> Normally for a single statement you don't need {} around it.
>
> No need to resubmit just because of this patch. I can handle the subject
> line change, fix up the style issue and change the comment according to
> David's suggestion while committing if you don't object to any of them.
>
Ok, thank you.
> I won't commit this patch right away though. I will wait until the QEMU
> patch is acked because I would avoid committing things that have no
> users.
>
> If you end up submitting another version you can make those changes
> yourself.
>
>
> Wei.
>
Paulina
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 1/2] Interface for grant copy operation in libs.
@ 2016-08-04 10:27 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-04 10:27 UTC (permalink / raw)
To: Wei Liu
Cc: sstabellini, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, xen-devel, roger.pau
On 08/04/2016 11:38 AM, Wei Liu wrote:
> The code looks ok. I have two minor suggestions below.
>
> I would suggest changing the subject line to:
>
> libs/gnttab: introduce grant copy interface
>
> On Tue, Aug 02, 2016 at 04:06:29PM +0200, Paulina Szubarczyk wrote:
>> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
>> system call is invoked. In mini-os the operation is yet not
>> implemented. For the OSs that does not implement gnttab the
>> call of the grant copy operation causes abort.
>>
>> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com>
>> ---
>> Changes since v3:
>> - revert to cast from xengnttab_grant_copy_segment_t
>> to ioctl_gntdev_grant_copy.
>> - added compile-time check to compare the libs
>> xengnttab_grant_copy_segment_t with the ioctl structure.
>> The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON
>> in libs/gnttab.
>
> I should resubmit that one soon.
>
>> ---
> [...]
>> + rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©);
>> + if (rc)
>> + {
>> + GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
>> + }
>
> Normally for a single statement you don't need {} around it.
>
> No need to resubmit just because of this patch. I can handle the subject
> line change, fix up the style issue and change the comment according to
> David's suggestion while committing if you don't object to any of them.
>
Ok, thank you.
> I won't commit this patch right away though. I will wait until the QEMU
> patch is acked because I would avoid committing things that have no
> users.
>
> If you end up submitting another version you can make those changes
> yourself.
>
>
> Wei.
>
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-02 14:06 ` Paulina Szubarczyk
@ 2016-08-08 11:11 ` Roger Pau Monné
-1 siblings, 0 replies; 27+ messages in thread
From: Roger Pau Monné @ 2016-08-08 11:11 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel
On Tue, Aug 02, 2016 at 04:06:30PM +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>
Thanks, this is looking good, just a couple of minor comments below.
> ---
> Changes since v3:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> * I did not change the way of testing if grant_copy operation is
> implemented. As far as I understand if the code from
> gnttab_unimp.c is used then the gnttab device is unavailable
> and the handler to gntdev would be invalid. But if the handler
> is valid then the ioctl should return operation unimplemented
> if the gntdev does not implement the operation.
> ---
> configure | 56 +++++++++++++++++
> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 25 ++++++++
> 3 files changed, 218 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index f57fcc6..b5bf7d4 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 0, 0);
I don't think you need to check xengnttab_map_grant_ref, just
xengnttab_grant_copy should be enough? Since xengnttab_grant_copy was added
later you can assume xengnttab_map_grant_ref will be there.
> + 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +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);
> + if (!ioreq->pages) {
> + return -1;
> + }
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = 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 = 0, r, rc;
Why do you need to initialize count to 0 here? A couple of lines below you
inconditionally set it to ioreq->v.niov.
> + 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;
> + } else {
> + r = 0;
> + }
Do you really need both r and rc here? I think you could just have rc and
use it below also.
> + 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", rc,
> + ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + r = -1;
> + }
> + }
> +
> + return r;
> +}
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
Roger.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
@ 2016-08-08 11:11 ` Roger Pau Monné
0 siblings, 0 replies; 27+ messages in thread
From: Roger Pau Monné @ 2016-08-08 11:11 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, xen-devel
On Tue, Aug 02, 2016 at 04:06:30PM +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>
Thanks, this is looking good, just a couple of minor comments below.
> ---
> Changes since v3:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> * I did not change the way of testing if grant_copy operation is
> implemented. As far as I understand if the code from
> gnttab_unimp.c is used then the gnttab device is unavailable
> and the handler to gntdev would be invalid. But if the handler
> is valid then the ioctl should return operation unimplemented
> if the gntdev does not implement the operation.
> ---
> configure | 56 +++++++++++++++++
> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 25 ++++++++
> 3 files changed, 218 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index f57fcc6..b5bf7d4 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 0, 0);
I don't think you need to check xengnttab_map_grant_ref, just
xengnttab_grant_copy should be enough? Since xengnttab_grant_copy was added
later you can assume xengnttab_map_grant_ref will be there.
> + 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +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);
> + if (!ioreq->pages) {
> + return -1;
> + }
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = 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 = 0, r, rc;
Why do you need to initialize count to 0 here? A couple of lines below you
inconditionally set it to ioreq->v.niov.
> + 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;
> + } else {
> + r = 0;
> + }
Do you really need both r and rc here? I think you could just have rc and
use it below also.
> + 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", rc,
> + ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + r = -1;
> + }
> + }
> +
> + return r;
> +}
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-08 11:11 ` Roger Pau Monné
(?)
@ 2016-08-08 11:34 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-08 11:34 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel
On 08/08/2016 01:11 PM, Roger Pau Monné wrote:
> On Tue, Aug 02, 2016 at 04:06:30PM +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>
>
> Thanks, this is looking good, just a couple of minor comments below.
>
Thank you for the review, I will apply the comments.
>> ---
>> Changes since v3:
>> - qemu_memalign/qemu_free is used instead function allocating
>> memory from xc.
>> - removed the get_buffer function instead there is a direct call
>> to qemu_memalign.
>> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
>> - added struct xengnttab_grant_copy_segment_t and stub in
>> xen_common.h for version of xen earlier then 480.
>> - added checking for version 480 to configure. The test repeats
>> all the operation that are required for version < 480 and
>> checks if xengnttab_grant_copy() is implemented.
>>
>> * I did not change the way of testing if grant_copy operation is
>> implemented. As far as I understand if the code from
>> gnttab_unimp.c is used then the gnttab device is unavailable
>> and the handler to gntdev would be invalid. But if the handler
>> is valid then the ioctl should return operation unimplemented
>> if the gntdev does not implement the operation.
>> ---
>> configure | 56 +++++++++++++++++
>> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/xen/xen_common.h | 25 ++++++++
>> 3 files changed, 218 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index f57fcc6..b5bf7d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 0, 0);
>
> I don't think you need to check xengnttab_map_grant_ref, just
> xengnttab_grant_copy should be enough? Since xengnttab_grant_copy was added
> later you can assume xengnttab_map_grant_ref will be there.
>
>> + 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
>> return 0;
>> }
>>
>> +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);
>> + if (!ioreq->pages) {
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>> + ioreq->v.iov[i].iov_base = 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 = 0, r, rc;
>
> Why do you need to initialize count to 0 here? A couple of lines below you
> inconditionally set it to ioreq->v.niov.
>
>> + 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;
>> + } else {
>> + r = 0;
>> + }
>
> Do you really need both r and rc here? I think you could just have rc and
> use it below also.
>
>> + 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", rc,
>> + ioreq->refs[i], ioreq->domids[i]);
>> + ioreq->aio_errors++;
>> + r = -1;
>> + }
>> + }
>> +
>> + return r;
>> +}
>> +
>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>
>> static void qemu_aio_complete(void *opaque, int ret)
>
> Roger.
>
Paulina
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-08 11:11 ` Roger Pau Monné
(?)
(?)
@ 2016-08-08 11:34 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-08 11:34 UTC (permalink / raw)
To: Roger Pau Monné
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, xen-devel
On 08/08/2016 01:11 PM, Roger Pau Monné wrote:
> On Tue, Aug 02, 2016 at 04:06:30PM +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>
>
> Thanks, this is looking good, just a couple of minor comments below.
>
Thank you for the review, I will apply the comments.
>> ---
>> Changes since v3:
>> - qemu_memalign/qemu_free is used instead function allocating
>> memory from xc.
>> - removed the get_buffer function instead there is a direct call
>> to qemu_memalign.
>> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
>> - added struct xengnttab_grant_copy_segment_t and stub in
>> xen_common.h for version of xen earlier then 480.
>> - added checking for version 480 to configure. The test repeats
>> all the operation that are required for version < 480 and
>> checks if xengnttab_grant_copy() is implemented.
>>
>> * I did not change the way of testing if grant_copy operation is
>> implemented. As far as I understand if the code from
>> gnttab_unimp.c is used then the gnttab device is unavailable
>> and the handler to gntdev would be invalid. But if the handler
>> is valid then the ioctl should return operation unimplemented
>> if the gntdev does not implement the operation.
>> ---
>> configure | 56 +++++++++++++++++
>> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/xen/xen_common.h | 25 ++++++++
>> 3 files changed, 218 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index f57fcc6..b5bf7d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 0, 0);
>
> I don't think you need to check xengnttab_map_grant_ref, just
> xengnttab_grant_copy should be enough? Since xengnttab_grant_copy was added
> later you can assume xengnttab_map_grant_ref will be there.
>
>> + 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
>> return 0;
>> }
>>
>> +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);
>> + if (!ioreq->pages) {
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>> + ioreq->v.iov[i].iov_base = 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 = 0, r, rc;
>
> Why do you need to initialize count to 0 here? A couple of lines below you
> inconditionally set it to ioreq->v.niov.
>
>> + 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;
>> + } else {
>> + r = 0;
>> + }
>
> Do you really need both r and rc here? I think you could just have rc and
> use it below also.
>
>> + 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", rc,
>> + ioreq->refs[i], ioreq->domids[i]);
>> + ioreq->aio_errors++;
>> + r = -1;
>> + }
>> + }
>> +
>> + return r;
>> +}
>> +
>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>
>> static void qemu_aio_complete(void *opaque, int ret)
>
> Roger.
>
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-02 14:06 ` Paulina Szubarczyk
` (2 preceding siblings ...)
(?)
@ 2016-08-08 11:44 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-08 11:44 UTC (permalink / raw)
To: xen-devel
Cc: roger.pau, wei.liu2, ian.jackson, david.vrabel, sstabellini,
anthony.perard, qemu-devel
On 08/02/2016 04:06 PM, 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 v3:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> * I did not change the way of testing if grant_copy operation is
> implemented. As far as I understand if the code from
> gnttab_unimp.c is used then the gnttab device is unavailable
> and the handler to gntdev would be invalid. But if the handler
> is valid then the ioctl should return operation unimplemented
> if the gntdev does not implement the operation.
> ---
> configure | 56 +++++++++++++++++
> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 25 ++++++++
> 3 files changed, 218 insertions(+), 5 deletions(-)
> /* qemu block driver */
> DriveInfo *dinfo;
> BlockBackend *blk;
> @@ -489,6 +492,95 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +603,29 @@ 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 */
> + ioreq_copy(ioreq);
I would add a condition to invoke the grant copy only if the ret
argument with which the callback from BlockBackend
'qemu_aio_complete(void *opaque, int ret)' is called is equal to 0
to not unnecessary copy invalid data.
> + 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:
Paulina
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-02 14:06 ` Paulina Szubarczyk
(?)
(?)
@ 2016-08-08 11:44 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-08 11:44 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
anthony.perard, roger.pau
On 08/02/2016 04:06 PM, 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 v3:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> * I did not change the way of testing if grant_copy operation is
> implemented. As far as I understand if the code from
> gnttab_unimp.c is used then the gnttab device is unavailable
> and the handler to gntdev would be invalid. But if the handler
> is valid then the ioctl should return operation unimplemented
> if the gntdev does not implement the operation.
> ---
> configure | 56 +++++++++++++++++
> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 25 ++++++++
> 3 files changed, 218 insertions(+), 5 deletions(-)
> /* qemu block driver */
> DriveInfo *dinfo;
> BlockBackend *blk;
> @@ -489,6 +492,95 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +603,29 @@ 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 */
> + ioreq_copy(ioreq);
I would add a condition to invoke the grant copy only if the ret
argument with which the callback from BlockBackend
'qemu_aio_complete(void *opaque, int ret)' is called is equal to 0
to not unnecessary copy invalid data.
> + 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:
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-02 14:06 ` Paulina Szubarczyk
@ 2016-08-09 16:56 ` Anthony PERARD
-1 siblings, 0 replies; 27+ messages in thread
From: Anthony PERARD @ 2016-08-09 16:56 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
sstabellini, qemu-devel
On Tue, Aug 02, 2016 at 04:06:30PM +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>
> ---
> Changes since v3:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> * I did not change the way of testing if grant_copy operation is
> implemented. As far as I understand if the code from
> gnttab_unimp.c is used then the gnttab device is unavailable
> and the handler to gntdev would be invalid. But if the handler
> is valid then the ioctl should return operation unimplemented
> if the gntdev does not implement the operation.
> ---
> configure | 56 +++++++++++++++++
> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 25 ++++++++
> 3 files changed, 218 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index f57fcc6..b5bf7d4 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +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);
> + if (!ioreq->pages) {
qemu_memalign never returns NULL, you don't have to check.
> + return -1;
> + }
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = 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 = 0, r, 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;
> + } else {
> + r = 0;
> + }
> +
> + 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", rc,
> + ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + r = -1;
> + }
> + }
> +
> + return r;
> +}
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +603,29 @@ 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 */
> + 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 +651,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 +707,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 +718,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 +1158,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 640c31e..e80c61f 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -25,6 +25,31 @@
> */
>
> /* Xen 4.2 through 4.6 */
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> +
> +struct xengnttab_grant_copy_segment {
> + union xengnttab_copy_ptr {
> + void *virt;
> + struct {
> + uint32_t ref;
> + uint16_t offset;
> + uint16_t domid;
> + } foreign;
> + } source, dest;
> + uint16_t len;
> + uint16_t flags;
> + int16_t status;
> +};
I don't think it's a good idee to define a struct that is not going to
be used, and does not belong here. The typedef is OK.
In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
them by stubs when Xen does not support grant copy. I think that would
be better.
Also, could you try to compile again xen unstable, without your other patch?
Right now, it does not compile.
> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> +
> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> + xengnttab_grant_copy_segment_t *segs)
> +{
> + return -1;
return -ENOSYS would be more appropriate.
Otherwise, the patch looks good.
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
@ 2016-08-09 16:56 ` Anthony PERARD
0 siblings, 0 replies; 27+ messages in thread
From: Anthony PERARD @ 2016-08-09 16:56 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
xen-devel, roger.pau
On Tue, Aug 02, 2016 at 04:06:30PM +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>
> ---
> Changes since v3:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> * I did not change the way of testing if grant_copy operation is
> implemented. As far as I understand if the code from
> gnttab_unimp.c is used then the gnttab device is unavailable
> and the handler to gntdev would be invalid. But if the handler
> is valid then the ioctl should return operation unimplemented
> if the gntdev does not implement the operation.
> ---
> configure | 56 +++++++++++++++++
> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
> include/hw/xen/xen_common.h | 25 ++++++++
> 3 files changed, 218 insertions(+), 5 deletions(-)
>
> diff --git a/configure b/configure
> index f57fcc6..b5bf7d4 100755
> --- a/configure
> +++ b/configure
> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
> return 0;
> }
>
> +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);
> + if (!ioreq->pages) {
qemu_memalign never returns NULL, you don't have to check.
> + return -1;
> + }
> +
> + for (i = 0; i < ioreq->v.niov; i++) {
> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
> + ioreq->v.iov[i].iov_base = 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 = 0, r, 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;
> + } else {
> + r = 0;
> + }
> +
> + 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", rc,
> + ioreq->refs[i], ioreq->domids[i]);
> + ioreq->aio_errors++;
> + r = -1;
> + }
> + }
> +
> + return r;
> +}
> +
> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>
> static void qemu_aio_complete(void *opaque, int ret)
> @@ -511,8 +603,29 @@ 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 */
> + 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 +651,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 +707,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 +718,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 +1158,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 640c31e..e80c61f 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -25,6 +25,31 @@
> */
>
> /* Xen 4.2 through 4.6 */
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> +
> +struct xengnttab_grant_copy_segment {
> + union xengnttab_copy_ptr {
> + void *virt;
> + struct {
> + uint32_t ref;
> + uint16_t offset;
> + uint16_t domid;
> + } foreign;
> + } source, dest;
> + uint16_t len;
> + uint16_t flags;
> + int16_t status;
> +};
I don't think it's a good idee to define a struct that is not going to
be used, and does not belong here. The typedef is OK.
In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
them by stubs when Xen does not support grant copy. I think that would
be better.
Also, could you try to compile again xen unstable, without your other patch?
Right now, it does not compile.
> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> +
> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> + xengnttab_grant_copy_segment_t *segs)
> +{
> + return -1;
return -ENOSYS would be more appropriate.
Otherwise, the patch looks good.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-09 16:56 ` Anthony PERARD
@ 2016-08-09 17:34 ` Paulina Szubarczyk
-1 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-09 17:34 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
sstabellini, qemu-devel
On 08/09/2016 06:56 PM, Anthony PERARD wrote:
> On Tue, Aug 02, 2016 at 04:06:30PM +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>
>> ---
>> Changes since v3:
>> - qemu_memalign/qemu_free is used instead function allocating
>> memory from xc.
>> - removed the get_buffer function instead there is a direct call
>> to qemu_memalign.
>> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
>> - added struct xengnttab_grant_copy_segment_t and stub in
>> xen_common.h for version of xen earlier then 480.
>> - added checking for version 480 to configure. The test repeats
>> all the operation that are required for version < 480 and
>> checks if xengnttab_grant_copy() is implemented.
>>
>> * I did not change the way of testing if grant_copy operation is
>> implemented. As far as I understand if the code from
>> gnttab_unimp.c is used then the gnttab device is unavailable
>> and the handler to gntdev would be invalid. But if the handler
>> is valid then the ioctl should return operation unimplemented
>> if the gntdev does not implement the operation.
>> ---
>> configure | 56 +++++++++++++++++
>> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/xen/xen_common.h | 25 ++++++++
>> 3 files changed, 218 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index f57fcc6..b5bf7d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
>> return 0;
>> }
>>
>> +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);
>> + if (!ioreq->pages) {
>
> qemu_memalign never returns NULL, you don't have to check.
>
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>> + ioreq->v.iov[i].iov_base = 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 = 0, r, 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;
>> + } else {
>> + r = 0;
>> + }
>> +
>> + 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", rc,
>> + ioreq->refs[i], ioreq->domids[i]);
>> + ioreq->aio_errors++;
>> + r = -1;
>> + }
>> + }
>> +
>> + return r;
>> +}
>> +
>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>
>> static void qemu_aio_complete(void *opaque, int ret)
>> @@ -511,8 +603,29 @@ 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 */
>> + 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 +651,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 +707,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 +718,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 +1158,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 640c31e..e80c61f 100644
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -25,6 +25,31 @@
>> */
>>
>> /* Xen 4.2 through 4.6 */
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
>> +
>> +struct xengnttab_grant_copy_segment {
>> + union xengnttab_copy_ptr {
>> + void *virt;
>> + struct {
>> + uint32_t ref;
>> + uint16_t offset;
>> + uint16_t domid;
>> + } foreign;
>> + } source, dest;
>> + uint16_t len;
>> + uint16_t flags;
>> + int16_t status;
>> +};
>
> I don't think it's a good idee to define a struct that is not going to
> be used, and does not belong here. The typedef is OK.
I added it since it is needed to know all the fields of that struct in
ioreq_copy but if I could replace that function with stubs I will do that.
>
> In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
> around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
> them by stubs when Xen does not support grant copy. I think that would
> be better.
>
> Also, could you try to compile again xen unstable, without your other patch?
> Right now, it does not compile.
That is true, I am sorry. I need to add the headers that are included in
the
#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 */
#include <xenevtchn.h>
#include <xengnttab.h>
#include <xenforeignmemory.h>
#endif
May I move that part to separate #if CONFIG_XEN_CTRL_INTERFACE_VERSION
>= 471 in that header?
>
>> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
>> +
>> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
>> + xengnttab_grant_copy_segment_t *segs)
>> +{
>> + return -1;
>
> return -ENOSYS would be more appropriate.
>
>
> Otherwise, the patch looks good.
>
> Thanks,
>
Thank you,
Paulina
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
@ 2016-08-09 17:34 ` Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-09 17:34 UTC (permalink / raw)
To: Anthony PERARD
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
xen-devel, roger.pau
On 08/09/2016 06:56 PM, Anthony PERARD wrote:
> On Tue, Aug 02, 2016 at 04:06:30PM +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>
>> ---
>> Changes since v3:
>> - qemu_memalign/qemu_free is used instead function allocating
>> memory from xc.
>> - removed the get_buffer function instead there is a direct call
>> to qemu_memalign.
>> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
>> - added struct xengnttab_grant_copy_segment_t and stub in
>> xen_common.h for version of xen earlier then 480.
>> - added checking for version 480 to configure. The test repeats
>> all the operation that are required for version < 480 and
>> checks if xengnttab_grant_copy() is implemented.
>>
>> * I did not change the way of testing if grant_copy operation is
>> implemented. As far as I understand if the code from
>> gnttab_unimp.c is used then the gnttab device is unavailable
>> and the handler to gntdev would be invalid. But if the handler
>> is valid then the ioctl should return operation unimplemented
>> if the gntdev does not implement the operation.
>> ---
>> configure | 56 +++++++++++++++++
>> hw/block/xen_disk.c | 142 ++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/xen/xen_common.h | 25 ++++++++
>> 3 files changed, 218 insertions(+), 5 deletions(-)
>>
>> diff --git a/configure b/configure
>> index f57fcc6..b5bf7d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1956,6 +1956,62 @@ 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_map_grant_ref(xg, 0, 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..2dd1464 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,95 @@ static int ioreq_map(struct ioreq *ioreq)
>> return 0;
>> }
>>
>> +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);
>> + if (!ioreq->pages) {
>
> qemu_memalign never returns NULL, you don't have to check.
>
>> + return -1;
>> + }
>> +
>> + for (i = 0; i < ioreq->v.niov; i++) {
>> + ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
>> + ioreq->v.iov[i].iov_base = 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 = 0, r, 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;
>> + } else {
>> + r = 0;
>> + }
>> +
>> + 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", rc,
>> + ioreq->refs[i], ioreq->domids[i]);
>> + ioreq->aio_errors++;
>> + r = -1;
>> + }
>> + }
>> +
>> + return r;
>> +}
>> +
>> static int ioreq_runio_qemu_aio(struct ioreq *ioreq);
>>
>> static void qemu_aio_complete(void *opaque, int ret)
>> @@ -511,8 +603,29 @@ 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 */
>> + 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 +651,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 +707,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 +718,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 +1158,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 640c31e..e80c61f 100644
>> --- a/include/hw/xen/xen_common.h
>> +++ b/include/hw/xen/xen_common.h
>> @@ -25,6 +25,31 @@
>> */
>>
>> /* Xen 4.2 through 4.6 */
>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
>> +
>> +struct xengnttab_grant_copy_segment {
>> + union xengnttab_copy_ptr {
>> + void *virt;
>> + struct {
>> + uint32_t ref;
>> + uint16_t offset;
>> + uint16_t domid;
>> + } foreign;
>> + } source, dest;
>> + uint16_t len;
>> + uint16_t flags;
>> + int16_t status;
>> +};
>
> I don't think it's a good idee to define a struct that is not going to
> be used, and does not belong here. The typedef is OK.
I added it since it is needed to know all the fields of that struct in
ioreq_copy but if I could replace that function with stubs I will do that.
>
> In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
> around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
> them by stubs when Xen does not support grant copy. I think that would
> be better.
>
> Also, could you try to compile again xen unstable, without your other patch?
> Right now, it does not compile.
That is true, I am sorry. I need to add the headers that are included in
the
#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 */
#include <xenevtchn.h>
#include <xengnttab.h>
#include <xenforeignmemory.h>
#endif
May I move that part to separate #if CONFIG_XEN_CTRL_INTERFACE_VERSION
>= 471 in that header?
>
>> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
>> +
>> +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
>> + xengnttab_grant_copy_segment_t *segs)
>> +{
>> + return -1;
>
> return -ENOSYS would be more appropriate.
>
>
> Otherwise, the patch looks good.
>
> Thanks,
>
Thank you,
Paulina
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-09 17:34 ` Paulina Szubarczyk
(?)
@ 2016-08-10 10:29 ` Anthony PERARD
-1 siblings, 0 replies; 27+ messages in thread
From: Anthony PERARD @ 2016-08-10 10:29 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: xen-devel, roger.pau, wei.liu2, ian.jackson, david.vrabel,
sstabellini, qemu-devel
On Tue, Aug 09, 2016 at 07:34:14PM +0200, Paulina Szubarczyk wrote:
> On 08/09/2016 06:56 PM, Anthony PERARD wrote:
> > On Tue, Aug 02, 2016 at 04:06:30PM +0200, Paulina Szubarczyk wrote:
> > > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> > > index 640c31e..e80c61f 100644
> > > --- a/include/hw/xen/xen_common.h
> > > +++ b/include/hw/xen/xen_common.h
> > > @@ -25,6 +25,31 @@
> > > */
> > >
> > > /* Xen 4.2 through 4.6 */
> > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> > > +
> > > +struct xengnttab_grant_copy_segment {
> > > + union xengnttab_copy_ptr {
> > > + void *virt;
> > > + struct {
> > > + uint32_t ref;
> > > + uint16_t offset;
> > > + uint16_t domid;
> > > + } foreign;
> > > + } source, dest;
> > > + uint16_t len;
> > > + uint16_t flags;
> > > + int16_t status;
> > > +};
> >
> > I don't think it's a good idee to define a struct that is not going to
> > be used, and does not belong here. The typedef is OK.
>
> I added it since it is needed to know all the fields of that struct in
> ioreq_copy but if I could replace that function with stubs I will do that.
>
> >
> > In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
> > around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
> > them by stubs when Xen does not support grant copy. I think that would
> > be better.
> >
> > Also, could you try to compile again xen unstable, without your other patch?
> > Right now, it does not compile.
>
> That is true, I am sorry. I need to add the headers that are included in the
> #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 */
>
> #include <xenevtchn.h>
> #include <xengnttab.h>
> #include <xenforeignmemory.h>
>
> #endif
>
> May I move that part to separate #if CONFIG_XEN_CTRL_INTERFACE_VERSION >=
> 471 in that header?
You could could add your code at the end of xen_common.h instead of the
beginning, I think that would work. (At the end of xen_common.h,
xengnttab_handle should be define, either in the file or via an
include.)
> >
> > > +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> > > +
> > > +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> > > + xengnttab_grant_copy_segment_t *segs)
> > > +{
> > > + return -1;
> >
> > return -ENOSYS would be more appropriate.
> >
> >
> > Otherwise, the patch looks good.
> >
> > Thanks,
> >
> Thank you,
>
> Paulina
--
Anthony PERARD
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation
2016-08-09 17:34 ` Paulina Szubarczyk
(?)
(?)
@ 2016-08-10 10:29 ` Anthony PERARD
-1 siblings, 0 replies; 27+ messages in thread
From: Anthony PERARD @ 2016-08-10 10:29 UTC (permalink / raw)
To: Paulina Szubarczyk
Cc: sstabellini, wei.liu2, ian.jackson, qemu-devel, david.vrabel,
xen-devel, roger.pau
On Tue, Aug 09, 2016 at 07:34:14PM +0200, Paulina Szubarczyk wrote:
> On 08/09/2016 06:56 PM, Anthony PERARD wrote:
> > On Tue, Aug 02, 2016 at 04:06:30PM +0200, Paulina Szubarczyk wrote:
> > > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> > > index 640c31e..e80c61f 100644
> > > --- a/include/hw/xen/xen_common.h
> > > +++ b/include/hw/xen/xen_common.h
> > > @@ -25,6 +25,31 @@
> > > */
> > >
> > > /* Xen 4.2 through 4.6 */
> > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> > > +
> > > +struct xengnttab_grant_copy_segment {
> > > + union xengnttab_copy_ptr {
> > > + void *virt;
> > > + struct {
> > > + uint32_t ref;
> > > + uint16_t offset;
> > > + uint16_t domid;
> > > + } foreign;
> > > + } source, dest;
> > > + uint16_t len;
> > > + uint16_t flags;
> > > + int16_t status;
> > > +};
> >
> > I don't think it's a good idee to define a struct that is not going to
> > be used, and does not belong here. The typedef is OK.
>
> I added it since it is needed to know all the fields of that struct in
> ioreq_copy but if I could replace that function with stubs I will do that.
>
> >
> > In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
> > around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
> > them by stubs when Xen does not support grant copy. I think that would
> > be better.
> >
> > Also, could you try to compile again xen unstable, without your other patch?
> > Right now, it does not compile.
>
> That is true, I am sorry. I need to add the headers that are included in the
> #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 */
>
> #include <xenevtchn.h>
> #include <xengnttab.h>
> #include <xenforeignmemory.h>
>
> #endif
>
> May I move that part to separate #if CONFIG_XEN_CTRL_INTERFACE_VERSION >=
> 471 in that header?
You could could add your code at the end of xen_common.h instead of the
beginning, I think that would work. (At the end of xen_common.h,
xengnttab_handle should be define, either in the file or via an
include.)
> >
> > > +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> > > +
> > > +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> > > + xengnttab_grant_copy_segment_t *segs)
> > > +{
> > > + return -1;
> >
> > return -ENOSYS would be more appropriate.
> >
> >
> > Otherwise, the patch looks good.
> >
> > Thanks,
> >
> Thank you,
>
> Paulina
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v4 0/2] qemu-qdisk: Implementation of grant copy operation.
@ 2016-08-16 6:55 Paulina Szubarczyk
0 siblings, 0 replies; 27+ messages in thread
From: Paulina Szubarczyk @ 2016-08-16 6:55 UTC (permalink / raw)
To: xen-devel, roger.pau
Cc: wei.liu2, ian.jackson, david.vrabel, sstabellini, anthony.perard,
qemu-devel, Paulina Szubarczyk
Hi,
It is a proposition for implementation of grant copy operation in qemu-qdisk and interface in libxc/libs.
Changes since v3:
Interface:
- revert to cast from xengnttab_grant_copy_segment_t
to ioctl_gntdev_grant_copy.
- added compile-time check to compare the libs
xengnttab_grant_copy_segment_t with the ioctl structure.
The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON in libs/gnttab.
qemu-qdisk:
- qemu_memalign/qemu_free is used instead function allocating
memory from xc.
- removed the get_buffer function instead there is a direct call
to qemu_memalign.
- moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
- added struct xengnttab_grant_copy_segment_t and stub in
xen_common.h for version of Xen earlier then 480.
- added checking for version 480 to configure. The test repeats
all the operation that are required for version < 480 and
checks if xengnttab_grant_copy() is implemented.
Changes since v2:
Interface:
- dropped the changes in libxc/include/xenctrl_compat
- changed the MINOR version in Makefile
- replaced 'return -1' -> 'abort()'in libs/gnttab/gnttab_unimp.c
- moved the struct 'xengnttab_copy_grant_segment' to
libs/gnttab/include/xengnttab.h
- added explicit assingment to ioctl_gntdev_grant_copy_segment
to the linux part
qemu-qdisk:
- to use the xengnttab_* function directly added -lxengnttab to configure
and include <xengnttab.h> in include/hw/xen/xen_common.h
- in ioreq_copy removed an out path, changed a log level, made explicit
assignement to 'xengnttab_copy_grant_segment'
* I did not change the way of testing if grant_copy operation is implemented.
As far as I understand if the code from gnttab_unimp.c is used then the gnttab
device is unavailable and the handler to gntdev would be invalid. But
if the handler is valid then the ioctl should return operation unimplemented
if the gntdev does not implement the operation.
Changes since v1:
Interface:
- changed the interface to call grant copy operation to match ioctl
int xengnttab_grant_copy(xengnttab_handle *xgt,
uint32_t count,
xengnttab_grant_copy_segment_t* segs)
- added a struct 'xengnttab_copy_grant_segment' definition to tools/libs
/gnttab/private.h, tools/libxc/include/xenctrl_compat.h
- changed the function 'osdep_gnttab_grant_copy' which right now just
call the ioctl
- added a new VER1.1 to tools/libs/gnttab/libxengnttab.map
qemu-qdisk:
- removed the 'ioreq_write','ioreq_read_init','ioreq_read' functions
- implemented 'ioreq_init_copy_buffers', 'ioreq_copy'
- reverted the removal of grant map and introduced conditional invoking
grant copy or grant map
- resigned from caching the local buffers on behalf of allocating the
required amount of pages at once. The cached structure would require
to have an lock guard and I suppose that the performance improvement
would degraded.
For the functional test I attached the device with a qdisk backend to the guest,
mounted, performed some reads and writes.
I run fio tests[1] with different iodepth and size of the block. The test can be
accessed on my github[2] but mainly after the warm up I run for 60 seconds:
fio --time_based \
--clocksource=clock_gettime \
--rw=randread \
--random_distribution=pareto:0.9 \
--size=10g \
--direct='1' \
--ioengine=libaio \
--filename=$DEV \
--iodepth=$IODEPTH \
--bs=$BS \
--name=$NAME \
--runtime=$RUNTIME >> $FILENAME
The test were repeated at least three times.
[1] https://docs.google.com/spreadsheets/d/1E6AMiB8ceJpExL6jWpH9u2yy6DZxzhmDUyFf-eUuJ0c/edit?usp=sharing
[2] https://github.com/paulina-szubarczyk/xen-benchmark
- multitest_with_iodepth.sh
Thanks and regards,
Paulina
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2016-08-16 6:56 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-02 14:06 [Qemu-devel] [PATCH v4 0/2] qemu-qdisk: Implementation of grant copy operation Paulina Szubarczyk
2016-08-02 14:06 ` Paulina Szubarczyk
2016-08-02 14:06 ` [Qemu-devel] [PATCH v4 1/2] Interface for grant copy operation in libs Paulina Szubarczyk
2016-08-02 14:06 ` Paulina Szubarczyk
2016-08-03 14:36 ` David Vrabel
2016-08-03 14:36 ` [Qemu-devel] [Xen-devel] " David Vrabel
2016-08-04 9:42 ` David Vrabel
2016-08-04 9:42 ` David Vrabel
2016-08-04 9:38 ` Wei Liu
2016-08-04 9:38 ` [Qemu-devel] " Wei Liu
2016-08-04 10:27 ` Paulina Szubarczyk
2016-08-04 10:27 ` Paulina Szubarczyk
2016-08-02 14:06 ` [Qemu-devel] [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation Paulina Szubarczyk
2016-08-02 14:06 ` Paulina Szubarczyk
2016-08-08 11:11 ` [Qemu-devel] " Roger Pau Monné
2016-08-08 11:11 ` Roger Pau Monné
2016-08-08 11:34 ` [Qemu-devel] " Paulina Szubarczyk
2016-08-08 11:34 ` Paulina Szubarczyk
2016-08-08 11:44 ` Paulina Szubarczyk
2016-08-08 11:44 ` [Qemu-devel] " Paulina Szubarczyk
2016-08-09 16:56 ` Anthony PERARD
2016-08-09 16:56 ` Anthony PERARD
2016-08-09 17:34 ` [Qemu-devel] " Paulina Szubarczyk
2016-08-09 17:34 ` Paulina Szubarczyk
2016-08-10 10:29 ` [Qemu-devel] " Anthony PERARD
2016-08-10 10:29 ` Anthony PERARD
2016-08-16 6:55 [Qemu-devel] [PATCH v4 0/2] qemu-qdisk: Implementation of grant copy operation Paulina Szubarczyk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.