qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC v3 1/5] vhost-vdpa: Decouple the IOVA allocator

From: Jonah Palmer
Subject: Re: [RFC v3 1/5] vhost-vdpa: Decouple the IOVA allocator
Date: Wed, 22 Jan 2025 11:14:20 -0500
User-agent: Mozilla Thunderbird 


On 1/21/25 12:25 PM, Eugenio Perez Martin wrote:

On Tue, Jan 21, 2025 at 3:53 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:




On 1/16/25 11:44 AM, Eugenio Perez Martin wrote:

On Fri, Jan 10, 2025 at 6:09 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:


Decouples the IOVA allocator from the full IOVA->HVA tree to support a
SVQ IOVA->HVA tree for host-only memory mappings.

The IOVA allocator still allocates an IOVA range but instead adds this
range to an IOVA-only tree (iova_map) that keeps track of allocated IOVA
ranges for both guest & host-only memory mappings.

A new API function vhost_iova_tree_insert() is also created for adding
IOVA->HVA mappings into the SVQ IOVA->HVA tree, since the allocator is
no longer doing that.



What is the reason for not adding IOVA -> HVA tree on _alloc
automatically? The problematic one is GPA -> HVA, isn't it? Doing this
way we force all the allocations to do the two calls (alloc+insert),
or the trees will be inconsistent.



Ah, I believe you also made a similar comment in RFC v1, saying it
wasn't intuitive for the user to follow up with a
vhost_iova_tree_insert() call afterwards (e.g. in
vhost_vdpa_svq_map_ring() or vhost_vdpa_cvq_map_buf()).

I believe what I ended up doing in RFC v2 was creating separate alloc
functions for host-only memory mapping (e.g. vhost_vdpa_svq_map_ring()
and vhost_vdpa_cvq_map_buf()) and guest-backed memory mapping (e.g.
vhost_vdpa_listener_region_add()).

This way, for host-only memory, in the alloc function we allocate an
IOVA range (in the IOVA-only tree) and then also inserts the IOVA->HVA
mapping into the SVQ IOVA->HVA tree. Similarly, for guest-backed memory,
we create its own alloc function (e.g. vhost_iova_tree_map_alloc_gpa()),
allocate the IOVA range (in the IOVA-only tree) and then insert the
GPA->IOVA mapping into the GPA->IOVA tree.

This was done so that we didn't have to rely on the user to also call
the insertion function after calling the allocation function.

Is this kinda what you're thinking of here?



Right, I think it makes more sense. Do you think differently, maybe I
missed any drawbacks?



No, I totally think this is fine. I can't think of any serious drawbacks.

Will do in the next series!


Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
   hw/virtio/vhost-iova-tree.c | 35 +++++++++++++++++++++++++++++++----
   hw/virtio/vhost-iova-tree.h |  1 +
   hw/virtio/vhost-vdpa.c      | 21 ++++++++++++++++-----
   net/vhost-vdpa.c            | 13 +++++++++++--
   4 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/vhost-iova-tree.c b/hw/virtio/vhost-iova-tree.c
index 3d03395a77..b1cfd17843 100644
--- a/hw/virtio/vhost-iova-tree.c
+++ b/hw/virtio/vhost-iova-tree.c
@@ -28,12 +28,15 @@ struct VhostIOVATree {

       /* IOVA address to qemu memory maps. */
       IOVATree *iova_taddr_map;
+
+    /* Allocated IOVA addresses */
+    IOVATree *iova_map;
   };

   /**
- * Create a new IOVA tree
+ * Create a new VhostIOVATree
    *
- * Returns the new IOVA tree
+ * Returns the new VhostIOVATree
    */
   VhostIOVATree *vhost_iova_tree_new(hwaddr iova_first, hwaddr iova_last)
   {
@@ -44,15 +47,17 @@ VhostIOVATree *vhost_iova_tree_new(hwaddr iova_first, 
hwaddr iova_last)
       tree->iova_last = iova_last;

       tree->iova_taddr_map = iova_tree_new();
+    tree->iova_map = iova_tree_new();
       return tree;
   }

   /**
- * Delete an iova tree
+ * Delete a VhostIOVATree


Thanks for fixing the doc of new and delete :) Maybe it is better to
put it in an independent patch?



Sure can :)


    */
   void vhost_iova_tree_delete(VhostIOVATree *iova_tree)
   {
       iova_tree_destroy(iova_tree->iova_taddr_map);
+    iova_tree_destroy(iova_tree->iova_map);
       g_free(iova_tree);
   }

@@ -94,7 +99,7 @@ int vhost_iova_tree_map_alloc(VhostIOVATree *tree, DMAMap 
*map)
       }

       /* Allocate a node in IOVA address */
-    return iova_tree_alloc_map(tree->iova_taddr_map, map, iova_first,
+    return iova_tree_alloc_map(tree->iova_map, map, iova_first,
                                  tree->iova_last);
   }

@@ -107,4 +112,26 @@ int vhost_iova_tree_map_alloc(VhostIOVATree *tree, DMAMap 
*map)
   void vhost_iova_tree_remove(VhostIOVATree *iova_tree, DMAMap map)
   {
       iova_tree_remove(iova_tree->iova_taddr_map, map);
+    iova_tree_remove(iova_tree->iova_map, map);
+}
+
+/**
+ * Insert a new mapping to the IOVA->HVA tree
+ *
+ * @tree: The VhostIOVATree
+ * @map: The IOVA->HVA mapping
+ *
+ * Returns:
+ * - IOVA_OK if the map fits in the container
+ * - IOVA_ERR_INVALID if the map does not make sense (e.g. size overflow)
+ * - IOVA_ERR_OVERLAP if the IOVA range overlaps with an existing range
+ */
+int vhost_iova_tree_insert(VhostIOVATree *iova_tree, DMAMap *map)
+{
+    if (map->translated_addr + map->size < map->translated_addr ||
+        map->perm == IOMMU_NONE) {
+        return IOVA_ERR_INVALID;
+    }
+
+    return iova_tree_insert(iova_tree->iova_taddr_map, map);
   }
diff --git a/hw/virtio/vhost-iova-tree.h b/hw/virtio/vhost-iova-tree.h
index 4adfd79ff0..8bf7b64786 100644
--- a/hw/virtio/vhost-iova-tree.h
+++ b/hw/virtio/vhost-iova-tree.h
@@ -23,5 +23,6 @@ const DMAMap *vhost_iova_tree_find_iova(const VhostIOVATree 
*iova_tree,
                                           const DMAMap *map);
   int vhost_iova_tree_map_alloc(VhostIOVATree *iova_tree, DMAMap *map);
   void vhost_iova_tree_remove(VhostIOVATree *iova_tree, DMAMap map);
+int vhost_iova_tree_insert(VhostIOVATree *iova_tree, DMAMap *map);

   #endif
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3cdaa12ed5..f5803f35f4 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1142,18 +1142,29 @@ static void vhost_vdpa_svq_unmap_rings(struct vhost_dev 
*dev,
    *
    * @v: Vhost-vdpa device
    * @needle: The area to search iova
+ * @taddr: The translated address (SVQ HVA)
    * @errorp: Error pointer
    */
   static bool vhost_vdpa_svq_map_ring(struct vhost_vdpa *v, DMAMap *needle,
-                                    Error **errp)
+                                    hwaddr taddr, Error **errp)
   {
       int r;

+    /* Allocate an IOVA range in the IOVA tree */
       r = vhost_iova_tree_map_alloc(v->shared->iova_tree, needle);
       if (unlikely(r != IOVA_OK)) {
           error_setg(errp, "Cannot allocate iova (%d)", r);
           return false;
       }
+    needle->translated_addr = taddr;
+
+    /* Add IOVA->HVA mapping to the IOVA->HVA tree */
+    r = vhost_iova_tree_insert(v->shared->iova_tree, needle);
+    if (unlikely(r != IOVA_OK)) {
+        error_setg(errp, "Cannot add SVQ vring mapping (%d)", r);
+        vhost_iova_tree_remove(v->shared->iova_tree, *needle);
+        return false;
+    }

       r = vhost_vdpa_dma_map(v->shared, v->address_space_id, needle->iova,
                              needle->size + 1,
@@ -1192,11 +1203,11 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev 
*dev,
       vhost_svq_get_vring_addr(svq, &svq_addr);

       driver_region = (DMAMap) {
-        .translated_addr = svq_addr.desc_user_addr,
           .size = driver_size - 1,
           .perm = IOMMU_RO,
       };
-    ok = vhost_vdpa_svq_map_ring(v, &driver_region, errp);
+    ok = vhost_vdpa_svq_map_ring(v, &driver_region, svq_addr.desc_user_addr,
+                                 errp);
       if (unlikely(!ok)) {
           error_prepend(errp, "Cannot create vq driver region: ");
           return false;
@@ -1206,11 +1217,11 @@ static bool vhost_vdpa_svq_map_rings(struct vhost_dev 
*dev,
       addr->avail_user_addr = driver_region.iova + avail_offset;

       device_region = (DMAMap) {
-        .translated_addr = svq_addr.used_user_addr,
           .size = device_size - 1,
           .perm = IOMMU_RW,
       };
-    ok = vhost_vdpa_svq_map_ring(v, &device_region, errp);
+    ok = vhost_vdpa_svq_map_ring(v, &device_region, svq_addr.used_user_addr,
+                                 errp);
       if (unlikely(!ok)) {
           error_prepend(errp, "Cannot create vq device region: ");
           vhost_vdpa_svq_unmap_ring(v, driver_region.translated_addr);
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 231b45246c..1ef555e04e 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -512,14 +512,23 @@ static int vhost_vdpa_cvq_map_buf(struct vhost_vdpa *v, 
void *buf, size_t size,
       DMAMap map = {};
       int r;

-    map.translated_addr = (hwaddr)(uintptr_t)buf;
       map.size = size - 1;
       map.perm = write ? IOMMU_RW : IOMMU_RO,
+
+    /* Allocate an IOVA range in the IOVA tree */
       r = vhost_iova_tree_map_alloc(v->shared->iova_tree, &map);
       if (unlikely(r != IOVA_OK)) {
-        error_report("Cannot map injected element");
+        error_report("Cannot allocate IOVA range for injected element");
           return r;
       }
+    map.translated_addr = (hwaddr)(uintptr_t)buf;
+
+    /* Add IOVA->HVA mapping to the IOVA->HVA tree */
+    r = vhost_iova_tree_insert(v->shared->iova_tree, &map);
+    if (unlikely(r != IOVA_OK)) {
+        error_report("Cannot map injected element into IOVA->HVA tree");
+        goto dma_map_err;
+    }

       r = vhost_vdpa_dma_map(v->shared, v->address_space_id, map.iova,
                              vhost_vdpa_net_cvq_cmd_page_len(), buf, !write);
--
2.43.5
reply via email to
[Prev in Thread] Current Thread [Next in Thread]