|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] libs, libxc: Interface for grant copy operation
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@xxxxxxxxx>
>
> ---
> Changes since v1:
> - changed the interface to call grant copy operation to match ioctl
> int xengnttab_grant_copy(xengnttab_handle *xgt,
> uint32_t count,
> xengnttab_grant_copy_segment_t* segs)
> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs
> /gnttab/private.h, tools/libxc/include/xenctrl_compat.h
> - changed the function osdep_gnttab_grant_copy which right now just call
> the ioctl
> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map
>
> tools/include/xen-sys/Linux/gntdev.h | 21 +++++++++++++++++++++
> tools/libs/gnttab/gnttab_core.c | 6 ++++++
> tools/libs/gnttab/gnttab_unimp.c | 6 ++++++
> tools/libs/gnttab/include/xengnttab.h | 14 ++++++++++++++
> tools/libs/gnttab/libxengnttab.map | 4 ++++
> tools/libs/gnttab/linux.c | 18 ++++++++++++++++++
> tools/libs/gnttab/minios.c | 6 ++++++
> tools/libs/gnttab/private.h | 18 ++++++++++++++++++
> tools/libxc/include/xenctrl_compat.h | 23 +++++++++++++++++++++++
> tools/libxc/xc_gnttab_compat.c | 7 +++++++
> 10 files changed, 123 insertions(+)
>
> diff --git a/tools/include/xen-sys/Linux/gntdev.h
> b/tools/include/xen-sys/Linux/gntdev.h
> index caf6fb4..0ca07c9 100644
> --- a/tools/include/xen-sys/Linux/gntdev.h
> +++ b/tools/include/xen-sys/Linux/gntdev.h
> @@ -147,4 +147,25 @@ struct ioctl_gntdev_unmap_notify {
> /* Send an interrupt on the indicated event channel */
> #define UNMAP_NOTIFY_SEND_EVENT 0x2
>
> +struct ioctl_gntdev_grant_copy_segment {
> + union {
> + void *virt;
> + struct {
> + uint32_t ref;
> + uint16_t offset;
> + uint16_t domid;
> + } foreign;
> + } source, dest;
> + uint16_t len;
> + uint16_t flags;
> + int16_t status;
> +};
> +
> +#define IOCTL_GNTDEV_GRANT_COPY \
> +_IOC(_IOC_NONE, 'G', 8, sizeof(struct ioctl_gntdev_grant_copy))
> +struct ioctl_gntdev_grant_copy {
> + unsigned int count;
> + struct ioctl_gntdev_grant_copy_segment *segments;
> +};
> +
This is ok.
> #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
> index 5d0474d..9badc58 100644
> --- a/tools/libs/gnttab/gnttab_core.c
> +++ b/tools/libs/gnttab/gnttab_core.c
> @@ -113,6 +113,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void
> *start_address, uint32_t count)
> return osdep_gnttab_unmap(xgt, start_address, count);
> }
>
> +int xengnttab_grant_copy(xengnttab_handle *xgt,
> + uint32_t count,
> + xengnttab_grant_copy_segment_t* segs)
> +{
> + return osdep_gnttab_grant_copy(xgt, count, segs);
> +}
> /*
> * Local variables:
> * mode: C
> diff --git a/tools/libs/gnttab/gnttab_unimp.c
> b/tools/libs/gnttab/gnttab_unimp.c
> index b3a4a20..79a5c88 100644
> --- a/tools/libs/gnttab/gnttab_unimp.c
> +++ b/tools/libs/gnttab/gnttab_unimp.c
> @@ -78,6 +78,12 @@ int xengnttab_unmap(xengnttab_handle *xgt, void
> *start_address, uint32_t count)
> abort();
> }
>
> +int xengnttab_copy_grant(xengnttab_handle *xgt,
> + uint32_t count,
> + xengnttab_copy_grant_segment_t* segs)
Coding style. Should be " *segs" instead of "* segs".
Please also fix other instances of this nit.
> +{
> + return -1;
Please use abort() here instead.
> +}
> /*
> * Local variables:
> * mode: C
> diff --git a/tools/libs/gnttab/include/xengnttab.h
> b/tools/libs/gnttab/include/xengnttab.h
> index 0431dcf..a9fa1fe 100644
> --- a/tools/libs/gnttab/include/xengnttab.h
> +++ b/tools/libs/gnttab/include/xengnttab.h
> @@ -258,6 +258,20 @@ int xengnttab_unmap(xengnttab_handle *xgt, void
> *start_address, uint32_t count);
> int xengnttab_set_max_grants(xengnttab_handle *xgt,
> uint32_t nr_grants);
>
> +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> +
> +/**
> + * Copy memory from or to grant references. The information of each
> operations
> + * are contained in 'xengnttab_grant_copy_segment_t'. The @flag value
> indicate
> + * the direction of an operation (GNTCOPY_source_gref\GNTCOPY_dest_gref).
> + *
> + * The sum of fields @offset[i] and @len[i] of
> 'xengnttab_grant_copy_segment_t'
> + * should not exceed XEN_PAGE_SIZE
> + */
> +int xengnttab_grant_copy(xengnttab_handle *xgt,
> + uint32_t count,
> + xengnttab_grant_copy_segment_t* segs);
> +
> /*
> * Grant Sharing Interface (allocating and granting pages to others)
> */
> diff --git a/tools/libs/gnttab/libxengnttab.map
> b/tools/libs/gnttab/libxengnttab.map
> index dc737ac..0928963 100644
> --- a/tools/libs/gnttab/libxengnttab.map
> +++ b/tools/libs/gnttab/libxengnttab.map
> @@ -21,3 +21,7 @@ VERS_1.0 {
> xengntshr_unshare;
> local: *; /* Do not expose anything by default */
> };
> +
> +VERS_1.1 {
Missing "global:" here?
> + xengnttab_grant_copy;
> +} VERS_1.0;
You also need to modify the "MINOR" field in gnttab/Makefile so that the
generate so file contains the correct version number.
> \ No newline at end of file
> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> index 7b0fba4..26bfbdc 100644
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -235,6 +235,24 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
> return 0;
> }
>
> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> index d286c86..22ad53a 100644
> --- a/tools/libs/gnttab/private.h
> +++ b/tools/libs/gnttab/private.h
> @@ -9,6 +9,20 @@ struct xengntdev_handle {
> int fd;
> };
>
> +struct xengnttab_copy_grant_segment {
> + union xengnttab_copy_ptr {
> + void *virt;
> + struct {
> + uint32_t ref;
> + uint16_t offset;
> + uint16_t domid;
> + } foreign;
> + } source, dest;
> + uint16_t len;
> + uint16_t flags;
> + int16_t status;
> +};
> +
The struct is more or less a direct copy of Linux structure. It is
probably fine as-is, but I don't want to risk making this library Linux
centric. If you look at other functions, they accept a bunch of discrete
arguments then assemble those arguments into OS dependent structure in
osdep functions. I know having discrete arguments for the API you want
to introduce seems cumbersome, but I want to at least tell you all the
background information needed for a thorough discussion. I would be
interested in Roger's view on this.
I apologise for not having commented on your series earlier.
> int osdep_gnttab_open(xengnttab_handle *xgt);
> int osdep_gnttab_close(xengnttab_handle *xgt);
>
> @@ -23,6 +37,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
> int osdep_gnttab_unmap(xengnttab_handle *xgt,
> void *start_address,
> uint32_t count);
> +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> + uint32_t count,
> + xengnttab_grant_copy_segment_t* segs);
> +
> int osdep_gntshr_open(xengntshr_handle *xgs);
> int osdep_gntshr_close(xengntshr_handle *xgs);
>
> diff --git a/tools/libxc/include/xenctrl_compat.h
> b/tools/libxc/include/xenctrl_compat.h
> index 93ccadb..7bb24a4 100644
> --- a/tools/libxc/include/xenctrl_compat.h
> +++ b/tools/libxc/include/xenctrl_compat.h
> @@ -105,6 +105,29 @@ int xc_gnttab_munmap(xc_gnttab *xcg,
> int xc_gnttab_set_max_grants(xc_gnttab *xcg,
> uint32_t count);
>
> +union xengnttab_grant_copy_ptr {
> + void *virt;
> + struct {
> + uint32_t ref;
> + uint16_t offset;
> + uint16_t domid;
> + } foreign;
> +};
> +
> +struct xengnttab_grant_copy_segment {
> + union xengnttab_grant_copy_ptr source, dest;
> + uint16_t len;
> + uint16_t flags;
> + int16_t status;
> +};
> +
> +typedef struct xengnttab_grant_copy_segment xc_gnttab_grant_copy_segment_t;
> +typedef union xengnttab_grant_copy_ptr xc_gnttab_grant_copy_ptr_t;
> +
> +int xc_gnttab_grant_copy(xc_gnttab *xgt,
> + uint32_t count,
> + xc_gnttab_grant_copy_segment_t* segs);
> +
The purpose of compact header is to make old application code that uses
old APIs (the xc_* functions) continue to compile by providing necessary
stubs.
This is new API, so it doesn't belong to the compat header. Anyone who
wants to use this new API should be using libxengnttab instead. We won't
be adding more xc_ functions.
I think you can just drop these changes completely.
This also means the compatibility should be on QEMU side. I think QEMU
already knows how to link against libxengnttab. What you need to do is
to actually test if the API exists and take appropriate action there.
If I'm not clear enough, feel free to ask.
Wei.
> typedef struct xengntdev_handle xc_gntshr;
>
> xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
> diff --git a/tools/libxc/xc_gnttab_compat.c b/tools/libxc/xc_gnttab_compat.c
> index 6f036d8..c265bb0 100644
> --- a/tools/libxc/xc_gnttab_compat.c
> +++ b/tools/libxc/xc_gnttab_compat.c
> @@ -69,6 +69,13 @@ int xc_gnttab_set_max_grants(xc_gnttab *xcg,
> return xengnttab_set_max_grants(xcg, count);
> }
>
> +int xc_gnttab_grant_copy(xc_gnttab *xgt,
> + uint32_t count,
> + xc_gnttab_grant_copy_segment_t* segs)
> +{
> + return xengnttab_grant_copy(xgt, count, segs);
> +}
> +
> xc_gntshr *xc_gntshr_open(xentoollog_logger *logger,
> unsigned open_flags)
> {
> --
> 1.9.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |