On 2026/03/06 16:47, Kasireddy, Vivek wrote:
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.
In that case we know udmabuf is doing fine but it still reports an
error, which is misleading.
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?
Setting -1 is not strictly needed but improves consistency. You are
right about virtio_gpu_destroy_dmabuf().
Regards,
Akihiko Odaki
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;
