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

[Peter Xu]

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?

Thanks,

-- 
Peter Xu