[PATCH 1/2] dma: call unconditionally to unmap_page and unmap_sg callbacks

[PATCH 1/2] dma: call unconditionally to unmap_page and unmap_sg callbacks

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Almost all users of ->map_page()/map_sg() callbacks implement
->unmap_page()/unmap_sg() callbacks too. One user which doesn't do it,
is dummy DMA ops interface, and the use of this interface is to fail
the operation and in such case, there won't be any call to
->unmap_page()/unmap_sg().

This patch removes the existence checks of ->unmap_page()/unmap_sg()
and calls to it directly to create symmetrical interface to
->map_page()/map_sg().

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 kernel/dma/mapping.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 81de84318ccc..6832fd6f0796 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -177,7 +177,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
 	if (dma_map_direct(dev, ops) ||
 	    arch_dma_unmap_page_direct(dev, addr + size))
 		dma_direct_unmap_page(dev, addr, size, dir, attrs);
-	else if (ops->unmap_page)
+	else
 		ops->unmap_page(dev, addr, size, dir, attrs);
 	debug_dma_unmap_page(dev, addr, size, dir);
 }
@@ -291,7 +291,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
 	if (dma_map_direct(dev, ops) ||
 	    arch_dma_unmap_sg_direct(dev, sg, nents))
 		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
-	else if (ops->unmap_sg)
+	else
 		ops->unmap_sg(dev, sg, nents, dir, attrs);
 }
 EXPORT_SYMBOL(dma_unmap_sg_attrs);
-- 
2.45.2

[PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Most of the IOMMU drivers are using the same DMA operations, which are
default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
to properly set them as a default with direct call without need to
perform function pointer dereference.

During system initialization, the IOMMU driver can set its own DMA and
in such case, the default DMA operations will be overridden.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 MAINTAINERS               |  1 +
 drivers/iommu/dma-iommu.c | 24 +++++++-------
 include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
 kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
 kernel/dma/mapping.c      |  9 +++---
 5 files changed, 134 insertions(+), 17 deletions(-)
 create mode 100644 include/linux/iommu-dma.h
 create mode 100644 kernel/dma/iommu.h

diff --git a/MAINTAINERS b/MAINTAINERS
index da5352dbd4f3..1e64be463da7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11544,6 +11544,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux.git
 F:	Documentation/devicetree/bindings/iommu/
 F:	Documentation/userspace-api/iommu.rst
 F:	drivers/iommu/
+F:	include/linux/iommu-dma.h
 F:	include/linux/iommu.h
 F:	include/linux/iova.h
 F:	include/linux/of_iommu.h
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 43520e7275cc..54e95792ed90 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -17,6 +17,7 @@
 #include <linux/gfp.h>
 #include <linux/huge_mm.h>
 #include <linux/iommu.h>
+#include <linux/iommu-dma.h>
 #include <linux/iova.h>
 #include <linux/irq.h>
 #include <linux/list_sort.h>
@@ -1134,9 +1135,9 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
 			arch_sync_dma_for_device(sg_phys(sg), sg->length, dir);
 }
 
-static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
-		unsigned long offset, size_t size, enum dma_data_direction dir,
-		unsigned long attrs)
+dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
+			      unsigned long offset, size_t size,
+			      enum dma_data_direction dir, unsigned long attrs)
 {
 	phys_addr_t phys = page_to_phys(page) + offset;
 	bool coherent = dev_is_dma_coherent(dev);
@@ -1194,8 +1195,9 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 	return iova;
 }
 
-static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
-		size_t size, enum dma_data_direction dir, unsigned long attrs)
+void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
+			  size_t size, enum dma_data_direction dir,
+			  unsigned long attrs)
 {
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	phys_addr_t phys;
@@ -1348,8 +1350,8 @@ static int iommu_dma_map_sg_swiotlb(struct device *dev, struct scatterlist *sg,
  * impedance-matching, to be able to hand off a suitably-aligned list,
  * but still preserve the original offsets and sizes for the caller.
  */
-static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
-		int nents, enum dma_data_direction dir, unsigned long attrs)
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+		     enum dma_data_direction dir, unsigned long attrs)
 {
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
@@ -1468,8 +1470,8 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	return ret;
 }
 
-static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
-		int nents, enum dma_data_direction dir, unsigned long attrs)
+void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
+			enum dma_data_direction dir, unsigned long attrs)
 {
 	dma_addr_t end = 0, start;
 	struct scatterlist *tmp;
@@ -1731,10 +1733,6 @@ static const struct dma_map_ops iommu_dma_ops = {
 	.free_noncontiguous	= iommu_dma_free_noncontiguous,
 	.mmap			= iommu_dma_mmap,
 	.get_sgtable		= iommu_dma_get_sgtable,
-	.map_page		= iommu_dma_map_page,
-	.unmap_page		= iommu_dma_unmap_page,
-	.map_sg			= iommu_dma_map_sg,
-	.unmap_sg		= iommu_dma_unmap_sg,
 	.sync_single_for_cpu	= iommu_dma_sync_single_for_cpu,
 	.sync_single_for_device	= iommu_dma_sync_single_for_device,
 	.sync_sg_for_cpu	= iommu_dma_sync_sg_for_cpu,
diff --git a/include/linux/iommu-dma.h b/include/linux/iommu-dma.h
new file mode 100644
index 000000000000..b42487bf8f8e
--- /dev/null
+++ b/include/linux/iommu-dma.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
+ *
+ * DMA operations that map physical memory through IOMMU.
+ */
+#ifndef _LINUX_IOMMU_DMA_H
+#define _LINUX_IOMMU_DMA_H
+
+#include <linux/dma-direction.h>
+
+#ifdef CONFIG_IOMMU_API
+dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
+			      unsigned long offset, size_t size,
+			      enum dma_data_direction dir, unsigned long attrs);
+void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
+			  size_t size, enum dma_data_direction dir,
+			  unsigned long attrs);
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+		     enum dma_data_direction dir, unsigned long attrs);
+void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
+			enum dma_data_direction dir, unsigned long attrs);
+#else
+static inline dma_addr_t iommu_dma_map_page(struct device *dev,
+					    struct page *page,
+					    unsigned long offset, size_t size,
+					    enum dma_data_direction dir,
+					    unsigned long attrs)
+{
+	return DMA_MAPPING_ERROR;
+}
+static inline void iommu_dma_unmap_page(struct device *dev,
+					dma_addr_t dma_handle, size_t size,
+					enum dma_data_direction dir,
+					unsigned long attrs)
+{
+}
+static inline int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+				   int nents, enum dma_data_direction dir,
+				   unsigned long attrs)
+{
+	return -EINVAL;
+}
+static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
+			       int nents, enum dma_data_direction dir,
+			       unsigned long attrs)
+{
+}
+#endif /* CONFIG_IOMMU_API */
+#endif /* _LINUX_IOMMU_DMA_H */
diff --git a/kernel/dma/iommu.h b/kernel/dma/iommu.h
new file mode 100644
index 000000000000..4abaea2dfc49
--- /dev/null
+++ b/kernel/dma/iommu.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
+ *
+ * DMA operations that map physical memory through IOMMU.
+ */
+#ifndef _KERNEL_DMA_IOMMU_H
+#define _KERNEL_DMA_IOMMU_H
+
+#include <linux/iommu-dma.h>
+
+static inline dma_addr_t dma_iommu_map_page(struct device *dev,
+					    struct page *page, size_t offset,
+					    size_t size,
+					    enum dma_data_direction dir,
+					    unsigned long attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (ops->map_page)
+		return ops->map_page(dev, page, offset, size, dir, attrs);
+
+	return iommu_dma_map_page(dev, page, offset, size, dir, attrs);
+}
+
+static inline void dma_iommu_unmap_page(struct device *dev, dma_addr_t addr,
+					size_t size,
+					enum dma_data_direction dir,
+					unsigned long attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (ops->unmap_page) {
+		ops->unmap_page(dev, addr, size, dir, attrs);
+		return;
+	}
+
+	iommu_dma_unmap_page(dev, addr, size, dir, attrs);
+}
+
+static inline int dma_iommu_map_sg(struct device *dev, struct scatterlist *sg,
+				   int nents, enum dma_data_direction dir,
+				   unsigned long attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (ops->map_sg)
+		return ops->map_sg(dev, sg, nents, dir, attrs);
+
+	return iommu_dma_map_sg(dev, sg, nents, dir, attrs);
+}
+
+static inline void dma_iommu_unmap_sg(struct device *dev,
+				      struct scatterlist *sg, int nents,
+				      enum dma_data_direction dir,
+				      unsigned long attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (ops->unmap_sg) {
+		ops->unmap_sg(dev, sg, nents, dir, attrs);
+		return;
+	}
+
+	iommu_dma_unmap_sg(dev, sg, nents, dir, attrs);
+}
+#endif /* _KERNEL_DMA_IOMMU_H */
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 6832fd6f0796..a9aa4036362a 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 #include "debug.h"
 #include "direct.h"
+#include "iommu.h"
 
 #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
 	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
@@ -160,7 +161,7 @@ dma_addr_t dma_map_page_attrs(struct device *dev, struct page *page,
 	    arch_dma_map_page_direct(dev, page_to_phys(page) + offset + size))
 		addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
 	else
-		addr = ops->map_page(dev, page, offset, size, dir, attrs);
+		addr = dma_iommu_map_page(dev, page, offset, size, dir, attrs);
 	kmsan_handle_dma(page, offset, size, dir);
 	debug_dma_map_page(dev, page, offset, size, dir, addr, attrs);
 
@@ -178,7 +179,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
 	    arch_dma_unmap_page_direct(dev, addr + size))
 		dma_direct_unmap_page(dev, addr, size, dir, attrs);
 	else
-		ops->unmap_page(dev, addr, size, dir, attrs);
+		dma_iommu_unmap_page(dev, addr, size, dir, attrs);
 	debug_dma_unmap_page(dev, addr, size, dir);
 }
 EXPORT_SYMBOL(dma_unmap_page_attrs);
@@ -198,7 +199,7 @@ static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
 	    arch_dma_map_sg_direct(dev, sg, nents))
 		ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
 	else
-		ents = ops->map_sg(dev, sg, nents, dir, attrs);
+		ents = dma_iommu_map_sg(dev, sg, nents, dir, attrs);
 
 	if (ents > 0) {
 		kmsan_handle_dma_sg(sg, nents, dir);
@@ -292,7 +293,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
 	    arch_dma_unmap_sg_direct(dev, sg, nents))
 		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
 	else
-		ops->unmap_sg(dev, sg, nents, dir, attrs);
+		dma_iommu_unmap_sg(dev, sg, nents, dir, attrs);
 }
 EXPORT_SYMBOL(dma_unmap_sg_attrs);
 
-- 
2.45.2

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Thu, Jul 11, 2024 at 01:38:55PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Most of the IOMMU drivers are using the same DMA operations, which are
> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> to properly set them as a default with direct call without need to
> perform function pointer dereference.
> 
> During system initialization, the IOMMU driver can set its own DMA and
> in such case, the default DMA operations will be overridden.
> 
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  MAINTAINERS               |  1 +
>  drivers/iommu/dma-iommu.c | 24 +++++++-------
>  include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
>  kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
>  kernel/dma/mapping.c      |  9 +++---
>  5 files changed, 134 insertions(+), 17 deletions(-)
>  create mode 100644 include/linux/iommu-dma.h
>  create mode 100644 kernel/dma/iommu.h

<...>

> +++ b/include/linux/iommu-dma.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + *
> + * DMA operations that map physical memory through IOMMU.
> + */
> +#ifndef _LINUX_IOMMU_DMA_H
> +#define _LINUX_IOMMU_DMA_H
> +
> +#include <linux/dma-direction.h>
> +
> +#ifdef CONFIG_IOMMU_API

This should be CONFIG_IOMMU_DMA.

diff --git a/include/linux/iommu-dma.h b/include/linux/iommu-dma.h
index b42487bf8f8e..bfdcc9d65daf 100644
--- a/include/linux/iommu-dma.h
+++ b/include/linux/iommu-dma.h
@@ -9,7 +9,7 @@

 #include <linux/dma-direction.h>

-#ifdef CONFIG_IOMMU_API
+#ifdef CONFIG_IOMMU_DMA
 dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
                              unsigned long offset, size_t size,
                              enum dma_data_direction dir, unsigned long attrs);
@@ -46,5 +46,5 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
                               unsigned long attrs)
 {
 }
-#endif /* CONFIG_IOMMU_API */
+#endif /* CONFIG_IOMMU_DMA */
 #endif /* _LINUX_IOMMU_DMA_H */

Thanks

Re: [PATCH 1/2] dma: call unconditionally to unmap_page and unmap_sg callbacks

[Robin Murphy]

On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Almost all users of ->map_page()/map_sg() callbacks implement
> ->unmap_page()/unmap_sg() callbacks too. One user which doesn't do it,
> is dummy DMA ops interface, and the use of this interface is to fail
> the operation and in such case, there won't be any call to
> ->unmap_page()/unmap_sg().
> 
> This patch removes the existence checks of ->unmap_page()/unmap_sg()
> and calls to it directly to create symmetrical interface to
> ->map_page()/map_sg().

Now that all the common cases have been mopped up by dma-direct, I'm 
inclined to agree that this seems reasonable. For sure there is code out 
there still abusing random DMA API calls as a cache maintenance 
interface because it thinks it knows better, but even that's going to be 
running on platforms where it expects unmap to have the desired effect 
anyway, so the chance of somehow ending up with the dummy ops and 
crashing seems sufficiently unlikely.

However, I'm a little wary of the static checker brigade noticing that 
and trying to "fix" it by reinstating these tests, so perhaps it's worth 
just adding unmaps to the dummy ops (with a WARN() in them) as well for 
the sake of cleanliness and avoidance of any doubt.

Thanks,
Robin.

> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>   kernel/dma/mapping.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 81de84318ccc..6832fd6f0796 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -177,7 +177,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
>   	if (dma_map_direct(dev, ops) ||
>   	    arch_dma_unmap_page_direct(dev, addr + size))
>   		dma_direct_unmap_page(dev, addr, size, dir, attrs);
> -	else if (ops->unmap_page)
> +	else
>   		ops->unmap_page(dev, addr, size, dir, attrs);
>   	debug_dma_unmap_page(dev, addr, size, dir);
>   }
> @@ -291,7 +291,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>   	if (dma_map_direct(dev, ops) ||
>   	    arch_dma_unmap_sg_direct(dev, sg, nents))
>   		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> -	else if (ops->unmap_sg)
> +	else
>   		ops->unmap_sg(dev, sg, nents, dir, attrs);
>   }
>   EXPORT_SYMBOL(dma_unmap_sg_attrs);

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Easwar Hariharan]

On 7/11/2024 3:38 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Most of the IOMMU drivers are using the same DMA operations, which are
> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense

s/iomem/iommu?

> to properly set them as a default with direct call without need to
> perform function pointer dereference.
> 
> During system initialization, the IOMMU driver can set its own DMA and
> in such case, the default DMA operations will be overridden.
> 
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  MAINTAINERS               |  1 +
>  drivers/iommu/dma-iommu.c | 24 +++++++-------
>  include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
>  kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
>  kernel/dma/mapping.c      |  9 +++---
>  5 files changed, 134 insertions(+), 17 deletions(-)
>  create mode 100644 include/linux/iommu-dma.h
>  create mode 100644 kernel/dma/iommu.h
> 

<snip>

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Robin Murphy]

On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Most of the IOMMU drivers are using the same DMA operations, which are
> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> to properly set them as a default with direct call without need to
> perform function pointer dereference.
> 
> During system initialization, the IOMMU driver can set its own DMA and
> in such case, the default DMA operations will be overridden.

I'm going to guess you don't actually mean "IOMMU drivers" in the usual 
sense of drivers/iommu/, but rather "arch DMA ops (which often, but not 
always, involve some sort of IOMMU)."

If so, I'd much rather see this done properly, i.e. hook everything up 
similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern" 
dma-direct/iommu-dma architectures entirely. Furthermore the 
implementation here isn't right - not only is it not conceptually 
appropriate to make iommu-dma responsibile for proxying random arch DMA 
ops, but in practial terms it's just plain broken, since the 
architectures which still have their own DMA ops also don't use 
iommu-dma, so this is essentially disabling the entire streaming DMA API 
on ARM/PowerPC/etc.

Thanks,
Robin.

> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>   MAINTAINERS               |  1 +
>   drivers/iommu/dma-iommu.c | 24 +++++++-------
>   include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
>   kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
>   kernel/dma/mapping.c      |  9 +++---
>   5 files changed, 134 insertions(+), 17 deletions(-)
>   create mode 100644 include/linux/iommu-dma.h
>   create mode 100644 kernel/dma/iommu.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index da5352dbd4f3..1e64be463da7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11544,6 +11544,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux.git
>   F:	Documentation/devicetree/bindings/iommu/
>   F:	Documentation/userspace-api/iommu.rst
>   F:	drivers/iommu/
> +F:	include/linux/iommu-dma.h
>   F:	include/linux/iommu.h
>   F:	include/linux/iova.h
>   F:	include/linux/of_iommu.h
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 43520e7275cc..54e95792ed90 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -17,6 +17,7 @@
>   #include <linux/gfp.h>
>   #include <linux/huge_mm.h>
>   #include <linux/iommu.h>
> +#include <linux/iommu-dma.h>
>   #include <linux/iova.h>
>   #include <linux/irq.h>
>   #include <linux/list_sort.h>
> @@ -1134,9 +1135,9 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
>   			arch_sync_dma_for_device(sg_phys(sg), sg->length, dir);
>   }
>   
> -static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> -		unsigned long offset, size_t size, enum dma_data_direction dir,
> -		unsigned long attrs)
> +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> +			      unsigned long offset, size_t size,
> +			      enum dma_data_direction dir, unsigned long attrs)
>   {
>   	phys_addr_t phys = page_to_phys(page) + offset;
>   	bool coherent = dev_is_dma_coherent(dev);
> @@ -1194,8 +1195,9 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
>   	return iova;
>   }
>   
> -static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> -		size_t size, enum dma_data_direction dir, unsigned long attrs)
> +void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> +			  size_t size, enum dma_data_direction dir,
> +			  unsigned long attrs)
>   {
>   	struct iommu_domain *domain = iommu_get_dma_domain(dev);
>   	phys_addr_t phys;
> @@ -1348,8 +1350,8 @@ static int iommu_dma_map_sg_swiotlb(struct device *dev, struct scatterlist *sg,
>    * impedance-matching, to be able to hand off a suitably-aligned list,
>    * but still preserve the original offsets and sizes for the caller.
>    */
> -static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> -		int nents, enum dma_data_direction dir, unsigned long attrs)
> +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		     enum dma_data_direction dir, unsigned long attrs)
>   {
>   	struct iommu_domain *domain = iommu_get_dma_domain(dev);
>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
> @@ -1468,8 +1470,8 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	return ret;
>   }
>   
> -static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> -		int nents, enum dma_data_direction dir, unsigned long attrs)
> +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> +			enum dma_data_direction dir, unsigned long attrs)
>   {
>   	dma_addr_t end = 0, start;
>   	struct scatterlist *tmp;
> @@ -1731,10 +1733,6 @@ static const struct dma_map_ops iommu_dma_ops = {
>   	.free_noncontiguous	= iommu_dma_free_noncontiguous,
>   	.mmap			= iommu_dma_mmap,
>   	.get_sgtable		= iommu_dma_get_sgtable,
> -	.map_page		= iommu_dma_map_page,
> -	.unmap_page		= iommu_dma_unmap_page,
> -	.map_sg			= iommu_dma_map_sg,
> -	.unmap_sg		= iommu_dma_unmap_sg,
>   	.sync_single_for_cpu	= iommu_dma_sync_single_for_cpu,
>   	.sync_single_for_device	= iommu_dma_sync_single_for_device,
>   	.sync_sg_for_cpu	= iommu_dma_sync_sg_for_cpu,
> diff --git a/include/linux/iommu-dma.h b/include/linux/iommu-dma.h
> new file mode 100644
> index 000000000000..b42487bf8f8e
> --- /dev/null
> +++ b/include/linux/iommu-dma.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + *
> + * DMA operations that map physical memory through IOMMU.
> + */
> +#ifndef _LINUX_IOMMU_DMA_H
> +#define _LINUX_IOMMU_DMA_H
> +
> +#include <linux/dma-direction.h>
> +
> +#ifdef CONFIG_IOMMU_API
> +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> +			      unsigned long offset, size_t size,
> +			      enum dma_data_direction dir, unsigned long attrs);
> +void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> +			  size_t size, enum dma_data_direction dir,
> +			  unsigned long attrs);
> +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		     enum dma_data_direction dir, unsigned long attrs);
> +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> +			enum dma_data_direction dir, unsigned long attrs);
> +#else
> +static inline dma_addr_t iommu_dma_map_page(struct device *dev,
> +					    struct page *page,
> +					    unsigned long offset, size_t size,
> +					    enum dma_data_direction dir,
> +					    unsigned long attrs)
> +{
> +	return DMA_MAPPING_ERROR;
> +}
> +static inline void iommu_dma_unmap_page(struct device *dev,
> +					dma_addr_t dma_handle, size_t size,
> +					enum dma_data_direction dir,
> +					unsigned long attrs)
> +{
> +}
> +static inline int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> +				   int nents, enum dma_data_direction dir,
> +				   unsigned long attrs)
> +{
> +	return -EINVAL;
> +}
> +static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> +			       int nents, enum dma_data_direction dir,
> +			       unsigned long attrs)
> +{
> +}
> +#endif /* CONFIG_IOMMU_API */
> +#endif /* _LINUX_IOMMU_DMA_H */
> diff --git a/kernel/dma/iommu.h b/kernel/dma/iommu.h
> new file mode 100644
> index 000000000000..4abaea2dfc49
> --- /dev/null
> +++ b/kernel/dma/iommu.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + *
> + * DMA operations that map physical memory through IOMMU.
> + */
> +#ifndef _KERNEL_DMA_IOMMU_H
> +#define _KERNEL_DMA_IOMMU_H
> +
> +#include <linux/iommu-dma.h>
> +
> +static inline dma_addr_t dma_iommu_map_page(struct device *dev,
> +					    struct page *page, size_t offset,
> +					    size_t size,
> +					    enum dma_data_direction dir,
> +					    unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->map_page)
> +		return ops->map_page(dev, page, offset, size, dir, attrs);
> +
> +	return iommu_dma_map_page(dev, page, offset, size, dir, attrs);
> +}
> +
> +static inline void dma_iommu_unmap_page(struct device *dev, dma_addr_t addr,
> +					size_t size,
> +					enum dma_data_direction dir,
> +					unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->unmap_page) {
> +		ops->unmap_page(dev, addr, size, dir, attrs);
> +		return;
> +	}
> +
> +	iommu_dma_unmap_page(dev, addr, size, dir, attrs);
> +}
> +
> +static inline int dma_iommu_map_sg(struct device *dev, struct scatterlist *sg,
> +				   int nents, enum dma_data_direction dir,
> +				   unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->map_sg)
> +		return ops->map_sg(dev, sg, nents, dir, attrs);
> +
> +	return iommu_dma_map_sg(dev, sg, nents, dir, attrs);
> +}
> +
> +static inline void dma_iommu_unmap_sg(struct device *dev,
> +				      struct scatterlist *sg, int nents,
> +				      enum dma_data_direction dir,
> +				      unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->unmap_sg) {
> +		ops->unmap_sg(dev, sg, nents, dir, attrs);
> +		return;
> +	}
> +
> +	iommu_dma_unmap_sg(dev, sg, nents, dir, attrs);
> +}
> +#endif /* _KERNEL_DMA_IOMMU_H */
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 6832fd6f0796..a9aa4036362a 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -16,6 +16,7 @@
>   #include <linux/vmalloc.h>
>   #include "debug.h"
>   #include "direct.h"
> +#include "iommu.h"
>   
>   #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
>   	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> @@ -160,7 +161,7 @@ dma_addr_t dma_map_page_attrs(struct device *dev, struct page *page,
>   	    arch_dma_map_page_direct(dev, page_to_phys(page) + offset + size))
>   		addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
>   	else
> -		addr = ops->map_page(dev, page, offset, size, dir, attrs);
> +		addr = dma_iommu_map_page(dev, page, offset, size, dir, attrs);
>   	kmsan_handle_dma(page, offset, size, dir);
>   	debug_dma_map_page(dev, page, offset, size, dir, addr, attrs);
>   
> @@ -178,7 +179,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
>   	    arch_dma_unmap_page_direct(dev, addr + size))
>   		dma_direct_unmap_page(dev, addr, size, dir, attrs);
>   	else
> -		ops->unmap_page(dev, addr, size, dir, attrs);
> +		dma_iommu_unmap_page(dev, addr, size, dir, attrs);
>   	debug_dma_unmap_page(dev, addr, size, dir);
>   }
>   EXPORT_SYMBOL(dma_unmap_page_attrs);
> @@ -198,7 +199,7 @@ static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>   	    arch_dma_map_sg_direct(dev, sg, nents))
>   		ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
>   	else
> -		ents = ops->map_sg(dev, sg, nents, dir, attrs);
> +		ents = dma_iommu_map_sg(dev, sg, nents, dir, attrs);
>   
>   	if (ents > 0) {
>   		kmsan_handle_dma_sg(sg, nents, dir);
> @@ -292,7 +293,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>   	    arch_dma_unmap_sg_direct(dev, sg, nents))
>   		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
>   	else
> -		ops->unmap_sg(dev, sg, nents, dir, attrs);
> +		dma_iommu_unmap_sg(dev, sg, nents, dir, attrs);
>   }
>   EXPORT_SYMBOL(dma_unmap_sg_attrs);
>   

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Thu, Jul 11, 2024 at 11:17:28AM -0700, Easwar Hariharan wrote:
> On 7/11/2024 3:38 AM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Most of the IOMMU drivers are using the same DMA operations, which are
> > default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> 
> s/iomem/iommu?

Thanks, will fix it.

> 
> > to properly set them as a default with direct call without need to
> > perform function pointer dereference.
> > 
> > During system initialization, the IOMMU driver can set its own DMA and
> > in such case, the default DMA operations will be overridden.
> > 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  MAINTAINERS               |  1 +
> >  drivers/iommu/dma-iommu.c | 24 +++++++-------
> >  include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
> >  kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
> >  kernel/dma/mapping.c      |  9 +++---
> >  5 files changed, 134 insertions(+), 17 deletions(-)
> >  create mode 100644 include/linux/iommu-dma.h
> >  create mode 100644 kernel/dma/iommu.h
> > 
> 
> <snip>

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
> On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Most of the IOMMU drivers are using the same DMA operations, which are
> > default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> > to properly set them as a default with direct call without need to
> > perform function pointer dereference.
> > 
> > During system initialization, the IOMMU driver can set its own DMA and
> > in such case, the default DMA operations will be overridden.
> 
> I'm going to guess you don't actually mean "IOMMU drivers" in the usual
> sense of drivers/iommu/, but rather "arch DMA ops (which often, but not
> always, involve some sort of IOMMU)."

Yes, you are right. I used word "drivers" as a general term to describe
everything that implements their own ->map_page() callback.

> 
> If so, I'd much rather see this done properly, i.e. hook everything up
> similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern"
> dma-direct/iommu-dma architectures entirely. Furthermore the implementation
> here isn't right - not only is it not conceptually appropriate to make
> iommu-dma responsibile for proxying random arch DMA ops, but in practial
> terms it's just plain broken, since the architectures which still have their
> own DMA ops also don't use iommu-dma, so this is essentially disabling the
> entire streaming DMA API on ARM/PowerPC/etc.

Sorry but I'm not sure that I understood your reply. Can you please clarify
what does it mean broken in this context?

All archs have one of the following options:
1. No DMA ops -> for them dma_map_direct() will return True and they
never will enter into IOMMU path.
2. Have DMA ops but without .map_page() -> they will use default IOMMU
3. Have DMA ops with .map_page() -> they will use their own implementation

Thanks

> 
> Thanks,
> Robin.
> 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >   MAINTAINERS               |  1 +
> >   drivers/iommu/dma-iommu.c | 24 +++++++-------
> >   include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
> >   kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
> >   kernel/dma/mapping.c      |  9 +++---
> >   5 files changed, 134 insertions(+), 17 deletions(-)
> >   create mode 100644 include/linux/iommu-dma.h
> >   create mode 100644 kernel/dma/iommu.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index da5352dbd4f3..1e64be463da7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11544,6 +11544,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux.git
> >   F:	Documentation/devicetree/bindings/iommu/
> >   F:	Documentation/userspace-api/iommu.rst
> >   F:	drivers/iommu/
> > +F:	include/linux/iommu-dma.h
> >   F:	include/linux/iommu.h
> >   F:	include/linux/iova.h
> >   F:	include/linux/of_iommu.h
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 43520e7275cc..54e95792ed90 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -17,6 +17,7 @@
> >   #include <linux/gfp.h>
> >   #include <linux/huge_mm.h>
> >   #include <linux/iommu.h>
> > +#include <linux/iommu-dma.h>
> >   #include <linux/iova.h>
> >   #include <linux/irq.h>
> >   #include <linux/list_sort.h>
> > @@ -1134,9 +1135,9 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
> >   			arch_sync_dma_for_device(sg_phys(sg), sg->length, dir);
> >   }
> > -static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> > -		unsigned long offset, size_t size, enum dma_data_direction dir,
> > -		unsigned long attrs)
> > +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> > +			      unsigned long offset, size_t size,
> > +			      enum dma_data_direction dir, unsigned long attrs)
> >   {
> >   	phys_addr_t phys = page_to_phys(page) + offset;
> >   	bool coherent = dev_is_dma_coherent(dev);
> > @@ -1194,8 +1195,9 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> >   	return iova;
> >   }
> > -static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> > -		size_t size, enum dma_data_direction dir, unsigned long attrs)
> > +void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> > +			  size_t size, enum dma_data_direction dir,
> > +			  unsigned long attrs)
> >   {
> >   	struct iommu_domain *domain = iommu_get_dma_domain(dev);
> >   	phys_addr_t phys;
> > @@ -1348,8 +1350,8 @@ static int iommu_dma_map_sg_swiotlb(struct device *dev, struct scatterlist *sg,
> >    * impedance-matching, to be able to hand off a suitably-aligned list,
> >    * but still preserve the original offsets and sizes for the caller.
> >    */
> > -static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> > -		int nents, enum dma_data_direction dir, unsigned long attrs)
> > +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> > +		     enum dma_data_direction dir, unsigned long attrs)
> >   {
> >   	struct iommu_domain *domain = iommu_get_dma_domain(dev);
> >   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
> > @@ -1468,8 +1470,8 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> >   	return ret;
> >   }
> > -static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> > -		int nents, enum dma_data_direction dir, unsigned long attrs)
> > +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> > +			enum dma_data_direction dir, unsigned long attrs)
> >   {
> >   	dma_addr_t end = 0, start;
> >   	struct scatterlist *tmp;
> > @@ -1731,10 +1733,6 @@ static const struct dma_map_ops iommu_dma_ops = {
> >   	.free_noncontiguous	= iommu_dma_free_noncontiguous,
> >   	.mmap			= iommu_dma_mmap,
> >   	.get_sgtable		= iommu_dma_get_sgtable,
> > -	.map_page		= iommu_dma_map_page,
> > -	.unmap_page		= iommu_dma_unmap_page,
> > -	.map_sg			= iommu_dma_map_sg,
> > -	.unmap_sg		= iommu_dma_unmap_sg,
> >   	.sync_single_for_cpu	= iommu_dma_sync_single_for_cpu,
> >   	.sync_single_for_device	= iommu_dma_sync_single_for_device,
> >   	.sync_sg_for_cpu	= iommu_dma_sync_sg_for_cpu,
> > diff --git a/include/linux/iommu-dma.h b/include/linux/iommu-dma.h
> > new file mode 100644
> > index 000000000000..b42487bf8f8e
> > --- /dev/null
> > +++ b/include/linux/iommu-dma.h
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> > + *
> > + * DMA operations that map physical memory through IOMMU.
> > + */
> > +#ifndef _LINUX_IOMMU_DMA_H
> > +#define _LINUX_IOMMU_DMA_H
> > +
> > +#include <linux/dma-direction.h>
> > +
> > +#ifdef CONFIG_IOMMU_API
> > +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> > +			      unsigned long offset, size_t size,
> > +			      enum dma_data_direction dir, unsigned long attrs);
> > +void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> > +			  size_t size, enum dma_data_direction dir,
> > +			  unsigned long attrs);
> > +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> > +		     enum dma_data_direction dir, unsigned long attrs);
> > +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> > +			enum dma_data_direction dir, unsigned long attrs);
> > +#else
> > +static inline dma_addr_t iommu_dma_map_page(struct device *dev,
> > +					    struct page *page,
> > +					    unsigned long offset, size_t size,
> > +					    enum dma_data_direction dir,
> > +					    unsigned long attrs)
> > +{
> > +	return DMA_MAPPING_ERROR;
> > +}
> > +static inline void iommu_dma_unmap_page(struct device *dev,
> > +					dma_addr_t dma_handle, size_t size,
> > +					enum dma_data_direction dir,
> > +					unsigned long attrs)
> > +{
> > +}
> > +static inline int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> > +				   int nents, enum dma_data_direction dir,
> > +				   unsigned long attrs)
> > +{
> > +	return -EINVAL;
> > +}
> > +static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> > +			       int nents, enum dma_data_direction dir,
> > +			       unsigned long attrs)
> > +{
> > +}
> > +#endif /* CONFIG_IOMMU_API */
> > +#endif /* _LINUX_IOMMU_DMA_H */
> > diff --git a/kernel/dma/iommu.h b/kernel/dma/iommu.h
> > new file mode 100644
> > index 000000000000..4abaea2dfc49
> > --- /dev/null
> > +++ b/kernel/dma/iommu.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> > + *
> > + * DMA operations that map physical memory through IOMMU.
> > + */
> > +#ifndef _KERNEL_DMA_IOMMU_H
> > +#define _KERNEL_DMA_IOMMU_H
> > +
> > +#include <linux/iommu-dma.h>
> > +
> > +static inline dma_addr_t dma_iommu_map_page(struct device *dev,
> > +					    struct page *page, size_t offset,
> > +					    size_t size,
> > +					    enum dma_data_direction dir,
> > +					    unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->map_page)
> > +		return ops->map_page(dev, page, offset, size, dir, attrs);
> > +
> > +	return iommu_dma_map_page(dev, page, offset, size, dir, attrs);
> > +}
> > +
> > +static inline void dma_iommu_unmap_page(struct device *dev, dma_addr_t addr,
> > +					size_t size,
> > +					enum dma_data_direction dir,
> > +					unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->unmap_page) {
> > +		ops->unmap_page(dev, addr, size, dir, attrs);
> > +		return;
> > +	}
> > +
> > +	iommu_dma_unmap_page(dev, addr, size, dir, attrs);
> > +}
> > +
> > +static inline int dma_iommu_map_sg(struct device *dev, struct scatterlist *sg,
> > +				   int nents, enum dma_data_direction dir,
> > +				   unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->map_sg)
> > +		return ops->map_sg(dev, sg, nents, dir, attrs);
> > +
> > +	return iommu_dma_map_sg(dev, sg, nents, dir, attrs);
> > +}
> > +
> > +static inline void dma_iommu_unmap_sg(struct device *dev,
> > +				      struct scatterlist *sg, int nents,
> > +				      enum dma_data_direction dir,
> > +				      unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->unmap_sg) {
> > +		ops->unmap_sg(dev, sg, nents, dir, attrs);
> > +		return;
> > +	}
> > +
> > +	iommu_dma_unmap_sg(dev, sg, nents, dir, attrs);
> > +}
> > +#endif /* _KERNEL_DMA_IOMMU_H */
> > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> > index 6832fd6f0796..a9aa4036362a 100644
> > --- a/kernel/dma/mapping.c
> > +++ b/kernel/dma/mapping.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/vmalloc.h>
> >   #include "debug.h"
> >   #include "direct.h"
> > +#include "iommu.h"
> >   #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
> >   	defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
> > @@ -160,7 +161,7 @@ dma_addr_t dma_map_page_attrs(struct device *dev, struct page *page,
> >   	    arch_dma_map_page_direct(dev, page_to_phys(page) + offset + size))
> >   		addr = dma_direct_map_page(dev, page, offset, size, dir, attrs);
> >   	else
> > -		addr = ops->map_page(dev, page, offset, size, dir, attrs);
> > +		addr = dma_iommu_map_page(dev, page, offset, size, dir, attrs);
> >   	kmsan_handle_dma(page, offset, size, dir);
> >   	debug_dma_map_page(dev, page, offset, size, dir, addr, attrs);
> > @@ -178,7 +179,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
> >   	    arch_dma_unmap_page_direct(dev, addr + size))
> >   		dma_direct_unmap_page(dev, addr, size, dir, attrs);
> >   	else
> > -		ops->unmap_page(dev, addr, size, dir, attrs);
> > +		dma_iommu_unmap_page(dev, addr, size, dir, attrs);
> >   	debug_dma_unmap_page(dev, addr, size, dir);
> >   }
> >   EXPORT_SYMBOL(dma_unmap_page_attrs);
> > @@ -198,7 +199,7 @@ static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
> >   	    arch_dma_map_sg_direct(dev, sg, nents))
> >   		ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
> >   	else
> > -		ents = ops->map_sg(dev, sg, nents, dir, attrs);
> > +		ents = dma_iommu_map_sg(dev, sg, nents, dir, attrs);
> >   	if (ents > 0) {
> >   		kmsan_handle_dma_sg(sg, nents, dir);
> > @@ -292,7 +293,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
> >   	    arch_dma_unmap_sg_direct(dev, sg, nents))
> >   		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> >   	else
> > -		ops->unmap_sg(dev, sg, nents, dir, attrs);
> > +		dma_iommu_unmap_sg(dev, sg, nents, dir, attrs);
> >   }
> >   EXPORT_SYMBOL(dma_unmap_sg_attrs);

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Robin Murphy]

On 2024-07-11 7:57 pm, Leon Romanovsky wrote:
> On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
>> On 11/07/2024 11:38 am, Leon Romanovsky wrote:
>>> From: Leon Romanovsky <leonro@nvidia.com>
>>>
>>> Most of the IOMMU drivers are using the same DMA operations, which are
>>> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
>>> to properly set them as a default with direct call without need to
>>> perform function pointer dereference.
>>>
>>> During system initialization, the IOMMU driver can set its own DMA and
>>> in such case, the default DMA operations will be overridden.
>>
>> I'm going to guess you don't actually mean "IOMMU drivers" in the usual
>> sense of drivers/iommu/, but rather "arch DMA ops (which often, but not
>> always, involve some sort of IOMMU)."
> 
> Yes, you are right. I used word "drivers" as a general term to describe
> everything that implements their own ->map_page() callback.
> 
>>
>> If so, I'd much rather see this done properly, i.e. hook everything up
>> similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern"
>> dma-direct/iommu-dma architectures entirely. Furthermore the implementation
>> here isn't right - not only is it not conceptually appropriate to make
>> iommu-dma responsibile for proxying random arch DMA ops, but in practial
>> terms it's just plain broken, since the architectures which still have their
>> own DMA ops also don't use iommu-dma, so this is essentially disabling the
>> entire streaming DMA API on ARM/PowerPC/etc.
> 
> Sorry but I'm not sure that I understood your reply. Can you please clarify
> what does it mean broken in this context?
> 
> All archs have one of the following options:
> 1. No DMA ops -> for them dma_map_direct() will return True and they
> never will enter into IOMMU path.
> 2. Have DMA ops but without .map_page() -> they will use default IOMMU
> 3. Have DMA ops with .map_page() -> they will use their own implementation

Urgh, sorry, let that instead be a lesson in not adding needless layers 
of indirection that are named as confusingly as possible, then. Seems I 
saw stubs plus static inline wrappers, managed to misread dma_iommu_* 
vs. iommu_dma_*, and jump to the wrong conclusion of what was stubbed 
out, not least since that was the only interpretation in which adding an 
extra layer of inline wrappers would seem necessary in the first place. 
If these dma_iommu_* functions serve no purpose other than to make the 
code even more of a maze of twisty little passages, all alike, then 
please just feed them to a grue instead.

Thanks,
Robin.

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Christoph Hellwig]

On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
> If so, I'd much rather see this done properly, i.e. hook everything up 
> similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern" 
> dma-direct/iommu-dma architectures entirely.

Yes.  That is much better than doing hacks based on missing ops.

Note that the media subsystems just added another abuse of dma_map_ops
in drivers/ in addition to the existing vdpa one that we'll need to
kill again to fully get rid of dma_map_ops for common setups (in
addition to merging swiotlb-xen into main swiotlb finally).

Re: [PATCH 1/2] dma: call unconditionally to unmap_page and unmap_sg callbacks

[Leon Romanovsky]

On Thu, Jul 11, 2024 at 07:02:39PM +0100, Robin Murphy wrote:
> On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Almost all users of ->map_page()/map_sg() callbacks implement
> > ->unmap_page()/unmap_sg() callbacks too. One user which doesn't do it,
> > is dummy DMA ops interface, and the use of this interface is to fail
> > the operation and in such case, there won't be any call to
> > ->unmap_page()/unmap_sg().
> > 
> > This patch removes the existence checks of ->unmap_page()/unmap_sg()
> > and calls to it directly to create symmetrical interface to
> > ->map_page()/map_sg().
> 
> Now that all the common cases have been mopped up by dma-direct, I'm
> inclined to agree that this seems reasonable. For sure there is code out
> there still abusing random DMA API calls as a cache maintenance interface
> because it thinks it knows better, but even that's going to be running on
> platforms where it expects unmap to have the desired effect anyway, so the
> chance of somehow ending up with the dummy ops and crashing seems
> sufficiently unlikely.
> 
> However, I'm a little wary of the static checker brigade noticing that and
> trying to "fix" it by reinstating these tests, so perhaps it's worth just
> adding unmaps to the dummy ops (with a WARN() in them) as well for the sake
> of cleanliness and avoidance of any doubt.

I'll add, however the plan is to get rid of dummy ops in the near future.

Thanks

> 
> Thanks,
> Robin.
> 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >   kernel/dma/mapping.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> > index 81de84318ccc..6832fd6f0796 100644
> > --- a/kernel/dma/mapping.c
> > +++ b/kernel/dma/mapping.c
> > @@ -177,7 +177,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
> >   	if (dma_map_direct(dev, ops) ||
> >   	    arch_dma_unmap_page_direct(dev, addr + size))
> >   		dma_direct_unmap_page(dev, addr, size, dir, attrs);
> > -	else if (ops->unmap_page)
> > +	else
> >   		ops->unmap_page(dev, addr, size, dir, attrs);
> >   	debug_dma_unmap_page(dev, addr, size, dir);
> >   }
> > @@ -291,7 +291,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
> >   	if (dma_map_direct(dev, ops) ||
> >   	    arch_dma_unmap_sg_direct(dev, sg, nents))
> >   		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> > -	else if (ops->unmap_sg)
> > +	else
> >   		ops->unmap_sg(dev, sg, nents, dir, attrs);
> >   }
> >   EXPORT_SYMBOL(dma_unmap_sg_attrs);

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Thu, Jul 11, 2024 at 09:08:49PM +0100, Robin Murphy wrote:
> On 2024-07-11 7:57 pm, Leon Romanovsky wrote:
> > On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
> > > On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > 
> > > > Most of the IOMMU drivers are using the same DMA operations, which are
> > > > default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> > > > to properly set them as a default with direct call without need to
> > > > perform function pointer dereference.
> > > > 
> > > > During system initialization, the IOMMU driver can set its own DMA and
> > > > in such case, the default DMA operations will be overridden.
> > > 
> > > I'm going to guess you don't actually mean "IOMMU drivers" in the usual
> > > sense of drivers/iommu/, but rather "arch DMA ops (which often, but not
> > > always, involve some sort of IOMMU)."
> > 
> > Yes, you are right. I used word "drivers" as a general term to describe
> > everything that implements their own ->map_page() callback.
> > 
> > > 
> > > If so, I'd much rather see this done properly, i.e. hook everything up
> > > similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern"
> > > dma-direct/iommu-dma architectures entirely. Furthermore the implementation
> > > here isn't right - not only is it not conceptually appropriate to make
> > > iommu-dma responsibile for proxying random arch DMA ops, but in practial
> > > terms it's just plain broken, since the architectures which still have their
> > > own DMA ops also don't use iommu-dma, so this is essentially disabling the
> > > entire streaming DMA API on ARM/PowerPC/etc.
> > 
> > Sorry but I'm not sure that I understood your reply. Can you please clarify
> > what does it mean broken in this context?
> > 
> > All archs have one of the following options:
> > 1. No DMA ops -> for them dma_map_direct() will return True and they
> > never will enter into IOMMU path.
> > 2. Have DMA ops but without .map_page() -> they will use default IOMMU
> > 3. Have DMA ops with .map_page() -> they will use their own implementation
> 
> Urgh, sorry, let that instead be a lesson in not adding needless layers of
> indirection that are named as confusingly as possible, then. Seems I saw
> stubs plus static inline wrappers, managed to misread dma_iommu_* vs.
> iommu_dma_*, and jump to the wrong conclusion of what was stubbed out, not
> least since that was the only interpretation in which adding an extra layer
> of inline wrappers would seem necessary in the first place. If these
> dma_iommu_* functions serve no purpose other than to make the code even more
> of a maze of twisty little passages, all alike, then please just feed them
> to a grue instead.

This is done to keep layering similar to existing in DMA subsystem. We
have special files and calls to dma-direct, it looks natural to have
special files and call to dma-iommu. It is not nice to call to drivers/iommu
from kernel/dma/mapping.c

Thanks

> 
> Thanks,
> Robin.

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Fri, Jul 12, 2024 at 06:49:37AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
> > If so, I'd much rather see this done properly, i.e. hook everything up 
> > similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern" 
> > dma-direct/iommu-dma architectures entirely.
> 
> Yes.  That is much better than doing hacks based on missing ops.

There is nothing wrong to do conversion to iommu direct in the steps.
The first step is to add the dma-iommu calls with if (!..) checks,
so everyone will get better performance right now. The second step
is to remove the if (!..) checks together with dummy ops.

Thanks

> 
> Note that the media subsystems just added another abuse of dma_map_ops
> in drivers/ in addition to the existing vdpa one that we'll need to
> kill again to fully get rid of dma_map_ops for common setups (in
> addition to merging swiotlb-xen into main swiotlb finally).
> 

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Robin Murphy]

On 12/07/2024 6:50 am, Leon Romanovsky wrote:
> On Thu, Jul 11, 2024 at 09:08:49PM +0100, Robin Murphy wrote:
>> On 2024-07-11 7:57 pm, Leon Romanovsky wrote:
>>> On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
>>>> On 11/07/2024 11:38 am, Leon Romanovsky wrote:
>>>>> From: Leon Romanovsky <leonro@nvidia.com>
>>>>>
>>>>> Most of the IOMMU drivers are using the same DMA operations, which are
>>>>> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
>>>>> to properly set them as a default with direct call without need to
>>>>> perform function pointer dereference.
>>>>>
>>>>> During system initialization, the IOMMU driver can set its own DMA and
>>>>> in such case, the default DMA operations will be overridden.
>>>>
>>>> I'm going to guess you don't actually mean "IOMMU drivers" in the usual
>>>> sense of drivers/iommu/, but rather "arch DMA ops (which often, but not
>>>> always, involve some sort of IOMMU)."
>>>
>>> Yes, you are right. I used word "drivers" as a general term to describe
>>> everything that implements their own ->map_page() callback.
>>>
>>>>
>>>> If so, I'd much rather see this done properly, i.e. hook everything up
>>>> similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern"
>>>> dma-direct/iommu-dma architectures entirely. Furthermore the implementation
>>>> here isn't right - not only is it not conceptually appropriate to make
>>>> iommu-dma responsibile for proxying random arch DMA ops, but in practial
>>>> terms it's just plain broken, since the architectures which still have their
>>>> own DMA ops also don't use iommu-dma, so this is essentially disabling the
>>>> entire streaming DMA API on ARM/PowerPC/etc.
>>>
>>> Sorry but I'm not sure that I understood your reply. Can you please clarify
>>> what does it mean broken in this context?
>>>
>>> All archs have one of the following options:
>>> 1. No DMA ops -> for them dma_map_direct() will return True and they
>>> never will enter into IOMMU path.
>>> 2. Have DMA ops but without .map_page() -> they will use default IOMMU
>>> 3. Have DMA ops with .map_page() -> they will use their own implementation
>>
>> Urgh, sorry, let that instead be a lesson in not adding needless layers of
>> indirection that are named as confusingly as possible, then. Seems I saw
>> stubs plus static inline wrappers, managed to misread dma_iommu_* vs.
>> iommu_dma_*, and jump to the wrong conclusion of what was stubbed out, not
>> least since that was the only interpretation in which adding an extra layer
>> of inline wrappers would seem necessary in the first place. If these
>> dma_iommu_* functions serve no purpose other than to make the code even more
>> of a maze of twisty little passages, all alike, then please just feed them
>> to a grue instead.
> 
> This is done to keep layering similar to existing in DMA subsystem. We
> have special files and calls to dma-direct, it looks natural to have
> special files and call to dma-iommu. It is not nice to call to drivers/iommu
> from kernel/dma/mapping.c

That's where I firmly disagree. In the DMA API aspect, iommu-dma is 
exactly a peer of dma-direct, however it lives in drivers/iommu for 
practical reasons because it's more closely coupled to IOMMU API 
internals in all other aspects of its implementation. TBH what I'd 
really like is a private interface between mapping.c and dma-iommu.c 
which doesn't expose internal details via include/linux/ at all, but 
*that's* a level that I think people may rightfully start to object to.

Thanks,
Robin.

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Easwar Hariharan]

On 7/11/2024 3:38 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Most of the IOMMU drivers are using the same DMA operations, which are
> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> to properly set them as a default with direct call without need to
> perform function pointer dereference.
> 
> During system initialization, the IOMMU driver can set its own DMA and
> in such case, the default DMA operations will be overridden.
> 
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  MAINTAINERS               |  1 +
>  drivers/iommu/dma-iommu.c | 24 +++++++-------
>  include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
>  kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
>  kernel/dma/mapping.c      |  9 +++---
>  5 files changed, 134 insertions(+), 17 deletions(-)
>  create mode 100644 include/linux/iommu-dma.h
>  create mode 100644 kernel/dma/iommu.h
> 

<snip>
> diff --git a/include/linux/iommu-dma.h b/include/linux/iommu-dma.h
> new file mode 100644
> index 000000000000..b42487bf8f8e
> --- /dev/null
> +++ b/include/linux/iommu-dma.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + *
> + * DMA operations that map physical memory through IOMMU.
> + */
> +#ifndef _LINUX_IOMMU_DMA_H
> +#define _LINUX_IOMMU_DMA_H
> +
> +#include <linux/dma-direction.h>
> +
> +#ifdef CONFIG_IOMMU_API
> +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> +			      unsigned long offset, size_t size,
> +			      enum dma_data_direction dir, unsigned long attrs);
> +void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> +			  size_t size, enum dma_data_direction dir,
> +			  unsigned long attrs);
> +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		     enum dma_data_direction dir, unsigned long attrs);
> +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> +			enum dma_data_direction dir, unsigned long attrs);
> +#else
> +static inline dma_addr_t iommu_dma_map_page(struct device *dev,
> +					    struct page *page,
> +					    unsigned long offset, size_t size,
> +					    enum dma_data_direction dir,
> +					    unsigned long attrs)
> +{
> +	return DMA_MAPPING_ERROR;
> +}
> +static inline void iommu_dma_unmap_page(struct device *dev,
> +					dma_addr_t dma_handle, size_t size,
> +					enum dma_data_direction dir,
> +					unsigned long attrs)
> +{
> +}
> +static inline int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> +				   int nents, enum dma_data_direction dir,
> +				   unsigned long attrs)
> +{
> +	return -EINVAL;
> +}
> +static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> +			       int nents, enum dma_data_direction dir,
> +			       unsigned long attrs)
> +{
> +}
> +#endif /* CONFIG_IOMMU_API */
> +#endif /* _LINUX_IOMMU_DMA_H */
> diff --git a/kernel/dma/iommu.h b/kernel/dma/iommu.h
> new file mode 100644
> index 000000000000..4abaea2dfc49
> --- /dev/null
> +++ b/kernel/dma/iommu.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> + *
> + * DMA operations that map physical memory through IOMMU.
> + */
> +#ifndef _KERNEL_DMA_IOMMU_H
> +#define _KERNEL_DMA_IOMMU_H
> +
> +#include <linux/iommu-dma.h>
> +
> +static inline dma_addr_t dma_iommu_map_page(struct device *dev,
> +					    struct page *page, size_t offset,
> +					    size_t size,
> +					    enum dma_data_direction dir,
> +					    unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->map_page)
> +		return ops->map_page(dev, page, offset, size, dir, attrs);
> +
> +	return iommu_dma_map_page(dev, page, offset, size, dir, attrs);
> +}
> +
> +static inline void dma_iommu_unmap_page(struct device *dev, dma_addr_t addr,
> +					size_t size,
> +					enum dma_data_direction dir,
> +					unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->unmap_page) {
> +		ops->unmap_page(dev, addr, size, dir, attrs);
> +		return;
> +	}
> +
> +	iommu_dma_unmap_page(dev, addr, size, dir, attrs);
> +}
> +
> +static inline int dma_iommu_map_sg(struct device *dev, struct scatterlist *sg,
> +				   int nents, enum dma_data_direction dir,
> +				   unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->map_sg)
> +		return ops->map_sg(dev, sg, nents, dir, attrs);
> +
> +	return iommu_dma_map_sg(dev, sg, nents, dir, attrs);
> +}
> +
> +static inline void dma_iommu_unmap_sg(struct device *dev,
> +				      struct scatterlist *sg, int nents,
> +				      enum dma_data_direction dir,
> +				      unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (ops->unmap_sg) {
> +		ops->unmap_sg(dev, sg, nents, dir, attrs);
> +		return;
> +	}
> +
> +	iommu_dma_unmap_sg(dev, sg, nents, dir, attrs);
> +}

Can we use _dma_iommu_* instead of the transposition pattern we have
going on here? Having dma_iommu_* call iommu_dma_* is, I feel, a recipe
for confusion when reading the code, especially after a few passes when
the eyes start to glaze over.

I think you're going for the typical pattern of iommu_dma* being an
internal detail that provides an implementation, but correct me if
there's some significance to the current naming scheme.

Thanks,
Easwar

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Christoph Hellwig]

On Fri, Jul 12, 2024 at 03:21:55PM +0100, Robin Murphy wrote:
>> This is done to keep layering similar to existing in DMA subsystem. We
>> have special files and calls to dma-direct, it looks natural to have
>> special files and call to dma-iommu. It is not nice to call to drivers/iommu
>> from kernel/dma/mapping.c
>
> That's where I firmly disagree. In the DMA API aspect, iommu-dma is exactly 
> a peer of dma-direct,

Exactly.

> however it lives in drivers/iommu for practical 
> reasons because it's more closely coupled to IOMMU API internals in all 
> other aspects of its implementation.

TBH I think kernel/dma/ would be the better place.  But that's really the
least my concernes at the moment.

But the important point is that I would really prefer to avoid another
magic layer - just do direct calls like for dma-direct to keep it
understandable (and probably faster).

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Fri, Jul 12, 2024 at 01:51:36PM -0700, Easwar Hariharan wrote:
> On 7/11/2024 3:38 AM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Most of the IOMMU drivers are using the same DMA operations, which are
> > default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
> > to properly set them as a default with direct call without need to
> > perform function pointer dereference.
> > 
> > During system initialization, the IOMMU driver can set its own DMA and
> > in such case, the default DMA operations will be overridden.
> > 
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  MAINTAINERS               |  1 +
> >  drivers/iommu/dma-iommu.c | 24 +++++++-------
> >  include/linux/iommu-dma.h | 50 +++++++++++++++++++++++++++++
> >  kernel/dma/iommu.h        | 67 +++++++++++++++++++++++++++++++++++++++
> >  kernel/dma/mapping.c      |  9 +++---
> >  5 files changed, 134 insertions(+), 17 deletions(-)
> >  create mode 100644 include/linux/iommu-dma.h
> >  create mode 100644 kernel/dma/iommu.h
> > 
> 
> <snip>
> > diff --git a/include/linux/iommu-dma.h b/include/linux/iommu-dma.h
> > new file mode 100644
> > index 000000000000..b42487bf8f8e
> > --- /dev/null
> > +++ b/include/linux/iommu-dma.h
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> > + *
> > + * DMA operations that map physical memory through IOMMU.
> > + */
> > +#ifndef _LINUX_IOMMU_DMA_H
> > +#define _LINUX_IOMMU_DMA_H
> > +
> > +#include <linux/dma-direction.h>
> > +
> > +#ifdef CONFIG_IOMMU_API
> > +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> > +			      unsigned long offset, size_t size,
> > +			      enum dma_data_direction dir, unsigned long attrs);
> > +void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> > +			  size_t size, enum dma_data_direction dir,
> > +			  unsigned long attrs);
> > +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> > +		     enum dma_data_direction dir, unsigned long attrs);
> > +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> > +			enum dma_data_direction dir, unsigned long attrs);
> > +#else
> > +static inline dma_addr_t iommu_dma_map_page(struct device *dev,
> > +					    struct page *page,
> > +					    unsigned long offset, size_t size,
> > +					    enum dma_data_direction dir,
> > +					    unsigned long attrs)
> > +{
> > +	return DMA_MAPPING_ERROR;
> > +}
> > +static inline void iommu_dma_unmap_page(struct device *dev,
> > +					dma_addr_t dma_handle, size_t size,
> > +					enum dma_data_direction dir,
> > +					unsigned long attrs)
> > +{
> > +}
> > +static inline int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> > +				   int nents, enum dma_data_direction dir,
> > +				   unsigned long attrs)
> > +{
> > +	return -EINVAL;
> > +}
> > +static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
> > +			       int nents, enum dma_data_direction dir,
> > +			       unsigned long attrs)
> > +{
> > +}
> > +#endif /* CONFIG_IOMMU_API */
> > +#endif /* _LINUX_IOMMU_DMA_H */
> > diff --git a/kernel/dma/iommu.h b/kernel/dma/iommu.h
> > new file mode 100644
> > index 000000000000..4abaea2dfc49
> > --- /dev/null
> > +++ b/kernel/dma/iommu.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved
> > + *
> > + * DMA operations that map physical memory through IOMMU.
> > + */
> > +#ifndef _KERNEL_DMA_IOMMU_H
> > +#define _KERNEL_DMA_IOMMU_H
> > +
> > +#include <linux/iommu-dma.h>
> > +
> > +static inline dma_addr_t dma_iommu_map_page(struct device *dev,
> > +					    struct page *page, size_t offset,
> > +					    size_t size,
> > +					    enum dma_data_direction dir,
> > +					    unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->map_page)
> > +		return ops->map_page(dev, page, offset, size, dir, attrs);
> > +
> > +	return iommu_dma_map_page(dev, page, offset, size, dir, attrs);
> > +}
> > +
> > +static inline void dma_iommu_unmap_page(struct device *dev, dma_addr_t addr,
> > +					size_t size,
> > +					enum dma_data_direction dir,
> > +					unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->unmap_page) {
> > +		ops->unmap_page(dev, addr, size, dir, attrs);
> > +		return;
> > +	}
> > +
> > +	iommu_dma_unmap_page(dev, addr, size, dir, attrs);
> > +}
> > +
> > +static inline int dma_iommu_map_sg(struct device *dev, struct scatterlist *sg,
> > +				   int nents, enum dma_data_direction dir,
> > +				   unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->map_sg)
> > +		return ops->map_sg(dev, sg, nents, dir, attrs);
> > +
> > +	return iommu_dma_map_sg(dev, sg, nents, dir, attrs);
> > +}
> > +
> > +static inline void dma_iommu_unmap_sg(struct device *dev,
> > +				      struct scatterlist *sg, int nents,
> > +				      enum dma_data_direction dir,
> > +				      unsigned long attrs)
> > +{
> > +	const struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	if (ops->unmap_sg) {
> > +		ops->unmap_sg(dev, sg, nents, dir, attrs);
> > +		return;
> > +	}
> > +
> > +	iommu_dma_unmap_sg(dev, sg, nents, dir, attrs);
> > +}
> 
> Can we use _dma_iommu_* instead of the transposition pattern we have
> going on here? Having dma_iommu_* call iommu_dma_* is, I feel, a recipe
> for confusion when reading the code, especially after a few passes when
> the eyes start to glaze over.
> 
> I think you're going for the typical pattern of iommu_dma* being an
> internal detail that provides an implementation, but correct me if
> there's some significance to the current naming scheme.

Given the review feedback from Robin and Christoph, I will drop this
layer anyway.

Thanks

> 
> Thanks,
> Easwar

Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Leon Romanovsky]

On Sat, Jul 13, 2024 at 07:18:13AM +0200, Christoph Hellwig wrote:
> On Fri, Jul 12, 2024 at 03:21:55PM +0100, Robin Murphy wrote:
> >> This is done to keep layering similar to existing in DMA subsystem. We
> >> have special files and calls to dma-direct, it looks natural to have
> >> special files and call to dma-iommu. It is not nice to call to drivers/iommu
> >> from kernel/dma/mapping.c
> >
> > That's where I firmly disagree. In the DMA API aspect, iommu-dma is exactly 
> > a peer of dma-direct,
> 
> Exactly.
> 
> > however it lives in drivers/iommu for practical 
> > reasons because it's more closely coupled to IOMMU API internals in all 
> > other aspects of its implementation.
> 
> TBH I think kernel/dma/ would be the better place.  But that's really the
> least my concernes at the moment.
> 
> But the important point is that I would really prefer to avoid another
> magic layer - just do direct calls like for dma-direct to keep it
> understandable (and probably faster).

I'll change my patch to be without extra layer, but is it unlikely that
it will make any difference in performance.

Thanks

>