Received-SPF: pass (google.com: domain of linux-kernel+bounces-73916-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20231027000525.1278806-1-tina.zhang@intel.com>
 <20231027000525.1278806-6-tina.zhang@intel.com> <CABQgh9GWcqUeBkHQCpj5tzu6FnEgpOp3KOQ6s9c0X0KU7Ov1qw@mail.gmail.com>
 <MW5PR11MB5881C5CC15452017867B412889502@MW5PR11MB5881.namprd11.prod.outlook.com>
 <CABQgh9GxQmGU2HR73bSoZLuf4XZFhThXwUs_HJx6KwxDmXrXgg@mail.gmail.com>
In-Reply-To: <CABQgh9GxQmGU2HR73bSoZLuf4XZFhThXwUs_HJx6KwxDmXrXgg@mail.gmail.com>
From: Zhangfei Gao <zhangfei.gao@linaro.org>
Date: Wed, 21 Feb 2024 09:53:34 +0800
Message-ID: <CABQgh9HTH9Utx1CGfni42VykJAFDoYChVbucSA0tmCpeV6Wpxg@mail.gmail.com>
Subject: Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains
To: "Zhang, Tina" <tina.zhang@intel.com>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, David Woodhouse <dwmw2@infradead.org>, 
	Lu Baolu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, 
	Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, Jason Gunthorpe <jgg@ziepe.ca>, 
	"Tian, Kevin" <kevin.tian@intel.com>, Nicolin Chen <nicolinc@nvidia.com>, 
	Michael Shavit <mshavit@google.com>, Vasant Hegde <vasant.hegde@amd.com>, 
	Jason Gunthorpe <jgg@nvidia.com>
Content-Type: text/plain; charset="UTF-8"

On Wed, 21 Feb 2024 at 09:28, Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
> Hi, Tina
>
> On Wed, 21 Feb 2024 at 07:58, Zhang, Tina <tina.zhang@intel.com> wrote:
>
> > > >  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?
>
> Yes
>
> The device can bind to the same mm multi-times and return different handle,
> Since the refcount, no need to unbind and bind sequently,
> The unbind can happen later with the handle.


With this diff can solve the issue, what's your suggestion?

@@ -88,10 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct
device *dev, struct mm_struct *mm
        /* 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);
-               if (!ret) {
+               if (!ret || ret == -EBUSY) {
                        domain->users++;
                        goto out;
                }
@@ -141,8 +151,8 @@ 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, iommu_mm->pasid);
        if (--domain->users == 0) {
+               iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
                list_del(&domain->next);
                iommu_domain_free(domain);
        }

Thanks