Re: [PATCH v8 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
From: Nicolin Chen <hidden>
Date: 2025-07-10 05:34:40
Also in:
linux-doc, linux-iommu, linux-kselftest, linux-patches, linux-tegra, lkml
Subsystem:
iommu subsystem, iommufd, kernel selftest framework, the rest · Maintainers:
Joerg Roedel, Will Deacon, Jason Gunthorpe, Kevin Tian, Shuah Khan, Linus Torvalds
On Wed, Jul 09, 2025 at 11:55:20AM -0700, Nicolin Chen wrote:
On Wed, Jul 09, 2025 at 02:09:04PM -0300, Jason Gunthorpe wrote:quoted
On Fri, Jul 04, 2025 at 06:13:30PM -0700, Nicolin Chen wrote:quoted
+static struct iommufd_access * +iommufd_hw_queue_alloc_phys(struct iommu_hw_queue_alloc *cmd, + struct iommufd_viommu *viommu, phys_addr_t *base_pa) +{ + struct iommufd_access *access; + struct page **pages; + size_t max_npages; + size_t length; + u64 offset; + size_t i; + int rc; + + offset = + cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);PAGE_ALIGN is ALIGN UP, that is the wrong direction? It is just: offset = cmd->nesting_parent_iova % PAGE_SIZE; And this is missing: *base_pa = (page_to_pfn(pages[0]) << PAGE_SHIFT) + offset; ??Yes, my bad. And I realized that all my testings are page aligned. Maybe we could add an IOMMU_TEST_OP_HW_QUEUE_CHECK_ADDR..
I found that this needed a bit more care. The pin_pages() API accepts aligned iova and length inputs. So, doing this instead (and adding a selftest coverage):
diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 00641204efb2..91339f799916 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c@@ -206,7 +206,11 @@ static void iommufd_hw_queue_destroy_access(struct iommufd_ctx *ictx, struct iommufd_access *access, u64 base_iova, size_t length) { - iommufd_access_unpin_pages(access, base_iova, length); + u64 aligned_iova = PAGE_ALIGN_DOWN(base_iova); + u64 offset = base_iova - aligned_iova; + + iommufd_access_unpin_pages(access, aligned_iova, + PAGE_ALIGN(length + offset)); iommufd_access_detach_internal(access); iommufd_access_destroy_internal(ictx, access); }
@@ -239,22 +243,23 @@ static struct iommufd_access * iommufd_hw_queue_alloc_phys(struct iommu_hw_queue_alloc *cmd, struct iommufd_viommu *viommu, phys_addr_t *base_pa) { + u64 aligned_iova = PAGE_ALIGN_DOWN(cmd->nesting_parent_iova); + u64 offset = cmd->nesting_parent_iova - aligned_iova; struct iommufd_access *access; struct page **pages; size_t max_npages; size_t length; - u64 offset; size_t i; int rc; - offset = - cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova); - /* DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE) */ + /* max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE) */ if (check_add_overflow(offset, cmd->length, &length)) return ERR_PTR(-ERANGE); if (check_add_overflow(length, PAGE_SIZE - 1, &length)) return ERR_PTR(-ERANGE); max_npages = length / PAGE_SIZE; + /* length needs to be page aligned too */ + length = max_npages * PAGE_SIZE; /* * Use kvcalloc() to avoid memory fragmentation for a large page array.
@@ -274,8 +279,7 @@ iommufd_hw_queue_alloc_phys(struct iommu_hw_queue_alloc *cmd, if (rc) goto out_destroy; - rc = iommufd_access_pin_pages(access, cmd->nesting_parent_iova, - cmd->length, pages, 0); + rc = iommufd_access_pin_pages(access, aligned_iova, length, pages, 0); if (rc) goto out_detach;
@@ -287,13 +291,12 @@ iommufd_hw_queue_alloc_phys(struct iommu_hw_queue_alloc *cmd, goto out_unpin; } - *base_pa = page_to_pfn(pages[0]) << PAGE_SHIFT; + *base_pa = (page_to_pfn(pages[0]) << PAGE_SHIFT) + offset; kfree(pages); return access; out_unpin: - iommufd_access_unpin_pages(access, cmd->nesting_parent_iova, - cmd->length); + iommufd_access_unpin_pages(access, aligned_iova, length); out_detach: iommufd_access_detach_internal(access); out_destroy:
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 9d5b852d5e19..694b178f8ca4 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c@@ -3104,7 +3104,7 @@ TEST_F(iommufd_viommu, hw_queue) /* Allocate index=0, declare ownership of the iova */ test_cmd_hw_queue_alloc(viommu_id, IOMMU_HW_QUEUE_TYPE_SELFTEST, 0, iova, PAGE_SIZE, &hw_queue_id[0]); - /* Fail duplicate */ + /* Fail duplicated index */ test_err_hw_queue_alloc(EEXIST, viommu_id, IOMMU_HW_QUEUE_TYPE_SELFTEST, 0, iova, PAGE_SIZE, &hw_queue_id[0]); /* Fail unmap, due to iova ownership */
@@ -3112,9 +3112,10 @@ TEST_F(iommufd_viommu, hw_queue) /* The 2nd page is not pinned, so it can be unmmap */ test_ioctl_ioas_unmap(iova + PAGE_SIZE, PAGE_SIZE); - /* Allocate index=1 */ + /* Allocate index=1, with an unaligned case */ test_cmd_hw_queue_alloc(viommu_id, IOMMU_HW_QUEUE_TYPE_SELFTEST, 1, - iova, PAGE_SIZE, &hw_queue_id[1]); + iova + PAGE_SIZE / 2, PAGE_SIZE / 2, + &hw_queue_id[1]); /* Fail to destroy, due to dependency */ EXPECT_ERRNO(EBUSY, _test_ioctl_destroy(self->fd, hw_queue_id[0]));