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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Peter Xu <peterx@redhat.com>
>Sent: Friday, June 9, 2023 3:53 AM
>To: Jason Gunthorpe <jgg@nvidia.com>
>Cc: Liu, Yi L <yi.l.liu@intel.com>; 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; 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 01:27:50PM -0300, Jason Gunthorpe wrote:
>> On Thu, Jun 08, 2023 at 11:40:55AM -0400, Peter Xu wrote:
>>
>> > > 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.
>>
>> qemu has to fix it. The kernel API is object based, not paged
>> based. You cannot unmap partions of a prior mapping.
>>
>> I assume for this kind of emulation it is doing 4k objects because
>> it has no idea what size of mapping the client will use?
>
>MAP is fine, before notify() to VFIO or anything, qemu scans the pgtable
>and handles it in page size or huge page size, so it can be >4K but always
>guest iommu pgsize aligned.
>
>I think we rely on guest behaving right, so it should also always operate
>on that size minimum when mapped huge.  It shouldn't violate the
>"per-object" protocol of iommufd.
>
>IIUC the same to vfio type1v2 from that aspect.
>
>It's more about UNMAP batching, but I assume iommufd is fine if it's fine
>with holes inside for that case.  The only difference of "not exist" of
>-ENOENT seems to be just same as before as long as QEMU treats it as 0 like
>before.
>
>Though that does look slightly special, because the whole empty UNMAP
>region can be seen as a hole too; not sure when that -ENOENT will be useful
>if qemu should always bypass it anyway.  Indeed not a problem to qemu.
>
>>
>> > We can definitely lookup every single object and explicitly unmap, but it
>> > loses partial of the point of batching that guest OS does.
>>
>> You don't need every single object, but it would be faster to check
>> where things are mapped and then call the kernel correctly instead of
>> trying to iterate with the unmapped reults.
>
>Maybe yes.  If so, It'll be great if Zhenzhong could just attach some proof
>on that, meanwhile drop the "iommufd UNMAP warnings" section in the commit
>message.

Seems vtd_page_walk_one() already works in above way, checking mapping
changes and calling kernel for changed entries?

Thanks
Zhenzhong