Re: [PATCH 5/5] iommu: Retire group->domain

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 11, 2025 at 10:05:40PM -0800, Yi Liu wrote:
>  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);
> +				struct iommu_group *group,
> +				struct iommu_attach_handle *handle);

I think I would split this particular transformation and it's related
into its own patch, it would shrink this alot. It is principally about
sharing the handle and non-handle paths..

> @@ -563,11 +591,12 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  	 * iommu_setup_default_domain()
>  	 */
>  	list_add_tail(&gdev->list, &group->devices);
> -	WARN_ON(group->default_domain && !group->domain);
> +	gdomain = iommu_group_domain(group);
> +	WARN_ON(group->default_domain && !gdomain);
>  	if (group->default_domain)
>  		iommu_create_device_direct_mappings(group->default_domain, dev);
> -	if (group->domain) {
> -		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
> +	if (gdomain) {
> +		ret = __iommu_device_set_domain(group, dev, gdomain, 0);
>  		if (ret)
>  			goto err_remove_gdev;
>  	} else if (!group->default_domain && !group_list) {
> @@ -625,6 +654,8 @@ static void __iommu_group_free_device(struct iommu_group *group,
>  {
>  	struct device *dev = grp_dev->dev;
>  
> +	lockdep_assert_held(&group->mutex);
> +
>  	sysfs_remove_link(group->devices_kobj, grp_dev->name);
>  	sysfs_remove_link(&dev->kobj, "iommu_group");
>  
> @@ -637,7 +668,7 @@ static void __iommu_group_free_device(struct iommu_group *group,
>  	 */
>  	if (list_empty(&group->devices))
>  		WARN_ON(group->owner_cnt ||
> -			group->domain != group->default_domain);
> +			iommu_group_domain(group) != group->default_domain);

You could also probably put the conversion of locked group->domain
into iommu_group_domain() into its own patch (though not using the
xarray to start), that conversion looks like 9 hunks.

> +static void *iommu_make_pasid_entry(struct iommu_domain *domain,
> +				    struct iommu_attach_handle *handle);

Should move the helper to the top of the file in the patch that
introduces it

>  static int __iommu_group_set_domain_internal(struct iommu_group *group,
>  					     struct iommu_domain *new_domain,
> +					     struct iommu_attach_handle *handle,
>  					     unsigned int flags)
>  {
>  	struct group_device *last_gdev;
> +	struct iommu_domain *gdomain;
>  	struct group_device *gdev;
> +	void *pasid_entry;
>  	int result;
>  	int ret;
>  
>  	lockdep_assert_held(&group->mutex);
>  
> -	if (group->domain == new_domain)
> +	gdomain = iommu_group_domain(group);
> +	if (gdomain == new_domain)
>  		return 0;

This is not quite right anymore, we could replace a domain with a
handle or viceversa.

Probably like this:

	if (WARN_ON(!new_domain))
		return -EINVAL;

	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
	curr = xa_cmpxchg(&group->pasid_array, IOMMU_NO_PASID, NULL,
			  XA_ZERO_ENTRY, GFP_KERNEL);
	if (xa_is_err(curr))
		return xa_err(curr);
	if (curr == pasid_entry)
		return 0;
	gdomain = decode_pasid_entry(curr);

> @@ -2290,7 +2343,10 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>  			goto err_revert;
>  		}
>  	}
> -	group->domain = new_domain;
> +
> +	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
> +	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
> +				   IOMMU_NO_PASID, pasid_entry, GFP_KERNEL)));

Ah, this flow uses the same as I suggested a few messages ago already!

> @@ -2302,16 +2358,17 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>  	for_each_group_device(group, gdev) {
>  		/*
>  		 * A NULL domain can happen only for first probe, in which case
> -		 * we leave group->domain as NULL and let release clean
> +		 * we leave domain in pasid_array as NULL and let release clean
>  		 * everything up.
>  		 */
> -		if (group->domain)
> +		if (gdomain)
>  			WARN_ON(__iommu_device_set_domain(
> -				group, gdev->dev, group->domain,
> +				group, gdev->dev, gdomain,
>  				IOMMU_SET_DOMAIN_MUST_SUCCEED));
>  		if (gdev == last_gdev)
>  			break;
>  	}
> +	xa_release(&group->pasid_array, IOMMU_NO_PASID);

This does not seem right, the prior flow left group->domain unchanged
here. IMHO just drop it, the xarray is always populated and almost
never has NULL.

Jason