[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BN9PR11MB527658A85092F88329EB73E98CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Wed, 5 Jun 2024 08:15:53 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>, Jason Gunthorpe <jgg@...pe.ca>, Joerg
Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, Robin Murphy
<robin.murphy@....com>, Jean-Philippe Brucker <jean-philippe@...aro.org>,
Nicolin Chen <nicolinc@...dia.com>, "Liu, Yi L" <yi.l.liu@...el.com>, "Jacob
Pan" <jacob.jun.pan@...ux.intel.com>, Joel Granados <j.granados@...sung.com>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
"virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 02/10] iommu: Remove sva handle list
> From: Lu Baolu <baolu.lu@...ux.intel.com>
> Sent: Monday, May 27, 2024 12:05 PM
>
> @@ -69,11 +68,16 @@ static struct iommu_mm_data
> *iommu_alloc_mm_data(struct mm_struct *mm, struct de
> */
> struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct
> mm_struct *mm)
> {
> + struct iommu_group *group = dev->iommu_group;
> + struct iommu_attach_handle *attach_handle;
> struct iommu_mm_data *iommu_mm;
> struct iommu_domain *domain;
> struct iommu_sva *handle;
it's confusing to have both 'handle' and 'attach_handle' in one function.
Clearer to rename 'handle' as 'sva'.
> int ret;
>
> + if (!group)
> + return ERR_PTR(-ENODEV);
> +
> mutex_lock(&iommu_sva_lock);
>
> /* Allocate mm->pasid if necessary. */
> @@ -83,12 +87,13 @@ struct iommu_sva *iommu_sva_bind_device(struct
> device *dev, struct mm_struct *mm
> goto out_unlock;
> }
>
> - list_for_each_entry(handle, &mm->iommu_mm->sva_handles,
> handle_item) {
> - if (handle->dev == dev) {
> - refcount_inc(&handle->users);
> - mutex_unlock(&iommu_sva_lock);
> - return handle;
> - }
> + /* A bond already exists, just take a reference`. */
> + attach_handle = iommu_attach_handle_get(group, iommu_mm-
> >pasid, IOMMU_DOMAIN_SVA);
> + if (!IS_ERR(attach_handle)) {
> + handle = container_of(attach_handle, struct iommu_sva,
> handle);
> + refcount_inc(&handle->users);
> + mutex_unlock(&iommu_sva_lock);
> + return handle;
> }
It's counter-intuitive to move forward when an error is returned.
e.g. if it's -EBUSY indicating the pasid already used for another type then
following attempts shouldn't been tried.
probably we should have iommu_attach_handle_get() return NULL
instead of -ENOENT when the entry is free? then:
attach_handle = iommu_attach_handle_get();
if (IS_ERR(attach_handle)) {
ret = PTR_ERR(attach_handle);
goto out_unlock;
} else if (attach_handle) {
/* matched and increase handle->users */
}
/* free entry falls through */
But then there is one potential issue with the design that 'handle'
can be optional in iommu_attach_device_pasid(). In that case
xa_load returns NULL then we cannot differentiate a real unused
PASID vs. one which has been attached w/o an handle.
Does it suggest that having the caller to always provide a handle
makes more sense?
Powered by blists - more mailing lists