Introduce device_iommu_maped() function

Hi,

here is a patch-set to replace the dev->iommu_group checks in the source tree by a proper function call. The pointer checks mostly happen to check whether a device is managed my an IOMMU. For that purpose a pointer check is not very descriptable, so replace it by a function call that make its purpose readable.

This also starts to remove direct access to the dev->iommu_group pointer outside of iommu-code. This is another move towards consolidating the various iommu-related pointers in 'struct device' into one pointer only.

Please review.

Thanks,

Joerg

Joerg Roedel (5):
driver core: Introduce device_iommu_mapped() function iommu/of: Use device_iommu_mapped() ACPI/IORT: Use device_iommu_mapped() powerpc/iommu: Use device_iommu_mapped()
xhci: Use device_iommu_mapped()

 arch/powerpc/kernel/eeh.c   |  2 +-
 arch/powerpc/kernel/iommu.c |  6 +++---
 drivers/acpi/arm64/iort.c   |  2 +-
 drivers/iommu/of_iommu.c    |  2 +-
 drivers/usb/host/xhci.c     |  2 +-
 include/linux/device.h      | 10 ++++++++++
 6 files changed, 17 insertions(+), 7 deletions(-)

2.17.1

From: Joerg Roedel <[email protected]>

Some places in the kernel check the iommu_group pointer in 'struct device' in order to find ot whether a device is mapped by an IOMMU.

This is not good way to make this check, as the pointer will be moved to 'struct dev_iommu_data'. This way to make the check is also not very readable.

Introduce an explicit function to perform this check.

---
 include/linux/device.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 1b25c7a43f4c..6cb4640b6160 100644
@@ -1058,6 +1058,16 @@ static inline struct device *kobj_to_dev(struct kobject *kobj)
 	return container_of(kobj, struct device, kobj);
 }
 
+/**
+ * device_iommu_mapped - Returns true when the device DMA is translated
+ *			 by an IOMMU
+ * @dev: Device to perform the check on
+ */
+static inline bool device_iommu_mapped(struct device *dev)
+{
+	return (dev->iommu_group != NULL);
+}
+
 /* Get the wakeup routines, which depend on struct device */
 #include <linux/pm_wakeup.h>
 

2.17.1

Thanks a lot, Greg. Added your Acked-by to the patch.

Regards,

Joerg

From: Joerg Roedel <[email protected]>

Use Use device_iommu_mapped() to check if the device is already mapped by an IOMMU.

---
 drivers/iommu/of_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c5dd63072529..bfcf139503f0 100644
@@ -220,7 +220,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 	 * If we have reason to believe the IOMMU driver missed the initial
 	 * add_device callback for dev, replay it to get things in order.
 	 */
-	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
+	if (ops && ops->add_device && dev->bus && !device_iommu_mapped(dev))
 		err = ops->add_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */

2.17.1

FWIW, this check (and its ACPI equivalent in patch #3) is specifically asking "has .add_device() already been called?", rather than the more general "is this device managed by an IOMMU?" (to which the exact answer at this point is "yes, provided we return successfully from here").

I have no objection to the change as-is - especially if that usage is within the intended scope of this API - I just wanted to call it out in case you're also planning to introduce something else which would be even more appropriate for that.

Robin.

Hi Robin,

Yes, the purpose of the device_iommu_mapped() functions is to check whether the device has been initialized by the IOMMU driver that handles it, if any.

So it answers the question: Can I use the device in an IOMMU-API call? And it is more readable than the dev->iommu_group checks everywhere :)

Regards,

Joerg

OK, another way to consider the usage here would be "is the device ready to use in IOMMU API calls (or do I need to call add_device to finish setting it up)?", so it does in fact seem like a perfect fit, great!

For sure - although I am now wondering whether "mapped" is perhaps a little ambiguous in the naming, since the answer to "can I use the API" is yes even when the device may currently be attached to an identity/passthrough domain or blocked completely, neither of which involve any "mapping". Maybe simply "device_has_iommu()" would convey the intent better?

Robin.

The name is shorter version of:

	device_is_behind_an_iommu_remapping_its_dma_transactions()
	:)

The name is not perfect, but device_has_iommu() is not better because it might be considered as a check whether the device itself has an IOMMU built-in.

In the end an identity-mapping is also still a mapping (if the iommu needs a page-table for that is an implementation detail), as is a mapping with no page-table entries at all (blocking). So I think device_iommu_mapped() is a reasonable choice.

Regards,

Joerg

From: Joerg Roedel <[email protected]>

Replace the iommu-check with a proper and readable function call.

---
 drivers/acpi/arm64/iort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 70f4e80b9246..0125c8eb9e81 100644
@@ -805,7 +805,7 @@ static inline int iort_add_device_replay(const struct iommu_ops *ops,
 {
 	int err = 0;
 
-	if (ops->add_device && dev->bus && !dev->iommu_group)
+	if (ops->add_device && dev->bus && !device_iommu_mapped(dev))
 		err = ops->add_device(dev);
 
 	return err;

2.17.1

From: Joerg Roedel <[email protected]>

Use the new function to replace the open-coded iommu check.

---
 arch/powerpc/kernel/eeh.c   | 2 +-
 arch/powerpc/kernel/iommu.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

index 6cae6b56ffd6..23fe62f11486 100644
@@ -1472,7 +1472,7 @@ static int dev_has_iommu_table(struct device *dev, void *data)
 	if (!dev)
 		return 0;
 
-	if (dev->iommu_group) {
+	if (device_iommu_mapped(dev)) {
 		*ppdev = pdev;
 		return 1;
 	}

index f0dc680e659a..48d58d1dcac2 100644
@@ -1086,7 +1086,7 @@ int iommu_add_device(struct device *dev)
 	if (!device_is_registered(dev))
 		return -ENOENT;
 
-	if (dev->iommu_group) {
+	if (device_iommu_mapped(dev)) {
 		pr_debug("%s: Skipping device %s with iommu group %d\n",
 			 __func__, dev_name(dev),
 			 iommu_group_id(dev->iommu_group));

@@ -1129,7 +1129,7 @@ void iommu_del_device(struct device *dev)
 	 * and we needn't detach them from the associated
 	 * IOMMU groups
 	 */
-	if (!dev->iommu_group) {
+	if (!device_iommu_mapped(dev)) {
 		pr_debug("iommu_tce: skipping device %s with no tbl\n",
 			 dev_name(dev));
 		return;

@@ -1148,7 +1148,7 @@ static int tce_iommu_bus_notifier(struct notifier_block *nb,
         case BUS_NOTIFY_ADD_DEVICE:
                 return iommu_add_device(dev);
         case BUS_NOTIFY_DEL_DEVICE:
-                if (dev->iommu_group)
+                if (device_iommu_mapped(dev))
                         iommu_del_device(dev);
                 return 0;
         default:

2.17.1

From: Joerg Roedel <[email protected]>

Replace the dev->iommu_group check with a proper function call that better reprensents its purpose.

---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c928dbbff881..5ab97e54d070 100644
@@ -244,7 +244,7 @@ static void xhci_zero_64b_regs(struct xhci_hcd *xhci)
 	 * an iommu. Doing anything when there is no iommu is definitely
 	 * unsafe...
 	 */
-	if (!(xhci->quirks & XHCI_ZERO_64B_REGS) || !dev->iommu_group)
+	if (!(xhci->quirks & XHCI_ZERO_64B_REGS) || !device_iommu_mapped(dev))
 		return;
 
 	xhci_info(xhci, "Zeroing 64bit base registers, expecting fault\n");

2.17.1

Hi Joerg,

Nice, we can also clean up a whole load of vague iommu_present() usage and even one or two odd iommu_get_domain_for_dev() calls once we have this.

There looks to be one more here:

drivers/dma/sh/rcar-dmac.c: rcar_dmac_probe()

Other than that and a minor comment on the OF/IORT part, though, for the whole series:

Right, I didn't think of that yet, but it's certainly true.

True, I added a patch for that too.

Thanks, I added you Acked-by to the 5 patches I posted here.

Regards,

Joerg