Messages in this thread Patch in this message | | | Date | Wed, 31 Aug 2022 09:49:44 +0800 | Subject | Re: [PATCH v12 07/17] iommu: Try to allocate blocking domain when probing device | From | Baolu Lu <> |
| |
On 8/30/22 9:29 PM, Jason Gunthorpe wrote: > On Tue, Aug 30, 2022 at 09:46:01AM +0800, Baolu Lu wrote: >> On 2022/8/30 01:27, Jason Gunthorpe wrote: >>> On Mon, Aug 29, 2022 at 11:40:24AM +0800, Baolu Lu wrote: >>>> On 2022/8/26 22:52, Jason Gunthorpe wrote: >>>>> On Fri, Aug 26, 2022 at 08:11:31PM +0800, Lu Baolu wrote: >>>>>> Allocate the blocking domain when probing devices if the driver supports >>>>>> blocking domain allocation. Otherwise, revert to the previous behavior, >>>>>> that is, use UNMANAGED domain instead when the blocking domain is needed. >>>>>> >>>>>> 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> >>>>>> --- >>>>>> drivers/iommu/iommu.c | 29 +++++++++++++++++------------ >>>>>> 1 file changed, 17 insertions(+), 12 deletions(-) >>>>> This seems like a lot of overhead to allocate these things for every >>>>> group? >>>>> >>>>> Why not add a simple refcount on the blocking domain instead and >>>>> allocate the domain on the pasid attach like we do for ownership? >>>> >>>> I am working towards implementing static instance of blocking domain for >>>> each IOMMU driver, and then, there's no much overhead to allocate it in >>>> the probing device path. >>> >>> Well, I thought about that and I don't think we can get >>> there in a short order. >> >> Yes. Fair enough. >> >>> Would rather you progress this series without >>> getting entangled in such a big adventure >> >> Agreed. I will drop this patch and add below code in the iommu >> interface: >> >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -3219,6 +3219,26 @@ int iommu_attach_device_pasid(struct iommu_domain >> *domain, >> return -ENODEV; >> >> mutex_lock(&group->mutex); >> + >> + /* >> + * The underlying IOMMU driver needs to support blocking domain >> + * allocation and the callback to block DMA transactions with a >> + * specific PASID. >> + */ >> + if (!group->blocking_domain) { >> + group->blocking_domain = __iommu_domain_alloc(dev->bus, >> + IOMMU_DOMAIN_BLOCKED); >> + if (!group->blocking_domain) { >> + ret = -ENODEV; >> + goto out_unlock; >> + } >> + } >> + >> + if (!group->blocking_domain->ops->set_dev_pasid) { >> + ret = -EOPNOTSUPP; >> + goto out_unlock; >> + } >> + >> curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, >> GFP_KERNEL); >> if (curr) { >> ret = xa_err(curr) ? : -EBUSY; >> >> Currently both ARM SMMUv3 and VT-d drivers use static blocking domain. >> Hence I didn't use a refcount for blocking domain release here. > > I don't think that works in the general case, you can't just destroy > what is in group->blocking_domain..
If I understand you correctly, we can't just free the blocking domain and forget about whether this domain is still set on any device?
> > Maybe all of this is just the good reason to go to a simple > device->ops->remove_dev_pasid() callback and forget about blocking > domain here.
Do you mean rolling back to what we did in v10?
--- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -262,6 +262,8 @@ struct iommu_ops { * struct iommu_domain_ops - domain specific operations * @attach_dev: attach an iommu domain to a device * @detach_dev: detach an iommu domain from a device + * @set_dev_pasid: set an iommu domain to a pasid of device + * @block_dev_pasid: block pasid of device from using iommu domain * @map: map a physically contiguous memory region to an iommu domain * @map_pages: map a physically contiguous set of pages of the same size to * an iommu domain. @@ -282,6 +284,10 @@ struct iommu_ops { struct iommu_domain_ops { int (*attach_dev)(struct iommu_domain *domain, struct device *dev); void (*detach_dev)(struct iommu_domain *domain, struct device *dev); + int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev, + ioasid_t pasid); + void (*block_dev_pasid)(struct iommu_domain *domain, struct device *dev, + ioasid_t pasid);
Best regards, baolu
| |