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

[Baolu Lu]

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@xxxxxxxxxxxxxxx>
> > ---
> >   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