Re: [PATCH v8 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

[Nicolin Chen <nicolinc@nvidia.com>]

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:
> > On Fri, Jul 04, 2025 at 06:13:30PM -0700, Nicolin Chen wrote:
> > > +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]));