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

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Mar 21, 2025 at 11:26:02PM +0800, Yi Liu wrote:
> On 2025/3/21 08:34, Nicolin Chen wrote:
> > > +	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?
> 
> these are for non-pasid-capable device. without variant, we only create the
> pasid-capable device, hence the above test in the if statement are not
> necessary.

Yea, I mean, just create two devices unconditionally, one for
pasid and another for non-pasid. I think it's good to have a
non-pasid device to cover these negative pasid attach cases.

> > > +
> > > +	/*
> > > +	 * 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?
> 
> In this v10, attaching pasid to ioas will allocated a pasid-comapt
> hwpt. But this is really messy. So I will make the auto_hwpt always
> non-pasid-compat. Hence it's aligned between RID and PASID path.

I see. A pasid slot can only store a pasid-compat HWPT. That's
clear now :)

> > > +	/*
> > > +	 * 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"?
> 
> hmmm. This naming is a bit tricky. It still requires the user side
> to detach 1024. That flag is more for the conveniency of faking error.

Ah, so maybe that "pasid_1024_attached" should be named just
"fake_attach_error?

Thanks
Nicolin