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@xxxxxxxxxx>
>> Tested-by: Avihai Horon <avihaih@xxxxxxxxxx>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>> 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.
