Re: [PATCH 2/2] libgnttab: Add support for Linux dma-buf offset

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Oleksandr Andrushchenko writes ("[PATCH 2/2] libgnttab: Add support for Linux 
dma-buf offset"):
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Add version 2 of the dma-buf ioctls which adds data_ofs parameter.
> 
> dma-buf is backed by a scatter-gather table and has offset parameter
> which tells where the actual data starts. Relevant ioctls are extended
> to support that offset:
>   - when dma-buf is created (exported) from grant references then
>     data_ofs is used to set the offset field in the scatter list
>     of the new dma-buf
>   - when dma-buf is imported and grant references provided then
>     data_ofs is used to report that offset to user-space

Thanks.  I'm not a DMA expert, but I think this is probably going in
roughly the right direction.  I will probably want a review from a DMA
expert too, but let me get on with my questions:

When you say "the protocol changes are already accepted" I think you
mean the Linux ioctl changes ?  If not, what *do* you mean ?

> +/*
> + * Version 2 of the ioctls adds @data_ofs parameter.
> + *
> + * dma-buf is backed by a scatter-gather table and has offset
> + * parameter which tells where the actual data starts.
> + * Relevant ioctls are extended to support that offset:
> + *   - when dma-buf is created (exported) from grant references then
> + *     @data_ofs is used to set the offset field in the scatter list
> + *     of the new dma-buf
> + *   - when dma-buf is imported and grant references are provided then
> + *     @data_ofs is used to report that offset to user-space
> + */
> +#define IOCTL_GNTDEV_DMABUF_EXP_FROM_REFS_V2 \
> +    _IOC(_IOC_NONE, 'G', 13, \

I think this was copied from a Linux header file ?  If so please quote
the precise file and revision in the commit message.  And be sure to
copy the copyright informtaion appropriately.

> +int osdep_gnttab_dmabuf_exp_from_refs_v2(xengnttab_handle *xgt, uint32_t 
> domid,
> +                                         uint32_t flags, uint32_t count,
> +                                         const uint32_t *refs,
> +                                         uint32_t *dmabuf_fd, uint32_t 
> data_ofs)
> +{
> +    abort();

I'm pretty sure this is wrong.

This leads me to ask about compatibility, both across versions of the
various components, and API compatibility across different platforms.

libxengnttab is supposed to have a stable API and ABI.  This means
that old programs should work with the new library - which I think you
have achieved.

But I think it also means that it should work with new programs, and
the new library, on old kernels.  What is your compatibility story
here ?  What is the intended mode of use by an application ?

And the same application code should be useable, so far as possible,
across different plaatforms that support Xen.

What fallback would be possible for application do if the v2 function
is not available ?  I think that fallback action needs to be
selectable at runtime, to support new userspace on old kernels.

What architectures is the new Linux ioctl available on ?


> diff --git a/tools/libs/gnttab/include/xengnttab.h 
> b/tools/libs/gnttab/include/xengnttab.h
> index 111fc88caeb3..0956bd91e0df 100644
> --- a/tools/libs/gnttab/include/xengnttab.h
> +++ b/tools/libs/gnttab/include/xengnttab.h
> @@ -322,12 +322,19 @@ int xengnttab_grant_copy(xengnttab_handle *xgt,
>   * Returns 0 if dma-buf was successfully created and the corresponding
>   * dma-buf's file descriptor is returned in @fd.
>   *
> +
> + * Version 2 also accepts @data_ofs offset of the data in the buffer.
> + *
>   * [1] 
> https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/dma-buf.rst
>   */
>  int xengnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>                                     uint32_t flags, uint32_t count,
>                                     const uint32_t *refs, uint32_t *fd);
>  
> +int xengnttab_dmabuf_exp_from_refs_v2(xengnttab_handle *xgt, uint32_t domid,
> +                                      uint32_t flags, uint32_t count,
> +                                      const uint32_t *refs, uint32_t *fd,
> +                                      uint32_t data_ofs);

I think the information about the meaning of @data_ofs must be in the
doc comment.  Indeed, that should be the primary location.

Conversely there is no need to duplicate information between the patch
contents, and the commit message.

Is _v2 really the best name for this ?  Are we likely to want to
extend this again in future ?  Perhaps it should be called ..._offset
or something ?  Please think about this and tell me your opinion.

> +int osdep_gnttab_dmabuf_exp_from_refs_v2(xengnttab_handle *xgt, uint32_t 
> domid,
> +                                         uint32_t flags, uint32_t count,
> +                                         const uint32_t *refs,
> +                                         uint32_t *dmabuf_fd,
> +                                         uint32_t data_ofs)
> +{
> +    struct ioctl_gntdev_dmabuf_exp_from_refs_v2 *from_refs_v2 = NULL;
> +    int rc = -1;
> +
> +    if ( !count )
> +    {
> +        errno = EINVAL;
> +        goto out;
> +    }
> +
> +    from_refs_v2 = malloc(sizeof(*from_refs_v2) +
> +                          (count - 1) * sizeof(from_refs_v2->refs[0]));
> +    if ( !from_refs_v2 )
> +    {
> +        errno = ENOMEM;
> +        goto out;
> +    }
> +
> +    from_refs_v2->flags = flags;
> +    from_refs_v2->count = count;
> +    from_refs_v2->domid = domid;
> +    from_refs_v2->data_ofs = data_ofs;
> +
> +    memcpy(from_refs_v2->refs, refs, count * sizeof(from_refs_v2->refs[0]));
> +
> +    if ( (rc = ioctl(xgt->fd, IOCTL_GNTDEV_DMABUF_EXP_FROM_REFS_V2,
> +                     from_refs_v2)) )
> +    {
> +        GTERROR(xgt->logger, "ioctl DMABUF_EXP_FROM_REFS_V2 failed");
> +        goto out;
> +    }

This seems just a fairly obvious wrapper for this ioctl.  I think it
would be best for me to review this in detail with reference to the
ioctl documentation (which you helpfully refer to - thank you!) after
I see the answers to my other questions.

> +int osdep_gnttab_dmabuf_imp_to_refs_v2(xengnttab_handle *xgt, uint32_t domid,
> +                                       uint32_t fd, uint32_t count,
> +                                       uint32_t *refs,
> +                                       uint32_t *data_ofs)
> +{

This function is very similar to the previous one.  I'm uncomfortable
with the duplication, but I see that
   osdep_gnttab_dmabuf_{imp_to,exp_from}_refs
are very duplicative already, so I am also somewhat uncomfortable with
asking you to clean this up with refactoring.  But perhaps if you felt
like thinking about combioning some of this, that might be nice.

What do my co-maintainers think ?


Regards,
Ian.