Hi Akihiko,
> Subject: Re: [PATCH v2 10/10] virtio-gpu-dmabuf: Create dmabuf for blobs
> associated with VFIO devices
>
> On 2025/11/17 13:19, Kasireddy, Vivek wrote:
> > Hi Akihiko,
> >
> >> Subject: Re: [PATCH v2 10/10] virtio-gpu-dmabuf: Create dmabuf for blobs
> >> associated with VFIO devices
> >>
> >> On 2025/11/13 12:17, Kasireddy, Vivek wrote:
> >>> Hi Akihiko,
> >>>
> >>>> Subject: Re: [PATCH v2 10/10] virtio-gpu-dmabuf: Create dmabuf for
> >> blobs
> >>>> associated with VFIO devices
> >>>>
> >>>> On 2025/11/12 13:26, Kasireddy, Vivek wrote:
> >>>>> Hi Akihiko,
> >>>>>
> >>>>>> Subject: Re: [PATCH v2 10/10] virtio-gpu-dmabuf: Create dmabuf for
> >>>> blobs
> >>>>>> associated with VFIO devices
> >>>>>>
> >>>>>> On 2025/11/09 14:33, Vivek Kasireddy wrote:
> >>>>>>> In addition to memfd, a blob resource can also have its backing
> >>>>>>> storage in a VFIO device region. Therefore, we first need to figure
> >>>>>>> out if the blob is backed by a VFIO device region or a memfd before
> >>>>>>> we can call the right API to get a dmabuf fd created.
> >>>>>>>
> >>>>>>> So, once we have the ramblock and the associated mr, we rely on
> >>>>>>> memory_region_is_ram_device() to tell us where the backing storage
> >>>>>>> is located. If the blob resource is VFIO backed, we try to find the
> >>>>>>> right VFIO device that contains the blob and then invoke the API
> >>>>>>> vfio_device_create_dmabuf().
> >>>>>>>
> >>>>>>> Note that in virtio_gpu_remap_udmabuf(), we first try to test if
> >>>>>>> the VFIO dmabuf exporter supports mmap or not. If it doesn't, we
> >>>>>>> use the VFIO device fd directly to create the CPU mapping.
> >>>>>>
> >>>>>> I have just remembered that mremap() will work for either of udmabuf
> >>>> and
> >>>>>> VFIO. That will avoid having two different methods and make
> >>>>>> vfio_get_region_index_from_mr() and vfio_device_get_region_info()
> >>>>>> unnecessary.
> >>>>> IIUC, the name virtio_gpu_remap_dmabuf() is misleading because we
> >> are
> >>>> not
> >>>>> actually doing remap but are simply calling mmap(). In other words, we
> >>>> are not
> >>>>> expanding or shrinking existing mapping but are creating a new
> >> mapping.
> >>>>> And, for dmabufs associated with VFIO devices, without having to call
> >>>>> vfio_get_region_index_from_mr() and vfio_device_get_region_info(), I
> >>>> don't see
> >>>>> any other way to determine the region offset.
> >>>>>
> >>>>> So, I guess I'll create a new patch to do s/remapped/map.
> >>>>
> >>>> I mean calling mremap() with 0 as the old_size parameter. The man page
> >>>> says:
> >>>> > If the value of old_size is zero, and old_address refers to a
> >>>> > shareable mapping (see the description of MAP_SHARED in
> mmap(2)),
> >>>> then
> >>>> > mremap() will create a new mapping of the same pages.
> >>> It might be possible to use mremap() here but using mmap() seems very
> >>> straightforward given that we are actually not shrinking or expanding
> >>> an existing mapping but are instead creating a new mapping. Also, I am
> >>> wondering what benefit would mremap() bring as opposed to just using
> >>> mmap()?
> >>
> >> As I noted earlier, mremap() removes the need of having two different
> >> paths for udmabuf and VFIO, and make vfio_get_region_index_from_mr()
> >> and
> >> vfio_device_get_region_info() unnecessary, reducing code complexity.
> > Sorry, I should have researched thoroughly before but after looking at the
> code
> > again, I don't see how mremap() removes the need for having two different
> > paths for udmabuf and VFIO and make vfio_get_region_index_from_mr()
> > and vfio_device_get_region_info() unnecessary. Could you please elaborate
> > how it can be done?
>
> Not tested, but something like the following:
>
> head = qemu_ram_mmap(-1, res->blob_size, qemu_real_host_page_size(),
> QEMU_MAP_READONLY | QEMU_MAP_SHARED, 0);
> if (head == MAP_FAILED) {
> return NULL;
> }
>
> cursor = head;
>
> for (i = 0; i < res->iov_cnt; i++) {
> if (mremap(res->iov[i].iov_base, 0, res->iov[i].iov_len,
> MREMAP_FIXED, cursor) == MAP_FAILED) {
This is very elegant and I can now see how it is expected to work. However,
I went ahead and tested it and it does not seem to work for VFIO backed
buffers. It works for buffers based out of System RAM though. Here is the
actual code I tested with that I am unconditionally calling for both VFIO
and udmabuf cases:
static void *vfio_dmabuf_mmap2(struct virtio_gpu_simple_resource *res,
VFIODevice *vdev)
{
void *head, *cursor;
int i;
head = qemu_ram_mmap(-1, res->blob_size, qemu_real_host_page_size(),
QEMU_MAP_READONLY |
QEMU_MAP_SHARED, 0);
if (head == MAP_FAILED) {
return head;
}
cursor = head;
for (i = 0; i < res->iov_cnt; i++) {
if (mremap(res->iov[i].iov_base, 0, res->iov[i].iov_len,
MREMAP_FIXED | MREMAP_MAYMOVE, cursor) == MAP_FAILED) {
goto err;
}
cursor += res->iov[i].iov_len;
}
return head;
err:
qemu_ram_munmap(-1, head, res->blob_size);
return MAP_FAILED;
}
It (mremap) initially errored with -EINVAL in all cases but adding
MREMAP_MAYMOVE
fixed it for buffers based out of RAM but for VFIO backed buffers, it seems to
be
throwing -EFAULT/Bad Address error. I did not yet check why or where the kernel
driver is returning this error from.
Thanks,
Vivek
> qemu_ram_munmap(-1, head, res->blob_size);
> return NULL;
> }
>
> cursor += res->iov[i].iov_len;
> }
>
> return head;
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks,
> > Vivek
> >
> >>
> >> mremap() is also sufficiently straightforward. The man page explicitly
> >> states it is designed to create a new mapping. Using it for the purpose
> >> (not shrinking or expanding an existing mapping) is not an abuse of the
> >> API.
> >>
> >> Regards,
> >> Akihiko Odaki
