Re: [PATCH v4 07/18] iommufd: Add IOMMU_HWPT_GET_DIRTY_IOVA

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Oct 18, 2023 at 09:27:04PM +0100, Joao Martins wrote:

> +int iommufd_check_iova_range(struct iommufd_ioas *ioas,
> +			     struct iommu_hwpt_get_dirty_iova *bitmap)
> +{
> +	unsigned long pgshift, npages;
> +	size_t iommu_pgsize;
> +	int rc = -EINVAL;
> +
> +	pgshift = __ffs(bitmap->page_size);
> +	npages = bitmap->length >> pgshift;

npages = bitmap->length / bitmap->page_size;

? (if page_size is a bitmask it is badly named)

> +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 last_iova = iova + length - 1;
> +	int ret = -EINVAL;
> +
> +	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,
> +						0, dirty);

This seems like a lot of stuff going on with ret..

> +		if (ret)

return ret

> +			break;
> +	}
> +
> +	if (!iopt_area_contig_done(&iter))
> +		ret = -EINVAL;

return  -EINVAL

> +
> +	return ret;

return 0;

And remove the -EINVAL. iopt_area_contig_done() captures the case
where the iova range is not fully contained by areas, even the case
where there are no areas.

But otherwise

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason