| Message ID | 20220826121141.50743-8-baolu.lu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | iommu: SVA and IOPF refactoring | expand |
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? Jason
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. Best regards, baolu
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. Would rather you progress this series without getting entangled in such a big adventure Jason
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. Best regards, baolu
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.. 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. Jason
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
On Wed, Aug 31, 2022 at 09:49:44AM +0800, Baolu Lu wrote: > > 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? Yeah, but it shouldn't be a domain_op, removing a pasid is a device op Just remove_dev_pasid(struct device *dev, ioasid_t pasid) Jason
On 2022/8/31 22:10, Jason Gunthorpe wrote: > On Wed, Aug 31, 2022 at 09:49:44AM +0800, Baolu Lu wrote: >>> 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? > > Yeah, but it shouldn't be a domain_op, removing a pasid is a device op > > Just > > remove_dev_pasid(struct device *dev, ioasid_t pasid) It's clear now. Thanks! How about below iommu_attach/detach_device_pasid() code? By the way, how about naming it "block_dev_pasid(dev, pasid)"? +static int __iommu_set_group_pasid(struct iommu_domain *domain, + struct iommu_group *group, ioasid_t pasid) +{ + struct group_device *device; + int ret = 0; + + list_for_each_entry(device, &group->devices, list) { + ret = domain->ops->set_dev_pasid(domain, device->dev, pasid); + if (ret) + break; + } + + return ret; +} + +static void __iommu_remove_group_pasid(struct iommu_group *group, + ioasid_t pasid) +{ + struct group_device *device; + const struct iommu_ops *ops; + + list_for_each_entry(device, &group->devices, list) { + ops = dev_iommu_ops(device->dev); + ops->remove_dev_pasid(device->dev, pasid); + } +} + +/* + * 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. + * + * Return: 0 on success, or an error. + */ +int iommu_attach_device_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) +{ + struct iommu_group *group; + void *curr; + int ret; + + if (!domain->ops->set_dev_pasid) + return -EOPNOTSUPP; + + group = iommu_group_get(dev); + if (!group) + return -ENODEV; + + mutex_lock(&group->mutex); + curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL); + if (curr) { + ret = xa_err(curr) ? : -EBUSY; + goto out_unlock; + } + + ret = __iommu_set_group_pasid(domain, group, pasid); + if (ret) { + __iommu_remove_group_pasid(group, pasid); + xa_erase(&group->pasid_array, pasid); + } +out_unlock: + mutex_unlock(&group->mutex); + iommu_group_put(group); + + return ret; +} +EXPORT_SYMBOL_GPL(iommu_attach_device_pasid); + +/* + * 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_attach_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); + + mutex_lock(&group->mutex); + __iommu_remove_group_pasid(group, pasid); + WARN_ON(xa_erase(&group->pasid_array, pasid) != domain); + mutex_unlock(&group->mutex); + + iommu_group_put(group); +} +EXPORT_SYMBOL_GPL(iommu_detach_device_pasid); + * @remove_dev_pasid: Remove any translation configurations of a specific + * pasid, so that any DMA transactions with this pasid + * will be blocked by the hardware. * @pgsize_bitmap: bitmap of all possible supported page sizes * @owner: Driver module providing these ops */ @@ -256,6 +259,7 @@ struct iommu_ops { struct iommu_page_response *msg); int (*def_domain_type)(struct device *dev); + void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid); Best regards, baolu
On Thu, Sep 01, 2022 at 06:44:10PM +0800, Baolu Lu wrote: > On 2022/8/31 22:10, Jason Gunthorpe wrote: > > On Wed, Aug 31, 2022 at 09:49:44AM +0800, Baolu Lu wrote: > > > > 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? > > > > Yeah, but it shouldn't be a domain_op, removing a pasid is a device op > > > > Just > > > > remove_dev_pasid(struct device *dev, ioasid_t pasid) > > It's clear now. Thanks! > > How about below iommu_attach/detach_device_pasid() code? I think this is probably the right thing > By the way, how about naming it "block_dev_pasid(dev, pasid)"? set/remove is a better pairing that set/block Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 2c9488d966ab..e985f4d5895f 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -318,6 +318,10 @@ int iommu_probe_device(struct device *dev) mutex_lock(&group->mutex); iommu_alloc_default_domain(group, dev); + /* Try to allocate a blocking domain */ + group->blocking_domain = __iommu_domain_alloc(dev->bus, + IOMMU_DOMAIN_BLOCKED); + /* * If device joined an existing group which has been claimed, don't * attach the default domain. @@ -1778,6 +1782,10 @@ int bus_iommu_probe(struct bus_type *bus) /* Try to allocate default domain */ probe_alloc_default_domain(bus, group); + /* Try to allocate blocking domain */ + group->blocking_domain = + __iommu_domain_alloc(bus, IOMMU_DOMAIN_BLOCKED); + if (!group->default_domain) { mutex_unlock(&group->mutex); continue; @@ -3165,18 +3173,15 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group) if (group->blocking_domain) return 0; - group->blocking_domain = - __iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED); - if (!group->blocking_domain) { - /* - * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED - * create an empty domain instead. - */ - group->blocking_domain = __iommu_domain_alloc( - dev->dev->bus, IOMMU_DOMAIN_UNMANAGED); - if (!group->blocking_domain) - return -EINVAL; - } + /* + * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED + * create an empty domain instead. + */ + group->blocking_domain = __iommu_domain_alloc(dev->dev->bus, + IOMMU_DOMAIN_UNMANAGED); + if (!group->blocking_domain) + return -EINVAL; + return 0; }