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 {
...
}
Thanks,
Vivek
>
> In general, checking prerequisite conditions before performing
> operations implemented elsewhere is dangerous because the checks and
> the
> operation implementation can be diverged, and these facts demonstrate
> that.
>
> Regards,
> Akihiko Odaki
