Re: [PATCH v1 3/8] iommu: Extend iommu_at[de]tach_device() for multi-device groups

[Joerg Roedel <joro@8bytes.org>]

On Thu, Jan 06, 2022 at 10:20:48AM +0800, Lu Baolu wrote:
>  int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
>  {
>  	struct iommu_group *group;
> -	int ret;
> +	int ret = 0;
> +
> +	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
> +		return -EINVAL;
>  
>  	group = iommu_group_get(dev);
>  	if (!group)
>  		return -ENODEV;
>  
> -	/*
> -	 * Lock the group to make sure the device-count doesn't
> -	 * change while we are attaching
> -	 */
>  	mutex_lock(&group->mutex);
> -	ret = -EINVAL;
> -	if (iommu_group_device_count(group) != 1)
> -		goto out_unlock;
> +	if (group->owner_cnt) {
> +		/*
> +		 * Group has been used for kernel-api dma or claimed explicitly
> +		 * for exclusive occupation. For backward compatibility, device
> +		 * in a singleton group is allowed to ignore setting the
> +		 * drv.no_kernel_api_dma field.
> +		 */
> +		if ((group->domain == group->default_domain &&
> +		     iommu_group_device_count(group) != 1) ||
> +		    group->owner) {
> +			ret = -EBUSY;
> +			goto unlock_out;
> +		}
> +	}
>  
> -	ret = __iommu_attach_group(domain, group);
> +	if (!group->attach_cnt) {
> +		ret = __iommu_attach_group(domain, group);
> +		if (ret)
> +			goto unlock_out;
> +	} else {
> +		if (group->domain != domain) {
> +			ret = -EPERM;
> +			goto unlock_out;
> +		}
> +	}
>  
> -out_unlock:
> +	group->owner_cnt++;
> +	group->attach_cnt++;
> +
> +unlock_out:
>  	mutex_unlock(&group->mutex);
>  	iommu_group_put(group);

This extends iommu_attach_device() to behave as iommu_attach_group(),
changing the domain for the whole group. Wouldn't it be better to scrap
the iommu_attach_device() interface instead and only rely on
iommu_attach_group()? This way it is clear that a call changes the whole
group.

IIUC this work is heading towards allowing multiple domains in one group
as long as the group is owned by one entity. That is a valid
requirement, but the way to get there is in my eyes:

	1) Introduce a concept of a sub-group (or whatever we want to
	   call it), which groups devices together which must be in the
	   same domain because they use the same request ID and thus
	   look all the same to the IOMMU.

	2) Keep todays IOMMU groups to group devices together which can
	   bypass the IOMMU when talking to each other, like
	   multi-function devices and devices behind a no-ACS bridge.

	3) Rework group->domain and group->default_domain, eventually
	   moving them to sub-groups.

This is an important distinction to make and also the reason the
iommu_attach/detach_device() interface will always be misleading. Item
1) in this list will also be beneficial to other parts of the iommu
code, namely iommu-dma code which can have finer-grained DMA-API domains
with sub-groups instead of groups.

Regards,

	Joerg