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/17 13:14, 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/13 12:39, 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/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? > >> > >> 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? And, also move the (memory_region_is_ram_device(rb->mr) && (vdev = vfio_device_lookup(rb->mr))) check inside vfio_create_dmabuf() right?
Thanks, Vivek > > Another concern is complexity. Calling another function > (virtio_gpu_have_udmabuf()) do not reduce complexity. > > I do not understand why having such extra function calls and > conditionals will make the code more robust. > > Regards, > Akihiko Odaki
