Nouveau - Apr 2018 - [PATCH 3/4] ARM: dma-mapping: Implement arch_iommu_detach_device()

From: Thierry Reding <treding at nvidia.com>

Depending on the kernel configuration, early ARM architecture setup code
may have attached the GPU to a DMA/IOMMU mapping that transparently uses
the IOMMU to back the DMA API. Tegra requires special handling for IOMMU
backed buffers (a special bit in the GPU's MMU page tables indicates the
memory path to take: via the SMMU or directly to the memory controller).
Transparently backing DMA memory with an IOMMU prevents Nouveau from
properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory
don't need to be physically contiguous from the GPU's perspective since
the GPU can map them into contiguous buffers using its own MMU. Mapping
these buffers through the IOMMU is unnecessary and will even lead to
performance degradation because of the additional translation.

Signed-off-by: Thierry Reding <treding at nvidia.com>
---
I had already sent this out independently to fix a regression that was
introduced in v4.16, but then Christoph pointed out that it should've
been sent to a wider audience and should use a core API rather than
calling into architecture code directly.

I've added it to this series for easier reference and to show the need
for the new API.

 .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 78597da6313a..23428a7056e9 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -19,6 +19,11 @@
  * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
  * DEALINGS IN THE SOFTWARE.
  */
+
+#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
+#include <asm/dma-iommu.h>
+#endif
+
 #include <core/tegra.h>
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
 #include "priv.h"
@@ -105,6 +110,20 @@ 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_release_mapping(mapping);
+		arm_iommu_detach_device(dev);
+
+		if (dev->archdata.dma_coherent)
+			set_dma_ops(dev, &arm_coherent_dma_ops);
+		else
+			set_dma_ops(dev, &arm_dma_ops);
+	}
+#endif
+
 	if (!tdev->func->iommu_bit)
 		return;
 
-- 
2.17.0

From: Thierry Reding <treding at nvidia.com>

The dma_iommu_detach_device() API can be used by drivers to forcibly
detach a device from an IOMMU that architecture code might have attached
to. This is useful for drivers that need explicit control over the IOMMU
using the IOMMU API directly.

Signed-off-by: Thierry Reding <treding at nvidia.com>
---
 drivers/base/dma-mapping.c  | 8 ++++++++
 include/linux/dma-mapping.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index d82566d6e237..18ddf32b10c9 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -366,3 +366,11 @@ void dma_deconfigure(struct device *dev)
 	of_dma_deconfigure(dev);
 	acpi_dma_deconfigure(dev);
 }
+
+void dma_iommu_detach_device(struct device *dev)
+{
+#ifdef arch_iommu_detach_device
+	arch_iommu_detach_device(dev);
+#endif
+}
+EXPORT_SYMBOL(dma_iommu_detach_device);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index f8ab1c0f589e..732191a2c64e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -671,6 +671,8 @@ static inline void arch_setup_dma_ops(struct device *dev,
u64 dma_base,
 static inline void arch_teardown_dma_ops(struct device *dev) { }
 #endif
 
+extern void dma_iommu_detach_device(struct device *dev);
+
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
 {
 	if (dev->dma_parms && dev->dma_parms->max_segment_size)
-- 
2.17.0

From: Thierry Reding <treding at nvidia.com>

Implement this function to enable drivers from detaching from any IOMMU
domains that architecture code might have attached them to so that they
can take exclusive control of the IOMMU via the IOMMU API.

Signed-off-by: Thierry Reding <treding at nvidia.com>
---
 arch/arm/include/asm/dma-mapping.h |  3 +++
 arch/arm/mm/dma-mapping-nommu.c    |  4 ++++
 arch/arm/mm/dma-mapping.c          | 19 +++++++++++++++++++
 3 files changed, 26 insertions(+)

diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index 8436f6ade57d..d6d5bd44f962 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -103,6 +103,9 @@ extern void arch_setup_dma_ops(struct device *dev, u64
dma_base, u64 size,
 #define arch_teardown_dma_ops arch_teardown_dma_ops
 extern void arch_teardown_dma_ops(struct device *dev);
 
+#define arch_iommu_detach_device arch_iommu_detach_device
+extern void arch_iommu_detach_device(struct device *dev);
+
 /* do not use this function in a driver */
 static inline bool is_device_dma_coherent(struct device *dev)
 {
diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
index 619f24a42d09..60fef97d8452 100644
--- a/arch/arm/mm/dma-mapping-nommu.c
+++ b/arch/arm/mm/dma-mapping-nommu.c
@@ -242,6 +242,10 @@ void arch_teardown_dma_ops(struct device *dev)
 {
 }
 
+void arch_iommu_detach_device(struct device *dev)
+{
+}
+
 #define PREALLOC_DMA_DEBUG_ENTRIES	4096
 
 static int __init dma_debug_do_init(void)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 8c398fedbbb6..1957938d8c9c 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2366,6 +2366,21 @@ static void arm_teardown_iommu_dma_ops(struct device
*dev)
 	arm_iommu_release_mapping(mapping);
 }
 
+void arch_iommu_detach_device(struct device *dev)
+{
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
+	const struct dma_map_ops *dma_ops;
+
+	if (!mapping)
+		return;
+
+	arm_iommu_release_mapping(mapping);
+	arm_iommu_detach_device(dev);
+
+	dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
+	set_dma_ops(dev, dma_ops);
+}
+
 #else
 
 static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
@@ -2378,6 +2393,10 @@ static void arm_teardown_iommu_dma_ops(struct device
*dev) { }
 
 #define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
 
+void arch_iommu_detach_device(struct device *dev)
+{
+}
+
 #endif	/* CONFIG_ARM_DMA_USE_IOMMU */
 
 static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
-- 
2.17.0

From: Thierry Reding <treding at nvidia.com>

Use the new dma_iommu_detach_device() function to replace the open-coded
equivalent.

Signed-off-by: Thierry Reding <treding at nvidia.com>
---
 .../drm/nouveau/nvkm/engine/device/tegra.c    | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
index 23428a7056e9..c0a7f3839cbb 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c
@@ -20,10 +20,6 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
 #include <core/tegra.h>
 #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER
 #include "priv.h"
@@ -110,19 +106,8 @@ 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_release_mapping(mapping);
-		arm_iommu_detach_device(dev);
-
-		if (dev->archdata.dma_coherent)
-			set_dma_ops(dev, &arm_coherent_dma_ops);
-		else
-			set_dma_ops(dev, &arm_dma_ops);
-	}
-#endif
+	/* make sure we can use the IOMMU exclusively */
+	dma_iommu_detach_device(dev);
 
 	if (!tdev->func->iommu_bit)
 		return;
-- 
2.17.0

On Wed, Apr 25, 2018 at 11:18:14AM +0200, Thierry Reding wrote:
[...]
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 8c398fedbbb6..1957938d8c9c 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2366,6 +2366,21 @@ static void arm_teardown_iommu_dma_ops(struct device
*dev)
>  	arm_iommu_release_mapping(mapping);
>  }
>  
> +void arch_iommu_detach_device(struct device *dev)
> +{
> +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
> +	const struct dma_map_ops *dma_ops;
> +
> +	if (!mapping)
> +		return;
> +
> +	arm_iommu_release_mapping(mapping);
> +	arm_iommu_detach_device(dev);
> +
> +	dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
> +	set_dma_ops(dev, dma_ops);
> +}
Oh, the irony! This doesn't actually build and I failed to notice
because I forgot to enable ARM_DMA_USE_IOMMU...

Sorry about that, but I think we can use this as basis to discuss
whether the API is good and I'll make sure to properly test the next
revision before sending it out.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20180425/ee6a8fdb/attachment.sig>

Hi Thierry,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]

url:   
https://github.com/0day-ci/linux/commits/Thierry-Reding/drm-nouveau-tegra-Detach-from-ARM-DMA-IOMMU-mapping/20180426-140854
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

   arch/arm/mm/dma-mapping.c: In function
'arch_iommu_detach_device':
>> arch/arm/mm/dma-mapping.c:2380:12: error: implicit declaration of
function 'arm_get_dma_map_ops'; did you mean
'arm_get_iommu_dma_map_ops'? [-Werror=implicit-function-declaration]
     dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
               ^~~~~~~~~~~~~~~~~~~
               arm_get_iommu_dma_map_ops
>> arch/arm/mm/dma-mapping.c:2380:10: warning: assignment makes pointer
from integer without a cast [-Wint-conversion]
     dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
             ^
   arch/arm/mm/dma-mapping.c: At top level:
>> arch/arm/mm/dma-mapping.c:2402:34: error: conflicting types for
'arm_get_dma_map_ops'
    static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
                                     ^~~~~~~~~~~~~~~~~~~
   arch/arm/mm/dma-mapping.c:2380:12: note: previous implicit declaration of
'arm_get_dma_map_ops' was here
     dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
               ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +2380 arch/arm/mm/dma-mapping.c

  2368	
  2369	void arch_iommu_detach_device(struct device *dev)
  2370	{
  2371		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
  2372		const struct dma_map_ops *dma_ops;
  2373	
  2374		if (!mapping)
  2375			return;
  2376	
  2377		arm_iommu_release_mapping(mapping);
  2378		arm_iommu_detach_device(dev);
  2379	
> 2380		dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
  2381		set_dma_ops(dev, dma_ops);
  2382	}
  2383	
  2384	#else
  2385	
  2386	static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
size,
  2387					    const struct iommu_ops *iommu)
  2388	{
  2389		return false;
  2390	}
  2391	
  2392	static void arm_teardown_iommu_dma_ops(struct device *dev) { }
  2393	
  2394	#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
  2395	
  2396	void arch_iommu_detach_device(struct device *dev)
  2397	{
  2398	}
  2399	
  2400	#endif	/* CONFIG_ARM_DMA_USE_IOMMU */
  2401	
> 2402	static const struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
  2403	{
  2404		return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
  2405	}
  2406	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 33598 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20180427/3a09a331/attachment-0001.gz>