RE: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains

[Zhang, Tina]

Hi Zhangfei,

> -----Original Message-----
> From: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
> Sent: Wednesday, February 21, 2024 12:27 AM
> To: Zhang, Tina <tina.zhang@xxxxxxxxx>
> Cc: iommu@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; David Woodhouse
> <dwmw2@xxxxxxxxxxxxx>; Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>; Joerg
> Roedel <joro@xxxxxxxxxx>; Will Deacon <will@xxxxxxxxxx>; Robin Murphy
> <robin.murphy@xxxxxxx>; Jason Gunthorpe <jgg@xxxxxxxx>; Tian, Kevin
> <kevin.tian@xxxxxxxxx>; Nicolin Chen <nicolinc@xxxxxxxxxx>; Michael Shavit
> <mshavit@xxxxxxxxxx>; Vasant Hegde <vasant.hegde@xxxxxxx>; Jason
> Gunthorpe <jgg@xxxxxxxxxx>
> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Vasant Hegde <vasant.hegde@xxxxxxx>
> > Reviewed-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Tested-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Signed-off-by: Tina Zhang <tina.zhang@xxxxxxxxx>
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > ---
> >  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
Sounds like the test case tries to bind a device to a same mm multiple times without unbinding the device and the expectation is that it can always return a valid handle to pass the test. Right?

Regards,
-Tina
> 
> > +               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