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

Baolu Lu
2023-01-03T02:53:26Z

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