Hi Akihiko, > Subject: Re: [PATCH v2 02/10] virtio-gpu: Find hva for Guest's DMA addr > associated with a ram device > > On 2025/11/12 13:30, Kasireddy, Vivek wrote: > > Hi Akihiko, > > > >> Subject: Re: [PATCH v2 02/10] virtio-gpu: Find hva for Guest's DMA addr > >> associated with a ram device > >> > >> On 2025/11/09 14:33, Vivek Kasireddy wrote: > >>> If the Guest provides a DMA address that is associated with a ram > >>> device (such as a PCI device region and not its system memory), then > >>> we can obtain the hva (host virtual address) by invoking > >>> address_space_translate() followed by memory_region_get_ram_ptr(). > >>> > >>> This is because the ram device's address space is not accessible to > >>> virtio-gpu directly and hence dma_memory_map() cannot be used. > >>> Therefore, we first need to identify the memory region associated with > >>> the DMA address and figure out if it belongs to a ram device or not > >>> and decide how to obtain the host address accordingly. > >>> > >>> Note that we take a reference on the memory region if it belongs to a > >>> ram device but we would still call dma_memory_unmap() later (to unref > >>> mr) regardless of how we obtained the hva. > >>> > >>> 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.c | 24 +++++++++++++++++++++--- > >>> 1 file changed, 21 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index > >>> 199b18c746..d352b5afd6 100644 > >>> --- a/hw/display/virtio-gpu.c > >>> +++ b/hw/display/virtio-gpu.c > >>> @@ -798,6 +798,26 @@ static void > virtio_gpu_set_scanout_blob(VirtIOGPU > >> *g, > >>> &fb, res, &ss.r, &cmd->error); > >>> } > >>> > >>> +static void *virtio_gpu_dma_memory_map(VirtIOGPU *g, > >>> + struct virtio_gpu_ctrl_command > >>> *cmd, > >>> + uint64_t a, hwaddr *len) { > >>> + MemoryRegion *mr = NULL; > >>> + hwaddr xlat; > >>> + > >>> + mr = address_space_translate(VIRTIO_DEVICE(g)->dma_as, a, &xlat, > len, > >>> + DMA_DIRECTION_TO_DEVICE, > >>> + MEMTXATTRS_UNSPECIFIED); > >>> + if (memory_region_is_ram_device(mr)) { > >>> + memory_region_ref(mr); > >>> + return memory_region_get_ram_ptr(mr) + xlat; > >>> + } > >>> + > >>> + return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, len, > >>> + DMA_DIRECTION_TO_DEVICE, > >>> + MEMTXATTRS_UNSPECIFIED); > >> > >> This function should: > >> - call memory_region_get_ram_ptr(mr) > >> if memory_region_is_ram(mr) > >> - return NULL otherwise > >> > >> There are a few reasons. First, the documentation of > dma_memory_map() > >> tells to use it "only for reads OR writes - not for read-modify-write > >> operations." It can be used for read-modify-write operations so > >> dma_memory_map() should be avoided. > > This patch series only deals with non-virgl use-cases where AFAICS > resources > > are not written to on the Host. > > > >> > >> Second, it ensures that the mapped pointer is writable. > >> "[PATCH v2 10/10] virtio-gpu-dmabuf: Create dmabuf for blobs > associated > >> with VFIO devices" adds checks for memory_region_is_ram() and > >> memory_region_is_ram_device() to virtio_gpu_init_dmabuf(), but the > other > >> callers also use the function to map writable pointers. > > Unless I am missing something, I don't see where writable pointers are > used > > in non-virgl use-cases? > > Rutabaga uses too, but you are right about that 2D operations won't use it. > > That said, exposing non-writable memory to Virgl and Rutabaga lets the > guest corrupt memory so should be avoided. On the other hand, it is > unlikely that rejecting non-writable memory will cause any problem. You > can also add another code path to use > memory_region_supports_direct_access() instead of > memory_region_is_ram() > for virtio-gpu for 2D and avoid calling memory_region_is_ram() in > virtio_gpu_init_dmabuf() if you want to keep non-writable memory working. AFAICS, virtio_gpu_init_dmabuf() is only called in non-virgl/non-rutabaga code. And, this patch series and my use-case (GPU SRIOV) only needs to deal with non-writeable memory because the rendering is already done by the Guest and the Host only needs to display the Guest's FB.
However, I see that virtio_gpu_create_mapping_iov() is used by virgl/rutabaga code as well, so I am wondering how do things work right now given that virtio_gpu_create_mapping_iov() always calls dma_memory_map()? In other words, is there no problem currently with non-writeable memory in virgl/rutabaga use-cases? > > > > >> > >> It also makes the check of memory_region_is_ram_device() and > >> memory_region_is_ram() unnecessary for virtio_gpu_init_dmabuf(), > reducing > >> the overall complexity. > > Since buffers reside completely in either ram or ram_device regions, using > both > > memory_region_is_ram_device() and memory_region_is_ram() to check > where > > they are located seems necessary and unavoidable. > > It can unconditionally call virtio_gpu_create_udmabuf(), and if the > function finds the memory is incompatible with udmabuf, it can call > vfio_device_lookup() to tell if the memory belongs to VFIO or not. Yeah, what you suggest is doable but seems a bit convoluted to have to first call virtio_gpu_create_udmabuf() and if it fails then call VFIO related functions. I think using memory_region_is_ram_device() and memory_region_is_ram() to identify the right memory region and calling either virtio_gpu_create_udmabuf() or vfio_create_dmabuf() is much more intuitive and readable. Thanks, Vivek > > Regards, > Akihiko Odaki
