RE: [PATCH v3 5/5] intel_iommu: Optimize out some unnecessary UNMAP calls

[Duan, Zhenzhong]

>-----Original Message-----
>From: Peter Xu <peterx@redhat.com>
>Sent: Thursday, June 8, 2023 11:41 PM
>To: Jason Gunthorpe <jgg@nvidia.com>; Liu, Yi L <yi.l.liu@intel.com>; Duan,
>Zhenzhong <zhenzhong.duan@intel.com>
>Cc: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org; mst@redhat.com; jasowang@redhat.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>marcel.apfelbaum@gmail.com; alex.williamson@redhat.com;
>clg@redhat.com; david@redhat.com; philmd@linaro.org;
>kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng,
>Chao P <chao.p.peng@intel.com>
>Subject: Re: [PATCH v3 5/5] intel_iommu: Optimize out some unnecessary
>UNMAP calls
>
>On Thu, Jun 08, 2023 at 11:11:15AM -0300, Jason Gunthorpe wrote:
>> On Thu, Jun 08, 2023 at 10:05:08AM -0400, Peter Xu wrote:
>>
>> > IIUC what VFIO does here is it returns succeed if unmap over nothing
>rather
>> > than failing like iommufd.  Curious (like JasonW) on why that retval?  I'd
>> > assume for returning "how much unmapped" we can at least still return 0
>for
>> > nothing.
>>
>> In iommufd maps are objects, you can only map or unmap entire
>> objects. The ability to batch unmap objects by specifying an range
>> that spans many is something that was easy to do and that VFIO had,
>> but I'm not sure it is actually usefull..
>>
>> So asking to unmap an object that is already known not to be mapped is
>> actually possibly racy, especially if you consider iommufd's support
>> for kernel-side IOVA allocation. It should not be done, or if it is
>> done, with user space locking to protect it.
>>
>> For VFIO, long long ago, VFIO could unmap IOVA page at a time - ie it
>> wasn't objects. In this world it made some sense that the unmap would
>> 'succeed' as the end result was unmapped.
>>
>> > Are you probably suggesting that we can probably handle that in QEMU
>side
>> > on -ENOENT here for iommufd only (a question to Yi?).
>>
>> Yes, this can be done, ENOENT is reliably returned and qemu doesn't
>> use the kernel-side IOVA allocator.
>>
>> But if there is the proper locks to prevent a map/unmap race, then
>> there should also be the proper locks to check that there is no map in
>> the first place and avoid the kernel call..
>
>The problem is IIRC guest iommu driver can do smart things like batching
>invalidations, it means when QEMU gets it from the guest OS it may already
>not matching one mapped objects.
>
>We can definitely lookup every single object and explicitly unmap, but it
>loses partial of the point of batching that guest OS does.  Logically QEMU
>can redirect that batched invalidation into one ioctl() to the host, rather
>than a lot of smaller ones.
>
>While for this specific patch - Zhenzhong/Yi, do you agree that we should
>just handle -ENOENT in the iommufd series (I assume it's still under work),
>then for this specific patch it's only about performance difference?

Yes, that make sense.

Thanks
Zhenzhong