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