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
>> +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)
>> + if (ret)
>
> return ret
>
>> + break;
>> + }
>> +
>> + if (!iopt_area_contig_done(&iter))
>> + ret = -EINVAL;
>
> return -EINVAL
>
>> +
>> + return ret;
>
> return 0;
>
> And remove the -EINVAL.
OK
> 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@xxxxxxxxxx>
>
> Jason
