Re: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

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

On 2022-05-05 16:33, Jason Gunthorpe wrote:
> On Thu, May 05, 2022 at 10:56:28AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Thursday, May 5, 2022 3:09 AM
>>>
>>> Once the group enters 'owned' mode it can never be assigned back to the
>>> default_domain or to a NULL domain. It must always be actively assigned to
>>
>> worth pointing out that a NULL domain is not always translated to DMA
>> blocked on all platforms. That was a wrong assumption before this patch.
> 
> This is described in a comment blow
> 
>>> @@ -2040,7 +2037,8 @@ static int __iommu_attach_group(struct
>>> iommu_domain *domain,
>>>   {
>>>   	int ret;
>>>
>>> -	if (group->domain && group->domain != group->default_domain)
>>> +	if (group->domain && group->domain != group->default_domain &&
>>> +	    group->domain != group->blocking_domain)
>>>   		return -EBUSY;
>>>
>>>   	ret = __iommu_group_for_each_dev(group, domain,
>>
>> Suppose this can be also replaced by __iommu_group_attach_domain()?
> 
> It can, but lets do this as a follow patching since it is a bit big
> 
>>> @@ -2072,38 +2070,68 @@ static int
>>> iommu_group_do_detach_device(struct device *dev, void *data)
>>>   	return 0;
>>>   }
>>>
>>> -static void __iommu_detach_group(struct iommu_domain *domain,
>>> -				 struct iommu_group *group)
>>> +static int __iommu_group_attach_domain(struct iommu_group *group,
>>> +				       struct iommu_domain *new_domain)
>>>   {
>>>   	int ret;
>>>
>>> +	if (group->domain == new_domain)
>>> +		return 0;
>>> +
>>>   	/*
>>> -	 * If the group has been claimed already, do not re-attach the default
>>> -	 * domain.
>>> +	 * New drivers should support default domains and so the
>>> detach_dev() op
>>> +	 * will never be called. Otherwise the NULL domain indicates the
>>> +	 * translation for the group should be set so it will work with the
>>
>> translation should be 'blocked'?
> 
> No, not blocked.
> 
>>> +	 * platform DMA ops.
>>
>> I didn't get the meaning of the last sentence.
> 
> It is as discussed with Robin, NULL means to return the group back to
> some platform defined translation, and in some cases this means
> returning back to work with the platform's DMA ops - presumably if it
> isn't using the dma api.
> 
>>> +	/*
>>> +	 * Changing the domain is done by calling attach on the new domain.
>>> +	 * Drivers should implement this so that DMA is always translated by
>>
>> what does 'this' mean?
> 
> The code below - attach_dev called in a loop for each dev in the group.
> 
>>> +	 * either the new, old, or a blocking domain. DMA should never
>>
>> isn't the blocking domain passed in as the new domain?
> 
> Soemtimes. This is a statement about the required
> atomicity. New/old/block are all valid translations during the
> operation. Identity is not.
>   
>>> +	 * untranslated.
>>> +	 *
>>> +	 * Note that this is called in error unwind paths, attaching to a
>>> +	 * domain that has already been attached cannot fail.
>>
>> this is called in the normal path. Where does error unwind happen?
> 
> Any place that checks the return and does WARN_ON
> 
> Also the loop over each dev doesn't error unwind so it assumes that if
> the first dev succeeds then all subsequent devs will succeed.
>>>   /**
>>>    * iommu_group_claim_dma_owner() - Set DMA ownership of a group
>>>    * @group: The group.
>>> @@ -3111,9 +3162,15 @@ int iommu_group_claim_dma_owner(struct
>>> iommu_group *group, void *owner)
>>>   			goto unlock_out;
>>>   		}
>>>
>>> +		ret = __iommu_group_alloc_blocking_domain(group);
>>> +		if (ret)
>>> +			goto unlock_out;
>>> +
>>> +		ret = __iommu_group_attach_domain(group,
>>> +						  group->blocking_domain);
>>> +		if (ret)
>>> +			goto unlock_out;
>>>   		group->owner = owner;
>>
>> Here can use __iommu_group_attach_core_domain() if calling it after
>> setting group->owner.
> 
> That is obfuscating. This function must always and only attach the
> blocking_domain.
> 
> So, I'm going to leave this patch as-is since it has been tested now
> and we need to get the series back into iommu-next ASAP.

Ack to that, there are certainly further improvements to consider once 
we've got known-working code into a released kernel, but let's not get 
ahead of ourselves just now.

(I'm pretty sure we could get away with a single blocking domain per 
IOMMU instance, rather than per group, but I deliberately saved that one 
for later - right now one per group to match default domains is simpler 
to reason about and less churny in the context of the current ownership 
patches)

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu