Re: [PATCH RFC 01/19] iommu: Add iommu_domain ops for dirty tracking

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 28, 2022 at 10:09:15PM +0100, Joao Martins wrote:
> +
> +unsigned int iommu_dirty_bitmap_record(struct iommu_dirty_bitmap *dirty,
> +				       unsigned long iova, unsigned long length)
> +{

Lets put iommu_dirty_bitmap in its own patch, the VFIO driver side
will want to use this same data structure.

> +	while (nbits > 0) {
> +		kaddr = kmap(dirty->pages[idx]) + start_offset;

kmap_local?

> +/**
> + * struct iommu_dirty_bitmap - Dirty IOVA bitmap state
> + *
> + * @iova: IOVA representing the start of the bitmap, the first bit of the bitmap
> + * @pgshift: Page granularity of the bitmap
> + * @gather: Range information for a pending IOTLB flush
> + * @start_offset: Offset of the first user page
> + * @pages: User pages representing the bitmap region
> + * @npages: Number of user pages pinned
> + */
> +struct iommu_dirty_bitmap {
> +	unsigned long iova;
> +	unsigned long pgshift;
> +	struct iommu_iotlb_gather *gather;
> +	unsigned long start_offset;
> +	unsigned long npages;
> +	struct page **pages;

In many (all?) cases I would expect this to be called from a process
context, can we just store the __user pointer here, or is the idea
that with modern kernels poking a u64 to userspace is slower than a
kmap?

I'm particularly concerend that this starts to require high
order allocations with more than 2M of bitmap.. Maybe one direction is
to GUP 2M chunks at a time and walk the __user pointer.

> +static inline void iommu_dirty_bitmap_init(struct iommu_dirty_bitmap *dirty,
> +					   unsigned long base,
> +					   unsigned long pgshift,
> +					   struct iommu_iotlb_gather *gather)
> +{
> +	memset(dirty, 0, sizeof(*dirty));
> +	dirty->iova = base;
> +	dirty->pgshift = pgshift;
> +	dirty->gather = gather;
> +
> +	if (gather)
> +		iommu_iotlb_gather_init(dirty->gather);
> +}

I would expect all the GUPing logic to be here too?

Jason