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

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Mar 21, 2025 at 09:43:48AM +0800, Yi Liu wrote:
> On 2025/3/21 07:17, Nicolin Chen wrote:
> > 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
> 
> yep
> 
> > Also, the coding style of the multi-line comments is a bit odd.
> 
> yeah, but it cannot be finished in one line. And I think it is necessary
> to add it to note how userspace should set the id field and pasid field.

At least multi-line comments in general should be:
 /*
  * abc
  * efg
  */

And I think now we have IOMMU_TEST_PASID_RESERVED, we can move
that line of "pasid" to the macro, so what's left will be just
"@id is stdev_id".

> > > 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?
> 
> maybe move it to mdev as Jason suggested in another email.

Yes

> > > +	 * 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?
> 
> do you mean when does it detach?

So, this after all is a "toggle-to-fake-an-error" thing, right?
Let's make it straightforward then: "fake_attach_error" or so?

> > > +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?
> 
> not intend to make it optional. just wants to use 0 as a special
> value hence no need to retrieve hwpt. Hence be able to check if this
> pasid is attached or not.
> 
> > 
> > > +			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?

I have the other reply that I think we may try getting rid of the
"result" and just use the ioctl return value to tell user space
tester whether everything is okay or not, given that all the user
space expects is a succeeded "result".

> > > +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()?
> 
> It should be reverting to the old hwpt. It lacks of a helper to get the old
> hwpt so far. I can add one since we have pasid_attach array now. But it
> ends up with helpers used only by selftest which is not so positive. Also,
> it requires a mock_dev->lock to sync the attach/replace/detach. Then I
> found iommufd_test_mock_domain_replace() just returns without revert. So
> I chose the simpler way.

Yea, I see that the existing replace() doesn't revert either, so
I think we can be fine with this too. If anything bad happen to
a basic iommufd_ucmd_respond, the test wouldn't probably finish
anyway to provide us an accurate result.

Thanks
Nicolin