Re: [PATCH v10 18/18] iommufd/selftest: Add coverage for iommufd pasid attach/detach

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, Mar 20, 2025 at 06:47:44AM -0700, Yi Liu wrote:

> +TEST_F(iommufd_device_pasid, pasid_attach)
> +{
> +	struct iommu_hwpt_selftest data = {
> +		.iotlb =  IOMMU_TEST_IOTLB_DEFAULT,
> +	};
> +	uint32_t nested_hwpt_id[3] = {};
> +	uint32_t parent_hwpt_id = 0;
> +	uint32_t fault_id, fault_fd;
> +	uint32_t s2_hwpt_id = 0;
> +	uint32_t iopf_hwpt_id;
> +	uint32_t pasid = 100;
> +	uint32_t auto_hwpt;
> +	uint32_t viommu_id;
> +	bool result;
> +
> +	/* Allocate two nested hwpts sharing one common parent hwpt */
> +	test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
> +			    IOMMU_HWPT_ALLOC_NEST_PARENT,
> +			    &parent_hwpt_id);
> +	test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id,
> +				   IOMMU_HWPT_ALLOC_PASID,
> +				   &nested_hwpt_id[0],
> +				   IOMMU_HWPT_DATA_SELFTEST,
> +				   &data, sizeof(data));
> +	test_cmd_hwpt_alloc_nested(self->device_id, parent_hwpt_id,
> +				   IOMMU_HWPT_ALLOC_PASID,
> +				   &nested_hwpt_id[1],
> +				   IOMMU_HWPT_DATA_SELFTEST,
> +				   &data, sizeof(data));
> +
> +	/* Faulte related preparation */

Fault

> +	/* Allocate a regular nested hwpt based on viommu */
> +	test_cmd_viommu_alloc(self->device_id, parent_hwpt_id,
> +			      IOMMU_VIOMMU_TYPE_SELFTEST,
> +			      &viommu_id);
> +	test_cmd_hwpt_alloc_nested(self->device_id, viommu_id,
> +				   IOMMU_HWPT_ALLOC_PASID,
> +				   &nested_hwpt_id[2],
> +				   IOMMU_HWPT_DATA_SELFTEST, &data,
> +				   sizeof(data));
> +
> +	test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
> +			    IOMMU_HWPT_ALLOC_PASID,
> +			    &s2_hwpt_id);
> +
> +	/* Attach RID to non-pasid compat domain, */
> +	test_cmd_mock_domain_replace(self->stdev_id, parent_hwpt_id);
> +	/* then attach to pasid should fail */
> +	test_err_pasid_attach(EINVAL, pasid, s2_hwpt_id, NULL);
> +
> +	/* Attach RID to pasid compat domain, */
> +	test_cmd_mock_domain_replace(self->stdev_id, s2_hwpt_id);
> +	/* then attach to pasid should succeed, */
> +	test_cmd_pasid_attach(pasid, nested_hwpt_id[0], NULL);
> +	/* but attach RID to non-pasid compat domain should fail now. */
> +	test_err_mock_domain_replace(EINVAL, self->stdev_id, parent_hwpt_id);
> +	test_cmd_pasid_detach(pasid);
> +
> +	if (!variant->pasid_capable) {
> +		/*
> +		 * PASID-compatible domain can be used by non-PASID-capable
> +		 * device.
> +		 */
> +		test_cmd_mock_domain_replace(self->no_pasid_stdev_id, nested_hwpt_id[0]);
> +		test_cmd_mock_domain_replace(self->no_pasid_stdev_id, self->ioas_id);
> +		/*
> +		 * Attach hwpt to pasid#100 of non-PASID-capable device,
> +		 * should fail, no matter domain is pasid-comapt or not.
> +		 */
> +		EXPECT_ERRNO(EINVAL,
> +			     _test_cmd_pasid_attach(self->fd, self->no_pasid_stdev_id,
> +						    pasid, parent_hwpt_id, NULL));
> +		EXPECT_ERRNO(EINVAL,
> +			     _test_cmd_pasid_attach(self->fd, self->no_pasid_stdev_id,
> +						    pasid, s2_hwpt_id, NULL));
> +	}

It seems that we should test these anyway without a variant?

> +
> +	/*
> +	 * Attach non pasid compat hwpt to pasid-capable device, should
> +	 * fail, and have null domain.
> +	 */
> +	test_err_pasid_attach(EINVAL, pasid, parent_hwpt_id, NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, 0, &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Attach ioas to pasid 100, should succeed, domain should
> +	 * be valid.
> +	 */
> +	test_cmd_pasid_attach(pasid, self->ioas_id, &auto_hwpt);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, auto_hwpt, &result));
> +	EXPECT_EQ(1, result);

Hmm, I thought that a non-RID PASID slot could only attach a PASID-
compatible HWPT. I think I am totally confused now... lol

Perhaps we need a detailed documentation somewhere, at least as a
reminder or so?

> +
> +	/* Attach to pasid 100 which has been attached, should fail. */
> +	test_err_pasid_attach(EBUSY, pasid, self->ioas_id, &auto_hwpt);
> +
> +	/*
> +	 * Try attach pasid 100 with another hwpt, should FAIL
> +	 * as attach does not allow overwrite, use REPLACE instead.
> +	 */
> +	test_err_pasid_attach(EBUSY, pasid, nested_hwpt_id[0], NULL);
> +
> +	/*
> +	 * Detach hwpt from pasid 100, and check if the pasid 100
> +	 * has null domain. Should be done before the next attach.
> +	 */
> +	test_cmd_pasid_detach(pasid);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, 0, &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Attach nested hwpt to pasid 100, should succeed, domain
> +	 * should be valid.
> +	 */
> +	test_cmd_pasid_attach(pasid, nested_hwpt_id[0], NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, nested_hwpt_id[0],
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/* Attach to pasid 100 which has been attached, should fail. */
> +	test_err_pasid_attach(EBUSY, pasid, nested_hwpt_id[0], NULL);
> +
> +	/*
> +	 * Detach hwpt from pasid 100, and check if the pasid 100
> +	 * has null domain
> +	 */
> +	test_cmd_pasid_detach(pasid);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, 0, &result));
> +	EXPECT_EQ(1, result);
> +
> +	/* Replace tests */
> +
> +	pasid = 200;
> +	/*
> +	 * Replace pasid 200 without attaching it first, should
> +	 * fail with -EINVAL.
> +	 */
> +	test_err_cmd_pasid_replace(EINVAL, pasid, s2_hwpt_id, NULL);
> +
> +	/*
> +	 * Attach a s2 hwpt to pasid 200, should succeed, domain should

Attach the ..

> +	 * be valid.
> +	 */
> +	test_cmd_pasid_attach(pasid, s2_hwpt_id, NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, s2_hwpt_id,
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Replace pasid 200 with self->ioas_id, should succeed,
> +	 * and have valid domain.
> +	 */
> +	test_cmd_pasid_replace(pasid, self->ioas_id, &auto_hwpt);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, auto_hwpt,
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Replace a nested hwpt for pasid 200, should succeed,
> +	 * and have valid domain.
> +	 */
> +	test_cmd_pasid_replace(pasid, nested_hwpt_id[0], NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, nested_hwpt_id[0],
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Replace with another nested hwpt for pasid 200, should
> +	 * succeed, and have valid domain.
> +	 */
> +	test_cmd_pasid_replace(pasid, nested_hwpt_id[1], NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, nested_hwpt_id[1],
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Detach hwpt from pasid 200, and check if the pasid 200
> +	 * has null domain.
> +	 */
> +	test_cmd_pasid_detach(pasid);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, 0, &result));
> +	EXPECT_EQ(1, result);
> +
> +	/* Negative Tests for pasid replace, use pasid 1024 */
> +
> +	/*
> +	 * Attach a s2 hwpt to pasid 1024, should succeed, domain should

Attach the ...

> +	 * be valid.
> +	 */
> +	pasid = 1024;
> +	test_cmd_pasid_attach(pasid, s2_hwpt_id, NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, s2_hwpt_id,
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Replace pasid 1024 with self->ioas_id, should fail,
> +	 * but have the old valid domain. This is a designed
> +	 * negative case, normally replace with self->ioas_id
> +	 * could succeed.
> +	 */
> +	test_err_cmd_pasid_replace(ENOMEM, pasid, self->ioas_id, NULL);
> +	ASSERT_EQ(0,
> +		  test_cmd_pasid_check_domain(self->fd, self->stdev_id,
> +					      pasid, s2_hwpt_id,
> +					      &result));
> +	EXPECT_EQ(1, result);
> +
> +	/*
> +	 * Detach hwpt from pasid 1024, and check if the pasid 1024
> +	 * has null domain.
> +	 */
> +	test_cmd_pasid_detach(pasid);

The designed "failing" replace does "pasid_1024_attached = false",
meaning that this detach() isn't necessary?

Or perhaps the designed "failing" shouldn't set "attached = false"?

> +	/* Detach the s2_hwpt_id from RID */
> +	test_cmd_mock_domain_replace(self->stdev_id, self->ioas_id);
> +
> +	test_ioctl_destroy(nested_hwpt_id[0]);
> +	test_ioctl_destroy(nested_hwpt_id[1]);
> +	test_ioctl_destroy(nested_hwpt_id[2]);
> +	test_ioctl_destroy(viommu_id);
> +	test_ioctl_destroy(parent_hwpt_id);
> +	test_ioctl_destroy(s2_hwpt_id);

Once detached, all the destroys can be done automatically?