Nouveau - Jul 2018 - [RFC PATCH v1 0/6] Resolve unwanted DMA backing with IOMMU

Hello,

There is a trouble on ARM with DMA allocations made by device drivers,
the trouble is that DMA allocations are getting implicitly backed with
IOMMU mapping by the driver core if IOMMU presents in a system and IOMMU
could handle device. This is an undesired behaviour for drivers that
manage IOMMU by themselves, like NVIDIA Tegra GPU driver. On arm32 the
implicit backing happens if CONFIG_ARM_DMA_USE_IOMMU=y (multiplatform
kernel configuration), on arm64 it happens if IOMMU domain type for a
device is equal to IOMMU_DOMAIN_DMA.

The proposed solution adds a new option to the base device driver
structure that allows device drivers to explicitly convey to the drivers
core that the implicit IOMMU backing for devices must not happen.

Dmitry Osipenko (6):
  driver core: Add option for disabling of backing devices DMA with
    IOMMU
  of/device: Don't back devices DMA with IOMMU if that's undesired by
    driver
  drm/tegra: Avoid implicit DMA backing with IOMMU
  gpu: host1x: Avoid implicit DMA backing with IOMMU
  drm/nouveau: tegra: Universally avoid implicit DMA backing with IOMMU
  Revert "drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping"

 drivers/gpu/drm/nouveau/nouveau_platform.c         |  1 +
 drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 13 -------------
 drivers/gpu/drm/tegra/dc.c                         |  1 +
 drivers/gpu/drm/tegra/gr2d.c                       |  1 +
 drivers/gpu/drm/tegra/gr3d.c                       |  1 +
 drivers/gpu/drm/tegra/vic.c                        |  1 +
 drivers/gpu/host1x/dev.c                           |  1 +
 drivers/of/device.c                                |  7 +++++++
 include/linux/device.h                             |  2 ++
 9 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.18.0

This allows device drivers to convey the drivers core that implicit IOMMU
backing for devices DMA shouldn't happen. It is needed for drivers that
manage IOMMU by themselves, like for example it is needed by the NVIDIA
Tegra GPU driver.

Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
---
 include/linux/device.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index ad43a97b50c8..f9e3c1d42abd 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -244,6 +244,7 @@ enum probe_type {
  * @bus:	The bus which the device of this driver belongs to.
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
+ * @no_implicit_iommu: Disables backing DMA allocations with IOMMU mapping.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
  * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
  * @of_match_table: The open firmware table.
@@ -281,6 +282,7 @@ struct device_driver {
 	struct module		*owner;
 	const char		*mod_name;	/* used for built-in modules */
 
+	bool no_implicit_iommu;		/* disables implicit IOMMU for DMA */
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
 	enum probe_type probe_type;
 
-- 
2.18.0

Respect device driver requirement for device DMA not to be implicitly
backed with IOMMU by skipping the backing setup for drivers that do not
want that.

Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
---
 drivers/of/device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 33d85511d790..e70b7a886875 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -163,6 +163,13 @@ int of_dma_configure(struct device *dev, struct device_node
*np, bool force_dma)
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
+	/*
+	 * Respect device driver requirement for device DMA not to be
+	 * implicitly backed with IOMMU.
+	 */
+	if (iommu && dev->driver->no_implicit_iommu)
+		iommu = NULL;
+
 	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
 
 	return 0;
-- 
2.18.0

Tegra DRM manages IOMMU by itself, backing DMA with IOMMU by the drivers
core breaks the Tegra driver.

Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
---
 drivers/gpu/drm/tegra/dc.c   | 1 +
 drivers/gpu/drm/tegra/gr2d.c | 1 +
 drivers/gpu/drm/tegra/gr3d.c | 1 +
 drivers/gpu/drm/tegra/vic.c  | 1 +
 4 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index eb9bb83f8f5d..4827770939e3 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -2675,6 +2675,7 @@ static const struct dev_pm_ops tegra_dc_pm_ops = {
 struct platform_driver tegra_dc_driver = {
 	.driver = {
 		.name = "tegra-dc",
+		.no_implicit_iommu = true,
 		.of_match_table = tegra_dc_of_match,
 		.pm = &tegra_dc_pm_ops,
 	},
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index 3c5503f1bf3d..e427585ef5cc 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -327,6 +327,7 @@ static int gr2d_remove(struct platform_device *pdev)
 struct platform_driver tegra_gr2d_driver = {
 	.driver = {
 		.name = "tegra-gr2d",
+		.no_implicit_iommu = true,
 		.of_match_table = gr2d_match,
 	},
 	.probe = gr2d_probe,
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 651e697ccbba..f54710c36afb 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -481,6 +481,7 @@ static int gr3d_remove(struct platform_device *pdev)
 struct platform_driver tegra_gr3d_driver = {
 	.driver = {
 		.name = "tegra-gr3d",
+		.no_implicit_iommu = true,
 		.of_match_table = tegra_gr3d_match,
 	},
 	.probe = gr3d_probe,
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
index 0e6642bb8524..4ecc466ee490 100644
--- a/drivers/gpu/drm/tegra/vic.c
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -402,6 +402,7 @@ static const struct dev_pm_ops vic_pm_ops = {
 struct platform_driver tegra_vic_driver = {
 	.driver = {
 		.name = "tegra-vic",
+		.no_implicit_iommu = true,
 		.of_match_table = vic_match,
 		.pm = &vic_pm_ops
 	},
-- 
2.18.0

Host1x driver manages IOMMU by itself, backing DMA with IOMMU by the
drivers core breaks the Host1x driver.

Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
---
 drivers/gpu/host1x/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index afabd33a48d9..0966415d4ccd 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -361,6 +361,7 @@ static int host1x_remove(struct platform_device *pdev)
 static struct platform_driver tegra_host1x_driver = {
 	.driver = {
 		.name = "tegra-host1x",
+		.no_implicit_iommu = true,
 		.of_match_table = host1x_of_match,
 	},
 	.probe = host1x_probe,
-- 
2.18.0

Implicit backing DMA with IOMMU breaks Nouveau on Tegra, the current
approach with detaching device from IOMMU that was added in commit
b59fb482b522 ("drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping")
works only for arm32 which has the CONFIG_ARM_DMA_USE_IOMMU, but not for
arm64 which doesn't have that config option. Drivers core now allows to
avoid the implicit backing, that is a universal solution unlike the
current variant with the detaching.

Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
---
 drivers/gpu/drm/nouveau/nouveau_platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_platform.c
b/drivers/gpu/drm/nouveau/nouveau_platform.c
index 039e23548e08..0b57f4f9b638 100644
--- a/drivers/gpu/drm/nouveau/nouveau_platform.c
+++ b/drivers/gpu/drm/nouveau/nouveau_platform.c
@@ -90,6 +90,7 @@ MODULE_DEVICE_TABLE(of, nouveau_platform_match);
 struct platform_driver nouveau_platform_driver = {
 	.driver = {
 		.name = "nouveau",
+		.no_implicit_iommu = true,
 		.of_match_table = of_match_ptr(nouveau_platform_match),
 	},
 	.probe = nouveau_platform_probe,
-- 
2.18.0

Improper DMA backing with IOMMU has been resolved now using the new
drivers core option that allows to avoid the implicit backing, hence
detaching isn't necessary anymore.

This reverts commit b59fb482b52269977ee5de205308e5b236a03917.

Signed-off-by: Dmitry Osipenko <digetx at gmail.com>
---
 drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 0e372a190d3f..78597da6313a 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -23,10 +23,6 @@
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
 #include "priv.h"
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
 static int
 nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev)
 {
@@ -109,15 +105,6 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra
*tdev)
 	unsigned long pgsize_bitmap;
 	int ret;
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);
-		arm_iommu_release_mapping(mapping);
-	}
-#endif
-
 	if (!tdev->func->iommu_bit)
 		return;
 
-- 
2.18.0

On Fri, Jul 27, 2018 at 02:16:18AM +0300, Dmitry Osipenko
wrote:
> The proposed solution adds a new option to the base device driver
> structure that allows device drivers to explicitly convey to the drivers
> core that the implicit IOMMU backing for devices must not happen.
Why is IOMMU mapping a problem for the Tegra GPU driver?

If we add something like this then it should not be the choice of the
device driver, but of the user and/or the firmware.


	Joerg

On Fri, Jul 27, 2018 at 10:25:13AM +0200, Joerg Roedel
wrote:
> On Fri, Jul 27, 2018 at 02:16:18AM +0300, Dmitry Osipenko wrote:
> > The proposed solution adds a new option to the base device driver
> > structure that allows device drivers to explicitly convey to the
drivers
> > core that the implicit IOMMU backing for devices must not happen.
> 
> Why is IOMMU mapping a problem for the Tegra GPU driver?
> 
> If we add something like this then it should not be the choice of the
> device driver, but of the user and/or the firmware.
Agreed, and it would still need somebody to configure an identity domain so
that transactions aren't aborted immediately. We currently allow the
identity domain to be used by default via a command-line option, so I guess
we'd need a way for firmware to request that on a per-device basis.

Will