iommu: Retire bus ops

The pattern for picking the first device out of the group list is repeated a few times now, so it's clearly worth factoring out, which also helps hide the iommu_group_dev detail from places that don't need to know. Similarly, the safety check for dev_iommu_ops() at certain public interfaces starts looking a bit repetitive, and might not be completely obvious at first glance, so let's factor that out for clarity as well, in preparation for more uses of both.

Reviewed-by Lu Baolu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: Rename dev_iommu_ops_valid() to reflect what it's actually checking,
rather than an implied consequence.
v4: Rebase, also catch the sneaky one in iommu_get_group_resv_regions()
which wasn't the full pattern (but really should be since it guards the dev_iommu_ops() invocation in iommu_get_resv_regions()).

---
 drivers/iommu/iommu.c | 56 ++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

The pattern for picking the first device out of the group list is repeated a few times now, so it's clearly worth factoring out, which also helps hide the iommu_group_dev detail from places that don't need to know. Similarly, the safety check for dev_iommu_ops() at certain public interfaces starts looking a bit repetitive, and might not be completely obvious at first glance, so let's factor that out for clarity as well, in preparation for more uses of both.

Reviewed-by Lu Baolu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: Rename dev_iommu_ops_valid() to reflect what it's actually checking,
rather than an implied consequence.
v4: Rebase, also catch the sneaky one in iommu_get_group_resv_regions()
which wasn't the full pattern (but really should be since it guards the dev_iommu_ops() invocation in iommu_get_resv_regions()).
---

Much as I'd like to remove iommu_present(), the final remaining users are proving stubbornly difficult to clean up, so kick that can down the road and just rework it to preserve the current behaviour without depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup

to IOMMU device registration"), any registered IOMMU instance is already considered "present" for every entry in iommu_buses, so it's simply a case of validating the bus and checking we have at least once IOMMU.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: Tweak to use the ops-based check rather than group-based, to
properly match the existing behaviour
v4: Just look for IOMMU instances instead of managed devices

---
  drivers/iommu/iommu.c | 21 ++++++++++++++++++++-
  1 file changed, 20 insertions(+), 1 deletion(-)

drivers/iommu/iommu.c +20 -1

index 5a3ce293a5de..7bb92e8b7a49 100644
@@ -2000,9 +2000,28 @@ int bus_iommu_probe(const struct bus_type *bus)
  	return 0;
  }
 
+/**
+ * iommu_present() - make platform-specific assumptions about an IOMMU
+ * @bus: bus to check
+ *
+ * Do not use this function. You want device_iommu_mapped() instead.
+ *
+ * Return: true if some IOMMU is present and aware of devices on the given bus;
+ * in general it may not be the only IOMMU, and it may not have anything to do
+ * with whatever device you are ultimately interested in.
+ */
  bool iommu_present(const struct bus_type *bus)
  {
-	return bus->iommu_ops != NULL;
+	bool ret = false;
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+		if (iommu_buses[i] == bus) {
+			spin_lock(&iommu_device_lock);
+			ret = !list_empty(&iommu_device_list);
+			spin_unlock(&iommu_device_lock);
+		}
+	}
+	return ret;
  }
  EXPORT_SYMBOL_GPL(iommu_present);
 

Quoted 4 lines · Robin Murphy

Much as I'd like to remove iommu_present(), the final remaining users are proving stubbornly difficult to clean up, so kick that can down the road and just rework it to preserve the current behaviour without depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup

The iommu_present() is only used in below two drivers.

$ git grep iommu_present
drivers/gpu/drm/mediatek/mtk_drm_drv.c: if (!iommu_present(&platform_bus_type))
drivers/gpu/drm/tegra/drm.c:    if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) {

Both are platform drivers and have the device pointer passed in. Just out of curiosity, why not replacing them with device_iommu_mapped() instead? Sorry if I overlooked previous discussion.

Best regards,
baolu

Quoted 50 lines

to IOMMU device registration"), any registered IOMMU instance is already considered "present" for every entry in iommu_buses, so it's simply a case of validating the bus and checking we have at least once IOMMU.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: Tweak to use the ops-based check rather than group-based, to properly match the existing behaviour
v4: Just look for IOMMU instances instead of managed devices
---
  drivers/iommu/iommu.c | 21 ++++++++++++++++++++-

  1 file changed, 20 insertions(+), 1 deletion(-)

drivers/iommu/iommu.c +8 -0

index 5a3ce293a5de..7bb92e8b7a49 100644
@@ -2000,9 +2000,28 @@ int bus_iommu_probe(const struct bus_type *bus)
       return 0;
   }
+/**
+ * iommu_present() - make platform-specific assumptions about an IOMMU
+ * @bus: bus to check
+ *
+ * Do not use this function. You want device_iommu_mapped() instead.
+ *
+ * Return: true if some IOMMU is present and aware of devices on the 
 given bus;
+ * in general it may not be the only IOMMU, and it may not have 

anything to do
+ * with whatever device you are ultimately interested in.
+ */
  bool iommu_present(const struct bus_type *bus) {
-    return bus->iommu_ops != NULL;
+    bool ret = false;
+
+    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { +        if (iommu_buses[i] == bus) {
+            spin_lock(&iommu_device_lock);
+            ret = !list_empty(&iommu_device_list); +            spin_unlock(&iommu_device_lock);
+        }
+    }
+    return ret;
  }
  EXPORT_SYMBOL_GPL(iommu_present);

Quoted 14 lines · Baolu Lu

Quoted 4 lines · Robin Murphy

Much as I'd like to remove iommu_present(), the final remaining users are proving stubbornly difficult to clean up, so kick that can down the road and just rework it to preserve the current behaviour without depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup

The iommu_present() is only used in below two drivers.

$ git grep iommu_present
drivers/gpu/drm/mediatek/mtk_drm_drv.c: if
(!iommu_present(&platform_bus_type))
drivers/gpu/drm/tegra/drm.c:    if (host1x_drm_wants_iommu(dev) &&
iommu_present(&platform_bus_type)) {

Quoted 3 lines

Both are platform drivers and have the device pointer passed in. Just out of curiosity, why not replacing them with device_iommu_mapped() instead? Sorry if I overlooked previous discussion.

Yes, we've already gone round in circles on this several times, that's why it's explicitly called out as "stubbornly difficult" in the commit message. The Mediatek one is entirely redundant, but it seems I have yet to figure out the right CC list to get anyone to care about that patch[1].

Quoted 15 lines · Robin Murphy

Quoted 14 lines · Baolu Lu

Quoted 4 lines · Robin Murphy

Much as I'd like to remove iommu_present(), the final remaining users are proving stubbornly difficult to clean up, so kick that can down the road and just rework it to preserve the current behaviour without depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup

The iommu_present() is only used in below two drivers.

$ git grep iommu_present
drivers/gpu/drm/mediatek/mtk_drm_drv.c: if
(!iommu_present(&platform_bus_type))
drivers/gpu/drm/tegra/drm.c:    if (host1x_drm_wants_iommu(dev) &&
iommu_present(&platform_bus_type)) {

Quoted 8 lines

Quoted 3 lines

Both are platform drivers and have the device pointer passed in. Just out of curiosity, why not replacing them with device_iommu_mapped() instead? Sorry if I overlooked previous discussion.

Yes, we've already gone round in circles on this several times, that's why it's explicitly called out as "stubbornly difficult" in the commit message. The Mediatek one is entirely redundant, but it seems I have yet to figure out the right CC list to get anyone to care about that patch[1].

Please just have Joerg take such a trivial patch, there is no reason we need to torture outselves because DRM side is not behaving well. :(

Much as I'd like to remove iommu_present(), the final remaining users are proving stubbornly difficult to clean up, so kick that can down the road and just rework it to preserve the current behaviour without depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup to IOMMU device registration"), any registered IOMMU instance is already considered "present" for every entry in iommu_buses, so it's simply a case of validating the bus and checking we have at least once IOMMU.

Reviewed-by Jason Gunthorpe<[email protected]>

Signed-off-by Robin Murphy<[email protected]>

Much as I'd like to remove iommu_present(), the final remaining users are proving stubbornly difficult to clean up, so kick that can down the road and just rework it to preserve the current behaviour without depending on bus ops. Since commit 57365a04c921 ("iommu: Move bus setup to IOMMU device registration"), any registered IOMMU instance is already considered "present" for every entry in iommu_buses, so it's simply a case of validating the bus and checking we have at least once IOMMU.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v3: Tweak to use the ops-based check rather than group-based, to
properly match the existing behaviour
v4: Just look for IOMMU instances instead of managed devices
---

Before we can allow drivers to coexist, we need to make sure that one driver's domain ops can't misinterpret another driver's dev_iommu_priv data. To that end, add a token to the domain so we can remember how it was allocated - for now this may as well be the device ops, since they still correlate 1:1 with drivers. We can trust ourselves for internal default domain attachment, so add checks to cover all the public attach interfaces.

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v4: Cover iommu_attach_device_pasid() as well, and improve robustness
against theoretical attempts to attach a noiommu group.
---

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct device *dev) · line 2291

Quoted 13 lines · Robin Murphy

drivers/iommu/iommu.c +5 -0

@@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
  static int __iommu_attach_group(struct iommu_domain *domain,
  				struct iommu_group *group)
  {
+	struct device *dev;
+
  	if (group->domain && group->domain != group->default_domain &&
  	    group->domain != group->blocking_domain)
  		return -EBUSY;
 
+	dev = iommu_group_first_dev(group);
+	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+		return -EINVAL;

I was thinking about this later, how does this work for the global static domains? domain->owner will not be set?

	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
		return ops->identity_domain;
	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
		return ops->blocked_domain;

Seems like it will break everything?

Quoted 23 lines · Jason Gunthorpe

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct device *dev) · line 2290

Quoted 12 lines · Robin Murphy

drivers/iommu/iommu.c +5 -0

@@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
  static int __iommu_attach_group(struct iommu_domain *domain,
  				struct iommu_group *group)
  {
+	struct device *dev;
+
  	if (group->domain && group->domain != group->default_domain &&
  	    group->domain != group->blocking_domain)
  		return -EBUSY;
+	dev = iommu_group_first_dev(group);
+	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+		return -EINVAL;

I was thinking about this later, how does this work for the global static domains? domain->owner will not be set?

	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
		return ops->identity_domain;
	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
		return ops->blocked_domain;

Quoted 1 line

Seems like it will break everything?

I don't believe it makes any significant difference - as the commit message points out, this validation is only applied at the public interface boundaries of iommu_attach_group(), iommu_attach_device(),

and iommu_attach_device_pasid(), which are only expected to be operating on explicitly-allocated unmanaged domains.

Quoted 24 lines · Robin Murphy

Quoted 23 lines · Jason Gunthorpe

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct device *dev) · line 2290

Quoted 12 lines · Robin Murphy

drivers/iommu/iommu.c +5 -0

@@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
   static int __iommu_attach_group(struct iommu_domain *domain,
   				struct iommu_group *group)
   {
+	struct device *dev;
+
   	if (group->domain && group->domain != group->default_domain &&
   	    group->domain != group->blocking_domain)
   		return -EBUSY;
+	dev = iommu_group_first_dev(group);
+	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+		return -EINVAL;

I was thinking about this later, how does this work for the global static domains? domain->owner will not be set?

	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
		return ops->identity_domain;
	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
		return ops->blocked_domain;

Quoted 5 lines

Quoted 1 line

Seems like it will break everything?

I don't believe it makes any significant difference - as the commit message points out, this validation is only applied at the public interface boundaries of iommu_attach_group(), iommu_attach_device(),

Oh, making it only work for on domain type seems kind of hacky..

If that is the intention maybe the owner set should be moved into iommu_domain_alloc() with a little comment noting that it is limited to work in only a few cases?

I certainly didn't understand from the commit message to mean it was only actually working for one domain type and this also blocks using other types with the public interface.

Quoted 2 lines

and iommu_attach_device_pasid(), which are only expected to be operating on explicitly-allocated unmanaged domains.

We have nesting now in the iommufd branch, and SVA will come soon for these APIs.

Regardless this will clash with the iommufd branch for this reason so I guess it needs to wait till rc1.

Quoted 33 lines · Jason Gunthorpe

Quoted 24 lines · Robin Murphy

Quoted 23 lines · Jason Gunthorpe

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct device *dev) · line 2290

Quoted 12 lines · Robin Murphy

drivers/iommu/iommu.c +5 -0

@@ -2279,10 +2280,16 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev)
   static int __iommu_attach_group(struct iommu_domain *domain,
   				struct iommu_group *group)
   {
+	struct device *dev;
+
   	if (group->domain && group->domain != group->default_domain &&
   	    group->domain != group->blocking_domain)
   		return -EBUSY;
+	dev = iommu_group_first_dev(group);
+	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
+		return -EINVAL;

I was thinking about this later, how does this work for the global static domains? domain->owner will not be set?

	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
		return ops->identity_domain;
	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
		return ops->blocked_domain;

Quoted 5 lines

Quoted 1 line

Seems like it will break everything?

I don't believe it makes any significant difference - as the commit message points out, this validation is only applied at the public interface boundaries of iommu_attach_group(), iommu_attach_device(),

Oh, making it only work for on domain type seems kind of hacky..

Quoted 7 lines

If that is the intention maybe the owner set should be moved into iommu_domain_alloc() with a little comment noting that it is limited to work in only a few cases?

I certainly didn't understand from the commit message to mean it was only actually working for one domain type and this also blocks using other types with the public interface.

It's not about one particular domain type, it's about the scope of what we consider valid usage. External API users should almost always be attaching to their own domain which they have allocated, however we also tolerate co-attaching additional groups to the same DMA domain in rare cases where it's reasonable. The fact is that those users cannot allocate blocking or identity domains, and I can't see that they would ever have any legitimate business trying to do anything with them anyway. So although yes, we technically lose some functionality once this intersects with the static domain optimisation, it's only questionable functionality which was never explicitly intended anyway.

As the final remaining piece of bus-dependent API, iommu_domain_alloc() can now take responsibility for the "one iommu_ops per bus" rule for itself. It turns out we can't safely make the internal allocation call any more group-based or device-based yet - that will have to wait until the external callers can pass the right thing - but we can at least get as far as deriving "bus ops" based on which driver is actually managing devices on the given bus, rather than whichever driver won the race to register first.

This will then leave us able to convert the last of the core internals over to the IOMMU-instance model, allow multiple drivers to register and actually coexist (modulo the above limitation for unmanaged domain users in the short term), and start trying to solve the long-standing iommu_probe_device() mess.

Signed-off-by Robin Murphy <[email protected]>

---

v5: Rewrite, de-scoping to just retrieve ops under the same assumptions
as the existing code.

---
 drivers/iommu/iommu.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

As the final remaining piece of bus-dependent API, iommu_domain_alloc() can now take responsibility for the "one iommu_ops per bus" rule for itself. It turns out we can't safely make the internal allocation call any more group-based or device-based yet - that will have to wait until the external callers can pass the right thing - but we can at least get as far as deriving "bus ops" based on which driver is actually managing devices on the given bus, rather than whichever driver won the race to register first.

This will then leave us able to convert the last of the core internals over to the IOMMU-instance model, allow multiple drivers to register and actually coexist (modulo the above limitation for unmanaged domain users in the short term), and start trying to solve the long-standing iommu_probe_device() mess.

Signed-off-by Robin Murphy <[email protected]>

---

v5: Rewrite, de-scoping to just retrieve ops under the same assumptions
as the existing code.
---

When using the legacy binding we bypass the of_xlate mechanism, so avoid registering the instance fwnodes which act as keys for that. This will help __iommu_probe_device() to retrieve the registered ops the same way as for x86 etc. when no fwspec has previously been set up by of_xlate.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/iommu/arm/arm-smmu/arm-smmu.c +2 -1

index d6d1a2a55cc0..4b83a3adacd6 100644
@@ -2161,7 +2161,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
+	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
+				    using_legacy_binding ? NULL : dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
 		iommu_device_sysfs_remove(&smmu->iommu);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

When using the legacy binding we bypass the of_xlate mechanism, so avoid registering the instance fwnodes which act as keys for that. This will help __iommu_probe_device() to retrieve the registered ops the same way as for x86 etc. when no fwspec has previously been set up by of_xlate.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/iommu/arm/arm-smmu/arm-smmu.c +2 -1

index d6d1a2a55cc0..4b83a3adacd6 100644
@@ -2161,7 +2161,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
+	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
+				    using_legacy_binding ? NULL : dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
 		iommu_device_sysfs_remove(&smmu->iommu);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

With the rest of the API internals converted, it's time to finally tackle probe_device and how we bootstrap the per-device ops association to begin with. This ends up being disappointingly straightforward, since fwspec users are already doing it in order to find their of_xlate callback, and it works out that we can easily do the equivalent for other drivers too. Then shuffle the remaining awareness of iommu_ops into the couple of core headers that still need it, and breathe a sigh of relief.

Ding dong the bus ops are gone!

CC Rafael J. Wysocki <[email protected]>

Acked-by Christoph Hellwig <[email protected]>

Acked-by Greg Kroah-Hartman <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v4: Don't forget new reference in iommu_device_register_bus()

---
 drivers/iommu/iommu.c       | 31 ++++++++++++++++++-------------
 include/acpi/acpi_bus.h     |  2 ++
 include/linux/device.h      |  1 -
 include/linux/device/bus.h  |  5 -----
 include/linux/dma-map-ops.h |  1 +
 5 files changed, 21 insertions(+), 19 deletions(-)

Some drivers already implement their own defence against the possibility of being given someone else's device. Since this is now taken care of by the core code (and via a slightly different path from the original fwspec-based idea), let's clean them up.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  3 ---
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  9 +--------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     | 16 +++-------------
 drivers/iommu/mtk_iommu.c                   |  7 +------
 drivers/iommu/mtk_iommu_v1.c                |  3 ---
 drivers/iommu/sprd-iommu.c                  |  8 +-------
 drivers/iommu/virtio-iommu.c                |  3 ---
 7 files changed, 6 insertions(+), 43 deletions(-)

Some drivers already implement their own defence against the possibility of being given someone else's device. Since this is now taken care of by the core code (and via a slightly different path from the original fwspec-based idea), let's clean them up.

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  3 ---
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  9 +--------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     | 16 +++-------------
 drivers/iommu/mtk_iommu.c                   |  7 +------
 drivers/iommu/mtk_iommu_v1.c                |  3 ---
 drivers/iommu/sprd-iommu.c                  |  8 +-------
 drivers/iommu/virtio-iommu.c                |  3 ---
 7 files changed, 6 insertions(+), 43 deletions(-)