Hi Akihiko,
> Subject: Re: [PATCH v8 10/10] virtio-gpu-dmabuf: Create dmabuf for
> blobs associated with VFIO devices
> >> On 2026/02/28 7:13, Vivek Kasireddy wrote:
> >>> In addition to memfd, a blob resource can also have its backing
> >>> storage in a VFIO device region. Since, there is no effective way
> >>> to determine where the backing storage is located, we first try to
> >>> create a dmabuf assuming it is in memfd. If that fails, we try to
> >>> create a dmabuf assuming it is in VFIO device region.
> >>>
> >>> So, we first call virtio_gpu_create_udmabuf() to check if the blob's
> >>> backing storage is located in a memfd or not. If it is not, we invoke
> >>> the vfio_device_create_dmabuf_fd() API which identifies the right
> >>> VFIO device and eventually creates a dmabuf fd.
> >>>
> >>> Note that in virtio_gpu_remap_dmabuf(), we first try to test if
> >>> the VFIO dmabuf exporter supports mmap or not. If it doesn't, we
> >>> fall back to the vfio_device_mmap_dmabuf() API to get a mapping
> >>> created for the dmabuf.
> >>>
> >>> Cc: Marc-André Lureau <[email protected]>
> >>> Cc: Alex Bennée <[email protected]>
> >>> Cc: Akihiko Odaki <[email protected]>
> >>> Cc: Dmitry Osipenko <[email protected]>
> >>> Cc: Alex Williamson <[email protected]>
> >>> Cc: Cédric Le Goater <[email protected]>
> >>> Signed-off-by: Vivek Kasireddy <[email protected]>
> >>> ---
> >>> hw/display/virtio-gpu-dmabuf.c | 35
> +++++++++++++++++++++++++++-
> >> ------
> >>> 1 file changed, 28 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
> >> dmabuf.c
> >>> index b2badfc1bd..533d3d601c 100644
> >>> --- a/hw/display/virtio-gpu-dmabuf.c
> >>> +++ b/hw/display/virtio-gpu-dmabuf.c
> >>> @@ -74,13 +74,21 @@ static int virtio_gpu_create_udmabuf(struct
> >> virtio_gpu_simple_resource *res,
> >>> static bool virtio_gpu_remap_dmabuf(struct
> >> virtio_gpu_simple_resource *res,
> >>> Error **errp)
> >>> {
> >>> + Error *mmap_err = NULL;
> >>> void *map;
> >>>
> >>> map = mmap(NULL, res->blob_size, PROT_READ, MAP_SHARED,
> res-
> >>> dmabuf_fd, 0);
> >>> if (map == MAP_FAILED) {
> >>> - error_setg_errno(errp, errno, "dmabuf mmap failed");
> >>> + error_setg_errno(&mmap_err, errno, "dmabuf mmap failed:
> ");
> >>> res->remapped = NULL;
> >>> - return false;
> >>> +
> >>> + if (!vfio_device_mmap_dmabuf(res->iov, res->iov_cnt, &map,
> >>> + res->blob_size, errp)) {
> >>
> >> Let's call vfio_device_mmap_dmabuf() only when
> >> vfio_device_create_dmabuf_fd() is called. Hopefully it will simplify
> the
> >> logic structure.
> > The logic looks OK to me but are you suggesting that it would be
> simpler to
> > remove the call to vfio_device_mmap_dmabuf() from
> virtio_gpu_remap_dmabuf()
> > and call it where vfio_device_create_dmabuf_fd() gets called?
> >
> > Or, if we keep the call to vfio_device_mmap_dmabuf() in
> virtio_gpu_remap_dmabuf(),
> > then to implement your suggestion, I think we need a flag to identify
> dmabufs
> > that are created via VFIO so that we would call
> vfio_device_mmap_dmabuf()
> > only for these ones.
> >
> > Also, for VFIO based dmabufs, should we directly call
> vfio_device_mmap_dmabuf()
> > or is it ok if we first call mmap() and check if it works or not?
>
> The current logic structure looks like as follows:
> Initialize DMA-BUF
> Create DMA-BUF
> Create DMA-BUF, assuming it is memfd
> If it turned out it is not memfd,
> Create DMA-BUF, assuming it is VFIO
> Map memory
> Map memory, assuming it is memfd
> If it turned out it is not memfd,
> Map memory, assuming it is VFIO
>
> The suggested logic structure looks like as follows:
>
> Initialize DMA-BUF
> Initialize DMA-BUF, assuming it is memfd
> Create DMA-BUF
> Map memory
> If it turned out it is not memfd
> Initialize DMA-BUF, assuming it is VFIO
> Create DMA-BUF
> Map memory
>
> See we will have less "if".
I'll try out your suggestion but I think the current logic structure is very
straight forward and readable with not so deep indentation levels and
clear separation of dmabuf creation and mmap. Your suggested logic
structure while looks elegant, IMO, might not be very intuitive when
it comes to the code.
Thanks,
Vivek
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks,
> > Vivek
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>> + error_report_err(mmap_err);
> >>> + return false;
> >>> + }
> >>> + error_free(mmap_err);
> >>> + mmap_err = NULL;
> >>> }
> >>> res->remapped = map;
> >>> return true;
> >>> @@ -137,7 +145,7 @@ bool virtio_gpu_have_udmabuf(void)
> >>>
> >>> void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource
> *res)
> >>> {
> >>> - Error *local_err = NULL;
> >>> + Error *local_err = NULL, *vfio_err = NULL;
> >>> void *pdata = NULL;
> >>> int ret;
> >>>
> >>> @@ -151,15 +159,28 @@ void virtio_gpu_init_dmabuf(struct
> >> virtio_gpu_simple_resource *res)
> >>> error_prepend(&local_err, "Cannot create dmabuf: ");
> >>>
> >>> if (ret == VFIO_DMABUF_CREATE_GUEST_ERROR) {
> >>> - qemu_log_mask(LOG_GUEST_ERROR,
> >>> - "Cannot create dmabuf: incompatible
> memory\n");
> >>> - error_free(local_err);
> >>> - return;
> >>> + ret = vfio_device_create_dmabuf_fd(res->iov, res-
> >iov_cnt,
> >>> + &vfio_err);
> >>> + if (ret > 0) {
> >>> + error_free(local_err);
> >>> + local_err = NULL;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + if (ret == VFIO_DMABUF_CREATE_GUEST_ERROR) {
> >>> + qemu_log_mask(LOG_GUEST_ERROR,
> >>> + "Cannot create dmabuf: incompatible
> memory\n");
> >>> + error_free(vfio_err);
> >>> + error_free(local_err);
> >>> + return;
> >>> + }
> >>> + error_report_err(vfio_err);
> >>> }
> >>> error_report_err(local_err);
> >>> return;
> >>> }
> >>>
> >>> +out:
> >>> res->dmabuf_fd = ret;
> >>> if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
> >>> error_report_err(local_err);
> >
