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

[David Vrabel <david.vrabel@citrix.com>]

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

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