Hi Akihiko,
> Subject: Re: [PATCH v9 10/10] virtio-gpu-dmabuf: Create dmabuf for
> blobs associated with VFIO devices
>
> On 2026/03/05 15:54, 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, for mmapping the dmabuf, we directly call mmap() if the
> > dmabuf fd was created via virtio_gpu_create_udmabuf() since we
> know
> > that the udmabuf driver supports mmap(). However, if the dmabuf
> was
> > created via vfio_device_create_dmabuf_fd(), we use the
> > vfio_device_mmap_dmabuf() API to get a mapping 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 | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
> dmabuf.c
> > index dd64fbbc2d..7f4c399416 100644
> > --- a/hw/display/virtio-gpu-dmabuf.c
> > +++ b/hw/display/virtio-gpu-dmabuf.c
> > @@ -136,7 +136,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;
> >
> > @@ -155,10 +155,24 @@ void virtio_gpu_init_dmabuf(struct
> virtio_gpu_simple_resource *res)
> > }
> >
> > if (ret == VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
> > - 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) {
>
> Zero represents a success by convention.
>
> > + if (vfio_device_mmap_dmabuf(res->iov, res->iov_cnt,
> &pdata,
> > + res->blob_size, &vfio_err)) {
> > + error_free(local_err);
> > + goto out;
> > + }
> > + }
> > +
> > + if (ret == VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
> > + 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);
>
> This reports invalid IOV even when it is actually a valid pointer to
> VFIO and the VFIO is failing. Such an error report does not help and we
> should instead report only the VFIO error.
I thought reporting more errors to the user made sense here in this case.
>
> And there is another bug with the previous patch where res-
> >dmabuf_fd is
> invalid when virtio_gpu_remap_dmabuf() uses it.
Yeah, I see it. Will fix it in the next version. FYI, this was the only version
I did not test as my test environment was not available.
>
> Below is my attempt to fix these issues:
It is a different style and structure (compared to mine) but I guess I'll adopt
it in the next version to get things going. I do have one comment/question
below.
>
> if (res->iov_cnt == 1 &&
> res->iov[0].iov_len < 4096) {
> res->dmabuf_fd = -1;
> pdata = res->iov[0].iov_base;
> } else {
> res->dmabuf_fd = virtio_gpu_create_udmabuf(res, &local_err);
> if (res->dmabuf_fd == VFIO_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 ==
> VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
> res->dmabuf_fd = -1;
> error_free_or_abort(local_err);
> qemu_log_mask(LOG_GUEST_ERROR,
> "Cannot create dmabuf: incompatible
> memory\n");
> return;
> }
>
> if (res->dmabuf_fd < 0 ||
> !vfio_device_mmap_dmabuf(res->iov, res->iov_cnt, &pdata,
> res->blob_size, &local_err)) {
> res->dmabuf_fd = -1;
I am wondering here and in the below else if what is the benefit of setting
res->dmabuf_fd to -1 if it is already negative? Also, if res->dmabuf_fd is
valid (non-negative) and mmap fails, shouldn't we call
virtio_gpu_destroy_dmabuf()
to close the dmabuf fd?
Thanks,
Vivek
> }
> } else if (res->dmabuf_fd < 0 ||
> !virtio_gpu_remap_dmabuf(res, &pdata, &local_err)) {
> res->dmabuf_fd = -1;
> }
>
> if (res->dmabuf_fd < 0) {
> error_report_err(local_err);
> return;
> }
>
> res->remapped = pdata;
> }
>
> Regards,
> Akihiko Odaki
>
> > return;
