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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Oct 17, 2023 at 04:51:25PM +0100, Joao Martins wrote:
> On 17/10/2023 16:29, Jason Gunthorpe wrote:
> > On Tue, Oct 17, 2023 at 01:06:12PM +0100, Joao Martins wrote:
> >> On 23/09/2023 02:40, Joao Martins wrote:
> >>> On 23/09/2023 02:24, Joao Martins wrote:
> >>>> +int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
> >>>> +				   struct iommu_domain *domain,
> >>>> +				   unsigned long flags,
> >>>> +				   struct iommufd_dirty_data *bitmap)
> >>>> +{
> >>>> +	unsigned long last_iova, iova = bitmap->iova;
> >>>> +	unsigned long length = bitmap->length;
> >>>> +	int ret = -EOPNOTSUPP;
> >>>> +
> >>>> +	if ((iova & (iopt->iova_alignment - 1)))
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	if (check_add_overflow(iova, length - 1, &last_iova))
> >>>> +		return -EOVERFLOW;
> >>>> +
> >>>> +	down_read(&iopt->iova_rwsem);
> >>>> +	ret = iommu_read_and_clear_dirty(domain, flags, bitmap);
> >>>> +	up_read(&iopt->iova_rwsem);
> >>>> +	return ret;
> >>>> +}
> >>>
> >>> I need to call out that a mistake I made, noticed while submitting. I should be
> >>> walk over iopt_areas here (or in iommu_read_and_clear_dirty()) to check
> >>> area::pages. So this is a comment I have to fix for next version. 
> >>
> >> Below is how I fixed it.
> >>
> >> Essentially the thinking being that the user passes either an mapped IOVA area
> >> it mapped *or* a subset of a mapped IOVA area. This should also allow the
> >> possibility of having multiple threads read dirties from huge IOVA area splitted
> >> in different chunks (in the case it gets splitted into lowest level).
> > 
> > What happens if the iommu_read_and_clear_dirty is done on unmapped
> > PTEs? It fails?
> 
> If there's no IOPTE or the IOPTE is non-present, it keeps walking to the next
> base page (or level-0 IOVA range). For both drivers in this series.

Hum, so this check doesn't seem quite right then as it is really an
input validation that the iova is within the tree. It should be able
to span contiguous areas.

Write it with the intersection logic:

for (area = iopt_area_iter_first(iopt, iova, iova_last); area;
     area = iopt_area_iter_next(area, iova, iova_last)) {
    if (!area->pages)
       // fail

    if (cur_iova < area_first)
       // fail

    if (last_iova <= area_last)
       // success, do iommu_read_and_clear_dirty()

    cur_iova = area_last + 1;
}

// else fail if not success

Jason