[PATCH 0/7] VIRTIO-IOMMU/HostIOMMUDevice: Fixes and page size mask rework

[PATCH 0/7] VIRTIO-IOMMU/HostIOMMUDevice: Fixes and page size mask rework

[Eric Auger]

The 2 first patches are fixes of
cf2647a76e ("virtio-iommu: Compute host reserved regions")
They can be taken separately of the rest.

Then the series uses the HostIOMMUDevice interface to fetch
information about the page size mask supported along the assigned
device and propagate it to the virtio-iommu. This is a similar
work as what was done in

VIRTIO-IOMMU/VFIO: Fix host iommu geometry handling series

but this time for the page size mask. Using this new
infrastructure allows to handle page size mask conflicts
earlier on device hotplug as opposed to current QEMU
abort:

qemu-system-aarch64: virtio-iommu virtio-iommu-memory-region-8-0
does not support frozen granule 0x10000
qemu: hardware error: vfio: DMA mapping failed, unable to continue

With this series the hotplug nicely fails.

Also this allows to remove hacks consisting in transiently enabling
IOMMU MRs to collect coldplugged device page size mask before machine
init. Those hacks were introduced by

94df5b2180d6 ("virtio-iommu: Fix 64kB host page size VFIO device
assignment")

The series can be found at:
https://github.com/eauger/qemu/tree/virtio-iommu-psmask-rework-v1


Eric Auger (7):
  virtio-iommu: Fix error handling in
    virtio_iommu_set_host_iova_ranges()
  vfio-container-base: Introduce vfio_container_get_iova_ranges() helper
  HostIOMMUDevice : remove Error handle from get_iova_ranges callback
  HostIOMMUDevice: Introduce get_page_size_mask() callback
  virtio-iommu : Retrieve page size mask on
    virtio_iommu_set_iommu_device()
  memory: remove IOMMU MR iommu_set_page_size_mask() callback
  virtio-iommu: Revert transient enablement of IOMMU MR in bypass mode

 include/exec/memory.h                 |  38 --------
 include/hw/vfio/vfio-container-base.h |   9 ++
 include/sysemu/host_iommu_device.h    |  11 ++-
 hw/vfio/common.c                      |   8 --
 hw/vfio/container-base.c              |  15 ++++
 hw/vfio/container.c                   |  16 ++--
 hw/vfio/iommufd.c                     |  21 +++--
 hw/virtio/virtio-iommu.c              | 121 +++++++++++++-------------
 system/memory.c                       |  13 ---
 hw/virtio/trace-events                |   2 +-
 10 files changed, 117 insertions(+), 137 deletions(-)

-- 
2.41.0

[PATCH 1/7] virtio-iommu: Fix error handling in virtio_iommu_set_host_iova_ranges()

[Eric Auger]

In case no IOMMUPciBus/IOMMUDevice are found we need to properly
set the error handle and return.

Fixes : Coverity CID 1549006
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Fixes: cf2647a76e ("virtio-iommu: Compute host reserved regions")
---
 hw/virtio/virtio-iommu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index b9a7ddcd14..b708fb96fd 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -543,10 +543,15 @@ static int virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
     int ret = -EINVAL;
 
     if (!sbus) {
-        error_report("%s no sbus", __func__);
+        error_setg(errp, "%s: no IOMMUPciBus found!", __func__);
+        return ret;
     }
 
     sdev = sbus->pbdev[devfn];
+    if (!sdev) {
+        error_setg(errp, "%s: no IOMMUDevice found!", __func__);
+        return ret;
+    }
 
     current_ranges = sdev->host_resv_ranges;
 
-- 
2.41.0

[PATCH 2/7] vfio-container-base: Introduce vfio_container_get_iova_ranges() helper

[Eric Auger]

Introduce vfio_container_get_iova_ranges() to retrieve the usable
IOVA regions of the base container and use it in the Host IOMMU
device implementations of get_iova_ranges() callback.

We also fix a UAF bug as the list was shallow copied while
g_list_free_full() was used both on the single call site, in
virtio_iommu_set_iommu_device() but also in
vfio_container_instance_finalize(). Instead use g_list_copy_deep.

Fixes: cf2647a76e ("virtio-iommu: Compute host reserved regions")
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/vfio/vfio-container-base.h |  2 ++
 hw/vfio/container-base.c              | 15 +++++++++++++++
 hw/vfio/container.c                   |  8 +-------
 hw/vfio/iommufd.c                     |  8 +-------
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 419e45ee7a..45d7c40fce 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -86,6 +86,8 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
 
+GList *vfio_container_get_iova_ranges(const VFIOContainerBase *bcontainer);
+
 #define TYPE_VFIO_IOMMU "vfio-iommu"
 #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
 #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 50b1664f89..809b157674 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -83,6 +83,21 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                                                errp);
 }
 
+static gpointer copy_iova_range(gconstpointer src, gpointer data)
+{
+     Range *source = (Range *)src;
+     Range *dest = g_new(Range, 1);
+
+     range_set_bounds(dest, range_lob(source), range_upb(source));
+     return dest;
+}
+
+GList *vfio_container_get_iova_ranges(const VFIOContainerBase *bcontainer)
+{
+    assert(bcontainer);
+    return g_list_copy_deep(bcontainer->iova_ranges, copy_iova_range, NULL);
+}
+
 static void vfio_container_instance_finalize(Object *obj)
 {
     VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 2e7ecdf10e..2ad57cd845 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1169,15 +1169,9 @@ static GList *
 hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
 {
     VFIODevice *vdev = hiod->agent;
-    GList *l = NULL;
 
     g_assert(vdev);
-
-    if (vdev->bcontainer) {
-        l = g_list_copy(vdev->bcontainer->iova_ranges);
-    }
-
-    return l;
+    return vfio_container_get_iova_ranges(vdev->bcontainer);
 }
 
 static void vfio_iommu_legacy_instance_init(Object *obj)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index c2f158e603..890d8d6a38 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -647,15 +647,9 @@ static GList *
 hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
 {
     VFIODevice *vdev = hiod->agent;
-    GList *l = NULL;
 
     g_assert(vdev);
-
-    if (vdev->bcontainer) {
-        l = g_list_copy(vdev->bcontainer->iova_ranges);
-    }
-
-    return l;
+    return vfio_container_get_iova_ranges(vdev->bcontainer);
 }
 
 static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
-- 
2.41.0

[PATCH 3/7] HostIOMMUDevice : remove Error handle from get_iova_ranges callback

[Eric Auger]

The error handle argument is not used anywhere. let's remove it.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/sysemu/host_iommu_device.h | 3 +--
 hw/vfio/container.c                | 2 +-
 hw/vfio/iommufd.c                  | 2 +-
 hw/virtio/virtio-iommu.c           | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
index ee6c813c8b..05c7324a0d 100644
--- a/include/sysemu/host_iommu_device.h
+++ b/include/sysemu/host_iommu_device.h
@@ -87,9 +87,8 @@ struct HostIOMMUDeviceClass {
      * @hiod Host IOMMU device
      *
      * @hiod: handle to the host IOMMU device
-     * @errp: error handle
      */
-    GList* (*get_iova_ranges)(HostIOMMUDevice *hiod, Error **errp);
+    GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
 };
 
 /*
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 2ad57cd845..adeab1ac89 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1166,7 +1166,7 @@ static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
 }
 
 static GList *
-hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
+hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
 {
     VFIODevice *vdev = hiod->agent;
 
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 890d8d6a38..211e7223f1 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -644,7 +644,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
 }
 
 static GList *
-hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
+hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
 {
     VFIODevice *vdev = hiod->agent;
 
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index b708fb96fd..b8f75d2b1a 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -615,7 +615,7 @@ static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
 
     if (hiodc->get_iova_ranges) {
         int ret;
-        host_iova_ranges = hiodc->get_iova_ranges(hiod, errp);
+        host_iova_ranges = hiodc->get_iova_ranges(hiod);
         if (!host_iova_ranges) {
             return true; /* some old kernels may not support that capability */
         }
-- 
2.41.0

[PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask() callback

[Eric Auger]

This callback will be used to retrieve the page size mask supported
along a given Host IOMMU device.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/hw/vfio/vfio-container-base.h |  7 +++++++
 include/sysemu/host_iommu_device.h    |  8 ++++++++
 hw/vfio/container.c                   | 10 ++++++++++
 hw/vfio/iommufd.c                     | 11 +++++++++++
 4 files changed, 36 insertions(+)

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 45d7c40fce..62a8b60d87 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -88,6 +88,13 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
 
 GList *vfio_container_get_iova_ranges(const VFIOContainerBase *bcontainer);
 
+static inline uint64_t
+vfio_container_get_page_size_mask(const VFIOContainerBase *bcontainer)
+{
+    assert(bcontainer);
+    return bcontainer->pgsizes;
+}
+
 #define TYPE_VFIO_IOMMU "vfio-iommu"
 #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
 #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
index 05c7324a0d..c1bf74ae2c 100644
--- a/include/sysemu/host_iommu_device.h
+++ b/include/sysemu/host_iommu_device.h
@@ -89,6 +89,14 @@ struct HostIOMMUDeviceClass {
      * @hiod: handle to the host IOMMU device
      */
     GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
+    /**
+     *
+     * @get_page_size_mask: Return the page size mask supported along this
+     * @hiod Host IOMMU device
+     *
+     * @hiod: handle to the host IOMMU device
+     */
+    uint64_t (*get_page_size_mask)(HostIOMMUDevice *hiod);
 };
 
 /*
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index adeab1ac89..b5ce559a0d 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1174,6 +1174,15 @@ hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
     return vfio_container_get_iova_ranges(vdev->bcontainer);
 }
 
+static uint64_t
+hiod_legacy_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
+{
+    VFIODevice *vdev = hiod->agent;
+
+    g_assert(vdev);
+    return vfio_container_get_page_size_mask(vdev->bcontainer);
+}
+
 static void vfio_iommu_legacy_instance_init(Object *obj)
 {
     VFIOContainer *container = VFIO_IOMMU_LEGACY(obj);
@@ -1188,6 +1197,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
     hioc->realize = hiod_legacy_vfio_realize;
     hioc->get_cap = hiod_legacy_vfio_get_cap;
     hioc->get_iova_ranges = hiod_legacy_vfio_get_iova_ranges;
+    hioc->get_page_size_mask = hiod_legacy_vfio_get_page_size_mask;
 };
 
 static const TypeInfo types[] = {
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 211e7223f1..7b5f87a148 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -652,12 +652,23 @@ hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
     return vfio_container_get_iova_ranges(vdev->bcontainer);
 }
 
+static uint64_t
+hiod_iommufd_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
+{
+    VFIODevice *vdev = hiod->agent;
+
+    g_assert(vdev);
+    return vfio_container_get_page_size_mask(vdev->bcontainer);
+}
+
+
 static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
 {
     HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
 
     hiodc->realize = hiod_iommufd_vfio_realize;
     hiodc->get_iova_ranges = hiod_iommufd_vfio_get_iova_ranges;
+    hiodc->get_page_size_mask = hiod_iommufd_vfio_get_page_size_mask;
 };
 
 static const TypeInfo types[] = {
-- 
2.41.0

[PATCH 5/7] virtio-iommu : Retrieve page size mask on virtio_iommu_set_iommu_device()

[Eric Auger]

Retrieve the Host IOMMU Device page size mask when this latter is set.
This allows to get the information much sooner than when relying on
IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU MR
gets enabled. We introduce check_page_size_mask() helper whose code
is inherited from current virtio_iommu_set_page_size_mask()
implementation. This callback will be removed in a subsequent patch.

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

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index b8f75d2b1a..631589735a 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -598,9 +598,39 @@ out:
     return ret;
 }
 
+static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t new_mask,
+                                 Error **errp)
+{
+    uint64_t cur_mask = viommu->config.page_size_mask;
+
+    if ((cur_mask & new_mask) == 0) {
+        error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64
+                   " incompatible with currently supported mask 0x%"PRIx64,
+                   new_mask, cur_mask);
+        return false;
+    }
+    /*
+     * Once the granule is frozen we can't change the mask anymore. If by
+     * chance the hotplugged device supports the same granule, we can still
+     * accept it.
+     */
+    if (viommu->granule_frozen) {
+        int cur_granule = ctz64(cur_mask);
+
+        if (!(BIT_ULL(cur_granule) & new_mask)) {
+            error_setg(errp,
+                       "virtio-iommu does not support frozen granule 0x%llx",
+                       BIT_ULL(cur_granule));
+            return false;
+        }
+    }
+    return true;
+}
+
 static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
                                           HostIOMMUDevice *hiod, Error **errp)
 {
+    ERRP_GUARD();
     VirtIOIOMMU *viommu = opaque;
     HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
     struct hiod_key *new_key;
@@ -623,8 +653,26 @@ static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
                                                 hiod->aliased_devfn,
                                                 host_iova_ranges, errp);
         if (ret) {
-            g_list_free_full(host_iova_ranges, g_free);
-            return false;
+            goto error;
+        }
+    }
+    if (hiodc->get_page_size_mask) {
+        uint64_t new_mask = hiodc->get_page_size_mask(hiod);
+
+        if (check_page_size_mask(viommu, new_mask, errp)) {
+            /*
+             * The default mask depends on the "granule" property. For example,
+             * with 4k granule, it is -(4 * KiB). When an assigned device has
+             * page size restrictions due to the hardware IOMMU configuration,
+             * apply this restriction to the mask.
+             */
+            trace_virtio_iommu_update_page_size_mask(hiod->name,
+                                                     viommu->config.page_size_mask,
+                                                     new_mask);
+            viommu->config.page_size_mask &= new_mask;
+        } else {
+            error_prepend(errp, "%s: ", hiod->name);
+            goto error;
         }
     }
 
@@ -637,6 +685,9 @@ static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
     g_list_free_full(host_iova_ranges, g_free);
 
     return true;
+error:
+    g_list_free_full(host_iova_ranges, g_free);
+    return false;
 }
 
 static void
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 3cf84e04a7..599d855ff6 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name, uint64_t virt_start, uint64_t virt_end
 virtio_iommu_notify_unmap(const char *name, uint64_t virt_start, uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
 virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64" phys_start=0x%"PRIx64
 virtio_iommu_set_page_size_mask(const char *name, uint64_t old, uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
+virtio_iommu_update_page_size_mask(const char *name, uint64_t old, uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
 virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
 virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"
 virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, bool on) "Device %02x:%02x.%x switching address space (iommu enabled=%d)"
-- 
2.41.0

[PATCH 6/7] memory: remove IOMMU MR iommu_set_page_size_mask() callback

[Eric Auger]

Everything is now in place to use the Host IOMMU Device callbacks
to retrieve the page size mask usable with a given assigned device.
This new method brings the advantage to pass the info much earlier
to the virtual IOMMU and before the IOMMU MR gets enabled. So let's
remove the call to memory_region_iommu_set_page_size_mask in
vfio common.c and remove the single implementation of the IOMMU MR
callback in the virtio-iommu.c

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/exec/memory.h    | 38 ---------------------------------
 hw/vfio/common.c         |  8 -------
 hw/virtio/virtio-iommu.c | 45 ----------------------------------------
 system/memory.c          | 13 ------------
 hw/virtio/trace-events   |  1 -
 5 files changed, 105 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0903513d13..6f9c78cc14 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -504,32 +504,6 @@ struct IOMMUMemoryRegionClass {
      * @iommu: the IOMMUMemoryRegion
      */
     int (*num_indexes)(IOMMUMemoryRegion *iommu);
-
-    /**
-     * @iommu_set_page_size_mask:
-     *
-     * Restrict the page size mask that can be supported with a given IOMMU
-     * memory region. Used for example to propagate host physical IOMMU page
-     * size mask limitations to the virtual IOMMU.
-     *
-     * Optional method: if this method is not provided, then the default global
-     * page mask is used.
-     *
-     * @iommu: the IOMMUMemoryRegion
-     *
-     * @page_size_mask: a bitmask of supported page sizes. At least one bit,
-     * representing the smallest page size, must be set. Additional set bits
-     * represent supported block sizes. For example a host physical IOMMU that
-     * uses page tables with a page size of 4kB, and supports 2MB and 4GB
-     * blocks, will set mask 0x40201000. A granule of 4kB with indiscriminate
-     * block sizes is specified with mask 0xfffffffffffff000.
-     *
-     * Returns 0 on success, or a negative error. In case of failure, the error
-     * object must be created.
-     */
-     int (*iommu_set_page_size_mask)(IOMMUMemoryRegion *iommu,
-                                     uint64_t page_size_mask,
-                                     Error **errp);
 };
 
 typedef struct RamDiscardListener RamDiscardListener;
@@ -1919,18 +1893,6 @@ int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
  */
 int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr);
 
-/**
- * memory_region_iommu_set_page_size_mask: set the supported page
- * sizes for a given IOMMU memory region
- *
- * @iommu_mr: IOMMU memory region
- * @page_size_mask: supported page size mask
- * @errp: pointer to Error*, to store an error if it happens.
- */
-int memory_region_iommu_set_page_size_mask(IOMMUMemoryRegion *iommu_mr,
-                                           uint64_t page_size_mask,
-                                           Error **errp);
-
 /**
  * memory_region_name: get a memory region's name
  *
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7cdb969fd3..6d15b36e0b 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -622,14 +622,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
                             int128_get64(llend),
                             iommu_idx);
 
-        ret = memory_region_iommu_set_page_size_mask(giommu->iommu_mr,
-                                                     bcontainer->pgsizes,
-                                                     &err);
-        if (ret) {
-            g_free(giommu);
-            goto fail;
-        }
-
         ret = memory_region_register_iommu_notifier(section->mr, &giommu->n,
                                                     &err);
         if (ret) {
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 631589735a..b24e10de81 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1363,50 +1363,6 @@ static int virtio_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu_mr,
     return 0;
 }
 
-/*
- * The default mask depends on the "granule" property. For example, with
- * 4k granule, it is -(4 * KiB). When an assigned device has page size
- * restrictions due to the hardware IOMMU configuration, apply this restriction
- * to the mask.
- */
-static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
-                                           uint64_t new_mask,
-                                           Error **errp)
-{
-    IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
-    VirtIOIOMMU *s = sdev->viommu;
-    uint64_t cur_mask = s->config.page_size_mask;
-
-    trace_virtio_iommu_set_page_size_mask(mr->parent_obj.name, cur_mask,
-                                          new_mask);
-
-    if ((cur_mask & new_mask) == 0) {
-        error_setg(errp, "virtio-iommu %s reports a page size mask 0x%"PRIx64
-                   " incompatible with currently supported mask 0x%"PRIx64,
-                   mr->parent_obj.name, new_mask, cur_mask);
-        return -1;
-    }
-
-    /*
-     * Once the granule is frozen we can't change the mask anymore. If by
-     * chance the hotplugged device supports the same granule, we can still
-     * accept it.
-     */
-    if (s->granule_frozen) {
-        int cur_granule = ctz64(cur_mask);
-
-        if (!(BIT_ULL(cur_granule) & new_mask)) {
-            error_setg(errp, "virtio-iommu %s does not support frozen granule 0x%llx",
-                       mr->parent_obj.name, BIT_ULL(cur_granule));
-            return -1;
-        }
-        return 0;
-    }
-
-    s->config.page_size_mask &= new_mask;
-    return 0;
-}
-
 static void virtio_iommu_system_reset(void *opaque)
 {
     VirtIOIOMMU *s = opaque;
@@ -1731,7 +1687,6 @@ static void virtio_iommu_memory_region_class_init(ObjectClass *klass,
     imrc->translate = virtio_iommu_translate;
     imrc->replay = virtio_iommu_replay;
     imrc->notify_flag_changed = virtio_iommu_notify_flag_changed;
-    imrc->iommu_set_page_size_mask = virtio_iommu_set_page_size_mask;
 }
 
 static const TypeInfo virtio_iommu_info = {
diff --git a/system/memory.c b/system/memory.c
index 2d69521360..5e6eb459d5 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1901,19 +1901,6 @@ static int memory_region_update_iommu_notify_flags(IOMMUMemoryRegion *iommu_mr,
     return ret;
 }
 
-int memory_region_iommu_set_page_size_mask(IOMMUMemoryRegion *iommu_mr,
-                                           uint64_t page_size_mask,
-                                           Error **errp)
-{
-    IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
-    int ret = 0;
-
-    if (imrc->iommu_set_page_size_mask) {
-        ret = imrc->iommu_set_page_size_mask(iommu_mr, page_size_mask, errp);
-    }
-    return ret;
-}
-
 int memory_region_register_iommu_notifier(MemoryRegion *mr,
                                           IOMMUNotifier *n, Error **errp)
 {
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 599d855ff6..b7c04f0856 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -131,7 +131,6 @@ virtio_iommu_fill_resv_property(uint32_t devid, uint8_t subtype, uint64_t start,
 virtio_iommu_notify_map(const char *name, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start, uint32_t flags) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64" phys_start=0x%"PRIx64" flags=%d"
 virtio_iommu_notify_unmap(const char *name, uint64_t virt_start, uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
 virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64" phys_start=0x%"PRIx64
-virtio_iommu_set_page_size_mask(const char *name, uint64_t old, uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
 virtio_iommu_update_page_size_mask(const char *name, uint64_t old, uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
 virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
 virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"
-- 
2.41.0

[PATCH 7/7] virtio-iommu: Revert transient enablement of IOMMU MR in bypass mode

[Eric Auger]

In 94df5b2180d6 ("virtio-iommu: Fix 64kB host page size VFIO device
assignment"), in case of bypass mode, we transiently enabled the
IOMMU MR to allow the set_page_size_mask() to be called and pass
information about the page size mask constraint of cold plugged
VFIO devices. Now we do not use the IOMMU MR callback anymore, we
can just get rid of this hack.

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

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index b24e10de81..f87359b3e7 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1385,18 +1385,6 @@ static void virtio_iommu_freeze_granule(Notifier *notifier, void *data)
     VirtIOIOMMU *s = container_of(notifier, VirtIOIOMMU, machine_done);
     int granule;
 
-    if (likely(s->config.bypass)) {
-        /*
-         * Transient IOMMU MR enable to collect page_size_mask requirements
-         * through memory_region_iommu_set_page_size_mask() called by
-         * VFIO region_add() callback
-         */
-         s->config.bypass = false;
-         virtio_iommu_switch_address_space_all(s);
-         /* restore default */
-         s->config.bypass = true;
-         virtio_iommu_switch_address_space_all(s);
-    }
     s->granule_frozen = true;
     granule = ctz64(s->config.page_size_mask);
     trace_virtio_iommu_freeze_granule(BIT_ULL(granule));
-- 
2.41.0

Re: [PATCH 1/7] virtio-iommu: Fix error handling in virtio_iommu_set_host_iova_ranges()

[Cédric Le Goater]

On 6/26/24 10:26 AM, Eric Auger wrote:
> In case no IOMMUPciBus/IOMMUDevice are found we need to properly
> set the error handle and return.
> 
> Fixes : Coverity CID 1549006
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Fixes: cf2647a76e ("virtio-iommu: Compute host reserved regions")


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

Thanks,

C.


> ---
>   hw/virtio/virtio-iommu.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index b9a7ddcd14..b708fb96fd 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -543,10 +543,15 @@ static int virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>       int ret = -EINVAL;
>   
>       if (!sbus) {
> -        error_report("%s no sbus", __func__);
> +        error_setg(errp, "%s: no IOMMUPciBus found!", __func__);
> +        return ret;
>       }
>   
>       sdev = sbus->pbdev[devfn];
> +    if (!sdev) {
> +        error_setg(errp, "%s: no IOMMUDevice found!", __func__);
> +        return ret;
> +    }
>   
>       current_ranges = sdev->host_resv_ranges;
>   

Re: [PATCH 2/7] vfio-container-base: Introduce vfio_container_get_iova_ranges() helper

[Cédric Le Goater]

On 6/26/24 10:26 AM, Eric Auger wrote:
> Introduce vfio_container_get_iova_ranges() to retrieve the usable
> IOVA regions of the base container and use it in the Host IOMMU
> device implementations of get_iova_ranges() callback.
> 
> We also fix a UAF bug as the list was shallow copied while
> g_list_free_full() was used both on the single call site, in
> virtio_iommu_set_iommu_device() but also in
> vfio_container_instance_finalize(). Instead use g_list_copy_deep.
> 
> Fixes: cf2647a76e ("virtio-iommu: Compute host reserved regions")
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Cédric Le Goater <clg@redhat.com>


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

Thanks,

C.


> ---
>   include/hw/vfio/vfio-container-base.h |  2 ++
>   hw/vfio/container-base.c              | 15 +++++++++++++++
>   hw/vfio/container.c                   |  8 +-------
>   hw/vfio/iommufd.c                     |  8 +-------
>   4 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 419e45ee7a..45d7c40fce 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -86,6 +86,8 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>   int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                      VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
>   
> +GList *vfio_container_get_iova_ranges(const VFIOContainerBase *bcontainer);
> +
>   #define TYPE_VFIO_IOMMU "vfio-iommu"
>   #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
>   #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 50b1664f89..809b157674 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -83,6 +83,21 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                                                  errp);
>   }
>   
> +static gpointer copy_iova_range(gconstpointer src, gpointer data)
> +{
> +     Range *source = (Range *)src;
> +     Range *dest = g_new(Range, 1);
> +
> +     range_set_bounds(dest, range_lob(source), range_upb(source));
> +     return dest;
> +}
> +
> +GList *vfio_container_get_iova_ranges(const VFIOContainerBase *bcontainer)
> +{
> +    assert(bcontainer);
> +    return g_list_copy_deep(bcontainer->iova_ranges, copy_iova_range, NULL);
> +}
> +
>   static void vfio_container_instance_finalize(Object *obj)
>   {
>       VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 2e7ecdf10e..2ad57cd845 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1169,15 +1169,9 @@ static GList *
>   hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
>   {
>       VFIODevice *vdev = hiod->agent;
> -    GList *l = NULL;
>   
>       g_assert(vdev);
> -
> -    if (vdev->bcontainer) {
> -        l = g_list_copy(vdev->bcontainer->iova_ranges);
> -    }
> -
> -    return l;
> +    return vfio_container_get_iova_ranges(vdev->bcontainer);
>   }
>   
>   static void vfio_iommu_legacy_instance_init(Object *obj)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index c2f158e603..890d8d6a38 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -647,15 +647,9 @@ static GList *
>   hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
>   {
>       VFIODevice *vdev = hiod->agent;
> -    GList *l = NULL;
>   
>       g_assert(vdev);
> -
> -    if (vdev->bcontainer) {
> -        l = g_list_copy(vdev->bcontainer->iova_ranges);
> -    }
> -
> -    return l;
> +    return vfio_container_get_iova_ranges(vdev->bcontainer);
>   }
>   
>   static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)

Re: [PATCH 3/7] HostIOMMUDevice : remove Error handle from get_iova_ranges callback

[Cédric Le Goater]

On 6/26/24 10:26 AM, Eric Auger wrote:
> The error handle argument is not used anywhere. let's remove it.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


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

Thanks,

C.


> ---
>   include/sysemu/host_iommu_device.h | 3 +--
>   hw/vfio/container.c                | 2 +-
>   hw/vfio/iommufd.c                  | 2 +-
>   hw/virtio/virtio-iommu.c           | 2 +-
>   4 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> index ee6c813c8b..05c7324a0d 100644
> --- a/include/sysemu/host_iommu_device.h
> +++ b/include/sysemu/host_iommu_device.h
> @@ -87,9 +87,8 @@ struct HostIOMMUDeviceClass {
>        * @hiod Host IOMMU device
>        *
>        * @hiod: handle to the host IOMMU device
> -     * @errp: error handle
>        */
> -    GList* (*get_iova_ranges)(HostIOMMUDevice *hiod, Error **errp);
> +    GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
>   };
>   
>   /*
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 2ad57cd845..adeab1ac89 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1166,7 +1166,7 @@ static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
>   }
>   
>   static GList *
> -hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
> +hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>   {
>       VFIODevice *vdev = hiod->agent;
>   
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 890d8d6a38..211e7223f1 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -644,7 +644,7 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>   }
>   
>   static GList *
> -hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
> +hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>   {
>       VFIODevice *vdev = hiod->agent;
>   
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index b708fb96fd..b8f75d2b1a 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -615,7 +615,7 @@ static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>   
>       if (hiodc->get_iova_ranges) {
>           int ret;
> -        host_iova_ranges = hiodc->get_iova_ranges(hiod, errp);
> +        host_iova_ranges = hiodc->get_iova_ranges(hiod);
>           if (!host_iova_ranges) {
>               return true; /* some old kernels may not support that capability */
>           }

Re: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask() callback

[Cédric Le Goater]

On 6/26/24 10:26 AM, Eric Auger wrote:
> This callback will be used to retrieve the page size mask supported
> along a given Host IOMMU device.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


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

Thanks,

C.


> ---
>   include/hw/vfio/vfio-container-base.h |  7 +++++++
>   include/sysemu/host_iommu_device.h    |  8 ++++++++
>   hw/vfio/container.c                   | 10 ++++++++++
>   hw/vfio/iommufd.c                     | 11 +++++++++++
>   4 files changed, 36 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 45d7c40fce..62a8b60d87 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -88,6 +88,13 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>   
>   GList *vfio_container_get_iova_ranges(const VFIOContainerBase *bcontainer);
>   
> +static inline uint64_t
> +vfio_container_get_page_size_mask(const VFIOContainerBase *bcontainer)
> +{
> +    assert(bcontainer);
> +    return bcontainer->pgsizes;
> +}
> +
>   #define TYPE_VFIO_IOMMU "vfio-iommu"
>   #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
>   #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> index 05c7324a0d..c1bf74ae2c 100644
> --- a/include/sysemu/host_iommu_device.h
> +++ b/include/sysemu/host_iommu_device.h
> @@ -89,6 +89,14 @@ struct HostIOMMUDeviceClass {
>        * @hiod: handle to the host IOMMU device
>        */
>       GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
> +    /**
> +     *
> +     * @get_page_size_mask: Return the page size mask supported along this
> +     * @hiod Host IOMMU device
> +     *
> +     * @hiod: handle to the host IOMMU device
> +     */
> +    uint64_t (*get_page_size_mask)(HostIOMMUDevice *hiod);
>   };
>   
>   /*
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index adeab1ac89..b5ce559a0d 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1174,6 +1174,15 @@ hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>       return vfio_container_get_iova_ranges(vdev->bcontainer);
>   }
>   
> +static uint64_t
> +hiod_legacy_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
> +{
> +    VFIODevice *vdev = hiod->agent;
> +
> +    g_assert(vdev);
> +    return vfio_container_get_page_size_mask(vdev->bcontainer);
> +}
> +
>   static void vfio_iommu_legacy_instance_init(Object *obj)
>   {
>       VFIOContainer *container = VFIO_IOMMU_LEGACY(obj);
> @@ -1188,6 +1197,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>       hioc->realize = hiod_legacy_vfio_realize;
>       hioc->get_cap = hiod_legacy_vfio_get_cap;
>       hioc->get_iova_ranges = hiod_legacy_vfio_get_iova_ranges;
> +    hioc->get_page_size_mask = hiod_legacy_vfio_get_page_size_mask;
>   };
>   
>   static const TypeInfo types[] = {
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 211e7223f1..7b5f87a148 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -652,12 +652,23 @@ hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>       return vfio_container_get_iova_ranges(vdev->bcontainer);
>   }
>   
> +static uint64_t
> +hiod_iommufd_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
> +{
> +    VFIODevice *vdev = hiod->agent;
> +
> +    g_assert(vdev);
> +    return vfio_container_get_page_size_mask(vdev->bcontainer);
> +}
> +
> +
>   static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
>   {
>       HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
>   
>       hiodc->realize = hiod_iommufd_vfio_realize;
>       hiodc->get_iova_ranges = hiod_iommufd_vfio_get_iova_ranges;
> +    hiodc->get_page_size_mask = hiod_iommufd_vfio_get_page_size_mask;
>   };
>   
>   static const TypeInfo types[] = {

Re: [PATCH 6/7] memory: remove IOMMU MR iommu_set_page_size_mask() callback

[Cédric Le Goater]

On 6/26/24 10:26 AM, Eric Auger wrote:
> Everything is now in place to use the Host IOMMU Device callbacks
> to retrieve the page size mask usable with a given assigned device.
> This new method brings the advantage to pass the info much earlier
> to the virtual IOMMU and before the IOMMU MR gets enabled. So let's
> remove the call to memory_region_iommu_set_page_size_mask in
> vfio common.c and remove the single implementation of the IOMMU MR
> callback in the virtio-iommu.c
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>


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

Thanks,

C.


> ---
>   include/exec/memory.h    | 38 ---------------------------------
>   hw/vfio/common.c         |  8 -------
>   hw/virtio/virtio-iommu.c | 45 ----------------------------------------
>   system/memory.c          | 13 ------------
>   hw/virtio/trace-events   |  1 -
>   5 files changed, 105 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 0903513d13..6f9c78cc14 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -504,32 +504,6 @@ struct IOMMUMemoryRegionClass {
>        * @iommu: the IOMMUMemoryRegion
>        */
>       int (*num_indexes)(IOMMUMemoryRegion *iommu);
> -
> -    /**
> -     * @iommu_set_page_size_mask:
> -     *
> -     * Restrict the page size mask that can be supported with a given IOMMU
> -     * memory region. Used for example to propagate host physical IOMMU page
> -     * size mask limitations to the virtual IOMMU.
> -     *
> -     * Optional method: if this method is not provided, then the default global
> -     * page mask is used.
> -     *
> -     * @iommu: the IOMMUMemoryRegion
> -     *
> -     * @page_size_mask: a bitmask of supported page sizes. At least one bit,
> -     * representing the smallest page size, must be set. Additional set bits
> -     * represent supported block sizes. For example a host physical IOMMU that
> -     * uses page tables with a page size of 4kB, and supports 2MB and 4GB
> -     * blocks, will set mask 0x40201000. A granule of 4kB with indiscriminate
> -     * block sizes is specified with mask 0xfffffffffffff000.
> -     *
> -     * Returns 0 on success, or a negative error. In case of failure, the error
> -     * object must be created.
> -     */
> -     int (*iommu_set_page_size_mask)(IOMMUMemoryRegion *iommu,
> -                                     uint64_t page_size_mask,
> -                                     Error **errp);
>   };
>   
>   typedef struct RamDiscardListener RamDiscardListener;
> @@ -1919,18 +1893,6 @@ int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
>    */
>   int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr);
>   
> -/**
> - * memory_region_iommu_set_page_size_mask: set the supported page
> - * sizes for a given IOMMU memory region
> - *
> - * @iommu_mr: IOMMU memory region
> - * @page_size_mask: supported page size mask
> - * @errp: pointer to Error*, to store an error if it happens.
> - */
> -int memory_region_iommu_set_page_size_mask(IOMMUMemoryRegion *iommu_mr,
> -                                           uint64_t page_size_mask,
> -                                           Error **errp);
> -
>   /**
>    * memory_region_name: get a memory region's name
>    *
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 7cdb969fd3..6d15b36e0b 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -622,14 +622,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
>                               int128_get64(llend),
>                               iommu_idx);
>   
> -        ret = memory_region_iommu_set_page_size_mask(giommu->iommu_mr,
> -                                                     bcontainer->pgsizes,
> -                                                     &err);
> -        if (ret) {
> -            g_free(giommu);
> -            goto fail;
> -        }
> -
>           ret = memory_region_register_iommu_notifier(section->mr, &giommu->n,
>                                                       &err);
>           if (ret) {
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 631589735a..b24e10de81 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -1363,50 +1363,6 @@ static int virtio_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu_mr,
>       return 0;
>   }
>   
> -/*
> - * The default mask depends on the "granule" property. For example, with
> - * 4k granule, it is -(4 * KiB). When an assigned device has page size
> - * restrictions due to the hardware IOMMU configuration, apply this restriction
> - * to the mask.
> - */
> -static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
> -                                           uint64_t new_mask,
> -                                           Error **errp)
> -{
> -    IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
> -    VirtIOIOMMU *s = sdev->viommu;
> -    uint64_t cur_mask = s->config.page_size_mask;
> -
> -    trace_virtio_iommu_set_page_size_mask(mr->parent_obj.name, cur_mask,
> -                                          new_mask);
> -
> -    if ((cur_mask & new_mask) == 0) {
> -        error_setg(errp, "virtio-iommu %s reports a page size mask 0x%"PRIx64
> -                   " incompatible with currently supported mask 0x%"PRIx64,
> -                   mr->parent_obj.name, new_mask, cur_mask);
> -        return -1;
> -    }
> -
> -    /*
> -     * Once the granule is frozen we can't change the mask anymore. If by
> -     * chance the hotplugged device supports the same granule, we can still
> -     * accept it.
> -     */
> -    if (s->granule_frozen) {
> -        int cur_granule = ctz64(cur_mask);
> -
> -        if (!(BIT_ULL(cur_granule) & new_mask)) {
> -            error_setg(errp, "virtio-iommu %s does not support frozen granule 0x%llx",
> -                       mr->parent_obj.name, BIT_ULL(cur_granule));
> -            return -1;
> -        }
> -        return 0;
> -    }
> -
> -    s->config.page_size_mask &= new_mask;
> -    return 0;
> -}
> -
>   static void virtio_iommu_system_reset(void *opaque)
>   {
>       VirtIOIOMMU *s = opaque;
> @@ -1731,7 +1687,6 @@ static void virtio_iommu_memory_region_class_init(ObjectClass *klass,
>       imrc->translate = virtio_iommu_translate;
>       imrc->replay = virtio_iommu_replay;
>       imrc->notify_flag_changed = virtio_iommu_notify_flag_changed;
> -    imrc->iommu_set_page_size_mask = virtio_iommu_set_page_size_mask;
>   }
>   
>   static const TypeInfo virtio_iommu_info = {
> diff --git a/system/memory.c b/system/memory.c
> index 2d69521360..5e6eb459d5 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1901,19 +1901,6 @@ static int memory_region_update_iommu_notify_flags(IOMMUMemoryRegion *iommu_mr,
>       return ret;
>   }
>   
> -int memory_region_iommu_set_page_size_mask(IOMMUMemoryRegion *iommu_mr,
> -                                           uint64_t page_size_mask,
> -                                           Error **errp)
> -{
> -    IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
> -    int ret = 0;
> -
> -    if (imrc->iommu_set_page_size_mask) {
> -        ret = imrc->iommu_set_page_size_mask(iommu_mr, page_size_mask, errp);
> -    }
> -    return ret;
> -}
> -
>   int memory_region_register_iommu_notifier(MemoryRegion *mr,
>                                             IOMMUNotifier *n, Error **errp)
>   {
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 599d855ff6..b7c04f0856 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -131,7 +131,6 @@ virtio_iommu_fill_resv_property(uint32_t devid, uint8_t subtype, uint64_t start,
>   virtio_iommu_notify_map(const char *name, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start, uint32_t flags) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64" phys_start=0x%"PRIx64" flags=%d"
>   virtio_iommu_notify_unmap(const char *name, uint64_t virt_start, uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
>   virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64" phys_start=0x%"PRIx64
> -virtio_iommu_set_page_size_mask(const char *name, uint64_t old, uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
>   virtio_iommu_update_page_size_mask(const char *name, uint64_t old, uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
>   virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
>   virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"

Re: [PATCH 7/7] virtio-iommu: Revert transient enablement of IOMMU MR in bypass mode

[Cédric Le Goater]

On 6/26/24 10:26 AM, Eric Auger wrote:
> In 94df5b2180d6 ("virtio-iommu: Fix 64kB host page size VFIO device
> assignment"), in case of bypass mode, we transiently enabled the
> IOMMU MR to allow the set_page_size_mask() to be called and pass
> information about the page size mask constraint of cold plugged
> VFIO devices. Now we do not use the IOMMU MR callback anymore, we
> can just get rid of this hack.
> 
> 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 | 12 ------------
>   1 file changed, 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index b24e10de81..f87359b3e7 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -1385,18 +1385,6 @@ static void virtio_iommu_freeze_granule(Notifier *notifier, void *data)
>       VirtIOIOMMU *s = container_of(notifier, VirtIOIOMMU, machine_done);
>       int granule;
>   
> -    if (likely(s->config.bypass)) {
> -        /*
> -         * Transient IOMMU MR enable to collect page_size_mask requirements
> -         * through memory_region_iommu_set_page_size_mask() called by
> -         * VFIO region_add() callback
> -         */
> -         s->config.bypass = false;
> -         virtio_iommu_switch_address_space_all(s);
> -         /* restore default */
> -         s->config.bypass = true;
> -         virtio_iommu_switch_address_space_all(s);
> -    }
>       s->granule_frozen = true;
>       granule = ctz64(s->config.page_size_mask);
>       trace_virtio_iommu_freeze_granule(BIT_ULL(granule));

RE: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask() callback

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask()
>callback
>
>This callback will be used to retrieve the page size mask supported
>along a given Host IOMMU device.
>
>Signed-off-by: Eric Auger <eric.auger@redhat.com>
>---
> include/hw/vfio/vfio-container-base.h |  7 +++++++
> include/sysemu/host_iommu_device.h    |  8 ++++++++
> hw/vfio/container.c                   | 10 ++++++++++
> hw/vfio/iommufd.c                     | 11 +++++++++++
> 4 files changed, 36 insertions(+)
>
>diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>index 45d7c40fce..62a8b60d87 100644
>--- a/include/hw/vfio/vfio-container-base.h
>+++ b/include/hw/vfio/vfio-container-base.h
>@@ -88,6 +88,13 @@ int vfio_container_query_dirty_bitmap(const
>VFIOContainerBase *bcontainer,
>
> GList *vfio_container_get_iova_ranges(const VFIOContainerBase
>*bcontainer);
>
>+static inline uint64_t
>+vfio_container_get_page_size_mask(const VFIOContainerBase *bcontainer)
>+{
>+    assert(bcontainer);
>+    return bcontainer->pgsizes;
>+}
>+
> #define TYPE_VFIO_IOMMU "vfio-iommu"
> #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
> #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
>diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>index 05c7324a0d..c1bf74ae2c 100644
>--- a/include/sysemu/host_iommu_device.h
>+++ b/include/sysemu/host_iommu_device.h
>@@ -89,6 +89,14 @@ struct HostIOMMUDeviceClass {
>      * @hiod: handle to the host IOMMU device
>      */
>     GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
>+    /**
>+     *
>+     * @get_page_size_mask: Return the page size mask supported along
>this
>+     * @hiod Host IOMMU device
>+     *
>+     * @hiod: handle to the host IOMMU device
>+     */
>+    uint64_t (*get_page_size_mask)(HostIOMMUDevice *hiod);

Not sure if it's simpler to utilize existing .get_cap() to get pgsizes.

Thanks
Zhenzhong

> };
>
> /*
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index adeab1ac89..b5ce559a0d 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -1174,6 +1174,15 @@
>hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>     return vfio_container_get_iova_ranges(vdev->bcontainer);
> }
>
>+static uint64_t
>+hiod_legacy_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
>+{
>+    VFIODevice *vdev = hiod->agent;
>+
>+    g_assert(vdev);
>+    return vfio_container_get_page_size_mask(vdev->bcontainer);
>+}
>+
> static void vfio_iommu_legacy_instance_init(Object *obj)
> {
>     VFIOContainer *container = VFIO_IOMMU_LEGACY(obj);
>@@ -1188,6 +1197,7 @@ static void
>hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>     hioc->realize = hiod_legacy_vfio_realize;
>     hioc->get_cap = hiod_legacy_vfio_get_cap;
>     hioc->get_iova_ranges = hiod_legacy_vfio_get_iova_ranges;
>+    hioc->get_page_size_mask = hiod_legacy_vfio_get_page_size_mask;
> };
>
> static const TypeInfo types[] = {
>diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>index 211e7223f1..7b5f87a148 100644
>--- a/hw/vfio/iommufd.c
>+++ b/hw/vfio/iommufd.c
>@@ -652,12 +652,23 @@
>hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>     return vfio_container_get_iova_ranges(vdev->bcontainer);
> }
>
>+static uint64_t
>+hiod_iommufd_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
>+{
>+    VFIODevice *vdev = hiod->agent;
>+
>+    g_assert(vdev);
>+    return vfio_container_get_page_size_mask(vdev->bcontainer);
>+}
>+
>+
> static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
> {
>     HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
>
>     hiodc->realize = hiod_iommufd_vfio_realize;
>     hiodc->get_iova_ranges = hiod_iommufd_vfio_get_iova_ranges;
>+    hiodc->get_page_size_mask = hiod_iommufd_vfio_get_page_size_mask;
> };
>
> static const TypeInfo types[] = {
>--
>2.41.0

Re: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask() callback

[Eric Auger]

Hi Zhenzhong,

On 6/27/24 05:06, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask()
>> callback
>>
>> This callback will be used to retrieve the page size mask supported
>> along a given Host IOMMU device.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> include/hw/vfio/vfio-container-base.h |  7 +++++++
>> include/sysemu/host_iommu_device.h    |  8 ++++++++
>> hw/vfio/container.c                   | 10 ++++++++++
>> hw/vfio/iommufd.c                     | 11 +++++++++++
>> 4 files changed, 36 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>> container-base.h
>> index 45d7c40fce..62a8b60d87 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -88,6 +88,13 @@ int vfio_container_query_dirty_bitmap(const
>> VFIOContainerBase *bcontainer,
>>
>> GList *vfio_container_get_iova_ranges(const VFIOContainerBase
>> *bcontainer);
>>
>> +static inline uint64_t
>> +vfio_container_get_page_size_mask(const VFIOContainerBase *bcontainer)
>> +{
>> +    assert(bcontainer);
>> +    return bcontainer->pgsizes;
>> +}
>> +
>> #define TYPE_VFIO_IOMMU "vfio-iommu"
>> #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
>> #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index 05c7324a0d..c1bf74ae2c 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -89,6 +89,14 @@ struct HostIOMMUDeviceClass {
>>      * @hiod: handle to the host IOMMU device
>>      */
>>     GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
>> +    /**
>> +     *
>> +     * @get_page_size_mask: Return the page size mask supported along
>> this
>> +     * @hiod Host IOMMU device
>> +     *
>> +     * @hiod: handle to the host IOMMU device
>> +     */
>> +    uint64_t (*get_page_size_mask)(HostIOMMUDevice *hiod);
> Not sure if it's simpler to utilize existing .get_cap() to get pgsizes.
I chose to introduce a new callback because the page_mask can be U64_MAX
and get_cap is likely to return a negative value. So we could not
distinguish between an error and a full mask.

Thanks

Eric

>
> Thanks
> Zhenzhong
>
>> };
>>
>> /*
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index adeab1ac89..b5ce559a0d 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1174,6 +1174,15 @@
>> hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>>     return vfio_container_get_iova_ranges(vdev->bcontainer);
>> }
>>
>> +static uint64_t
>> +hiod_legacy_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
>> +{
>> +    VFIODevice *vdev = hiod->agent;
>> +
>> +    g_assert(vdev);
>> +    return vfio_container_get_page_size_mask(vdev->bcontainer);
>> +}
>> +
>> static void vfio_iommu_legacy_instance_init(Object *obj)
>> {
>>     VFIOContainer *container = VFIO_IOMMU_LEGACY(obj);
>> @@ -1188,6 +1197,7 @@ static void
>> hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>     hioc->realize = hiod_legacy_vfio_realize;
>>     hioc->get_cap = hiod_legacy_vfio_get_cap;
>>     hioc->get_iova_ranges = hiod_legacy_vfio_get_iova_ranges;
>> +    hioc->get_page_size_mask = hiod_legacy_vfio_get_page_size_mask;
>> };
>>
>> static const TypeInfo types[] = {
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 211e7223f1..7b5f87a148 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -652,12 +652,23 @@
>> hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
>>     return vfio_container_get_iova_ranges(vdev->bcontainer);
>> }
>>
>> +static uint64_t
>> +hiod_iommufd_vfio_get_page_size_mask(HostIOMMUDevice *hiod)
>> +{
>> +    VFIODevice *vdev = hiod->agent;
>> +
>> +    g_assert(vdev);
>> +    return vfio_container_get_page_size_mask(vdev->bcontainer);
>> +}
>> +
>> +
>> static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
>> {
>>     HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
>>
>>     hiodc->realize = hiod_iommufd_vfio_realize;
>>     hiodc->get_iova_ranges = hiod_iommufd_vfio_get_iova_ranges;
>> +    hiodc->get_page_size_mask = hiod_iommufd_vfio_get_page_size_mask;
>> };
>>
>> static const TypeInfo types[] = {
>> --
>> 2.41.0

RE: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask() callback

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH 4/7] HostIOMMUDevice: Introduce
>get_page_size_mask() callback
>
>Hi Zhenzhong,
>
>On 6/27/24 05:06, Duan, Zhenzhong wrote:
>> Hi Eric,
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> Subject: [PATCH 4/7] HostIOMMUDevice: Introduce get_page_size_mask()
>>> callback
>>>
>>> This callback will be used to retrieve the page size mask supported
>>> along a given Host IOMMU device.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>> include/hw/vfio/vfio-container-base.h |  7 +++++++
>>> include/sysemu/host_iommu_device.h    |  8 ++++++++
>>> hw/vfio/container.c                   | 10 ++++++++++
>>> hw/vfio/iommufd.c                     | 11 +++++++++++
>>> 4 files changed, 36 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>>> container-base.h
>>> index 45d7c40fce..62a8b60d87 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -88,6 +88,13 @@ int vfio_container_query_dirty_bitmap(const
>>> VFIOContainerBase *bcontainer,
>>>
>>> GList *vfio_container_get_iova_ranges(const VFIOContainerBase
>>> *bcontainer);
>>>
>>> +static inline uint64_t
>>> +vfio_container_get_page_size_mask(const VFIOContainerBase
>*bcontainer)
>>> +{
>>> +    assert(bcontainer);
>>> +    return bcontainer->pgsizes;
>>> +}
>>> +
>>> #define TYPE_VFIO_IOMMU "vfio-iommu"
>>> #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
>>> #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
>>> diff --git a/include/sysemu/host_iommu_device.h
>>> b/include/sysemu/host_iommu_device.h
>>> index 05c7324a0d..c1bf74ae2c 100644
>>> --- a/include/sysemu/host_iommu_device.h
>>> +++ b/include/sysemu/host_iommu_device.h
>>> @@ -89,6 +89,14 @@ struct HostIOMMUDeviceClass {
>>>      * @hiod: handle to the host IOMMU device
>>>      */
>>>     GList* (*get_iova_ranges)(HostIOMMUDevice *hiod);
>>> +    /**
>>> +     *
>>> +     * @get_page_size_mask: Return the page size mask supported along
>>> this
>>> +     * @hiod Host IOMMU device
>>> +     *
>>> +     * @hiod: handle to the host IOMMU device
>>> +     */
>>> +    uint64_t (*get_page_size_mask)(HostIOMMUDevice *hiod);
>> Not sure if it's simpler to utilize existing .get_cap() to get pgsizes.
>I chose to introduce a new callback because the page_mask can be
>U64_MAX
>and get_cap is likely to return a negative value. So we could not
>distinguish between an error and a full mask.

I see, you are right.

Thanks
Zhenzhong

RE: [PATCH 5/7] virtio-iommu : Retrieve page size mask on virtio_iommu_set_iommu_device()

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: [PATCH 5/7] virtio-iommu : Retrieve page size mask on
>virtio_iommu_set_iommu_device()
>
>Retrieve the Host IOMMU Device page size mask when this latter is set.
>This allows to get the information much sooner than when relying on
>IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU
>MR
>gets enabled. We introduce check_page_size_mask() helper whose code
>is inherited from current virtio_iommu_set_page_size_mask()
>implementation. This callback will be removed in a subsequent patch.
>
>Signed-off-by: Eric Auger <eric.auger@redhat.com>
>---
> hw/virtio/virtio-iommu.c | 55
>++++++++++++++++++++++++++++++++++++++--
> hw/virtio/trace-events   |  1 +
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
>diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>index b8f75d2b1a..631589735a 100644
>--- a/hw/virtio/virtio-iommu.c
>+++ b/hw/virtio/virtio-iommu.c
>@@ -598,9 +598,39 @@ out:
>     return ret;
> }
>
>+static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>new_mask,
>+                                 Error **errp)
>+{
>+    uint64_t cur_mask = viommu->config.page_size_mask;
>+
>+    if ((cur_mask & new_mask) == 0) {
>+        error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64
>+                   " incompatible with currently supported mask 0x%"PRIx64,
>+                   new_mask, cur_mask);
>+        return false;
>+    }
>+    /*
>+     * Once the granule is frozen we can't change the mask anymore. If by
>+     * chance the hotplugged device supports the same granule, we can still
>+     * accept it.
>+     */
>+    if (viommu->granule_frozen) {
>+        int cur_granule = ctz64(cur_mask);
>+
>+        if (!(BIT_ULL(cur_granule) & new_mask)) {
>+            error_setg(errp,
>+                       "virtio-iommu does not support frozen granule 0x%llx",
>+                       BIT_ULL(cur_granule));
>+            return false;
>+        }
>+    }
>+    return true;
>+}
>+
> static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque,
>int devfn,
>                                           HostIOMMUDevice *hiod, Error **errp)
> {
>+    ERRP_GUARD();
>     VirtIOIOMMU *viommu = opaque;
>     HostIOMMUDeviceClass *hiodc =
>HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>     struct hiod_key *new_key;
>@@ -623,8 +653,26 @@ static bool
>virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>                                                 hiod->aliased_devfn,
>                                                 host_iova_ranges, errp);
>         if (ret) {
>-            g_list_free_full(host_iova_ranges, g_free);
>-            return false;
>+            goto error;
>+        }
>+    }
>+    if (hiodc->get_page_size_mask) {
>+        uint64_t new_mask = hiodc->get_page_size_mask(hiod);
>+
>+        if (check_page_size_mask(viommu, new_mask, errp)) {
>+            /*
>+             * The default mask depends on the "granule" property. For example,
>+             * with 4k granule, it is -(4 * KiB). When an assigned device has
>+             * page size restrictions due to the hardware IOMMU configuration,
>+             * apply this restriction to the mask.
>+             */
>+            trace_virtio_iommu_update_page_size_mask(hiod->name,
>+                                                     viommu->config.page_size_mask,
>+                                                     new_mask);
>+            viommu->config.page_size_mask &= new_mask;

This is a bit different from original logic, it may update page_size_mask after frozen.
Will that make issue?

Except this question, for all other patches,

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

Thanks
Zhenzhong

>+        } else {
>+            error_prepend(errp, "%s: ", hiod->name);
>+            goto error;
>         }
>     }
>
>@@ -637,6 +685,9 @@ static bool
>virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>     g_list_free_full(host_iova_ranges, g_free);
>
>     return true;
>+error:
>+    g_list_free_full(host_iova_ranges, g_free);
>+    return false;
> }
>
> static void
>diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>index 3cf84e04a7..599d855ff6 100644
>--- a/hw/virtio/trace-events
>+++ b/hw/virtio/trace-events
>@@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name,
>uint64_t virt_start, uint64_t virt_end
> virtio_iommu_notify_unmap(const char *name, uint64_t virt_start,
>uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
> virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t
>virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64"
>virt_end=0x%"PRIx64" phys_start=0x%"PRIx64
> virtio_iommu_set_page_size_mask(const char *name, uint64_t old,
>uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
>+virtio_iommu_update_page_size_mask(const char *name, uint64_t old,
>uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64"
>new_mask=0x%"PRIx64
> virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
> virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"
> virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn,
>bool on) "Device %02x:%02x.%x switching address space (iommu
>enabled=%d)"
>--
>2.41.0

Re: [PATCH 5/7] virtio-iommu : Retrieve page size mask on virtio_iommu_set_iommu_device()

[Eric Auger]

Hi Zhenhong,

On 6/27/24 13:32, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: [PATCH 5/7] virtio-iommu : Retrieve page size mask on
>> virtio_iommu_set_iommu_device()
>>
>> Retrieve the Host IOMMU Device page size mask when this latter is set.
>> This allows to get the information much sooner than when relying on
>> IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU
>> MR
>> gets enabled. We introduce check_page_size_mask() helper whose code
>> is inherited from current virtio_iommu_set_page_size_mask()
>> implementation. This callback will be removed in a subsequent patch.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/virtio/virtio-iommu.c | 55
>> ++++++++++++++++++++++++++++++++++++++--
>> hw/virtio/trace-events   |  1 +
>> 2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index b8f75d2b1a..631589735a 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -598,9 +598,39 @@ out:
>>     return ret;
>> }
>>
>> +static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>> new_mask,
>> +                                 Error **errp)
>> +{
>> +    uint64_t cur_mask = viommu->config.page_size_mask;
>> +
>> +    if ((cur_mask & new_mask) == 0) {
>> +        error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64
>> +                   " incompatible with currently supported mask 0x%"PRIx64,
>> +                   new_mask, cur_mask);
>> +        return false;
>> +    }
>> +    /*
>> +     * Once the granule is frozen we can't change the mask anymore. If by
>> +     * chance the hotplugged device supports the same granule, we can still
>> +     * accept it.
>> +     */
>> +    if (viommu->granule_frozen) {
>> +        int cur_granule = ctz64(cur_mask);
>> +
>> +        if (!(BIT_ULL(cur_granule) & new_mask)) {
>> +            error_setg(errp,
>> +                       "virtio-iommu does not support frozen granule 0x%llx",
>> +                       BIT_ULL(cur_granule));
>> +            return false;
>> +        }
>> +    }
>> +    return true;
>> +}
>> +
>> static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque,
>> int devfn,
>>                                           HostIOMMUDevice *hiod, Error **errp)
>> {
>> +    ERRP_GUARD();
>>     VirtIOIOMMU *viommu = opaque;
>>     HostIOMMUDeviceClass *hiodc =
>> HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>>     struct hiod_key *new_key;
>> @@ -623,8 +653,26 @@ static bool
>> virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>                                                 hiod->aliased_devfn,
>>                                                 host_iova_ranges, errp);
>>         if (ret) {
>> -            g_list_free_full(host_iova_ranges, g_free);
>> -            return false;
>> +            goto error;
>> +        }
>> +    }
>> +    if (hiodc->get_page_size_mask) {
>> +        uint64_t new_mask = hiodc->get_page_size_mask(hiod);
>> +
>> +        if (check_page_size_mask(viommu, new_mask, errp)) {
>> +            /*
>> +             * The default mask depends on the "granule" property. For example,
>> +             * with 4k granule, it is -(4 * KiB). When an assigned device has
>> +             * page size restrictions due to the hardware IOMMU configuration,
>> +             * apply this restriction to the mask.
>> +             */
>> +            trace_virtio_iommu_update_page_size_mask(hiod->name,
>> +                                                     viommu->config.page_size_mask,
>> +                                                     new_mask);
>> +            viommu->config.page_size_mask &= new_mask;
> This is a bit different from original logic, it may update page_size_mask after frozen.
> Will that make issue?
You're fully right. I need to replace

viommu->config.page_size_mask &= new_mask;
by
if (s->granule_frozen) {
    viommu->config.page_size_mask &= new_mask;
}    

> Except this question, for all other patches,
>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Thanks!

Eric
>
> Thanks
> Zhenzhong
>
>> +        } else {
>> +            error_prepend(errp, "%s: ", hiod->name);
>> +            goto error;
>>         }
>>     }
>>
>> @@ -637,6 +685,9 @@ static bool
>> virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>     g_list_free_full(host_iova_ranges, g_free);
>>
>>     return true;
>> +error:
>> +    g_list_free_full(host_iova_ranges, g_free);
>> +    return false;
>> }
>>
>> static void
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index 3cf84e04a7..599d855ff6 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name,
>> uint64_t virt_start, uint64_t virt_end
>> virtio_iommu_notify_unmap(const char *name, uint64_t virt_start,
>> uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
>> virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t
>> virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64"
>> virt_end=0x%"PRIx64" phys_start=0x%"PRIx64
>> virtio_iommu_set_page_size_mask(const char *name, uint64_t old,
>> uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
>> +virtio_iommu_update_page_size_mask(const char *name, uint64_t old,
>> uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64"
>> new_mask=0x%"PRIx64
>> virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
>> virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"
>> virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn,
>> bool on) "Device %02x:%02x.%x switching address space (iommu
>> enabled=%d)"
>> --
>> 2.41.0

Re: [PATCH 0/7] VIRTIO-IOMMU/HostIOMMUDevice: Fixes and page size mask rework

[Michael S. Tsirkin]

On Wed, Jun 26, 2024 at 10:26:45AM +0200, Eric Auger wrote:
> The 2 first patches are fixes of
> cf2647a76e ("virtio-iommu: Compute host reserved regions")
> They can be taken separately of the rest.
> 
> Then the series uses the HostIOMMUDevice interface to fetch
> information about the page size mask supported along the assigned
> device and propagate it to the virtio-iommu. This is a similar
> work as what was done in
> 
> VIRTIO-IOMMU/VFIO: Fix host iommu geometry handling series
> 
> but this time for the page size mask. Using this new
> infrastructure allows to handle page size mask conflicts
> earlier on device hotplug as opposed to current QEMU
> abort:
> 
> qemu-system-aarch64: virtio-iommu virtio-iommu-memory-region-8-0
> does not support frozen granule 0x10000
> qemu: hardware error: vfio: DMA mapping failed, unable to continue
> 
> With this series the hotplug nicely fails.
> 
> Also this allows to remove hacks consisting in transiently enabling
> IOMMU MRs to collect coldplugged device page size mask before machine
> init. Those hacks were introduced by
> 
> 94df5b2180d6 ("virtio-iommu: Fix 64kB host page size VFIO device
> assignment")
> 
> The series can be found at:
> https://github.com/eauger/qemu/tree/virtio-iommu-psmask-rework-v1


virtio things:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Feel free to merge.

> 
> Eric Auger (7):
>   virtio-iommu: Fix error handling in
>     virtio_iommu_set_host_iova_ranges()
>   vfio-container-base: Introduce vfio_container_get_iova_ranges() helper
>   HostIOMMUDevice : remove Error handle from get_iova_ranges callback
>   HostIOMMUDevice: Introduce get_page_size_mask() callback
>   virtio-iommu : Retrieve page size mask on
>     virtio_iommu_set_iommu_device()
>   memory: remove IOMMU MR iommu_set_page_size_mask() callback
>   virtio-iommu: Revert transient enablement of IOMMU MR in bypass mode
> 
>  include/exec/memory.h                 |  38 --------
>  include/hw/vfio/vfio-container-base.h |   9 ++
>  include/sysemu/host_iommu_device.h    |  11 ++-
>  hw/vfio/common.c                      |   8 --
>  hw/vfio/container-base.c              |  15 ++++
>  hw/vfio/container.c                   |  16 ++--
>  hw/vfio/iommufd.c                     |  21 +++--
>  hw/virtio/virtio-iommu.c              | 121 +++++++++++++-------------
>  system/memory.c                       |  13 ---
>  hw/virtio/trace-events                |   2 +-
>  10 files changed, 117 insertions(+), 137 deletions(-)
> 
> -- 
> 2.41.0