On 2025/11/18 14:26, 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/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?
No, my suggestion is to remove it at all. Creating udmabuf only requires
that the memory regions are backed with memfd.
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
