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