[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 0/5] Allow memory_region_register_iommu_notifier() to fail
From: |
Auger Eric |
Subject: |
Re: [PATCH v2 0/5] Allow memory_region_register_iommu_notifier() to fail |
Date: |
Fri, 20 Sep 2019 09:05:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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,
>
- [Qemu-arm] [PATCH v2 0/5] Allow memory_region_register_iommu_notifier() to fail, Eric Auger, 2019/09/19
- [Qemu-arm] [PATCH v2 1/5] memory: allow memory_region_register_iommu_notifier() to fail, Eric Auger, 2019/09/19
- [Qemu-arm] [PATCH v2 2/5] vfio/common: Handle memory_region_register_iommu_notifier() failure, Eric Auger, 2019/09/19
- [Qemu-arm] [PATCH v2 3/5] exec: assert on memory_region_register_iommu_notifier() failure, Eric Auger, 2019/09/19
- [Qemu-arm] [PATCH v2 4/5] vhost: assert on memory_region_register_iommu_notifier() failure, Eric Auger, 2019/09/19
- [Qemu-arm] [PATCH v2 5/5] amd_iommu: Let amdvi_iommu_notify_flag_changed() fail, Eric Auger, 2019/09/19
- Re: [PATCH v2 0/5] Allow memory_region_register_iommu_notifier() to fail, Peter Xu, 2019/09/20
- Re: [PATCH v2 0/5] Allow memory_region_register_iommu_notifier() to fail,
Auger Eric <=