Messages in this thread | | | Date | Fri, 22 Sep 2023 09:07:28 +0800 | Subject | Re: [PATCH v4 5/6] iommu: Support mm PASID 1:n with sva domains | From | Tina Zhang <> |
| |
Hi Jason,
On 9/22/23 07:31, Jason Gunthorpe wrote: > On Tue, Sep 12, 2023 at 08:59:35PM +0800, Tina Zhang wrote: >> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c >> index 0f956ecd0c9b..79779bbef653 100644 >> --- a/drivers/iommu/iommu-sva.c >> +++ b/drivers/iommu/iommu-sva.c >> @@ -15,6 +15,7 @@ static DEFINE_MUTEX(iommu_sva_lock); >> static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev) >> { >> ioasid_t pasid; >> + struct iommu_mm_data *iommu_mm; >> int ret = 0; >> >> if (!arch_pgtable_dma_compat(mm)) >> @@ -28,12 +29,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev) >> goto out; >> } >> >> + iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL); >> + if (!iommu_mm) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> pasid = iommu_alloc_global_pasid(dev); >> if (pasid == IOMMU_PASID_INVALID) { >> + kfree(iommu_mm); >> ret = -ENOSPC; >> goto out; >> } >> - mm->pasid = pasid; >> + iommu_mm->pasid = pasid; >> + INIT_LIST_HEAD(&iommu_mm->sva_domains); >> + mm->iommu_mm = iommu_mm; >> + >> ret = 0; >> out: >> mutex_unlock(&iommu_sva_lock); >> @@ -73,16 +84,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm >> >> mutex_lock(&iommu_sva_lock); >> /* Search for an existing domain. */ >> - domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm), >> - IOMMU_DOMAIN_SVA); >> - if (IS_ERR(domain)) { >> - ret = PTR_ERR(domain); >> - goto out_unlock; >> - } >> - >> - if (domain) { >> - domain->users++; >> - goto out; >> + list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) { >> + ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm)); >> + if (!ret) { >> + domain->users++; >> + goto out; >> + } >> } >> >> /* Allocate a new domain and set it on device pasid. */ >> @@ -96,6 +103,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm >> if (ret) >> goto out_free_domain; >> domain->users = 1; >> + list_add(&domain->next, &mm->iommu_mm->sva_domains); >> + >> out: >> mutex_unlock(&iommu_sva_lock); >> handle->dev = dev; >> @@ -128,8 +137,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle) >> struct device *dev = handle->dev; >> >> mutex_lock(&iommu_sva_lock); >> + iommu_detach_device_pasid(domain, dev, pasid); >> if (--domain->users == 0) { >> - iommu_detach_device_pasid(domain, dev, pasid); >> + list_del(&domain->next); >> iommu_domain_free(domain); >> } >> mutex_unlock(&iommu_sva_lock); >> @@ -209,4 +219,5 @@ void mm_pasid_drop(struct mm_struct *mm) >> return; >> >> iommu_free_global_pasid(mm_get_pasid(mm)); >> + kfree(mm->iommu_mm); >> } >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index b9c9f14a95cc..c61bc45d5a82 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -109,6 +109,11 @@ struct iommu_domain { >> struct { /* IOMMU_DOMAIN_SVA */ >> struct mm_struct *mm; >> int users; >> + /* >> + * Next iommu_domain in mm->iommu_mm->sva-domains list >> + * protected by iommu_sva_lock. >> + */ >> + struct list_head next; >> }; >> }; >> }; >> @@ -1186,17 +1191,13 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream >> } >> >> #ifdef CONFIG_IOMMU_SVA >> -static inline void mm_pasid_init(struct mm_struct *mm) >> -{ >> - mm->pasid = IOMMU_PASID_INVALID; >> -} >> static inline bool mm_valid_pasid(struct mm_struct *mm) >> { >> - return mm->pasid != IOMMU_PASID_INVALID; >> + return mm->iommu_mm ? true : false; >> } > > HUm this isn't locked very nicely. > > Above do > > smp_store_release(&mm->iommu_mm, iommu_mm); > > And then do > > return READ_ONCE(mm->iommu_mm) Good suggestion. Thanks.
> > (no need for ternaries with bools, compiler generates it automatically) > > >> static inline u32 mm_get_pasid(struct mm_struct *mm) >> { >> - return mm->pasid; >> + return mm->iommu_mm ? mm->iommu_mm->pasid : IOMMU_PASID_INVALID; >> } > > Then this should be > > struct iommu_mm_data *iommu_mm = READ_ONCE(mm->iommu_mm); > > if (!iommu_mm) > return IOMMU_PASID_INVALID; > return iommu_mm->pasid; > > Keeping in mind that the kfree(mm->iommu_mm) being placed in the > mm_drop is critical to this working safely. Right. Thanks.
Regards, -Tina > > Otherwise the logic looks OK. > > Jason >
| |