Re: [PATCH 1/5] vfio/iommufd: Add framework code to support getting dirty bitmap before unmap

["Cédric Le Goater" <clg@redhat.com>]

On 9/23/25 04:45, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH 1/5] vfio/iommufd: Add framework code to support
>> getting dirty bitmap before unmap
>>
>> On 9/22/25 05:17, Duan, Zhenzhong wrote:
>>> Hi Cedric,
>>>
>>>> -----Original Message-----
>>>> From: Cédric Le Goater <clg@redhat.com>
>>>> Subject: Re: [PATCH 1/5] vfio/iommufd: Add framework code to support
>>>> getting dirty bitmap before unmap
>>>>
>>>> Hello Zhenzhong
>>>>
>>>> On 9/10/25 04:36, Zhenzhong Duan wrote:
>>>>> Currently we support device and iommu dirty tracking, device dirty
>>>>> tracking is preferred.
>>>>>
>>>>> Add the framework code in iommufd_cdev_unmap_one() to choose
>> either
>>>>> device or iommu dirty tracking, just like vfio_legacy_dma_unmap_one().
>>>>
>>>> I wonder if commit 567d7d3e6be5 ("vfio/common: Work around kernel
>>>> overflow bug in DMA unmap") could be removed now to make the code
>>>> common to both VFIO IOMMU Type1 and IOMMUFD backends.
>>>
>>> I am not clear if there is other reason to keep the workaround, but the
>> original
>>> kernel issue had been fixed with below commit:
>>>
>>> commit 58fec830fc19208354895d9832785505046d6c01
>>> Author: Alex Williamson <alex.williamson@redhat.com>
>>> Date:   Mon Jan 7 22:13:22 2019 -0700
>>>
>>>       vfio/type1: Fix unmap overflow off-by-one
>>>
>>>       The below referenced commit adds a test for integer overflow, but in
>>>       doing so prevents the unmap ioctl from ever including the last page
>> of
>>>       the address space.  Subtract one to compare to the last address of
>> the
>>>       unmap to avoid the overflow and wrap-around.
>>>
>>>       Fixes: 71a7d3d78e3c ("vfio/type1: silence integer overflow warning")
>>>       Link: https://bugzilla.redhat.com/show_bug.cgi?id=1662291
>>>       Cc: stable@vger.kernel.org # v4.15+
>>>       Reported-by: Pei Zhang <pezhang@redhat.com>
>>>       Debugged-by: Peter Xu <peterx@redhat.com>
>>>       Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>       Reviewed-by: Peter Xu <peterx@redhat.com>
>>>       Tested-by: Peter Xu <peterx@redhat.com>
>>>       Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>       Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>
>>>>
>>>> I asked Alex and Peter in another thread.
>>>
>>> Just curious on the answer, may I ask which thread?
>>
>> According to Alex, the QEMU workaround can be removed :
>>
>> https://lore.kernel.org/qemu-devel/20250919102447.748e17fe.alex.williams
>> on@redhat.com/
>>
>>> btw: I just found unmapping in halves seems unnecessary as both backends
>> of kernel side support unmap_all now.
>>>
>>>       if (unmap_all) {
>>>           /* The unmap ioctl doesn't accept a full 64-bit span. */
>>>           Int128 llsize = int128_rshift(int128_2_64(), 1);
>>>
>>>           ret = vfio_legacy_dma_unmap_one(bcontainer, 0,
>> int128_get64(llsize),
>>>                                           iotlb);
>>>
>>>           if (ret == 0) {
>>>               ret = vfio_legacy_dma_unmap_one(bcontainer,
>> int128_get64(llsize),
>>>                                               int128_get64(llsize),
>> iotlb);
>>>           }
>>>
>>>       } else {
>>>           ret = vfio_legacy_dma_unmap_one(bcontainer, iova, size,
>> iotlb);
>>>       }
>>
>> Good. So we can simply both backends it seems.

*ify

> 
> Will you handle them or not? I mean the workaround removing and unmapping_all optimization.

I can revert 567d7d3e6be5 ("vfio/common: Work around kernel overflow
bug in DMA unmap") but, AFAICT, the "unmap DMAs in halves" method (see
1b296c3def4b) should be kept.

Thanks,

C.