diff mbox series

[v3,18/20] iommu: Call set_platform_dma if default domain is unavailable

Message ID 20221128064648.1934720-19-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Retire detach_dev callback |

Commit Message

Baolu Lu Nov. 28, 2022, 6:46 a.m. UTC 
  If the IOMMU driver has no default domain support, call set_platform_dma
explicitly to return the kernel DMA control back to the platform DMA ops.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

Comments

Jason Gunthorpe Nov. 28, 2022, 2:57 p.m. UTC | #1 
On Mon, Nov 28, 2022 at 02:46:46PM +0800, Lu Baolu wrote:
> If the IOMMU driver has no default domain support, call set_platform_dma
> explicitly to return the kernel DMA control back to the platform DMA ops.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7c99d8eb3182..e4966f088184 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2040,16 +2040,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>  	return 0;
>  }
>  
> -static void __iommu_detach_device(struct iommu_domain *domain,
> -				  struct device *dev)
> -{
> -	if (iommu_is_attach_deferred(dev))
> -		return;

This removal might want to be its own patch with an explanation.

It looks like at the current moment __iommu_detach_device() is only
called via call chains that are after the device driver is attached -
eg via explicit attach APIs called by the device driver.

So it should just unconditionally work. It is actually looks like a
bug that we were blocking detach on these paths since the attach was
unconditional and the caller is going to free the (probably) UNAMANGED
domain once this returns.

The only place we should be testing for deferred attach is during the
initial point the dma device is linked to the group, and then again
during the dma api calls to check if the device

This maybe the patch that is needed to explain this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d69ebba81bebd8..06f1fe6563bb30 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -993,8 +993,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
-	if (group->domain  && !iommu_is_attach_deferred(dev))
-		ret = __iommu_attach_device(group->domain, dev);
+	if (group->domain)
+		ret = iommu_group_do_dma_first_attach(dev, group->domain);
 	mutex_unlock(&group->mutex);
 	if (ret)
 		goto err_put_group;
@@ -1760,21 +1760,24 @@ static void probe_alloc_default_domain(struct bus_type *bus,
 
 }
 
-static int iommu_group_do_dma_attach(struct device *dev, void *data)
+static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
 {
 	struct iommu_domain *domain = data;
-	int ret = 0;
 
-	if (!iommu_is_attach_deferred(dev))
-		ret = __iommu_attach_device(domain, dev);
+	lockdep_assert_held(&dev->iommu_group->mutex);
 
-	return ret;
+	if (iommu_is_attach_deferred(dev)) {
+		dev->iommu->attach_deferred = 1;
+		return 0;
+	}
+
+	return __iommu_attach_device(domain, dev);
 }
 
-static int __iommu_group_dma_attach(struct iommu_group *group)
+static int __iommu_group_dma_first_attach(struct iommu_group *group)
 {
 	return __iommu_group_for_each_dev(group, group->default_domain,
-					  iommu_group_do_dma_attach);
+					  iommu_group_do_dma_first_attach);
 }
 
 static int iommu_group_do_probe_finalize(struct device *dev, void *data)
@@ -1839,7 +1842,7 @@ int bus_iommu_probe(struct bus_type *bus)
 
 		iommu_group_create_direct_mappings(group);
 
-		ret = __iommu_group_dma_attach(group);
+		ret = __iommu_group_dma_first_attach(group);
 
 		mutex_unlock(&group->mutex);
 
@@ -1971,9 +1974,11 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 		return -ENODEV;
 
 	ret = domain->ops->attach_dev(domain, dev);
-	if (!ret)
-		trace_attach_device_to_domain(dev);
-	return ret;
+	if (ret)
+		return ret;
+	dev->iommu->attach_deferred = 0;
+	trace_attach_device_to_domain(dev);
+	return 0;
 }
 
 /**
@@ -2018,7 +2023,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
 
 int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 {
-	if (iommu_is_attach_deferred(dev))
+	if (dev->iommu && dev->iommu->attach_deferred)
 		return __iommu_attach_device(domain, dev);
 
 	return 0;
@@ -2027,9 +2032,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {
-	if (iommu_is_attach_deferred(dev))
-		return;
-
 	domain->ops->detach_dev(domain, dev);
 	trace_detach_device_from_domain(dev);
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1690c334e51631..ebac04a13fff68 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -413,6 +413,7 @@ struct dev_iommu {
 	struct iommu_device		*iommu_dev;
 	void				*priv;
 	u32				max_pasids;
+	u8				attach_deferred;
 };
 
 int iommu_device_register(struct iommu_device *iommu,
Baolu Lu Jan. 3, 2023, 2:45 a.m. UTC | #2 
Hi Jason,

On 11/28/22 10:57 PM, Jason Gunthorpe wrote:
> On Mon, Nov 28, 2022 at 02:46:46PM +0800, Lu Baolu wrote:
>> If the IOMMU driver has no default domain support, call set_platform_dma
>> explicitly to return the kernel DMA control back to the platform DMA ops.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 28 ++++++++--------------------
>>   1 file changed, 8 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 7c99d8eb3182..e4966f088184 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2040,16 +2040,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>>   	return 0;
>>   }
>>   
>> -static void __iommu_detach_device(struct iommu_domain *domain,
>> -				  struct device *dev)
>> -{
>> -	if (iommu_is_attach_deferred(dev))
>> -		return;
> 
> This removal might want to be its own patch with an explanation.
> 
> It looks like at the current moment __iommu_detach_device() is only
> called via call chains that are after the device driver is attached -
> eg via explicit attach APIs called by the device driver.
> 
> So it should just unconditionally work. It is actually looks like a
> bug that we were blocking detach on these paths since the attach was
> unconditional and the caller is going to free the (probably) UNAMANGED
> domain once this returns.
> 
> The only place we should be testing for deferred attach is during the
> initial point the dma device is linked to the group, and then again
> during the dma api calls to check if the device
> 
> This maybe the patch that is needed to explain this:
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d69ebba81bebd8..06f1fe6563bb30 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -993,8 +993,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   
>   	mutex_lock(&group->mutex);
>   	list_add_tail(&device->list, &group->devices);
> -	if (group->domain  && !iommu_is_attach_deferred(dev))
> -		ret = __iommu_attach_device(group->domain, dev);
> +	if (group->domain)
> +		ret = iommu_group_do_dma_first_attach(dev, group->domain);
>   	mutex_unlock(&group->mutex);
>   	if (ret)
>   		goto err_put_group;
> @@ -1760,21 +1760,24 @@ static void probe_alloc_default_domain(struct bus_type *bus,
>   
>   }
>   
> -static int iommu_group_do_dma_attach(struct device *dev, void *data)
> +static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
>   {
>   	struct iommu_domain *domain = data;
> -	int ret = 0;
>   
> -	if (!iommu_is_attach_deferred(dev))
> -		ret = __iommu_attach_device(domain, dev);
> +	lockdep_assert_held(&dev->iommu_group->mutex);
>   
> -	return ret;
> +	if (iommu_is_attach_deferred(dev)) {
> +		dev->iommu->attach_deferred = 1;
> +		return 0;
> +	}
> +
> +	return __iommu_attach_device(domain, dev);
>   }
>   
> -static int __iommu_group_dma_attach(struct iommu_group *group)
> +static int __iommu_group_dma_first_attach(struct iommu_group *group)
>   {
>   	return __iommu_group_for_each_dev(group, group->default_domain,
> -					  iommu_group_do_dma_attach);
> +					  iommu_group_do_dma_first_attach);
>   }
>   
>   static int iommu_group_do_probe_finalize(struct device *dev, void *data)
> @@ -1839,7 +1842,7 @@ int bus_iommu_probe(struct bus_type *bus)
>   
>   		iommu_group_create_direct_mappings(group);
>   
> -		ret = __iommu_group_dma_attach(group);
> +		ret = __iommu_group_dma_first_attach(group);
>   
>   		mutex_unlock(&group->mutex);
>   
> @@ -1971,9 +1974,11 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   		return -ENODEV;
>   
>   	ret = domain->ops->attach_dev(domain, dev);
> -	if (!ret)
> -		trace_attach_device_to_domain(dev);
> -	return ret;
> +	if (ret)
> +		return ret;
> +	dev->iommu->attach_deferred = 0;
> +	trace_attach_device_to_domain(dev);
> +	return 0;
>   }
>   
>   /**
> @@ -2018,7 +2023,7 @@ EXPORT_SYMBOL_GPL(iommu_attach_device);
>   
>   int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>   {
> -	if (iommu_is_attach_deferred(dev))
> +	if (dev->iommu && dev->iommu->attach_deferred)
>   		return __iommu_attach_device(domain, dev);
>   
>   	return 0;
> @@ -2027,9 +2032,6 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
>   static void __iommu_detach_device(struct iommu_domain *domain,
>   				  struct device *dev)
>   {
> -	if (iommu_is_attach_deferred(dev))
> -		return;
> -
>   	domain->ops->detach_dev(domain, dev);
>   	trace_detach_device_from_domain(dev);
>   }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 1690c334e51631..ebac04a13fff68 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -413,6 +413,7 @@ struct dev_iommu {
>   	struct iommu_device		*iommu_dev;
>   	void				*priv;
>   	u32				max_pasids;
> +	u8				attach_deferred;
>   };
>   
>   int iommu_device_register(struct iommu_device *iommu,

Thanks for the patch! It seems that we also need to call
iommu_group_do_dma_first_attach() in the iommu_probe_device() path?

@@ -401,7 +425,7 @@ int iommu_probe_device(struct device *dev)
          * attach the default domain.
          */
         if (group->default_domain && !group->owner) {
-               ret = __iommu_attach_device(group->default_domain, dev);
+               ret = iommu_group_do_dma_first_attach(dev, 
group->default_domain);
                 if (ret) {
                         mutex_unlock(&group->mutex);
                         iommu_group_put(group);

By the way, I'd like to put above code in a separated patch of the next
version, can I add your signed-off-by?

--
Best regards,
baolu
Jason Gunthorpe Jan. 3, 2023, 2:25 p.m. UTC | #3 
On Tue, Jan 03, 2023 at 10:45:24AM +0800, Baolu Lu wrote:

> Thanks for the patch! It seems that we also need to call
> iommu_group_do_dma_first_attach() in the iommu_probe_device() path?

Yes

> By the way, I'd like to put above code in a separated patch of the next
> version, can I add your signed-off-by?

Yes

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7c99d8eb3182..e4966f088184 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2040,16 +2040,6 @@  int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 	return 0;
 }
 
-static void __iommu_detach_device(struct iommu_domain *domain,
-				  struct device *dev)
-{
-	if (iommu_is_attach_deferred(dev))
-		return;
-
-	domain->ops->detach_dev(domain, dev);
-	trace_detach_device_from_domain(dev);
-}
-
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 {
 	struct iommu_group *group;
@@ -2154,11 +2144,12 @@  int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_group);
 
-static int iommu_group_do_detach_device(struct device *dev, void *data)
+static int iommu_group_do_set_platform_dma(struct device *dev, void *data)
 {
-	struct iommu_domain *domain = data;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 
-	__iommu_detach_device(domain, dev);
+	if (!WARN_ON(!ops->set_platform_dma))
+		ops->set_platform_dma(dev);
 
 	return 0;
 }
@@ -2172,15 +2163,12 @@  static int __iommu_group_set_domain(struct iommu_group *group,
 		return 0;
 
 	/*
-	 * New drivers should support default domains and so the detach_dev() op
-	 * will never be called. Otherwise the NULL domain represents some
-	 * platform specific behavior.
+	 * New drivers should support default domains. Otherwise the NULL
+	 * domain represents returning control back to the platform DMA.
 	 */
 	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);
+		__iommu_group_for_each_dev(group, NULL,
+					   iommu_group_do_set_platform_dma);
 		group->domain = NULL;
 		return 0;
 	}