[PATCH 0/5] Introduce device_iommu_maped() function

[PATCH 0/5] Introduce device_iommu_maped() function

[Joerg Roedel]

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

[PATCH 1/5] driver core: Introduce device_iommu_mapped() function

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 include/linux/device.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 1b25c7a43f4c..6cb4640b6160 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -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

Re: [PATCH 1/5] driver core: Introduce device_iommu_mapped() function

[Greg Kroah-Hartman]

On Tue, Dec 04, 2018 at 06:25:00PM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  include/linux/device.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/5] driver core: Introduce device_iommu_mapped() function

[Joerg Roedel]

On Thu, Dec 06, 2018 at 01:55:20PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 04, 2018 at 06:25:00PM +0100, Joerg Roedel wrote:
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Joerg Roedel <jroedel@suse.de>
> > ---
> >  include/linux/device.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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


Regards,

	Joerg

[PATCH 2/5] iommu/of: Use device_iommu_mapped()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/of_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c5dd63072529..bfcf139503f0 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -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

Re: [PATCH 2/5] iommu/of: Use device_iommu_mapped()

[Robin Murphy]

On 04/12/2018 17:25, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Use Use device_iommu_mapped() to check if the device is
> already mapped by an IOMMU.

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.

> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/of_iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c5dd63072529..bfcf139503f0 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -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 */
> 

Re: [PATCH 2/5] iommu/of: Use device_iommu_mapped()

[Joerg Roedel]

Hi Robin,

On Wed, Dec 05, 2018 at 05:17:54PM +0000, Robin Murphy wrote:
> 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.

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

Re: [PATCH 2/5] iommu/of: Use device_iommu_mapped()

[Robin Murphy]

On 06/12/2018 15:35, Joerg Roedel wrote:
> Hi Robin,
> 
> On Wed, Dec 05, 2018 at 05:17:54PM +0000, Robin Murphy wrote:
>> 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.
> 
> 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?

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!

> And it is more readable than the dev->iommu_group checks everywhere :)

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.

Re: [PATCH 2/5] iommu/of: Use device_iommu_mapped()

[Joerg Roedel]

On Thu, Dec 06, 2018 at 05:42:16PM +0000, Robin Murphy wrote:
> 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?

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

[PATCH 3/5] ACPI/IORT: Use device_iommu_mapped()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/acpi/arm64/iort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 70f4e80b9246..0125c8eb9e81 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -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

[PATCH 4/5] powerpc/iommu: Use device_iommu_mapped()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Russell Currey <ruscur@russell.cc>
Cc: Sam Bobroff <sbobroff@linux.ibm.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/powerpc/kernel/eeh.c   | 2 +-
 arch/powerpc/kernel/iommu.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 6cae6b56ffd6..23fe62f11486 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -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;
 	}
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index f0dc680e659a..48d58d1dcac2 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -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

[PATCH 5/5] xhci: Use device_iommu_mapped()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

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

Cc: Mathias Nyman <mathias.nyman@intel.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c928dbbff881..5ab97e54d070 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -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

Re: [PATCH 0/5] Introduce device_iommu_maped() function

[Robin Murphy]

Hi Joerg,

On 04/12/2018 17:24, Joerg Roedel wrote:
> 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.

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.

> 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(-)

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:

Acked-by: Robin Murphy <robin.murphy@arm.com>

Re: [PATCH 0/5] Introduce device_iommu_maped() function

[Joerg Roedel]

On Wed, Dec 05, 2018 at 05:17:29PM +0000, Robin Murphy wrote:
> On 04/12/2018 17:24, Joerg Roedel wrote:

> 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.

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

> There looks to be one more here:
> 
> drivers/dma/sh/rcar-dmac.c: rcar_dmac_probe()

True, I added a patch for that too.

> Other than that and a minor comment on the OF/IORT part, though, for the
> whole series:
> 
> Acked-by: Robin Murphy <robin.murphy@arm.com>

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


Regards,

	Joerg