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

[Eugenio Perez Martin]

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.

> 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?

>   */
>  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
>