Re: [Xen-devel] [PATCH v2 1/2] libs, libxc: Interface for grant copy operation

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@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.
> 
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel