| Message ID | 1465811036-17026-2-git-send-email-paulinaszubarczyk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/06/16 10:43, Paulina Szubarczyk wrote: > Implentation of interface for grant copy operation in libs and > libxc. > > In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..) > system call is invoked. In mini-os the operation is yet not > implemented. For other OSs there is a dummy implementation. Reviewed-by: David Vrabel <david.vrabel@citrix.com> David
On Mon, Jun 13, 2016 at 11:43:55AM +0200, Paulina Szubarczyk wrote: > Implentation of interface for grant copy operation in libs and > libxc. > > In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..) > system call is invoked. In mini-os the operation is yet not > implemented. For other OSs there is a dummy implementation. > > Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com> > > --- > Changes since v1: > - changed the interface to call grant copy operation to match ioctl > int xengnttab_grant_copy(xengnttab_handle *xgt, > uint32_t count, > xengnttab_grant_copy_segment_t* segs) > - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs > /gnttab/private.h, tools/libxc/include/xenctrl_compat.h > - changed the function osdep_gnttab_grant_copy which right now just call > the ioctl > - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map > > tools/include/xen-sys/Linux/gntdev.h | 21 +++++++++++++++++++++ > tools/libs/gnttab/gnttab_core.c | 6 ++++++ > tools/libs/gnttab/gnttab_unimp.c | 6 ++++++ > tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++ > tools/libs/gnttab/libxengnttab.map | 4 ++++ > tools/libs/gnttab/linux.c | 18 ++++++++++++++++++ > tools/libs/gnttab/minios.c | 6 ++++++ > tools/libs/gnttab/private.h | 18 ++++++++++++++++++ > tools/libxc/include/xenctrl_compat.h | 23 +++++++++++++++++++++++ > tools/libxc/xc_gnttab_compat.c | 7 +++++++ > 10 files changed, 123 insertions(+) > > diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h > index caf6fb4..0ca07c9 100644 > --- a/tools/include/xen-sys/Linux/gntdev.h > +++ b/tools/include/xen-sys/Linux/gntdev.h > @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify { > /* Send an interrupt on the indicated event channel */ > #define UNMAP_NOTIFY_SEND_EVENT 0x2 > > +struct ioctl_gntdev_grant_copy_segment { > + union { > + void *virt; > + struct { > + uint32_t ref; > + uint16_t offset; > + uint16_t domid; > + } foreign; > + } source, dest; > + uint16_t len; > + uint16_t flags; > + int16_t status; > +}; > + > +#define IOCTL_GNTDEV_GRANT_COPY \ > +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy)) > +struct ioctl_gntdev_grant_copy { > + unsigned int count; > + struct ioctl_gntdev_grant_copy_segment *segments; > +}; > + This is ok. > #endif /* __LINUX_PUBLIC_GNTDEV_H__ */ > diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c > index 5d0474d..9badc58 100644 > --- a/tools/libs/gnttab/gnttab_core.c > +++ b/tools/libs/gnttab/gnttab_core.c > @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) > return osdep_gnttab_unmap(xgt, start_address, count); > } > > +int xengnttab_grant_copy(xengnttab_handle *xgt, > + uint32_t count, > + xengnttab_grant_copy_segment_t* segs) > +{ > + return osdep_gnttab_grant_copy(xgt, count, segs); > +} > /* > * Local variables: > * mode: C > diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c > index b3a4a20..79a5c88 100644 > --- a/tools/libs/gnttab/gnttab_unimp.c > +++ b/tools/libs/gnttab/gnttab_unimp.c > @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) > abort(); > } > > +int xengnttab_copy_grant(xengnttab_handle *xgt, > + uint32_t count, > + xengnttab_copy_grant_segment_t* segs) Coding style. Should be " *segs" instead of "* segs". Please also fix other instances of this nit. > +{ > + return -1; Please use abort() here instead. > +} > /* > * Local variables: > * mode: C > diff --git a/tools/libs/gnttab/include/xengnttab.h b/tools/libs/gnttab/include/xengnttab.h > index 0431dcf..a9fa1fe 100644 > --- a/tools/libs/gnttab/include/xengnttab.h > +++ b/tools/libs/gnttab/include/xengnttab.h > @@ -258,6 +258,20 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count); > int xengnttab_set_max_grants(xengnttab_handle *xgt, > uint32_t nr_grants); > > +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t; > + > +/** > + * Copy memory from or to grant references. The information of each operations > + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate > + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref). > + * > + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t' > + * should not exceed XEN_PAGE_SIZE > + */ > +int xengnttab_grant_copy(xengnttab_handle *xgt, > + uint32_t count, > + xengnttab_grant_copy_segment_t* segs); > + > /* > * Grant Sharing Interface (allocating and granting pages to others) > */ > diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map > index dc737ac..0928963 100644 > --- a/tools/libs/gnttab/libxengnttab.map > +++ b/tools/libs/gnttab/libxengnttab.map > @@ -21,3 +21,7 @@ VERS_1.0 { > xengntshr_unshare; > local: *; /* Do not expose anything by default */ > }; > + > +VERS_1.1 { Missing "global:" here? > + xengnttab_grant_copy; > +} VERS_1.0; You also need to modify the "MINOR" field in gnttab/Makefile so that the generate so file contains the correct version number. > \ No newline at end of file > diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c > index 7b0fba4..26bfbdc 100644 > --- a/tools/libs/gnttab/linux.c > +++ b/tools/libs/gnttab/linux.c > @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt, > return 0; > } > > diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h > index d286c86..22ad53a 100644 > --- a/tools/libs/gnttab/private.h > +++ b/tools/libs/gnttab/private.h > @@ -9,6 +9,20 @@ struct xengntdev_handle { > int fd; > }; > > +struct xengnttab_copy_grant_segment { > + union xengnttab_copy_ptr { > + void *virt; > + struct { > + uint32_t ref; > + uint16_t offset; > + uint16_t domid; > + } foreign; > + } source, dest; > + uint16_t len; > + uint16_t flags; > + int16_t status; > +}; > + The struct is more or less a direct copy of Linux structure. It is probably fine as-is, but I don't want to risk making this library Linux centric. If you look at other functions, they accept a bunch of discrete arguments then assemble those arguments into OS dependent structure in osdep functions. I know having discrete arguments for the API you want to introduce seems cumbersome, but I want to at least tell you all the background information needed for a thorough discussion. I would be interested in Roger's view on this. I apologise for not having commented on your series earlier. > int osdep_gnttab_open(xengnttab_handle *xgt); > int osdep_gnttab_close(xengnttab_handle *xgt); > > @@ -23,6 +37,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt, > int osdep_gnttab_unmap(xengnttab_handle *xgt, > void *start_address, > uint32_t count); > +int osdep_gnttab_grant_copy(xengnttab_handle *xgt, > + uint32_t count, > + xengnttab_grant_copy_segment_t* segs); > + > int osdep_gntshr_open(xengntshr_handle *xgs); > int osdep_gntshr_close(xengntshr_handle *xgs); > > diff --git a/tools/libxc/include/xenctrl_compat.h b/tools/libxc/include/xenctrl_compat.h > index 93ccadb..7bb24a4 100644 > --- a/tools/libxc/include/xenctrl_compat.h > +++ b/tools/libxc/include/xenctrl_compat.h > @@ -105,6 +105,29 @@ int xc_gnttab_munmap(xc_gnttab *xcg, > int xc_gnttab_set_max_grants(xc_gnttab *xcg, > uint32_t count); > > +union xengnttab_grant_copy_ptr { > + void *virt; > + struct { > + uint32_t ref; > + uint16_t offset; > + uint16_t domid; > + } foreign; > +}; > + > +struct xengnttab_grant_copy_segment { > + union xengnttab_grant_copy_ptr source, dest; > + uint16_t len; > + uint16_t flags; > + int16_t status; > +}; > + > +typedef struct xengnttab_grant_copy_segment xc_gnttab_grant_copy_segment_t; > +typedef union xengnttab_grant_copy_ptr xc_gnttab_grant_copy_ptr_t; > + > +int xc_gnttab_grant_copy(xc_gnttab *xgt, > + uint32_t count, > + xc_gnttab_grant_copy_segment_t* segs); > + The purpose of compact header is to make old application code that uses old APIs (the xc_* functions) continue to compile by providing necessary stubs. This is new API, so it doesn't belong to the compat header. Anyone who wants to use this new API should be using libxengnttab instead. We won't be adding more xc_ functions. I think you can just drop these changes completely. This also means the compatibility should be on QEMU side. I think QEMU already knows how to link against libxengnttab. What you need to do is to actually test if the API exists and take appropriate action there. If I'm not clear enough, feel free to ask. Wei. > typedef struct xengntdev_handle xc_gntshr; > > xc_gntshr *xc_gntshr_open(xentoollog_logger *logger, > diff --git a/tools/libxc/xc_gnttab_compat.c b/tools/libxc/xc_gnttab_compat.c > index 6f036d8..c265bb0 100644 > --- a/tools/libxc/xc_gnttab_compat.c > +++ b/tools/libxc/xc_gnttab_compat.c > @@ -69,6 +69,13 @@ int xc_gnttab_set_max_grants(xc_gnttab *xcg, > return xengnttab_set_max_grants(xcg, count); > } > > +int xc_gnttab_grant_copy(xc_gnttab *xgt, > + uint32_t count, > + xc_gnttab_grant_copy_segment_t* segs) > +{ > + return xengnttab_grant_copy(xgt, count, segs); > +} > + > xc_gntshr *xc_gntshr_open(xentoollog_logger *logger, > unsigned open_flags) > { > -- > 1.9.1 >
On 16/06/16 13:16, Wei Liu wrote: > On Mon, Jun 13, 2016 at 11:43:55AM +0200, Paulina Szubarczyk wrote: >> Implentation of interface for grant copy operation in libs and >> libxc. >> >> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..) >> system call is invoked. In mini-os the operation is yet not >> implemented. For other OSs there is a dummy implementation. >> >> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com> >> >> --- >> Changes since v1: >> - changed the interface to call grant copy operation to match ioctl >> int xengnttab_grant_copy(xengnttab_handle *xgt, >> uint32_t count, >> xengnttab_grant_copy_segment_t* segs) >> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs >> /gnttab/private.h, tools/libxc/include/xenctrl_compat.h >> - changed the function osdep_gnttab_grant_copy which right now just call >> the ioctl >> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map >> >> tools/include/xen-sys/Linux/gntdev.h | 21 +++++++++++++++++++++ >> tools/libs/gnttab/gnttab_core.c | 6 ++++++ >> tools/libs/gnttab/gnttab_unimp.c | 6 ++++++ >> tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++ >> tools/libs/gnttab/libxengnttab.map | 4 ++++ >> tools/libs/gnttab/linux.c | 18 ++++++++++++++++++ >> tools/libs/gnttab/minios.c | 6 ++++++ >> tools/libs/gnttab/private.h | 18 ++++++++++++++++++ >> tools/libxc/include/xenctrl_compat.h | 23 +++++++++++++++++++++++ >> tools/libxc/xc_gnttab_compat.c | 7 +++++++ >> 10 files changed, 123 insertions(+) >> >> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h >> index caf6fb4..0ca07c9 100644 >> --- a/tools/include/xen-sys/Linux/gntdev.h >> +++ b/tools/include/xen-sys/Linux/gntdev.h >> @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify { >> /* Send an interrupt on the indicated event channel */ >> #define UNMAP_NOTIFY_SEND_EVENT 0x2 >> >> +struct ioctl_gntdev_grant_copy_segment { >> + union { >> + void *virt; >> + struct { >> + uint32_t ref; >> + uint16_t offset; >> + uint16_t domid; >> + } foreign; >> + } source, dest; >> + uint16_t len; >> + uint16_t flags; >> + int16_t status; >> +}; >> + >> +#define IOCTL_GNTDEV_GRANT_COPY \ >> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy)) >> +struct ioctl_gntdev_grant_copy { >> + unsigned int count; >> + struct ioctl_gntdev_grant_copy_segment *segments; >> +}; >> + > > This is ok. > >> #endif /* __LINUX_PUBLIC_GNTDEV_H__ */ >> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c >> index 5d0474d..9badc58 100644 >> --- a/tools/libs/gnttab/gnttab_core.c >> +++ b/tools/libs/gnttab/gnttab_core.c >> @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) >> return osdep_gnttab_unmap(xgt, start_address, count); >> } >> >> +int xengnttab_grant_copy(xengnttab_handle *xgt, >> + uint32_t count, >> + xengnttab_grant_copy_segment_t* segs) >> +{ >> + return osdep_gnttab_grant_copy(xgt, count, segs); >> +} >> /* >> * Local variables: >> * mode: C >> diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c >> index b3a4a20..79a5c88 100644 >> --- a/tools/libs/gnttab/gnttab_unimp.c >> +++ b/tools/libs/gnttab/gnttab_unimp.c >> @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) >> abort(); >> } >> >> +int xengnttab_copy_grant(xengnttab_handle *xgt, >> + uint32_t count, >> + xengnttab_copy_grant_segment_t* segs) > > Coding style. Should be " *segs" instead of "* segs". > > Please also fix other instances of this nit. > >> +{ >> + return -1; > > Please use abort() here instead. This function is used to test whether grant copy is supported so cannot abort(). It is correctly returning an error. >> \ No newline at end of file >> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c >> index 7b0fba4..26bfbdc 100644 >> --- a/tools/libs/gnttab/linux.c >> +++ b/tools/libs/gnttab/linux.c >> @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt, >> return 0; >> } >> >> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h >> index d286c86..22ad53a 100644 >> --- a/tools/libs/gnttab/private.h >> +++ b/tools/libs/gnttab/private.h >> @@ -9,6 +9,20 @@ struct xengntdev_handle { >> int fd; >> }; >> >> +struct xengnttab_copy_grant_segment { >> + union xengnttab_copy_ptr { >> + void *virt; >> + struct { >> + uint32_t ref; >> + uint16_t offset; >> + uint16_t domid; >> + } foreign; >> + } source, dest; >> + uint16_t len; >> + uint16_t flags; >> + int16_t status; >> +}; >> + > > The struct is more or less a direct copy of Linux structure. Not really. It's a copy of the hypercall structure, adjusted to have a virt address instead of gfn/offset for local buffers. The Linux structure is also a similar copy which is why they happen to look the same. The previous threads explain why it is like this, but in summary this allows the library to present the same functionality as the underlying hypercall. I would also suggest you look at the previous version of this series to compare the user of this API. The one using this structure is nicer. David
On Thu, Jun 16, 2016 at 01:36:32PM +0100, David Vrabel wrote: > On 16/06/16 13:16, Wei Liu wrote: > > On Mon, Jun 13, 2016 at 11:43:55AM +0200, Paulina Szubarczyk wrote: > >> Implentation of interface for grant copy operation in libs and > >> libxc. > >> > >> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..) > >> system call is invoked. In mini-os the operation is yet not > >> implemented. For other OSs there is a dummy implementation. > >> > >> Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com> > >> > >> --- > >> Changes since v1: > >> - changed the interface to call grant copy operation to match ioctl > >> int xengnttab_grant_copy(xengnttab_handle *xgt, > >> uint32_t count, > >> xengnttab_grant_copy_segment_t* segs) > >> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs > >> /gnttab/private.h, tools/libxc/include/xenctrl_compat.h > >> - changed the function osdep_gnttab_grant_copy which right now just call > >> the ioctl > >> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map > >> > >> tools/include/xen-sys/Linux/gntdev.h | 21 +++++++++++++++++++++ > >> tools/libs/gnttab/gnttab_core.c | 6 ++++++ > >> tools/libs/gnttab/gnttab_unimp.c | 6 ++++++ > >> tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++ > >> tools/libs/gnttab/libxengnttab.map | 4 ++++ > >> tools/libs/gnttab/linux.c | 18 ++++++++++++++++++ > >> tools/libs/gnttab/minios.c | 6 ++++++ > >> tools/libs/gnttab/private.h | 18 ++++++++++++++++++ > >> tools/libxc/include/xenctrl_compat.h | 23 +++++++++++++++++++++++ > >> tools/libxc/xc_gnttab_compat.c | 7 +++++++ > >> 10 files changed, 123 insertions(+) > >> > >> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h > >> index caf6fb4..0ca07c9 100644 > >> --- a/tools/include/xen-sys/Linux/gntdev.h > >> +++ b/tools/include/xen-sys/Linux/gntdev.h > >> @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify { > >> /* Send an interrupt on the indicated event channel */ > >> #define UNMAP_NOTIFY_SEND_EVENT 0x2 > >> > >> +struct ioctl_gntdev_grant_copy_segment { > >> + union { > >> + void *virt; > >> + struct { > >> + uint32_t ref; > >> + uint16_t offset; > >> + uint16_t domid; > >> + } foreign; > >> + } source, dest; > >> + uint16_t len; > >> + uint16_t flags; > >> + int16_t status; > >> +}; > >> + > >> +#define IOCTL_GNTDEV_GRANT_COPY \ > >> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy)) > >> +struct ioctl_gntdev_grant_copy { > >> + unsigned int count; > >> + struct ioctl_gntdev_grant_copy_segment *segments; > >> +}; > >> + > > > > This is ok. > > > >> #endif /* __LINUX_PUBLIC_GNTDEV_H__ */ > >> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c > >> index 5d0474d..9badc58 100644 > >> --- a/tools/libs/gnttab/gnttab_core.c > >> +++ b/tools/libs/gnttab/gnttab_core.c > >> @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) > >> return osdep_gnttab_unmap(xgt, start_address, count); > >> } > >> > >> +int xengnttab_grant_copy(xengnttab_handle *xgt, > >> + uint32_t count, > >> + xengnttab_grant_copy_segment_t* segs) > >> +{ > >> + return osdep_gnttab_grant_copy(xgt, count, segs); > >> +} > >> /* > >> * Local variables: > >> * mode: C > >> diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c > >> index b3a4a20..79a5c88 100644 > >> --- a/tools/libs/gnttab/gnttab_unimp.c > >> +++ b/tools/libs/gnttab/gnttab_unimp.c > >> @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) > >> abort(); > >> } > >> > >> +int xengnttab_copy_grant(xengnttab_handle *xgt, > >> + uint32_t count, > >> + xengnttab_copy_grant_segment_t* segs) > > > > Coding style. Should be " *segs" instead of "* segs". > > > > Please also fix other instances of this nit. > > > >> +{ > >> + return -1; > > > > Please use abort() here instead. > > This function is used to test whether grant copy is supported so cannot > abort(). It is correctly returning an error. > No. For the "unimplemented" implementation, the code shouldn't call this function in the first place because the attempt to open a handle would have already failed. This is an impossible state for the "unimplemented" implementation. Any application uses the API like this deserves to be aborted. > >> \ No newline at end of file > >> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c > >> index 7b0fba4..26bfbdc 100644 > >> --- a/tools/libs/gnttab/linux.c > >> +++ b/tools/libs/gnttab/linux.c > >> @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt, > >> return 0; > >> } > >> > >> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h > >> index d286c86..22ad53a 100644 > >> --- a/tools/libs/gnttab/private.h > >> +++ b/tools/libs/gnttab/private.h > >> @@ -9,6 +9,20 @@ struct xengntdev_handle { > >> int fd; > >> }; > >> > >> +struct xengnttab_copy_grant_segment { > >> + union xengnttab_copy_ptr { > >> + void *virt; > >> + struct { > >> + uint32_t ref; > >> + uint16_t offset; > >> + uint16_t domid; > >> + } foreign; > >> + } source, dest; > >> + uint16_t len; > >> + uint16_t flags; > >> + int16_t status; > >> +}; > >> + > > > > The struct is more or less a direct copy of Linux structure. > > Not really. It's a copy of the hypercall structure, adjusted to have a > virt address instead of gfn/offset for local buffers. > > The Linux structure is also a similar copy which is why they happen to > look the same. > > The previous threads explain why it is like this, but in summary this > allows the library to present the same functionality as the underlying > hypercall. > > I would also suggest you look at the previous version of this series to > compare the user of this API. The one using this structure is nicer. > Right. I will go back and read the thread. Wei. > David
On Thu, Jun 16, 2016 at 01:16:54PM +0100, Wei Liu wrote: [...] [...] > > diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h > > index d286c86..22ad53a 100644 > > --- a/tools/libs/gnttab/private.h > > +++ b/tools/libs/gnttab/private.h > > @@ -9,6 +9,20 @@ struct xengntdev_handle { > > int fd; > > }; > > > > +struct xengnttab_copy_grant_segment { > > + union xengnttab_copy_ptr { > > + void *virt; > > + struct { > > + uint32_t ref; > > + uint16_t offset; > > + uint16_t domid; > > + } foreign; > > + } source, dest; > > + uint16_t len; > > + uint16_t flags; > > + int16_t status; > > +}; > > > > The struct is more or less a direct copy of Linux structure. It is > probably fine as-is, but I don't want to risk making this library Linux > centric. If you look at other functions, they accept a bunch of discrete > arguments then assemble those arguments into OS dependent structure in > osdep functions. I know having discrete arguments for the API you want > to introduce seems cumbersome, but I want to at least tell you all the > background information needed for a thorough discussion. I would be > interested in Roger's view on this. > > I apologise for not having commented on your series earlier. > After checking various places I'm convinced that this structure is fine as-is. BSDes have not yet had a user space grant table driver, so I don't really worry about that at this point. As I have asked you to removed all the stuff in xenctrl_compat.h, you will need to move this to libs/gnttab/xengnttab.h. Also I have one further comment for code: > +int osdep_gnttab_grant_copy(xengnttab_handle *xgt, > + uint32_t count, > + xengnttab_grant_copy_segment_t* segs) > +{ > + int fd = xgt->fd; > + struct ioctl_gntdev_grant_copy copy; > + > + copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs; Please create an array of ioctl structure and use explicit field by field assignment here. Casting like this can easily introduce bug -- just imagine ioctl gets changed, or the segment structure gets changed. Wei.
On Fri, 2016-06-17 at 17:43 +0100, Wei Liu wrote: > On Thu, Jun 16, 2016 at 01:16:54PM +0100, Wei Liu wrote: > [...] > [...] > > > diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h > > > index d286c86..22ad53a 100644 > > > --- a/tools/libs/gnttab/private.h > > > +++ b/tools/libs/gnttab/private.h > > > @@ -9,6 +9,20 @@ struct xengntdev_handle { > > > int fd; > > > }; > > > > > > +struct xengnttab_copy_grant_segment { > > > + union xengnttab_copy_ptr { > > > + void *virt; > > > + struct { > > > + uint32_t ref; > > > + uint16_t offset; > > > + uint16_t domid; > > > + } foreign; > > > + } source, dest; > > > + uint16_t len; > > > + uint16_t flags; > > > + int16_t status; > > > +}; > > > > > > > The struct is more or less a direct copy of Linux structure. It is > > probably fine as-is, but I don't want to risk making this library Linux > > centric. If you look at other functions, they accept a bunch of discrete > > arguments then assemble those arguments into OS dependent structure in > > osdep functions. I know having discrete arguments for the API you want > > to introduce seems cumbersome, but I want to at least tell you all the > > background information needed for a thorough discussion. I would be > > interested in Roger's view on this. > > > > I apologise for not having commented on your series earlier. > > > > After checking various places I'm convinced that this structure is fine > as-is. > > BSDes have not yet had a user space grant table driver, so I don't > really worry about that at this point. > > As I have asked you to removed all the stuff in xenctrl_compat.h, you > will need to move this to libs/gnttab/xengnttab.h. > > Also I have one further comment for code: > > > +int osdep_gnttab_grant_copy(xengnttab_handle *xgt, > > + uint32_t count, > > + xengnttab_grant_copy_segment_t* segs) > > +{ > > + int fd = xgt->fd; > > + struct ioctl_gntdev_grant_copy copy; > > + > > + copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs; > > Please create an array of ioctl structure and use explicit field by > field assignment here. > > Casting like this can easily introduce bug -- just imagine ioctl gets > changed, or the segment structure gets changed. > > Wei. Hi Wei, Thank you for all the remarks. I am working on correcting the patch. Paulina
diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h index caf6fb4..0ca07c9 100644 --- a/tools/include/xen-sys/Linux/gntdev.h +++ b/tools/include/xen-sys/Linux/gntdev.h @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify { /* Send an interrupt on the indicated event channel */ #define UNMAP_NOTIFY_SEND_EVENT 0x2 +struct ioctl_gntdev_grant_copy_segment { + union { + void *virt; + struct { + uint32_t ref; + uint16_t offset; + uint16_t domid; + } foreign; + } source, dest; + uint16_t len; + uint16_t flags; + int16_t status; +}; + +#define IOCTL_GNTDEV_GRANT_COPY \ +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy)) +struct ioctl_gntdev_grant_copy { + unsigned int count; + struct ioctl_gntdev_grant_copy_segment *segments; +}; + #endif /* __LINUX_PUBLIC_GNTDEV_H__ */ diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c index 5d0474d..9badc58 100644 --- a/tools/libs/gnttab/gnttab_core.c +++ b/tools/libs/gnttab/gnttab_core.c @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) return osdep_gnttab_unmap(xgt, start_address, count); } +int xengnttab_grant_copy(xengnttab_handle *xgt, + uint32_t count, + xengnttab_grant_copy_segment_t* segs) +{ + return osdep_gnttab_grant_copy(xgt, count, segs); +} /* * Local variables: * mode: C diff --git a/tools/libs/gnttab/gnttab_unimp.c b/tools/libs/gnttab/gnttab_unimp.c index b3a4a20..79a5c88 100644 --- a/tools/libs/gnttab/gnttab_unimp.c +++ b/tools/libs/gnttab/gnttab_unimp.c @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count) abort(); } +int xengnttab_copy_grant(xengnttab_handle *xgt, + uint32_t count, + xengnttab_copy_grant_segment_t* segs) +{ + return -1; +} /* * Local variables: * mode: C diff --git a/tools/libs/gnttab/include/xengnttab.h b/tools/libs/gnttab/include/xengnttab.h index 0431dcf..a9fa1fe 100644 --- a/tools/libs/gnttab/include/xengnttab.h +++ b/tools/libs/gnttab/include/xengnttab.h @@ -258,6 +258,20 @@ int xengnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count); int xengnttab_set_max_grants(xengnttab_handle *xgt, uint32_t nr_grants); +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t; + +/** + * Copy memory from or to grant references. The information of each operations + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value indicate + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref). + * + * The sum of fields @offset[i] and @len[i] of 'xengnttab_grant_copy_segment_t' + * should not exceed XEN_PAGE_SIZE + */ +int xengnttab_grant_copy(xengnttab_handle *xgt, + uint32_t count, + xengnttab_grant_copy_segment_t* segs); + /* * Grant Sharing Interface (allocating and granting pages to others) */ diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map index dc737ac..0928963 100644 --- a/tools/libs/gnttab/libxengnttab.map +++ b/tools/libs/gnttab/libxengnttab.map @@ -21,3 +21,7 @@ VERS_1.0 { xengntshr_unshare; local: *; /* Do not expose anything by default */ }; + +VERS_1.1 { + xengnttab_grant_copy; +} VERS_1.0; \ No newline at end of file diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c index 7b0fba4..26bfbdc 100644 --- a/tools/libs/gnttab/linux.c +++ b/tools/libs/gnttab/linux.c @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt, return 0; } +int osdep_gnttab_grant_copy(xengnttab_handle *xgt, + uint32_t count, + xengnttab_grant_copy_segment_t* segs) +{ + int fd = xgt->fd; + struct ioctl_gntdev_grant_copy copy; + + copy.segments = (struct ioctl_gntdev_grant_copy_segment*)segs; + copy.count = count; + + if (ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, ©)) { + GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno); + return -1; + } + + return 0; +} + int osdep_gntshr_open(xengntshr_handle *xgs) { int fd = open(DEVXEN "gntalloc", O_RDWR); diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c index 7e04174..93e4f89 100644 --- a/tools/libs/gnttab/minios.c +++ b/tools/libs/gnttab/minios.c @@ -106,6 +106,12 @@ int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count) return ret; } +int osdep_gnttab_grant_copy(xengnttab_handle *xgt, + uint32_t count, + xengnttab_grant_copy_segment_t* segs) +{ + return -1; +} /* * Local variables: * mode: C diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h index d286c86..22ad53a 100644 --- a/tools/libs/gnttab/private.h +++ b/tools/libs/gnttab/private.h @@ -9,6 +9,20 @@ struct xengntdev_handle { int fd; }; +struct xengnttab_copy_grant_segment { + union xengnttab_copy_ptr { + void *virt; + struct { + uint32_t ref; + uint16_t offset; + uint16_t domid; + } foreign; + } source, dest; + uint16_t len; + uint16_t flags; + int16_t status; +}; + int osdep_gnttab_open(xengnttab_handle *xgt); int osdep_gnttab_close(xengnttab_handle *xgt); @@ -23,6 +37,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt, int osdep_gnttab_unmap(xengnttab_handle *xgt, void *start_address, uint32_t count); +int osdep_gnttab_grant_copy(xengnttab_handle *xgt, + uint32_t count, + xengnttab_grant_copy_segment_t* segs); + int osdep_gntshr_open(xengntshr_handle *xgs); int osdep_gntshr_close(xengntshr_handle *xgs); diff --git a/tools/libxc/include/xenctrl_compat.h b/tools/libxc/include/xenctrl_compat.h index 93ccadb..7bb24a4 100644 --- a/tools/libxc/include/xenctrl_compat.h +++ b/tools/libxc/include/xenctrl_compat.h @@ -105,6 +105,29 @@ int xc_gnttab_munmap(xc_gnttab *xcg, int xc_gnttab_set_max_grants(xc_gnttab *xcg, uint32_t count); +union xengnttab_grant_copy_ptr { + void *virt; + struct { + uint32_t ref; + uint16_t offset; + uint16_t domid; + } foreign; +}; + +struct xengnttab_grant_copy_segment { + union xengnttab_grant_copy_ptr source, dest; + uint16_t len; + uint16_t flags; + int16_t status; +}; + +typedef struct xengnttab_grant_copy_segment xc_gnttab_grant_copy_segment_t; +typedef union xengnttab_grant_copy_ptr xc_gnttab_grant_copy_ptr_t; + +int xc_gnttab_grant_copy(xc_gnttab *xgt, + uint32_t count, + xc_gnttab_grant_copy_segment_t* segs); + typedef struct xengntdev_handle xc_gntshr; xc_gntshr *xc_gntshr_open(xentoollog_logger *logger, diff --git a/tools/libxc/xc_gnttab_compat.c b/tools/libxc/xc_gnttab_compat.c index 6f036d8..c265bb0 100644 --- a/tools/libxc/xc_gnttab_compat.c +++ b/tools/libxc/xc_gnttab_compat.c @@ -69,6 +69,13 @@ int xc_gnttab_set_max_grants(xc_gnttab *xcg, return xengnttab_set_max_grants(xcg, count); } +int xc_gnttab_grant_copy(xc_gnttab *xgt, + uint32_t count, + xc_gnttab_grant_copy_segment_t* segs) +{ + return xengnttab_grant_copy(xgt, count, segs); +} + xc_gntshr *xc_gntshr_open(xentoollog_logger *logger, unsigned open_flags) {
Implentation of interface for grant copy operation in libs and libxc. In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..) system call is invoked. In mini-os the operation is yet not implemented. For other OSs there is a dummy implementation. Signed-off-by: Paulina Szubarczyk <paulinaszubarczyk@gmail.com> --- Changes since v1: - changed the interface to call grant copy operation to match ioctl int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count, xengnttab_grant_copy_segment_t* segs) - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs /gnttab/private.h, tools/libxc/include/xenctrl_compat.h - changed the function osdep_gnttab_grant_copy which right now just call the ioctl - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map tools/include/xen-sys/Linux/gntdev.h | 21 +++++++++++++++++++++ tools/libs/gnttab/gnttab_core.c | 6 ++++++ tools/libs/gnttab/gnttab_unimp.c | 6 ++++++ tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++ tools/libs/gnttab/libxengnttab.map | 4 ++++ tools/libs/gnttab/linux.c | 18 ++++++++++++++++++ tools/libs/gnttab/minios.c | 6 ++++++ tools/libs/gnttab/private.h | 18 ++++++++++++++++++ tools/libxc/include/xenctrl_compat.h | 23 +++++++++++++++++++++++ tools/libxc/xc_gnttab_compat.c | 7 +++++++ 10 files changed, 123 insertions(+)