On Tue, Jan 21, 2025 at 3:53 PM Jonah Palmer <jonah.pal...@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.pal...@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?
> >> Signed-off-by: Jonah Palmer <jonah.pal...@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
> >>
> >
>
