Re: [PATCH 7/7] iommu/vtd: Split paging_domain_compatible()

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

On 6/10/25 03:58, Jason Gunthorpe wrote:
> Make First/Second stage specific functions that follow the same pattern in
> intel_iommu_domain_alloc_first/second_stage() for computing
> EOPNOTSUPP. This makes the code easier to understand as if we couldn't
> create a domain with the parameters for this IOMMU instance then we
> certainly are not compatible with it.
> 
> Check superpage support directly against the per-stage cap bits and the
> pgsize_bitmap.
> 
> Add a note that the force_snooping is read without locking. The locking
> needs to cover the compatible check and the add of the device to the list.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   drivers/iommu/intel/iommu.c | 66 ++++++++++++++++++++++++++++++-------
>   1 file changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index ab2e9fef75293c..a482d1b77d1203 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3416,33 +3416,75 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)
>   	kfree(dmar_domain);
>   }
>   
> +static int paging_domain_compatible_first_stage(struct dmar_domain *dmar_domain,
> +						struct intel_iommu *iommu)
> +{
> +	if (WARN_ON(dmar_domain->domain.dirty_ops ||
> +		    dmar_domain->nested_parent))
> +		return -EINVAL;
> +
> +	/* Only SL is available in legacy mode */
> +	if (!sm_supported(iommu) || !ecap_flts(iommu->ecap))
> +		return -EINVAL;
> +
> +	/* Same page size support */
> +	if (!cap_fl1gp_support(iommu->cap) &&
> +	    (dmar_domain->domain.pgsize_bitmap & SZ_1G))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int
> +paging_domain_compatible_second_stage(struct dmar_domain *dmar_domain,
> +				      struct intel_iommu *iommu)
> +{
> +	unsigned int sslps = cap_super_page_val(iommu->cap);
> +
> +	if (dmar_domain->domain.dirty_ops && !ssads_supported(iommu))
> +		return -EINVAL;
> +	if (dmar_domain->nested_parent && !nested_supported(iommu))
> +		return -EINVAL;
> +
> +	/* Legacy mode always supports second stage */
> +	if (sm_supported(iommu) && !ecap_slts(iommu->ecap))
> +		return -EINVAL;
> +
> +	/* Same page size support */
> +	if (!(sslps & BIT(0)) && (dmar_domain->domain.pgsize_bitmap & SZ_2M))
> +		return -EINVAL;
> +	if (!(sslps & BIT(1)) && (dmar_domain->domain.pgsize_bitmap & SZ_1G))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>   int paging_domain_compatible(struct iommu_domain *domain, struct device *dev)
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>   	struct intel_iommu *iommu = info->iommu;
> +	int ret = -EINVAL;
>   	int addr_width;
>   
> -	if (WARN_ON_ONCE(!(domain->type & __IOMMU_DOMAIN_PAGING)))
> -		return -EPERM;
> +	if (domain->ops == &intel_fs_paging_domain_ops)
> +		ret = paging_domain_compatible_first_stage(dmar_domain, iommu);
> +	else if (domain->ops == &intel_ss_paging_domain_ops)
> +		ret = paging_domain_compatible_second_stage(dmar_domain, iommu);
> +	else if (WARN_ON(true))
> +		ret = -EINVAL;
> +	if (ret)
> +		return ret;
>   
> +	/*
> +	 * FIXME this is locked wrong, it needs to be under the
> +	 * dmar_domain->lock
> +	 */
>   	if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
>   		return -EINVAL;

Perhaps we can use group->mutex to fix this in the future?

paging_domain_compatible() is in the domain attaching path, which is
already synchronized by group->mutex. We can further expose an iommu
interface for cache coherency enforcement, which would also apply group-
 >mutex.

Thanks,
baolu