Hi Akihiko,
> Subject: Re: [PATCH v13 1/9] virtio-gpu: Find hva for Guest's DMA addr
> associated with a ram device
>
> >>> If the Guest provides a DMA address that is associated with a ram
> >>> device (such as a VFIO 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().
> >>>
> >>> We cannot use dma_memory_map() because for memory regions that
> >> do
> >>> not support direct access, it would create bounce buffers instead
> >>> of returning the actual hva, which is not desirable here. And, since
> >>> VFIO device regions are not considered directly accessible (because
> >>> they are mostly MMIO based), virtio-gpu cannot invoke
> >> dma_memory_map()
> >>> to obtain the hva in this case.
> >>>
> >>> Therefore, in order to handle addresses associated with VFIO devices,
> >>> we need to use the address_space_translate() API to first identify
> >>> the right memory region and the appropriate offset within that
> >>> region and then use memory_region_get_ram_ptr() to get the hva.
> >>> This approach also works for addresses associated with the system
> >>> memory region.
> >>>
> >>> Note that, although we take an explicit reference on the memory
> >>> region, we would still rely on dma_memory_unmap() to drop that
> >>> reference when the dma mapping is eventually unmapped.
> >>>
> >>> 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]>
> >>> Reviewed-by: Akihiko Odaki <[email protected]>
> >>> Signed-off-by: Vivek Kasireddy <[email protected]>
> >>> ---
> >>> hw/display/virtio-gpu.c | 21 ++++++++++++++++++---
> >>> 1 file changed, 18 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>> index bb0f41bb1e..386db05e5b 100644
> >>> --- a/hw/display/virtio-gpu.c
> >>> +++ b/hw/display/virtio-gpu.c
> >>> @@ -948,6 +948,23 @@ 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;
> >>> + 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(mr)) {
> >>> + memory_region_ref(mr);
> >>> + return memory_region_get_ram_ptr(mr) + xlat;
> >>
> >> On rechecking this, I don't think it is safe with Xen.
> >>
> >> memory_region_get_ram_ptr(mr) + xlat assumes a contiguous host
> >> mapping
> >> of the entire region. Xen maps guest RAM on demand through its
> >> mapcache.
> >> Here, memory_region_get_ram_ptr() obtains a mapping at offset zero
> >> without pinning the cache entry, and adding xlat afterward does not
> >> ensure that the requested range is mapped.
> >>
> >> The MemoryRegion reference keeps the region alive, but does not
> >> prevent
> >> the mapcache entry from being reused. Resource backing can therefore
> >> receive an invalid or unpinned pointer.
> >>
> >> I think we need to extend dma_memory_map() for this use case instead
> >> of
> >> replacing it, preserving its mapping and unmapping semantics.
> > I am not sure if extending dma_memory_map() might be a
> better/cleaner
> > solution for this use-case. If obtaining the hva/host ptr via
> > memory_region_get_ram_ptr(mr) + xlat for guest RAM is not safe with
> > Xen, then would it not make more sense to go back to using
> dma_memory_map()
> > for Guest RAM and memory_region_get_ram_ptr(mr) + xlat for VFIO
> > backed regions? That is, doing the following:
> >
> > 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);
> > }
>
> Checking this logic again, I found this patch is not necessary since
> commit ff77a515113f ("system/memory: Make ram device region directly
> accessible"). We can just drop it.
You are right, this patch is no longer necessary in light of ff77a515113f.
I dropped it from this series and re-tested my use-case again and everything
seems to work as expected.
Thanks,
Vivek
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks,
> > Vivek
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>> + }
> >>> + return NULL;
> >>> +}
> >>> +
> >>> int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> >>> uint32_t nr_entries, uint32_t offset,
> >>> struct virtio_gpu_ctrl_command *cmd,
> >>> @@ -993,9 +1010,7 @@ int
> virtio_gpu_create_mapping_iov(VirtIOGPU
> >> *g,
> >>> /* TODO: a common DMA map SG helper */
> >>> do {
> >>> len = l;
> >>> - map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a,
> >> &len,
> >>> - DMA_DIRECTION_TO_DEVICE,
> >>> - MEMTXATTRS_UNSPECIFIED);
> >>> + map = virtio_gpu_dma_memory_map(g, cmd, a, &len);
> >>> if (!map) {
> >>> qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map
> >> MMIO memory for"
> >>> " element %d\n", __func__, e);
> >
