On Mon, Oct 20, 2025 at 11:51:49AM -0700, Nicolin Chen wrote:
> On Mon, Oct 20, 2025 at 01:27:36PM -0300, Jason Gunthorpe wrote:
> > On Sun, Oct 12, 2025 at 05:04:59PM -0700, Nicolin Chen wrote:
> >
> > > And keep them within the group->mutex, so drivers can simply move all the
> > > sanity and compatibility tests from their attach_dev callbacks to the new
> > > test_dev callbacks without concerning about a race condition.
> >
> > I'm not sure about this.. For the problem we are trying to solve this
> > would be racy as the test would be done and the group mutex
> > unlocked. Then later it will be re-tested and attached.
>
> Oh right, we'll have to retest in iommu_dev_reset_done(). I missed
> that.
>
> > > @@ -751,6 +760,8 @@ struct iommu_ops {
> > > * @free: Release the domain after use.
> > > */
> > > struct iommu_domain_ops {
> > > + int (*test_dev)(struct iommu_domain *domain, struct device *dev,
> > > + ioasid_t pasid, struct iommu_domain *old);
> >
> > Because of the starting remark I'm skeptical that old should be
> > included here.
>
> Hmm, the followings functions sanitizes "old":
> - qcom_iommu_identity_attach() drivers/iommu/arm/arm-smmu/qcom_iommu.c
That shouldn't be copied over to test??
if (domain == identity_domain || !domain)
return 0;
That is just optimizing away the attach if it has nothing to do
qcom_domain = to_qcom_iommu_domain(domain);
if (WARN_ON(!qcom_domain->iommu))
return -EINVAL;
That can't never happen
> - iommu_sva_set_dev_pasid() in drivers/iommu/amd/pasid.c
Its broken, you are not required by API to detach a domain before
setting a new one. Keep it in attach, hope someone fixes this driver
someday.
Jason
