Re: [PATCH RFC v3 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA interfaces

[Lu Baolu]

On 2022/4/12 15:19, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Sunday, April 10, 2022 6:25 PM
> > +struct iommu_sva *
> > +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> > *drvdata)
> > +{
> > +	int ret = -EINVAL;
> > +	struct iommu_sva *handle;
> > +	struct iommu_domain *domain;
> > +	struct iommu_sva_ioas *ioas;
> > +
> > +	/*
> > +	 * TODO: Remove the drvdata parameter after kernel PASID support
> > is
> > +	 * enabled for the idxd driver.
> > +	 */
> > +	if (drvdata)
> > +		return ERR_PTR(-EOPNOTSUPP);
> > +
> > +	/* Allocate mm->pasid if necessary. */
> > +	ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits)
> > - 1);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	mutex_lock(&iommu_sva_lock);
> > +	ioas = iommu_sva_ioas_get(mm, mm->pasid);
> > +	if (!ioas) {
> > +		ret = -ENOMEM;
>
> ioas can be NULL for multiple reasons, e.g. :
>
> 1) ioas->mm != mm;
> 2) kzalloc failure;
> 3) xa_store failure;
>
> It's more sensible to return error from iommu_sva_ioas_get() directly.

Fair enough.

> > +		goto out_unlock;
> > +	}
> > +
> > +	/* Search for an existing bond. */
> > +	list_for_each_entry(handle, &ioas->bonds, node) {
> > +		if (handle->dev == dev) {
> > +			refcount_inc(&handle->users);
> > +			/* No new bond, drop the counter. */
> > +			iommu_sva_ioas_put(ioas);
> > +			goto out_success;
> > +		}
> > +	}
> > +
> > +	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
>
> s/handle/bond/?

"handle" is used in the previous implementation but "bond" looks better
to read. :-)

> > +	if (!handle) {
> > +		ret = -ENOMEM;
> > +		goto out_put_ioas;
> > +	}
> > +
> > +	/* The reference to ioas will be kept until domain free. */
> > +	domain = iommu_sva_alloc_domain(dev, ioas);
>
> Shouldn't we first try whether existing domains are compatible to this
> device?

If we think that here domain represents a hardware pagetable actually
used by IOMMU for a {device, pasid}, we are able to use per-{device,
pasid} domain without checking compatibility. Sharing a domain among
devices under the same IOMMU may be an optimization. That could be done
in the IOMMU driver just like what vt-d driver is doing for pass-through
DMA domains.

> > @@ -1952,6 +1954,7 @@ EXPORT_SYMBOL_GPL(iommu_domain_alloc);
> >   void iommu_domain_free(struct iommu_domain *domain)
> >   {
> >   	iommu_put_dma_cookie(domain);
> > +	iommu_sva_ioas_put(domain->sva_ioas);
> >   	domain->ops->free(domain);
> >   }
>
> is it good to have general iommu_domain_free() to always call a put()
> function for a specific domain type? Why cannot it be done before
> calling iommu_domain_free() in sva-lib.c?

It's better to have a generic free() helper since an sva domain could be
freed outside of iommu sva code as you can see in the following patches.

Best regards,
baolu