[PATCH 0/6] VIRTIO-IOMMU/VFIO: Revert IOMMUDevice clear and fix hotunplug

[PATCH 0/6] VIRTIO-IOMMU/VFIO: Revert IOMMUDevice clear and fix hotunplug

[Eric Auger]

1b889d6e39c3 ("virtio-iommu: Clear IOMMUDevice when VFIO
device is unplugged" fixes the VFIO hotplug/hotunplug/hotplug
sequence by clearing the IOMMUDevice which backs the VFIO device.
However this brings other troubles such as a memory corruption.

Even when fixing some cleanups that were missed on the first
attempt the memory corruption still exists if the IOMMUDevice is
freed. Until we understand the exact cause let's make things simpler:
let the backing IOMMUDevice survive the unplug as what is done
on intel iommu for instance. Clean up/reset resources that would
prevent the device from being hotplugged again (probe_done is removed,
[host_]resv_regions are reset). By doing this we also rule out the
use case of aliased BDFs which is known to be not functional with
virtio-iommu (missing guest kernel support) and the virtio-iommu device
implementation is not ready either.

This series can be found at:
https://github.com/eauger/qemu/tree/virtio_iommu_device_clear_fixes_v1


Eric Auger (6):
  Revert "virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged"
  virtio-iommu: Remove probe_done
  virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices
  virtio-iommu: Remove the end point on detach
  hw/vfio/common: Add vfio_listener_region_del_iommu trace event
  virtio-iommu: Add trace point on
    virtio_iommu_detach_endpoint_from_domain

 include/hw/virtio/virtio-iommu.h |  1 -
 hw/vfio/common.c                 |  3 +-
 hw/virtio/virtio-iommu.c         | 88 +++++++++++---------------------
 hw/vfio/trace-events             |  3 +-
 hw/virtio/trace-events           |  1 +
 5 files changed, 35 insertions(+), 61 deletions(-)

-- 
2.41.0

[PATCH 1/6] Revert "virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged"

[Eric Auger]

This reverts commit 1b889d6e39c32d709f1114699a014b381bcf1cb1.
There are different problems with that tentative fix:
- Some resources are left dangling (resv_regions,
  host_resv_ranges) and memory subregions are left attached to
  the root MR although freed as embedded in the sdev IOMMUDevice.
  Finally the sdev->as is not destroyed and associated listeners
  are left.
- Even when fixing the above we observe a memory corruption
  associated with the deallocation of the IOMMUDevice. This can
  be observed when a VFIO device is hotplugged, hot-unplugged
  and a system reset is issued. At this stage we have not been
  able to identify the root cause (IOMMU MR or as structs beeing
  overwritten and used later on?).
- Another issue is HostIOMMUDevice are indexed by non aliased
  BDF whereas the IOMMUDevice is indexed by aliased BDF - yes the
  current naming is really misleading -. Given the state of the
  code I don't think the virtio-iommu device works in non
  singleton group case though.

So let's revert the patch for now. This means the IOMMU MR/as survive
the hotunplug. This is what is done in the intel_iommu for instance.
It does not sound very logical to keep those but currently there is
no symetric function to pci_device_iommu_address_space().

probe_done issue will be handled in a subsequent patch. Also
resv_regions and host_resv_regions will be deallocated separately.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/virtio/virtio-iommu.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 33ae61c4a6..4e34dacd6e 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -467,26 +467,6 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
     return &sdev->as;
 }
 
-static void virtio_iommu_device_clear(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(sdev->resv_regions, g_free);
-    sdev->resv_regions = NULL;
-    g_free(sdev);
-    sbus->pbdev[devfn] = NULL;
-}
-
 static gboolean hiod_equal(gconstpointer v1, gconstpointer v2)
 {
     const struct hiod_key *key1 = v1;
@@ -728,7 +708,6 @@ virtio_iommu_unset_iommu_device(PCIBus *bus, void *opaque, int devfn)
     }
 
     g_hash_table_remove(viommu->host_iommu_devices, &key);
-    virtio_iommu_device_clear(viommu, bus, devfn);
 }
 
 static const PCIIOMMUOps virtio_iommu_ops = {
-- 
2.41.0

Re: [PATCH 1/6] Revert "virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged"

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> This reverts commit 1b889d6e39c32d709f1114699a014b381bcf1cb1.
> There are different problems with that tentative fix:
> - Some resources are left dangling (resv_regions,
>    host_resv_ranges) and memory subregions are left attached to
>    the root MR although freed as embedded in the sdev IOMMUDevice.
>    Finally the sdev->as is not destroyed and associated listeners
>    are left.
> - Even when fixing the above we observe a memory corruption
>    associated with the deallocation of the IOMMUDevice. This can
>    be observed when a VFIO device is hotplugged, hot-unplugged
>    and a system reset is issued. At this stage we have not been
>    able to identify the root cause (IOMMU MR or as structs beeing
>    overwritten and used later on?).
> - Another issue is HostIOMMUDevice are indexed by non aliased
>    BDF whereas the IOMMUDevice is indexed by aliased BDF - yes the
>    current naming is really misleading -. Given the state of the
>    code I don't think the virtio-iommu device works in non
>    singleton group case though.
> 
> So let's revert the patch for now. This means the IOMMU MR/as survive
> the hotunplug. This is what is done in the intel_iommu for instance.
> It does not sound very logical to keep those but currently there is
> no symetric function to pci_device_iommu_address_space().

Fully agree.
  
> probe_done issue will be handled in a subsequent patch. Also
> resv_regions and host_resv_regions will be deallocated separately.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/virtio/virtio-iommu.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 33ae61c4a6..4e34dacd6e 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -467,26 +467,6 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
>       return &sdev->as;
>   }
>   
> -static void virtio_iommu_device_clear(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(sdev->resv_regions, g_free);
> -    sdev->resv_regions = NULL;
> -    g_free(sdev);
> -    sbus->pbdev[devfn] = NULL;
> -}
> -
>   static gboolean hiod_equal(gconstpointer v1, gconstpointer v2)
>   {
>       const struct hiod_key *key1 = v1;
> @@ -728,7 +708,6 @@ virtio_iommu_unset_iommu_device(PCIBus *bus, void *opaque, int devfn)
>       }
>   
>       g_hash_table_remove(viommu->host_iommu_devices, &key);
> -    virtio_iommu_device_clear(viommu, bus, devfn);
>   }
>   
>   static const PCIIOMMUOps virtio_iommu_ops = {

[PATCH 2/6] virtio-iommu: Remove probe_done

[Eric Auger]

Now we have switched to PCIIOMMUOps to convey host IOMMU information,
the host reserved regions are transmitted when the PCIe topology is
built. This happens way before the virtio-iommu driver calls the probe
request. So let's remove the probe_done flag that allowed to check
the probe was not done before the IOMMU MR got enabled. Besides this
probe_done flag had a flaw wrt migration since it was not saved/restored.

The only case at risk is if 2 devices were plugged to a
PCIe to PCI bridge and thus aliased. First of all we
discovered in the past this case was not properly supported for
neither SMMU nor virtio-iommu on guest kernel side: see

[RFC] virtio-iommu: Take into account possible aliasing in virtio_iommu_mr()
https://lore.kernel.org/all/20230116124709.793084-1-eric.auger@redhat.com/

If this were supported by the guest kernel, it is unclear what the call
sequence would be from a virtio-iommu driver point of view.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/virtio/virtio-iommu.h | 1 -
 hw/virtio/virtio-iommu.c         | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index bdb3da72d0..7db4210b16 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -43,7 +43,6 @@ typedef struct IOMMUDevice {
     MemoryRegion bypass_mr;     /* The alias of shared memory MR */
     GList *resv_regions;
     GList *host_resv_ranges;
-    bool probe_done;
 } IOMMUDevice;
 
 typedef struct IOMMUPciBus {
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 4e34dacd6e..2c54c0d976 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -555,8 +555,6 @@ static int virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
 
     current_ranges = sdev->host_resv_ranges;
 
-    g_assert(!sdev->probe_done);
-
     /* check that each new resv region is included in an existing one */
     if (sdev->host_resv_ranges) {
         range_inverse_array(iova_ranges,
@@ -956,7 +954,6 @@ static int virtio_iommu_probe(VirtIOIOMMU *s,
     }
     buf += count;
     free -= count;
-    sdev->probe_done = true;
 
     return VIRTIO_IOMMU_S_OK;
 }
-- 
2.41.0

Re: [PATCH 2/6] virtio-iommu: Remove probe_done

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> Now we have switched to PCIIOMMUOps to convey host IOMMU information,
> the host reserved regions are transmitted when the PCIe topology is
> built. This happens way before the virtio-iommu driver calls the probe
> request. So let's remove the probe_done flag that allowed to check
> the probe was not done before the IOMMU MR got enabled. Besides this
> probe_done flag had a flaw wrt migration since it was not saved/restored.
> 
> The only case at risk is if 2 devices were plugged to a
> PCIe to PCI bridge and thus aliased. First of all we
> discovered in the past this case was not properly supported for
> neither SMMU nor virtio-iommu on guest kernel side: see
> 
> [RFC] virtio-iommu: Take into account possible aliasing in virtio_iommu_mr()
> https://lore.kernel.org/all/20230116124709.793084-1-eric.auger@redhat.com/
> 
> If this were supported by the guest kernel, it is unclear what the call
> sequence would be from a virtio-iommu driver point of view.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   include/hw/virtio/virtio-iommu.h | 1 -
>   hw/virtio/virtio-iommu.c         | 3 ---
>   2 files changed, 4 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
> index bdb3da72d0..7db4210b16 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -43,7 +43,6 @@ typedef struct IOMMUDevice {
>       MemoryRegion bypass_mr;     /* The alias of shared memory MR */
>       GList *resv_regions;
>       GList *host_resv_ranges;
> -    bool probe_done;
>   } IOMMUDevice;
>   
>   typedef struct IOMMUPciBus {
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 4e34dacd6e..2c54c0d976 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -555,8 +555,6 @@ static int virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>   
>       current_ranges = sdev->host_resv_ranges;
>   
> -    g_assert(!sdev->probe_done);
> -
>       /* check that each new resv region is included in an existing one */
>       if (sdev->host_resv_ranges) {
>           range_inverse_array(iova_ranges,
> @@ -956,7 +954,6 @@ static int virtio_iommu_probe(VirtIOIOMMU *s,
>       }
>       buf += count;
>       free -= count;
> -    sdev->probe_done = true;
>   
>       return VIRTIO_IOMMU_S_OK;
>   }

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

[Eric Auger]

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);
+}
+
+
 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

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

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> 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>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

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

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

[Eric Auger]

Hi Zhenzhong,

On 7/17/24 05:06, Duan, Zhenzhong wrote:
>
>> -----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.
My goal was to reset the state that existed before the

virtio_iommu_set_host_iova_ranges() was called. prop resv regions were originally added in virtio_iommu_find_add_as. 
The next device to be hotplugged at this aliased bdf is not necessarily a VFIO device (may be a virtio one), in which case we would miss the prop resv regions.

>
> Other than that, for the whole series,
>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks!

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

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: Re: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>unset_iommu_devices
>
>Hi Zhenzhong,
>
>On 7/17/24 05:06, Duan, Zhenzhong wrote:
>>
>>> -----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.
>My goal was to reset the state that existed before the
>
>virtio_iommu_set_host_iova_ranges() was called. prop resv regions were
>originally added in virtio_iommu_find_add_as.
>The next device to be hotplugged at this aliased bdf is not necessarily a VFIO
>device (may be a virtio one), in which case we would miss the prop resv
>regions.

Yeah, you are right, we must have it.

Thanks
Zhenzhong

>
>>
>> Other than that, for the whole series,
>>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Thanks!
>
>Eric
>>
>> 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

[PATCH 4/6] virtio-iommu: Remove the end point on detach

[Eric Auger]

We currently miss the removal of the endpoint in case of detach.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/virtio/virtio-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 2de41ab412..440dfa6e92 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -786,6 +786,7 @@ static int virtio_iommu_detach(VirtIOIOMMU *s,
     if (QLIST_EMPTY(&domain->endpoint_list)) {
         g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
     }
+    g_tree_remove(s->endpoints, GUINT_TO_POINTER(ep_id));
     return VIRTIO_IOMMU_S_OK;
 }
 
-- 
2.41.0

Re: [PATCH 4/6] virtio-iommu: Remove the end point on detach

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> We currently miss the removal of the endpoint in case of detach.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/virtio/virtio-iommu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 2de41ab412..440dfa6e92 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -786,6 +786,7 @@ static int virtio_iommu_detach(VirtIOIOMMU *s,
>       if (QLIST_EMPTY(&domain->endpoint_list)) {
>           g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
>       }
> +    g_tree_remove(s->endpoints, GUINT_TO_POINTER(ep_id));
>       return VIRTIO_IOMMU_S_OK;
>   }
>   

[PATCH 5/6] hw/vfio/common: Add vfio_listener_region_del_iommu trace event

[Eric Auger]

Trace when VFIO gets notified about the deletion of an IOMMU MR.
Also trace the name of the region in the add_iommu trace message.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/common.c     | 3 ++-
 hw/vfio/trace-events | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 6d15b36e0b..cfc44a4569 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -599,7 +599,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
         IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
         int iommu_idx;
 
-        trace_vfio_listener_region_add_iommu(iova, end);
+        trace_vfio_listener_region_add_iommu(section->mr->name, iova, end);
         /*
          * FIXME: For VFIO iommu types which have KVM acceleration to
          * avoid bouncing all map/unmaps through qemu this way, this
@@ -725,6 +725,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
     if (memory_region_is_iommu(section->mr)) {
         VFIOGuestIOMMU *giommu;
 
+        trace_vfio_listener_region_del_iommu(section->mr->name);
         QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
             if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
                 giommu->n.start == section->offset_within_region) {
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index e16179b507..98bd4dccea 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -95,7 +95,8 @@ vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t d
 vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_skip(const char *name, uint64_t start, uint64_t end) "SKIPPING %s 0x%"PRIx64" - 0x%"PRIx64
 vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
-vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64
+vfio_listener_region_add_iommu(const char* name, uint64_t start, uint64_t end) "region_add [iommu] %s 0x%"PRIx64" - 0x%"PRIx64
+vfio_listener_region_del_iommu(const char *name) "region_del [iommu] %s"
 vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]"
 vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_size=0x%"PRIxPTR
 vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA"
-- 
2.41.0

Re: [PATCH 5/6] hw/vfio/common: Add vfio_listener_region_del_iommu trace event

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> Trace when VFIO gets notified about the deletion of an IOMMU MR.
> Also trace the name of the region in the add_iommu trace message.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/common.c     | 3 ++-
>   hw/vfio/trace-events | 3 ++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6d15b36e0b..cfc44a4569 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -599,7 +599,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>           int iommu_idx;
>   
> -        trace_vfio_listener_region_add_iommu(iova, end);
> +        trace_vfio_listener_region_add_iommu(section->mr->name, iova, end);
>           /*
>            * FIXME: For VFIO iommu types which have KVM acceleration to
>            * avoid bouncing all map/unmaps through qemu this way, this
> @@ -725,6 +725,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>       if (memory_region_is_iommu(section->mr)) {
>           VFIOGuestIOMMU *giommu;
>   
> +        trace_vfio_listener_region_del_iommu(section->mr->name);
>           QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>               if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>                   giommu->n.start == section->offset_within_region) {
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index e16179b507..98bd4dccea 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -95,7 +95,8 @@ vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t d
>   vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
>   vfio_listener_region_skip(const char *name, uint64_t start, uint64_t end) "SKIPPING %s 0x%"PRIx64" - 0x%"PRIx64
>   vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
> -vfio_listener_region_add_iommu(uint64_t start, uint64_t end) "region_add [iommu] 0x%"PRIx64" - 0x%"PRIx64
> +vfio_listener_region_add_iommu(const char* name, uint64_t start, uint64_t end) "region_add [iommu] %s 0x%"PRIx64" - 0x%"PRIx64
> +vfio_listener_region_del_iommu(const char *name) "region_del [iommu] %s"
>   vfio_listener_region_add_ram(uint64_t iova_start, uint64_t iova_end, void *vaddr) "region_add [ram] 0x%"PRIx64" - 0x%"PRIx64" [%p]"
>   vfio_known_safe_misalignment(const char *name, uint64_t iova, uint64_t offset_within_region, uintptr_t page_size) "Region \"%s\" iova=0x%"PRIx64" offset_within_region=0x%"PRIx64" qemu_real_host_page_size=0x%"PRIxPTR
>   vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t size, uint64_t page_size) "Region \"%s\" 0x%"PRIx64" size=0x%"PRIx64" is not aligned to 0x%"PRIx64" and cannot be mapped for DMA"

[PATCH 6/6] virtio-iommu: Add trace point on virtio_iommu_detach_endpoint_from_domain

[Eric Auger]

Add a trace point on virtio_iommu_detach_endpoint_from_domain().

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/virtio/virtio-iommu.c | 1 +
 hw/virtio/trace-events   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 440dfa6e92..59ef4fb217 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -308,6 +308,7 @@ static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint *ep)
     if (!ep->domain) {
         return;
     }
+    trace_virtio_iommu_detach_endpoint_from_domain(domain->id, ep->id);
     g_tree_foreach(domain->mappings, virtio_iommu_notify_unmap_cb,
                    ep->iommu_mr);
     QLIST_REMOVE(ep, next);
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index b7c04f0856..04e36ae047 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -116,6 +116,7 @@ virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, u
 virtio_iommu_set_config(uint8_t bypass) "bypass=0x%x"
 virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
+virtio_iommu_detach_endpoint_from_domain(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
 virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
 virtio_iommu_unmap_done(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
-- 
2.41.0

Re: [PATCH 6/6] virtio-iommu: Add trace point on virtio_iommu_detach_endpoint_from_domain

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> Add a trace point on virtio_iommu_detach_endpoint_from_domain().
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/virtio/virtio-iommu.c | 1 +
>   hw/virtio/trace-events   | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 440dfa6e92..59ef4fb217 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -308,6 +308,7 @@ static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint *ep)
>       if (!ep->domain) {
>           return;
>       }
> +    trace_virtio_iommu_detach_endpoint_from_domain(domain->id, ep->id);
>       g_tree_foreach(domain->mappings, virtio_iommu_notify_unmap_cb,
>                      ep->iommu_mr);
>       QLIST_REMOVE(ep, next);
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index b7c04f0856..04e36ae047 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -116,6 +116,7 @@ virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, u
>   virtio_iommu_set_config(uint8_t bypass) "bypass=0x%x"
>   virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
>   virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
> +virtio_iommu_detach_endpoint_from_domain(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
>   virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
>   virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
>   virtio_iommu_unmap_done(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64

Re: [PATCH 0/6] VIRTIO-IOMMU/VFIO: Revert IOMMUDevice clear and fix hotunplug

[Cédric Le Goater]

On 7/16/24 11:45, Eric Auger wrote:
> 1b889d6e39c3 ("virtio-iommu: Clear IOMMUDevice when VFIO
> device is unplugged" fixes the VFIO hotplug/hotunplug/hotplug
> sequence by clearing the IOMMUDevice which backs the VFIO device.
> However this brings other troubles such as a memory corruption.
> 
> Even when fixing some cleanups that were missed on the first
> attempt the memory corruption still exists if the IOMMUDevice is
> freed. Until we understand the exact cause let's make things simpler:
> let the backing IOMMUDevice survive the unplug as what is done
> on intel iommu for instance. Clean up/reset resources that would
> prevent the device from being hotplugged again (probe_done is removed,
> [host_]resv_regions are reset). By doing this we also rule out the
> use case of aliased BDFs which is known to be not functional with
> virtio-iommu (missing guest kernel support) and the virtio-iommu device
> implementation is not ready either.
> 
> This series can be found at:
> https://github.com/eauger/qemu/tree/virtio_iommu_device_clear_fixes_v1
> 
> 
> Eric Auger (6):
>    Revert "virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged"
>    virtio-iommu: Remove probe_done
>    virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices
>    virtio-iommu: Remove the end point on detach
>    hw/vfio/common: Add vfio_listener_region_del_iommu trace event
>    virtio-iommu: Add trace point on
>      virtio_iommu_detach_endpoint_from_domain
> 
>   include/hw/virtio/virtio-iommu.h |  1 -
>   hw/vfio/common.c                 |  3 +-
>   hw/virtio/virtio-iommu.c         | 88 +++++++++++---------------------
>   hw/vfio/trace-events             |  3 +-
>   hw/virtio/trace-events           |  1 +
>   5 files changed, 35 insertions(+), 61 deletions(-)
> 


Tested-by: Cédric Le Goater <clg@redhat.com>

with a vfio-pci device (mlx5 VF) and a virtio-net-pci device.

Thanks,

C.

Re: [PATCH 0/6] VIRTIO-IOMMU/VFIO: Revert IOMMUDevice clear and fix hotunplug

[Eric Auger]

Hi Cédric,

On 7/16/24 16:02, Cédric Le Goater wrote:
> On 7/16/24 11:45, Eric Auger wrote:
>> 1b889d6e39c3 ("virtio-iommu: Clear IOMMUDevice when VFIO
>> device is unplugged" fixes the VFIO hotplug/hotunplug/hotplug
>> sequence by clearing the IOMMUDevice which backs the VFIO device.
>> However this brings other troubles such as a memory corruption.
>>
>> Even when fixing some cleanups that were missed on the first
>> attempt the memory corruption still exists if the IOMMUDevice is
>> freed. Until we understand the exact cause let's make things simpler:
>> let the backing IOMMUDevice survive the unplug as what is done
>> on intel iommu for instance. Clean up/reset resources that would
>> prevent the device from being hotplugged again (probe_done is removed,
>> [host_]resv_regions are reset). By doing this we also rule out the
>> use case of aliased BDFs which is known to be not functional with
>> virtio-iommu (missing guest kernel support) and the virtio-iommu device
>> implementation is not ready either.
>>
>> This series can be found at:
>> https://github.com/eauger/qemu/tree/virtio_iommu_device_clear_fixes_v1
>>
>>
>> Eric Auger (6):
>>    Revert "virtio-iommu: Clear IOMMUDevice when VFIO device is
>> unplugged"
>>    virtio-iommu: Remove probe_done
>>    virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices
>>    virtio-iommu: Remove the end point on detach
>>    hw/vfio/common: Add vfio_listener_region_del_iommu trace event
>>    virtio-iommu: Add trace point on
>>      virtio_iommu_detach_endpoint_from_domain
>>
>>   include/hw/virtio/virtio-iommu.h |  1 -
>>   hw/vfio/common.c                 |  3 +-
>>   hw/virtio/virtio-iommu.c         | 88 +++++++++++---------------------
>>   hw/vfio/trace-events             |  3 +-
>>   hw/virtio/trace-events           |  1 +
>>   5 files changed, 35 insertions(+), 61 deletions(-)
>>
>
>
> Tested-by: Cédric Le Goater <clg@redhat.com>
>
> with a vfio-pci device (mlx5 VF) and a virtio-net-pci device.

Many Thanks!

Eric
>
> Thanks,
>
> C.
>