Message ID | 20190919121845.29520-1-eric.auger@redhat.com |
---|---|
Headers | show |
Series | Allow memory_region_register_iommu_notifier() to fail | expand |
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, >