* [PATCH v1 0/2] vfio/iova_bitmap: bug fixes
@ 2022-10-25 19:31 Joao Martins
2022-10-25 19:31 ` [PATCH v1 1/2] vfio/iova_bitmap: Explicitly include linux/slab.h Joao Martins
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Joao Martins @ 2022-10-25 19:31 UTC (permalink / raw)
To: kvm; +Cc: Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas
Hey,
This small series addresses one small improvement and one bug fix:
1) Avoid obscurity into how I use slab helpers yet I was
never include it directly. And it wasn't detected thanks to a unrelated
commit in v5.18 that lead linux/mm.h to indirectly include slab.h.
2) A bug reported by Avihai reported during migration testing where
we weren't handling correctly PAGE_SIZE unaligned bitmaps, which lead
to miss dirty marked correctly on some IOVAs.
Comments appreciated
Thanks,
Joao
Joao Martins (2):
vfio/iova_bitmap: Explicitly include linux/slab.h
vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps
drivers/vfio/iova_bitmap.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
--
2.17.2
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v1 1/2] vfio/iova_bitmap: Explicitly include linux/slab.h 2022-10-25 19:31 [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins @ 2022-10-25 19:31 ` Joao Martins 2022-10-25 19:31 ` [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps Joao Martins ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Joao Martins @ 2022-10-25 19:31 UTC (permalink / raw) To: kvm; +Cc: Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas kzalloc/kzfree are used so include `slab.h`. While it happens to work without it, due to commit 8b9f3ac5b01d ("fs: introduce alloc_inode_sb() to allocate filesystems specific inode") which indirectly includes via: . ./include/linux/mm.h .. ./include/linux/huge_mm.h ... ./include/linux/fs.h .... ./include/linux/slab.h Make it explicit should any of its indirect dependencies be dropped/changed for entirely different reasons as it was the cause prior to commit above recently (i.e. <= v5.18). Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- drivers/vfio/iova_bitmap.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c index c20ffce1a0fd..389f36cae355 100644 --- a/drivers/vfio/iova_bitmap.c +++ b/drivers/vfio/iova_bitmap.c @@ -5,6 +5,7 @@ */ #include <linux/iova_bitmap.h> #include <linux/mm.h> +#include <linux/slab.h> #include <linux/highmem.h> #define BITS_PER_PAGE (PAGE_SIZE * BITS_PER_BYTE) -- 2.17.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps 2022-10-25 19:31 [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins 2022-10-25 19:31 ` [PATCH v1 1/2] vfio/iova_bitmap: Explicitly include linux/slab.h Joao Martins @ 2022-10-25 19:31 ` Joao Martins 2022-11-24 18:20 ` Jason Gunthorpe 2022-11-09 20:09 ` [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins 2022-11-10 20:16 ` Alex Williamson 3 siblings, 1 reply; 9+ messages in thread From: Joao Martins @ 2022-10-25 19:31 UTC (permalink / raw) To: kvm; +Cc: Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas iova_bitmap_set() doesn't consider the end of the page boundary when the first bitmap page offset isn't zero, and wrongly changes the consecutive page right after. Consequently this leads to missing dirty pages from reported by the device as seen from the VMM. The current logic iterates over a given number of base pages and clamps it to the remaining indexes to iterate in the last page. Instead of having to consider extra pages to pin (e.g. first and extra pages), just handle the first page as its own range and let the rest of the bitmap be handled as if it was base page aligned. This is done by changing iova_bitmap_mapped_remaining() to return PAGE_SIZE - pgoff (on the first bitmap page), and leads to pgoff being set to 0 on following iterations. Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") Reported-by: Avihai Horon <avihaih@nvidia.com> Tested-by: Avihai Horon <avihaih@nvidia.com> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- I have a small test suite that I have been using for functional and some performance tests; I try to cover all the edge cases. Though I happened to miss having a test case (which is also fixed) ... leading to this bug. I wonder if this test suite is something of interest to have in tree? --- drivers/vfio/iova_bitmap.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/iova_bitmap.c b/drivers/vfio/iova_bitmap.c index 389f36cae355..40463c51da31 100644 --- a/drivers/vfio/iova_bitmap.c +++ b/drivers/vfio/iova_bitmap.c @@ -296,11 +296,15 @@ void iova_bitmap_free(struct iova_bitmap *bitmap) */ static unsigned long iova_bitmap_mapped_remaining(struct iova_bitmap *bitmap) { - unsigned long remaining; + 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; remaining = bitmap->mapped_total_index - bitmap->mapped_base_index; remaining = min_t(unsigned long, remaining, - (bitmap->mapped.npages << PAGE_SHIFT) / sizeof(*bitmap->bitmap)); + bytes / sizeof(*bitmap->bitmap)); return remaining; } -- 2.17.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps 2022-10-25 19:31 ` [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps Joao Martins @ 2022-11-24 18:20 ` Jason Gunthorpe 2022-11-25 10:37 ` Joao Martins 0 siblings, 1 reply; 9+ messages in thread From: Jason Gunthorpe @ 2022-11-24 18:20 UTC (permalink / raw) To: Joao Martins Cc: kvm, Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas On Tue, Oct 25, 2022 at 08:31:14PM +0100, Joao Martins wrote: > iova_bitmap_set() doesn't consider the end of the page boundary when the > first bitmap page offset isn't zero, and wrongly changes the consecutive > page right after. Consequently this leads to missing dirty pages from > reported by the device as seen from the VMM. > > The current logic iterates over a given number of base pages and clamps it > to the remaining indexes to iterate in the last page. Instead of having to > consider extra pages to pin (e.g. first and extra pages), just handle the > first page as its own range and let the rest of the bitmap be handled as if > it was base page aligned. > > This is done by changing iova_bitmap_mapped_remaining() to return PAGE_SIZE > - pgoff (on the first bitmap page), and leads to pgoff being set to 0 on > following iterations. > > Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") > Reported-by: Avihai Horon <avihaih@nvidia.com> > Tested-by: Avihai Horon <avihaih@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > I have a small test suite that I have been using for functional and some > performance tests; I try to cover all the edge cases. Though I happened to > miss having a test case (which is also fixed) ... leading to this bug. > I wonder if this test suite is something of interest to have in > tree? Yes, when we move this to iommufd the test suite should be included, either as integrated using the mock domain and the selftests or otherwise. This is really a problem in iova_bitmap_set(), it isn't doing the math right. > + /* Cap to one page in the first iteration, if PAGE_SIZE unaligned. */ Why isn't it just bitmap->mapped.npages * PAGE_SIZE - bitmap->mapped.pgoff And fix the bug in iova_bitmap_set: void iova_bitmap_set(struct iova_bitmap *bitmap, unsigned long iova, size_t length) { struct iova_bitmap_map *mapped = &bitmap->mapped; unsigned cur_bit = ((iova - mapped->iova) >> mapped->pgshift) + mapped->pgoff * 8; unsigned long last_bit = (((iova + length - 1) - mapped->iova) >> mapped->pgshift) + mapped->pgoff * 8; do { unsigned int page_idx = cur_bit / BITS_PER_PAGE; unsigned int nbits = min(BITS_PER_PAGE - cur_bit, last_bit - cur_bit + 1); void *kaddr; kaddr = kmap_local_page(mapped->pages[page_idx]); bitmap_set(kaddr, cur_bit % BITS_PER_PAGE, nbits); kunmap_local(kaddr); cur_bit += nbits; } while (cur_bit <= last_bit); } EXPORT_SYMBOL_GPL(iova_bitmap_set); Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps 2022-11-24 18:20 ` Jason Gunthorpe @ 2022-11-25 10:37 ` Joao Martins 2022-11-25 12:42 ` Jason Gunthorpe 0 siblings, 1 reply; 9+ messages in thread From: Joao Martins @ 2022-11-25 10:37 UTC (permalink / raw) To: Jason Gunthorpe Cc: kvm, Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas On 24/11/2022 18:20, Jason Gunthorpe wrote: > On Tue, Oct 25, 2022 at 08:31:14PM +0100, Joao Martins wrote: >> iova_bitmap_set() doesn't consider the end of the page boundary when the >> first bitmap page offset isn't zero, and wrongly changes the consecutive >> page right after. Consequently this leads to missing dirty pages from >> reported by the device as seen from the VMM. >> >> The current logic iterates over a given number of base pages and clamps it >> to the remaining indexes to iterate in the last page. Instead of having to >> consider extra pages to pin (e.g. first and extra pages), just handle the >> first page as its own range and let the rest of the bitmap be handled as if >> it was base page aligned. >> >> This is done by changing iova_bitmap_mapped_remaining() to return PAGE_SIZE >> - pgoff (on the first bitmap page), and leads to pgoff being set to 0 on >> following iterations. >> >> Fixes: 58ccf0190d19 ("vfio: Add an IOVA bitmap support") >> Reported-by: Avihai Horon <avihaih@nvidia.com> >> Tested-by: Avihai Horon <avihaih@nvidia.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> I have a small test suite that I have been using for functional and some >> performance tests; I try to cover all the edge cases. Though I happened to >> miss having a test case (which is also fixed) ... leading to this bug. >> I wonder if this test suite is something of interest to have in >> tree? > > Yes, when we move this to iommufd the test suite should be included, > either as integrated using the mock domain and the selftests or > otherwise. > So in iommufd counterpart I have already tests which exercise this. But not as extensive. This testsuite is a bit more focused on all the functions in iova_bitmap and making sure I iterate the ranges right with small and huge bitmaps (up to 4T of IOVA space bitmaps), and testing that all the bits are set as expected, etc. I'll see if I merge both of these, there's probably no need for two separate units in the testsuite. The only test infra need for standalone iova-bitmap tests was the fact that I use GUP, and so I couldn't but this enterily in userspace. > This is really a problem in iova_bitmap_set(), it isn't doing the math > right. > Ack > >> + /* Cap to one page in the first iteration, if PAGE_SIZE unaligned. */ > > Why isn't it just > > bitmap->mapped.npages * PAGE_SIZE - bitmap->mapped.pgoff > This would return a higher than necessary number of indexes. But in fact the logic currently is correct and if we instead fix iova_bitmap_set() this new capping I was doing isn't needed at all. > And fix the bug in iova_bitmap_set: > This is right. I'm not handling cross page boundaries right in iova_bitmap_set() > void iova_bitmap_set(struct iova_bitmap *bitmap, > unsigned long iova, size_t length) > { > struct iova_bitmap_map *mapped = &bitmap->mapped; > unsigned cur_bit = > ((iova - mapped->iova) >> mapped->pgshift) + mapped->pgoff * 8; > unsigned long last_bit = > (((iova + length - 1) - mapped->iova) >> mapped->pgshift) + > mapped->pgoff * 8; > > do { > unsigned int page_idx = cur_bit / BITS_PER_PAGE; > unsigned int nbits = > min(BITS_PER_PAGE - cur_bit, last_bit - cur_bit + 1); > void *kaddr; > > kaddr = kmap_local_page(mapped->pages[page_idx]); > bitmap_set(kaddr, cur_bit % BITS_PER_PAGE, nbits); > kunmap_local(kaddr); > cur_bit += nbits; > } while (cur_bit <= last_bit); > } > EXPORT_SYMBOL_GPL(iova_bitmap_set); > Let me test this and respin. Not sure if the vfio tree is a rebasing tree (or not?) and can just send a new version, or whether I should explicitly revert the already commited one and apply a new version. I'll do a v2 of this patch and wait for Alex direction on where to go on the v2 patch in case I need to go via the latter. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps 2022-11-25 10:37 ` Joao Martins @ 2022-11-25 12:42 ` Jason Gunthorpe 2022-11-25 14:31 ` Joao Martins 0 siblings, 1 reply; 9+ messages in thread From: Jason Gunthorpe @ 2022-11-25 12:42 UTC (permalink / raw) To: Joao Martins Cc: kvm, Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas On Fri, Nov 25, 2022 at 10:37:39AM +0000, Joao Martins wrote: > > Yes, when we move this to iommufd the test suite should be included, > > either as integrated using the mock domain and the selftests or > > otherwise. > > So in iommufd counterpart I have already tests which exercise this. But not as > extensive. We are getting to the point where we should start posting the iommufd dirty tracking stuff. Do you have time to work on it for the next cycle? Meaning get it largely sorted out in the next 3 weeks for review? > > void iova_bitmap_set(struct iova_bitmap *bitmap, > > unsigned long iova, size_t length) > > { > > struct iova_bitmap_map *mapped = &bitmap->mapped; > > unsigned cur_bit = > > ((iova - mapped->iova) >> mapped->pgshift) + mapped->pgoff * 8; > > unsigned long last_bit = > > (((iova + length - 1) - mapped->iova) >> mapped->pgshift) + > > mapped->pgoff * 8; > > > > do { > > unsigned int page_idx = cur_bit / BITS_PER_PAGE; > > unsigned int nbits = > > min(BITS_PER_PAGE - cur_bit, last_bit - cur_bit + 1); min(BITS_PER_PAGE - (cur_bit % BITS_PER_PAGE), ...) > Not sure if the vfio tree is a rebasing tree (or not?) and can just send a new > version, It isn't, you should just post a new patch on top of Alex's current tree "rework iova_bitmap_et to handle all page crossings" and along the way revert the first bit Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps 2022-11-25 12:42 ` Jason Gunthorpe @ 2022-11-25 14:31 ` Joao Martins 0 siblings, 0 replies; 9+ messages in thread From: Joao Martins @ 2022-11-25 14:31 UTC (permalink / raw) To: Jason Gunthorpe Cc: kvm, Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas On 25/11/2022 12:42, Jason Gunthorpe wrote: > On Fri, Nov 25, 2022 at 10:37:39AM +0000, Joao Martins wrote: > >>> Yes, when we move this to iommufd the test suite should be included, >>> either as integrated using the mock domain and the selftests or >>> otherwise. >> >> So in iommufd counterpart I have already tests which exercise this. But not as >> extensive. > > We are getting to the point where we should start posting the iommufd > dirty tracking stuff. Do you have time to work on it for the next > cycle? Meaning get it largely sorted out in the next 3 weeks for review? > I'll post it for the next cycle -- It has been a bit on crazy on my end this past month or so. >>> void iova_bitmap_set(struct iova_bitmap *bitmap, >>> unsigned long iova, size_t length) >>> { >>> struct iova_bitmap_map *mapped = &bitmap->mapped; >>> unsigned cur_bit = >>> ((iova - mapped->iova) >> mapped->pgshift) + mapped->pgoff * 8; >>> unsigned long last_bit = >>> (((iova + length - 1) - mapped->iova) >> mapped->pgshift) + >>> mapped->pgoff * 8; >>> >>> do { >>> unsigned int page_idx = cur_bit / BITS_PER_PAGE; >>> unsigned int nbits = >>> min(BITS_PER_PAGE - cur_bit, last_bit - cur_bit + 1); > > min(BITS_PER_PAGE - (cur_bit % BITS_PER_PAGE), ...) > I actually had this already in my changeset :) as the earlier snip wasn't passing my tests. Plus I need to account for less indexes with pgoff, contrary to what I said earlier in the remaining() function calculation. >> Not sure if the vfio tree is a rebasing tree (or not?) and can just send a new >> version, > > It isn't, you should just post a new patch on top of Alex's current > tree "rework iova_bitmap_et to handle all page crossings" and along > the way revert the first bit OK -- makes sense. The other fix wasn't incorrect either (as we need to account for pgoff on the same function), this one though fixes the real issue of iova_bitmap_set(). Also, I'll add your Signed-off-by+Co-developed-by -- let me know otherwise if I should not. Joao ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/2] vfio/iova_bitmap: bug fixes 2022-10-25 19:31 [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins 2022-10-25 19:31 ` [PATCH v1 1/2] vfio/iova_bitmap: Explicitly include linux/slab.h Joao Martins 2022-10-25 19:31 ` [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps Joao Martins @ 2022-11-09 20:09 ` Joao Martins 2022-11-10 20:16 ` Alex Williamson 3 siblings, 0 replies; 9+ messages in thread From: Joao Martins @ 2022-11-09 20:09 UTC (permalink / raw) To: kvm; +Cc: Alex Williamson, Cornelia Huck, Avihai Horon, Yishai Hadas On 25/10/2022 20:31, Joao Martins wrote: > Hey, > > This small series addresses one small improvement and one bug fix: > > 1) Avoid obscurity into how I use slab helpers yet I was > never include it directly. And it wasn't detected thanks to a unrelated > commit in v5.18 that lead linux/mm.h to indirectly include slab.h. > > 2) A bug reported by Avihai reported during migration testing where > we weren't handling correctly PAGE_SIZE unaligned bitmaps, which lead > to miss dirty marked correctly on some IOVAs. > > Comments appreciated > Ping? > Thanks, > Joao > > Joao Martins (2): > vfio/iova_bitmap: Explicitly include linux/slab.h > vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps > > drivers/vfio/iova_bitmap.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/2] vfio/iova_bitmap: bug fixes 2022-10-25 19:31 [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins ` (2 preceding siblings ...) 2022-11-09 20:09 ` [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins @ 2022-11-10 20:16 ` Alex Williamson 3 siblings, 0 replies; 9+ messages in thread From: Alex Williamson @ 2022-11-10 20:16 UTC (permalink / raw) To: Joao Martins; +Cc: kvm, Cornelia Huck, Avihai Horon, Yishai Hadas On Tue, 25 Oct 2022 20:31:12 +0100 Joao Martins <joao.m.martins@oracle.com> wrote: > Hey, > > This small series addresses one small improvement and one bug fix: > > 1) Avoid obscurity into how I use slab helpers yet I was > never include it directly. And it wasn't detected thanks to a unrelated > commit in v5.18 that lead linux/mm.h to indirectly include slab.h. > > 2) A bug reported by Avihai reported during migration testing where > we weren't handling correctly PAGE_SIZE unaligned bitmaps, which lead > to miss dirty marked correctly on some IOVAs. > > Comments appreciated > > Thanks, > Joao > > Joao Martins (2): > vfio/iova_bitmap: Explicitly include linux/slab.h > vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps > > drivers/vfio/iova_bitmap.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > Applied to vfio next branch for v6.2. Thanks, Alex ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-25 14:31 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-25 19:31 [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins 2022-10-25 19:31 ` [PATCH v1 1/2] vfio/iova_bitmap: Explicitly include linux/slab.h Joao Martins 2022-10-25 19:31 ` [PATCH v1 2/2] vfio/iova_bitmap: Fix PAGE_SIZE unaligned bitmaps Joao Martins 2022-11-24 18:20 ` Jason Gunthorpe 2022-11-25 10:37 ` Joao Martins 2022-11-25 12:42 ` Jason Gunthorpe 2022-11-25 14:31 ` Joao Martins 2022-11-09 20:09 ` [PATCH v1 0/2] vfio/iova_bitmap: bug fixes Joao Martins 2022-11-10 20:16 ` Alex Williamson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.