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? Thanks, Vivek > > Regards, > Akihiko Odaki > > > > > Thanks, > > Vivek > >> > >> > >>> typedef struct VFIODeviceOps VFIODeviceOps; > >>> typedef struct VFIODeviceIOOps VFIODeviceIOOps; > >>> typedef struct VFIOMigration VFIOMigration; > >>> @@ -323,6 +330,21 @@ bool > vfio_device_has_region_cap(VFIODevice > >> *vbasedev, int region, uint16_t cap_t > >>> > >>> int vfio_device_get_irq_info(VFIODevice *vbasedev, int index, > >>> struct vfio_irq_info *info); > >>> + > >>> +/** > >>> + * Create a dmabuf fd by first translating the addresses in the > >>> + * iovec array into VFIO region offsets and then invoking the > >>> + * VFIO_DEVICE_FEATURE_DMA_BUF feature. > >>> + * > >>> + * @iov: array of iovec entries associated with a buffer > >>> + * @iov_cnt: number of entries in the iovec array > >>> + * @errp: pointer to Error*, to store an error if it happens. > >>> + * > >>> + * Returns the created dmabuf fd or either > >> VFIO_DMABUF_CREATE_ERR_UNSPEC > >>> + * or VFIO_DMABUF_CREATE_ERR_INVALID_IOV on error. > >>> + */ > >>> +int vfio_device_create_dmabuf_fd(struct iovec *iov, unsigned int > >> iov_cnt, > >>> + Error **errp); > >>> #endif > >>> > >>> /* Returns 0 on success, or a negative errno. */ > >>> diff --git a/hw/vfio/device.c b/hw/vfio/device.c > >>> index 4f11959633..d95503a5f1 100644 > >>> --- a/hw/vfio/device.c > >>> +++ b/hw/vfio/device.c > >>> @@ -21,7 +21,9 @@ > >>> #include "qemu/osdep.h" > >>> #include <sys/ioctl.h> > >>> > >>> +#include "system/ramblock.h" > >>> #include "hw/vfio/vfio-device.h" > >>> +#include "hw/vfio/vfio-region.h" > >>> #include "hw/vfio/pci.h" > >>> #include "hw/core/iommu.h" > >>> #include "trace.h" > >>> @@ -667,3 +669,110 @@ static VFIODeviceIOOps > >> vfio_device_io_ops_ioctl = { > >>> .region_read = vfio_device_io_region_read, > >>> .region_write = vfio_device_io_region_write, > >>> }; > >>> + > >>> +/* > >>> + * This helper looks up the VFIODevice corresponding to the given > iov. > >> It > >>> + * can be useful to determinine if a buffer (represented by the iov) > >> belongs > >>> + * to a VFIO device or not. This is mainly invoked when external > >> components > >>> + * such as virtio-gpu request creation of dmabuf fds for buffers > that may > >>> + * belong to a VFIO device. > >>> + */ > >>> +static bool vfio_device_lookup(struct iovec *iov, VFIODevice > >> **vbasedevp, > >>> + RAMBlock **first_rbp, Error **errp) > >>> +{ > >>> + VFIODevice *vbasedev; > >>> + RAMBlock *first_rb; > >>> + ram_addr_t offset; > >>> + > >>> + first_rb = qemu_ram_block_from_host(iov[0].iov_base, false, > >> &offset); > >>> + if (!first_rb) { > >>> + error_setg(errp, "Could not find first ramblock\n"); > >> > >> no \n > >> > >> > >>> + return false; > >>> + } > >>> + > >>> + *first_rbp = first_rb; > >>> + QLIST_FOREACH(vbasedev, &vfio_device_list, next) { > >> > >> wrong list : global_next. > >> > >> > >>> + if (vbasedev->dev == first_rb->mr->dev) { > >>> + *vbasedevp = vbasedev; > >>> + return true; > >>> + } > >>> + } > >>> + error_setg(errp, "No VFIO device found to create dmabuf > from\n"); > >> > >> no \n > >> > >>> + return false; > >>> +} > >>> + > >>> +/* > >>> + * This helper looks up the VFIORegion corresponding to the given > >> address. > >>> + * It also verifies that the RAMBlock associated with the address is > the > >>> + * same as the first_rb passed in. This is to ensure that all > addresses > >>> + * in the iov belong to the same region. > >>> + */ > >>> +static bool vfio_region_lookup(void *addr, VFIORegion **regionp, > >>> + RAMBlock *first_rb, ram_addr_t *offsetp, > >>> + Error **errp) > >>> +{ > >>> + VFIORegion *region; > >>> + RAMBlock *rb; > >>> + > >>> + rb = qemu_ram_block_from_host(addr, false, offsetp); > >>> + if (!rb || rb != first_rb) { > >>> + error_setg(errp, "Dmabuf segments must belong to the same > >> region\n"); > >> > >> remove \n > >> > >>> + return false; > >>> + } > >>> + > >>> + region = vfio_get_region_from_mr(rb->mr); > >>> + if (region) { > >>> + *regionp = region; > >>> + return true; > >>> + } > >>> + error_setg(errp, "Could not find valid region for dmabuf > segment\n"); > >> > >> here too. > >> > >>> + return false; > >>> +} > >>> + > >>> +int vfio_device_create_dmabuf_fd(struct iovec *iov, unsigned int > >> iov_cnt, > >>> + Error **errp) > >>> +{ > >>> + g_autofree struct vfio_device_feature *feature = NULL; > >>> + struct vfio_device_feature_dma_buf *dma_buf; > >>> + RAMBlock *first_rb = NULL; > >>> + VFIODevice *vbasedev; > >>> + VFIORegion *region; > >>> + ram_addr_t offset; > >>> + size_t argsz; > >>> + int i, ret; > >>> + > >>> + if (!vfio_device_lookup(iov, &vbasedev, &first_rb, errp)) { > >>> + return VFIO_DMABUF_CREATE_ERR_INVALID_IOV; > >> > >> return -EINVAL; > >> > >>> + } > >>> + > >>> + argsz = sizeof(*feature) + sizeof (*dma_buf) + > >>> + sizeof(struct vfio_region_dma_range) * iov_cnt; > >>> + feature = g_malloc0(argsz); > >>> + *feature = (struct vfio_device_feature) { > >>> + .argsz = argsz, > >>> + .flags = VFIO_DEVICE_FEATURE_GET | > >> VFIO_DEVICE_FEATURE_DMA_BUF, > >>> + }; > >>> + dma_buf = (struct vfio_device_feature_dma_buf *)feature- > >data; > >>> + > >>> + for (i = 0; i < iov_cnt; i++) { > >>> + if (!vfio_region_lookup(iov[i].iov_base, ®ion, > >>> + first_rb, &offset, errp)) { > >>> + return VFIO_DMABUF_CREATE_ERR_INVALID_IOV; > >> > >> return -EINVAL; > >> > >>> + } > >>> + > >>> + dma_buf->region_index = region->nr; > >>> + dma_buf->dma_ranges[i].offset = offset; > >>> + dma_buf->dma_ranges[i].length = iov[i].iov_len; > >>> + } > >>> + > >>> + dma_buf->nr_ranges = iov_cnt; > >>> + dma_buf->open_flags = O_RDONLY | O_CLOEXEC; > >>> + > >>> + ret = vfio_device_get_feature(vbasedev, feature); > >>> + if (ret < 0) { > >>> + error_setg_errno(errp, -ret, > >>> + "Could not create dmabuf fd via VFIO device"); > >>> + return VFIO_DMABUF_CREATE_ERR_UNSPEC; > >> > >> return ret; > >> > >>> + } > >>> + return ret; > >>> +} > >>> diff --git a/hw/vfio/dmabuf-stubs.c b/hw/vfio/dmabuf-stubs.c > >>> new file mode 100644 > >>> index 0000000000..fb5318e929 > >>> --- /dev/null > >>> +++ b/hw/vfio/dmabuf-stubs.c > >> > >> Fails to build on MinGW/Windows. > >> > >> hw/vfio/dmabuf-stubs.c:12:5: error: no previous prototype for > >> 'vfio_device_create_dmabuf_fd' > >> hw/vfio/dmabuf-stubs.c:19:7: error: no previous prototype for > >> 'vfio_device_mmap_dmabuf' > >> > >> > >>> @@ -0,0 +1,17 @@ > >>> +/* > >>> + * Copyright (c) 2026 Intel and/or its affiliates. > >>> + * > >>> + * SPDX-License-Identifier: GPL-2.0-or-later > >>> + */ > >>> + > >>> +#include "qemu/osdep.h" > >>> +#include "qemu/error-report.h" > >>> +#include "qemu/iov.h" > >>> +#include "hw/vfio/vfio-device.h" > >>> + > >>> +int vfio_device_create_dmabuf_fd(struct iovec *iov, unsigned int > >> iov_cnt, > >>> + Error **errp) > >>> +{ > >>> + error_setg(errp, "VFIO dmabuf support is not enabled"); > >>> + return VFIO_DMABUF_CREATE_ERR_UNSPEC; > >> > >> return -ENOTSUP; > >> > >> > >>> +} > >>> diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build > >>> index c7c31b78f6..aa6f4bc1b4 100644 > >>> --- a/hw/vfio/meson.build > >>> +++ b/hw/vfio/meson.build > >>> @@ -38,3 +38,4 @@ stub_ss.add(files('iommufd-stubs.c')) > >>> system_ss.add(when: 'CONFIG_VFIO_PCI', if_true: files( > >>> 'display.c', > >>> )) > >>> +system_ss.add(when: 'CONFIG_VFIO', if_false: files('dmabuf- > stubs.c')) > >
