[PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

Once the group enters 'owned' mode it can never be assigned back to the
default_domain or to a NULL domain. It must always be actively assigned to
a current domain. If the caller hasn't provided a domain then the core
must provide an explicit DMA blocking domain that has no DMA map.

Lazily create a group-global blocking DMA domain when
iommu_group_claim_dma_owner is first called and immediately assign the
group to it. This ensures that DMA is immediately fully isolated on all
IOMMU drivers.

If the user attaches/detaches while owned then detach will set the group
back to the blocking domain.

Slightly reorganize the call chains so that
__iommu_group_attach_core_domain() is the function that removes any caller
configured domain and sets the domains back a core owned domain with an
appropriate lifetime.

__iommu_group_attach_domain() is the worker function that can change the
domain assigned to a group to any target domain, including NULL.

Add comments clarifying how the NULL vs detach_dev vs default_domain works
based on Robin's remarks.

This fixes an oops with VFIO and SMMUv3 because VFIO will call
iommu_detach_group() and then immediately iommu_domain_free(), but
SMMUv3 has no way to know that the domain it is holding a pointer to
has been freed. Now the iommu_detach_group() will assign the blocking
domain and SMMUv3 will no longer hold a stale domain reference.

Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management interfaces")
Reported-by: Qian Cai <quic_qiancai@quicinc.com>
Tested-by: Baolu Lu <baolu.lu@linux.intel.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Co-developed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 122 ++++++++++++++++++++++++++++++------------
 1 file changed, 87 insertions(+), 35 deletions(-)

Joerg, this should address the issue, Nicolin reproduced the original issue
and verified this fix on ARM SMMUv3.

v2:
 - Remove redundant detach_dev ops check in __iommu_detach_device and
   make the added WARN_ON fail instead
 - Check for blocking_domain in __iommu_attach_group() so VFIO can
   actually attach a new group
 - Update comments and spelling
 - Fix missed change to new_domain in iommu_group_do_detach_device()
v1: https://lore.kernel.org/r/0-v1-6e9d2d0a759d+11b-iommu_dma_block_jgg@nvidia.com

Thanks,
Jason

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0c42ece2585406..c1bdec807ea381 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -44,6 +44,7 @@ struct iommu_group {
 	char *name;
 	int id;
 	struct iommu_domain *default_domain;
+	struct iommu_domain *blocking_domain;
 	struct iommu_domain *domain;
 	struct list_head entry;
 	unsigned int owner_cnt;
@@ -82,8 +83,7 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group);
-static void __iommu_detach_group(struct iommu_domain *domain,
-				 struct iommu_group *group);
+static void __iommu_group_attach_core_domain(struct iommu_group *group);
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -596,6 +596,8 @@ static void iommu_group_release(struct kobject *kobj)
 
 	if (group->default_domain)
 		iommu_domain_free(group->default_domain);
+	if (group->blocking_domain)
+		iommu_domain_free(group->blocking_domain);
 
 	kfree(group->name);
 	kfree(group);
@@ -1963,9 +1965,6 @@ static void __iommu_detach_device(struct iommu_domain *domain,
 	if (iommu_is_attach_deferred(dev))
 		return;
 
-	if (unlikely(domain->ops->detach_dev == NULL))
-		return;
-
 	domain->ops->detach_dev(domain, dev);
 	trace_detach_device_from_domain(dev);
 }
@@ -1979,12 +1978,10 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 		return;
 
 	mutex_lock(&group->mutex);
-	if (iommu_group_device_count(group) != 1) {
-		WARN_ON(1);
+	if (WARN_ON(domain != group->domain) ||
+	    WARN_ON(iommu_group_device_count(group) != 1))
 		goto out_unlock;
-	}
-
-	__iommu_detach_group(domain, group);
+	__iommu_group_attach_core_domain(group);
 
 out_unlock:
 	mutex_unlock(&group->mutex);
@@ -2040,7 +2037,8 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (group->domain && group->domain != group->default_domain)
+	if (group->domain && group->domain != group->default_domain &&
+	    group->domain != group->blocking_domain)
 		return -EBUSY;
 
 	ret = __iommu_group_for_each_dev(group, domain,
@@ -2072,38 +2070,68 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 	return 0;
 }
 
-static void __iommu_detach_group(struct iommu_domain *domain,
-				 struct iommu_group *group)
+static int __iommu_group_attach_domain(struct iommu_group *group,
+				       struct iommu_domain *new_domain)
 {
 	int ret;
 
+	if (group->domain == new_domain)
+		return 0;
+
 	/*
-	 * If the group has been claimed already, do not re-attach the default
-	 * domain.
+	 * New drivers should support default domains and so the detach_dev() op
+	 * will never be called. Otherwise the NULL domain indicates the
+	 * translation for the group should be set so it will work with the
+	 * platform DMA ops.
 	 */
-	if (!group->default_domain || group->owner) {
-		__iommu_group_for_each_dev(group, domain,
+	if (!new_domain) {
+		if (WARN_ON(!group->domain->ops->detach_dev))
+			return -EINVAL;
+		__iommu_group_for_each_dev(group, group->domain,
 					   iommu_group_do_detach_device);
 		group->domain = NULL;
-		return;
+		return 0;
 	}
 
-	if (group->domain == group->default_domain)
-		return;
-
-	/* Detach by re-attaching to the default domain */
-	ret = __iommu_group_for_each_dev(group, group->default_domain,
+	/*
+	 * Changing the domain is done by calling attach on the new domain.
+	 * Drivers should implement this so that DMA is always translated by
+	 * either the new, old, or a blocking domain. DMA should never become
+	 * untranslated.
+	 *
+	 * Note that this is called in error unwind paths, attaching to a
+	 * domain that has already been attached cannot fail.
+	 */
+	ret = __iommu_group_for_each_dev(group, new_domain,
 					 iommu_group_do_attach_device);
-	if (ret != 0)
-		WARN_ON(1);
+	if (ret)
+		return ret;
+	group->domain = new_domain;
+	return 0;
+}
+
+/*
+ * Put the group's domain back to the appropriate core-owned domain - either the
+ * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
+ */
+static void __iommu_group_attach_core_domain(struct iommu_group *group)
+{
+	struct iommu_domain *new_domain;
+	int ret;
+
+	if (group->owner)
+		new_domain = group->blocking_domain;
 	else
-		group->domain = group->default_domain;
+		new_domain = group->default_domain;
+
+	ret = __iommu_group_attach_domain(group, new_domain);
+	WARN(ret, "iommu driver failed to attach the default/blocking domain");
 }
 
 void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group)
 {
 	mutex_lock(&group->mutex);
-	__iommu_detach_group(domain, group);
+	__iommu_group_attach_core_domain(group);
 	mutex_unlock(&group->mutex);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_group);
@@ -3088,6 +3116,29 @@ void iommu_device_unuse_default_domain(struct device *dev)
 	iommu_group_put(group);
 }
 
+static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
+{
+	struct group_device *dev =
+		list_first_entry(&group->devices, struct group_device, list);
+
+	if (group->blocking_domain)
+		return 0;
+
+	group->blocking_domain =
+		__iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
+	if (!group->blocking_domain) {
+		/*
+		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
+		 * create an empty domain instead.
+		 */
+		group->blocking_domain = __iommu_domain_alloc(
+			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
+		if (!group->blocking_domain)
+			return -EINVAL;
+	}
+	return 0;
+}
+
 /**
  * iommu_group_claim_dma_owner() - Set DMA ownership of a group
  * @group: The group.
@@ -3111,9 +3162,15 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
 			goto unlock_out;
 		}
 
+		ret = __iommu_group_alloc_blocking_domain(group);
+		if (ret)
+			goto unlock_out;
+
+		ret = __iommu_group_attach_domain(group,
+						  group->blocking_domain);
+		if (ret)
+			goto unlock_out;
 		group->owner = owner;
-		if (group->domain)
-			__iommu_detach_group(group->domain, group);
 	}
 
 	group->owner_cnt++;
@@ -3137,13 +3194,8 @@ void iommu_group_release_dma_owner(struct iommu_group *group)
 		goto unlock_out;
 
 	group->owner_cnt = 0;
-	/*
-	 * The UNMANAGED domain should be detached before all USER
-	 * owners have been released.
-	 */
-	if (!WARN_ON(group->domain) && group->default_domain)
-		__iommu_attach_group(group->default_domain, group);
 	group->owner = NULL;
+	__iommu_group_attach_core_domain(group);
 unlock_out:
 	mutex_unlock(&group->mutex);
 }

base-commit: dc7afe17339c2f5de8c377aaa0b976139a19e158
-- 
2.36.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, May 5, 2022 3:09 AM
> 
> Once the group enters 'owned' mode it can never be assigned back to the
> default_domain or to a NULL domain. It must always be actively assigned to

worth pointing out that a NULL domain is not always translated to DMA
blocked on all platforms. That was a wrong assumption before this patch.

> a current domain. If the caller hasn't provided a domain then the core
> must provide an explicit DMA blocking domain that has no DMA map.
> 
> Lazily create a group-global blocking DMA domain when
> iommu_group_claim_dma_owner is first called and immediately assign the
> group to it. This ensures that DMA is immediately fully isolated on all
> IOMMU drivers.
> 
> If the user attaches/detaches while owned then detach will set the group
> back to the blocking domain.
> 
> Slightly reorganize the call chains so that
> __iommu_group_attach_core_domain() is the function that removes any
> caller
> configured domain and sets the domains back a core owned domain with an
> appropriate lifetime.
> 
> __iommu_group_attach_domain() is the worker function that can change the
> domain assigned to a group to any target domain, including NULL.
> 
> Add comments clarifying how the NULL vs detach_dev vs default_domain
> works
> based on Robin's remarks.
> 
> This fixes an oops with VFIO and SMMUv3 because VFIO will call
> iommu_detach_group() and then immediately iommu_domain_free(), but
> SMMUv3 has no way to know that the domain it is holding a pointer to
> has been freed. Now the iommu_detach_group() will assign the blocking
> domain and SMMUv3 will no longer hold a stale domain reference.

Overall I like what this patch does. Just some nits below.

> 
> Fixes: 1ea2a07a532b ("iommu: Add DMA ownership management
> interfaces")
> Reported-by: Qian Cai <quic_qiancai@quicinc.com>
> Tested-by: Baolu Lu <baolu.lu@linux.intel.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Co-developed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommu.c | 122 ++++++++++++++++++++++++++++++------------
>  1 file changed, 87 insertions(+), 35 deletions(-)
> 
> Joerg, this should address the issue, Nicolin reproduced the original issue
> and verified this fix on ARM SMMUv3.
> 
> v2:
>  - Remove redundant detach_dev ops check in __iommu_detach_device and
>    make the added WARN_ON fail instead
>  - Check for blocking_domain in __iommu_attach_group() so VFIO can
>    actually attach a new group
>  - Update comments and spelling
>  - Fix missed change to new_domain in iommu_group_do_detach_device()
> v1: https://lore.kernel.org/r/0-v1-6e9d2d0a759d+11b-
> iommu_dma_block_jgg@nvidia.com
> 
> Thanks,
> Jason
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0c42ece2585406..c1bdec807ea381 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -44,6 +44,7 @@ struct iommu_group {
>  	char *name;
>  	int id;
>  	struct iommu_domain *default_domain;
> +	struct iommu_domain *blocking_domain;
>  	struct iommu_domain *domain;
>  	struct list_head entry;
>  	unsigned int owner_cnt;
> @@ -82,8 +83,7 @@ static int __iommu_attach_device(struct
> iommu_domain *domain,
>  				 struct device *dev);
>  static int __iommu_attach_group(struct iommu_domain *domain,
>  				struct iommu_group *group);
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group);
> +static void __iommu_group_attach_core_domain(struct iommu_group
> *group);
>  static int iommu_create_device_direct_mappings(struct iommu_group
> *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> @@ -596,6 +596,8 @@ static void iommu_group_release(struct kobject
> *kobj)
> 
>  	if (group->default_domain)
>  		iommu_domain_free(group->default_domain);
> +	if (group->blocking_domain)
> +		iommu_domain_free(group->blocking_domain);
> 
>  	kfree(group->name);
>  	kfree(group);
> @@ -1963,9 +1965,6 @@ static void __iommu_detach_device(struct
> iommu_domain *domain,
>  	if (iommu_is_attach_deferred(dev))
>  		return;
> 
> -	if (unlikely(domain->ops->detach_dev == NULL))
> -		return;
> -
>  	domain->ops->detach_dev(domain, dev);
>  	trace_detach_device_from_domain(dev);
>  }
> @@ -1979,12 +1978,10 @@ void iommu_detach_device(struct
> iommu_domain *domain, struct device *dev)
>  		return;
> 
>  	mutex_lock(&group->mutex);
> -	if (iommu_group_device_count(group) != 1) {
> -		WARN_ON(1);
> +	if (WARN_ON(domain != group->domain) ||
> +	    WARN_ON(iommu_group_device_count(group) != 1))
>  		goto out_unlock;
> -	}
> -
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_attach_core_domain(group);
> 
>  out_unlock:
>  	mutex_unlock(&group->mutex);
> @@ -2040,7 +2037,8 @@ static int __iommu_attach_group(struct
> iommu_domain *domain,
>  {
>  	int ret;
> 
> -	if (group->domain && group->domain != group->default_domain)
> +	if (group->domain && group->domain != group->default_domain &&
> +	    group->domain != group->blocking_domain)
>  		return -EBUSY;
> 
>  	ret = __iommu_group_for_each_dev(group, domain,

Suppose this can be also replaced by __iommu_group_attach_domain()? 

> @@ -2072,38 +2070,68 @@ static int
> iommu_group_do_detach_device(struct device *dev, void *data)
>  	return 0;
>  }
> 
> -static void __iommu_detach_group(struct iommu_domain *domain,
> -				 struct iommu_group *group)
> +static int __iommu_group_attach_domain(struct iommu_group *group,
> +				       struct iommu_domain *new_domain)
>  {
>  	int ret;
> 
> +	if (group->domain == new_domain)
> +		return 0;
> +
>  	/*
> -	 * If the group has been claimed already, do not re-attach the default
> -	 * domain.
> +	 * New drivers should support default domains and so the
> detach_dev() op
> +	 * will never be called. Otherwise the NULL domain indicates the
> +	 * translation for the group should be set so it will work with the

translation should be 'blocked'?

> +	 * platform DMA ops.

I didn't get the meaning of the last sentence.

>  	 */
> -	if (!group->default_domain || group->owner) {
> -		__iommu_group_for_each_dev(group, domain,
> +	if (!new_domain) {
> +		if (WARN_ON(!group->domain->ops->detach_dev))
> +			return -EINVAL;
> +		__iommu_group_for_each_dev(group, group->domain,
>  					   iommu_group_do_detach_device);
>  		group->domain = NULL;
> -		return;
> +		return 0;
>  	}
> 
> -	if (group->domain == group->default_domain)
> -		return;
> -
> -	/* Detach by re-attaching to the default domain */
> -	ret = __iommu_group_for_each_dev(group, group->default_domain,
> +	/*
> +	 * Changing the domain is done by calling attach on the new domain.
> +	 * Drivers should implement this so that DMA is always translated by

what does 'this' mean?

> +	 * either the new, old, or a blocking domain. DMA should never

isn't the blocking domain passed in as the new domain?

> become
> +	 * untranslated.
> +	 *
> +	 * Note that this is called in error unwind paths, attaching to a
> +	 * domain that has already been attached cannot fail.

this is called in the normal path. Where does error unwind happen?

> +	 */
> +	ret = __iommu_group_for_each_dev(group, new_domain,
>  					 iommu_group_do_attach_device);
> -	if (ret != 0)
> -		WARN_ON(1);
> +	if (ret)
> +		return ret;
> +	group->domain = new_domain;
> +	return 0;
> +}
> +
> +/*
> + * Put the group's domain back to the appropriate core-owned domain -
> either the
> + * standard kernel-mode DMA configuration or an all-DMA-blocked domain.
> + */
> +static void __iommu_group_attach_core_domain(struct iommu_group
> *group)
> +{
> +	struct iommu_domain *new_domain;
> +	int ret;
> +
> +	if (group->owner)
> +		new_domain = group->blocking_domain;
>  	else
> -		group->domain = group->default_domain;
> +		new_domain = group->default_domain;
> +
> +	ret = __iommu_group_attach_domain(group, new_domain);
> +	WARN(ret, "iommu driver failed to attach the default/blocking
> domain");
>  }
> 
>  void iommu_detach_group(struct iommu_domain *domain, struct
> iommu_group *group)
>  {
>  	mutex_lock(&group->mutex);
> -	__iommu_detach_group(domain, group);
> +	__iommu_group_attach_core_domain(group);
>  	mutex_unlock(&group->mutex);
>  }
>  EXPORT_SYMBOL_GPL(iommu_detach_group);
> @@ -3088,6 +3116,29 @@ void
> iommu_device_unuse_default_domain(struct device *dev)
>  	iommu_group_put(group);
>  }
> 
> +static int __iommu_group_alloc_blocking_domain(struct iommu_group
> *group)
> +{
> +	struct group_device *dev =
> +		list_first_entry(&group->devices, struct group_device, list);
> +
> +	if (group->blocking_domain)
> +		return 0;
> +
> +	group->blocking_domain =
> +		__iommu_domain_alloc(dev->dev->bus,
> IOMMU_DOMAIN_BLOCKED);
> +	if (!group->blocking_domain) {
> +		/*
> +		 * For drivers that do not yet understand
> IOMMU_DOMAIN_BLOCKED
> +		 * create an empty domain instead.
> +		 */
> +		group->blocking_domain = __iommu_domain_alloc(
> +			dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> +		if (!group->blocking_domain)
> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>   * @group: The group.
> @@ -3111,9 +3162,15 @@ int iommu_group_claim_dma_owner(struct
> iommu_group *group, void *owner)
>  			goto unlock_out;
>  		}
> 
> +		ret = __iommu_group_alloc_blocking_domain(group);
> +		if (ret)
> +			goto unlock_out;
> +
> +		ret = __iommu_group_attach_domain(group,
> +						  group->blocking_domain);
> +		if (ret)
> +			goto unlock_out;
>  		group->owner = owner;

Here can use __iommu_group_attach_core_domain() if calling it after
setting group->owner.

> -		if (group->domain)
> -			__iommu_detach_group(group->domain, group);
>  	}
> 
>  	group->owner_cnt++;
> @@ -3137,13 +3194,8 @@ void iommu_group_release_dma_owner(struct
> iommu_group *group)
>  		goto unlock_out;
> 
>  	group->owner_cnt = 0;
> -	/*
> -	 * The UNMANAGED domain should be detached before all USER
> -	 * owners have been released.
> -	 */
> -	if (!WARN_ON(group->domain) && group->default_domain)
> -		__iommu_attach_group(group->default_domain, group);
>  	group->owner = NULL;
> +	__iommu_group_attach_core_domain(group);
>  unlock_out:
>  	mutex_unlock(&group->mutex);
>  }
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Thu, May 05, 2022 at 10:56:28AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Thursday, May 5, 2022 3:09 AM
> > 
> > Once the group enters 'owned' mode it can never be assigned back to the
> > default_domain or to a NULL domain. It must always be actively assigned to
> 
> worth pointing out that a NULL domain is not always translated to DMA
> blocked on all platforms. That was a wrong assumption before this patch.

This is described in a comment blow

> > @@ -2040,7 +2037,8 @@ static int __iommu_attach_group(struct
> > iommu_domain *domain,
> >  {
> >  	int ret;
> > 
> > -	if (group->domain && group->domain != group->default_domain)
> > +	if (group->domain && group->domain != group->default_domain &&
> > +	    group->domain != group->blocking_domain)
> >  		return -EBUSY;
> > 
> >  	ret = __iommu_group_for_each_dev(group, domain,
> 
> Suppose this can be also replaced by __iommu_group_attach_domain()? 

It can, but lets do this as a follow patching since it is a bit big

> > @@ -2072,38 +2070,68 @@ static int
> > iommu_group_do_detach_device(struct device *dev, void *data)
> >  	return 0;
> >  }
> > 
> > -static void __iommu_detach_group(struct iommu_domain *domain,
> > -				 struct iommu_group *group)
> > +static int __iommu_group_attach_domain(struct iommu_group *group,
> > +				       struct iommu_domain *new_domain)
> >  {
> >  	int ret;
> > 
> > +	if (group->domain == new_domain)
> > +		return 0;
> > +
> >  	/*
> > -	 * If the group has been claimed already, do not re-attach the default
> > -	 * domain.
> > +	 * New drivers should support default domains and so the
> > detach_dev() op
> > +	 * will never be called. Otherwise the NULL domain indicates the
> > +	 * translation for the group should be set so it will work with the
> 
> translation should be 'blocked'?

No, not blocked.

> > +	 * platform DMA ops.
> 
> I didn't get the meaning of the last sentence.

It is as discussed with Robin, NULL means to return the group back to
some platform defined translation, and in some cases this means
returning back to work with the platform's DMA ops - presumably if it
isn't using the dma api.

> > +	/*
> > +	 * Changing the domain is done by calling attach on the new domain.
> > +	 * Drivers should implement this so that DMA is always translated by
>
> what does 'this' mean?

The code below - attach_dev called in a loop for each dev in the group.

> > +	 * either the new, old, or a blocking domain. DMA should never
> 
> isn't the blocking domain passed in as the new domain?

Soemtimes. This is a statement about the required
atomicity. New/old/block are all valid translations during the
operation. Identity is not.
 
> > +	 * untranslated.
> > +	 *
> > +	 * Note that this is called in error unwind paths, attaching to a
> > +	 * domain that has already been attached cannot fail.
> 
> this is called in the normal path. Where does error unwind happen?

Any place that checks the return and does WARN_ON

Also the loop over each dev doesn't error unwind so it assumes that if
the first dev succeeds then all subsequent devs will succeed.
> >  /**
> >   * iommu_group_claim_dma_owner() - Set DMA ownership of a group
> >   * @group: The group.
> > @@ -3111,9 +3162,15 @@ int iommu_group_claim_dma_owner(struct
> > iommu_group *group, void *owner)
> >  			goto unlock_out;
> >  		}
> > 
> > +		ret = __iommu_group_alloc_blocking_domain(group);
> > +		if (ret)
> > +			goto unlock_out;
> > +
> > +		ret = __iommu_group_attach_domain(group,
> > +						  group->blocking_domain);
> > +		if (ret)
> > +			goto unlock_out;
> >  		group->owner = owner;
> 
> Here can use __iommu_group_attach_core_domain() if calling it after
> setting group->owner.

That is obfuscating. This function must always and only attach the
blocking_domain.

So, I'm going to leave this patch as-is since it has been tested now
and we need to get the series back into iommu-next ASAP.

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-05 16:33, Jason Gunthorpe wrote:
> On Thu, May 05, 2022 at 10:56:28AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Thursday, May 5, 2022 3:09 AM
>>>
>>> Once the group enters 'owned' mode it can never be assigned back to the
>>> default_domain or to a NULL domain. It must always be actively assigned to
>>
>> worth pointing out that a NULL domain is not always translated to DMA
>> blocked on all platforms. That was a wrong assumption before this patch.
> 
> This is described in a comment blow
> 
>>> @@ -2040,7 +2037,8 @@ static int __iommu_attach_group(struct
>>> iommu_domain *domain,
>>>   {
>>>   	int ret;
>>>
>>> -	if (group->domain && group->domain != group->default_domain)
>>> +	if (group->domain && group->domain != group->default_domain &&
>>> +	    group->domain != group->blocking_domain)
>>>   		return -EBUSY;
>>>
>>>   	ret = __iommu_group_for_each_dev(group, domain,
>>
>> Suppose this can be also replaced by __iommu_group_attach_domain()?
> 
> It can, but lets do this as a follow patching since it is a bit big
> 
>>> @@ -2072,38 +2070,68 @@ static int
>>> iommu_group_do_detach_device(struct device *dev, void *data)
>>>   	return 0;
>>>   }
>>>
>>> -static void __iommu_detach_group(struct iommu_domain *domain,
>>> -				 struct iommu_group *group)
>>> +static int __iommu_group_attach_domain(struct iommu_group *group,
>>> +				       struct iommu_domain *new_domain)
>>>   {
>>>   	int ret;
>>>
>>> +	if (group->domain == new_domain)
>>> +		return 0;
>>> +
>>>   	/*
>>> -	 * If the group has been claimed already, do not re-attach the default
>>> -	 * domain.
>>> +	 * New drivers should support default domains and so the
>>> detach_dev() op
>>> +	 * will never be called. Otherwise the NULL domain indicates the
>>> +	 * translation for the group should be set so it will work with the
>>
>> translation should be 'blocked'?
> 
> No, not blocked.
> 
>>> +	 * platform DMA ops.
>>
>> I didn't get the meaning of the last sentence.
> 
> It is as discussed with Robin, NULL means to return the group back to
> some platform defined translation, and in some cases this means
> returning back to work with the platform's DMA ops - presumably if it
> isn't using the dma api.
> 
>>> +	/*
>>> +	 * Changing the domain is done by calling attach on the new domain.
>>> +	 * Drivers should implement this so that DMA is always translated by
>>
>> what does 'this' mean?
> 
> The code below - attach_dev called in a loop for each dev in the group.
> 
>>> +	 * either the new, old, or a blocking domain. DMA should never
>>
>> isn't the blocking domain passed in as the new domain?
> 
> Soemtimes. This is a statement about the required
> atomicity. New/old/block are all valid translations during the
> operation. Identity is not.
>   
>>> +	 * untranslated.
>>> +	 *
>>> +	 * Note that this is called in error unwind paths, attaching to a
>>> +	 * domain that has already been attached cannot fail.
>>
>> this is called in the normal path. Where does error unwind happen?
> 
> Any place that checks the return and does WARN_ON
> 
> Also the loop over each dev doesn't error unwind so it assumes that if
> the first dev succeeds then all subsequent devs will succeed.
>>>   /**
>>>    * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>>>    * @group: The group.
>>> @@ -3111,9 +3162,15 @@ int iommu_group_claim_dma_owner(struct
>>> iommu_group *group, void *owner)
>>>   			goto unlock_out;
>>>   		}
>>>
>>> +		ret = __iommu_group_alloc_blocking_domain(group);
>>> +		if (ret)
>>> +			goto unlock_out;
>>> +
>>> +		ret = __iommu_group_attach_domain(group,
>>> +						  group->blocking_domain);
>>> +		if (ret)
>>> +			goto unlock_out;
>>>   		group->owner = owner;
>>
>> Here can use __iommu_group_attach_core_domain() if calling it after
>> setting group->owner.
> 
> That is obfuscating. This function must always and only attach the
> blocking_domain.
> 
> So, I'm going to leave this patch as-is since it has been tested now
> and we need to get the series back into iommu-next ASAP.

Ack to that, there are certainly further improvements to consider once 
we've got known-working code into a released kernel, but let's not get 
ahead of ourselves just now.

(I'm pretty sure we could get away with a single blocking domain per 
IOMMU instance, rather than per group, but I deliberately saved that one 
for later - right now one per group to match default domains is simpler 
to reason about and less churny in the context of the current ownership 
patches)

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Thu, May 05, 2022 at 07:56:59PM +0100, Robin Murphy wrote:

> Ack to that, there are certainly further improvements to consider once we've
> got known-working code into a released kernel, but let's not get ahead of
> ourselves just now.

Yes please
 
> (I'm pretty sure we could get away with a single blocking domain per IOMMU
> instance, rather than per group, but I deliberately saved that one for later
> - right now one per group to match default domains is simpler to reason
> about and less churny in the context of the current ownership patches)

I noticed this too.. 

But I thought the driver can do a better job of this. There is no
reason it has to allocate memory to return a IOMMU_DOMAIN_BLOCKED
domain - this struct could be a globally allocated singleton for the
entire driver and that would be even better memory savings.

For instance, here is a sketch for the Intel driver based on Baolu's
remark that intel_iommu_detach_device() establishes a blocking
behavior already on detach_dev (Baolu if you like it can you make a
proper patch?):

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index df5c62ecf942b8..317945f6134d42 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4324,6 +4324,8 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
 		return domain;
 	case IOMMU_DOMAIN_IDENTITY:
 		return &si_domain->domain;
+	case IOMMU_DOMAIN_BLOCKED;
+		return &intel_blocking_domain;
 	default:
 		return NULL;
 	}
@@ -4404,12 +4406,25 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
 	return domain_add_dev_info(to_dmar_domain(domain), dev);
 }
 
-static void intel_iommu_detach_device(struct iommu_domain *domain,
-				      struct device *dev)
+static int intel_iommu_block_dev(struct iommu_domain *domain,
+				 struct device *dev)
 {
+	if (!device_to_iommu(dev, NULL, NULL))
+		return -EINVAL;
+
 	dmar_remove_one_dev_info(dev);
+	return 0;
 }
 
+static struct iommu_domain intel_blocking_domain {
+	.type = IOMMU_DOMAIN_BLOCKED,
+	.ops = &(const struct iommu_domain_ops){
+		.attach_dev = intel_iommu_detach_device,
+		// Fix core code so this works
+		.free = NULL,
+	},
+};
+
 static int intel_iommu_map(struct iommu_domain *domain,
 			   unsigned long iova, phys_addr_t hpa,
 			   size_t size, int iommu_prot, gfp_t gfp)
@@ -4890,7 +4905,6 @@ const struct iommu_ops intel_iommu_ops = {
 #endif
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev		= intel_iommu_attach_device,
-		.detach_dev		= intel_iommu_detach_device,
 		.map_pages		= intel_iommu_map_pages,
 		.unmap_pages		= intel_iommu_unmap_pages,
 		.iotlb_sync_map		= intel_iommu_iotlb_sync_map,

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-05 20:27, Jason Gunthorpe wrote:
> On Thu, May 05, 2022 at 07:56:59PM +0100, Robin Murphy wrote:
> 
>> Ack to that, there are certainly further improvements to consider once we've
>> got known-working code into a released kernel, but let's not get ahead of
>> ourselves just now.
> 
> Yes please
>   
>> (I'm pretty sure we could get away with a single blocking domain per IOMMU
>> instance, rather than per group, but I deliberately saved that one for later
>> - right now one per group to match default domains is simpler to reason
>> about and less churny in the context of the current ownership patches)
> 
> I noticed this too..
> 
> But I thought the driver can do a better job of this. There is no
> reason it has to allocate memory to return a IOMMU_DOMAIN_BLOCKED
> domain - this struct could be a globally allocated singleton for the
> entire driver and that would be even better memory savings.

I was thinking about the domain pointers themselves as well, but on 
reflection, little systems are most likely to have the simpler IOMMUs 
with one fixed group per instance anyway, while the kind of systems 
which end up with a couple of hundred groups can probably bear the cost 
of a couple of hundred extra pointers. So yeah, probably not worth the 
code cost of chasing down a group->devices->dev->iommu->blocking_domain 
in practice.

> For instance, here is a sketch for the Intel driver based on Baolu's
> remark that intel_iommu_detach_device() establishes a blocking
> behavior already on detach_dev (Baolu if you like it can you make a
> proper patch?):
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index df5c62ecf942b8..317945f6134d42 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4324,6 +4324,8 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
>   		return domain;
>   	case IOMMU_DOMAIN_IDENTITY:
>   		return &si_domain->domain;
> +	case IOMMU_DOMAIN_BLOCKED;
> +		return &intel_blocking_domain;
>   	default:
>   		return NULL;
>   	}
> @@ -4404,12 +4406,25 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
>   	return domain_add_dev_info(to_dmar_domain(domain), dev);
>   }
>   
> -static void intel_iommu_detach_device(struct iommu_domain *domain,
> -				      struct device *dev)
> +static int intel_iommu_block_dev(struct iommu_domain *domain,
> +				 struct device *dev)
>   {
> +	if (!device_to_iommu(dev, NULL, NULL))
> +		return -EINVAL;
> +
>   	dmar_remove_one_dev_info(dev);
> +	return 0;
>   }
>   
> +static struct iommu_domain intel_blocking_domain {
> +	.type = IOMMU_DOMAIN_BLOCKED,
> +	.ops = &(const struct iommu_domain_ops){
> +		.attach_dev = intel_iommu_detach_device,
> +		// Fix core code so this works
> +		.free = NULL,

TBH unless and until it becomes commonplace, I'd just have drivers 
provide a no-op .free callback for least surprise, if their .alloc does 
something funky. But either way, with domain ops now we can so easily do 
whatever we need, yay!

Cheers,
Robin.

> +	},
> +};
> +
>   static int intel_iommu_map(struct iommu_domain *domain,
>   			   unsigned long iova, phys_addr_t hpa,
>   			   size_t size, int iommu_prot, gfp_t gfp)
> @@ -4890,7 +4905,6 @@ const struct iommu_ops intel_iommu_ops = {
>   #endif
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
>   		.attach_dev		= intel_iommu_attach_device,
> -		.detach_dev		= intel_iommu_detach_device,
>   		.map_pages		= intel_iommu_map_pages,
>   		.unmap_pages		= intel_iommu_unmap_pages,
>   		.iotlb_sync_map		= intel_iommu_iotlb_sync_map,
> 
> Thanks,
> Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Baolu Lu]

On 2022/5/6 03:27, Jason Gunthorpe wrote:
> On Thu, May 05, 2022 at 07:56:59PM +0100, Robin Murphy wrote:
> 
>> Ack to that, there are certainly further improvements to consider once we've
>> got known-working code into a released kernel, but let's not get ahead of
>> ourselves just now.
> Yes please
>   
>> (I'm pretty sure we could get away with a single blocking domain per IOMMU
>> instance, rather than per group, but I deliberately saved that one for later
>> - right now one per group to match default domains is simpler to reason
>> about and less churny in the context of the current ownership patches)
> I noticed this too..
> 
> But I thought the driver can do a better job of this. There is no
> reason it has to allocate memory to return a IOMMU_DOMAIN_BLOCKED
> domain - this struct could be a globally allocated singleton for the
> entire driver and that would be even better memory savings.
> 
> For instance, here is a sketch for the Intel driver based on Baolu's
> remark that intel_iommu_detach_device() establishes a blocking
> behavior already on detach_dev (Baolu if you like it can you make a
> proper patch?):

Yes, I will.

The same scheme could also be applied to identity/sva/block domains.
Perhaps make it after v5.19.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Tian, Kevin]

> From: Jason Gunthorpe
> Sent: Thursday, May 5, 2022 11:33 PM
> > >  	/*
> > > -	 * If the group has been claimed already, do not re-attach the default
> > > -	 * domain.
> > > +	 * New drivers should support default domains and so the
> > > detach_dev() op
> > > +	 * will never be called. Otherwise the NULL domain indicates the
> > > +	 * translation for the group should be set so it will work with the
> >
> > translation should be 'blocked'?
> 
> No, not blocked.
> 
> > > +	 * platform DMA ops.
> >
> > I didn't get the meaning of the last sentence.
> 
> It is as discussed with Robin, NULL means to return the group back to
> some platform defined translation, and in some cases this means
> returning back to work with the platform's DMA ops - presumably if it
> isn't using the dma api.

This is clearer than the original text.

> 
> > > +	/*
> > > +	 * Changing the domain is done by calling attach on the new domain.
> > > +	 * Drivers should implement this so that DMA is always translated by
> >
> > what does 'this' mean?
> 
> The code below - attach_dev called in a loop for each dev in the group.

Yes.

> 
> > > +	 * either the new, old, or a blocking domain. DMA should never
> >
> > isn't the blocking domain passed in as the new domain?
> 
> Soemtimes. This is a statement about the required
> atomicity. New/old/block are all valid translations during the
> operation. Identity is not.

but new/old/block are not the same type of classifications. A group
has an old domain and a new domain at this transition point, and
both old/new domains have a domain type (dma, unmanged, block,
identity, etc.). Mixing them together only increases confusion here.

> 
> So, I'm going to leave this patch as-is since it has been tested now
> and we need to get the series back into iommu-next ASAP.
> 

Agree. Just hope some improvements on the code comment.

But either way given no more code change:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-06 00:28, Tian, Kevin wrote:
>> From: Jason Gunthorpe
>> Sent: Thursday, May 5, 2022 11:33 PM
>>>>   	/*
>>>> -	 * If the group has been claimed already, do not re-attach the default
>>>> -	 * domain.
>>>> +	 * New drivers should support default domains and so the
>>>> detach_dev() op
>>>> +	 * will never be called. Otherwise the NULL domain indicates the
>>>> +	 * translation for the group should be set so it will work with the
>>>
>>> translation should be 'blocked'?
>>
>> No, not blocked.
>>
>>>> +	 * platform DMA ops.
>>>
>>> I didn't get the meaning of the last sentence.
>>
>> It is as discussed with Robin, NULL means to return the group back to
>> some platform defined translation, and in some cases this means
>> returning back to work with the platform's DMA ops - presumably if it
>> isn't using the dma api.
> 
> This is clearer than the original text.

Perhaps we could just leave that sentence as "Otherwise the NULL domain 
represents platform-specific behaviour."

>>
>>>> +	/*
>>>> +	 * Changing the domain is done by calling attach on the new domain.
>>>> +	 * Drivers should implement this so that DMA is always translated by
>>>
>>> what does 'this' mean?
>>
>> The code below - attach_dev called in a loop for each dev in the group.
> 
> Yes.
> 
>>
>>>> +	 * either the new, old, or a blocking domain. DMA should never
>>>
>>> isn't the blocking domain passed in as the new domain?
>>
>> Soemtimes. This is a statement about the required
>> atomicity. New/old/block are all valid translations during the
>> operation. Identity is not.
> 
> but new/old/block are not the same type of classifications. A group
> has an old domain and a new domain at this transition point, and
> both old/new domains have a domain type (dma, unmanged, block,
> identity, etc.). Mixing them together only increases confusion here.

Good point - in particular I think the "DMA is always translated" part 
would be more accurately said as "DMA is always managed". When we're 
reattaching back to the default domain here, and it happens to be an 
identity domain, then DMA *may* be untranslated, but in a manner that 
we've knowingly chosen. The key point is that if the driver supports 
core domains, then it should never have any in-between state that allows 
access to anything *other* than the current domain or the new domain.

Thanks,
Robin.

> 
>>
>> So, I'm going to leave this patch as-is since it has been tested now
>> and we need to get the series back into iommu-next ASAP.
>>
> 
> Agree. Just hope some improvements on the code comment.
> 
> But either way given no more code change:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Fri, May 06, 2022 at 10:32:50AM +0100, Robin Murphy wrote:

> > > It is as discussed with Robin, NULL means to return the group back to
> > > some platform defined translation, and in some cases this means
> > > returning back to work with the platform's DMA ops - presumably if it
> > > isn't using the dma api.
> > 
> > This is clearer than the original text.
> 
> Perhaps we could just leave that sentence as "Otherwise the NULL domain
> represents platform-specific behaviour."

Sure, there are only three drivers that use this call path and who
knows what they do I guess..

> > > Soemtimes. This is a statement about the required
> > > atomicity. New/old/block are all valid translations during the
> > > operation. Identity is not.
> > 
> > but new/old/block are not the same type of classifications. A group
> > has an old domain and a new domain at this transition point, and
> > both old/new domains have a domain type (dma, unmanged, block,
> > identity, etc.). Mixing them together only increases confusion here.

Block refers to the global concept of blocking - not to a type.

> Good point - in particular I think the "DMA is always translated" part would
> be more accurately said as "DMA is always managed". 

So you want to say "DMA is always managed" when attaching a domain of
type IOMMU_DOMAIN_UNMANAGED? :)

Lets just clarify a bit that blocking isn't talking about a domain
type:

	/*
	 * Changing the domain is done by calling attach_dev() on the new
	 * domain. This switch does not have to be atomic and DMA can be
	 * discarded during the transition. DMA must always be translated by
	 * either new_domain or group->domain, never something else.
	 *
	 * Note that this is called in error unwind paths, attaching to a
	 * domain that has already been attached cannot fail.
	 */

(aside, I expect down the road we will have some domain capability
'attach is atomic' meaning if new and old have the same translation
then there is no disruption to DMA)

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-06 14:10, Jason Gunthorpe wrote:
> On Fri, May 06, 2022 at 10:32:50AM +0100, Robin Murphy wrote:
> 
>>>> It is as discussed with Robin, NULL means to return the group back to
>>>> some platform defined translation, and in some cases this means
>>>> returning back to work with the platform's DMA ops - presumably if it
>>>> isn't using the dma api.
>>>
>>> This is clearer than the original text.
>>
>> Perhaps we could just leave that sentence as "Otherwise the NULL domain
>> represents platform-specific behaviour."
> 
> Sure, there are only three drivers that use this call path and who
> knows what they do I guess..
> 
>>>> Soemtimes. This is a statement about the required
>>>> atomicity. New/old/block are all valid translations during the
>>>> operation. Identity is not.
>>>
>>> but new/old/block are not the same type of classifications. A group
>>> has an old domain and a new domain at this transition point, and
>>> both old/new domains have a domain type (dma, unmanged, block,
>>> identity, etc.). Mixing them together only increases confusion here.
> 
> Block refers to the global concept of blocking - not to a type.
> 
>> Good point - in particular I think the "DMA is always translated" part would
>> be more accurately said as "DMA is always managed".
> 
> So you want to say "DMA is always managed" when attaching a domain of
> type IOMMU_DOMAIN_UNMANAGED? :)

Touché ;) Just goes to confirm the point above that confusion between 
general concepts and specific API terms is all too easy. An "unmanaged" 
domain from the PoV of the API just means it's managed by the external 
caller, but you're right that that's not necessarily so obvious either.

> Lets just clarify a bit that blocking isn't talking about a domain
> type:
> 
> 	/*
> 	 * Changing the domain is done by calling attach_dev() on the new
> 	 * domain. This switch does not have to be atomic and DMA can be
> 	 * discarded during the transition. DMA must always be translated by

s/always be translated by/only be able to access/ and we have a deal :)

Cheers,
Robin.

> 	 * either new_domain or group->domain, never something else.
> 	 *
> 	 * Note that this is called in error unwind paths, attaching to a
> 	 * domain that has already been attached cannot fail.
> 	 */
> 
> (aside, I expect down the road we will have some domain capability
> 'attach is atomic' meaning if new and old have the same translation
> then there is no disruption to DMA)
> 
> Thanks,
> Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Fri, May 06, 2022 at 02:35:50PM +0100, Robin Murphy wrote:

> > So you want to say "DMA is always managed" when attaching a domain of
> > type IOMMU_DOMAIN_UNMANAGED? :)
> 
> Touché ;) Just goes to confirm the point above that confusion between
> general concepts and specific API terms is all too easy. An "unmanaged"
> domain from the PoV of the API just means it's managed by the external
> caller, but you're right that that's not necessarily so obvious either.

Yeah, I'm not so keen on the naming used for IOMMU_DOMAIN_*

I looked for a bit and could not figure out why we need to have
IOMMU_DOMAIN_DMA either.. I didn't find anthing obvious in the iommu
drivers that looked like a special case for this? Most drivers treat
it identically to UNMANAGED in their alloc functions

Only mtk, arm-smmu and some odd stuff in Intel seemed to be sensitive?

> > 	/*
> > 	 * Changing the domain is done by calling attach_dev() on the new
> > 	 * domain. This switch does not have to be atomic and DMA can be
> > 	 * discarded during the transition. DMA must always be translated by
> 
> s/always be translated by/only be able to access/ and we have a deal :)

Done, thanks

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-06 14:54, Jason Gunthorpe wrote:
> On Fri, May 06, 2022 at 02:35:50PM +0100, Robin Murphy wrote:
> 
>>> So you want to say "DMA is always managed" when attaching a domain of
>>> type IOMMU_DOMAIN_UNMANAGED? :)
>>
>> Touché ;) Just goes to confirm the point above that confusion between
>> general concepts and specific API terms is all too easy. An "unmanaged"
>> domain from the PoV of the API just means it's managed by the external
>> caller, but you're right that that's not necessarily so obvious either.
> 
> Yeah, I'm not so keen on the naming used for IOMMU_DOMAIN_*
> 
> I looked for a bit and could not figure out why we need to have
> IOMMU_DOMAIN_DMA either.. I didn't find anthing obvious in the iommu
> drivers that looked like a special case for this? Most drivers treat
> it identically to UNMANAGED in their alloc functions
> 
> Only mtk, arm-smmu and some odd stuff in Intel seemed to be sensitive?

Yes, that's a relatively recent change[1] - prior to that, drivers did 
still have to take (minimal) action to opt into iommu-dma support. 
IOMMU_DOMAIN_DMA still needs to exist as a type for the core code to 
differentiate internally, so driver involvement is mostly now just a 
case of saying "yeah OK fine" if they see it. IOMMU_DOMAIN_DMA_FQ now 
effectively takes over the original semantics of IOMMU_DOMAIN_DMA as the 
one that depends on driver-specific functionality.

FWIW, mtk-iommu doesn't really have any need to reject 
IOMMU_DOMAIN_UNMANAGED, they just don't have any relevant client drivers 
that want it; however arm-smmu definitely does need to continue 
rejecting IOMMU_DOMAIN_DMA when it can't rely on the DT code ensuring 
the correct probe ordering with the old DT binding (otherwise client 
drivers are liable to get broken DMA ops).

Thanks,
Robin.

[1] 
https://lore.kernel.org/linux-iommu/cover.1628682048.git.robin.murphy@arm.com/
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Mon, May 09, 2022 at 10:59:11AM +0100, Robin Murphy wrote:

> IOMMU_DOMAIN_DMA_FQ now effectively takes over the original
> semantics of IOMMU_DOMAIN_DMA as the one that depends on
> driver-specific functionality.

If I grasp the FQ stuff right, it seems that this only requires the
driver to implement ops->flush_iotlb_all()? I don't see anything
obvious in any driver that is specifically _FQ related?

If yes, it makes me wonder why I see drivers implementing
ops->flush_iotlb_all() but not supporting the _FQ domain during alloc?

Further, if yes, wouldn't it make sense to just trigger FQ based on
domain->op->flush_iotlb_all being set?

It seems like there is some confusion here, because I see the sysfs
default domain store path just does this:

	/* We can bring up a flush queue without tearing down the domain */
	if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
		ret = iommu_dma_init_fq(prev_dom);
		if (!ret)
			prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
		goto out;
	}

Which will allow a driver that rejected creating DMA_FQ during alloc
to end up with DMA_FQ anyhow???

> FWIW, mtk-iommu doesn't really have any need to reject
> IOMMU_DOMAIN_UNMANAGED, they just don't have any relevant client drivers
> that want it;

Ok..

> however arm-smmu definitely does need to continue rejecting
> IOMMU_DOMAIN_DMA when it can't rely on the DT code ensuring the
> correct probe ordering with the old DT binding (otherwise client
> drivers are liable to get broken DMA ops).

I saw this code and wondered what it does?

smmu alloc_domain returns NULL, which if I read everything right
causes NULL to become the default_domain.

But then what happens? This driver has no detach_dev so after, say
VFIO does stuff, how do we get back into whatever boot-time state NULL
represents?

Is it OK because dev_iommu_priv_get() in arm_smmu_attach_dev() will
always fail if legacy? If that is the case then why allow allocating
any domain at all?

It feels like this really wants a 'IOMMU_DOMAIN_PLATFORM_DMA_OPS' set
as the default_domain meaning that when that domain is assigned, the
platform's DMA ops are handling the iommu? If I get it right..

Anyhow, thanks, this sort of helps confirm my feeling that the domain
types are a little crufty..

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Robin Murphy]

On 2022-05-09 18:26, Jason Gunthorpe wrote:
> On Mon, May 09, 2022 at 10:59:11AM +0100, Robin Murphy wrote:
> 
>> IOMMU_DOMAIN_DMA_FQ now effectively takes over the original
>> semantics of IOMMU_DOMAIN_DMA as the one that depends on
>> driver-specific functionality.
> 
> If I grasp the FQ stuff right, it seems that this only requires the
> driver to implement ops->flush_iotlb_all()? I don't see anything
> obvious in any driver that is specifically _FQ related?
> 
> If yes, it makes me wonder why I see drivers implementing
> ops->flush_iotlb_all() but not supporting the _FQ domain during alloc?
> 
> Further, if yes, wouldn't it make sense to just trigger FQ based on
> domain->op->flush_iotlb_all being set?

The main thing drivers need to do for flush queue support is to actually 
implement the optimisations which make it worthwhile. It's up to 
individual drivers how much use they want to make of the 
iommu_iotlb_gather mechanism, and they're free to issue invalidations or 
even enforce their completion directly within their unmap callback if 
they so wish. In such cases, enabling a flush queue would do nothing but 
hurt performance and waste memory.

> It seems like there is some confusion here, because I see the sysfs
> default domain store path just does this:
> 
> 	/* We can bring up a flush queue without tearing down the domain */
> 	if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
> 		ret = iommu_dma_init_fq(prev_dom);
> 		if (!ret)
> 			prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
> 		goto out;
> 	}
> 
> Which will allow a driver that rejected creating DMA_FQ during alloc
> to end up with DMA_FQ anyhow???

Yes, I can't remember if I ever mentioned it anywhere, but that is not 
an oversight. The sysfs interface is a bit of a specialist sport, and if 
a user really wants to go out of their way to bring up a flush queue 
which won't help performance, they can, and they can observe the 
zero-to-negative performance gain, and maybe learn not to bother again 
on that system. It's just not worth the additional complexity to try to 
save users from themselves.

>> FWIW, mtk-iommu doesn't really have any need to reject
>> IOMMU_DOMAIN_UNMANAGED, they just don't have any relevant client drivers
>> that want it;
> 
> Ok..
> 
>> however arm-smmu definitely does need to continue rejecting
>> IOMMU_DOMAIN_DMA when it can't rely on the DT code ensuring the
>> correct probe ordering with the old DT binding (otherwise client
>> drivers are liable to get broken DMA ops).
> 
> I saw this code and wondered what it does?
> 
> smmu alloc_domain returns NULL, which if I read everything right
> causes NULL to become the default_domain.
> 
> But then what happens? This driver has no detach_dev so after, say
> VFIO does stuff, how do we get back into whatever boot-time state NULL
> represents?

Shhh... the main point of the arm-smmu legacy binding support is to do 
whatever the handful of people still using ancient AMD Seattle boards 
with original firmware need to do. Clearly they haven't been reassigning 
devices straight back from VFIO to a kernel driver for the last 
6-and-a-bit years since that's been broken, so I'm now discounting it as 
a supported legacy-binding-use-case. Don't give them ideas! ;)

> Is it OK because dev_iommu_priv_get() in arm_smmu_attach_dev() will
> always fail if legacy? If that is the case then why allow allocating
> any domain at all?
> 
> It feels like this really wants a 'IOMMU_DOMAIN_PLATFORM_DMA_OPS' set
> as the default_domain meaning that when that domain is assigned, the
> platform's DMA ops are handling the iommu? If I get it right..

Nah, we just need to actually finish introducing default domains. I'm OK 
to tackle the remaining SoC IOMMU drivers once my bus ops work meanders 
into the arch/arm iommu-dma conversion revival; it just needs people who 
understand s390 and fsl-pamu well enough to at least (presumably) bodge 
up enough IOMMU_DOMAIN_IDENTITY support to replicate their current 
"detached" behaviours and force CONFIG_IOMMU_DEFAULT_PASSTHROUGH on 
those architectures.

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

[Jason Gunthorpe via iommu]

On Mon, May 09, 2022 at 11:06:35PM +0100, Robin Murphy wrote:
> The main thing drivers need to do for flush queue support is to actually
> implement the optimisations which make it worthwhile. It's up to individual
> drivers how much use they want to make of the iommu_iotlb_gather mechanism,
> and they're free to issue invalidations or even enforce their completion
> directly within their unmap callback if they so wish. In such cases,
> enabling a flush queue would do nothing but hurt performance and waste
> memory.

It makes sense, but I think at this point it would be clearer to have
a domain->ops flag saying if the domain benefits from deferred flush or
not rather than overloading the type

> > But then what happens? This driver has no detach_dev so after, say
> > VFIO does stuff, how do we get back into whatever boot-time state NULL
> > represents?
> 
> Shhh... the main point of the arm-smmu legacy binding support is to do
> whatever the handful of people still using ancient AMD Seattle boards with
> original firmware need to do. Clearly they haven't been reassigning devices
> straight back from VFIO to a kernel driver for the last 6-and-a-bit years
> since that's been broken, so I'm now discounting it as a supported
> legacy-binding-use-case. Don't give them ideas! ;)

If everything is converted to use default domain what will this
return? NULL can't be a valid default domain..
 
> > It feels like this really wants a 'IOMMU_DOMAIN_PLATFORM_DMA_OPS' set
> > as the default_domain meaning that when that domain is assigned, the
> > platform's DMA ops are handling the iommu? If I get it right..
> 
> Nah, we just need to actually finish introducing default domains. I'm OK to
> tackle the remaining SoC IOMMU drivers once my bus ops work meanders into
> the arch/arm iommu-dma conversion revival; it just needs people who
> understand s390 and fsl-pamu well enough to at least (presumably) bodge up
> enough IOMMU_DOMAIN_IDENTITY support to replicate their current "detached"

Hum. Looking at s390 it is similar I think to smmu - when NULL they
expect their platform dma_ops to work in arch/s390/pci/pci_dma.c which
looks to me like a version of the normal iommu support through
IOMMU_DOMAIN_DMA. The good conversion is probably to move s390 to use
the normal dma API, the lame one is to use a
'IOMMU_DOMAIN_PLATFORM_DMA_OPS' and save it for another day.

fsl took some looking at, but it looks very much like IDENTITY. The
defconfig does not set CONFIG_DMA_OPS and there is no dma_ops code
aroudn it's arch support - so it follow the direct path. It seems like
there might be an offset though.

The two tegras look OK, they both look like blocking domains. Theirry
has been helpful at tegra stuff in pthe past.

Someone who cared about fsl tested a VFIO patch of mine last year, and
the s390 team is actively looking at nested domains right now.

> behaviours and force CONFIG_IOMMU_DEFAULT_PASSTHROUGH on those
> architectures.

Or should the iommu driver return IDENTITY from its ops
def_domain_type() ?

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu