Re: [PATCH v12 07/17] iommu: Try to allocate blocking domain when probing device
From: Baolu Lu
Date: Tue Aug 30 2022 - 21:55:27 EST
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@xxxxxxxxxxxxxxx>
Tested-by: Zhangfei Gao<zhangfei.gao@xxxxxxxxxx>
Tested-by: Tony Zhu<tony.zhu@xxxxxxxxx>
---
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