Re: [PATCH v2 07/14] iommu: Make iommu_group_do_dma_first_attach() simpler

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

On 3/30/23 7:40 AM, Jason Gunthorpe wrote:
> It should always attach to the current group->domain, so don't take in a
> domain parameter. Use the __iommu_device_set_domain() common code to
> handle the attach.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 22 +++++++++-------------
>   1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index af3af752c255e4..c68cf551d05dc4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -105,6 +105,10 @@ enum {
>   	IOMMU_SET_DOMAIN_WITH_DEFERRED = 1 << 1,
>   };
>   
> +static int __iommu_device_set_domain(struct iommu_group *group,
> +				     struct device *dev,
> +				     struct iommu_domain *new_domain,
> +				     unsigned int flags);
>   static int __iommu_group_set_domain_internal(struct iommu_group *group,
>   					     struct iommu_domain *new_domain,
>   					     unsigned int flags);
> @@ -405,18 +409,10 @@ static bool iommu_is_attach_deferred(struct device *dev)
>   	return false;
>   }
>   
> -static int iommu_group_do_dma_first_attach(struct device *dev, void *data)
> +static int iommu_group_do_dma_first_attach(struct iommu_group *group, struct device *dev)
>   {
> -	struct iommu_domain *domain = data;
> -
> -	lockdep_assert_held(&dev->iommu_group->mutex);
> -
> -	if (iommu_is_attach_deferred(dev)) {
> -		dev->iommu->attach_deferred = 1;
> -		return 0;
> -	}
> -
> -	return __iommu_attach_device(domain, dev);
> +	return __iommu_device_set_domain(group, dev, group->domain,
> +					 IOMMU_SET_DOMAIN_WITH_DEFERRED);

Just out of curiosity, why is this group->domain instead of
group->default_domain?

>   }
>   
>   int iommu_probe_device(struct device *dev)
> @@ -449,7 +445,7 @@ int iommu_probe_device(struct device *dev)
>   	 * attach the default domain.
>   	 */
>   	if (group->default_domain && !group->owner) {
> -		ret = iommu_group_do_dma_first_attach(dev, group->default_domain);
> +		ret = iommu_group_do_dma_first_attach(group, dev);
>   		if (ret) {
>   			mutex_unlock(&group->mutex);
>   			iommu_group_put(group);
> @@ -1117,7 +1113,7 @@ 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)
> -		ret = iommu_group_do_dma_first_attach(dev, group->domain);
> +		ret = iommu_group_do_dma_first_attach(group, dev);

I am wondering whether we need to do dma first attach here.

iommu_group_add_device() is called from

1) drivers/vfio/group.c
    The deferred attachment is only used for kdump. In the kdump capture
    kernel, vfio should not be used. Or not?

2) arch/powerpc/kernel/iommu.c
    Is powerpc iommu implementation compatible with the generic IOMMU
    framework and provide a is_attach_deferred op? If not, no need for
    dma first attach either.

If above are true, perhaps we could simply call
__iommu_group_set_domain() here?

>   	mutex_unlock(&group->mutex);
>   	if (ret)
>   		goto err_put_group;

Best regards,
baolu