Messages in this thread | | | Date | Sun, 24 Jul 2022 21:48:15 +0800 | Subject | Re: [PATCH v10 08/12] iommu/sva: Refactoring iommu_sva_bind/unbind_device() | From | Baolu Lu <> |
| |
On 2022/7/23 22:26, Jason Gunthorpe wrote: > On Tue, Jul 05, 2022 at 01:07:06PM +0800, Lu Baolu wrote: >> The existing iommu SVA interfaces are implemented by calling the SVA >> specific iommu ops provided by the IOMMU drivers. There's no need for >> any SVA specific ops in iommu_ops vector anymore as we can achieve >> this through the generic attach/detach_dev_pasid domain ops. >> >> This refactors the IOMMU SVA interfaces implementation by using the >> set/block_pasid_dev ops and align them with the concept of the SVA >> iommu domain. Put the new SVA code in the sva related file in order >> to make it self-contained. >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> Tested-by: Tony Zhu <tony.zhu@intel.com> >> --- >> include/linux/iommu.h | 67 +++++++++++-------- >> drivers/iommu/iommu-sva-lib.c | 98 ++++++++++++++++++++++++++++ >> drivers/iommu/iommu.c | 119 ++++++++-------------------------- >> 3 files changed, 165 insertions(+), 119 deletions(-) >> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >> index 42f0418dc22c..f59b0ecd3995 100644 >> --- a/include/linux/iommu.h >> +++ b/include/linux/iommu.h >> @@ -39,7 +39,6 @@ struct device; >> struct iommu_domain; >> struct iommu_domain_ops; >> struct notifier_block; >> -struct iommu_sva; >> struct iommu_fault_event; >> struct iommu_dma_cookie; >> >> @@ -57,6 +56,14 @@ struct iommu_domain_geometry { >> bool force_aperture; /* DMA only allowed in mappable range? */ >> }; >> >> +/** >> + * struct iommu_sva - handle to a device-mm bond >> + */ >> +struct iommu_sva { >> + struct device *dev; >> + refcount_t users; >> +}; >> + >> /* Domain feature flags */ >> #define __IOMMU_DOMAIN_PAGING (1U << 0) /* Support for iommu_map/unmap */ >> #define __IOMMU_DOMAIN_DMA_API (1U << 1) /* Domain for use in DMA-API >> @@ -105,6 +112,7 @@ struct iommu_domain { >> }; >> struct { /* IOMMU_DOMAIN_SVA */ >> struct mm_struct *mm; >> + struct iommu_sva bond; > > We can't store a single struct device inside a domain, this is not > layed out right.
Yes, agreed.
> > The API is really refcounting the PASID: > >> +struct iommu_sva *iommu_sva_bind_device(struct device *dev, >> + struct mm_struct *mm); >> +void iommu_sva_unbind_device(struct iommu_sva *handle); > > So what you need to do is store that 'iommu_sva' in the group's PASID > xarray. > > The bind logic would be > > sva = xa_load(group->pasid, mm->pasid) > if (sva) > refcount_inc(sva->users) > return sva > sva = kalloc > sva->domain = domain > xa_store(group->pasid, sva);
Thanks for the suggestion. It makes a lot of sense to me.
Furthermore, I'd like to separate the generic data from the caller- specific things because the group->pasid_array should also be able to serve other usages. Hence, the attach/detach_device_pasid interfaces might be changed like below:
/* Collection of per-pasid IOMMU data */ struct group_pasid { struct iommu_domain *domain; void *priv; };
/* * iommu_attach_device_pasid() - Attach a domain to pasid of device * @domain: the iommu domain. * @dev: the attached device. * @pasid: the pasid of the device. * @data: private data, NULL if not needed. * * Return: 0 on success, or an error. */ int iommu_attach_device_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid, void *data) { struct iommu_group *group; struct group_pasid *param; int ret = -EINVAL; void *curr;
if (!domain->ops->set_dev_pasid) return -EOPNOTSUPP;
group = iommu_group_get(dev); if (!group) return -ENODEV;
param = kzalloc(sizeof(*param), GFP_KERNEL); if (!param) { iommu_group_put(group); return -ENOMEM; } param->domain = domain; param->priv = data;
mutex_lock(&group->mutex); if (!iommu_group_immutable_singleton(group, dev)) goto out_unlock;
curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, param, GFP_KERNEL); if (curr) { ret = xa_err(curr) ? : -EBUSY; goto out_unlock; } ret = domain->ops->set_dev_pasid(domain, dev, pasid); if (ret) xa_erase(&group->pasid_array, pasid); out_unlock: mutex_unlock(&group->mutex); iommu_group_put(group); if (ret) kfree(param);
return ret; }
/* * iommu_detach_device_pasid() - Detach the domain from pasid of device * @domain: the iommu domain. * @dev: the attached device. * @pasid: the pasid of the device. * * The @domain must have been attached to @pasid of the @dev with * iommu_detach_device_pasid(). */ void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid) { struct iommu_group *group = iommu_group_get(dev); struct group_pasid *param;
mutex_lock(&group->mutex); domain->ops->set_dev_pasid(group->blocking_domain, dev, pasid); param = xa_erase(&group->pasid_array, pasid); WARN_ON(!param || param->domain != domain); mutex_unlock(&group->mutex);
iommu_group_put(group); kfree(param); }
Does this look right to you?
Best regards, baolu
| |