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 <[email protected]>
Cc: Cédric Le Goater <[email protected]>
Signed-off-by: Vivek Kasireddy <[email protected]>
---
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.
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.
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.
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/[email protected]/
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.
Now the flow,
udmabuf
├─ -EINVAL ──► VFIO
│ ├─ >= 0 ──► vfio_device_mmap_dmabuf ──► success
│ ├─ -EINVAL ──► guest error: incompatible memory
│ └─ other -errno ──► error_report_err ──► host error
│
├─ >= 0 ──► virtio_gpu_remap_dmabuf ──► success
│
├─ -ENODEV ──► error_report_err ──► fail (VFIO NOT tried)
├─ -ENOMEM ──► error_report_err ──► fail (VFIO NOT tried)
└─ other -errno ──► error_report_err ──► fail (VFIO NOT tried)
The fallback trigger should be widen to -EINVAL || -ENODEV.
How's that ?
Thanks,
C.
