Hi Akihiko, > Subject: Re: [PATCH v2 09/10] virtio-gpu-dmabuf: Introduce > qemu_iovec_same_memory_regions() > > On 2025/11/12 13:24, Kasireddy, Vivek wrote: > > Hi Akihiko, > > > >> Subject: Re: [PATCH v2 09/10] virtio-gpu-dmabuf: Introduce > >> qemu_iovec_same_memory_regions() > >> > >> On 2025/11/09 14:33, Vivek Kasireddy wrote: > >>> Add a helper to check whether the addresses in an iovec array > >>> belong to the same memory region or not. This is useful to verify > >>> before trying to create a dmabuf from an iovec array. > >>> > >>> 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 | 29 > +++++++++++++++++++++++++++++ > >>> 1 file changed, 29 insertions(+) > >>> > >>> diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu- > >> dmabuf.c > >>> index c34d4c85bc..80143034d4 100644 > >>> --- a/hw/display/virtio-gpu-dmabuf.c > >>> +++ b/hw/display/virtio-gpu-dmabuf.c > >>> @@ -27,6 +27,31 @@ > >>> #include "standard-headers/linux/udmabuf.h" > >>> #include "standard-headers/drm/drm_fourcc.h" > >>> > >>> +static bool qemu_iovec_same_memory_regions(const struct iovec *iov, > >> int iov_cnt) > >>> +{ > >>> + RAMBlock *rb, *curr_rb; > >>> + ram_addr_t offset; > >>> + int i; > >>> + > >>> + rb = qemu_ram_block_from_host(iov[0].iov_base, false, &offset); > >>> + if (!rb) { > >>> + qemu_log_mask(LOG_GUEST_ERROR, > >>> + "%s: Could not find ramblock/memory region\n", > __func__); > >>> + return false; > >>> + } > >>> + > >>> + for (i = 1; i < iov_cnt; i++) { > >>> + curr_rb = qemu_ram_block_from_host(iov[i].iov_base, false, > >> &offset); > >>> + if (curr_rb != rb) { > >>> + qemu_log_mask(LOG_GUEST_ERROR, > >>> + "%s: memory regions not same for iov > >>> entries\n", > >>> + __func__); > >>> + return false; > >>> + } > >>> + } > >>> + return true; > >>> +} > >>> + > >>> static void virtio_gpu_create_udmabuf(struct > virtio_gpu_simple_resource > >> *res) > >>> { > >>> struct udmabuf_create_list *list; > >>> @@ -137,6 +162,10 @@ void virtio_gpu_init_dmabuf(struct > >> virtio_gpu_simple_resource *res) > >>> res->iov[0].iov_len < 4096) { > >>> pdata = res->iov[0].iov_base; > >>> } else { > >>> + if (!qemu_iovec_same_memory_regions(res->iov, res->iov_cnt)) { > >>> + return; > >>> + } > >>> + > >> > >> This check is unnecessary. Perhaps rejecting iov with different memory > >> regions may be fine if that simplifies the code, but this actually adds > >> some code. > > I think we can keep this sanity check but I don't really mind dropping this > > patch given that buffers with mixed memory regions are not encountered > > in practical situations. Or, I guess I could move the if (curr_rb != rb) > > check > > to virtio_gpu_create_udmabuf() and vfio_device_create_dmabuf_fd() > > like you suggested previously. > > I won't call it a "sanity check"; it is "unlikely" to have different > memory regions, but it is still not "wrong" and is sane. I'd say "very unlikely". The only scenario I can imagine where this might happen is if a buffer is partially migrated (from one memory region to another). And, if we do come across such a buffer, it has to be rejected because there is no way we can create a dmabuf (via vfio or udmabuf) for such a buffer/resource.
> > The VFIO code path still needs to check if the memory regions belong to > one VFIO device. Trying to create one DMA-BUF using multiple VFIO > devices is wrong. Ok, I'll add this check in vfio_device_create_dmabuf_fd() to avoid iterating over all the entries separately. Thanks, Vivek > > Regards, > Akihiko Odaki
