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.
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'))
