Re: [PATCH] vfio/iova_bitmap: refactor iova_bitmap_set() to better handle page boundaries

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 28 Nov 2022 19:22:10 +0000
Joao Martins <joao.m.martins@oracle.com> wrote:

> On 28/11/2022 19:12, Alex Williamson wrote:
> > On Fri, 25 Nov 2022 17:29:56 +0000
> > Joao Martins <joao.m.martins@oracle.com> wrote:
> >   
> >> Commit f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps")
> >> had fixed the unaligned bitmaps by capping the remaining iterable set at
> >> the start of the bitmap. Although, that mistakenly worked around
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> iova_bitmap_set() incorrectly setting bits across page boundary.
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> Fix this by reworking the loop inside iova_bitmap_set() to iterate over a
     ^^^^^^^^^^^...
> >> range of bits to set (cur_bit .. last_bit) which may span different pinned
> >> pages, thus updating @page_idx and @offset as it sets the bits. The
> >> previous cap to the first page is now adjusted to be always accounted
> >> rather than when there's only a non-zero pgoff.
> >>
> >> While at it, make @page_idx , @offset and @nbits to be unsigned int given
> >> that it won't be more than 512 and 4096 respectively (even a bigger
> >> PAGE_SIZE or a smaller struct page size won't make this bigger than the
> >> above 32-bit max). Also, delete the stale kdoc on Return type.
> >>
> >> Cc: Avihai Horon <avihaih@nvidia.com>
> >> Co-developed-by: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>  
> > 
> > Should this have:
> > 
> > Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps")
> > 
> > ?  
> 
> I was at two minds with the Fixes tag.
> 
> The commit you referenced above is still a fix (or workaround), this patch is a
> better fix that superseeds as opposed to fixing a bug that commit f38044e5ef58
> introduced. So perhaps the right one ought to be:
> 
> Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support")

The above highlighted text certainly suggests that there's a fix to
f38044e5ef58 here.  We might still be iterating on a problem that was
originally introduced in 58ccf0190d19, but this more directly addresses
the version of that problem that exists after f38044e5ef58.  I think
it's more helpful for backporters to see this progression rather than
two patches claiming to fix the same commit with one depending on
another.  If you'd rather that stable have a different backport that
short circuits the interim fix in f38044e5ef58, that could be posted
separately, but imo it's better to follow the mainline progression.
Thanks,

Alex