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

[Jason Gunthorpe via iommu <iommu@lists.linux-foundation.org>]

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.

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