[PATCH 0/2] Add an iommufd selftest for replacement between domain and handle

[PATCH 0/2] Add an iommufd selftest for replacement between domain and handle

[Yi Liu]

Now iommu core has two sets of group attach APIs for RID. One is with
handle and another one without handle. iommu core allows replacing a
group which has been attached via the API without handle. However, no
known code used it yet. Add an iommufd selftest to cover it.

Based on the below series:
https://lore.kernel.org/linux-iommu/20250226011849.5102-1-yi.l.liu@intel.com/

Yi Liu (2):
  iommufd/selftest: Add a helper to get test device
  iommufd/selftest: Add coverage for RID replace between handle and
    non-handle

 drivers/iommu/iommufd/iommufd_test.h          |   5 +
 drivers/iommu/iommufd/selftest.c              | 134 ++++++++++++++++--
 tools/testing/selftests/iommu/iommufd.c       |   2 +
 .../selftests/iommu/iommufd_fail_nth.c        |   3 +
 tools/testing/selftests/iommu/iommufd_utils.h |  18 +++
 5 files changed, 151 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 1/2] iommufd/selftest: Add a helper to get test device

[Yi Liu]

There is need to get the selftest device (sobj->type == TYPE_IDEV) in
multiple places, so have a helper to for it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/selftest.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index f7d67bc15359..20a48cb067e6 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -968,29 +968,39 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
 	return rc;
 }
 
-/* Replace the mock domain with a manually allocated hw_pagetable */
-static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
-					    unsigned int device_id, u32 pt_id,
-					    struct iommu_test_cmd *cmd)
+static struct selftest_obj *
+iommufd_test_get_self_test_device(struct iommufd_ctx *ictx, u32 id)
 {
 	struct iommufd_object *dev_obj;
 	struct selftest_obj *sobj;
-	int rc;
 
 	/*
 	 * Prefer to use the OBJ_SELFTEST because the destroy_rwsem will ensure
 	 * it doesn't race with detach, which is not allowed.
 	 */
-	dev_obj =
-		iommufd_get_object(ucmd->ictx, device_id, IOMMUFD_OBJ_SELFTEST);
+	dev_obj = iommufd_get_object(ictx, id, IOMMUFD_OBJ_SELFTEST);
 	if (IS_ERR(dev_obj))
-		return PTR_ERR(dev_obj);
+		return ERR_CAST(dev_obj);
 
 	sobj = to_selftest_obj(dev_obj);
 	if (sobj->type != TYPE_IDEV) {
-		rc = -EINVAL;
-		goto out_dev_obj;
+		iommufd_put_object(ictx, dev_obj);
+		return ERR_PTR(-EINVAL);
 	}
+	return sobj;
+}
+
+/* Replace the mock domain with a manually allocated hw_pagetable */
+static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
+					    unsigned int device_id, u32 pt_id,
+					    struct iommu_test_cmd *cmd)
+{
+	struct selftest_obj *sobj;
+	int rc;
+
+	sobj = iommufd_test_get_self_test_device(ucmd->ictx, device_id);
+	if (IS_ERR(sobj))
+		return PTR_ERR(sobj);
 
 	rc = iommufd_device_replace(sobj->idev.idev, &pt_id);
 	if (rc)
@@ -1000,7 +1010,7 @@ static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 
 out_dev_obj:
-	iommufd_put_object(ucmd->ictx, dev_obj);
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
 	return rc;
 }
 
-- 
2.34.1

Re: [PATCH 1/2] iommufd/selftest: Add a helper to get test device

[Nicolin Chen]

On Tue, Feb 25, 2025 at 09:01:29PM -0800, Yi Liu wrote:
> There is need to get the selftest device (sobj->type == TYPE_IDEV) in
> multiple places, so have a helper to for it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

With two nits below:

> @@ -968,29 +968,39 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
>  	return rc;
>  }
>  
> -/* Replace the mock domain with a manually allocated hw_pagetable */
> -static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
> -					    unsigned int device_id, u32 pt_id,
> -					    struct iommu_test_cmd *cmd)
> +static struct selftest_obj *
> +iommufd_test_get_self_test_device(struct iommufd_ctx *ictx, u32 id)

Since it wants selftest_obj, maybe just iommufd_test_get_selftest_obj.

> @@ -1000,7 +1010,7 @@ static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
>  	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>  
>  out_dev_obj:

out_sobj:

> -	iommufd_put_object(ucmd->ictx, dev_obj);
> +	iommufd_put_object(ucmd->ictx, &sobj->obj);
>  	return rc;

Thanks
Nic

Re: [PATCH 1/2] iommufd/selftest: Add a helper to get test device

[Yi Liu]

On 2025/2/27 05:45, Nicolin Chen wrote:
> On Tue, Feb 25, 2025 at 09:01:29PM -0800, Yi Liu wrote:
>> There is need to get the selftest device (sobj->type == TYPE_IDEV) in
>> multiple places, so have a helper to for it.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With two nits below:
> 
>> @@ -968,29 +968,39 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
>>   	return rc;
>>   }
>>   
>> -/* Replace the mock domain with a manually allocated hw_pagetable */
>> -static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
>> -					    unsigned int device_id, u32 pt_id,
>> -					    struct iommu_test_cmd *cmd)
>> +static struct selftest_obj *
>> +iommufd_test_get_self_test_device(struct iommufd_ctx *ictx, u32 id)
> 
> Since it wants selftest_obj, maybe just iommufd_test_get_selftest_obj.
> 
>> @@ -1000,7 +1010,7 @@ static int iommufd_test_mock_domain_replace(struct iommufd_ucmd *ucmd,
>>   	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
>>   
>>   out_dev_obj:
> 
> out_sobj:
> 
>> -	iommufd_put_object(ucmd->ictx, dev_obj);
>> +	iommufd_put_object(ucmd->ictx, &sobj->obj);
>>   	return rc;
got it.

-- 
Regards,
Yi Liu

[PATCH 2/2] iommufd/selftest: Add coverage for RID replace between handle and non-handle

[Yi Liu]

The iommu core supports replace between the attach with/without handle,
add test to cover it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/iommufd_test.h          |   5 +
 drivers/iommu/iommufd/selftest.c              | 102 ++++++++++++++++++
 tools/testing/selftests/iommu/iommufd.c       |   2 +
 .../selftests/iommu/iommufd_fail_nth.c        |   3 +
 tools/testing/selftests/iommu/iommufd_utils.h |  18 ++++
 5 files changed, 130 insertions(+)

diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h
index a6b7a163f636..87dd123d1ff0 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -24,6 +24,7 @@ enum {
 	IOMMU_TEST_OP_MD_CHECK_IOTLB,
 	IOMMU_TEST_OP_TRIGGER_IOPF,
 	IOMMU_TEST_OP_DEV_CHECK_CACHE,
+	IOMMU_TEST_OP_MIX_REPLACE_HANDLE,
 };
 
 enum {
@@ -145,6 +146,10 @@ struct iommu_test_cmd {
 			__u32 id;
 			__u32 cache;
 		} check_dev_cache;
+		struct {
+			__u32 pt_id;
+			/* @id is stdev_id */
+		} mix_replace_handle;
 	};
 	__u32 last;
 };
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 20a48cb067e6..f237a6ef58b7 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -1629,6 +1629,105 @@ static int iommufd_test_trigger_iopf(struct iommufd_ucmd *ucmd,
 	return 0;
 }
 
+static inline struct iommufd_hw_pagetable *
+iommufd_get_hwpt(struct iommufd_ucmd *ucmd, u32 id)
+{
+	struct iommufd_object *pt_obj;
+
+	pt_obj = iommufd_get_object(ucmd->ictx, id, IOMMUFD_OBJ_ANY);
+	if (IS_ERR(pt_obj))
+		return ERR_CAST(pt_obj);
+
+	if (pt_obj->type != IOMMUFD_OBJ_HWPT_NESTED &&
+	    pt_obj->type != IOMMUFD_OBJ_HWPT_PAGING) {
+		iommufd_put_object(ucmd->ictx, pt_obj);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return container_of(pt_obj, struct iommufd_hw_pagetable, obj);
+}
+
+static int iommufd_test_mixed_group_replace(struct iommufd_device *idev,
+					    struct iommu_domain *domain)
+{
+	struct iommu_domain *old_domain = iommu_get_domain_for_dev(idev->dev);
+	struct iommu_group *group = idev->igroup->group;
+	struct iommu_attach_handle *handle, *old_handle;
+	int ret;
+
+	/*
+	 * The test device should have been attached by the _handle API,
+	 * hence there should be an entry in the group->pasid_array.
+	 */
+	old_handle = iommu_attach_handle_get(group, IOMMU_NO_PASID, 0);
+	if (IS_ERR(old_handle))
+		return PTR_ERR(old_handle);
+
+	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+	if (!handle)
+		return -ENOMEM;
+
+	mutex_lock(&idev->igroup->lock);
+	iommu_detach_group(domain, group);
+
+	/* attach without handle */
+	ret = iommu_attach_group(domain, group);
+	if (ret)
+		goto out_revert;
+
+	/* replace with handle should succeed */
+	ret = iommu_replace_group_handle(group, domain, handle);
+	if (ret)
+		goto out_revert;
+
+	if (iommu_get_domain_for_dev(idev->dev) != domain) {
+		ret = -ENODEV;
+		goto out_revert;
+	}
+
+	/* replace with the same handle/domain should succeed */
+	ret = iommu_replace_group_handle(group, domain, handle);
+
+out_revert:
+	WARN_ON(iommu_replace_group_handle(group, old_domain, old_handle));
+	mutex_unlock(&idev->igroup->lock);
+	kfree(handle);
+	return ret;
+}
+
+static int iommufd_test_mixed_handle_replace(struct iommufd_ucmd *ucmd,
+					     unsigned int device_id,
+					     u32 pt_id)
+{
+	struct iommufd_hw_pagetable *hwpt;
+	struct iommufd_device *idev;
+	struct iommu_domain *domain;
+	struct selftest_obj *sobj;
+	int rc;
+
+	sobj = iommufd_test_get_self_test_device(ucmd->ictx, device_id);
+	if (IS_ERR(sobj))
+		return PTR_ERR(sobj);
+
+	idev = sobj->idev.idev;
+
+	hwpt = iommufd_get_hwpt(ucmd, pt_id);
+	if (IS_ERR(hwpt)) {
+		rc = PTR_ERR(hwpt);
+		goto out_dev_obj;
+	}
+
+	domain = hwpt->domain;
+
+	rc = iommufd_test_mixed_group_replace(idev, domain);
+
+	iommufd_put_object(ucmd->ictx, &hwpt->obj);
+
+out_dev_obj:
+	iommufd_put_object(ucmd->ictx, &sobj->obj);
+	return rc;
+}
+
 void iommufd_selftest_destroy(struct iommufd_object *obj)
 {
 	struct selftest_obj *sobj = to_selftest_obj(obj);
@@ -1710,6 +1809,9 @@ int iommufd_test(struct iommufd_ucmd *ucmd)
 					  cmd->dirty.flags);
 	case IOMMU_TEST_OP_TRIGGER_IOPF:
 		return iommufd_test_trigger_iopf(ucmd, cmd);
+	case IOMMU_TEST_OP_MIX_REPLACE_HANDLE:
+		return iommufd_test_mixed_handle_replace(ucmd, cmd->id,
+						cmd->mix_replace_handle.pt_id);
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index a1b2b657999d..8d8ce97b53ee 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -1951,6 +1951,8 @@ TEST_F(iommufd_mock_domain, alloc_hwpt)
 		test_cmd_hwpt_alloc(self->idev_ids[i], self->ioas_id,
 				    IOMMU_HWPT_ALLOC_NEST_PARENT, &hwpt_id[1]);
 
+		test_cmd_mixed_replace(self->stdev_ids[i], hwpt_id[0]);
+
 		/* Do a hw_pagetable rotation test */
 		test_cmd_mock_domain_replace(self->stdev_ids[i], hwpt_id[0]);
 		EXPECT_ERRNO(EBUSY, _test_ioctl_destroy(self->fd, hwpt_id[0]));
diff --git a/tools/testing/selftests/iommu/iommufd_fail_nth.c b/tools/testing/selftests/iommu/iommufd_fail_nth.c
index 64b1f8e1b0cf..fa6d8cbbe6e1 100644
--- a/tools/testing/selftests/iommu/iommufd_fail_nth.c
+++ b/tools/testing/selftests/iommu/iommufd_fail_nth.c
@@ -665,6 +665,9 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 				 IOMMU_HWPT_DATA_NONE, 0, 0))
 		return -1;
 
+	if (_test_cmd_mixed_replace(self->fd, stdev_id, hwpt_id))
+		return -1;
+
 	if (_test_cmd_mock_domain_replace(self->fd, stdev_id, ioas_id2, NULL))
 		return -1;
 
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h
index d979f5b0efe8..27d51f880369 100644
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -936,3 +936,21 @@ static int _test_cmd_vdevice_alloc(int fd, __u32 viommu_id, __u32 idev_id,
 	EXPECT_ERRNO(_errno,                                                 \
 		     _test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id,   \
 					     virt_id, vdev_id))
+
+static int _test_cmd_mixed_replace(int fd, __u32 stdev_id, __u32 pt_id)
+{
+	struct iommu_test_cmd test_mixed_replace = {
+		.size = sizeof(test_mixed_replace),
+		.op = IOMMU_TEST_OP_MIX_REPLACE_HANDLE,
+		.id = stdev_id,
+		.mix_replace_handle = {
+			.pt_id = pt_id,
+		},
+	};
+
+	return ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_MIX_REPLACE_HANDLE),
+		     &test_mixed_replace);
+}
+
+#define test_cmd_mixed_replace(stdev_id, hwpt_id)                         \
+	ASSERT_EQ(0, _test_cmd_mixed_replace(self->fd, stdev_id, hwpt_id))
-- 
2.34.1

Re: [PATCH 2/2] iommufd/selftest: Add coverage for RID replace between handle and non-handle

[Nicolin Chen]

On Tue, Feb 25, 2025 at 09:01:30PM -0800, Yi Liu wrote:
> The iommu core supports replace between the attach with/without handle,
> add test to cover it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
[...]
> +static inline struct iommufd_hw_pagetable *
> +iommufd_get_hwpt(struct iommufd_ucmd *ucmd, u32 id)
> +{
> +	struct iommufd_object *pt_obj;
> +
> +	pt_obj = iommufd_get_object(ucmd->ictx, id, IOMMUFD_OBJ_ANY);
> +	if (IS_ERR(pt_obj))
> +		return ERR_CAST(pt_obj);
> +
> +	if (pt_obj->type != IOMMUFD_OBJ_HWPT_NESTED &&
> +	    pt_obj->type != IOMMUFD_OBJ_HWPT_PAGING) {
> +		iommufd_put_object(ucmd->ictx, pt_obj);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return container_of(pt_obj, struct iommufd_hw_pagetable, obj);

Will there be any other caller potentially?

> +static int iommufd_test_mixed_group_replace(struct iommufd_device *idev,
> +					    struct iommu_domain *domain)
> +{
> +	struct iommu_domain *old_domain = iommu_get_domain_for_dev(idev->dev);
> +	struct iommu_group *group = idev->igroup->group;
> +	struct iommu_attach_handle *handle, *old_handle;
> +	int ret;
> +
> +	/*
> +	 * The test device should have been attached by the _handle API,
> +	 * hence there should be an entry in the group->pasid_array.
> +	 */
> +	old_handle = iommu_attach_handle_get(group, IOMMU_NO_PASID, 0);
> +	if (IS_ERR(old_handle))
> +		return PTR_ERR(old_handle);
> +
> +	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> +	if (!handle)
> +		return -ENOMEM;
> +
> +	mutex_lock(&idev->igroup->lock);
> +	iommu_detach_group(domain, group);
> +
> +	/* attach without handle */
> +	ret = iommu_attach_group(domain, group);
> +	if (ret)
> +		goto out_revert;
> +
> +	/* replace with handle should succeed */
> +	ret = iommu_replace_group_handle(group, domain, handle);
> +	if (ret)
> +		goto out_revert;
> +
> +	if (iommu_get_domain_for_dev(idev->dev) != domain) {
> +		ret = -ENODEV;
> +		goto out_revert;
> +	}
> +
> +	/* replace with the same handle/domain should succeed */
> +	ret = iommu_replace_group_handle(group, domain, handle);

Hmm, this is switching between domains with/without a handle?
It feels odd to me, as we should do this in the user space code
other than in the kernel?

Also, I think that any iommufd-allocated domain (via hwpt) has
an attach_handle now, right? Mind elaborating a use case with
iommufd that can switch between domains with/without a handle?

Thanks
Nic

Re: [PATCH 2/2] iommufd/selftest: Add coverage for RID replace between handle and non-handle

[Yi Liu]

On 2025/2/27 05:58, Nicolin Chen wrote:
> On Tue, Feb 25, 2025 at 09:01:30PM -0800, Yi Liu wrote:
>> The iommu core supports replace between the attach with/without handle,
>> add test to cover it.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> [...]
>> +static inline struct iommufd_hw_pagetable *
>> +iommufd_get_hwpt(struct iommufd_ucmd *ucmd, u32 id)
>> +{
>> +	struct iommufd_object *pt_obj;
>> +
>> +	pt_obj = iommufd_get_object(ucmd->ictx, id, IOMMUFD_OBJ_ANY);
>> +	if (IS_ERR(pt_obj))
>> +		return ERR_CAST(pt_obj);
>> +
>> +	if (pt_obj->type != IOMMUFD_OBJ_HWPT_NESTED &&
>> +	    pt_obj->type != IOMMUFD_OBJ_HWPT_PAGING) {
>> +		iommufd_put_object(ucmd->ictx, pt_obj);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	return container_of(pt_obj, struct iommufd_hw_pagetable, obj);
> 
> Will there be any other caller potentially?

yes. The iommufd pasid series has another selftest op that needs it.

https://lore.kernel.org/linux-iommu/20250226114032.4591-1-yi.l.liu@intel.com/T/#mdea85bdfec37d86fe7275b0944df5d39e1858422

>> +static int iommufd_test_mixed_group_replace(struct iommufd_device *idev,
>> +					    struct iommu_domain *domain)
>> +{
>> +	struct iommu_domain *old_domain = iommu_get_domain_for_dev(idev->dev);
>> +	struct iommu_group *group = idev->igroup->group;
>> +	struct iommu_attach_handle *handle, *old_handle;
>> +	int ret;
>> +
>> +	/*
>> +	 * The test device should have been attached by the _handle API,
>> +	 * hence there should be an entry in the group->pasid_array.
>> +	 */
>> +	old_handle = iommu_attach_handle_get(group, IOMMU_NO_PASID, 0);
>> +	if (IS_ERR(old_handle))
>> +		return PTR_ERR(old_handle);
>> +
>> +	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>> +	if (!handle)
>> +		return -ENOMEM;
>> +
>> +	mutex_lock(&idev->igroup->lock);
>> +	iommu_detach_group(domain, group);
>> +
>> +	/* attach without handle */
>> +	ret = iommu_attach_group(domain, group);
>> +	if (ret)
>> +		goto out_revert;
>> +
>> +	/* replace with handle should succeed */
>> +	ret = iommu_replace_group_handle(group, domain, handle);
>> +	if (ret)
>> +		goto out_revert;
>> +
>> +	if (iommu_get_domain_for_dev(idev->dev) != domain) {
>> +		ret = -ENODEV;
>> +		goto out_revert;
>> +	}
>> +
>> +	/* replace with the same handle/domain should succeed */
>> +	ret = iommu_replace_group_handle(group, domain, handle);
> 
> Hmm, this is switching between domains with/without a handle?
> It feels odd to me, as we should do this in the user space code
> other than in the kernel?

yes. but iommufd now always attach with handle.

> Also, I think that any iommufd-allocated domain (via hwpt) has
> an attach_handle now, right? Mind elaborating a use case with
> iommufd that can switch between domains with/without a handle?

I don't have a usage yet. It's a remark[1] from Jason that reminds me
that we now actually support it but does not have any code to test
it. Hence, I got this test case.

[1] https://lore.kernel.org/linux-iommu/20250218195756.GG4183890@nvidia.com/

-- 
Regards,
Yi Liu

Re: [PATCH 2/2] iommufd/selftest: Add coverage for RID replace between handle and non-handle

[Jason Gunthorpe]

On Tue, Feb 25, 2025 at 09:01:30PM -0800, Yi Liu wrote:
> The iommu core supports replace between the attach with/without handle,
> add test to cover it.

Hum, it is not really an iommufd test anymore like this..

Though I see why you'd want to it this way

Jason