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?
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) {
> >
