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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 19, 2023 at 12:43:19AM +0100, Joao Martins wrote:
> On 18/10/2023 23:39, Jason Gunthorpe wrote:
> > 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)
> > 
> 
> It was a way to avoid the divide by zero, but
> I can switch to the above, and check for bitmap->page_size
> being non-zero. should be less obscure

Why would we ge so far along with a 0 page size? Reject that when
bitmap is created??

> >> +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..
> 
> All to have a single return exit point, given no different cleanup is required.
> Thought it was the general best way (when possible)

Mm, at least I'm not a believer of single exit point :)

Jason