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 10:41 PM, Jason Gunthorpe wrote:
> 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.

Yeah! That's clearer.

Best regards,
baolu