[PATCH v4 0/2] Allow memory_region_register_iommu_notifier() to fail

[PATCH v4 0/2] Allow memory_region_register_iommu_notifier() to fail

[Eric Auger]

This series allows the memory_region_register_iommu_notifier()
to fail. As of now, when a MAP notifier is attempted to be
registered along with SMMUv3 or AMD IOMMU, we exit in the IOMMU
MR notify_flag_changed() callback.

In case of VFIO assigned device hotplug, this could be handled
more nicely directly within the VFIO code, simply rejecting
the hotplug without exiting. This is what the series achieves
by handling the memory_region_register_iommu_notifier() returned
value and Error object.

To propagate errors collected during vfio_listener_region_add()
we now store the error handle inside the VFIO container instead
of a returned value.

The message now is:
(QEMU) device_add id=hot0 driver=vfio-pci host=0000:89:00.0 bus=pcie.1
{"error": {"class": "GenericError", "desc": "vfio 0000:89:00.0: failed
to setup container for group 2: memory listener initialization failed:
Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
notifier which is not currently supported"}}

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/v4.1.0_register_iommu_notifier_fail_v4

History:

v3 -> v4:
- added Peter's R-b on 2d patch
- 1st patch: restore hw_error, remove useless ret assignment, improve
  DMA host window error message, remove local mr variable

v2 -> v3:
- also pass an Error handle (suggested by Peter)

v1 -> v2:
- Intel IOMMU now handles the problem differently with machine init done
  notifier and machine hotplug allowed hook.
- use assert(!ret)
- message rewording in SMMUv3

Follow-up of "VFIO/SMMUv3: Fail on VFIO/HW nested paging detection"
https://patchew.org/QEMU/20190829090141.21821-1-eric.auger@redhat.com/


Eric Auger (2):
  vfio: Turn the container error into an Error handle
  memory: allow memory_region_register_iommu_notifier() to fail

 exec.c                        | 10 +++++--
 hw/arm/smmuv3.c               | 18 ++++++------
 hw/i386/amd_iommu.c           | 17 +++++++-----
 hw/i386/intel_iommu.c         |  8 ++++--
 hw/ppc/spapr_iommu.c          |  8 ++++--
 hw/vfio/common.c              | 52 +++++++++++++++++++++++------------
 hw/vfio/spapr.c               |  4 ++-
 hw/virtio/vhost.c             |  9 ++++--
 include/exec/memory.h         | 21 ++++++++++----
 include/hw/vfio/vfio-common.h |  2 +-
 memory.c                      | 31 +++++++++++++--------
 11 files changed, 120 insertions(+), 60 deletions(-)

-- 
2.20.1

[PATCH v4 1/2] vfio: Turn the container error into an Error handle

[Eric Auger]

The container error integer field is currently used to store
the first error potentially encountered during any
vfio_listener_region_add() call. However this fails to propagate
detailed error messages up to the vfio_connect_container caller.
Instead of using an integer, let's use an Error handle.

Messages are slightly reworded to accomodate the propagation.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- remove ret useless assignments and restore hw_error()
- remove mr local variable
- trace [start, end] instead of [start, size] and improve
  the message for overalp with existing DMA host window
---
 hw/vfio/common.c              | 43 +++++++++++++++++++++++------------
 hw/vfio/spapr.c               |  4 +++-
 include/hw/vfio/vfio-common.h |  2 +-
 3 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3e03c495d8..cebbb1c28a 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -509,6 +509,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
     int ret;
     VFIOHostDMAWindow *hostwin;
     bool hostwin_found;
+    Error *err = NULL;
 
     if (vfio_listener_skipped_section(section)) {
         trace_vfio_listener_region_add_skip(
@@ -543,13 +544,20 @@ static void vfio_listener_region_add(MemoryListener *listener,
                                hostwin->max_iova - hostwin->min_iova + 1,
                                section->offset_within_address_space,
                                int128_get64(section->size))) {
-                ret = -1;
+                error_setg(&err,
+                    "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
+                    "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
+                    section->offset_within_address_space,
+                    section->offset_within_address_space +
+                        int128_get64(section->size) - 1,
+                    hostwin->min_iova, hostwin->max_iova);
                 goto fail;
             }
         }
 
         ret = vfio_spapr_create_window(container, section, &pgsize);
         if (ret) {
+            error_setg_errno(&err, -ret, "Failed to create SPAPR window");
             goto fail;
         }
 
@@ -594,10 +602,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
     }
 
     if (!hostwin_found) {
-        error_report("vfio: IOMMU container %p can't map guest IOVA region"
-                     " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
-                     container, iova, end);
-        ret = -EFAULT;
+        error_setg(&err, "Container %p can't map guest IOVA region"
+                   " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, end);
         goto fail;
     }
 
@@ -664,11 +670,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
     ret = vfio_dma_map(container, iova, int128_get64(llsize),
                        vaddr, section->readonly);
     if (ret) {
-        error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
-                     "0x%"HWADDR_PRIx", %p) = %d (%m)",
-                     container, iova, int128_get64(llsize), vaddr, ret);
+        error_setg(&err, "vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
+                   "0x%"HWADDR_PRIx", %p) = %d (%m)",
+                   container, iova, int128_get64(llsize), vaddr, ret);
         if (memory_region_is_ram_device(section->mr)) {
             /* Allow unexpected mappings not to be fatal for RAM devices */
+            error_report_err(err);
             return;
         }
         goto fail;
@@ -688,9 +695,14 @@ fail:
      */
     if (!container->initialized) {
         if (!container->error) {
-            container->error = ret;
+            error_propagate_prepend(&container->error, err,
+                                    "Region %s: ",
+                                    memory_region_name(section->mr));
+        } else {
+            error_free(err);
         }
     } else {
+        error_report_err(err);
         hw_error("vfio: DMA mapping failed, unable to continue");
     }
 }
@@ -1251,6 +1263,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     container = g_malloc0(sizeof(*container));
     container->space = space;
     container->fd = fd;
+    container->error = NULL;
     QLIST_INIT(&container->giommu_list);
     QLIST_INIT(&container->hostwin_list);
 
@@ -1308,9 +1321,9 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
                                      &address_space_memory);
             if (container->error) {
                 memory_listener_unregister(&container->prereg_listener);
-                ret = container->error;
-                error_setg(errp,
-                    "RAM memory listener initialization failed for container");
+                ret = -1;
+                error_propagate_prepend(errp, container->error,
+                    "RAM memory listener initialization failed: ");
                 goto free_container_exit;
             }
         }
@@ -1365,9 +1378,9 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     memory_listener_register(&container->listener, container->space->as);
 
     if (container->error) {
-        ret = container->error;
-        error_setg_errno(errp, -ret,
-                         "memory listener initialization failed for container");
+        ret = -1;
+        error_propagate_prepend(errp, container->error,
+            "memory listener initialization failed: ");
         goto listener_release_exit;
     }
 
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 96c0ad9d9b..e853eebe11 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -17,6 +17,7 @@
 #include "hw/hw.h"
 #include "exec/ram_addr.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "trace.h"
 
 static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
@@ -85,7 +86,8 @@ static void vfio_prereg_listener_region_add(MemoryListener *listener,
          */
         if (!container->initialized) {
             if (!container->error) {
-                container->error = ret;
+                error_setg_errno(&container->error, -ret,
+                                 "Memory registering failed");
             }
         } else {
             hw_error("vfio: Memory registering failed, unable to continue");
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 9107bd41c0..fd564209ac 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -71,7 +71,7 @@ typedef struct VFIOContainer {
     MemoryListener listener;
     MemoryListener prereg_listener;
     unsigned iommu_type;
-    int error;
+    Error *error;
     bool initialized;
     unsigned long pgsizes;
     QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
-- 
2.20.1

Re: [PATCH v4 1/2] vfio: Turn the container error into an Error handle

[Paolo Bonzini]

On 24/09/19 10:25, Eric Auger wrote:
> The container error integer field is currently used to store
> the first error potentially encountered during any
> vfio_listener_region_add() call. However this fails to propagate
> detailed error messages up to the vfio_connect_container caller.
> Instead of using an integer, let's use an Error handle.
> 
> Messages are slightly reworded to accomodate the propagation.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v3 -> v4:
> - remove ret useless assignments and restore hw_error()
> - remove mr local variable
> - trace [start, end] instead of [start, size] and improve
>   the message for overalp with existing DMA host window
> ---
>  hw/vfio/common.c              | 43 +++++++++++++++++++++++------------
>  hw/vfio/spapr.c               |  4 +++-
>  include/hw/vfio/vfio-common.h |  2 +-
>  3 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 3e03c495d8..cebbb1c28a 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -509,6 +509,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      int ret;
>      VFIOHostDMAWindow *hostwin;
>      bool hostwin_found;
> +    Error *err = NULL;
>  
>      if (vfio_listener_skipped_section(section)) {
>          trace_vfio_listener_region_add_skip(
> @@ -543,13 +544,20 @@ static void vfio_listener_region_add(MemoryListener *listener,
>                                 hostwin->max_iova - hostwin->min_iova + 1,
>                                 section->offset_within_address_space,
>                                 int128_get64(section->size))) {
> -                ret = -1;
> +                error_setg(&err,
> +                    "region [0x%"PRIx64",0x%"PRIx64"] overlaps with existing"
> +                    "host DMA window [0x%"PRIx64",0x%"PRIx64"]",
> +                    section->offset_within_address_space,
> +                    section->offset_within_address_space +
> +                        int128_get64(section->size) - 1,
> +                    hostwin->min_iova, hostwin->max_iova);
>                  goto fail;
>              }
>          }
>  
>          ret = vfio_spapr_create_window(container, section, &pgsize);
>          if (ret) {
> +            error_setg_errno(&err, -ret, "Failed to create SPAPR window");
>              goto fail;
>          }
>  
> @@ -594,10 +602,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      }
>  
>      if (!hostwin_found) {
> -        error_report("vfio: IOMMU container %p can't map guest IOVA region"
> -                     " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> -                     container, iova, end);
> -        ret = -EFAULT;
> +        error_setg(&err, "Container %p can't map guest IOVA region"
> +                   " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container, iova, end);
>          goto fail;
>      }
>  
> @@ -664,11 +670,12 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      ret = vfio_dma_map(container, iova, int128_get64(llsize),
>                         vaddr, section->readonly);
>      if (ret) {
> -        error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> -                     "0x%"HWADDR_PRIx", %p) = %d (%m)",
> -                     container, iova, int128_get64(llsize), vaddr, ret);
> +        error_setg(&err, "vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> +                   "0x%"HWADDR_PRIx", %p) = %d (%m)",
> +                   container, iova, int128_get64(llsize), vaddr, ret);
>          if (memory_region_is_ram_device(section->mr)) {
>              /* Allow unexpected mappings not to be fatal for RAM devices */
> +            error_report_err(err);
>              return;
>          }
>          goto fail;
> @@ -688,9 +695,14 @@ fail:
>       */
>      if (!container->initialized) {
>          if (!container->error) {
> -            container->error = ret;
> +            error_propagate_prepend(&container->error, err,
> +                                    "Region %s: ",
> +                                    memory_region_name(section->mr));
> +        } else {
> +            error_free(err);
>          }
>      } else {
> +        error_report_err(err);
>          hw_error("vfio: DMA mapping failed, unable to continue");
>      }
>  }
> @@ -1251,6 +1263,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      container = g_malloc0(sizeof(*container));
>      container->space = space;
>      container->fd = fd;
> +    container->error = NULL;
>      QLIST_INIT(&container->giommu_list);
>      QLIST_INIT(&container->hostwin_list);
>  
> @@ -1308,9 +1321,9 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>                                       &address_space_memory);
>              if (container->error) {
>                  memory_listener_unregister(&container->prereg_listener);
> -                ret = container->error;
> -                error_setg(errp,
> -                    "RAM memory listener initialization failed for container");
> +                ret = -1;
> +                error_propagate_prepend(errp, container->error,
> +                    "RAM memory listener initialization failed: ");
>                  goto free_container_exit;
>              }
>          }
> @@ -1365,9 +1378,9 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      memory_listener_register(&container->listener, container->space->as);
>  
>      if (container->error) {
> -        ret = container->error;
> -        error_setg_errno(errp, -ret,
> -                         "memory listener initialization failed for container");
> +        ret = -1;
> +        error_propagate_prepend(errp, container->error,
> +            "memory listener initialization failed: ");
>          goto listener_release_exit;
>      }
>  
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 96c0ad9d9b..e853eebe11 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -17,6 +17,7 @@
>  #include "hw/hw.h"
>  #include "exec/ram_addr.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  #include "trace.h"
>  
>  static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
> @@ -85,7 +86,8 @@ static void vfio_prereg_listener_region_add(MemoryListener *listener,
>           */
>          if (!container->initialized) {
>              if (!container->error) {
> -                container->error = ret;
> +                error_setg_errno(&container->error, -ret,
> +                                 "Memory registering failed");
>              }
>          } else {
>              hw_error("vfio: Memory registering failed, unable to continue");
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9107bd41c0..fd564209ac 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -71,7 +71,7 @@ typedef struct VFIOContainer {
>      MemoryListener listener;
>      MemoryListener prereg_listener;
>      unsigned iommu_type;
> -    int error;
> +    Error *error;
>      bool initialized;
>      unsigned long pgsizes;
>      QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

[PATCH v4 2/2] memory: allow memory_region_register_iommu_notifier() to fail

[Eric Auger]

Currently, when a notifier is attempted to be registered and its
flags are not supported (especially the MAP one) by the IOMMU MR,
we generally abruptly exit in the IOMMU code. The failure could be
handled more nicely in the caller and especially in the VFIO code.

So let's allow memory_region_register_iommu_notifier() to fail as
well as notify_flag_changed() callback.

All sites implementing the callback are updated. This patch does
not yet remove the exit(1) in the amd_iommu code.

in SMMUv3 we turn the warning message into an error message saying
that the assigned device would not work properly.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>

---

v2 -> v3:
- turn the warning message into an error message
---
 exec.c                | 10 ++++++++--
 hw/arm/smmuv3.c       | 18 ++++++++++--------
 hw/i386/amd_iommu.c   | 17 ++++++++++-------
 hw/i386/intel_iommu.c |  8 +++++---
 hw/ppc/spapr_iommu.c  |  8 +++++---
 hw/vfio/common.c      |  9 +++++++--
 hw/virtio/vhost.c     |  9 +++++++--
 include/exec/memory.h | 21 ++++++++++++++++-----
 memory.c              | 31 ++++++++++++++++++++-----------
 9 files changed, 88 insertions(+), 43 deletions(-)

diff --git a/exec.c b/exec.c
index 8b998974f8..7379330756 100644
--- a/exec.c
+++ b/exec.c
@@ -663,7 +663,8 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
      */
     MemoryRegion *mr = MEMORY_REGION(iommu_mr);
     TCGIOMMUNotifier *notifier;
-    int i;
+    Error *err = NULL;
+    int i, ret;
 
     for (i = 0; i < cpu->iommu_notifiers->len; i++) {
         notifier = g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier *, i);
@@ -692,7 +693,12 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
                             0,
                             HWADDR_MAX,
                             iommu_idx);
-        memory_region_register_iommu_notifier(notifier->mr, &notifier->n);
+        ret = memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
+                                                    &err);
+        if (ret) {
+            error_report_err(err);
+            exit(1);
+        }
     }
 
     if (!notifier->active) {
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index db051dcac8..e2fbb8357e 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1469,20 +1469,21 @@ static void smmuv3_class_init(ObjectClass *klass, void *data)
     dc->realize = smmu_realize;
 }
 
-static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
-                                       IOMMUNotifierFlag old,
-                                       IOMMUNotifierFlag new)
+static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
+                                      IOMMUNotifierFlag old,
+                                      IOMMUNotifierFlag new,
+                                      Error **errp)
 {
     SMMUDevice *sdev = container_of(iommu, SMMUDevice, iommu);
     SMMUv3State *s3 = sdev->smmu;
     SMMUState *s = &(s3->smmu_state);
 
     if (new & IOMMU_NOTIFIER_MAP) {
-        int bus_num = pci_bus_num(sdev->bus);
-        PCIDevice *pcidev = pci_find_device(sdev->bus, bus_num, sdev->devfn);
-
-        warn_report("SMMUv3 does not support notification on MAP: "
-                     "device %s will not function properly", pcidev->name);
+        error_setg(errp,
+                   "device %02x.%02x.%x requires iommu MAP notifier which is "
+                   "not currently supported", pci_bus_num(sdev->bus),
+                   PCI_SLOT(sdev->devfn), PCI_FUNC(sdev->devfn));
+        return -EINVAL;
     }
 
     if (old == IOMMU_NOTIFIER_NONE) {
@@ -1492,6 +1493,7 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
         trace_smmuv3_notify_flag_del(iommu->parent_obj.name);
         QLIST_REMOVE(sdev, next);
     }
+    return 0;
 }
 
 static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 08884523e2..d3726361dd 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1466,18 +1466,21 @@ static const MemoryRegionOps mmio_mem_ops = {
     }
 };
 
-static void amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
-                                            IOMMUNotifierFlag old,
-                                            IOMMUNotifierFlag new)
+static int amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
+                                           IOMMUNotifierFlag old,
+                                           IOMMUNotifierFlag new,
+                                           Error **errp)
 {
     AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
 
     if (new & IOMMU_NOTIFIER_MAP) {
-        error_report("device %02x.%02x.%x requires iommu notifier which is not "
-                     "currently supported", as->bus_num, PCI_SLOT(as->devfn),
-                     PCI_FUNC(as->devfn));
-        exit(1);
+        error_setg(errp,
+                   "device %02x.%02x.%x requires iommu notifier which is not "
+                   "currently supported", as->bus_num, PCI_SLOT(as->devfn),
+                   PCI_FUNC(as->devfn));
+        return -EINVAL;
     }
+    return 0;
 }
 
 static void amdvi_init(AMDVIState *s)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f1de8fdb75..771bed25c9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2929,9 +2929,10 @@ static IOMMUTLBEntry vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
     return iotlb;
 }
 
-static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
-                                          IOMMUNotifierFlag old,
-                                          IOMMUNotifierFlag new)
+static int vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
+                                         IOMMUNotifierFlag old,
+                                         IOMMUNotifierFlag new,
+                                         Error **errp)
 {
     VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
     IntelIOMMUState *s = vtd_as->iommu_state;
@@ -2944,6 +2945,7 @@ static void vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
     } else if (new == IOMMU_NOTIFIER_NONE) {
         QLIST_REMOVE(vtd_as, next);
     }
+    return 0;
 }
 
 static int vtd_post_load(void *opaque, int version_id)
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index e87b3d50f7..3d3bcc8649 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -205,9 +205,10 @@ static int spapr_tce_get_attr(IOMMUMemoryRegion *iommu,
     return -EINVAL;
 }
 
-static void spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu,
-                                          IOMMUNotifierFlag old,
-                                          IOMMUNotifierFlag new)
+static int spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu,
+                                         IOMMUNotifierFlag old,
+                                         IOMMUNotifierFlag new,
+                                         Error **errp)
 {
     struct SpaprTceTable *tbl = container_of(iommu, SpaprTceTable, iommu);
 
@@ -216,6 +217,7 @@ static void spapr_tce_notify_flag_changed(IOMMUMemoryRegion *iommu,
     } else if (old != IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_NONE) {
         spapr_tce_set_need_vfio(tbl, false);
     }
+    return 0;
 }
 
 static int spapr_tce_table_post_load(void *opaque, int version_id)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index cebbb1c28a..5ca11488d6 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -636,9 +636,14 @@ static void vfio_listener_region_add(MemoryListener *listener,
                             section->offset_within_region,
                             int128_get64(llend),
                             iommu_idx);
-        QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
 
-        memory_region_register_iommu_notifier(section->mr, &giommu->n);
+        ret = memory_region_register_iommu_notifier(section->mr, &giommu->n,
+                                                    &err);
+        if (ret) {
+            g_free(giommu);
+            goto fail;
+        }
+        QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
         memory_region_iommu_replay(giommu->iommu, &giommu->n);
 
         return;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 34accdf615..9a65f034a5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -672,8 +672,9 @@ static void vhost_iommu_region_add(MemoryListener *listener,
                                          iommu_listener);
     struct vhost_iommu *iommu;
     Int128 end;
-    int iommu_idx;
+    int iommu_idx, ret;
     IOMMUMemoryRegion *iommu_mr;
+    Error *err = NULL;
 
     if (!memory_region_is_iommu(section->mr)) {
         return;
@@ -696,7 +697,11 @@ static void vhost_iommu_region_add(MemoryListener *listener,
     iommu->iommu_offset = section->offset_within_address_space -
                           section->offset_within_region;
     iommu->hdev = dev;
-    memory_region_register_iommu_notifier(section->mr, &iommu->n);
+    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, &err);
+    if (ret) {
+        error_report_err(err);
+        exit(1);
+    }
     QLIST_INSERT_HEAD(&dev->iommu_list, iommu, iommu_next);
     /* TODO: can replay help performance here? */
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index a30245c25a..c16430726c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -288,10 +288,16 @@ typedef struct IOMMUMemoryRegionClass {
      * @iommu: the IOMMUMemoryRegion
      * @old_flags: events which previously needed to be notified
      * @new_flags: events which now need to be notified
+     *
+     * Returns 0 on success, or a negative errno; in particular
+     * returns -EINVAL if the new flag bitmap is not supported by the
+     * IOMMU memory region. In case of failure, the error object
+     * must be created
      */
-    void (*notify_flag_changed)(IOMMUMemoryRegion *iommu,
-                                IOMMUNotifierFlag old_flags,
-                                IOMMUNotifierFlag new_flags);
+    int (*notify_flag_changed)(IOMMUMemoryRegion *iommu,
+                               IOMMUNotifierFlag old_flags,
+                               IOMMUNotifierFlag new_flags,
+                               Error **errp);
     /* Called to handle memory_region_iommu_replay().
      *
      * The default implementation of memory_region_iommu_replay() is to
@@ -1067,13 +1073,18 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
  * memory_region_register_iommu_notifier: register a notifier for changes to
  * IOMMU translation entries.
  *
+ * Returns 0 on success, or a negative errno otherwise. In particular,
+ * -EINVAL indicates that at least one of the attributes of the notifier
+ * is not supported (flag/range) by the IOMMU memory region. In case of error
+ * the error object must be created.
+ *
  * @mr: the memory region to observe
  * @n: the IOMMUNotifier to be added; the notify callback receives a
  *     pointer to an #IOMMUTLBEntry as the opaque value; the pointer
  *     ceases to be valid on exit from the notifier.
  */
-void memory_region_register_iommu_notifier(MemoryRegion *mr,
-                                           IOMMUNotifier *n);
+int memory_region_register_iommu_notifier(MemoryRegion *mr,
+                                          IOMMUNotifier *n, Error **errp);
 
 /**
  * memory_region_iommu_replay: replay existing IOMMU translations to
diff --git a/memory.c b/memory.c
index b9dd6b94ca..6df84a4f9c 100644
--- a/memory.c
+++ b/memory.c
@@ -1837,33 +1837,38 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client)
     return memory_region_get_dirty_log_mask(mr) & (1 << client);
 }
 
-static void memory_region_update_iommu_notify_flags(IOMMUMemoryRegion *iommu_mr)
+static int memory_region_update_iommu_notify_flags(IOMMUMemoryRegion *iommu_mr,
+                                                   Error **errp)
 {
     IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE;
     IOMMUNotifier *iommu_notifier;
     IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
+    int ret = 0;
 
     IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) {
         flags |= iommu_notifier->notifier_flags;
     }
 
     if (flags != iommu_mr->iommu_notify_flags && imrc->notify_flag_changed) {
-        imrc->notify_flag_changed(iommu_mr,
-                                  iommu_mr->iommu_notify_flags,
-                                  flags);
+        ret = imrc->notify_flag_changed(iommu_mr,
+                                        iommu_mr->iommu_notify_flags,
+                                        flags, errp);
     }
 
-    iommu_mr->iommu_notify_flags = flags;
+    if (!ret) {
+        iommu_mr->iommu_notify_flags = flags;
+    }
+    return ret;
 }
 
-void memory_region_register_iommu_notifier(MemoryRegion *mr,
-                                           IOMMUNotifier *n)
+int memory_region_register_iommu_notifier(MemoryRegion *mr,
+                                          IOMMUNotifier *n, Error **errp)
 {
     IOMMUMemoryRegion *iommu_mr;
+    int ret;
 
     if (mr->alias) {
-        memory_region_register_iommu_notifier(mr->alias, n);
-        return;
+        return memory_region_register_iommu_notifier(mr->alias, n, errp);
     }
 
     /* We need to register for at least one bitfield */
@@ -1874,7 +1879,11 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr,
            n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr));
 
     QLIST_INSERT_HEAD(&iommu_mr->iommu_notify, n, node);
-    memory_region_update_iommu_notify_flags(iommu_mr);
+    ret = memory_region_update_iommu_notify_flags(iommu_mr, errp);
+    if (ret) {
+        QLIST_REMOVE(n, node);
+    }
+    return ret;
 }
 
 uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr)
@@ -1927,7 +1936,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr,
     }
     QLIST_REMOVE(n, node);
     iommu_mr = IOMMU_MEMORY_REGION(mr);
-    memory_region_update_iommu_notify_flags(iommu_mr);
+    memory_region_update_iommu_notify_flags(iommu_mr, NULL);
 }
 
 void memory_region_notify_one(IOMMUNotifier *notifier,
-- 
2.20.1

Re: [PATCH v4 0/2] Allow memory_region_register_iommu_notifier() to fail

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]

On Tue, Sep 24, 2019 at 10:25:15AM +0200, Eric Auger wrote:
> This series allows the memory_region_register_iommu_notifier()
> to fail. As of now, when a MAP notifier is attempted to be
> registered along with SMMUv3 or AMD IOMMU, we exit in the IOMMU
> MR notify_flag_changed() callback.
> 
> In case of VFIO assigned device hotplug, this could be handled
> more nicely directly within the VFIO code, simply rejecting
> the hotplug without exiting. This is what the series achieves
> by handling the memory_region_register_iommu_notifier() returned
> value and Error object.
> 
> To propagate errors collected during vfio_listener_region_add()
> we now store the error handle inside the VFIO container instead
> of a returned value.
> 
> The message now is:
> (QEMU) device_add id=hot0 driver=vfio-pci host=0000:89:00.0 bus=pcie.1
> {"error": {"class": "GenericError", "desc": "vfio 0000:89:00.0: failed
> to setup container for group 2: memory listener initialization failed:
> Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
> notifier which is not currently supported"}}
> 
> Best Regards

For the series

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

and ppc parts

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> 
> Eric
> 
> This series can be found at:
> https://github.com/eauger/qemu/tree/v4.1.0_register_iommu_notifier_fail_v4
> 
> History:
> 
> v3 -> v4:
> - added Peter's R-b on 2d patch
> - 1st patch: restore hw_error, remove useless ret assignment, improve
>   DMA host window error message, remove local mr variable
> 
> v2 -> v3:
> - also pass an Error handle (suggested by Peter)
> 
> v1 -> v2:
> - Intel IOMMU now handles the problem differently with machine init done
>   notifier and machine hotplug allowed hook.
> - use assert(!ret)
> - message rewording in SMMUv3
> 
> Follow-up of "VFIO/SMMUv3: Fail on VFIO/HW nested paging detection"
> https://patchew.org/QEMU/20190829090141.21821-1-eric.auger@redhat.com/
> 
> 
> Eric Auger (2):
>   vfio: Turn the container error into an Error handle
>   memory: allow memory_region_register_iommu_notifier() to fail
> 
>  exec.c                        | 10 +++++--
>  hw/arm/smmuv3.c               | 18 ++++++------
>  hw/i386/amd_iommu.c           | 17 +++++++-----
>  hw/i386/intel_iommu.c         |  8 ++++--
>  hw/ppc/spapr_iommu.c          |  8 ++++--
>  hw/vfio/common.c              | 52 +++++++++++++++++++++++------------
>  hw/vfio/spapr.c               |  4 ++-
>  hw/virtio/vhost.c             |  9 ++++--
>  include/exec/memory.h         | 21 ++++++++++----
>  include/hw/vfio/vfio-common.h |  2 +-
>  memory.c                      | 31 +++++++++++++--------
>  11 files changed, 120 insertions(+), 60 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/2] Allow memory_region_register_iommu_notifier() to fail

[Alex Williamson]

On Tue, 24 Sep 2019 10:25:15 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> This series allows the memory_region_register_iommu_notifier()
> to fail. As of now, when a MAP notifier is attempted to be
> registered along with SMMUv3 or AMD IOMMU, we exit in the IOMMU
> MR notify_flag_changed() callback.
> 
> In case of VFIO assigned device hotplug, this could be handled
> more nicely directly within the VFIO code, simply rejecting
> the hotplug without exiting. This is what the series achieves
> by handling the memory_region_register_iommu_notifier() returned
> value and Error object.
> 
> To propagate errors collected during vfio_listener_region_add()
> we now store the error handle inside the VFIO container instead
> of a returned value.
> 
> The message now is:
> (QEMU) device_add id=hot0 driver=vfio-pci host=0000:89:00.0 bus=pcie.1
> {"error": {"class": "GenericError", "desc": "vfio 0000:89:00.0: failed
> to setup container for group 2: memory listener initialization failed:
> Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
> notifier which is not currently supported"}}
> 
> Best Regards
> 
> Eric
> 
> This series can be found at:
> https://github.com/eauger/qemu/tree/v4.1.0_register_iommu_notifier_fail_v4
> 
> History:
> 
> v3 -> v4:
> - added Peter's R-b on 2d patch
> - 1st patch: restore hw_error, remove useless ret assignment, improve
>   DMA host window error message, remove local mr variable
> 
> v2 -> v3:
> - also pass an Error handle (suggested by Peter)
> 
> v1 -> v2:
> - Intel IOMMU now handles the problem differently with machine init done
>   notifier and machine hotplug allowed hook.
> - use assert(!ret)
> - message rewording in SMMUv3
> 
> Follow-up of "VFIO/SMMUv3: Fail on VFIO/HW nested paging detection"
> https://patchew.org/QEMU/20190829090141.21821-1-eric.auger@redhat.com/
> 
> 
> Eric Auger (2):
>   vfio: Turn the container error into an Error handle
>   memory: allow memory_region_register_iommu_notifier() to fail
> 
>  exec.c                        | 10 +++++--
>  hw/arm/smmuv3.c               | 18 ++++++------
>  hw/i386/amd_iommu.c           | 17 +++++++-----
>  hw/i386/intel_iommu.c         |  8 ++++--
>  hw/ppc/spapr_iommu.c          |  8 ++++--
>  hw/vfio/common.c              | 52 +++++++++++++++++++++++------------
>  hw/vfio/spapr.c               |  4 ++-
>  hw/virtio/vhost.c             |  9 ++++--
>  include/exec/memory.h         | 21 ++++++++++----
>  include/hw/vfio/vfio-common.h |  2 +-
>  memory.c                      | 31 +++++++++++++--------
>  11 files changed, 120 insertions(+), 60 deletions(-)

For series,

Acked-by: Alex Williamson <alex.williamson@redhat.com>

Paolo, would this go in through you given the memory API changes and
greater girth in patch 2/2?  Thanks,

Alex

Re: [PATCH v4 0/2] Allow memory_region_register_iommu_notifier() to fail

[Paolo Bonzini]

On 24/09/19 23:31, Alex Williamson wrote:
> On Tue, 24 Sep 2019 10:25:15 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> This series allows the memory_region_register_iommu_notifier()
>> to fail. As of now, when a MAP notifier is attempted to be
>> registered along with SMMUv3 or AMD IOMMU, we exit in the IOMMU
>> MR notify_flag_changed() callback.
>>
>> In case of VFIO assigned device hotplug, this could be handled
>> more nicely directly within the VFIO code, simply rejecting
>> the hotplug without exiting. This is what the series achieves
>> by handling the memory_region_register_iommu_notifier() returned
>> value and Error object.
>>
>> To propagate errors collected during vfio_listener_region_add()
>> we now store the error handle inside the VFIO container instead
>> of a returned value.
>>
>> The message now is:
>> (QEMU) device_add id=hot0 driver=vfio-pci host=0000:89:00.0 bus=pcie.1
>> {"error": {"class": "GenericError", "desc": "vfio 0000:89:00.0: failed
>> to setup container for group 2: memory listener initialization failed:
>> Region smmuv3-iommu-memory-region-0-6: device 01.00.0 requires iommu MAP
>> notifier which is not currently supported"}}
>>
>> Best Regards
>>
>> Eric
>>
>> This series can be found at:
>> https://github.com/eauger/qemu/tree/v4.1.0_register_iommu_notifier_fail_v4
>>
>> History:
>>
>> v3 -> v4:
>> - added Peter's R-b on 2d patch
>> - 1st patch: restore hw_error, remove useless ret assignment, improve
>>   DMA host window error message, remove local mr variable
>>
>> v2 -> v3:
>> - also pass an Error handle (suggested by Peter)
>>
>> v1 -> v2:
>> - Intel IOMMU now handles the problem differently with machine init done
>>   notifier and machine hotplug allowed hook.
>> - use assert(!ret)
>> - message rewording in SMMUv3
>>
>> Follow-up of "VFIO/SMMUv3: Fail on VFIO/HW nested paging detection"
>> https://patchew.org/QEMU/20190829090141.21821-1-eric.auger@redhat.com/
>>
>>
>> Eric Auger (2):
>>   vfio: Turn the container error into an Error handle
>>   memory: allow memory_region_register_iommu_notifier() to fail
>>
>>  exec.c                        | 10 +++++--
>>  hw/arm/smmuv3.c               | 18 ++++++------
>>  hw/i386/amd_iommu.c           | 17 +++++++-----
>>  hw/i386/intel_iommu.c         |  8 ++++--
>>  hw/ppc/spapr_iommu.c          |  8 ++++--
>>  hw/vfio/common.c              | 52 +++++++++++++++++++++++------------
>>  hw/vfio/spapr.c               |  4 ++-
>>  hw/virtio/vhost.c             |  9 ++++--
>>  include/exec/memory.h         | 21 ++++++++++----
>>  include/hw/vfio/vfio-common.h |  2 +-
>>  memory.c                      | 31 +++++++++++++--------
>>  11 files changed, 120 insertions(+), 60 deletions(-)
> 
> For series,
> 
> Acked-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Paolo, would this go in through you given the memory API changes and
> greater girth in patch 2/2?  Thanks,

Fine, queued.

Paolo