Re: [PATCH v1 02/20] iommu: Introduce a test_dev domain op and an internal helper

Niklas Schnelle
2025-10-14T14:21:39Z

On Mon, 2025-10-13 at 10:22 -0700, Nicolin Chen wrote:
> 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 :)

Ok makes sense, though it does list __iommu_device_set_domain()
separately. Either way is fine for me.

>
> > > /**
> > > * 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.
>

Sounds good, thanks!