Messages in this thread | | | From | Zhangfei Gao <> | Date | Wed, 21 Feb 2024 00:26:56 +0800 | Subject | Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains |
| |
Hi, Tina
On Fri, 27 Oct 2023 at 08:06, Tina Zhang <tina.zhang@intel.com> wrote: > > Each mm bound to devices gets a PASID and corresponding sva domains > allocated in iommu_sva_bind_device(), which are referenced by iommu_mm > field of the mm. The PASID is released in __mmdrop(), while a sva domain > is released when no one is using it (the reference count is decremented > in iommu_sva_unbind_device()). However, although sva domains and their > PASID are separate objects such that their own life cycles could be > handled independently, an enqcmd use case may require releasing the > PASID in releasing the mm (i.e., once a PASID is allocated for a mm, it > will be permanently used by the mm and won't be released until the end > of mm) and only allows to drop the PASID after the sva domains are > released. To this end, mmgrab() is called in iommu_sva_domain_alloc() to > increment the mm reference count and mmdrop() is invoked in > iommu_domain_free() to decrement the mm reference count. > > Since the required info of PASID and sva domains is kept in struct > iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid > field in mm struct. The sva domain list is protected by iommu_sva_lock. > > Besides, this patch removes mm_pasid_init(), as with the introduced > iommu_mm structure, initializing mm pasid in mm_init() is unnecessary. > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> > Reviewed-by: Vasant Hegde <vasant.hegde@amd.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Tina Zhang <tina.zhang@intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu-sva.c | 92 +++++++++++++++++++++++---------------- > include/linux/iommu.h | 23 ++++++++-- > 2 files changed, 74 insertions(+), 41 deletions(-) > > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c > index 4a2f5699747f..5175e8d85247 100644 > --- a/drivers/iommu/iommu-sva.c > +++ b/drivers/iommu/iommu-sva.c > @@ -12,32 +12,42 @@ > static DEFINE_MUTEX(iommu_sva_lock); > > /* Allocate a PASID for the mm within range (inclusive) */ > -static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev) > +static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct device *dev) > { > + struct iommu_mm_data *iommu_mm; > ioasid_t pasid; > - int ret = 0; > + > + lockdep_assert_held(&iommu_sva_lock); > > if (!arch_pgtable_dma_compat(mm)) > - return -EBUSY; > + return ERR_PTR(-EBUSY); > > - mutex_lock(&iommu_sva_lock); > + iommu_mm = mm->iommu_mm; > /* Is a PASID already associated with this mm? */ > - if (mm_valid_pasid(mm)) { > - if (mm->pasid >= dev->iommu->max_pasids) > - ret = -EOVERFLOW; > - goto out; > + if (iommu_mm) { > + if (iommu_mm->pasid >= dev->iommu->max_pasids) > + return ERR_PTR(-EOVERFLOW); > + return iommu_mm; > } > > + iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL); > + if (!iommu_mm) > + return ERR_PTR(-ENOMEM); > + > pasid = iommu_alloc_global_pasid(dev); > if (pasid == IOMMU_PASID_INVALID) { > - ret = -ENOSPC; > - goto out; > + kfree(iommu_mm); > + return ERR_PTR(-ENOSPC); > } > - mm->pasid = pasid; > - ret = 0; > -out: > - mutex_unlock(&iommu_sva_lock); > - return ret; > + iommu_mm->pasid = pasid; > + INIT_LIST_HEAD(&iommu_mm->sva_domains); > + /* > + * Make sure the write to mm->iommu_mm is not reordered in front of > + * initialization to iommu_mm fields. If it does, readers may see a > + * valid iommu_mm with uninitialized values. > + */ > + smp_store_release(&mm->iommu_mm, iommu_mm); > + return iommu_mm; > } > > /** > @@ -58,31 +68,33 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev) > */ > struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm) > { > + struct iommu_mm_data *iommu_mm; > struct iommu_domain *domain; > struct iommu_sva *handle; > int ret; > > + mutex_lock(&iommu_sva_lock); > + > /* Allocate mm->pasid if necessary. */ > - ret = iommu_sva_alloc_pasid(mm, dev); > - if (ret) > - return ERR_PTR(ret); > + iommu_mm = iommu_alloc_mm_data(mm, dev); > + if (IS_ERR(iommu_mm)) { > + ret = PTR_ERR(iommu_mm); > + goto out_unlock; > + } > > handle = kzalloc(sizeof(*handle), GFP_KERNEL); > - if (!handle) > - return ERR_PTR(-ENOMEM); > - > - mutex_lock(&iommu_sva_lock); > - /* Search for an existing domain. */ > - domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid, > - IOMMU_DOMAIN_SVA); > - if (IS_ERR(domain)) { > - ret = PTR_ERR(domain); > + if (!handle) { > + ret = -ENOMEM; > goto out_unlock; > } > > - if (domain) { > - domain->users++; > - goto out;
Our multi bind test case broke since 6.8-rc1. The test case can use same domain & pasid, return different handle, 6.7 simply domain->users ++ and return.
> + /* Search for an existing domain. */ > + list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) { > + ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid);
Now iommu_attach_device_pasid return BUSY since the same pasid. And then iommu_sva_bind_device attach ret=-16
> + if (!ret) {
Simply tried if (!ret || ret == -EBUSY) The test passes, but report waring WARNING: CPU: 12 PID: 2992 at drivers/iommu/iommu.c:3591 iommu_detach_device_pasid+0xa4/0xd0
Will check more tomorrow.
> + domain->users++; > + goto out; > + } > } >
Thanks
| |