Re: [PATCH v2 02/12] iommu/vt-d: Check if SVA is supported when attaching the SVA domain

[Yi Liu <yi.l.liu@intel.com>]

On 2025/2/24 13:16, Lu Baolu wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Attach of a SVA domain should fail if SVA is not supported, move the check
> for SVA support out of IOMMU_DEV_FEAT_SVA and into attach.
> 
> Also check when allocating a SVA domain to match other drivers.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 37 +------------------------------
>   drivers/iommu/intel/svm.c   | 43 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+), 36 deletions(-)
> 

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index cc46098f875b..a4048de66378 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3851,41 +3851,6 @@ static struct iommu_group *intel_iommu_device_group(struct device *dev)
>   	return generic_device_group(dev);
>   }
>   
> -static int intel_iommu_enable_sva(struct device *dev)
> -{
> -	struct device_domain_info *info = dev_iommu_priv_get(dev);
> -	struct intel_iommu *iommu;
> -
> -	if (!info || dmar_disabled)
> -		return -EINVAL;
> -
> -	iommu = info->iommu;
> -	if (!iommu)
> -		return -EINVAL;
> -
> -	if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
> -		return -ENODEV;
> -
> -	if (!info->pasid_enabled || !info->ats_enabled)
> -		return -EINVAL;
> -
> -	/*
> -	 * Devices having device-specific I/O fault handling should not
> -	 * support PCI/PRI. The IOMMU side has no means to check the
> -	 * capability of device-specific IOPF.  Therefore, IOMMU can only
> -	 * default that if the device driver enables SVA on a non-PRI
> -	 * device, it will handle IOPF in its own way.
> -	 */
> -	if (!info->pri_supported)
> -		return 0;
> -
> -	/* Devices supporting PRI should have it enabled. */
> -	if (!info->pri_enabled)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>   static int context_flip_pri(struct device_domain_info *info, bool enable)
>   {
>   	struct intel_iommu *iommu = info->iommu;
> @@ -4006,7 +3971,7 @@ intel_iommu_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
>   		return intel_iommu_enable_iopf(dev);
>   
>   	case IOMMU_DEV_FEAT_SVA:
> -		return intel_iommu_enable_sva(dev);
> +		return 0;
>   
>   	default:
>   		return -ENODEV;
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index f5569347591f..ba93123cb4eb 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -110,6 +110,41 @@ static const struct mmu_notifier_ops intel_mmuops = {
>   	.free_notifier = intel_mm_free_notifier,
>   };
>   
> +static int intel_iommu_sva_supported(struct device *dev)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu;
> +
> +	if (!info || dmar_disabled)
> +		return -EINVAL;
> +
> +	iommu = info->iommu;
> +	if (!iommu)
> +		return -EINVAL;
> +
> +	if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
> +		return -ENODEV;
> +
> +	if (!info->pasid_enabled || !info->ats_enabled)
> +		return -EINVAL;
> +
> +	/*
> +	 * Devices having device-specific I/O fault handling should not
> +	 * support PCI/PRI. The IOMMU side has no means to check the
> +	 * capability of device-specific IOPF.  Therefore, IOMMU can only
> +	 * default that if the device driver enables SVA on a non-PRI
> +	 * device, it will handle IOPF in its own way.
> +	 */
> +	if (!info->pri_supported)
> +		return 0;
> +
> +	/* Devices supporting PRI should have it enabled. */
> +	if (!info->pri_enabled)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
>   				   struct device *dev, ioasid_t pasid,
>   				   struct iommu_domain *old)
> @@ -121,6 +156,10 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
>   	unsigned long sflags;
>   	int ret = 0;
>   
> +	ret = intel_iommu_sva_supported(dev);
> +	if (ret)
> +		return ret;
> +
>   	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
>   	if (IS_ERR(dev_pasid))
>   		return PTR_ERR(dev_pasid);
> @@ -161,6 +200,10 @@ struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
>   	struct dmar_domain *domain;
>   	int ret;
>   
> +	ret = intel_iommu_sva_supported(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>   	if (!domain)
>   		return ERR_PTR(-ENOMEM);

-- 
Regards,
Yi Liu