Re: [PATCH v2 0/5] Allow memory_region_register_iommu_notifier() to fail

[Auger Eric <eric.auger@redhat.com>]

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