Re: [PATCH v3 07/19] iommufd: Dirty tracking data support

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 17/10/2023 18:30, Joao Martins wrote:
> On 17/10/2023 18:13, Jason Gunthorpe wrote:
>> On Tue, Oct 17, 2023 at 05:51:49PM +0100, Joao Martins wrote:
>>  
>>> Perhaps that could be rewritten as e.g.
>>>
>>> 	ret = -EINVAL;
>>> 	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
>>> 		// do iommu_read_and_clear_dirty();
>>> 	}
>>>
>>> 	// else fail.
>>>
>>> Though OTOH, the places you wrote as to fail are skipped instead.
>>
>> Yeah, if consolidating the areas isn't important (it probably isn't)
>> then this is the better API
>>
> 
> Doing it in a single iommu_read_and_clear_dirty() saves me from manipulating the
> bitmap address in an atypical way. Considering that the first bit in each u8 is
> the iova we initialize the bitmap, so if I call it in multiple times in a single
> IOVA range (in each individual area as I think you suggested) then I need to
> align down the iova-length to the minimum granularity of the bitmap, which is an
> u8 (32k).

Or I can do the reverse, which is to iterate the bitmap like right now, and
iterate over individual iopt areas from within the iova-bitmap iterator, and
avoid this. That's should be a lot cleaner:

diff --git a/drivers/iommu/iommufd/io_pagetable.c
b/drivers/iommu/iommufd/io_pagetable.c
index 991c57458725..179afc6b74f2 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -415,6 +415,7 @@ int iopt_map_user_pages(struct iommufd_ctx *ictx, struct
io_pagetable *iopt,

 struct iova_bitmap_fn_arg {
        unsigned long flags;
+       struct io_pagetable *iopt;
        struct iommu_domain *domain;
        struct iommu_dirty_bitmap *dirty;
 };
@@ -423,16 +424,34 @@ static int __iommu_read_and_clear_dirty(struct iova_bitmap
*bitmap,
                                        unsigned long iova, size_t length,
                                        void *opaque)
 {
+       struct iopt_area *area;
+       struct iopt_area_contig_iter iter;
        struct iova_bitmap_fn_arg *arg = opaque;
        struct iommu_domain *domain = arg->domain;
        struct iommu_dirty_bitmap *dirty = arg->dirty;
        const struct iommu_dirty_ops *ops = domain->dirty_ops;
        unsigned long flags = arg->flags;
+       unsigned long last_iova = iova + length - 1;
+       int ret = -EINVAL;

-       return ops->read_and_clear_dirty(domain, iova, length, flags, dirty);
+       iopt_for_each_contig_area(&iter, area, arg->iopt, iova, last_iova) {
+               unsigned long last = min(last_iova, iopt_area_last_iova(area));
+
+               ret = ops->read_and_clear_dirty(domain, iter.cur_iova,
+                                               last - iter.cur_iova + 1,
+                                               flags, dirty);
+               if (ret)
+                       break;
+       }
+
+       if (!iopt_area_contig_done(&iter))
+               ret = -EINVAL;
+
+       return ret;
 }

 static int iommu_read_and_clear_dirty(struct iommu_domain *domain,
+                                     struct io_pagetable *iopt,
                                      unsigned long flags,
                                      struct iommu_hwpt_get_dirty_iova *bitmap)
 {
@@ -453,6 +472,7 @@ static int iommu_read_and_clear_dirty(struct iommu_domain
*domain,

        iommu_dirty_bitmap_init(&dirty, iter, &gather);

+       arg.iopt = iopt;
        arg.flags = flags;
        arg.domain = domain;
        arg.dirty = &dirty;