| Message ID | 20221129131235.38880-1-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] vfio/iova_bitmap: refactor iova_bitmap_set() to better handle page boundaries | expand |
Hi, On 29/11/2022 15:12, Joao Martins wrote: > External email: Use caution opening links or attachments > > > 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> > Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > 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> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > Changes since v1: > * Add Reviewed-by by Jason Gunthorpe > * Add Fixes tag (Alex Williamson) > > It passes my tests but to be extra sure: Avihai could you take this > patch a spin in your rig/tests as well? Thanks! Looks good on my side: Tested-by: Avihai Horon <avihaih@nvidia.com> > --- > drivers/vfio/iova_bitmap.c | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) > > diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c > index de6d6ea5c496..0848f920efb7 100644 > --- a/drivers/vfio/iova_bitmap.c > +++ b/drivers/vfio/iova_bitmap.c > @@ -298,9 +298,7 @@ static unsigned long iova_bitmap_mapped_remaining(struct iova_bitmap *bitmap) > { > unsigned long remaining, bytes; > > - /* Cap to one page in the first iteration, if PAGE_SIZE unaligned. */ > - bytes = !bitmap->mapped.pgoff ? bitmap->mapped.npages << PAGE_SHIFT : > - PAGE_SIZE - bitmap->mapped.pgoff; > + bytes = (bitmap->mapped.npages << PAGE_SHIFT) - bitmap->mapped.pgoff; > > remaining = bitmap->mapped_total_index - bitmap->mapped_base_index; > remaining = min_t(unsigned long, remaining, > @@ -399,29 +397,27 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, > * Set the bits corresponding to the range [iova .. iova+length-1] in > * the user bitmap. > * > - * Return: The number of bits set. > */ > void iova_bitmap_set(struct iova_bitmap *bitmap, > unsigned long iova, size_t length) > { > struct iova_bitmap_map *mapped = &bitmap->mapped; > - unsigned long offset = (iova - mapped->iova) >> mapped->pgshift; > - unsigned long nbits = max_t(unsigned long, 1, length >> mapped->pgshift); > - unsigned long page_idx = offset / BITS_PER_PAGE; > - unsigned long page_offset = mapped->pgoff; > - void *kaddr; > - > - offset = offset % BITS_PER_PAGE; > + unsigned long cur_bit = ((iova - mapped->iova) >> > + mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE; > + unsigned long last_bit = (((iova + length - 1) - mapped->iova) >> > + mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE; > > do { > - unsigned long size = min(BITS_PER_PAGE - offset, nbits); > + unsigned int page_idx = cur_bit / BITS_PER_PAGE; > + unsigned int offset = cur_bit % BITS_PER_PAGE; > + unsigned int nbits = min(BITS_PER_PAGE - offset, > + last_bit - cur_bit + 1); > + void *kaddr; > > kaddr = kmap_local_page(mapped->pages[page_idx]); > - bitmap_set(kaddr + page_offset, offset, size); > + bitmap_set(kaddr, offset, nbits); > kunmap_local(kaddr); > - page_offset = offset = 0; > - nbits -= size; > - page_idx++; > - } while (nbits > 0); > + cur_bit += nbits; > + } while (cur_bit <= last_bit); > } > EXPORT_SYMBOL_GPL(iova_bitmap_set); > -- > 2.17.2 >
On Tue, 29 Nov 2022 13:12:35 +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> > Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") > 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> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > Changes since v1: > * Add Reviewed-by by Jason Gunthorpe > * Add Fixes tag (Alex Williamson) > > It passes my tests but to be extra sure: Avihai could you take this > patch a spin in your rig/tests as well? Thanks! > --- > drivers/vfio/iova_bitmap.c | 30 +++++++++++++----------------- > 1 file changed, 13 insertions(+), 17 deletions(-) Applied to vfio next branch for v6.2 with Avihai's tested-by. Thanks, Alex
diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c index de6d6ea5c496..0848f920efb7 100644 --- a/drivers/vfio/iova_bitmap.c +++ b/drivers/vfio/iova_bitmap.c @@ -298,9 +298,7 @@ static unsigned long iova_bitmap_mapped_remaining(struct iova_bitmap *bitmap) { unsigned long remaining, bytes; - /* Cap to one page in the first iteration, if PAGE_SIZE unaligned. */ - bytes = !bitmap->mapped.pgoff ? bitmap->mapped.npages << PAGE_SHIFT : - PAGE_SIZE - bitmap->mapped.pgoff; + bytes = (bitmap->mapped.npages << PAGE_SHIFT) - bitmap->mapped.pgoff; remaining = bitmap->mapped_total_index - bitmap->mapped_base_index; remaining = min_t(unsigned long, remaining, @@ -399,29 +397,27 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque, * Set the bits corresponding to the range [iova .. iova+length-1] in * the user bitmap. * - * Return: The number of bits set. */ void iova_bitmap_set(struct iova_bitmap *bitmap, unsigned long iova, size_t length) { struct iova_bitmap_map *mapped = &bitmap->mapped; - unsigned long offset = (iova - mapped->iova) >> mapped->pgshift; - unsigned long nbits = max_t(unsigned long, 1, length >> mapped->pgshift); - unsigned long page_idx = offset / BITS_PER_PAGE; - unsigned long page_offset = mapped->pgoff; - void *kaddr; - - offset = offset % BITS_PER_PAGE; + unsigned long cur_bit = ((iova - mapped->iova) >> + mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE; + unsigned long last_bit = (((iova + length - 1) - mapped->iova) >> + mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE; do { - unsigned long size = min(BITS_PER_PAGE - offset, nbits); + unsigned int page_idx = cur_bit / BITS_PER_PAGE; + unsigned int offset = cur_bit % BITS_PER_PAGE; + unsigned int nbits = min(BITS_PER_PAGE - offset, + last_bit - cur_bit + 1); + void *kaddr; kaddr = kmap_local_page(mapped->pages[page_idx]); - bitmap_set(kaddr + page_offset, offset, size); + bitmap_set(kaddr, offset, nbits); kunmap_local(kaddr); - page_offset = offset = 0; - nbits -= size; - page_idx++; - } while (nbits > 0); + cur_bit += nbits; + } while (cur_bit <= last_bit); } EXPORT_SYMBOL_GPL(iova_bitmap_set);
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> Fixes: f38044e5ef58 ("vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps") 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> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> --- Changes since v1: * Add Reviewed-by by Jason Gunthorpe * Add Fixes tag (Alex Williamson) It passes my tests but to be extra sure: Avihai could you take this patch a spin in your rig/tests as well? Thanks! --- drivers/vfio/iova_bitmap.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)