RE: [PATCH RFC 04/11] iommu/vt-d: Add SVA domain support

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Sunday, March 20, 2022 2:40 PM
> 
> Add support for SVA domain allocation and provide an SVA-specific
> iommu_domain_ops.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/intel-iommu.h |  1 +
>  drivers/iommu/intel/iommu.c | 12 ++++++++++++
>  drivers/iommu/intel/svm.c   | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 2f9891cb3d00..c14283137fb5 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
>  u32 intel_svm_get_pasid(struct iommu_sva *handle);
>  int intel_svm_page_response(struct device *dev, struct iommu_fault_event
> *evt,
>  			    struct iommu_page_response *msg);
> +extern const struct iommu_domain_ops intel_svm_domain_ops;
> 
>  struct intel_svm_dev {
>  	struct list_head list;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c1b91bce1530..d55dca3eacf8 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4318,6 +4318,18 @@ static struct iommu_domain
> *intel_iommu_domain_alloc(unsigned type)
>  		return domain;
>  	case IOMMU_DOMAIN_IDENTITY:
>  		return &si_domain->domain;
> +#ifdef CONFIG_INTEL_IOMMU_SVM
> +	case IOMMU_DOMAIN_SVA:
> +		dmar_domain = alloc_domain(type);
> +		if (!dmar_domain) {
> +			pr_err("Can't allocate sva domain\n");
> +			return NULL;
> +		}
> +		domain = &dmar_domain->domain;
> +		domain->ops = &intel_svm_domain_ops;
> +
> +		return domain;
> +#endif /* CONFIG_INTEL_IOMMU_SVM */
>  	default:
>  		return NULL;
>  	}
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index ee5ecde5b318..b9f4dd7057d1 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -932,3 +932,37 @@ int intel_svm_page_response(struct device *dev,
>  	mutex_unlock(&pasid_mutex);
>  	return ret;
>  }
> +
> +static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
> +				      struct device *dev, ioasid_t pasid)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct mm_struct *mm = domain->sva_cookie;
> +	struct intel_iommu *iommu = info->iommu;
> +	struct iommu_sva *sva;
> +
> +	mutex_lock(&pasid_mutex);
> +	sva = intel_svm_bind_mm(iommu, dev, mm);
> +	mutex_unlock(&pasid_mutex);
> +

I'm not sure whether this is the right implementation of this callback.
In last patch you said it will be used for multiple usages but here it
is fixed to mm binding. Also the pasid argument is not used at all
and instead it is retrieved from mm struct implicitly.

Basically SVA requires three steps:

1) alloc a SVA-type domain;
2) construct the domain to wrap mm;
3) attach the domain to a PASID;

If we aim .attach_dev_pasid to be generic it may suggest that 1) and 2)
should be done before .attach_dev_pasid then within this callback it
just deals with domain/pasid attach in a generic way.

> +	return IS_ERR_OR_NULL(sva);
> +}
> +
> +static void intel_svm_detach_dev_pasid(struct iommu_domain *domain,
> +				       struct device *dev, ioasid_t pasid)
> +{
> +	mutex_lock(&pasid_mutex);
> +	intel_svm_unbind_mm(dev, pasid);
> +	mutex_unlock(&pasid_mutex);
> +}
> +
> +static void intel_svm_domain_free(struct iommu_domain *domain)
> +{
> +	kfree(domain);
> +}
> +
> +const struct iommu_domain_ops intel_svm_domain_ops = {
> +	.attach_dev_pasid	= intel_svm_attach_dev_pasid,
> +	.detach_dev_pasid	= intel_svm_detach_dev_pasid,
> +	.free			= intel_svm_domain_free,
> +};
> --
> 2.25.1