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? > >>>> > >>>> The current code is problematic, and using memory_region_is_ram() > will > >>>> fix it. > >>> Ok, I'll make the change. > >>> > >>>> > >>>>> > >>>>>>>> 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. > >>>> > >>>> memory_region_is_ram_device() and memory_region_is_ram() are not > >>>> sufficient to identify the right memory region. > >>>> memory_region_is_ram_device() returns true for RAM device created > by > >>>> non-VFIO devices, and memory_region_is_ram() returns true for > memory > >>>> regions created with memory_region_init_ram_ptr(), which is not > backed > >>>> with memfd. > >>> Right, but structuring the code in the following way would address your > >> concerns > >>> and make it more robust: > >>> if (memory_region_is_ram_device(rb->mr) && (vdev = > >> vfio_device_lookup(rb->mr))) { > >>> vfio_create_dmabuf(vdev, res); > >>> } else if (memory_region_is_ram(rb->mr) && > >> virtio_gpu_have_udmabuf()) { > >>> virtio_gpu_create_udmabuf(res); > >>> } else { > >>> ... > >>> } > >> > >> One of the concerns I raised is that having such checks has an inherent > >> hazard that they can be inconsistent with the actual implementations. > >> > >> The original checks had such inconsistency, and the updated one still > >> have too. memory_region_is_ram(rb->mr) && > virtio_gpu_have_udmabuf() > >> can > >> be still true even for memory regions that do not have memfd; please > >> refer to the example of memory_region_init_ram_ptr() I pointed out in > >> the last email. > >> > >> Even if you somehow managed to write checks that match with the > >> implementations, it is still possible that a future change can break it. > >> Letting the implementations check their prerequisite conditions > >> completely prevents such an error by construction and makes the code > >> more robust. > > IIUC, your suggestion is to add a check for > memory_region_supports_direct_access() > > inside virtio_gpu_create_udmabuf() and call it unconditionally right? > > No, my suggestion is to remove it at all. Creating udmabuf only requires > that the memory regions are backed with memfd. If we unconditionally call virtio_gpu_create_udmabuf() for VFIO backed buffers, I think it makes sense to return early without having to iterate over all the iov entries to check their memory regions. So, I am thinking adding a memory_region_supports_direct_access() or !memory_region_is_ram_device() check would help with this.
Thanks, Vivek > memory_region_supports_direct_access() on the other hand tells if the > host can access it directly by normal load and store instructions, which > is irrelevant when creating udmabuf. > > > And, also move the (memory_region_is_ram_device(rb->mr) && (vdev = > vfio_device_lookup(rb->mr))) > > check inside vfio_create_dmabuf() right? > > Moving vdev = vfio_device_lookup(rb->mr) into vfio_create_dmabuf() > sounds good to me. > memory_region_is_ram_device(rb->mr) is again irrelevant; it tells if the > memory region has a flag that prevents MMIO-incompatible operations, but > we only care if it belongs to a VFIO device. > > Regards, > Akihiko Odaki
