| Message ID | 1470146790-6168-2-git-send-email-paulinaszubarczyk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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);
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(-)