Received-SPF: pass (google.com: domain of linux-kernel+bounces-73354-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>
In-Reply-To: <20231027000525.1278806-6-tina.zhang@intel.com>
From: Zhangfei Gao <zhangfei.gao@linaro.org>
Date: Wed, 21 Feb 2024 00:26:56 +0800
Message-ID: <CABQgh9GWcqUeBkHQCpj5tzu6FnEgpOp3KOQ6s9c0X0KU7Ov1qw@mail.gmail.com>
Subject: Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains
To: Tina Zhang <tina.zhang@intel.com>
Cc: iommu@lists.linux.dev, 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>, Kevin Tian <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"

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