[v2,0/5] Allow memory_region_register_iommu_notifier() to fail

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, Intel iommu without caching mode
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.

Best Regards

Eric

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

History:

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 (5):
  memory: allow memory_region_register_iommu_notifier() to fail
  vfio/common: Handle memory_region_register_iommu_notifier() failure
  exec: assert on memory_region_register_iommu_notifier() failure
  vhost: assert on memory_region_register_iommu_notifier() failure
  amd_iommu: Let amdvi_iommu_notify_flag_changed() fail

 exec.c                |  6 ++++--
 hw/arm/smmuv3.c       | 12 +++++++-----
 hw/i386/amd_iommu.c   |  9 +++++----
 hw/i386/intel_iommu.c |  7 ++++---
 hw/ppc/spapr_iommu.c  |  7 ++++---
 hw/vfio/common.c      |  8 ++++++--
 hw/virtio/vhost.c     |  5 +++--
 include/exec/memory.h | 18 +++++++++++++-----
 memory.c              | 28 ++++++++++++++++++----------
 9 files changed, 64 insertions(+), 36 deletions(-)

On Thu, Sep 19, 2019 at 02:18:40PM +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, Intel iommu without caching mode
> 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.

Could I ask what's the code path for ARM when the hot plug failed?  Is
that vfio_realize() then vfio_connect_container() will fail with this?

    if (container->error) {
        ret = container->error;
        error_setg_errno(errp, -ret,
                         "memory listener initialization failed for container");
        goto listener_release_exit;
    }

If so, I would again suggest you to use Error** in patch 1.  IMHO we
can let vfio_listener_region_add() to be the first one to use the
Error** so that instead of this:

    /*
     * On the initfn path, store the first error in the container so we
     * can gracefully fail.  Runtime, there's not much we can do other
     * than throw a hardware error.
     */
    if (!container->initialized) {
        if (!container->error) {
            container->error = ret;
        }
    } else {
        hw_error("vfio: DMA mapping failed, unable to continue");
    }

We can also cache the Error** into container and return to user if the
user is using QMP which should be better than the int number (or
again, maybe return both errors?).  IIUC error_report() will not work
for QMP.

Regards,

Hi Peter,

On 9/20/19 8:16 AM, Peter Xu wrote:
> On Thu, Sep 19, 2019 at 02:18:40PM +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, Intel iommu without caching mode
>> 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.
> 
> Could I ask what's the code path for ARM when the hot plug failed?  Is
> that vfio_realize() then vfio_connect_container() will fail with this?
> 
>     if (container->error) {
>         ret = container->error;
>         error_setg_errno(errp, -ret,
>                          "memory listener initialization failed for container");
>         goto listener_release_exit;
>     }
> 
Yes that path is exercised.

> If so, I would again suggest you to use Error** in patch 1.  IMHO we
> can let vfio_listener_region_add() to be the first one to use the
> Error** so that instead of this:
> 
>     /*
>      * On the initfn path, store the first error in the container so we
>      * can gracefully fail.  Runtime, there's not much we can do other
>      * than throw a hardware error.
>      */
>     if (!container->initialized) {
>         if (!container->error) {
>             container->error = ret;
>         }
>     } else {
>         hw_error("vfio: DMA mapping failed, unable to continue");
>     }

OK. I agree that's better. That's also more code change ;-)

But let's do it!

Thank you for the review!

Best Regards

Eric
> 
> We can also cache the Error** into container and return to user if the
> user is using QMP which should be better than the int number (or
> again, maybe return both errors?).  IIUC error_report() will not work
> for QMP.
> 
> Regards,
>