[PATCH v1 00/10] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests

[PATCH v1 00/10] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests

[Joao Martins]

Hey,

This series purpose is to address what I thought was a complete fix[0].  It
also fixes arm64 64k base page cases that are broken right now with tiny
bitmap sizes (<= 128K IOVA ranges). It also makes selftest read_and_clear()
better iommu drivers and cover new bitmap sizes.

I go in details in the patch 9 but essentially over tons of live migrations
I find out two leftover issues that I failed to fix in my previous
series. The dirty case that that crosses iterations was indeed fixed by
[1], but it's the non dirty that was still generating issues:

a) Essentially, lack of alignment in iterations begin/end leads to checking
two times the same IOPTE for dirty bits reintroducing the 'partial
hugepage' check edge case. Though on big bitmaps (for a 1TB case which is
32MB bitmaps) the time taken pinning iterations can lead for the IOPTE
being dirty in the second time it is checked which makes it miss part of
the dirty page to be sent on the migration stream.

b) when we hit this hugepage represented across iterations we start
checking IOVAs being dirty slightly offsetted leading to set the wrong bits
when we find something dirty later on. This is only visible in >64G iova
ranges and PAGE_SIZE unaligned bitmaps.

The series is organized as:

* Patches 1-3: Fixes the 64k arm64 broken selftests and introduces two
more edge cases to cover the equivalent in 4k base pages.

* Patch 4: Fix selftest to match better the current iommu dirty
trackers, which was hiding the brokeness of (b) above. I have used the snip
below[*] to make sure all is passed, but I don't add it because testing
 >=64G takes longer than the selftests timeout tolerates. Furthermore this
is only relevant with the fixed window scheme which I remove in this series
and make it dynamic.

* Patches 5-10: Patches to prep for patch 9, that leads into removing
ithe partial hugepage represented across iterations that is self-induced.
upper layers not knowing what page size is for a given IOVA maps, but more
importantly because IOVA bitmaps deal with fixed 64G range passed into
read_and_clear(). To avoid further issues with this fixed scheme I redid
the fix and make it dynamic the iteration scheme i.e. pin the next 64G and
repin it everytime a dirty-size can't fit into the mapped pages without
requiring callers needing to work in a limited IOVA range (aka we can pass
read_and_clear() the whole thing and we pin the needed bitmap pages).
Additionally, because non dirty IOVA ranges don't need to get anything
pinned, it makes non-dirty cases very cheap as a bonus. I couldn't find a
measurable difference between dynamic vs fixed pinning, so I've removed it
in patch 10. It also makes the code simpler to follow & easier to maintain.
If folks agree with the approach I've taken here I can remove the callers
of iova_bitmap_for_each() in a follow-up cleanup series.

This series survives 1700 migrations of guests with applications doing RDMA
(which is roughly 3-days spent migration back and forth between hosts).
Additionally like the series in [1] I used a set of changes/tool to
exercise some dirty tracking tests but with real iommu drivers. Finally
tested >=64G IOMMU page sizes using this tool but with device-dax memory
(to generate >=64G contiguous RAM) to tackle the case of patch 9 when
iova_bitmap_set() gets called with enourmous page sizes that surprasse or
is equal to the maximum pin set of 64G.

Comments, review as usual appreciated.

	Joao

Joao Martins (10):
  iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps
  iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps
  iommufd/selftest: Add tests for <= u8 bitmap sizes
  iommufd/selftest: Do not record head iova to better match iommu drivers
  iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead
  iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct
  iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each()
  iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals
  iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()
  iommufd/iova_bitmap: Remove iterator logic

 drivers/iommu/iommufd/iova_bitmap.c           | 124 +++++++-----------
 drivers/iommu/iommufd/selftest.c              |   6 +-
 tools/testing/selftests/iommu/iommufd.c       |  28 +++-
 tools/testing/selftests/iommu/iommufd_utils.h |   6 +-
 4 files changed, 79 insertions(+), 85 deletions(-)

[0] https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-ed54f1351a9a@oracle.com/
[1] https://lore.kernel.org/linux-iommu/20240202133415.23819-1-joao.m.martins@oracle.com/

[*] Adds 72G test and adds the offset=0x1 which exercises both (b)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index e854d3f67205..171d3e437356 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -26,7 +26,7 @@ enum {

 enum {
        MOCK_APERTURE_START = 1UL << 24,
-       MOCK_APERTURE_LAST = (1UL << 31) - 1,
+       MOCK_APERTURE_LAST = (1UL << 37) - 1,
 };

 enum {
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 61189215e1ab..61770e31e66a 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -1841,6 +1841,13 @@ FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256M_huge)
        .hugepages = true,
 };

+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty72G_huge)
+{
+       /* 2304K bitmap (72G IOVA range) */
+       .buffer_size = 72 * 1024UL * 1024UL * 1024UL,
+       .hugepages = true,
+};
+
 TEST_F(iommufd_dirty_tracking, enforce_dirty)
 {
        uint32_t ioas_id, stddev_id, idev_id;
@@ -1898,7 +1905,7 @@ TEST_F(iommufd_dirty_tracking, device_dirty_capability)
        test_ioctl_destroy(hwpt_id);
 }
-TEST_F(iommufd_dirty_tracking, get_dirty_bitmap)
+TEST_F_TIMEOUT(iommufd_dirty_tracking, get_dirty_bitmap, 4095)
 {
        uint32_t page_size = MOCK_PAGE_SIZE;
        uint32_t hwpt_id;
@@ -1932,10 +1939,16 @@ TEST_F(iommufd_dirty_tracking, get_dirty_bitmap)
                                self->bitmap + 0xff1, self->bitmap_size, 0,
                                _metadata);

+       /* broken non-dirty page cross iterations (>= 64G tests) */
+       test_mock_dirty_bitmaps(hwpt_id, variant->buffer_size,
+                               MOCK_APERTURE_START, self->page_size, page_size,
+                               self->bitmap + 0x1, self->bitmap_size, 0,
+                               _metadata);
+
        test_ioctl_destroy(hwpt_id);
 }

-TEST_F(iommufd_dirty_tracking, get_dirty_bitmap_no_clear)
+TEST_F_TIMEOUT(iommufd_dirty_tracking, get_dirty_bitmap_no_clear, 4095)
 {
        uint32_t page_size = MOCK_PAGE_SIZE;
        uint32_t hwpt_id;
@@ -1967,13 +1980,20 @@ TEST_F(iommufd_dirty_tracking, get_dirty_bitmap_no_clear)
                                IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR,
                                _metadata);

-       /* u64 unaligned bitmap */
+       /* u8 unaligned bitmap */
        test_mock_dirty_bitmaps(hwpt_id, variant->buffer_size,
                                MOCK_APERTURE_START, self->page_size, page_size,
                                self->bitmap + 0xff1, self->bitmap_size,
                                IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR,
                                _metadata);

+       /* broken non-dirty page cross iterations (>= 64G tests) */
+       test_mock_dirty_bitmaps(hwpt_id, variant->buffer_size,
+                               MOCK_APERTURE_START, self->page_size, page_size,
+                               self->bitmap + 0x1, self->bitmap_size,
+                               IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR,
+                               _metadata);
+
        test_ioctl_destroy(hwpt_id);
 }

-- 
2.17.2

[PATCH v1 01/10] iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps

[Joao Martins]

With 64k base pages, the first 128k iova length test requires less than a
byte for a bitmap, exposing a bug in the tests that assume that bitmaps are
at least a byte.

Rather than dealing with bytes, have _test_mock_dirty_bitmaps() pass the
number of bits. The caller functions are adjusted to also use bits as well,
and converting to bytes when clearing, allocating and freeing the bitmap.

Reported-by: Matt Ochs <mochs@nvidia.com>
Fixes: a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 tools/testing/selftests/iommu/iommufd.c       | 10 +++++-----
 tools/testing/selftests/iommu/iommufd_utils.h |  6 ++++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index edf1c99c9936..0b04d782a19f 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -1722,6 +1722,7 @@ FIXTURE_VARIANT(iommufd_dirty_tracking)
 
 FIXTURE_SETUP(iommufd_dirty_tracking)
 {
+	unsigned long size;
 	int mmap_flags;
 	void *vrc;
 	int rc;
@@ -1749,12 +1750,11 @@ FIXTURE_SETUP(iommufd_dirty_tracking)
 	assert(vrc == self->buffer);
 
 	self->page_size = MOCK_PAGE_SIZE;
-	self->bitmap_size =
-		variant->buffer_size / self->page_size / BITS_PER_BYTE;
+	self->bitmap_size = variant->buffer_size / self->page_size;
 
 	/* Provision with an extra (PAGE_SIZE) for the unaligned case */
-	rc = posix_memalign(&self->bitmap, PAGE_SIZE,
-			    self->bitmap_size + PAGE_SIZE);
+	size = DIV_ROUND_UP(self->bitmap_size, BITS_PER_BYTE);
+	rc = posix_memalign(&self->bitmap, PAGE_SIZE, size + PAGE_SIZE);
 	assert(!rc);
 	assert(self->bitmap);
 	assert((uintptr_t)self->bitmap % PAGE_SIZE == 0);
@@ -1775,7 +1775,7 @@ FIXTURE_SETUP(iommufd_dirty_tracking)
 FIXTURE_TEARDOWN(iommufd_dirty_tracking)
 {
 	munmap(self->buffer, variant->buffer_size);
-	munmap(self->bitmap, self->bitmap_size);
+	munmap(self->bitmap, DIV_ROUND_UP(self->bitmap_size, BITS_PER_BYTE));
 	teardown_iommufd(self->fd, _metadata);
 }
 
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index 8d2b46b2114d..c612fbf0195b 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -22,6 +22,8 @@
 #define BIT_MASK(nr) (1UL << ((nr) % __BITS_PER_LONG))
 #define BIT_WORD(nr) ((nr) / __BITS_PER_LONG)
 
+#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
 static inline void set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
@@ -346,12 +348,12 @@ static int _test_cmd_mock_domain_set_dirty(int fd, __u32 hwpt_id, size_t length,
 static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length,
 				    __u64 iova, size_t page_size,
 				    size_t pte_page_size, __u64 *bitmap,
-				    __u64 bitmap_size, __u32 flags,
+				    __u64 nbits, __u32 flags,
 				    struct __test_metadata *_metadata)
 {
 	unsigned long npte = pte_page_size / page_size, pteset = 2 * npte;
-	unsigned long nbits = bitmap_size * BITS_PER_BYTE;
 	unsigned long j, i, nr = nbits / pteset ?: 1;
+	unsigned long bitmap_size = DIV_ROUND_UP(nbits, BITS_PER_BYTE);
 	__u64 out_dirty = 0;
 
 	/* Mark all even bits as dirty in the mock domain */
-- 
2.17.2

[PATCH v1 02/10] iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps

[Joao Martins]

The calculation returns 0 if it sets less than the number of bits per
byte. For calculating memory allocation from bits, lets round it up to
one byte.

Reported-by: Matt Ochs <mochs@nvidia.com>
Fixes: a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/selftest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 97ce62602e66..fea60db05c9f 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -1356,7 +1356,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 	}
 
 	max = length / page_size;
-	bitmap_size = max / BITS_PER_BYTE;
+	bitmap_size = DIV_ROUND_UP(max, BITS_PER_BYTE);
 
 	tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT);
 	if (!tmp) {
-- 
2.17.2

[PATCH v1 03/10] iommufd/selftest: Add tests for <= u8 bitmap sizes

[Joao Martins]

Add more tests for bitmaps smaller than or equal to an u8, though skip the
tests if the IOVA buffer size is smaller than the mock page size.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 tools/testing/selftests/iommu/iommufd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 0b04d782a19f..61189215e1ab 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -1727,6 +1727,12 @@ FIXTURE_SETUP(iommufd_dirty_tracking)
 	void *vrc;
 	int rc;
 
+	if (variant->buffer_size < MOCK_PAGE_SIZE) {
+		SKIP(return,
+		     "Skipping buffer_size=%lu, less than MOCK_PAGE_SIZE=%lu",
+		     variant->buffer_size, MOCK_PAGE_SIZE);
+	}
+
 	self->fd = open("/dev/iommu", O_RDWR);
 	ASSERT_NE(-1, self->fd);
 
@@ -1779,6 +1785,18 @@ FIXTURE_TEARDOWN(iommufd_dirty_tracking)
 	teardown_iommufd(self->fd, _metadata);
 }
 
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty8k)
+{
+	/* half of an u8 index bitmap */
+	.buffer_size = 8UL * 1024UL,
+};
+
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty16k)
+{
+	/* one u8 index bitmap */
+	.buffer_size = 16UL * 1024UL,
+};
+
 FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k)
 {
 	/* one u32 index bitmap */
-- 
2.17.2

RE: [PATCH v1 03/10] iommufd/selftest: Add tests for <= u8 bitmap sizes

[Tian, Kevin]

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Saturday, June 22, 2024 2:43 AM
> 
> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty8k)
> +{
> +	/* half of an u8 index bitmap */

the comment implies 16k

> +	.buffer_size = 8UL * 1024UL,
> +};
> +
> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty16k)
> +{
> +	/* one u8 index bitmap */

the comment implies 32k

> +	.buffer_size = 16UL * 1024UL,
> +};
> +
>  FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k)
>  {
>  	/* one u32 index bitmap */
> --
> 2.17.2
> 

Re: [PATCH v1 03/10] iommufd/selftest: Add tests for <= u8 bitmap sizes

[Joao Martins]

On 25/06/2024 04:33, Tian, Kevin wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Saturday, June 22, 2024 2:43 AM
>>
>> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty8k)
>> +{
>> +	/* half of an u8 index bitmap */
> 
> the comment implies 16k
> 

Except that in mock tests we are talking about MOCK_PAGE_SIZE which is 2K

That's why it's half the size :)

>> +	.buffer_size = 8UL * 1024UL,
>> +};
>> +
>> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty16k)
>> +{
>> +	/* one u8 index bitmap */
> 
> the comment implies 32k
> 
Likewise here.

RE: [PATCH v1 03/10] iommufd/selftest: Add tests for <= u8 bitmap sizes

[Tian, Kevin]

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Tuesday, June 25, 2024 7:05 PM
> 
> On 25/06/2024 04:33, Tian, Kevin wrote:
> >> From: Joao Martins <joao.m.martins@oracle.com>
> >> Sent: Saturday, June 22, 2024 2:43 AM
> >>
> >> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty8k)
> >> +{
> >> +	/* half of an u8 index bitmap */
> >
> > the comment implies 16k
> >
> 
> Except that in mock tests we are talking about MOCK_PAGE_SIZE which is 2K
> 
> That's why it's half the size :)
> 
> >> +	.buffer_size = 8UL * 1024UL,
> >> +};
> >> +
> >> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty16k)
> >> +{
> >> +	/* one u8 index bitmap */
> >
> > the comment implies 32k
> >
> Likewise here.

ah, that's it.

but then existing calculations are wrong if using 2k page size?

FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k)
{
	/* one u32 index bitmap */
	.buffer_size = 128UL * 1024UL,
};

FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256k)
{
	/* one u64 index bitmap */
	.buffer_size = 256UL * 1024UL,
};

...

Re: [PATCH v1 03/10] iommufd/selftest: Add tests for <= u8 bitmap sizes

[Joao Martins]

On 26/06/2024 03:33, Tian, Kevin wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Tuesday, June 25, 2024 7:05 PM
>>
>> On 25/06/2024 04:33, Tian, Kevin wrote:
>>>> From: Joao Martins <joao.m.martins@oracle.com>
>>>> Sent: Saturday, June 22, 2024 2:43 AM
>>>>
>>>> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty8k)
>>>> +{
>>>> +	/* half of an u8 index bitmap */
>>>
>>> the comment implies 16k
>>>
>>
>> Except that in mock tests we are talking about MOCK_PAGE_SIZE which is 2K
>>
>> That's why it's half the size :)
>>
>>>> +	.buffer_size = 8UL * 1024UL,
>>>> +};
>>>> +
>>>> +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty16k)
>>>> +{
>>>> +	/* one u8 index bitmap */
>>>
>>> the comment implies 32k
>>>
>> Likewise here.
> 
> ah, that's it.
> 
> but then existing calculations are wrong if using 2k page size?
> 

Ugh, literally all tests have sizes are based on 4K page size reporting. It
means we aren't just testing the u32 and 4K bitmap i.e. the 128k iova range
exercises u64 and 256k two u64 and so on 640K would exercise 3 u64 + 1 u32 and
128M would exercise 8K. Thanks for catching it!

Due to the u8 issues, I was planning on switching buffer sizes into "number of
pages/ptes" such that 64k pages are covered equally. Though problem with that is
that with 64k base pages the u64 case is already 4G and we would be allocating a
lot of memory.

> FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k)
> {
> 	/* one u32 index bitmap */
> 	.buffer_size = 128UL * 1024UL,
> };
> 
> FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256k)
> {
> 	/* one u64 index bitmap */
> 	.buffer_size = 256UL * 1024UL,
> };
> 
> ...

[PATCH v1 04/10] iommufd/selftest: Do not record head iova to better match iommu drivers

[Joao Martins]

Do not se a hugepage-aligned IOVA for incrementing an IOVA, to better match
current IOMMU driver implementations. Keep it for IOPTEs where it clears
all dirty bits for a hugepage even if it is starting from a partial range.
This is also similar to AMD driver (iommu v1 format) where IOMMU uses
various subpage PTE data for dirty tracking (for non-standard page sizes).

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/selftest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index fea60db05c9f..612d6c66813c 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -266,8 +266,8 @@ static int mock_domain_read_and_clear_dirty(struct iommu_domain *domain,
 
 		/* Clear dirty */
 		if (mock_test_and_clear_dirty(mock, head, pgsize, flags))
-			iommu_dirty_bitmap_record(dirty, head, pgsize);
-		iova = head + pgsize;
+			iommu_dirty_bitmap_record(dirty, iova, pgsize);
+		iova += pgsize;
 	} while (iova < end);
 
 	return 0;
-- 
2.17.2

RE: [PATCH v1 04/10] iommufd/selftest: Do not record head iova to better match iommu drivers

[Tian, Kevin]

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Saturday, June 22, 2024 2:43 AM
> 
> Do not se a hugepage-aligned IOVA for incrementing an IOVA, to better

what does 'se' mean?

> match
> current IOMMU driver implementations. Keep it for IOPTEs where it clears
> all dirty bits for a hugepage even if it is starting from a partial range.

there are three 'it' in this sentence. Not sure the exact object being
referred each.

Re: [PATCH v1 04/10] iommufd/selftest: Do not record head iova to better match iommu drivers

[Joao Martins]

On 25/06/2024 04:35, Tian, Kevin wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Saturday, June 22, 2024 2:43 AM
>>
>> Do not se a hugepage-aligned IOVA for incrementing an IOVA, to better
> 
> what does 'se' mean?
> 
Meant as 'set'

>> match
>> current IOMMU driver implementations. Keep it for IOPTEs where it clears
>> all dirty bits for a hugepage even if it is starting from a partial range.
> 
> there are three 'it' in this sentence. Not sure the exact object being
> referred each.
> 
I have the bad habit of using 'we' when I mean the imperative form, so usually I
sed manually each we to 'it'.

'it' in this paragraph broadly refers to the logic of the mock read_and_clear()
i.e. rephrasing would be, keep the logic of clearing all IOPTE Dirty bits for a
while hugepage, even if the range being dirtied starts from part of the hugepage.

Well, now that I said out loud I guess the third 'it' was a bit misleading where
I should have used 'the IOVA range'.

RE: [PATCH v1 04/10] iommufd/selftest: Do not record head iova to better match iommu drivers

[Tian, Kevin]

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Tuesday, June 25, 2024 7:11 PM
> 
> On 25/06/2024 04:35, Tian, Kevin wrote:
> >> From: Joao Martins <joao.m.martins@oracle.com>
> >> Sent: Saturday, June 22, 2024 2:43 AM
> >>
> >> Do not se a hugepage-aligned IOVA for incrementing an IOVA, to better
> >
> > what does 'se' mean?
> >
> Meant as 'set'
> 
> >> match
> >> current IOMMU driver implementations. Keep it for IOPTEs where it clears
> >> all dirty bits for a hugepage even if it is starting from a partial range.
> >
> > there are three 'it' in this sentence. Not sure the exact object being
> > referred each.
> >
> I have the bad habit of using 'we' when I mean the imperative form, so
> usually I
> sed manually each we to 'it'.
> 
> 'it' in this paragraph broadly refers to the logic of the mock read_and_clear()
> i.e. rephrasing would be, keep the logic of clearing all IOPTE Dirty bits for a
> while hugepage, even if the range being dirtied starts from part of the
> hugepage.

this is much easier to understand. 😊

> 
> Well, now that I said out loud I guess the third 'it' was a bit misleading where
> I should have used 'the IOVA range'.

[PATCH v1 05/10] iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead

[Joao Martins]

After iova_bitmap_set_ahead() returns it may be at the end of the range.
Move iova_bitmap_set_ahead() earlier to avoid unnecessary attempt in
trying to pin the next pages by reusing iova_bitmap_done() check.

Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped pages")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/iova_bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index db8c46bee155..e33ddfc239b5 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -384,8 +384,6 @@ static int iova_bitmap_advance(struct iova_bitmap *bitmap)
 	bitmap->mapped_base_index += count;
 
 	iova_bitmap_put(bitmap);
-	if (iova_bitmap_done(bitmap))
-		return 0;
 
 	/* Iterate, set and skip any bits requested for next iteration */
 	if (bitmap->set_ahead_length) {
@@ -396,6 +394,9 @@ static int iova_bitmap_advance(struct iova_bitmap *bitmap)
 			return ret;
 	}
 
+	if (iova_bitmap_done(bitmap))
+		return 0;
+
 	/* When advancing the index we pin the next set of bitmap pages */
 	return iova_bitmap_get(bitmap);
 }
-- 
2.17.2

[PATCH v1 06/10] iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct

[Joao Martins]

The amount of IOVA mapped will be used more often in iova_bitmap_set() in
preparation to dynamically iterate the bitmap. Cache said length to avoid
having to calculate it all the time.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/iova_bitmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index e33ddfc239b5..2b87ea16ad66 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -35,6 +35,9 @@ struct iova_bitmap_map {
 	/* base IOVA representing bit 0 of the first page */
 	unsigned long iova;
 
+	/* mapped length */
+	unsigned long length;
+
 	/* page size order that each bit granules to */
 	unsigned long pgshift;
 
@@ -156,6 +159,8 @@ static unsigned long iova_bitmap_mapped_iova(struct iova_bitmap *bitmap)
 	return bitmap->iova + iova_bitmap_index_to_offset(bitmap, skip);
 }
 
+static unsigned long iova_bitmap_mapped_length(struct iova_bitmap *bitmap);
+
 /*
  * Pins the bitmap user pages for the current range window.
  * This is internal to IOVA bitmap and called when advancing the
@@ -206,6 +211,7 @@ static int iova_bitmap_get(struct iova_bitmap *bitmap)
 	 * aligned.
 	 */
 	mapped->pgoff = offset_in_page(addr);
+	mapped->length = iova_bitmap_mapped_length(bitmap);
 	return 0;
 }
 
-- 
2.17.2

[PATCH v1 07/10] iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each()

[Joao Martins]

The pinned pages are only relevant when it starts iterating the bitmap so
defer that into iova_bitmap_for_each().

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/iova_bitmap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index 2b87ea16ad66..b94636b7977e 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -269,9 +269,6 @@ struct iova_bitmap *iova_bitmap_alloc(unsigned long iova, size_t length,
 		goto err;
 	}
 
-	rc = iova_bitmap_get(bitmap);
-	if (rc)
-		goto err;
 	return bitmap;
 
 err:
@@ -425,6 +422,10 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
 {
 	int ret = 0;
 
+	ret = iova_bitmap_get(bitmap);
+	if (ret)
+		return ret;
+
 	for (; !iova_bitmap_done(bitmap) && !ret;
 	     ret = iova_bitmap_advance(bitmap)) {
 		ret = fn(bitmap, iova_bitmap_mapped_iova(bitmap),
-- 
2.17.2

[PATCH v1 08/10] iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals

[Joao Martins]

There's no need to have two conditionals when they are closely tied
together. Move the setting of bitmap::set_ahead_length after it checks for
::pages array out of bounds access.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/iova_bitmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index b94636b7977e..be97bb464f6b 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -465,18 +465,18 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
 					 last_bit - cur_bit + 1);
 		void *kaddr;
 
-		if (unlikely(page_idx > last_page_idx))
+		if (unlikely(page_idx > last_page_idx)) {
+			unsigned long left =
+				((last_bit - cur_bit + 1) << mapped->pgshift);
+
+			bitmap->set_ahead_length = left;
 			break;
+		}
 
 		kaddr = kmap_local_page(mapped->pages[page_idx]);
 		bitmap_set(kaddr, offset, nbits);
 		kunmap_local(kaddr);
 		cur_bit += nbits;
 	} while (cur_bit <= last_bit);
-
-	if (unlikely(cur_bit <= last_bit)) {
-		bitmap->set_ahead_length =
-			((last_bit - cur_bit + 1) << bitmap->mapped.pgshift);
-	}
 }
 EXPORT_SYMBOL_NS_GPL(iova_bitmap_set, IOMMUFD);
-- 
2.17.2

[PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Joao Martins]

Today zerocopy iova bitmaps use a static iteration scheme where it walks the
bitmap data in a max iteration size of 2M of bitmap of data at a time. That
translates to a fixed window of IOVA space that can span up to 64G (e.g.
base pages, x86). Here 'window' refers to the IOVA space represented by the
bitmap data it is iterating. This static scheme is the ideal one where the
reported page-size is the same as the one behind the dirty tracker.

However, problems start to appear when the dirty tracker may
dirty in many PTE sizes beyond or unaligned at the boundaries of the
iteration window. Such is the case for the IOMMU and
commit 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped pages")
tried to fix the problem by handling the PTEs that get dirty which
surprass the end of the iteration. But the fix was incomplete and it
didn't handle all the data structure issues namely:

1) when there's nothing to dirty but the end of the iteration IOVA range is
a IOMMU hugepage PTE that crosses iterations, when it goes to the next
iteration it finds the other end of the said hugepage but don't account that
it had checked for that IOPTE already. iommu driver then walk the IOVA
space as if it is a new page without accounting that it is past the start
of a bigger page which ends up setting (future) dirty bits slightly
offset-ed. Note that the partial ranges here are self induced
due as a result of the fixed 'window' scheme being unaligned to this
hugepage IOPTE.

2) on the same line of thinking between pinning pages of different
iterations it could allow DMA to mark PTEs as dirty on the second part of
this previously mentioned partial hugepage. This leads to marking part of
the hugepage as dirty but still clearing IOPTE leading to missed dirty
data.

So to fix these problems more fundamentally and avoid future ones: instead
of iterating the whole bitmap in fixed chunks, instead only pin the bitmap
pages when it has dirty bits to set. The logic is simple in
iova_bitmap_set(): check where the current iova range to be marked as dirty
is pinned and pin the bitmap pages where to-be-recorded @iova starts if
it's not. If it's partially mapped out of the whole set, continue pinning
it and set bits until the whole dirty-size is covered. The latter is more
relevant with AMD iommu pgtable v1 format where you can have up
64G/128G/256G page sizes and thus you can set 64G at a time. Code also gets
simpler and easier to follow.

Fixing this without changing this iteration scheme means changing iommu
drivers to ignore any partial pages and not clear dirty bits, which is a
bit hacky. Though getting to walk only part of a IOMMU hugepage is a
self-induced due to this iteration scheme as it doesn't (and can't) align the
iteration boundary to the huge IOPTE at the end. Thus it can't know what
the hugepage size the iteration should align to until it walks the begin/end.

Dynamically pinning adds some comparisons inside iova_bitmap_set() to check
if something needs to be pinned if the IOVA range is out of range. Though
it has the benefit that non-dirty IOVA ranges only walk page tables without
needing to pin any bitmap pages. This dynamic scheme should be better for IOMMUs
where upper layers don't need or know what PTE sizes IOVAs map into (and there
could be more than one PTE size[*]) until they walk the IOMMU page tables.

A follow-up change will remove the iteration logic.

[*] Specially on AMD v1 iommu pgtable format where most powers of two are
supported as page-size.

Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-ed54f1351a9a@oracle.com/
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/iova_bitmap.c | 73 ++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index be97bb464f6b..b047e1e180be 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -119,6 +119,9 @@ struct iova_bitmap {
 
 	/* length of the IOVA range set ahead the pinned pages */
 	unsigned long set_ahead_length;
+
+	/* true if it using the iterator otherwise it pins dynamically */
+	bool iterator;
 };
 
 /*
@@ -340,6 +343,17 @@ static unsigned long iova_bitmap_mapped_length(struct iova_bitmap *bitmap)
 	return remaining;
 }
 
+/*
+ * Returns true if [@iova..@iova+@length-1] is part of the mapped IOVA range.
+ */
+static bool iova_bitmap_mapped_range(struct iova_bitmap_map *mapped,
+				     unsigned long iova, size_t length)
+{
+	return mapped->npages &&
+		(iova >= mapped->iova &&
+		 (iova + length - 1) <= (mapped->iova + mapped->length - 1));
+}
+
 /*
  * Returns true if there's not more data to iterate.
  */
@@ -374,6 +388,27 @@ static int iova_bitmap_set_ahead(struct iova_bitmap *bitmap,
 	return ret;
 }
 
+/*
+ * Advances to a selected range, releases the current pinned
+ * pages and pins the next set of bitmap pages.
+ * Returns 0 on success or otherwise errno.
+ */
+static int iova_bitmap_advance_to(struct iova_bitmap *bitmap,
+				  unsigned long iova)
+{
+	unsigned long index;
+
+	index = iova_bitmap_offset_to_index(bitmap, iova - bitmap->iova);
+	if (index >= bitmap->mapped_total_index)
+		return -EINVAL;
+	bitmap->mapped_base_index = index;
+
+	iova_bitmap_put(bitmap);
+
+	/* Pin the next set of bitmap pages */
+	return iova_bitmap_get(bitmap);
+}
+
 /*
  * Advances to the next range, releases the current pinned
  * pages and pins the next set of bitmap pages.
@@ -426,6 +461,7 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
 	if (ret)
 		return ret;
 
+	bitmap->iterator = true;
 	for (; !iova_bitmap_done(bitmap) && !ret;
 	     ret = iova_bitmap_advance(bitmap)) {
 		ret = fn(bitmap, iova_bitmap_mapped_iova(bitmap),
@@ -433,6 +469,7 @@ int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
 		if (ret)
 			break;
 	}
+	bitmap->iterator = false;
 
 	return ret;
 }
@@ -452,11 +489,28 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
 		     unsigned long iova, size_t length)
 {
 	struct iova_bitmap_map *mapped = &bitmap->mapped;
-	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;
-	unsigned long last_page_idx = mapped->npages - 1;
+	unsigned long cur_bit, last_bit, last_page_idx;
+
+update_indexes:
+	if (unlikely(!bitmap->iterator &&
+		     !iova_bitmap_mapped_range(mapped, iova, length))) {
+
+		/*
+		 * The attempt to advance the base index to @iova
+		 * may fail if it's out of bounds, or pinning the pages
+		 * returns an error.
+		 *
+		 * It is a no-op if within a iova_bitmap_for_each() closure.
+		 */
+		if (iova_bitmap_advance_to(bitmap, iova))
+			return;
+	}
+
+	last_page_idx = mapped->npages - 1;
+	cur_bit = ((iova - mapped->iova) >>
+		mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE;
+	last_bit = (((iova + length - 1) - mapped->iova) >>
+		mapped->pgshift) + mapped->pgoff * BITS_PER_BYTE;
 
 	do {
 		unsigned int page_idx = cur_bit / BITS_PER_PAGE;
@@ -469,8 +523,13 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
 			unsigned long left =
 				((last_bit - cur_bit + 1) << mapped->pgshift);
 
-			bitmap->set_ahead_length = left;
-			break;
+			if (bitmap->iterator) {
+				bitmap->set_ahead_length = left;
+				return;
+			}
+			iova += (length - left);
+			length = left;
+			goto update_indexes;
 		}
 
 		kaddr = kmap_local_page(mapped->pages[page_idx]);
-- 
2.17.2

RE: [PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Tian, Kevin]

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Saturday, June 22, 2024 2:43 AM
> 
> Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-
> ed54f1351a9a@oracle.com/
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

missed a fixed tag?

Re: [PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Joao Martins]

On 25/06/2024 04:38, Tian, Kevin wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Sent: Saturday, June 22, 2024 2:43 AM
>>
>> Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-
>> ed54f1351a9a@oracle.com/
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> 
> missed a fixed tag?

Technically it is:

Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped
pages")

Though I deliberately didn't add it, because we need the patch afterwards to be
fully fixed in iova_bitmap_for_each() callers. Not quite sure what to do in
these cases

Re: [PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Jason Gunthorpe]

On Tue, Jun 25, 2024 at 12:13:15PM +0100, Joao Martins wrote:
> On 25/06/2024 04:38, Tian, Kevin wrote:
> >> From: Joao Martins <joao.m.martins@oracle.com>
> >> Sent: Saturday, June 22, 2024 2:43 AM
> >>
> >> Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-
> >> ed54f1351a9a@oracle.com/
> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > 
> > missed a fixed tag?
> 
> Technically it is:
> 
> Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped
> pages")
> 
> Though I deliberately didn't add it, because we need the patch afterwards to be
> fully fixed in iova_bitmap_for_each() callers. Not quite sure what to do in
> these cases

Add the Fixes tag to both is better than not having the tag at all..

Jason

Re: [PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Joao Martins]

On 25/06/2024 13:27, Jason Gunthorpe wrote:
> On Tue, Jun 25, 2024 at 12:13:15PM +0100, Joao Martins wrote:
>> On 25/06/2024 04:38, Tian, Kevin wrote:
>>>> From: Joao Martins <joao.m.martins@oracle.com>
>>>> Sent: Saturday, June 22, 2024 2:43 AM
>>>>
>>>> Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-
>>>> ed54f1351a9a@oracle.com/
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>
>>> missed a fixed tag?
>>
>> Technically it is:
>>
>> Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped
>> pages")
>>
>> Though I deliberately didn't add it, because we need the patch afterwards to be
>> fully fixed in iova_bitmap_for_each() callers. Not quite sure what to do in
>> these cases
> 
> Add the Fixes tag to both is better than not having the tag at all..
> 
Right -- but do I propagate that to all the dependencies? That was sort of what
I was wondering. Technically it's patches 6-10 would then get the Fixes tag.
Though perhaps I am conflating just describing what it fixes versus how it goes
into stable tree which is a separate thing that happens to be automatically done
via the Fixes tag. Part of the reason I said 'not sure what to do in these
cases'. For just classifying the commit it fixes, then I just need on this
patch. Putting on the second one was more for stable tree, and in that case it
would actually be all from 6-10 not just 10.

Re: [PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Jason Gunthorpe]

On Tue, Jun 25, 2024 at 04:27:24PM +0100, Joao Martins wrote:
> On 25/06/2024 13:27, Jason Gunthorpe wrote:
> > On Tue, Jun 25, 2024 at 12:13:15PM +0100, Joao Martins wrote:
> >> On 25/06/2024 04:38, Tian, Kevin wrote:
> >>>> From: Joao Martins <joao.m.martins@oracle.com>
> >>>> Sent: Saturday, June 22, 2024 2:43 AM
> >>>>
> >>>> Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-
> >>>> ed54f1351a9a@oracle.com/
> >>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> >>>
> >>> missed a fixed tag?
> >>
> >> Technically it is:
> >>
> >> Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped
> >> pages")
> >>
> >> Though I deliberately didn't add it, because we need the patch afterwards to be
> >> fully fixed in iova_bitmap_for_each() callers. Not quite sure what to do in
> >> these cases
> > 
> > Add the Fixes tag to both is better than not having the tag at all..
> > 
> Right -- but do I propagate that to all the dependencies? That was sort of what
> I was wondering. Technically it's patches 6-10 would then get the
> Fixes tag.

Well, if the patch actually fixes something, even it is only part of
something then it should have the tag.

If it is just refactoring to prepare then no it doesn't.

> cases'. For just classifying the commit it fixes, then I just need on this
> patch. Putting on the second one was more for stable tree, and in that case it
> would actually be all from 6-10 not just 10.

Fixes tag is not for dependencies, that has to be sorted out some
other way..

Jason

Re: [PATCH v1 09/10] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()

[Joao Martins]

On 25/06/2024 16:31, Jason Gunthorpe wrote:
> On Tue, Jun 25, 2024 at 04:27:24PM +0100, Joao Martins wrote:
>> On 25/06/2024 13:27, Jason Gunthorpe wrote:
>>> On Tue, Jun 25, 2024 at 12:13:15PM +0100, Joao Martins wrote:
>>>> On 25/06/2024 04:38, Tian, Kevin wrote:
>>>>>> From: Joao Martins <joao.m.martins@oracle.com>
>>>>>> Sent: Saturday, June 22, 2024 2:43 AM
>>>>>>
>>>>>> Link: https://lore.kernel.org/linux-iommu/6b90f949-48da-4cb3-ad9a-
>>>>>> ed54f1351a9a@oracle.com/
>>>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>>>
>>>>> missed a fixed tag?
>>>>
>>>> Technically it is:
>>>>
>>>> Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped
>>>> pages")
>>>>
>>>> Though I deliberately didn't add it, because we need the patch afterwards to be
>>>> fully fixed in iova_bitmap_for_each() callers. Not quite sure what to do in
>>>> these cases
>>>
>>> Add the Fixes tag to both is better than not having the tag at all..
>>>
>> Right -- but do I propagate that to all the dependencies? That was sort of what
>> I was wondering. Technically it's patches 6-10 would then get the
>> Fixes tag.
> 
> Well, if the patch actually fixes something, even it is only part of
> something then it should have the tag.
> 
> If it is just refactoring to prepare then no it doesn't.
> 
I'll put it in the last two patches then (i.e. this one and next). This patch
introduces the fix, but the fix gets only in use on the next patch.

[PATCH v1 10/10] iommufd/iova_bitmap: Remove iterator logic

[Joao Martins]

The newly introduced dynamic pinning/windowing greatly simplifies the code
and there's no obvious performance advantage that has been identified that
justifies maintinaing both schemes.

Remove the iterator logic and have iova_bitmap_for_each() just invoke the
callback with the total iova/length.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/iova_bitmap.c | 97 +----------------------------
 1 file changed, 2 insertions(+), 95 deletions(-)

diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index b047e1e180be..b9e964b1ad5c 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -116,12 +116,6 @@ struct iova_bitmap {
 
 	/* length of the IOVA range for the whole bitmap */
 	size_t length;
-
-	/* length of the IOVA range set ahead the pinned pages */
-	unsigned long set_ahead_length;
-
-	/* true if it using the iterator otherwise it pins dynamically */
-	bool iterator;
 };
 
 /*
@@ -354,40 +348,6 @@ static bool iova_bitmap_mapped_range(struct iova_bitmap_map *mapped,
 		 (iova + length - 1) <= (mapped->iova + mapped->length - 1));
 }
 
-/*
- * Returns true if there's not more data to iterate.
- */
-static bool iova_bitmap_done(struct iova_bitmap *bitmap)
-{
-	return bitmap->mapped_base_index >= bitmap->mapped_total_index;
-}
-
-static int iova_bitmap_set_ahead(struct iova_bitmap *bitmap,
-				 size_t set_ahead_length)
-{
-	int ret = 0;
-
-	while (set_ahead_length > 0 && !iova_bitmap_done(bitmap)) {
-		unsigned long length = iova_bitmap_mapped_length(bitmap);
-		unsigned long iova = iova_bitmap_mapped_iova(bitmap);
-
-		ret = iova_bitmap_get(bitmap);
-		if (ret)
-			break;
-
-		length = min(length, set_ahead_length);
-		iova_bitmap_set(bitmap, iova, length);
-
-		set_ahead_length -= length;
-		bitmap->mapped_base_index +=
-			iova_bitmap_offset_to_index(bitmap, length - 1) + 1;
-		iova_bitmap_put(bitmap);
-	}
-
-	bitmap->set_ahead_length = 0;
-	return ret;
-}
-
 /*
  * Advances to a selected range, releases the current pinned
  * pages and pins the next set of bitmap pages.
@@ -409,36 +369,6 @@ static int iova_bitmap_advance_to(struct iova_bitmap *bitmap,
 	return iova_bitmap_get(bitmap);
 }
 
-/*
- * Advances to the next range, releases the current pinned
- * pages and pins the next set of bitmap pages.
- * Returns 0 on success or otherwise errno.
- */
-static int iova_bitmap_advance(struct iova_bitmap *bitmap)
-{
-	unsigned long iova = iova_bitmap_mapped_length(bitmap) - 1;
-	unsigned long count = iova_bitmap_offset_to_index(bitmap, iova) + 1;
-
-	bitmap->mapped_base_index += count;
-
-	iova_bitmap_put(bitmap);
-
-	/* Iterate, set and skip any bits requested for next iteration */
-	if (bitmap->set_ahead_length) {
-		int ret;
-
-		ret = iova_bitmap_set_ahead(bitmap, bitmap->set_ahead_length);
-		if (ret)
-			return ret;
-	}
-
-	if (iova_bitmap_done(bitmap))
-		return 0;
-
-	/* When advancing the index we pin the next set of bitmap pages */
-	return iova_bitmap_get(bitmap);
-}
-
 /**
  * iova_bitmap_for_each() - Iterates over the bitmap
  * @bitmap: IOVA bitmap to iterate
@@ -455,23 +385,7 @@ static int iova_bitmap_advance(struct iova_bitmap *bitmap)
 int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
 			 iova_bitmap_fn_t fn)
 {
-	int ret = 0;
-
-	ret = iova_bitmap_get(bitmap);
-	if (ret)
-		return ret;
-
-	bitmap->iterator = true;
-	for (; !iova_bitmap_done(bitmap) && !ret;
-	     ret = iova_bitmap_advance(bitmap)) {
-		ret = fn(bitmap, iova_bitmap_mapped_iova(bitmap),
-			 iova_bitmap_mapped_length(bitmap), opaque);
-		if (ret)
-			break;
-	}
-	bitmap->iterator = false;
-
-	return ret;
+	return fn(bitmap, bitmap->iova, bitmap->length, opaque);
 }
 EXPORT_SYMBOL_NS_GPL(iova_bitmap_for_each, IOMMUFD);
 
@@ -492,15 +406,12 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
 	unsigned long cur_bit, last_bit, last_page_idx;
 
 update_indexes:
-	if (unlikely(!bitmap->iterator &&
-		     !iova_bitmap_mapped_range(mapped, iova, length))) {
+	if (unlikely(!iova_bitmap_mapped_range(mapped, iova, length))) {
 
 		/*
 		 * The attempt to advance the base index to @iova
 		 * may fail if it's out of bounds, or pinning the pages
 		 * returns an error.
-		 *
-		 * It is a no-op if within a iova_bitmap_for_each() closure.
 		 */
 		if (iova_bitmap_advance_to(bitmap, iova))
 			return;
@@ -523,10 +434,6 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
 			unsigned long left =
 				((last_bit - cur_bit + 1) << mapped->pgshift);
 
-			if (bitmap->iterator) {
-				bitmap->set_ahead_length = left;
-				return;
-			}
 			iova += (length - left);
 			length = left;
 			goto update_indexes;
-- 
2.17.2

RE: [PATCH v1 00/10] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests

[Tian, Kevin]

> From: Joao Martins <joao.m.martins@oracle.com>
> Sent: Saturday, June 22, 2024 2:43 AM
> 
> This series survives 1700 migrations of guests with applications doing RDMA
> (which is roughly 3-days spent migration back and forth between hosts).
> Additionally like the series in [1] I used a set of changes/tool to
> exercise some dirty tracking tests but with real iommu drivers. Finally
> tested >=64G IOMMU page sizes using this tool but with device-dax memory
> (to generate >=64G contiguous RAM) to tackle the case of patch 9 when
> iova_bitmap_set() gets called with enourmous page sizes that surprasse or
> is equal to the maximum pin set of 64G.
> 
> Comments, review as usual appreciated.
> 
> 	Joao

for the series:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v1 00/10] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests

[Jason Gunthorpe]

On Fri, Jun 21, 2024 at 07:42:44PM +0100, Joao Martins wrote:
> Joao Martins (10):
>   iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps
>   iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps
>   iommufd/selftest: Add tests for <= u8 bitmap sizes
>   iommufd/selftest: Do not record head iova to better match iommu drivers
>   iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead
>   iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct
>   iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each()
>   iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals
>   iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()
>   iommufd/iova_bitmap: Remove iterator logic

It seems OK, you'll send a v2 with the little updates?

Thanks,
Jason

Re: [PATCH v1 00/10] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests

[Joao Martins]

On 26/06/2024 14:41, Jason Gunthorpe wrote:
> On Fri, Jun 21, 2024 at 07:42:44PM +0100, Joao Martins wrote:
>> Joao Martins (10):
>>   iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps
>>   iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps
>>   iommufd/selftest: Add tests for <= u8 bitmap sizes
>>   iommufd/selftest: Do not record head iova to better match iommu drivers
>>   iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead
>>   iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct
>>   iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each()
>>   iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals
>>   iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()
>>   iommufd/iova_bitmap: Remove iterator logic
> 
> It seems OK, you'll send a v2 with the little updates?

Yeap