Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH 2/4] iommu: Use group ownership to avoid driver attachment | Date | Wed, 15 Feb 2023 06:56:39 +0000 |
| |
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, February 15, 2023 1:51 PM > > On 2/13/23 10:19 PM, Jason Gunthorpe wrote: > > On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote: > >> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct > iommu_group *group, > >> else > >> return -EINVAL; > >> > >> + if (req_type != IOMMU_DOMAIN_DMA_FQ || > >> + group->default_domain->type != IOMMU_DOMAIN_DMA) { > >> + ret = iommu_group_claim_dma_owner(group, (void *)buf); > >> + if (ret) > >> + return ret; > >> + group_owner_claimed = true; > >> + } > > > > I don't get it, this should be done unconditionally. If we couldn't > > take ownership then we simply can't progress. > > The existing code allows the user to switch the default domain from > strict to lazy invalidation mode. The default domain is not changed, > hence it should be seamless and transparent to the device driver.
Is there real usage relying on this transition for a bound device?
In concept strict->lazy transition implies relaxed DMA security. It's hard to think of a motivation of doing so while the device might be doing in-fly DMAs.
Presumably such perf/security tradeoff should be planned way before binding device/driver together.
btw if strict->lazy is allowed why lazy->strict is prohibited?
> > > which also means this needs to be > > an externally version of iommu_group_claim_dma_owner() > > Sorry! What does "an externally version of > iommu_group_claim_dma_owner()" mean? > > My understanding is that we should limit iommu_group_claim_dma_owner() > use in the driver context. For this non-driver context, we should not > use iommu_group_claim_dma_owner() directly, but hold the group->mutex > and check the group->owner_cnt directly: > > mutex_lock(&group->mutex); > if (group->owner_cnt) { > ret = -EPERM; > goto unlock_out; > } > > the group->mutex should be held until everything is done. >
I guess you two meant the same thing.
mutex_lock(&group->mutex); iommu_group_claim_dma_owner_unlocked(); //blah blah mutex_unlock(&group->mutex);
| |