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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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