On 17/10/2023 17:01, Jason Gunthorpe wrote:
> 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
>
Perhaps that could be rewritten as e.g.
ret = -EINVAL;
iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
// do iommu_read_and_clear_dirty();
}
// else fail.
Though OTOH, the places you wrote as to fail are skipped instead.
