Received-SPF: pass (google.com: domain of linux-kernel+bounces-74276-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
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>
 <b05958ba-0c47-45ba-8159-372779f9cc8b@linux.intel.com> <CABQgh9FFpL=mEZ-7PqRRVg1eniYV176B7USbGP5MLPvhJaGo9A@mail.gmail.com>
 <MW5PR11MB5881B3EA808820BCBCFAF72589572@MW5PR11MB5881.namprd11.prod.outlook.com>
In-Reply-To: <MW5PR11MB5881B3EA808820BCBCFAF72589572@MW5PR11MB5881.namprd11.prod.outlook.com>
From: Zhangfei Gao <zhangfei.gao@linaro.org>
Date: Wed, 21 Feb 2024 16:01:56 +0800
Message-ID: <CABQgh9HPhp+LKz5pPnpN1bWXoU3C38k6swqhAqJj67xGJQUmwA@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: Baolu Lu <baolu.lu@linux.intel.com>, 
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, David Woodhouse <dwmw2@infradead.org>, 
	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>, Jean-Philippe Brucker <jean-philippe@linaro.org>, 
	Hao Fang <fanghao11@huawei.com>
Content-Type: text/plain; charset="UTF-8"

On Wed, 21 Feb 2024 at 15:41, Zhang, Tina <tina.zhang@intel.com> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: Zhangfei Gao <zhangfei.gao@linaro.org>
> > Sent: Wednesday, February 21, 2024 10:45 AM
> > To: Baolu Lu <baolu.lu@linux.intel.com>
> > Cc: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev; linux-
> > kernel@vger.kernel.org; David Woodhouse <dwmw2@infradead.org>; 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>; Jean-Philippe Brucker <jean-
> > philippe@linaro.org>; Hao Fang <fanghao11@huawei.com>
> > Subject: Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva
> > domains
> >
> > On Wed, 21 Feb 2024 at 10:06, Baolu Lu <baolu.lu@linux.intel.com> wrote:
> > >
> > > On 2024/2/21 9:28, Zhangfei Gao wrote:
> > > > 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.
> > >
> > > Is there any real use case to bind an mm to the pasid of a device
> > > multiple times? If there are cases, is it better to handle this in the
> > > uacce driver?
> >
> > Yes, it is required for multi-thread, the device can provide multi-queue to
> > speed up.
> >
> > >
> > >  From iommu core's perspective, it doesn't make sense to attach the
> > > same domain to the same device (or pasid) multiple times.
> >
> > But is it the refcount domain->user++ used for?
> > Is there any reason not doing this.
> The domain->user is a refcount of the devices (or iommu group) attached to the domain.  IOMMU core needs to keep this refcount to ensure that a sva domain will be released when no device uses it.

I think the limitation of one user only attach one domain one time
does not make sense.
Just like one file can only be opened one time by a user, then
refcount is meanless.

Thanks
>
> Regards,
> -Tina
>
> >
> > Thanks