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

Joao Martins
2023-10-17T18:15:24Z

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;