On 2026/10/12 15:14, Kasireddy, Vivek wrote:
Hi Akihiko,
Subject: Re: [PATCH v13 9/9] virtio-gpu-dmabuf: Create dmabuf for blobs
associated with VFIO devices
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]>
Reviewed-by: Akihiko Odaki <[email protected]>
Signed-off-by: Vivek Kasireddy <[email protected]>
---
hw/display/virtio-gpu-dmabuf.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
dmabuf.c
index c9de8e9ba3..f59a5e76ff 100644
--- a/hw/display/virtio-gpu-dmabuf.c
+++ b/hw/display/virtio-gpu-dmabuf.c
@@ -153,9 +153,25 @@ bool virtio_gpu_init_dmabuf(struct
virtio_gpu_simple_resource *res)
if (res->dmabuf_fd ==
VFIO_DMABUF_CREATE_ERR_INVALID_IOV) {
error_free_or_abort(&local_err);
- qemu_log_mask(LOG_GUEST_ERROR,
- "Cannot create dmabuf: incompatible memory\n");
- return false;
+ 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) {
+ error_free_or_abort(&local_err);
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "Cannot create dmabuf: incompatible memory\n");
+ return false;
+ }
+
+ if (res->dmabuf_fd >= 0) {
+ pdata = vfio_device_mmap_dmabuf(res->iov, res->iov_cnt,
+ &local_err);
This maps the complete IOV size, while virtio_gpu_fini_dmabuf()
unmaps
only blob_size. Create and attach accept backing larger than the blob,
so excess pages remain mapped after detach, unref or reset.
Good catch. I'll add a check in virtio_gpu_fini_dmabuf() to determine if
IOV size > blob_size, then do munmap(res->remapped, IOV size);
I think this should be safe even if the mapping (created with mmap)
size is blob_size. Or, is there a better solution you can think of such as
tracking the exact mapping size in res object?
Unmapping the IOV size is not safe if only blob_size was mapped:
munmap() can also remove unrelated mappings in the excess range.
Ok, got it.
A cleaner fix is to update vfio_device_mmap_dmabuf() to take the size to
map (or cap it to blob_size).
Currently, vfio_device_mmap_dmabuf() creates an IOV sized mapping, so it
is going to need extra logic to cap it to blob_size, which needs to be
explicitly
passed in. So, are you ok with this given that you previously did not like the
idea of passing in blob_size as a parameter?
Yes, I am fine with a mapping-length parameter for this purpose.
I should have made the distinction clearer.
Previously, total_size was required to equal the IOV size, adding
blob-specific validation to the VFIO helper. That validation belongs
in virtio-gpu.
Here, the parameter specifies how much of the IOV to map, which can
be less than its total size. That is a generic mapping operation;
the helper does not need to know about blobs. We can name the
parameter length and have virtio-gpu pass res->blob_size.
The extra logic to stop at that length and shorten the final mapped
segment as necessary is appropriate. This lets mapping and unmapping
use the same length without additional tracking.
Regards,
Akihiko Odaki
Thanks,
Vivek
This keeps it symmetric with the udmabuf
path and allows virtio_gpu_fini_dmabuf() to continue unmapping
blob_size
without needing extra tracking variables.
Regards,
Akihiko Odaki
Thanks,
Vivek
Regards,
Akihiko Odaki
+ if (!pdata) {
+ virtio_gpu_fini_dmabuf(res);
+ }
+ } else {
+ res->dmabuf_fd = -1;
+ }
} else if (res->dmabuf_fd >= 0) {
pdata = virtio_gpu_remap_dmabuf(res, &local_err);
if (!pdata) {
