Hi Niklas,
On Mon, Oct 13, 2025 at 11:53:55AM +0200, Niklas Schnelle wrote:
> On Sun, 2025-10-12 at 17:04 -0700, Nicolin Chen wrote:
> > Add a new test_dev domain op for driver to test the compatibility between
> > a domain and a device at the driver level, before calling into the actual
> > attachment/replacement of a domain. Support pasid for set_dev_pasid call.
> >
> > Move existing core-level compatibility tests to a helper function. Invoke
> > it prior to:
> > * __iommu_attach_device() or its wrapper __iommu_device_set_domain()
> > * __iommu_set_group_pasid()
>
> Should this list also include iommu_deferred_attach()? The code does
> include it.
iommu_deferred_attach() invokes __iommu_attach_device(), so it is
already included in the list :)
> > /**
> > * struct iommu_domain_ops - domain specific operations
> > - * @attach_dev: attach an iommu domain to a device
> > + * @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid
> > call.
> > + * A driver-level callback of this op should do a thorough
> > sanity, to
>
> You're missing the word "check" above.
Ack.
> > + * make sure a device is compatible with the domain. So the
> > following
> > + * @attach_dev and @set_dev_pasid functions would likely
> > succeed with
> > + * only one exception due to a temporary failure like out of
> > memory.
>
> Nit: "… only one exception …" / "… like out of memory …" this sounds a
> bit odd to me because on the one hand it's one exception but then also
> a group (temporary failures).
>
> Maybe better:
> "… would likely succeed with only the exception of temporary failures
> like out of memory."?
Sure. I can do that. Fixing both parts, it would be:
* @test_dev: Test compatibility prior to an @attach_dev or @set_dev_pasid call.
* A driver callback of this op should do a thorough sanity check, to
* make sure a device is compatible with the domain, so the following
* @attach_dev and @set_dev_pasid functions would likely succeed with
* only the exception of temporary failures like out of memory.
> --- snip ---
> > @@ -3615,6 +3657,11 @@ int iommu_replace_device_pasid(struct iommu_domain
> > *domain,
> > ret = 0;
> >
> > if (curr_domain != domain) {
> > + ret = __iommu_domain_test_device(domain, dev, pasid,
> > + curr_domain);
> > + if (ret)
> > + goto out_unlock;
> > +
> > ret = __iommu_set_group_pasid(domain, group,
> > pasid, curr_domain);
> > if (ret)
>
> Apart from the comment and commit description nits mentioned above this
> looks good to me.
>
> Reviewed-by: Niklas Schnelle <[email protected]>
Thanks for the review!
Nicolin
