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.
Ok, I'll make the change to have vfio_device_mmap_dmabuf() accept
a length parameter.
Thanks,
Vivek
>
> 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) {
> >>>
> >
