Re: [PATCH 09/11] iommu/vt-d: Reuse intel_iommu_set_dev_pasid function

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

On 2024/1/16 9:11, Tina Zhang wrote:
> Refactor intel_iommu_set_dev_pasid() and make it be reused by sva domain
> as the set_dev_pasid operation.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 43 ++++++++++++++++++++++++++-----------
>   drivers/iommu/intel/iommu.h |  2 ++
>   drivers/iommu/intel/svm.c   | 42 +-----------------------------------
>   3 files changed, 33 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index af466ba301b7..8d1b9de78a43 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4574,7 +4574,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	intel_drain_pasid_prq(dev, pasid);
>   }
>   
> -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> +int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   				     struct device *dev, ioasid_t pasid)
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
> @@ -4593,31 +4593,48 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
>   	if (context_copied(iommu, info->bus, info->devfn))
>   		return -EBUSY;
>   
> -	ret = prepare_domain_attach_device(domain, dev);
> -	if (ret)
> -		return ret;
> -
>   	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
>   	if (!dev_pasid)
>   		return -ENOMEM;
>   
> -	ret = domain_attach_iommu(dmar_domain, iommu);
> -	if (ret)
> -		goto out_free;
> +	dev_pasid->dev = dev;
> +	dev_pasid->pasid = pasid;
> +	if (domain_type_is_sva(dmar_domain)) {
> +		dev_pasid->did = FLPT_DEFAULT_DID;
> +		dev_pasid->sid = PCI_DEVID(info->bus, info->devfn);
> +		if (info->ats_enabled) {
> +			dev_pasid->qdep = info->ats_qdep;
> +			if (dev_pasid->qdep >= QI_DEV_EIOTLB_MAX_INVS)
> +				dev_pasid->qdep = 0;
> +		}

This is generic device information. There's no need to do it only for
the SVA domain; otherwise, you'll have to check the domain type every
time you use these fields.

> +	} else {
> +		ret = prepare_domain_attach_device(domain, dev);

The prepare_domain_attach_device() function is called to check whether
the device is compatible with the domain to be attached. You could make
it work for the SVA domain as well, rather than ignoring it.

> +		if (ret)
> +			goto out_free;
> +
> +		ret = domain_attach_iommu(dmar_domain, iommu);

The same here.

The domain_attach_iommu() is called to setup the relationship between a
domain and an iommu. You should make it compatible with sva domain as
well.

Best regards,
baolu