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

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Mar 30, 2023 at 02:42:20PM +0800, Baolu Lu wrote:

> > @@ -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?

The overall invariant is that all devices are attached to
group->domain, whatever it is.

Devices are not attached to the default_domain, the default_domain is
made the current group->domain which changes all the devices.

Eventually the caller is made to look like:

        if (group->domain) {
                ret = iommu_group_do_dma_first_attach(group, dev);
                if (ret)
                        goto err_unlock;
                iommu_create_device_direct_mappings(group->domain, dev);
        } else if (!group->default_domain) {
                ret = iommu_setup_default_domain(group, 0);
                if (ret)
                        goto err_unlock;
        }

Which makes it clear we can't assume that default_domain == domain at
this point.

> > @@ -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.

Correct, this is wonky - I fix this in the followup series.

Jason