Re: [PATCH v10 17/18] iommufd/selftest: Add test ops to test pasid attach/detach

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Mar 20, 2025 at 06:47:43AM -0700, Yi Liu wrote:
> @@ -150,6 +155,32 @@ struct iommu_test_cmd {
>  		struct {
>  			__u32 dev_id;
>  		} trigger_vevent;
> +		struct {
> +			__u32 pasid;
> +			__u32 pt_id;
> +			/* @id is stdev_id
> +			 * pasid#1024 is for special test, do not use it
> +			 * in normal case.
> +			 */

How about add on top of these structs:
#define IOMMU_TEST_PASID_RESERVED 1024

Also, the coding style of the multi-line comments is a bit odd.

> +		} pasid_attach;
> +		struct {
> +			__u32 pasid;
> +			__u32 pt_id;
> +			/* @id is stdev_id
> +			 * pasid#1024 is for special test, do not use it
> +			 * in normal case.
> +			 */

Ditto

> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> index 691e7a23f300..37c9cd285541 100644
> --- a/drivers/iommu/iommufd/selftest.c
> +++ b/drivers/iommu/iommufd/selftest.c
> @@ -223,10 +223,29 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
>  	return 0;
>  }
>  
> +static bool pasid_1024_attached;

I recall syzkaller would do multi-threading... We might need a
global mutex or something atomic_t?

>  static int mock_domain_set_dev_pasid_nop(struct iommu_domain *domain,
>  					 struct device *dev, ioasid_t pasid,
>  					 struct iommu_domain *old)
>  {
> +	/*
> +	 * First attach with pasid 1024 succ, second attach would fail.

succeeds?

> +	 * This is helpful to test the case in which the iommu core needs
> +	 * to rollback to old domain due to driver failure.
> +	 */
> +	if (pasid == 1024) {
> +		if (domain->type == IOMMU_DOMAIN_BLOCKED) {
> +			pasid_1024_attached = false;
> +		} else if (pasid_1024_attached) {
> +			pasid_1024_attached = false;
> +			// Fake an error to fail the replacement
> +			return -ENOMEM;

/* Fake an error to fail the replacement */

While failing this, why does it detach pasid-1024? Maybe some extra
comments for what's doing?

> +static int iommufd_test_pasid_check_domain(struct iommufd_ucmd *ucmd,
> +					   struct iommu_test_cmd *cmd)
> +{
> +	struct iommu_domain *attached_domain, *expect_domain = NULL;
> +	struct iommufd_hw_pagetable *hwpt = NULL;
> +	struct iommu_attach_handle *handle;
> +	struct selftest_obj *sobj;
> +	struct mock_dev *mdev;
> +	bool result;
> +	int rc = 0;
> +
> +	sobj = iommufd_test_get_selftest_obj(ucmd->ictx, cmd->id);
> +	if (IS_ERR(sobj))
> +		return PTR_ERR(sobj);
> +
> +	mdev = sobj->idev.mock_dev;
> +
> +	handle = iommu_attach_handle_get(mdev->dev.iommu_group,
> +					 cmd->pasid_check.pasid, 0);
> +	if (IS_ERR(handle))
> +		attached_domain = NULL;
> +	else
> +		attached_domain = handle->domain;
> +
> +	if (cmd->pasid_check.hwpt_id) {
> +		hwpt = iommufd_get_hwpt(ucmd, cmd->pasid_check.hwpt_id);
> +		if (IS_ERR(hwpt)) {

Do we need cmd->pasid_check.hwpt_id to be optional?

> +			rc = PTR_ERR(hwpt);
> +			goto out_put_dev;
> +		}
> +		expect_domain = hwpt->domain;
> +	}
> +
> +	result = (attached_domain == expect_domain) ? 1 : 0;
> +	if (copy_to_user(u64_to_user_ptr(cmd->pasid_check.out_result_ptr),
> +			 &result, sizeof(result)))
> +		rc = -EFAULT;

If we do want it to be optional, we can't unconditionally check the
result then?

> +static int iommufd_test_pasid_attach(struct iommufd_ucmd *ucmd,
> +				     struct iommu_test_cmd *cmd)
> +{
> +	struct selftest_obj *sobj;
> +	int rc;
> +
> +	sobj = iommufd_test_get_selftest_obj(ucmd->ictx, cmd->id);
> +	if (IS_ERR(sobj))
> +		return PTR_ERR(sobj);
> +
> +	rc = iommufd_device_attach(sobj->idev.idev, cmd->pasid_attach.pasid,
> +				   &cmd->pasid_attach.pt_id);
> +	if (rc)
> +		goto out_sobj;
> +
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +	if (rc)
> +		iommufd_device_detach(sobj->idev.idev,
> +				      cmd->pasid_attach.pasid);
> +
> +out_sobj:
> +	iommufd_put_object(ucmd->ictx, &sobj->obj);
> +	return rc;
> +}
> +
> +static int iommufd_test_pasid_replace(struct iommufd_ucmd *ucmd,
> +				      struct iommu_test_cmd *cmd)
> +{
> +	struct selftest_obj *sobj;
> +	int rc;
> +
> +	sobj = iommufd_test_get_selftest_obj(ucmd->ictx, cmd->id);
> +	if (IS_ERR(sobj))
> +		return PTR_ERR(sobj);
> +
> +	rc = iommufd_device_replace(sobj->idev.idev, cmd->pasid_attach.pasid,
> +				    &cmd->pasid_attach.pt_id);
> +	if (rc)
> +		goto out_sobj;
> +
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +
> +out_sobj:
> +	iommufd_put_object(ucmd->ictx, &sobj->obj);
> +	return rc;

If iommufd_ucmd_respond fails, do we need to revert like we do in
iommufd_test_pasid_attach()?

Thanks
Nicolin