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

[Lu Baolu <baolu.lu@linux.intel.com>]

On 2022/4/12 15:19, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> 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