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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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

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