* [PATCH v2 01/11] iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
@ 2024-06-27 11:00 ` Joao Martins
2024-06-27 11:00 ` [PATCH v2 02/11] iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps Joao Martins
` (11 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:00 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 02/11] iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
2024-06-27 11:00 ` [PATCH v2 01/11] iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps Joao Martins
@ 2024-06-27 11:00 ` Joao Martins
2024-06-27 11:00 ` [PATCH v2 03/11] iommufd/selftest: Add tests for <= u8 bitmap sizes Joao Martins
` (10 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:00 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 03/11] iommufd/selftest: Add tests for <= u8 bitmap sizes
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
2024-06-27 11:00 ` [PATCH v2 01/11] iommufd/selftest: Fix dirty bitmap tests with u8 bitmaps Joao Martins
2024-06-27 11:00 ` [PATCH v2 02/11] iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps Joao Martins
@ 2024-06-27 11:00 ` Joao Martins
2024-06-27 11:00 ` [PATCH v2 04/11] iommufd/selftest: Fix tests to use MOCK_PAGE_SIZE based buffer sizes Joao Martins
` (9 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:00 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 04/11] iommufd/selftest: Fix tests to use MOCK_PAGE_SIZE based buffer sizes
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (2 preceding siblings ...)
2024-06-27 11:00 ` [PATCH v2 03/11] iommufd/selftest: Add tests for <= u8 bitmap sizes Joao Martins
@ 2024-06-27 11:00 ` Joao Martins
2024-06-27 11:00 ` [PATCH v2 05/11] iommufd/selftest: Do not record head iova to better match iommu drivers Joao Martins
` (8 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:00 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
commit a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP")
added tests covering edge cases in the boundaries of iova bitmap. Although
it used buffer sizes thinking in PAGE_SIZE (4K) as opposed to the
MOCK_PAGE_SIZE (2K) that is used in iommufd mock selftests. This meant that
isn't correctly exercising everything specifically the u32 and 4K bitmap
test cases. Fix selftests buffer sizes to be based on mock page size.
Reported-by: Kevin Tian <kevin.tian@intel.com>
Closes: https://lore.kernel.org/linux-iommu/96efb6cf-a41c-420f-9673-2f0b682cac8c@oracle.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 | 36 ++++++++++++-------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 61189215e1ab..5f7d5a5ba89b 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -1797,47 +1797,47 @@ FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty16k)
.buffer_size = 16UL * 1024UL,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty64k)
{
/* one u32 index bitmap */
- .buffer_size = 128UL * 1024UL,
+ .buffer_size = 64UL * 1024UL,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256k)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k)
{
/* one u64 index bitmap */
- .buffer_size = 256UL * 1024UL,
+ .buffer_size = 128UL * 1024UL,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty640k)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty320k)
{
/* two u64 index and trailing end bitmap */
- .buffer_size = 640UL * 1024UL,
+ .buffer_size = 320UL * 1024UL,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128M)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty64M)
{
- /* 4K bitmap (128M IOVA range) */
- .buffer_size = 128UL * 1024UL * 1024UL,
+ /* 4K bitmap (64M IOVA range) */
+ .buffer_size = 64UL * 1024UL * 1024UL,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128M_huge)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty64M_huge)
{
- /* 4K bitmap (128M IOVA range) */
- .buffer_size = 128UL * 1024UL * 1024UL,
+ /* 4K bitmap (64M IOVA range) */
+ .buffer_size = 64UL * 1024UL * 1024UL,
.hugepages = true,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256M)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128M)
{
- /* 8K bitmap (256M IOVA range) */
- .buffer_size = 256UL * 1024UL * 1024UL,
+ /* 8K bitmap (128M IOVA range) */
+ .buffer_size = 128UL * 1024UL * 1024UL,
};
-FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256M_huge)
+FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128M_huge)
{
- /* 8K bitmap (256M IOVA range) */
- .buffer_size = 256UL * 1024UL * 1024UL,
+ /* 8K bitmap (128M IOVA range) */
+ .buffer_size = 128UL * 1024UL * 1024UL,
.hugepages = true,
};
--
2.17.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 05/11] iommufd/selftest: Do not record head iova to better match iommu drivers
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (3 preceding siblings ...)
2024-06-27 11:00 ` [PATCH v2 04/11] iommufd/selftest: Fix tests to use MOCK_PAGE_SIZE based buffer sizes Joao Martins
@ 2024-06-27 11:00 ` Joao Martins
2024-06-27 11:01 ` [PATCH v2 06/11] iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead Joao Martins
` (7 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:00 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
Do not set a hugepage-aligned IOVA for incrementing an IOVA, to better
match current IOMMU driver implementations. Keep the logic of clearing all
IOPTE dirty bits for a whole hugepage, even if the range being dirtied
starts from part of the hugepage. 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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 06/11] iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (4 preceding siblings ...)
2024-06-27 11:00 ` [PATCH v2 05/11] iommufd/selftest: Do not record head iova to better match iommu drivers Joao Martins
@ 2024-06-27 11:01 ` Joao Martins
2024-06-27 11:01 ` [PATCH v2 07/11] iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct Joao Martins
` (6 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:01 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 07/11] iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (5 preceding siblings ...)
2024-06-27 11:01 ` [PATCH v2 06/11] iommufd/iova_bitmap: Check iova_bitmap_done() after set ahead Joao Martins
@ 2024-06-27 11:01 ` Joao Martins
2024-06-27 11:01 ` [PATCH v2 08/11] iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each() Joao Martins
` (5 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:01 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 08/11] iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each()
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (6 preceding siblings ...)
2024-06-27 11:01 ` [PATCH v2 07/11] iommufd/iova_bitmap: Cache mapped length in iova_bitmap_map struct Joao Martins
@ 2024-06-27 11:01 ` Joao Martins
2024-06-27 11:01 ` [PATCH v2 09/11] iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals Joao Martins
` (4 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:01 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 09/11] iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (7 preceding siblings ...)
2024-06-27 11:01 ` [PATCH v2 08/11] iommufd/iova_bitmap: Move initial pinning to iova_bitmap_for_each() Joao Martins
@ 2024-06-27 11:01 ` Joao Martins
2024-06-27 11:01 ` [PATCH v2 10/11] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set() Joao Martins
` (3 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:01 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 10/11] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set()
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (8 preceding siblings ...)
2024-06-27 11:01 ` [PATCH v2 09/11] iommufd/iova_bitmap: Consolidate iova_bitmap_set exit conditionals Joao Martins
@ 2024-06-27 11:01 ` Joao Martins
2024-06-27 11:01 ` [PATCH v2 11/11] iommufd/iova_bitmap: Remove iterator logic Joao Martins
` (2 subsequent siblings)
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:01 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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/
Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped pages")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 11/11] iommufd/iova_bitmap: Remove iterator logic
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (9 preceding siblings ...)
2024-06-27 11:01 ` [PATCH v2 10/11] iommufd/iova_bitmap: Dynamic pinning on iova_bitmap_set() Joao Martins
@ 2024-06-27 11:01 ` Joao Martins
2024-06-27 16:48 ` [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Matt Ochs
2024-06-28 16:16 ` Jason Gunthorpe
12 siblings, 0 replies; 14+ messages in thread
From: Joao Martins @ 2024-06-27 11:01 UTC (permalink / raw)
To: iommu
Cc: Jason Gunthorpe, Kevin Tian, Suravee Suthikulpanit, Lu Baolu,
Joao Martins, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi, Matt Ochs
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.
Fixes: 2780025e01e2 ("iommufd/iova_bitmap: Handle recording beyond the mapped pages")
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (10 preceding siblings ...)
2024-06-27 11:01 ` [PATCH v2 11/11] iommufd/iova_bitmap: Remove iterator logic Joao Martins
@ 2024-06-27 16:48 ` Matt Ochs
2024-06-28 16:16 ` Jason Gunthorpe
12 siblings, 0 replies; 14+ messages in thread
From: Matt Ochs @ 2024-06-27 16:48 UTC (permalink / raw)
To: Joao Martins
Cc: iommu@lists.linux.dev, Jason Gunthorpe, Kevin Tian,
Suravee Suthikulpanit, Lu Baolu, Avihai Horon, Alex Williamson,
Shameerali Kolothum Thodi
> On Jun 27, 2024, at 6:00 AM, Joao Martins <joao.m.martins@oracle.com> wrote:
>
> * Patches 1-4: Fixes the 64k arm64 broken selftests and introduces two
> more edge cases to cover the equivalent in 4k base pages. Lastly it
> fixes buffer sizes to be based on MOCK_PAGE_SIZE for its size rather than
> PAGE_SIZE.
Tested-by: Matt Ochs <mochs@nvidia.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests
2024-06-27 11:00 [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Joao Martins
` (11 preceding siblings ...)
2024-06-27 16:48 ` [PATCH v2 00/11] iommufd/{iova_bitmap,selftest}: Fix dirty hugepages tracking and selftests Matt Ochs
@ 2024-06-28 16:16 ` Jason Gunthorpe
12 siblings, 0 replies; 14+ messages in thread
From: Jason Gunthorpe @ 2024-06-28 16:16 UTC (permalink / raw)
To: Joao Martins
Cc: iommu, Kevin Tian, Suravee Suthikulpanit, Lu Baolu, Avihai Horon,
Alex Williamson, Shameerali Kolothum Thodi, Matt Ochs
On Thu, Jun 27, 2024 at 12:00:54PM +0100, Joao Martins wrote:
> Joao Martins (11):
> 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: Fix tests to use MOCK_PAGE_SIZE based buffer 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
Applied to iommufd for-next, thanks
Jason
^ permalink raw reply [flat|nested] 14+ messages in thread