On 2022/4/13 22:49, Yi Liu wrote:
On 2022/4/13 22:36, Jason Gunthorpe wrote:
On Wed, Apr 13, 2022 at 10:02:58PM +0800, Yi Liu wrote:
+/**
+ * iopt_unmap_iova() - Remove a range of iova
+ * @iopt: io_pagetable to act on
+ * @iova: Starting iova to unmap
+ * @length: Number of bytes to unmap
+ *
+ * The requested range must exactly match an existing range.
+ * Splitting/truncating IOVA mappings is not allowed.
+ */
+int iopt_unmap_iova(struct io_pagetable *iopt, unsigned long iova,
+ unsigned long length)
+{
+ struct iopt_pages *pages;
+ struct iopt_area *area;
+ unsigned long iova_end;
+ int rc;
+
+ if (!length)
+ return -EINVAL;
+
+ if (check_add_overflow(iova, length - 1, &iova_end))
+ return -EOVERFLOW;
+
+ down_read(&iopt->domains_rwsem);
+ down_write(&iopt->iova_rwsem);
+ area = iopt_find_exact_area(iopt, iova, iova_end);
when testing vIOMMU with Qemu using iommufd, I hit a problem as log #3
shows. Qemu failed when trying to do map due to an IOVA still in use.
After debugging, the 0xfffff000 IOVA is mapped but not unmapped. But per
log
#2, Qemu has issued unmap with a larger range (0xff000000 -
0x100000000) which includes the 0xfffff000. But iopt_find_exact_area()
doesn't find any area. So 0xfffff000 is not unmapped. Is this correct? Same
test passed with vfio iommu type1 driver. any idea?
There are a couple of good reasons why the iopt_unmap_iova() should
proccess any contiguous range of fully contained areas, so I would
consider this something worth fixing. can you send a small patch and
test case and I'll fold it in?
sure. just spotted it, so haven't got fix patch yet. I may work on
it tomorrow.
Hi Jason,
Got below patch for it. Also pushed to the exploration branch.
https://github.com/luxis1999/iommufd/commit/d764f3288de0fd52c578684788a437701ec31b2d
From 22a758c401a1c7f6656625013bb87204c9ea65fe Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Sun, 17 Apr 2022 07:39:03 -0700
Subject: [PATCH] iommufd/io_pagetable: Support unmap fully contained areas
Changes:
- return the unmapped bytes to caller
- supports unmap fully containerd contiguous areas
- add a test case in selftest
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
drivers/iommu/iommufd/io_pagetable.c | 90 ++++++++++++-------------
drivers/iommu/iommufd/ioas.c | 8 ++-
drivers/iommu/iommufd/iommufd_private.h | 4 +-
drivers/iommu/iommufd/vfio_compat.c | 8 ++-
include/uapi/linux/iommufd.h | 2 +-
tools/testing/selftests/iommu/iommufd.c | 40 +++++++++++
6 files changed, 99 insertions(+), 53 deletions(-)
diff --git a/drivers/iommu/iommufd/io_pagetable.c
b/drivers/iommu/iommufd/io_pagetable.c
index f9f3b06946bf..5142f797a812 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -315,61 +315,26 @@ static int __iopt_unmap_iova(struct io_pagetable
*iopt, struct iopt_area *area,
return 0;
}
-/**
- * iopt_unmap_iova() - Remove a range of iova
- * @iopt: io_pagetable to act on
- * @iova: Starting iova to unmap
- * @length: Number of bytes to unmap
- *
- * The requested range must exactly match an existing range.
- * Splitting/truncating IOVA mappings is not allowed.
- */
-int iopt_unmap_iova(struct io_pagetable *iopt, unsigned long iova,
- unsigned long length)
-{
- struct iopt_pages *pages;
- struct iopt_area *area;
- unsigned long iova_end;
- int rc;
-
- if (!length)
- return -EINVAL;
-
- if (check_add_overflow(iova, length - 1, &iova_end))
- return -EOVERFLOW;
-
- down_read(&iopt->domains_rwsem);
- down_write(&iopt->iova_rwsem);
- area = iopt_find_exact_area(iopt, iova, iova_end);
- if (!area) {
- up_write(&iopt->iova_rwsem);
- up_read(&iopt->domains_rwsem);
- return -ENOENT;
- }
- pages = area->pages;
- area->pages = NULL;
- up_write(&iopt->iova_rwsem);
-
- rc = __iopt_unmap_iova(iopt, area, pages);
- up_read(&iopt->domains_rwsem);
- return rc;
-}
-
-int iopt_unmap_all(struct io_pagetable *iopt)
+static int __iopt_unmap_iova_range(struct io_pagetable *iopt,
+ unsigned long start,
+ unsigned long end,
+ unsigned long *unmapped)
{
struct iopt_area *area;
+ unsigned long unmapped_bytes = 0;
int rc;
down_read(&iopt->domains_rwsem);
down_write(&iopt->iova_rwsem);
- while ((area = iopt_area_iter_first(iopt, 0, ULONG_MAX))) {
+ while ((area = iopt_area_iter_first(iopt, start, end))) {
struct iopt_pages *pages;
- /* Userspace should not race unmap all and map */
- if (!area->pages) {
- rc = -EBUSY;
+ if (!area->pages || iopt_area_iova(area) < start ||
+ iopt_area_last_iova(area) > end) {
+ rc = -ENOENT;
goto out_unlock_iova;
}
+
pages = area->pages;
area->pages = NULL;
up_write(&iopt->iova_rwsem);
@@ -378,6 +343,10 @@ int iopt_unmap_all(struct io_pagetable *iopt)
if (rc)
goto out_unlock_domains;
+ start = iopt_area_last_iova(area) + 1;
+ unmapped_bytes +=
+ iopt_area_last_iova(area) - iopt_area_iova(area) + 1;
+
down_write(&iopt->iova_rwsem);
}
rc = 0;
@@ -386,9 +355,40 @@ int iopt_unmap_all(struct io_pagetable *iopt)
up_write(&iopt->iova_rwsem);
out_unlock_domains:
up_read(&iopt->domains_rwsem);
+ if (unmapped)
+ *unmapped = unmapped_bytes;
return rc;
}
+/**
+ * iopt_unmap_iova() - Remove a range of iova
+ * @iopt: io_pagetable to act on
+ * @iova: Starting iova to unmap
+ * @length: Number of bytes to unmap
+ * @unmapped: Return number of bytes unmapped
+ *
+ * The requested range must exactly match an existing range.
+ * Splitting/truncating IOVA mappings is not allowed.
+ */
+int iopt_unmap_iova(struct io_pagetable *iopt, unsigned long iova,
+ unsigned long length, unsigned long *unmapped)
+{
+ unsigned long iova_end;
+
+ if (!length)
+ return -EINVAL;
+
+ if (check_add_overflow(iova, length - 1, &iova_end))
+ return -EOVERFLOW;
+
+ return __iopt_unmap_iova_range(iopt, iova, iova_end, unmapped);
+}
+
+int iopt_unmap_all(struct io_pagetable *iopt, unsigned long *unmapped)
+{
+ return __iopt_unmap_iova_range(iopt, 0, ULONG_MAX, unmapped);
+}
+
/**
* iopt_access_pages() - Return a list of pages under the iova
* @iopt: io_pagetable to act on
diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c
index 48149988c84b..4e701d053ed6 100644
--- a/drivers/iommu/iommufd/ioas.c
+++ b/drivers/iommu/iommufd/ioas.c
@@ -14,7 +14,7 @@ void iommufd_ioas_destroy(struct iommufd_object *obj)
struct iommufd_ioas *ioas = container_of(obj, struct iommufd_ioas, obj);
int rc;
- rc = iopt_unmap_all(&ioas->iopt);
+ rc = iopt_unmap_all(&ioas->iopt, NULL);
WARN_ON(rc);
iopt_destroy_table(&ioas->iopt);
mutex_destroy(&ioas->mutex);
@@ -230,6 +230,7 @@ int iommufd_ioas_unmap(struct iommufd_ucmd *ucmd)
{
struct iommu_ioas_unmap *cmd = ucmd->cmd;
struct iommufd_ioas *ioas;
+ unsigned long unmapped;
int rc;
ioas = iommufd_get_ioas(ucmd, cmd->ioas_id);
@@ -237,16 +238,17 @@ int iommufd_ioas_unmap(struct iommufd_ucmd *ucmd)
return PTR_ERR(ioas);
if (cmd->iova == 0 && cmd->length == U64_MAX) {
- rc = iopt_unmap_all(&ioas->iopt);
+ rc = iopt_unmap_all(&ioas->iopt, &unmapped);
} else {
if (cmd->iova >= ULONG_MAX || cmd->length >= ULONG_MAX) {
rc = -EOVERFLOW;
goto out_put;
}
- rc = iopt_unmap_iova(&ioas->iopt, cmd->iova, cmd->length);
+ rc = iopt_unmap_iova(&ioas->iopt, cmd->iova, cmd->length, &unmapped);
}
out_put:
iommufd_put_object(&ioas->obj);
+ cmd->length = unmapped;
return rc;
}
diff --git a/drivers/iommu/iommufd/iommufd_private.h
b/drivers/iommu/iommufd/iommufd_private.h
index f55654278ac4..382704f4d698 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -46,8 +46,8 @@ int iopt_map_pages(struct io_pagetable *iopt, struct
iopt_pages *pages,
unsigned long *dst_iova, unsigned long start_byte,
unsigned long length, int iommu_prot, unsigned int flags);
int iopt_unmap_iova(struct io_pagetable *iopt, unsigned long iova,
- unsigned long length);
-int iopt_unmap_all(struct io_pagetable *iopt);
+ unsigned long length, unsigned long *unmapped);
+int iopt_unmap_all(struct io_pagetable *iopt, unsigned long *unmapped);
int iopt_access_pages(struct io_pagetable *iopt, unsigned long iova,
unsigned long npages, struct page **out_pages, bool write);
diff --git a/drivers/iommu/iommufd/vfio_compat.c
b/drivers/iommu/iommufd/vfio_compat.c
index 5b196de00ff9..4539ff45efd9 100644
--- a/drivers/iommu/iommufd/vfio_compat.c
+++ b/drivers/iommu/iommufd/vfio_compat.c
@@ -133,6 +133,7 @@ static int iommufd_vfio_unmap_dma(struct iommufd_ctx
*ictx, unsigned int cmd,
u32 supported_flags = VFIO_DMA_UNMAP_FLAG_ALL;
struct vfio_iommu_type1_dma_unmap unmap;
struct iommufd_ioas *ioas;
+ unsigned long unmapped;
int rc;
if (copy_from_user(&unmap, arg, minsz))
@@ -146,10 +147,13 @@ static int iommufd_vfio_unmap_dma(struct iommufd_ctx
*ictx, unsigned int cmd,
return PTR_ERR(ioas);
if (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL)
- rc = iopt_unmap_all(&ioas->iopt);
+ rc = iopt_unmap_all(&ioas->iopt, &unmapped);
else
- rc = iopt_unmap_iova(&ioas->iopt, unmap.iova, unmap.size);
+ rc = iopt_unmap_iova(&ioas->iopt, unmap.iova,
+ unmap.size, &unmapped);
iommufd_put_object(&ioas->obj);
+ unmap.size = unmapped;
+
return rc;
}
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 2c0f5ced4173..8cbc6a083156 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -172,7 +172,7 @@ struct iommu_ioas_copy {
* @size: sizeof(struct iommu_ioas_copy)
* @ioas_id: IOAS ID to change the mapping of
* @iova: IOVA to start the unmapping at
- * @length: Number of bytes to unmap
+ * @length: Number of bytes to unmap, and return back the bytes unmapped
*
* Unmap an IOVA range. The iova/length must exactly match a range
* used with IOMMU_IOAS_PAGETABLE_MAP, or be the values 0 & U64_MAX.
diff --git a/tools/testing/selftests/iommu/iommufd.c
b/tools/testing/selftests/iommu/iommufd.c
index 5c47d706ed94..42956acd2c04 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -357,6 +357,47 @@ TEST_F(iommufd_ioas, area)
ASSERT_EQ(0, ioctl(self->fd, IOMMU_IOAS_UNMAP, &unmap_cmd));
}
+TEST_F(iommufd_ioas, unmap_fully_contained_area)
+{
+ struct iommu_ioas_map map_cmd = {
+ .size = sizeof(map_cmd),
+ .ioas_id = self->ioas_id,
+ .flags = IOMMU_IOAS_MAP_FIXED_IOVA,
+ .length = PAGE_SIZE,
+ .user_va = (uintptr_t)buffer,
+ };
+ struct iommu_ioas_unmap unmap_cmd = {
+ .size = sizeof(unmap_cmd),
+ .ioas_id = self->ioas_id,
+ .length = PAGE_SIZE,
+ };
+ int i;
+
+ for (i = 0; i != 4; i++) {
+ map_cmd.iova = self->base_iova + i * 16 * PAGE_SIZE;
+ map_cmd.length = 8 * PAGE_SIZE;
+ ASSERT_EQ(0,
+ ioctl(self->fd, IOMMU_IOAS_MAP, &map_cmd));
+ }
+
+ /* Unmap not fully contained area doesn't work */
+ unmap_cmd.iova = self->base_iova - 4 * PAGE_SIZE;
+ unmap_cmd.length = 8 * PAGE_SIZE;
+ ASSERT_EQ(ENOENT,
+ ioctl(self->fd, IOMMU_IOAS_UNMAP, &unmap_cmd));
+
+ unmap_cmd.iova = self->base_iova + 3 * 16 * PAGE_SIZE + 8 * PAGE_SIZE - 4
* PAGE_SIZE;
+ unmap_cmd.length = 8 * PAGE_SIZE;
+ ASSERT_EQ(ENOENT,
+ ioctl(self->fd, IOMMU_IOAS_UNMAP, &unmap_cmd));
+
+ /* Unmap fully contained areas works */
+ unmap_cmd.iova = self->base_iova - 4 * PAGE_SIZE;
+ unmap_cmd.length = 3 * 16 * PAGE_SIZE + 8 * PAGE_SIZE + 4 * PAGE_SIZE;
+ ASSERT_EQ(0, ioctl(self->fd, IOMMU_IOAS_UNMAP, &unmap_cmd));
+ ASSERT_EQ(32, unmap_cmd.length);
+}
+
TEST_F(iommufd_ioas, area_auto_iova)
{
struct iommu_test_cmd test_cmd = {
--
2.27.0
--
Regards,
Yi Liu
