[RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device

Vivek Kasireddy posted 6 patches 5 months, 1 week ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
[RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device
Posted by Vivek Kasireddy 5 months, 1 week ago
If the Guest provides a gpa (guest physical address) associated with
a PCI region, then we can obtain the hva (host virtual address) via
gpa2hva() API instead of dma_memory_map(). Note that we would still
call dma_memory_unmap() (to unref mr) regardless of how we obtained
the hva.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 hw/display/virtio-gpu.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 1654a417b8..4af8390bb5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -32,6 +32,7 @@
 #include "qemu/module.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "monitor/monitor.h"
 
 #define VIRTIO_GPU_VM_VERSION 1
 
@@ -799,6 +800,36 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
                               &fb, res, &ss.r, &cmd->error);
 }
 
+static void *map_gpa2hva(VirtIOGPU *g,
+                         struct virtio_gpu_ctrl_command *cmd,
+                         uint64_t gpa, hwaddr *len)
+{
+    MemoryRegion *mr = NULL;
+    Error *errp = NULL;
+    void *map;
+
+    if (cmd->cmd_hdr.type != VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB) {
+        return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
+                              DMA_DIRECTION_TO_DEVICE,
+                              MEMTXATTRS_UNSPECIFIED);
+    }
+
+    map = gpa2hva(&mr, gpa, 1, &errp);
+    if (errp) {
+        error_report_err(errp);
+        return NULL;
+    }
+
+    if (!memory_region_is_ram_device(mr)) {
+        memory_region_unref(mr);
+        map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
+                             DMA_DIRECTION_TO_DEVICE,
+                             MEMTXATTRS_UNSPECIFIED);
+    }
+
+    return map;
+}
+
 int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
                                   uint32_t nr_entries, uint32_t offset,
                                   struct virtio_gpu_ctrl_command *cmd,
@@ -840,9 +871,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
 
         do {
             len = l;
-            map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, &len,
-                                 DMA_DIRECTION_TO_DEVICE,
-                                 MEMTXATTRS_UNSPECIFIED);
+            map = map_gpa2hva(g, cmd, a, &len);
             if (!map) {
                 qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory for"
                               " element %d\n", __func__, e);
-- 
2.50.1


Re: [RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device
Posted by Akihiko Odaki 5 months ago
On 2025/09/03 7:42, Vivek Kasireddy wrote:
> If the Guest provides a gpa (guest physical address) associated with
> a PCI region, then we can obtain the hva (host virtual address) via
> gpa2hva() API instead of dma_memory_map(). Note that we would still
> call dma_memory_unmap() (to unref mr) regardless of how we obtained
> the hva.

I think address_space_translate() should be used instead. The guest 
passes addresses that are valid in the DMA address space, which may not 
be a GPA if an IOMMU is in effect. address_space_translate() allows you 
specifying the DMA address space.

The motivation for this change should also be described in the patch 
message; otherwise a reader may wonder why dma_memory_map() does not 
suffice.

While I added comments for each patch, the overall implementation 
direction of this series looks good to me.

Regards,
Akihiko Odaki

> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>   hw/display/virtio-gpu.c | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 1654a417b8..4af8390bb5 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -32,6 +32,7 @@
>   #include "qemu/module.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
> +#include "monitor/monitor.h"
>   
>   #define VIRTIO_GPU_VM_VERSION 1
>   
> @@ -799,6 +800,36 @@ static void virtio_gpu_set_scanout_blob(VirtIOGPU *g,
>                                 &fb, res, &ss.r, &cmd->error);
>   }
>   
> +static void *map_gpa2hva(VirtIOGPU *g,
> +                         struct virtio_gpu_ctrl_command *cmd,
> +                         uint64_t gpa, hwaddr *len)
> +{
> +    MemoryRegion *mr = NULL;
> +    Error *errp = NULL;
> +    void *map;
> +
> +    if (cmd->cmd_hdr.type != VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB) {
> +        return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> +                              DMA_DIRECTION_TO_DEVICE,
> +                              MEMTXATTRS_UNSPECIFIED);
> +    }
> +
> +    map = gpa2hva(&mr, gpa, 1, &errp);
> +    if (errp) {
> +        error_report_err(errp);
> +        return NULL;
> +    }
> +
> +    if (!memory_region_is_ram_device(mr)) {
> +        memory_region_unref(mr);
> +        map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> +                             DMA_DIRECTION_TO_DEVICE,
> +                             MEMTXATTRS_UNSPECIFIED);
> +    }
> +
> +    return map;
> +}
> +
>   int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
>                                     uint32_t nr_entries, uint32_t offset,
>                                     struct virtio_gpu_ctrl_command *cmd,
> @@ -840,9 +871,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
>   
>           do {
>               len = l;
> -            map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, &len,
> -                                 DMA_DIRECTION_TO_DEVICE,
> -                                 MEMTXATTRS_UNSPECIFIED);
> +            map = map_gpa2hva(g, cmd, a, &len);
>               if (!map) {
>                   qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO memory for"
>                                 " element %d\n", __func__, e);


RE: [RFC 6/6] virtio-gpu: Find the host addr given gpa associated with a ram device
Posted by Kasireddy, Vivek 4 months, 4 weeks ago
Hi Akihiko,

> Subject: Re: [RFC 6/6] virtio-gpu: Find the host addr given gpa associated
> with a ram device
> 
> > If the Guest provides a gpa (guest physical address) associated with
> > a PCI region, then we can obtain the hva (host virtual address) via
> > gpa2hva() API instead of dma_memory_map(). Note that we would still
> > call dma_memory_unmap() (to unref mr) regardless of how we obtained
> > the hva.
> 
> I think address_space_translate() should be used instead. The guest
> passes addresses that are valid in the DMA address space, which may not
> be a GPA if an IOMMU is in effect. address_space_translate() allows you
> specifying the DMA address space.
Thank you for your suggestion. Looks like address_space_translate() followed
by memory_region_get_ram_ptr() also makes it work and is much cleaner. I'll
include this change in the next version.

> 
> The motivation for this change should also be described in the patch
> message; otherwise a reader may wonder why dma_memory_map() does
> not
> suffice.
Sure, I'll improve the commit message to explain why dma_memory_map()
does not work in this case.

> 
> While I added comments for each patch, the overall implementation
> direction of this series looks good to me.
Thank you for taking a look!

Thanks,
Vivek

> 
> Regards,
> Akihiko Odaki
> 
> >
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> > Cc: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >   hw/display/virtio-gpu.c | 35 ++++++++++++++++++++++++++++++++---
> >   1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 1654a417b8..4af8390bb5 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -32,6 +32,7 @@
> >   #include "qemu/module.h"
> >   #include "qapi/error.h"
> >   #include "qemu/error-report.h"
> > +#include "monitor/monitor.h"
> >
> >   #define VIRTIO_GPU_VM_VERSION 1
> >
> > @@ -799,6 +800,36 @@ static void
> virtio_gpu_set_scanout_blob(VirtIOGPU *g,
> >                                 &fb, res, &ss.r, &cmd->error);
> >   }
> >
> > +static void *map_gpa2hva(VirtIOGPU *g,
> > +                         struct virtio_gpu_ctrl_command *cmd,
> > +                         uint64_t gpa, hwaddr *len)
> > +{
> > +    MemoryRegion *mr = NULL;
> > +    Error *errp = NULL;
> > +    void *map;
> > +
> > +    if (cmd->cmd_hdr.type !=
> VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB) {
> > +        return dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> > +                              DMA_DIRECTION_TO_DEVICE,
> > +                              MEMTXATTRS_UNSPECIFIED);
> > +    }
> > +
> > +    map = gpa2hva(&mr, gpa, 1, &errp);
> > +    if (errp) {
> > +        error_report_err(errp);
> > +        return NULL;
> > +    }
> > +
> > +    if (!memory_region_is_ram_device(mr)) {
> > +        memory_region_unref(mr);
> > +        map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, gpa, len,
> > +                             DMA_DIRECTION_TO_DEVICE,
> > +                             MEMTXATTRS_UNSPECIFIED);
> > +    }
> > +
> > +    return map;
> > +}
> > +
> >   int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> >                                     uint32_t nr_entries, uint32_t offset,
> >                                     struct virtio_gpu_ctrl_command *cmd,
> > @@ -840,9 +871,7 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> >
> >           do {
> >               len = l;
> > -            map = dma_memory_map(VIRTIO_DEVICE(g)->dma_as, a, &len,
> > -                                 DMA_DIRECTION_TO_DEVICE,
> > -                                 MEMTXATTRS_UNSPECIFIED);
> > +            map = map_gpa2hva(g, cmd, a, &len);
> >               if (!map) {
> >                   qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map MMIO
> memory for"
> >                                 " element %d\n", __func__, e);