Re: [PATCH v13 6/9] vfio/device: Add support for creating dmabuf from multiple ranges

[Markus Armbruster <armbru@redhat.com>]

Cédric Le Goater <clg@redhat.com> writes:

> On 10/13/26 06:06, Kasireddy, Vivek wrote:
>> Hi Akihiko,
>> 
>>> Subject: Re: [PATCH v13 6/9] vfio/device: Add support for creating
>>> dmabuf from multiple ranges
>>>
>>>>>
>>>>>> In order to create a dmabuf associated with a buffer that spans
>>>>>> multiple ranges, we first need to identify the VFIO region and
>>>>>> index the buffer (represented by iovec) belongs to and then
>>>>>> translate its addresses to offsets within that region.
>>>>>>
>>>>>> The qemu_ram_block_from_host() API gives us both the region and
>>>>>> the offset info we need to populate the dma ranges so that we can
>>>>>> invoke the VFIO_DEVICE_FEATURE_DMA_BUF feature to create the dmabuf.
>>>>>>
>>>>>> Also, instead of iterating over all QOM devices to find the
>>>>>> VFIODevice associated with a memory region, introduce a helper
>>>>>> to just use the vfio_device_list to lookup the VFIODevice.
>>>>>> And, introduce another helper to lookup the VFIO region given an
>>>>>> address.
>>>>>>
>>>>>> Lastly, introduce an enum to return the type of error encountered
>>>>>> while creating the dmabuf fd.
>>>>>>
>>>>>> Cc: Alex Williamson <alex@shazbot.org>
>>>>>> Cc: Cédric Le Goater <clg@redhat.com>
>>>>>> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>>>>> ---
>>>>>>     include/hw/vfio/vfio-device.h |  22 +++++++
>>>>>>     hw/vfio/device.c              | 109 ++++++++++++++++++++++++++++++++++
>>>>>>     hw/vfio/dmabuf-stubs.c        |  17 ++++++
>>>>>>     hw/vfio/meson.build           |   1 +
>>>>>>     4 files changed, 149 insertions(+)
>>>>>>     create mode 100644 hw/vfio/dmabuf-stubs.c
>>>>>>
>>>>>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>>>>>> index 8472420d3f..c05430be8a 100644
>>>>>> --- a/include/hw/vfio/vfio-device.h
>>>>>> +++ b/include/hw/vfio/vfio-device.h
>>>>>> @@ -41,6 +41,13 @@ enum {
>>>>>>         VFIO_DEVICE_TYPE_AP = 3,
>>>>>>     };
>>>>>>
>>>>>> +enum {
>>>>>> +    /* The Guest passed addresses stored in IOV are invalid */
>>>>>> +    VFIO_DMABUF_CREATE_ERR_INVALID_IOV = -1,
>>>>>> +    /* Guest or Host may be at fault for this type of error */
>>>>>> +    VFIO_DMABUF_CREATE_ERR_UNSPEC = -2,
>>>>>> +};
>>>>>
>>>>> Nack.
>>>>>
>>>>> As said a few times already : it's a VFIO-specific enum leaking
>>>>> into vfio-device.h to serve virtio-gpu's fallback logic. The
>>>>> cleanest alternative is to use standard -errno returns and
>>>>> make the fd an output parameter.
>>>> It looks like Cedric remains unconvinced about the value or usefulness
>>>> of this error enum. Therefore, to make progress, I intend to adopt his
>>>> suggestion to use standard -errno returns for the next version, considering
>>>> that this series has been stuck for a year.
>>>>
>>>> AFAICS, dropping this error enum means that we would not be able to
>>>> clearly identify and log (or attribute) Guest vs Host errors in virtio-gpu,
>>>> particularly the INVALID_IOV case. I am not sure if this can be considered
>>>> a significant limitation. However, if there is a reasonable middle ground
>>>> solution or an alternative that you can think of that does not involve
>>>> using VFIO error enums in virtio-gpu, then I would like to pursue that
>>>> instead.
>>>
>>> Cédric's comment is a reiteration of previous points, but his concrete
>>> code suggestion is new and brings to light some challenges with errno
>>> propagation.
>>>
>>> First, errno does not precisely match the required semantics. We need
>>> to
>>> distinguish lookup failures from the other errors. However, with your
>>> suggestion, lookup failures become -EINVAL and
>>> vfio_device_get_feature()
>>> also returns the same value when the backend lacks the operation,
>>> making
>>> it impossible to distinguish them.

This matters only when something actually needs to distinguish them.

If -EINVAL on lookup failure bothers you, consider -ENOENT.

>>> Propagating errno also requires extra care to preserve it when
>>> necessary. For patch "[PATCH v13 8/9] virtio-gpu-dmabuf: Improve error
>>> handling with 'Error **' and err enum", you suggested to add the
>>> "saved_errno" variable to save errno before error_setg_errno(). That is
>>> redundant because error_setg_errno() preserves errno. Realizing that
>>> requires careful attention.
>>>
>>> While we could spend effort fixing these issues, we can simply avoid the
>>> churn by not using errno.

Two separate styles:

1. Libc commonly sets @errno and returns a single error value.  Care may
   indeed be necessary to preserve errno when passing errors up the
   stack in this style.

2. We commonly use negative errno codes as error values.  Preserving
   @errno is a non-issue then.

>>> In any case, I agree keeping this stuck is not ideal while I don't think
>>> I can provide any further insights. So I'm inviting Markus Armbruster to
>> Looks like you forgot to cc Markus. I have added him now.
>>
>>> this discussion. This is an area better handled by him, who maintains
>>> the error reporting facility, including include/qapi/error.h, which
>>> extensively documents codebase-wide error-handling convention.
>>>
>>> Markus, could you advise on the appropriate convention for returning
>>> this kind of classification alongside Error **? Your perspective
>>> across the codebase may help us settle on a consistent approach
>>> beyond the individual subsystems involved here. Perhaps you may
>>> improve
>>> the documentation according to a design decision made here.

error.h indeed recommends use of negative integers as error values, but
doesn't recommend any particular encoding.  Perhaps it should mention
the common practice of using negative errno codes.

>> Markus, here is some additional context from Akihiko as to why there is
>> a need to introduce an error enum in VFIO:
>> https://lore.kernel.org/qemu-devel/09f02fe2-63cb-4283-899e-4cffc5f60cf1@rsg.ci.i.u-tokyo.ac.jp/
>> 
>>>
>>> If anyone else has a clean middle-ground solution in the meantime, I
>>> would greatly appreciate your input as well.
>> I am not sure if it can be considered clean or elegant but one middle-ground
>> solution I can think of is to have (and use) the error enum just in virtio-gpu
>> only but associate EINVAL (return value of vfio_device_create_dmabuf_fd())
>> as the equivalent of INVALID_IOV. That is, the relevant code now becomes:
>>          res->dmabuf_fd = virtio_gpu_create_udmabuf(res, &local_err);
>>          if (res->dmabuf_fd == VIRTIO_GPU_DMABUF_CREATE_ERR_INVALID_IOV) {
>>              error_free_or_abort(&local_err);
>>              res->dmabuf_fd = vfio_device_create_dmabuf_fd(res->iov,
>>                                                            res->iov_cnt,
>>                                                            &local_err);
>>              if (res->dmabuf_fd == -EINVAL) {
>>                  error_free_or_abort(&local_err);
>>                  qemu_log_mask(LOG_GUEST_ERROR,
>>                                "Cannot create dmabuf: incompatible memory\n");
>>                  return false;
>>              }
>> I think this is reasonable IMO, but is this proposal acceptable to you?
>
> The return value drives control flow.
>
> AFAICT, in virtio_gpu_init_dmabuf(), the only distinction that seems
> to matter for control flow is : "should I try the next backend ?" and
> that's EINVAL, which means "these addresses are incompatible with this
> particular backend."
>
> The guest-vs-host attribution for logging/debugging is in the errp
> messages :
>
>  - "IOV memory address incompatible with udmabuf" : guest gave addresses udmabuf can't handle
>  - "No VFIO device found to create dmabuf from"   : guest addresses don't map to a VFIO device
>  - "UDMABUF_CREATE_LIST: ioctl failed" + errno    :  host/kernel issue
>
> The Error* object is designed to encode a human-readable attribution
> in a return value. It carries the diagnostic detail and the return
> code carries the machine action.

Yes, this is the overwhelmingly common way to do this[*].

When all errors are handled the same, a single return value signifying
failure suffices.  When callers need to handle different values
differently, which is rare, we need several error values.  Negative
errno is often convenient then.  Sometimes, it's a bad fit, so we come
up with some other set of error codes.

Regardless, error values need to be clearly documented.

To decide what error values to use, you should start with how errors are
handled.  Design for that.  In a library, you may not know.  But when
you do, resist the temptation to encode all possible errors perfectly.
KISS.  You can always make it more complicated when new callers require
it.

Does this help?  If not, try asking pointed questions.


[...]


[*] There's also the ErrorClass carried by Error objects.  It was meant
to carry the machine action, but it turned out to be a costly failure.
Use of error classes other than ERROR_CLASS_GENERIC_ERROR is strongly
discouraged.