Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Robin Murphy <robin.murphy@arm.com>]

On 2022-05-05 17:15, Jason Gunthorpe via iommu wrote:
> Call iommu_group_do_attach_device() only from
> __iommu_group_attach_domain() which should be used to attach any domain to
> the group.
> 
> The only unique thing __iommu_attach_group() does is to check if the group
> is already attached to some caller specified group. Put this test into
> __iommu_group_is_core_domain(), matching the
> __iommu_group_attach_core_domain() nomenclature.
> 
> Change the two callers to directly call __iommu_group_attach_domain() and
> test __iommu_group_is_core_domain().
> 
> iommu_attach_device() should trigger a WARN_ON if the group is attached as
> the caller is using the function wrong.

Nit: if that's true then it's equally true for iommu_attach_group() as well.

> Suggested-by: "Tian, Kevin" <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 42 +++++++++++++++++++-----------------------
>   1 file changed, 19 insertions(+), 23 deletions(-)
> 
> This goes after "iommu: iommu_group_claim_dma_owner() must always assign a
> domain" and simplifies some of the remaining duplication.
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c1bdec807ea381..09d00be887f924 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -81,9 +81,10 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>   						 unsigned type);
>   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 int __iommu_group_attach_domain(struct iommu_group *group,
> +				       struct iommu_domain *new_domain);
>   static void __iommu_group_attach_core_domain(struct iommu_group *group);
> +static bool __iommu_group_is_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);
> @@ -1938,10 +1939,11 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>   	 */
>   	mutex_lock(&group->mutex);
>   	ret = -EINVAL;
> -	if (iommu_group_device_count(group) != 1)
> +	if (iommu_group_device_count(group) != 1 ||
> +	    WARN_ON(!__iommu_group_is_core_domain(group)))
>   		goto out_unlock;
>   
> -	ret = __iommu_attach_group(domain, group);
> +	ret = __iommu_group_attach_domain(group, domain);
>   
>   out_unlock:
>   	mutex_unlock(&group->mutex);
> @@ -2032,31 +2034,19 @@ static int iommu_group_do_attach_device(struct device *dev, void *data)
>   	return __iommu_attach_device(domain, dev);
>   }
>   
> -static int __iommu_attach_group(struct iommu_domain *domain,
> -				struct iommu_group *group)
> -{
> -	int ret;
> -
> -	if (group->domain && group->domain != group->default_domain &&
> -	    group->domain != group->blocking_domain)
> -		return -EBUSY;
> -
> -	ret = __iommu_group_for_each_dev(group, domain,
> -					 iommu_group_do_attach_device);
> -	if (ret == 0)
> -		group->domain = domain;
> -
> -	return ret;
> -}
> -
>   int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
>   {
>   	int ret;
>   
>   	mutex_lock(&group->mutex);
> -	ret = __iommu_attach_group(domain, group);
> -	mutex_unlock(&group->mutex);
> +	if (!__iommu_group_is_core_domain(group)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
>   
> +	ret = __iommu_group_attach_domain(group, domain);
> +out_unlock:
> +	mutex_unlock(&group->mutex);
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(iommu_attach_group);
> @@ -2110,6 +2100,12 @@ static int __iommu_group_attach_domain(struct iommu_group *group,
>   	return 0;
>   }
>   
> +static bool __iommu_group_is_core_domain(struct iommu_group *group)

I can see the thought process behind it, but once we've had some time 
away from actively working on this area, this is clearly going to be a 
terrible name. "Is this group a core domain? Er, no, it's a group; what 
an odd question to ask :/" Even getting past that, does it make sense to 
say NULL is a core domain? I'm not convinced. For the sake of future 
readability, I'd prefer to call this something more purpose-related like 
__iommu_group_can_attach() (and also just define it earlier to avoid the 
forward-declaration).

In fact at that point I'd also be tempted to rename 
__iommu_group_attach_domain() to __iommu_group_set_domain(), to further 
clarify that attach/detach still reflects the external API, but the 
internal mechanism is really a bit different since default/core domains 
came in.

Thanks,
Robin.

> +{
> +	return !group->domain || group->domain == group->default_domain ||
> +	       group->domain == group->blocking_domain;
> +}
> +
>   /*
>    * 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.
> 
> base-commit: f9169ee95787fe691287eeed1a1c269ea72c8fb4
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu