RE: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>unset_iommu_devices
>
>We are currently missing the deallocation of the [host_]resv_regions
>in case of hot unplug. Also to make things more simple let's rule
>out the case where multiple HostIOMMUDevices would be aliased and
>attached to the same IOMMUDevice. This allows to remove the handling
>of conflicting Host reserved regions. Anyway this is not properly
>supported at guest kernel level. On hotunplug the reserved regions
>are reset to the ones set by virtio-iommu property.
>
>Signed-off-by: Eric Auger <eric.auger@redhat.com>
>---
> hw/virtio/virtio-iommu.c | 62 ++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 34 deletions(-)
>
>diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>index 2c54c0d976..2de41ab412 100644
>--- a/hw/virtio/virtio-iommu.c
>+++ b/hw/virtio/virtio-iommu.c
>@@ -538,8 +538,6 @@ static int
>virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
> {
>     IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>     IOMMUDevice *sdev;
>-    GList *current_ranges;
>-    GList *l, *tmp, *new_ranges = NULL;
>     int ret = -EINVAL;
>
>     if (!sbus) {
>@@ -553,33 +551,10 @@ static int
>virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>         return ret;
>     }
>
>-    current_ranges = sdev->host_resv_ranges;
>-
>-    /* check that each new resv region is included in an existing one */
>     if (sdev->host_resv_ranges) {
>-        range_inverse_array(iova_ranges,
>-                            &new_ranges,
>-                            0, UINT64_MAX);
>-
>-        for (tmp = new_ranges; tmp; tmp = tmp->next) {
>-            Range *newr = (Range *)tmp->data;
>-            bool included = false;
>-
>-            for (l = current_ranges; l; l = l->next) {
>-                Range * r = (Range *)l->data;
>-
>-                if (range_contains_range(r, newr)) {
>-                    included = true;
>-                    break;
>-                }
>-            }
>-            if (!included) {
>-                goto error;
>-            }
>-        }
>-        /* all new reserved ranges are included in existing ones */
>-        ret = 0;
>-        goto out;
>+        error_setg(errp, "%s virtio-iommu does not support aliased BDF",
>+                   __func__);
>+        return ret;
>     }
>
>     range_inverse_array(iova_ranges,
>@@ -588,14 +563,31 @@ static int
>virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>     rebuild_resv_regions(sdev);
>
>     return 0;
>-error:
>-    error_setg(errp, "%s Conflicting host reserved ranges set!",
>-               __func__);
>-out:
>-    g_list_free_full(new_ranges, g_free);
>-    return ret;
> }
>
>+static void virtio_iommu_unset_host_iova_ranges(VirtIOIOMMU *s,
>PCIBus *bus,
>+                                                int devfn)
>+{
>+    IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>+    IOMMUDevice *sdev;
>+
>+    if (!sbus) {
>+        return;
>+    }
>+
>+    sdev = sbus->pbdev[devfn];
>+    if (!sdev) {
>+        return;
>+    }
>+
>+    g_list_free_full(g_steal_pointer(&sdev->host_resv_ranges), g_free);
>+    g_list_free_full(sdev->resv_regions, g_free);
>+    sdev->host_resv_ranges = NULL;
>+    sdev->resv_regions = NULL;
>+    add_prop_resv_regions(sdev);

Is this necessary? rebuild_resv_regions() will do that again.

Other than that, for the whole series,

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>+}
>+
>+
> static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>new_mask,
>                                  Error **errp)
> {
>@@ -704,6 +696,8 @@ virtio_iommu_unset_iommu_device(PCIBus *bus,
>void *opaque, int devfn)
>     if (!hiod) {
>         return;
>     }
>+    virtio_iommu_unset_host_iova_ranges(viommu, hiod->aliased_bus,
>+                                        hiod->aliased_devfn);
>
>     g_hash_table_remove(viommu->host_iommu_devices, &key);
> }
>--
>2.41.0