Re: [PATCH Kernel v21 5/8] vfio iommu: Implementation of ioctl for dirty pages tracking

Kirti Wankhede
2020-05-16T19:08:30Z

DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
t=1589656095; bh=+tZ0dBYIJDY6PHAfvMYygljkbJgDRKM2mXYJTiJ5LAU=;
h=X-PGP-Universal:Subject:To:CC:References:X-Nvconfidentiality:From:
Message-ID:Date:User-Agent:MIME-Version:In-Reply-To:
X-Originating-IP:X-ClientProxiedBy:Content-Type:Content-Language:
Content-Transfer-Encoding;
b=AIbO+yRdmNHn4LV2XE0br8vquXdgTLtrWscElXmWTZiSzrFeRqlATyPGsHleQF3QU
nBcXsRa9tsbQOwgPkyh0nhMBzcV+q6CoKMw4c3CmRhkSXWG6XnQepdpEF4WDC5VJ1j
/kxVKDvmS/WIGEMLowaG/lra0BpLqY9FQLPkCc2up9t94NJ15nHzMx+poYTeVeomWq
x3b9j+KGJesMojeYHF4p02v5kpaquce7dYmP7FjlUMTdEZTgbB46FMu/GynDs3ZPLp
5Jj51SmTeTP/0NR8+K7XjbAFdNc/ux1RzpNITw6FFJ7kmcIImwoKGPat0qKhpN2P6u
J2ThfIZtvw0wg==

On 5/16/2020 4:03 AM, Alex Williamson wrote:
> On Sat, 16 May 2020 02:43:20 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>

<snip>

>> +static int vfio_iova_dirty_bitmap(u64 __user *bitmap, struct vfio_iommu *iommu,
>> + dma_addr_t iova, size_t size, size_t pgsize)
>> +{
>> + struct vfio_dma *dma;
>> + unsigned long pgshift = __ffs(pgsize);
>> + int ret;
>> +
>> + /*
>> + * GET_BITMAP request must fully cover vfio_dma mappings. Multiple
>> + * vfio_dma mappings may be clubbed by specifying large ranges, but
>> + * there must not be any previous mappings bisected by the range.
>> + * An error will be returned if these conditions are not met.
>> + */
>> + dma = vfio_find_dma(iommu, iova, 1);
>> + if (dma && dma->iova != iova)
>> + return -EINVAL;
>> +
>> + dma = vfio_find_dma(iommu, iova + size - 1, 0);
>> + if (dma && dma->iova + dma->size != iova + size)
>> + return -EINVAL;
>> +
>> + dma = vfio_find_dma(iommu, iova, size);
>> +
>> + while (dma && (dma->iova >= iova) &&
>> + (dma->iova + dma->size <= iova + size)) {
>
> Thanks for doing this! Unfortunately I think I've mislead you :(
> But I think there was a bug here in the last version as well, so maybe
> it's all for the better ;)
>
> vfio_find_dma() does not guarantee to find the first dma in the range
> (ie. the lowest iova), it only guarantees to find a dma in the range.
> Since we have a tree structure, as we descend the nodes we might find
> multiple nodes within the range. vfio_find_dma() only returns the first
> occurrence it finds, so we can't assume that other matching nodes are
> next in the tree or that their iovas are greater than the iova of the
> node we found.
>
> All the other use cases of vfio_find_dma() are looking for specific
> pages or boundaries or checking for the existence of a conflict or are
> removing all of the instances within the range, which is probably the
> example that was used in the v20 version of this patch, since it was
> quite similar to vfio_dma_do_unmap() but tried to adjust the size to
> get the next match rather than removing the entry. That could
> potentially lead to an entire unexplored half of the tree making our
> bitmap incomplete.
>
> So I think my initial suggestion[1] on the previous version is probably
> the way we should go. Sorry! OTOH, it would have been a nasty bug to
> find later, it's a subtle semantic that's easy to overlook. Thanks,
>
> Alex
>
> [1]https://lore.kernel.org/kvm/20200514212720.479cc3ba@x1.home/
>

Ok. Got your point.

Replacing
dma = vfio_find_dma(iommu, iova, size);

with below should work

for (n = rb_first(&iommu->dma_list); n; n = rb_next(n)) {
struct vfio_dma *ldma = rb_entry(n, struct vfio_dma, node);

if (ldma->iova >= iova)
break;
}

dma = n ? rb_entry(n, struct vfio_dma, node) : NULL;

Should I update all patches with v22 version? or Is it fine to update
this patch with v21 only?

Thanks,
Kirti