diff mbox series

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

Message ID 20200520090425.28558-3-andr2000@gmail.com (mailing list archive)
State New, archived
Headers show
Series displif: Protocol version 2 | expand

Commit Message

Oleksandr Andrushchenko May 20, 2020, 9:04 a.m. UTC
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 tools/include/xen-sys/Linux/gntdev.h  | 53 +++++++++++++++++
 tools/libs/gnttab/Makefile            |  2 +-
 tools/libs/gnttab/freebsd.c           | 15 +++++
 tools/libs/gnttab/gnttab_core.c       | 17 ++++++
 tools/libs/gnttab/include/xengnttab.h | 13 ++++
 tools/libs/gnttab/libxengnttab.map    |  6 ++
 tools/libs/gnttab/linux.c             | 86 +++++++++++++++++++++++++++
 tools/libs/gnttab/minios.c            | 15 +++++
 tools/libs/gnttab/private.h           |  9 +++
 9 files changed, 215 insertions(+), 1 deletion(-)

Comments

Oleksandr Andrushchenko June 30, 2020, 9:42 a.m. UTC | #1
Ian, Wei, would you mind looking at the below please?

Thank you in advance,

Oleksandr

On 5/20/20 12:04 PM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> 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
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   tools/include/xen-sys/Linux/gntdev.h  | 53 +++++++++++++++++
>   tools/libs/gnttab/Makefile            |  2 +-
>   tools/libs/gnttab/freebsd.c           | 15 +++++
>   tools/libs/gnttab/gnttab_core.c       | 17 ++++++
>   tools/libs/gnttab/include/xengnttab.h | 13 ++++
>   tools/libs/gnttab/libxengnttab.map    |  6 ++
>   tools/libs/gnttab/linux.c             | 86 +++++++++++++++++++++++++++
>   tools/libs/gnttab/minios.c            | 15 +++++
>   tools/libs/gnttab/private.h           |  9 +++
>   9 files changed, 215 insertions(+), 1 deletion(-)
>
> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
> index d16076044c71..0c43393cbee5 100644
> --- a/tools/include/xen-sys/Linux/gntdev.h
> +++ b/tools/include/xen-sys/Linux/gntdev.h
> @@ -274,4 +274,57 @@ struct ioctl_gntdev_dmabuf_imp_release {
>       uint32_t reserved;
>   };
>   
> +/*
> + * 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, \
> +         sizeof(struct ioctl_gntdev_dmabuf_exp_from_refs_v2))
> +struct ioctl_gntdev_dmabuf_exp_from_refs_v2 {
> +    /* IN parameters. */
> +    /* Specific options for this dma-buf: see GNTDEV_DMA_FLAG_XXX. */
> +    uint32_t flags;
> +    /* Number of grant references in @refs array. */
> +    uint32_t count;
> +    /* Offset of the data in the dma-buf. */
> +    uint32_t data_ofs;
> +    /* OUT parameters. */
> +    /* File descriptor of the dma-buf. */
> +    uint32_t fd;
> +    /* The domain ID of the grant references to be mapped. */
> +    uint32_t domid;
> +    /* Variable IN parameter. */
> +    /* Array of grant references of size @count. */
> +    uint32_t refs[1];
> +};
> +
> +#define IOCTL_GNTDEV_DMABUF_IMP_TO_REFS_V2 \
> +    _IOC(_IOC_NONE, 'G', 14, \
> +         sizeof(struct ioctl_gntdev_dmabuf_imp_to_refs_v2))
> +struct ioctl_gntdev_dmabuf_imp_to_refs_v2 {
> +    /* IN parameters. */
> +    /* File descriptor of the dma-buf. */
> +    uint32_t fd;
> +    /* Number of grant references in @refs array. */
> +    uint32_t count;
> +    /* The domain ID for which references to be granted. */
> +    uint32_t domid;
> +    /* Reserved - must be zero. */
> +    uint32_t reserved;
> +    /* OUT parameters. */
> +    /* Offset of the data in the dma-buf. */
> +    uint32_t data_ofs;
> +    /* Array of grant references of size @count. */
> +    uint32_t refs[1];
> +};
> +
>   #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
> diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
> index 2da8fbbb7f6f..5ee2d965214f 100644
> --- a/tools/libs/gnttab/Makefile
> +++ b/tools/libs/gnttab/Makefile
> @@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
>   include $(XEN_ROOT)/tools/Rules.mk
>   
>   MAJOR    = 1
> -MINOR    = 2
> +MINOR    = 3
>   LIBNAME  := gnttab
>   USELIBS  := toollog toolcore
>   
> diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
> index 886b588303a0..baf0f60aa4d3 100644
> --- a/tools/libs/gnttab/freebsd.c
> +++ b/tools/libs/gnttab/freebsd.c
> @@ -319,6 +319,14 @@ int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>       abort();
>   }
>   
> +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();
> +}
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms)
>   {
> @@ -331,6 +339,13 @@ int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>       abort();
>   }
>   
> +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)
> +{
> +    abort();
> +}
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       abort();
> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
> index 92e7228a2671..3af3cec80045 100644
> --- a/tools/libs/gnttab/gnttab_core.c
> +++ b/tools/libs/gnttab/gnttab_core.c
> @@ -144,6 +144,15 @@ int xengnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>                                                refs, 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)
> +{
> +    return osdep_gnttab_dmabuf_exp_from_refs_v2(xgt, domid, flags, count,
> +                                                refs, fd, data_ofs);
> +}
> +
>   int xengnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt, uint32_t fd,
>                                          uint32_t wait_to_ms)
>   {
> @@ -156,6 +165,14 @@ int xengnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>       return osdep_gnttab_dmabuf_imp_to_refs(xgt, domid, fd, count, refs);
>   }
>   
> +int xengnttab_dmabuf_imp_to_refs_v2(xengnttab_handle *xgt, uint32_t domid,
> +                                    uint32_t fd, uint32_t count, uint32_t *refs,
> +                                    uint32_t *data_ofs)
> +{
> +    return osdep_gnttab_dmabuf_imp_to_refs_v2(xgt, domid, fd, count, refs,
> +                                              data_ofs);
> +}
> +
>   int xengnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       return osdep_gnttab_dmabuf_imp_release(xgt, fd);
> 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/dma-buf.rst__;!!GF_29dbcQIUBPA!nBABkPpEyQW1_5nPE9nbyCbEaCvRjXQxOBKRpRSIGUgAdqcc0VCm4jL-9cCabcWNDS4bc_DR6Q$
>    */
>   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);
> +
>   /*
>    * This will block until the dma-buf with the file descriptor @fd is
>    * released. This is only valid for buffers created with
> @@ -345,10 +352,16 @@ int xengnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt, uint32_t fd,
>   /*
>    * Import a dma-buf with file descriptor @fd and export granted references
>    * to the pages of that dma-buf into array @refs of size @count.
> + *
> + * Version 2 also provides @data_ofs offset of the data in the buffer.
>    */
>   int xengnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>                                    uint32_t fd, uint32_t count, uint32_t *refs);
>   
> +int xengnttab_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 will close all references to an imported buffer, so it can be
>    * released by the owner. This is only valid for buffers created with
> diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
> index d2a9b7e18bea..ddf77e064b08 100644
> --- a/tools/libs/gnttab/libxengnttab.map
> +++ b/tools/libs/gnttab/libxengnttab.map
> @@ -36,3 +36,9 @@ VERS_1.2 {
>   		xengnttab_dmabuf_imp_to_refs;
>   		xengnttab_dmabuf_imp_release;
>   } VERS_1.1;
> +
> +VERS_1.3 {
> +    global:
> +		xengnttab_dmabuf_exp_from_refs_v2;
> +		xengnttab_dmabuf_imp_to_refs_v2;
> +} VERS_1.2;
> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> index a01bb6c698c6..75e249fb3202 100644
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -352,6 +352,51 @@ out:
>       return rc;
>   }
>   
> +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;
> +    }
> +
> +    *dmabuf_fd = from_refs_v2->fd;
> +    rc = 0;
> +
> +out:
> +    free(from_refs_v2);
> +    return rc;
> +}
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms)
>   {
> @@ -413,6 +458,47 @@ out:
>       return rc;
>   }
>   
> +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)
> +{
> +    struct ioctl_gntdev_dmabuf_imp_to_refs_v2 *to_refs_v2 = NULL;
> +    int rc = -1;
> +
> +    if ( !count )
> +    {
> +        errno = EINVAL;
> +        goto out;
> +    }
> +
> +    to_refs_v2 = malloc(sizeof(*to_refs_v2) +
> +                        (count - 1) * sizeof(to_refs_v2->refs[0]));
> +    if ( !to_refs_v2 )
> +    {
> +        errno = ENOMEM;
> +        goto out;
> +    }
> +
> +    to_refs_v2->fd = fd;
> +    to_refs_v2->count = count;
> +    to_refs_v2->domid = domid;
> +
> +    if ( (rc = ioctl(xgt->fd, IOCTL_GNTDEV_DMABUF_IMP_TO_REFS_V2, to_refs_v2)) )
> +    {
> +        GTERROR(xgt->logger, "ioctl DMABUF_IMP_TO_REFS_V2 failed");
> +        goto out;
> +    }
> +
> +    memcpy(refs, to_refs_v2->refs, count * sizeof(*refs));
> +    *data_ofs = to_refs_v2->data_ofs;
> +    rc = 0;
> +
> +out:
> +    free(to_refs_v2);
> +    return rc;
> +}
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       struct ioctl_gntdev_dmabuf_imp_release release;
> diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
> index f78caadd3043..298416b2a98d 100644
> --- a/tools/libs/gnttab/minios.c
> +++ b/tools/libs/gnttab/minios.c
> @@ -120,6 +120,14 @@ int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>       return -1;
>   }
>   
> +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 *fd,
> +                                         uint32_t data_ofs)
> +{
> +    return -1;
> +}
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms)
>   {
> @@ -133,6 +141,13 @@ int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>       return -1;
>   }
>   
> +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)
> +{
> +    return -1;
> +}
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       return -1;
> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> index c5e23639b141..07271637f609 100644
> --- a/tools/libs/gnttab/private.h
> +++ b/tools/libs/gnttab/private.h
> @@ -39,6 +39,11 @@ int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>                                         uint32_t flags, uint32_t count,
>                                         const uint32_t *refs, uint32_t *fd);
>   
> +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 *fd,
> +                                         uint32_t data_ofs);
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms);
>   
> @@ -46,6 +51,10 @@ int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>                                       uint32_t fd, uint32_t count,
>                                       uint32_t *refs);
>   
> +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);
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd);
>   
>   int osdep_gntshr_open(xengntshr_handle *xgs);
Oleksandr Andrushchenko July 31, 2020, 10:53 a.m. UTC | #2
Hello, Ian, Wei!

Initially I have sent this patch as a part of the series for the display protocol changes,

but later decided to split. At the moment the protocol changes are already

accepted, but this part is still missing feedback and is still wanted.

I would really appreciate if you could have a look at the change below.

Thank you in advance,

Oleksandr Andrushchenko

On 5/20/20 12:04 PM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> 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
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   tools/include/xen-sys/Linux/gntdev.h  | 53 +++++++++++++++++
>   tools/libs/gnttab/Makefile            |  2 +-
>   tools/libs/gnttab/freebsd.c           | 15 +++++
>   tools/libs/gnttab/gnttab_core.c       | 17 ++++++
>   tools/libs/gnttab/include/xengnttab.h | 13 ++++
>   tools/libs/gnttab/libxengnttab.map    |  6 ++
>   tools/libs/gnttab/linux.c             | 86 +++++++++++++++++++++++++++
>   tools/libs/gnttab/minios.c            | 15 +++++
>   tools/libs/gnttab/private.h           |  9 +++
>   9 files changed, 215 insertions(+), 1 deletion(-)
>
> diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
> index d16076044c71..0c43393cbee5 100644
> --- a/tools/include/xen-sys/Linux/gntdev.h
> +++ b/tools/include/xen-sys/Linux/gntdev.h
> @@ -274,4 +274,57 @@ struct ioctl_gntdev_dmabuf_imp_release {
>       uint32_t reserved;
>   };
>   
> +/*
> + * 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, \
> +         sizeof(struct ioctl_gntdev_dmabuf_exp_from_refs_v2))
> +struct ioctl_gntdev_dmabuf_exp_from_refs_v2 {
> +    /* IN parameters. */
> +    /* Specific options for this dma-buf: see GNTDEV_DMA_FLAG_XXX. */
> +    uint32_t flags;
> +    /* Number of grant references in @refs array. */
> +    uint32_t count;
> +    /* Offset of the data in the dma-buf. */
> +    uint32_t data_ofs;
> +    /* OUT parameters. */
> +    /* File descriptor of the dma-buf. */
> +    uint32_t fd;
> +    /* The domain ID of the grant references to be mapped. */
> +    uint32_t domid;
> +    /* Variable IN parameter. */
> +    /* Array of grant references of size @count. */
> +    uint32_t refs[1];
> +};
> +
> +#define IOCTL_GNTDEV_DMABUF_IMP_TO_REFS_V2 \
> +    _IOC(_IOC_NONE, 'G', 14, \
> +         sizeof(struct ioctl_gntdev_dmabuf_imp_to_refs_v2))
> +struct ioctl_gntdev_dmabuf_imp_to_refs_v2 {
> +    /* IN parameters. */
> +    /* File descriptor of the dma-buf. */
> +    uint32_t fd;
> +    /* Number of grant references in @refs array. */
> +    uint32_t count;
> +    /* The domain ID for which references to be granted. */
> +    uint32_t domid;
> +    /* Reserved - must be zero. */
> +    uint32_t reserved;
> +    /* OUT parameters. */
> +    /* Offset of the data in the dma-buf. */
> +    uint32_t data_ofs;
> +    /* Array of grant references of size @count. */
> +    uint32_t refs[1];
> +};
> +
>   #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
> diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
> index 2da8fbbb7f6f..5ee2d965214f 100644
> --- a/tools/libs/gnttab/Makefile
> +++ b/tools/libs/gnttab/Makefile
> @@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
>   include $(XEN_ROOT)/tools/Rules.mk
>   
>   MAJOR    = 1
> -MINOR    = 2
> +MINOR    = 3
>   LIBNAME  := gnttab
>   USELIBS  := toollog toolcore
>   
> diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
> index 886b588303a0..baf0f60aa4d3 100644
> --- a/tools/libs/gnttab/freebsd.c
> +++ b/tools/libs/gnttab/freebsd.c
> @@ -319,6 +319,14 @@ int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>       abort();
>   }
>   
> +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();
> +}
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms)
>   {
> @@ -331,6 +339,13 @@ int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>       abort();
>   }
>   
> +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)
> +{
> +    abort();
> +}
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       abort();
> diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
> index 92e7228a2671..3af3cec80045 100644
> --- a/tools/libs/gnttab/gnttab_core.c
> +++ b/tools/libs/gnttab/gnttab_core.c
> @@ -144,6 +144,15 @@ int xengnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>                                                refs, 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)
> +{
> +    return osdep_gnttab_dmabuf_exp_from_refs_v2(xgt, domid, flags, count,
> +                                                refs, fd, data_ofs);
> +}
> +
>   int xengnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt, uint32_t fd,
>                                          uint32_t wait_to_ms)
>   {
> @@ -156,6 +165,14 @@ int xengnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>       return osdep_gnttab_dmabuf_imp_to_refs(xgt, domid, fd, count, refs);
>   }
>   
> +int xengnttab_dmabuf_imp_to_refs_v2(xengnttab_handle *xgt, uint32_t domid,
> +                                    uint32_t fd, uint32_t count, uint32_t *refs,
> +                                    uint32_t *data_ofs)
> +{
> +    return osdep_gnttab_dmabuf_imp_to_refs_v2(xgt, domid, fd, count, refs,
> +                                              data_ofs);
> +}
> +
>   int xengnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       return osdep_gnttab_dmabuf_imp_release(xgt, fd);
> 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);
> +
>   /*
>    * This will block until the dma-buf with the file descriptor @fd is
>    * released. This is only valid for buffers created with
> @@ -345,10 +352,16 @@ int xengnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt, uint32_t fd,
>   /*
>    * Import a dma-buf with file descriptor @fd and export granted references
>    * to the pages of that dma-buf into array @refs of size @count.
> + *
> + * Version 2 also provides @data_ofs offset of the data in the buffer.
>    */
>   int xengnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>                                    uint32_t fd, uint32_t count, uint32_t *refs);
>   
> +int xengnttab_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 will close all references to an imported buffer, so it can be
>    * released by the owner. This is only valid for buffers created with
> diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
> index d2a9b7e18bea..ddf77e064b08 100644
> --- a/tools/libs/gnttab/libxengnttab.map
> +++ b/tools/libs/gnttab/libxengnttab.map
> @@ -36,3 +36,9 @@ VERS_1.2 {
>   		xengnttab_dmabuf_imp_to_refs;
>   		xengnttab_dmabuf_imp_release;
>   } VERS_1.1;
> +
> +VERS_1.3 {
> +    global:
> +		xengnttab_dmabuf_exp_from_refs_v2;
> +		xengnttab_dmabuf_imp_to_refs_v2;
> +} VERS_1.2;
> diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
> index a01bb6c698c6..75e249fb3202 100644
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -352,6 +352,51 @@ out:
>       return rc;
>   }
>   
> +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;
> +    }
> +
> +    *dmabuf_fd = from_refs_v2->fd;
> +    rc = 0;
> +
> +out:
> +    free(from_refs_v2);
> +    return rc;
> +}
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms)
>   {
> @@ -413,6 +458,47 @@ out:
>       return rc;
>   }
>   
> +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)
> +{
> +    struct ioctl_gntdev_dmabuf_imp_to_refs_v2 *to_refs_v2 = NULL;
> +    int rc = -1;
> +
> +    if ( !count )
> +    {
> +        errno = EINVAL;
> +        goto out;
> +    }
> +
> +    to_refs_v2 = malloc(sizeof(*to_refs_v2) +
> +                        (count - 1) * sizeof(to_refs_v2->refs[0]));
> +    if ( !to_refs_v2 )
> +    {
> +        errno = ENOMEM;
> +        goto out;
> +    }
> +
> +    to_refs_v2->fd = fd;
> +    to_refs_v2->count = count;
> +    to_refs_v2->domid = domid;
> +
> +    if ( (rc = ioctl(xgt->fd, IOCTL_GNTDEV_DMABUF_IMP_TO_REFS_V2, to_refs_v2)) )
> +    {
> +        GTERROR(xgt->logger, "ioctl DMABUF_IMP_TO_REFS_V2 failed");
> +        goto out;
> +    }
> +
> +    memcpy(refs, to_refs_v2->refs, count * sizeof(*refs));
> +    *data_ofs = to_refs_v2->data_ofs;
> +    rc = 0;
> +
> +out:
> +    free(to_refs_v2);
> +    return rc;
> +}
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       struct ioctl_gntdev_dmabuf_imp_release release;
> diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
> index f78caadd3043..298416b2a98d 100644
> --- a/tools/libs/gnttab/minios.c
> +++ b/tools/libs/gnttab/minios.c
> @@ -120,6 +120,14 @@ int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>       return -1;
>   }
>   
> +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 *fd,
> +                                         uint32_t data_ofs)
> +{
> +    return -1;
> +}
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms)
>   {
> @@ -133,6 +141,13 @@ int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>       return -1;
>   }
>   
> +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)
> +{
> +    return -1;
> +}
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
>   {
>       return -1;
> diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
> index c5e23639b141..07271637f609 100644
> --- a/tools/libs/gnttab/private.h
> +++ b/tools/libs/gnttab/private.h
> @@ -39,6 +39,11 @@ int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
>                                         uint32_t flags, uint32_t count,
>                                         const uint32_t *refs, uint32_t *fd);
>   
> +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 *fd,
> +                                         uint32_t data_ofs);
> +
>   int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
>                                             uint32_t fd, uint32_t wait_to_ms);
>   
> @@ -46,6 +51,10 @@ int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
>                                       uint32_t fd, uint32_t count,
>                                       uint32_t *refs);
>   
> +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);
> +
>   int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd);
>   
>   int osdep_gntshr_open(xengntshr_handle *xgs);
Ian Jackson Sept. 28, 2020, 3:20 p.m. UTC | #3
Oleksandr Andrushchenko writes ("[PATCH 2/2] libgnttab: Add support for Linux dma-buf offset"):
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> 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.
Oleksandr Andrushchenko Oct. 1, 2020, 6:35 a.m. UTC | #4
Hi,

On 9/28/20 6:20 PM, Ian Jackson wrote:
> Oleksandr Andrushchenko writes ("[PATCH 2/2] libgnttab: Add support for Linux dma-buf offset"):
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> 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 ?

I mean that the relevant protocol changes are already part of both Xen [1]

and Linux trees [2]. What is missing is ioctl implementation in the kernel and

its support in Xen' tools. This is why I have marked the patch as RFC in order

to get some view on the matter from Xen community. Once we agree on the

naming, structure etc. I'll send patches for both Xen and Linux

>
>> +/*
>> + * 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.
This is not upstream yet, please see explanation above
>    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.

First of all, Linux dma-bufs are only supported on Linux, so neither FreeBSD nor Mini-OS

will have that. If you are referring to "abort()" here, so I am just aligning to what previously

was there, e.g. all non-relevant dma-buf OS specifics were implemented like that.

>
> 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.
Yes
>
> 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 ?

Well, this is a tough story. If we have new software and new library, but old

kernel it means that the offset we are trying to get with the new ioctl will be

unavailable to that new software. In most cases we can use offset of 0, but some

platforms (iMX8) use offset of 64. So, we can workaround that for most(?) platforms

by reporting offset 0, but some platforms will fail. I am not sure if this is good to state that

this combination of software (as described above) "will mostly work" or just let

the system fail at run-time, by letting Linux return ENOTSUPP for the new ioctl.

By fail I mean that the display backend may decide if to use the previous version of the ioctl

without the offset field.

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

Well, as I said before, for the platforms with offset 0 we are "fine" ignoring the offset and

using v1 of the ioctl without the offset field. For the platforms with non-zero offset it results

at least in slight screen distortion and they do need v2 of the ioctl

>
> What architectures is the new Linux ioctl available on ?
x86/ARM
>
>> 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/dma-buf.rst__;!!GF_29dbcQIUBPA!ia7gsD5o9vPtgQzm3pUYmcPEaUmaODZRUkyniq74vNDZkbz9zGbqe-zCUesHHF3-ckRWLuIBKg$ [elixir[.]bootlin[.]com]
>>    */
>>   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.
Sure
>
> Conversely there is no need to duplicate information between the patch
> contents, and the commit message.

It's just a me that always wants the doc at handy location so I don't need to dig for

the commit messages? But at the same time the commit message should allow one

quickly understand what's in there. So, I would prefer to have more description in the

patch then

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

I don't actually like v2. Neither I can produce anything more cute ;)

On the other hand it is easier to understand that v2 is actually extends/removes/changes

something that was here before. Say, if you have 2 ioctls yyy and ddd you need to compare

the two to understand what is more relevant at the moment. Having explicit version in the

name leaves no doubt about what is newer.

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

Well, I have little to add as the only change and the reason is that scatter-gather table's

offset must be honored which was not a problem until we faced iMX8 platform which has

that offset non-zero. Frankly, lots of software assumes it is zero...

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

I hate having code duplication as well: less code less maintenance. But in this case

the common code makes that function full of "if"s so finally I gave up and make a copy-paste.

No strong opinion here: if you think "if"s are still better I'll rework that

>
> What do my co-maintainers think ?
>
>
> Regards,
> Ian.

Thank you for the review and your time,

Oleksandr

[1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f92337b6bffb3d9e509024d6ef5c3f2b112757d
Oleksandr Andrushchenko June 8, 2021, 7:54 a.m. UTC | #5
Hello, all!

I would like to bring back this old thread as it seems it has stuck long 
time ago

without clear NAck or Ack. I didn't rebase the changes because the 
change itself

requires answers on the way we should go here: new ioctl (seems to be 
better) or

extension of the existing one (not so great)

Thank you in advance,

Oleksandr

On 01.10.20 09:35, Oleksandr Andrushchenko wrote:
> Hi,
>
> On 9/28/20 6:20 PM, Ian Jackson wrote:
>> Oleksandr Andrushchenko writes ("[PATCH 2/2] libgnttab: Add support for Linux dma-buf offset"):
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> 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 ?
> I mean that the relevant protocol changes are already part of both Xen [1]
>
> and Linux trees [2]. What is missing is ioctl implementation in the kernel and
>
> its support in Xen' tools. This is why I have marked the patch as RFC in order
>
> to get some view on the matter from Xen community. Once we agree on the
>
> naming, structure etc. I'll send patches for both Xen and Linux
>
>>> +/*
>>> + * 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.
> This is not upstream yet, please see explanation above
>>     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.
> First of all, Linux dma-bufs are only supported on Linux, so neither FreeBSD nor Mini-OS
>
> will have that. If you are referring to "abort()" here, so I am just aligning to what previously
>
> was there, e.g. all non-relevant dma-buf OS specifics were implemented like that.
>
>> 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.
> Yes
>> 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 ?
> Well, this is a tough story. If we have new software and new library, but old
>
> kernel it means that the offset we are trying to get with the new ioctl will be
>
> unavailable to that new software. In most cases we can use offset of 0, but some
>
> platforms (iMX8) use offset of 64. So, we can workaround that for most(?) platforms
>
> by reporting offset 0, but some platforms will fail. I am not sure if this is good to state that
>
> this combination of software (as described above) "will mostly work" or just let
>
> the system fail at run-time, by letting Linux return ENOTSUPP for the new ioctl.
>
> By fail I mean that the display backend may decide if to use the previous version of the ioctl
>
> without the offset field.
>
>> 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.
> Well, as I said before, for the platforms with offset 0 we are "fine" ignoring the offset and
>
> using v1 of the ioctl without the offset field. For the platforms with non-zero offset it results
>
> at least in slight screen distortion and they do need v2 of the ioctl
>
>> What architectures is the new Linux ioctl available on ?
> x86/ARM
>>> 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/dma-buf.rst__;!!GF_29dbcQIUBPA!ia7gsD5o9vPtgQzm3pUYmcPEaUmaODZRUkyniq74vNDZkbz9zGbqe-zCUesHHF3-ckRWLuIBKg$ [elixir[.]bootlin[.]com]
>>>     */
>>>    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.
> Sure
>> Conversely there is no need to duplicate information between the patch
>> contents, and the commit message.
> It's just a me that always wants the doc at handy location so I don't need to dig for
>
> the commit messages? But at the same time the commit message should allow one
>
> quickly understand what's in there. So, I would prefer to have more description in the
>
> patch then
>
>> 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.
> I don't actually like v2. Neither I can produce anything more cute ;)
>
> On the other hand it is easier to understand that v2 is actually extends/removes/changes
>
> something that was here before. Say, if you have 2 ioctls yyy and ddd you need to compare
>
> the two to understand what is more relevant at the moment. Having explicit version in the
>
> name leaves no doubt about what is newer.
>
>>> +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.
> Well, I have little to add as the only change and the reason is that scatter-gather table's
>
> offset must be honored which was not a problem until we faced iMX8 platform which has
>
> that offset non-zero. Frankly, lots of software assumes it is zero...
>
>>> +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.
> I hate having code duplication as well: less code less maintenance. But in this case
>
> the common code makes that function full of "if"s so finally I gave up and make a copy-paste.
>
> No strong opinion here: if you think "if"s are still better I'll rework that
>
>> What do my co-maintainers think ?
>>
>>
>> Regards,
>> Ian.
> Thank you for the review and your time,
>
> Oleksandr
>
> [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=c27a184225eab54d20435c8cab5ad0ef384dc2c0
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f92337b6bffb3d9e509024d6ef5c3f2b112757d
>
diff mbox series

Patch

diff --git a/tools/include/xen-sys/Linux/gntdev.h b/tools/include/xen-sys/Linux/gntdev.h
index d16076044c71..0c43393cbee5 100644
--- a/tools/include/xen-sys/Linux/gntdev.h
+++ b/tools/include/xen-sys/Linux/gntdev.h
@@ -274,4 +274,57 @@  struct ioctl_gntdev_dmabuf_imp_release {
     uint32_t reserved;
 };
 
+/*
+ * 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, \
+         sizeof(struct ioctl_gntdev_dmabuf_exp_from_refs_v2))
+struct ioctl_gntdev_dmabuf_exp_from_refs_v2 {
+    /* IN parameters. */
+    /* Specific options for this dma-buf: see GNTDEV_DMA_FLAG_XXX. */
+    uint32_t flags;
+    /* Number of grant references in @refs array. */
+    uint32_t count;
+    /* Offset of the data in the dma-buf. */
+    uint32_t data_ofs;
+    /* OUT parameters. */
+    /* File descriptor of the dma-buf. */
+    uint32_t fd;
+    /* The domain ID of the grant references to be mapped. */
+    uint32_t domid;
+    /* Variable IN parameter. */
+    /* Array of grant references of size @count. */
+    uint32_t refs[1];
+};
+
+#define IOCTL_GNTDEV_DMABUF_IMP_TO_REFS_V2 \
+    _IOC(_IOC_NONE, 'G', 14, \
+         sizeof(struct ioctl_gntdev_dmabuf_imp_to_refs_v2))
+struct ioctl_gntdev_dmabuf_imp_to_refs_v2 {
+    /* IN parameters. */
+    /* File descriptor of the dma-buf. */
+    uint32_t fd;
+    /* Number of grant references in @refs array. */
+    uint32_t count;
+    /* The domain ID for which references to be granted. */
+    uint32_t domid;
+    /* Reserved - must be zero. */
+    uint32_t reserved;
+    /* OUT parameters. */
+    /* Offset of the data in the dma-buf. */
+    uint32_t data_ofs;
+    /* Array of grant references of size @count. */
+    uint32_t refs[1];
+};
+
 #endif /* __LINUX_PUBLIC_GNTDEV_H__ */
diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index 2da8fbbb7f6f..5ee2d965214f 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -2,7 +2,7 @@  XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 MAJOR    = 1
-MINOR    = 2
+MINOR    = 3
 LIBNAME  := gnttab
 USELIBS  := toollog toolcore
 
diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 886b588303a0..baf0f60aa4d3 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -319,6 +319,14 @@  int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
     abort();
 }
 
+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();
+}
+
 int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
                                           uint32_t fd, uint32_t wait_to_ms)
 {
@@ -331,6 +339,13 @@  int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
     abort();
 }
 
+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)
+{
+    abort();
+}
+
 int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
 {
     abort();
diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 92e7228a2671..3af3cec80045 100644
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -144,6 +144,15 @@  int xengnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
                                              refs, 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)
+{
+    return osdep_gnttab_dmabuf_exp_from_refs_v2(xgt, domid, flags, count,
+                                                refs, fd, data_ofs);
+}
+
 int xengnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt, uint32_t fd,
                                        uint32_t wait_to_ms)
 {
@@ -156,6 +165,14 @@  int xengnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
     return osdep_gnttab_dmabuf_imp_to_refs(xgt, domid, fd, count, refs);
 }
 
+int xengnttab_dmabuf_imp_to_refs_v2(xengnttab_handle *xgt, uint32_t domid,
+                                    uint32_t fd, uint32_t count, uint32_t *refs,
+                                    uint32_t *data_ofs)
+{
+    return osdep_gnttab_dmabuf_imp_to_refs_v2(xgt, domid, fd, count, refs,
+                                              data_ofs);
+}
+
 int xengnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
 {
     return osdep_gnttab_dmabuf_imp_release(xgt, fd);
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);
+
 /*
  * This will block until the dma-buf with the file descriptor @fd is
  * released. This is only valid for buffers created with
@@ -345,10 +352,16 @@  int xengnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt, uint32_t fd,
 /*
  * Import a dma-buf with file descriptor @fd and export granted references
  * to the pages of that dma-buf into array @refs of size @count.
+ *
+ * Version 2 also provides @data_ofs offset of the data in the buffer.
  */
 int xengnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
                                  uint32_t fd, uint32_t count, uint32_t *refs);
 
+int xengnttab_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 will close all references to an imported buffer, so it can be
  * released by the owner. This is only valid for buffers created with
diff --git a/tools/libs/gnttab/libxengnttab.map b/tools/libs/gnttab/libxengnttab.map
index d2a9b7e18bea..ddf77e064b08 100644
--- a/tools/libs/gnttab/libxengnttab.map
+++ b/tools/libs/gnttab/libxengnttab.map
@@ -36,3 +36,9 @@  VERS_1.2 {
 		xengnttab_dmabuf_imp_to_refs;
 		xengnttab_dmabuf_imp_release;
 } VERS_1.1;
+
+VERS_1.3 {
+    global:
+		xengnttab_dmabuf_exp_from_refs_v2;
+		xengnttab_dmabuf_imp_to_refs_v2;
+} VERS_1.2;
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index a01bb6c698c6..75e249fb3202 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -352,6 +352,51 @@  out:
     return rc;
 }
 
+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;
+    }
+
+    *dmabuf_fd = from_refs_v2->fd;
+    rc = 0;
+
+out:
+    free(from_refs_v2);
+    return rc;
+}
+
 int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
                                           uint32_t fd, uint32_t wait_to_ms)
 {
@@ -413,6 +458,47 @@  out:
     return rc;
 }
 
+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)
+{
+    struct ioctl_gntdev_dmabuf_imp_to_refs_v2 *to_refs_v2 = NULL;
+    int rc = -1;
+
+    if ( !count )
+    {
+        errno = EINVAL;
+        goto out;
+    }
+
+    to_refs_v2 = malloc(sizeof(*to_refs_v2) +
+                        (count - 1) * sizeof(to_refs_v2->refs[0]));
+    if ( !to_refs_v2 )
+    {
+        errno = ENOMEM;
+        goto out;
+    }
+
+    to_refs_v2->fd = fd;
+    to_refs_v2->count = count;
+    to_refs_v2->domid = domid;
+
+    if ( (rc = ioctl(xgt->fd, IOCTL_GNTDEV_DMABUF_IMP_TO_REFS_V2, to_refs_v2)) )
+    {
+        GTERROR(xgt->logger, "ioctl DMABUF_IMP_TO_REFS_V2 failed");
+        goto out;
+    }
+
+    memcpy(refs, to_refs_v2->refs, count * sizeof(*refs));
+    *data_ofs = to_refs_v2->data_ofs;
+    rc = 0;
+
+out:
+    free(to_refs_v2);
+    return rc;
+}
+
 int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
 {
     struct ioctl_gntdev_dmabuf_imp_release release;
diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
index f78caadd3043..298416b2a98d 100644
--- a/tools/libs/gnttab/minios.c
+++ b/tools/libs/gnttab/minios.c
@@ -120,6 +120,14 @@  int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
     return -1;
 }
 
+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 *fd,
+                                         uint32_t data_ofs)
+{
+    return -1;
+}
+
 int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
                                           uint32_t fd, uint32_t wait_to_ms)
 {
@@ -133,6 +141,13 @@  int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
     return -1;
 }
 
+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)
+{
+    return -1;
+}
+
 int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd)
 {
     return -1;
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index c5e23639b141..07271637f609 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -39,6 +39,11 @@  int osdep_gnttab_dmabuf_exp_from_refs(xengnttab_handle *xgt, uint32_t domid,
                                       uint32_t flags, uint32_t count,
                                       const uint32_t *refs, uint32_t *fd);
 
+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 *fd,
+                                         uint32_t data_ofs);
+
 int osdep_gnttab_dmabuf_exp_wait_released(xengnttab_handle *xgt,
                                           uint32_t fd, uint32_t wait_to_ms);
 
@@ -46,6 +51,10 @@  int osdep_gnttab_dmabuf_imp_to_refs(xengnttab_handle *xgt, uint32_t domid,
                                     uint32_t fd, uint32_t count,
                                     uint32_t *refs);
 
+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);
+
 int osdep_gnttab_dmabuf_imp_release(xengnttab_handle *xgt, uint32_t fd);
 
 int osdep_gntshr_open(xengntshr_handle *xgs);