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

[Kirti Wankhede]

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 <address@hidden> 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/address@hidden/

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