[PATCH v9 0/5] iommu: Support mappings/reservations in reserved-memory regions

[PATCH v9 0/5] iommu: Support mappings/reservations in reserved-memory regions

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

This version has several fixes over the previous v8, which can be found
here:

  https://lore.kernel.org/all/20220905170833.396892-1-thierry.reding@gmail.com/

An example is included in the DT bindings, but here is an extract of
what I've used to test this:

        reserved-memory {
                #address-cells = <2>;
                #size-cells = <2>;
                ranges;

                /*
                 * Creates an identity mapping for the framebuffer that
                 * the firmware has setup to scan out a bootsplash from.
                 */
                fb: framebuffer@92cb2000 {
                        reg = <0x0 0x92cb2000 0x0 0x00800000>;
                        iommu-addresses = <&dc0 0x0 0x92cb2000 0x0 0x00800000>;
                };

                /*
                 * Creates a reservation in the IOVA space to prevent
                 * any buffers from being mapped to that region. Note
                 * that on Tegra the range is actually quite different
                 * from this, but it would conflict with the display
                 * driver that I tested this against, so this is just
                 * a dummy region for testing.
                 */
                adsp: reservation-adsp {
                        iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00010000>;
                };
        };

        host1x@50000000 {
                dc@54200000 {
                        memory-region = <&fb>, <&adsp>;
                };
        };

This is abbreviated a little to focus on the essentials. Note also that
the ADSP reservation is not actually used on this device and the driver
for this doesn't exist yet, but I wanted to include this variant for
testing, because we'll want to use these bindings for the reservation
use-case as well at some point.

I've also been able to make use of this binding and the IOMMU code in
conjunction with the simple-framebuffer driver to hand over a display
configuration set up by UEFI to the Linux kernel.

Janne has confirmed[0] this to be suitable for indirect mappings as
well, though these patches don't implement that feature yet. Potential
extensions to this have been discussed but are not yet included at this
time to not further complicate things.

Thierry

[0]: https://lore.kernel.org/all/20220909144504.GA4024@jannau.net/

Navneet Kumar (1):
  iommu/tegra-smmu: Support managed domains

Thierry Reding (4):
  dt-bindings: reserved-memory: Document iommu-addresses
  iommu: Implement of_iommu_get_resv_regions()
  iommu: dma: Use of_iommu_get_resv_regions()
  iommu/tegra-smmu: Add support for reserved regions

 .../reserved-memory/reserved-memory.yaml      |  70 ++++++++++++
 drivers/iommu/dma-iommu.c                     |   3 +
 drivers/iommu/of_iommu.c                      | 104 ++++++++++++++++++
 drivers/iommu/tegra-smmu.c                    |  86 ++++++++++++---
 include/linux/of_iommu.h                      |   8 ++
 5 files changed, 254 insertions(+), 17 deletions(-)

-- 
2.37.3

[PATCH v9 1/5] dt-bindings: reserved-memory: Document iommu-addresses

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This adds the "iommu-addresses" property to reserved-memory nodes, which
allow describing the interaction of memory regions with IOMMUs. Two use-
cases are supported:

  1. Static mappings can be described by pairing the "iommu-addresses"
     property with a "reg" property. This is mostly useful for adopting
     firmware-allocated buffers via identity mappings. One common use-
     case where this is required is if early firmware or bootloaders
     have set up a bootsplash framebuffer that a display controller is
     actively scanning out from during the operating system boot
     process.

  2. If an "iommu-addresses" property exists without a "reg" property,
     the reserved-memory node describes an IOVA reservation. Such memory
     regions are excluded from the IOVA space available to operating
     system drivers and can be used for regions that must not be used to
     map arbitrary buffers.

Each mapping or reservation is tied to a specific device via a phandle
to the device's device tree node. This allows a reserved-memory region
to be reused across multiple devices.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v9:
- add Reviewed-by tags

Changes in v8:
- include validation warnings that had crept into an unrelated patch

Changes in v7:
- keep reserved-memory.txt to avoid broken references

Changes in v6:
- add device phandle to iommu-addresses property in examples
- remove now unused dt-bindings/reserved-memory.h header

 .../reserved-memory/reserved-memory.yaml      | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
index 44f72bcf1782..fb48d016e368 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
@@ -52,6 +52,30 @@ properties:
       Address and Length pairs. Specifies regions of memory that are
       acceptable to allocate from.
 
+  iommu-addresses:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: >
+      A list of phandle and specifier pairs that describe static IO virtual
+      address space mappings and carveouts associated with a given reserved
+      memory region. The phandle in the first cell refers to the device for
+      which the mapping or carveout is to be created.
+
+      The specifier consists of an address/size pair and denotes the IO
+      virtual address range of the region for the given device. The exact
+      format depends on the values of the "#address-cells" and "#size-cells"
+      properties of the device referenced via the phandle.
+
+      When used in combination with a "reg" property, an IOVA mapping is to
+      be established for this memory region. One example where this can be
+      useful is to create an identity mapping for physical memory that the
+      firmware has configured some hardware to access (such as a bootsplash
+      framebuffer).
+
+      If no "reg" property is specified, the "iommu-addresses" property
+      defines carveout regions in the IOVA space for the given device. This
+      can be useful if a certain memory region should not be mapped through
+      the IOMMU.
+
   no-map:
     type: boolean
     description: >
@@ -97,4 +121,50 @@ oneOf:
 
 additionalProperties: true
 
+examples:
+  - |
+    / {
+      compatible = "foo";
+      model = "foo";
+
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      reserved-memory {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ranges;
+
+        adsp_resv: reservation-adsp {
+          /*
+           * Restrict IOVA mappings for ADSP buffers to the 512 MiB region
+           * from 0x40000000 - 0x5fffffff. Anything outside is reserved by
+           * the ADSP for I/O memory and private memory allocations.
+           */
+          iommu-addresses = <&adsp 0x0 0x00000000 0x00 0x40000000>,
+                            <&adsp 0x0 0x60000000 0xff 0xa0000000>;
+        };
+
+        fb: framebuffer@90000000 {
+          reg = <0x0 0x90000000 0x0 0x00800000>;
+          iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
+        };
+      };
+
+      bus@0 {
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges = <0x0 0x0 0x0 0x40000000>;
+
+        adsp: adsp@2990000 {
+          reg = <0x2990000 0x2000>;
+          memory-region = <&adsp_resv>;
+        };
+
+        dc0: display@15200000 {
+          reg = <0x15200000 0x10000>;
+          memory-region = <&fb>;
+        };
+      };
+    };
 ...
-- 
2.37.3

[PATCH v9 2/5] iommu: Implement of_iommu_get_resv_regions()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This is an implementation that IOMMU drivers can use to obtain reserved
memory regions from a device tree node. It uses the reserved-memory DT
bindings to find the regions associated with a given device. If these
regions are marked accordingly, identity mappings will be created for
them in the IOMMU domain that the devices will be attached to.

Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v9:
- address review comments by Robin Murphy:
  - warn about non-direct mappings since they are not supported yet
  - cleanup code to require less indentation
  - narrow scope of variables

Changes in v8:
- cleanup set-but-unused variables

Changes in v6:
- remove reference to now unused dt-bindings/reserved-memory.h include

Changes in v5:
- update for new "iommu-addresses" device tree bindings

Changes in v4:
- fix build failure on !CONFIG_OF_ADDRESS

Changes in v3:
- change "active" property to identity mapping flag that is part of the
  memory region specifier (as defined by #memory-region-cells) to allow
  per-reference flags to be used

Changes in v2:
- use "active" property to determine whether direct mappings are needed

 drivers/iommu/of_iommu.c | 104 +++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h |   8 +++
 2 files changed, 112 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5696314ae69e..0bf2b08bca0a 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_iommu.h>
 #include <linux/of_pci.h>
 #include <linux/pci.h>
@@ -172,3 +173,106 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 
 	return ops;
 }
+
+static inline bool check_direct_mapping(struct device *dev, struct resource *phys,
+					phys_addr_t start, phys_addr_t end)
+{
+	if (start != phys->start || end != phys->end) {
+		dev_warn(dev, "treating non-direct mapping [%pr] -> [%pap-%pap] as reservation\n",
+			 &phys, &start, &end);
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * of_iommu_get_resv_regions - reserved region driver helper for device tree
+ * @dev: device for which to get reserved regions
+ * @list: reserved region list
+ *
+ * IOMMU drivers can use this to implement their .get_resv_regions() callback
+ * for memory regions attached to a device tree node. See the reserved-memory
+ * device tree bindings on how to use these:
+ *
+ *   Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+ */
+void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
+{
+#if IS_ENABLED(CONFIG_OF_ADDRESS)
+	struct of_phandle_iterator it;
+	int err;
+
+	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
+		const __be32 *maps, *end;
+		struct resource res;
+		int size;
+
+		memset(&res, 0, sizeof(res));
+
+		/*
+		 * The "reg" property is optional and can be omitted by reserved-memory regions
+		 * that represent reservations in the IOVA space, which are regions that should
+		 * not be mapped.
+		 */
+		if (of_find_property(it.node, "reg", NULL)) {
+			err = of_address_to_resource(it.node, 0, &res);
+			if (err < 0) {
+				dev_err(dev, "failed to parse memory region %pOF: %d\n",
+					it.node, err);
+				continue;
+			}
+		}
+
+		maps = of_get_property(it.node, "iommu-addresses", &size);
+		if (!maps)
+			continue;
+
+		end = maps + size / sizeof(__be32);
+
+		while (maps < end) {
+			struct device_node *np;
+			u32 phandle;
+			int na, ns;
+
+			phandle = be32_to_cpup(maps++);
+			np = of_find_node_by_phandle(phandle);
+			na = of_n_addr_cells(np);
+			ns = of_n_size_cells(np);
+
+			if (np == dev->of_node) {
+				int prot = IOMMU_READ | IOMMU_WRITE;
+				struct iommu_resv_region *region;
+				enum iommu_resv_type type;
+				phys_addr_t start;
+				size_t length;
+
+				start = of_translate_dma_address(np, maps);
+				length = of_read_number(maps + na, ns);
+
+				/*
+				 * IOMMU regions without an associated physical region cannot be
+				 * mapped and are simply reservations.
+				 */
+				if (res.end > res.start) {
+					phys_addr_t end = start + length - 1;
+
+					if (check_direct_mapping(dev, &res, start, end))
+						type = IOMMU_RESV_DIRECT_RELAXABLE;
+					else
+						type = IOMMU_RESV_RESERVED;
+				} else {
+					type = IOMMU_RESV_RESERVED;
+				}
+
+				region = iommu_alloc_resv_region(start, length, prot, type);
+				if (region)
+					list_add_tail(&region->list, list);
+			}
+
+			maps += na + ns;
+		}
+	}
+#endif
+}
+EXPORT_SYMBOL(of_iommu_get_resv_regions);
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 55c1eb300a86..9a5e6b410dd2 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -12,6 +12,9 @@ extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np,
 					const u32 *id);
 
+extern void of_iommu_get_resv_regions(struct device *dev,
+				      struct list_head *list);
+
 #else
 
 static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
@@ -21,6 +24,11 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+static inline void of_iommu_get_resv_regions(struct device *dev,
+					     struct list_head *list)
+{
+}
+
 #endif	/* CONFIG_OF_IOMMU */
 
 #endif /* __OF_IOMMU_H */
-- 
2.37.3

[PATCH v9 3/5] iommu: dma: Use of_iommu_get_resv_regions()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

For device tree nodes, use the standard of_iommu_get_resv_regions()
implementation to obtain the reserved memory regions associated with a
device.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/iommu/dma-iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 9297b741f5e8..709b05d3aad2 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -23,6 +23,7 @@
 #include <linux/memremap.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
+#include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
@@ -391,6 +392,8 @@ void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
 		iort_iommu_get_resv_regions(dev, list);
 
+	if (dev->of_node)
+		of_iommu_get_resv_regions(dev, list);
 }
 EXPORT_SYMBOL(iommu_dma_get_resv_regions);
 
-- 
2.37.3

[PATCH v9 4/5] iommu/tegra-smmu: Add support for reserved regions

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The Tegra DRM driver currently uses the IOMMU API explicitly. This means
that it has fine-grained control over when exactly the translation
through the IOMMU is enabled. This currently happens after the driver
probes, so the driver is in a DMA quiesced state when the IOMMU
translation is enabled.

During the transition of the Tegra DRM driver to use the DMA API instead
of the IOMMU API explicitly, it was observed that on certain platforms
the display controllers were still actively fetching from memory. When a
DMA IOMMU domain is created as part of the DMA/IOMMU API setup during
boot, the IOMMU translation for the display controllers can be enabled a
significant amount of time before the driver has had a chance to reset
the hardware into a sane state. This causes the SMMU to detect faults on
the addresses that the display controller is trying to fetch.

To avoid this, and as a byproduct paving the way for seamless transition
of display from the bootloader to the kernel, add support for reserved
regions in the Tegra SMMU driver. This is implemented using the standard
reserved memory device tree bindings, which let us describe regions of
memory which the kernel is forbidden from using for regular allocations.
The Tegra SMMU driver will parse the nodes associated with each device
via the "memory-region" property and return reserved regions that the
IOMMU core will then create direct mappings for prior to attaching the
IOMMU domains to the devices. This ensures that a 1:1 mapping is in
place when IOMMU translation starts and prevents the SMMU from detecting
any faults.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 50 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a86e5c8da1b1..57b4f2b37447 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -471,6 +472,7 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
 	tegra_smmu_free_asid(smmu, as->id);
 
 	dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
+	as->pd_dma = 0;
 
 	as->smmu = NULL;
 
@@ -534,6 +536,38 @@ static void tegra_smmu_set_pde(struct tegra_smmu_as *as, unsigned long iova,
 	struct tegra_smmu *smmu = as->smmu;
 	u32 *pd = page_address(as->pd);
 	unsigned long offset = pd_index * sizeof(*pd);
+	bool unmap = false;
+
+	/*
+	 * XXX Move this outside of this function. Perhaps add a struct
+	 * iommu_domain parameter to ->{get,put}_resv_regions() so that
+	 * the mapping can be done there.
+	 *
+	 * The problem here is that as->smmu is only known once we attach
+	 * the domain to a device (because then we look up the right SMMU
+	 * instance via the dev->archdata.iommu pointer). When the direct
+	 * mappings are created for reserved regions, the domain has not
+	 * been attached to a device yet, so we don't know. We currently
+	 * fix that up in ->apply_resv_regions() because that is the first
+	 * time where we have access to a struct device that will be used
+	 * with the IOMMU domain. However, that's asymmetric and doesn't
+	 * take care of the page directory mapping either, so we need to
+	 * come up with something better.
+	 */
+	if (WARN_ON_ONCE(as->pd_dma == 0)) {
+		as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
+					  DMA_TO_DEVICE);
+		if (dma_mapping_error(smmu->dev, as->pd_dma))
+			return;
+
+		if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
+			dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD,
+				       DMA_TO_DEVICE);
+			return;
+		}
+
+		unmap = true;
+	}
 
 	/* Set the page directory entry first */
 	pd[pd_index] = value;
@@ -546,6 +580,12 @@ static void tegra_smmu_set_pde(struct tegra_smmu_as *as, unsigned long iova,
 	smmu_flush_ptc(smmu, as->pd_dma, offset);
 	smmu_flush_tlb_section(smmu, as->id, iova);
 	smmu_flush(smmu);
+
+	if (unmap) {
+		dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD,
+			       DMA_TO_DEVICE);
+		as->pd_dma = 0;
+	}
 }
 
 static u32 *tegra_smmu_pte_offset(struct page *pt_page, unsigned long iova)
@@ -846,7 +886,6 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 		smmu = tegra_smmu_find(args.np);
 		if (smmu) {
 			err = tegra_smmu_configure(smmu, dev, &args);
-
 			if (err < 0) {
 				of_node_put(args.np);
 				return ERR_PTR(err);
@@ -864,6 +903,13 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 	return &smmu->iommu;
 }
 
+static void tegra_smmu_release_device(struct device *dev)
+{
+	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
+
+	put_device(smmu->dev);
+}
+
 static const struct tegra_smmu_group_soc *
 tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
 {
@@ -964,7 +1010,9 @@ static int tegra_smmu_of_xlate(struct device *dev,
 static const struct iommu_ops tegra_smmu_ops = {
 	.domain_alloc = tegra_smmu_domain_alloc,
 	.probe_device = tegra_smmu_probe_device,
+	.release_device = tegra_smmu_release_device,
 	.device_group = tegra_smmu_device_group,
+	.get_resv_regions = of_iommu_get_resv_regions,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.37.3

[PATCH v9 5/5] iommu/tegra-smmu: Support managed domains

[Thierry Reding]

From: Navneet Kumar <navneetk@nvidia.com>

Allow creating identity and DMA API compatible IOMMU domains. When
creating a DMA API compatible domain, make sure to also create the
required cookie.

Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v5:
- remove DMA cookie initialization that's now no longer needed

 drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 57b4f2b37447..7ad993330634 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -20,6 +20,8 @@
 #include <soc/tegra/ahb.h>
 #include <soc/tegra/mc.h>
 
+#include "dma-iommu.h"
+
 struct tegra_smmu_group {
 	struct list_head list;
 	struct tegra_smmu *smmu;
@@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 {
 	struct tegra_smmu_as *as;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_DMA &&
+	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
 	as = kzalloc(sizeof(*as), GFP_KERNEL);
@@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
 
 	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
-	if (!as->pd) {
-		kfree(as);
-		return NULL;
-	}
+	if (!as->pd)
+		goto free_as;
 
 	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
-	if (!as->count) {
-		__free_page(as->pd);
-		kfree(as);
-		return NULL;
-	}
+	if (!as->count)
+		goto free_pd_range;
 
 	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
-	if (!as->pts) {
-		kfree(as->count);
-		__free_page(as->pd);
-		kfree(as);
-		return NULL;
-	}
+	if (!as->pts)
+		goto free_pts;
 
 	spin_lock_init(&as->lock);
 
@@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 	as->domain.geometry.force_aperture = true;
 
 	return &as->domain;
+
+free_pts:
+	kfree(as->pts);
+free_pd_range:
+	__free_page(as->pd);
+free_as:
+	kfree(as);
+
+	return NULL;
 }
 
 static void tegra_smmu_domain_free(struct iommu_domain *domain)
@@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.probe_device = tegra_smmu_probe_device,
 	.release_device = tegra_smmu_release_device,
 	.device_group = tegra_smmu_device_group,
-	.get_resv_regions = of_iommu_get_resv_regions,
+	.get_resv_regions = iommu_dma_get_resv_regions,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.37.3

Re: [PATCH v9 0/5] iommu: Support mappings/reservations in reserved-memory regions

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3469 bytes --]

On Fri, Sep 23, 2022 at 02:35:52PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Hi,
> 
> This version has several fixes over the previous v8, which can be found
> here:
> 
>   https://lore.kernel.org/all/20220905170833.396892-1-thierry.reding@gmail.com/
> 
> An example is included in the DT bindings, but here is an extract of
> what I've used to test this:
> 
>         reserved-memory {
>                 #address-cells = <2>;
>                 #size-cells = <2>;
>                 ranges;
> 
>                 /*
>                  * Creates an identity mapping for the framebuffer that
>                  * the firmware has setup to scan out a bootsplash from.
>                  */
>                 fb: framebuffer@92cb2000 {
>                         reg = <0x0 0x92cb2000 0x0 0x00800000>;
>                         iommu-addresses = <&dc0 0x0 0x92cb2000 0x0 0x00800000>;
>                 };
> 
>                 /*
>                  * Creates a reservation in the IOVA space to prevent
>                  * any buffers from being mapped to that region. Note
>                  * that on Tegra the range is actually quite different
>                  * from this, but it would conflict with the display
>                  * driver that I tested this against, so this is just
>                  * a dummy region for testing.
>                  */
>                 adsp: reservation-adsp {
>                         iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00010000>;
>                 };
>         };
> 
>         host1x@50000000 {
>                 dc@54200000 {
>                         memory-region = <&fb>, <&adsp>;
>                 };
>         };
> 
> This is abbreviated a little to focus on the essentials. Note also that
> the ADSP reservation is not actually used on this device and the driver
> for this doesn't exist yet, but I wanted to include this variant for
> testing, because we'll want to use these bindings for the reservation
> use-case as well at some point.
> 
> I've also been able to make use of this binding and the IOMMU code in
> conjunction with the simple-framebuffer driver to hand over a display
> configuration set up by UEFI to the Linux kernel.
> 
> Janne has confirmed[0] this to be suitable for indirect mappings as
> well, though these patches don't implement that feature yet. Potential
> extensions to this have been discussed but are not yet included at this
> time to not further complicate things.
> 
> Thierry
> 
> [0]: https://lore.kernel.org/all/20220909144504.GA4024@jannau.net/
> 
> Navneet Kumar (1):
>   iommu/tegra-smmu: Support managed domains
> 
> Thierry Reding (4):
>   dt-bindings: reserved-memory: Document iommu-addresses
>   iommu: Implement of_iommu_get_resv_regions()
>   iommu: dma: Use of_iommu_get_resv_regions()
>   iommu/tegra-smmu: Add support for reserved regions
> 
>  .../reserved-memory/reserved-memory.yaml      |  70 ++++++++++++
>  drivers/iommu/dma-iommu.c                     |   3 +
>  drivers/iommu/of_iommu.c                      | 104 ++++++++++++++++++
>  drivers/iommu/tegra-smmu.c                    |  86 ++++++++++++---
>  include/linux/of_iommu.h                      |   8 ++
>  5 files changed, 254 insertions(+), 17 deletions(-)

Joerg, if there are no further concerns on this, can you pick this up
once v6.1-rc1 is released?

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 1/5] dt-bindings: reserved-memory: Document iommu-addresses

[Robin Murphy]

On 2022-09-23 13:35, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This adds the "iommu-addresses" property to reserved-memory nodes, which
> allow describing the interaction of memory regions with IOMMUs. Two use-
> cases are supported:
> 
>    1. Static mappings can be described by pairing the "iommu-addresses"
>       property with a "reg" property. This is mostly useful for adopting
>       firmware-allocated buffers via identity mappings. One common use-
>       case where this is required is if early firmware or bootloaders
>       have set up a bootsplash framebuffer that a display controller is
>       actively scanning out from during the operating system boot
>       process.
> 
>    2. If an "iommu-addresses" property exists without a "reg" property,
>       the reserved-memory node describes an IOVA reservation. Such memory
>       regions are excluded from the IOVA space available to operating
>       system drivers and can be used for regions that must not be used to
>       map arbitrary buffers.

Bah, I've only just realised: don't we also need to change the "oneOf: 
required: ..." schema to permit "iommu-addresses" without "reg" or "size"?

Thanks,
Robin.

> Each mapping or reservation is tied to a specific device via a phandle
> to the device's device tree node. This allows a reserved-memory region
> to be reused across multiple devices.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v9:
> - add Reviewed-by tags
> 
> Changes in v8:
> - include validation warnings that had crept into an unrelated patch
> 
> Changes in v7:
> - keep reserved-memory.txt to avoid broken references
> 
> Changes in v6:
> - add device phandle to iommu-addresses property in examples
> - remove now unused dt-bindings/reserved-memory.h header
> 
>   .../reserved-memory/reserved-memory.yaml      | 70 +++++++++++++++++++
>   1 file changed, 70 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> index 44f72bcf1782..fb48d016e368 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> @@ -52,6 +52,30 @@ properties:
>         Address and Length pairs. Specifies regions of memory that are
>         acceptable to allocate from.
>   
> +  iommu-addresses:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: >
> +      A list of phandle and specifier pairs that describe static IO virtual
> +      address space mappings and carveouts associated with a given reserved
> +      memory region. The phandle in the first cell refers to the device for
> +      which the mapping or carveout is to be created.
> +
> +      The specifier consists of an address/size pair and denotes the IO
> +      virtual address range of the region for the given device. The exact
> +      format depends on the values of the "#address-cells" and "#size-cells"
> +      properties of the device referenced via the phandle.
> +
> +      When used in combination with a "reg" property, an IOVA mapping is to
> +      be established for this memory region. One example where this can be
> +      useful is to create an identity mapping for physical memory that the
> +      firmware has configured some hardware to access (such as a bootsplash
> +      framebuffer).
> +
> +      If no "reg" property is specified, the "iommu-addresses" property
> +      defines carveout regions in the IOVA space for the given device. This
> +      can be useful if a certain memory region should not be mapped through
> +      the IOMMU.
> +
>     no-map:
>       type: boolean
>       description: >
> @@ -97,4 +121,50 @@ oneOf:
>   
>   additionalProperties: true
>   
> +examples:
> +  - |
> +    / {
> +      compatible = "foo";
> +      model = "foo";
> +
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      reserved-memory {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        ranges;
> +
> +        adsp_resv: reservation-adsp {
> +          /*
> +           * Restrict IOVA mappings for ADSP buffers to the 512 MiB region
> +           * from 0x40000000 - 0x5fffffff. Anything outside is reserved by
> +           * the ADSP for I/O memory and private memory allocations.
> +           */
> +          iommu-addresses = <&adsp 0x0 0x00000000 0x00 0x40000000>,
> +                            <&adsp 0x0 0x60000000 0xff 0xa0000000>;
> +        };
> +
> +        fb: framebuffer@90000000 {
> +          reg = <0x0 0x90000000 0x0 0x00800000>;
> +          iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
> +        };
> +      };
> +
> +      bus@0 {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges = <0x0 0x0 0x0 0x40000000>;
> +
> +        adsp: adsp@2990000 {
> +          reg = <0x2990000 0x2000>;
> +          memory-region = <&adsp_resv>;
> +        };
> +
> +        dc0: display@15200000 {
> +          reg = <0x15200000 0x10000>;
> +          memory-region = <&fb>;
> +        };
> +      };
> +    };
>   ...

Re: [PATCH v9 2/5] iommu: Implement of_iommu_get_resv_regions()

[Robin Murphy]

On 2022-09-23 13:35, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This is an implementation that IOMMU drivers can use to obtain reserved
> memory regions from a device tree node. It uses the reserved-memory DT
> bindings to find the regions associated with a given device. If these
> regions are marked accordingly, identity mappings will be created for
> them in the IOMMU domain that the devices will be attached to.
> 
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v9:
> - address review comments by Robin Murphy:
>    - warn about non-direct mappings since they are not supported yet
>    - cleanup code to require less indentation
>    - narrow scope of variables
> 
> Changes in v8:
> - cleanup set-but-unused variables
> 
> Changes in v6:
> - remove reference to now unused dt-bindings/reserved-memory.h include
> 
> Changes in v5:
> - update for new "iommu-addresses" device tree bindings
> 
> Changes in v4:
> - fix build failure on !CONFIG_OF_ADDRESS
> 
> Changes in v3:
> - change "active" property to identity mapping flag that is part of the
>    memory region specifier (as defined by #memory-region-cells) to allow
>    per-reference flags to be used
> 
> Changes in v2:
> - use "active" property to determine whether direct mappings are needed
> 
>   drivers/iommu/of_iommu.c | 104 +++++++++++++++++++++++++++++++++++++++
>   include/linux/of_iommu.h |   8 +++
>   2 files changed, 112 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5696314ae69e..0bf2b08bca0a 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -11,6 +11,7 @@
>   #include <linux/module.h>
>   #include <linux/msi.h>
>   #include <linux/of.h>
> +#include <linux/of_address.h>
>   #include <linux/of_iommu.h>
>   #include <linux/of_pci.h>
>   #include <linux/pci.h>
> @@ -172,3 +173,106 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>   
>   	return ops;
>   }
> +
> +static inline bool check_direct_mapping(struct device *dev, struct resource *phys,

Where "phys" is the virtual address, right? :(

> +					phys_addr_t start, phys_addr_t end)
> +{
> +	if (start != phys->start || end != phys->end) {
> +		dev_warn(dev, "treating non-direct mapping [%pr] -> [%pap-%pap] as reservation\n",
> +			 &phys, &start, &end);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * of_iommu_get_resv_regions - reserved region driver helper for device tree
> + * @dev: device for which to get reserved regions
> + * @list: reserved region list
> + *
> + * IOMMU drivers can use this to implement their .get_resv_regions() callback
> + * for memory regions attached to a device tree node. See the reserved-memory
> + * device tree bindings on how to use these:
> + *
> + *   Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> + */
> +void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
> +{
> +#if IS_ENABLED(CONFIG_OF_ADDRESS)
> +	struct of_phandle_iterator it;
> +	int err;
> +
> +	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
> +		const __be32 *maps, *end;
> +		struct resource res;
> +		int size;
> +
> +		memset(&res, 0, sizeof(res));
> +
> +		/*
> +		 * The "reg" property is optional and can be omitted by reserved-memory regions
> +		 * that represent reservations in the IOVA space, which are regions that should
> +		 * not be mapped.
> +		 */
> +		if (of_find_property(it.node, "reg", NULL)) {
> +			err = of_address_to_resource(it.node, 0, &res);
> +			if (err < 0) {
> +				dev_err(dev, "failed to parse memory region %pOF: %d\n",
> +					it.node, err);
> +				continue;
> +			}
> +		}
> +
> +		maps = of_get_property(it.node, "iommu-addresses", &size);
> +		if (!maps)
> +			continue;
> +
> +		end = maps + size / sizeof(__be32);
> +
> +		while (maps < end) {
> +			struct device_node *np;
> +			u32 phandle;
> +			int na, ns;
> +
> +			phandle = be32_to_cpup(maps++);
> +			np = of_find_node_by_phandle(phandle);
> +			na = of_n_addr_cells(np);
> +			ns = of_n_size_cells(np);
> +
> +			if (np == dev->of_node) {
> +				int prot = IOMMU_READ | IOMMU_WRITE;
> +				struct iommu_resv_region *region;
> +				enum iommu_resv_type type;
> +				phys_addr_t start;
> +				size_t length;
> +
> +				start = of_translate_dma_address(np, maps);
> +				length = of_read_number(maps + na, ns);
> +
> +				/*
> +				 * IOMMU regions without an associated physical region cannot be
> +				 * mapped and are simply reservations.
> +				 */
> +				if (res.end > res.start) {
> +					phys_addr_t end = start + length - 1;
> +
> +					if (check_direct_mapping(dev, &res, start, end))
> +						type = IOMMU_RESV_DIRECT_RELAXABLE;

Again I really don't think we should assume relaxable by default.

Looking at the shape of things now, it seems like 
check_direct_mappings() wants to subsume the check on res as well and 
grow in to a more general function for determining the iommu_resv_type. 
Then we've got a clear place to start special-casing things like 
simple-framebuffer that we do know a bit more about.

Thanks,
Robin.

> +					else
> +						type = IOMMU_RESV_RESERVED;
> +				} else {
> +					type = IOMMU_RESV_RESERVED;
> +				}
> +
> +				region = iommu_alloc_resv_region(start, length, prot, type);
> +				if (region)
> +					list_add_tail(&region->list, list);
> +			}
> +
> +			maps += na + ns;
> +		}
> +	}
> +#endif
> +}
> +EXPORT_SYMBOL(of_iommu_get_resv_regions);
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 55c1eb300a86..9a5e6b410dd2 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -12,6 +12,9 @@ extern const struct iommu_ops *of_iommu_configure(struct device *dev,
>   					struct device_node *master_np,
>   					const u32 *id);
>   
> +extern void of_iommu_get_resv_regions(struct device *dev,
> +				      struct list_head *list);
> +
>   #else
>   
>   static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> @@ -21,6 +24,11 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
>   	return NULL;
>   }
>   
> +static inline void of_iommu_get_resv_regions(struct device *dev,
> +					     struct list_head *list)
> +{
> +}
> +
>   #endif	/* CONFIG_OF_IOMMU */
>   
>   #endif /* __OF_IOMMU_H */

Re: [PATCH v9 5/5] iommu/tegra-smmu: Support managed domains

[Robin Murphy]

On 2022-09-23 13:35, Thierry Reding wrote:
> From: Navneet Kumar <navneetk@nvidia.com>
> 
> Allow creating identity and DMA API compatible IOMMU domains. When
> creating a DMA API compatible domain, make sure to also create the
> required cookie.

Nit: this description is now confusingly outdated.

> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v5:
> - remove DMA cookie initialization that's now no longer needed
> 
>   drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 57b4f2b37447..7ad993330634 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -20,6 +20,8 @@
>   #include <soc/tegra/ahb.h>
>   #include <soc/tegra/mc.h>
>   
> +#include "dma-iommu.h"
> +
>   struct tegra_smmu_group {
>   	struct list_head list;
>   	struct tegra_smmu *smmu;
> @@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   {
>   	struct tegra_smmu_as *as;
>   
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_DMA &&
> +	    type != IOMMU_DOMAIN_IDENTITY)

Since there's apparently no actual handling of IOMMU_DOMAIN_IDENTITY 
being added anywhere, AFAICS it's still going to set up an address space 
for translation with nothing mapped in its pagetable, which is pretty 
much the opposite of what's required :/

>   		return NULL;
>   
>   	as = kzalloc(sizeof(*as), GFP_KERNEL);
> @@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>   
>   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> -	if (!as->pd) {
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pd)
> +		goto free_as;
>   
>   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
> -	if (!as->count) {
> -		__free_page(as->pd);
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->count)
> +		goto free_pd_range;
>   
>   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
> -	if (!as->pts) {
> -		kfree(as->count);
> -		__free_page(as->pd);
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pts)
> +		goto free_pts;

Nit: all this part is now just unrelated refactoring.

Thanks,
Robin.

>   
>   	spin_lock_init(&as->lock);
>   
> @@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   	as->domain.geometry.force_aperture = true;
>   
>   	return &as->domain;
> +
> +free_pts:
> +	kfree(as->pts);
> +free_pd_range:
> +	__free_page(as->pd);
> +free_as:
> +	kfree(as);
> +
> +	return NULL;
>   }
>   
>   static void tegra_smmu_domain_free(struct iommu_domain *domain)
> @@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
>   	.probe_device = tegra_smmu_probe_device,
>   	.release_device = tegra_smmu_release_device,
>   	.device_group = tegra_smmu_device_group,
> -	.get_resv_regions = of_iommu_get_resv_regions,
> +	.get_resv_regions = iommu_dma_get_resv_regions,
>   	.of_xlate = tegra_smmu_of_xlate,
>   	.pgsize_bitmap = SZ_4K,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {

Re: [PATCH v9 1/5] dt-bindings: reserved-memory: Document iommu-addresses

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
> On 2022-09-23 13:35, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > This adds the "iommu-addresses" property to reserved-memory nodes, which
> > allow describing the interaction of memory regions with IOMMUs. Two use-
> > cases are supported:
> > 
> >    1. Static mappings can be described by pairing the "iommu-addresses"
> >       property with a "reg" property. This is mostly useful for adopting
> >       firmware-allocated buffers via identity mappings. One common use-
> >       case where this is required is if early firmware or bootloaders
> >       have set up a bootsplash framebuffer that a display controller is
> >       actively scanning out from during the operating system boot
> >       process.
> > 
> >    2. If an "iommu-addresses" property exists without a "reg" property,
> >       the reserved-memory node describes an IOVA reservation. Such memory
> >       regions are excluded from the IOVA space available to operating
> >       system drivers and can be used for regions that must not be used to
> >       map arbitrary buffers.
> 
> Bah, I've only just realised: don't we also need to change the "oneOf:
> required: ..." schema to permit "iommu-addresses" without "reg" or "size"?

Hm... good point. I think at least we'll want another:

     - required:
         - iommu-addresses

in there. I wonder if we also need to avoid the combination of "size"
and "iommu-addresses". When "size" is specified, is it guaranteed that
those regions will be allocated before the direct mapping needs to be
created?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 1/5] dt-bindings: reserved-memory: Document iommu-addresses

[Robin Murphy]

On 2022-10-07 14:54, Thierry Reding wrote:
> On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
>> On 2022-09-23 13:35, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> This adds the "iommu-addresses" property to reserved-memory nodes, which
>>> allow describing the interaction of memory regions with IOMMUs. Two use-
>>> cases are supported:
>>>
>>>     1. Static mappings can be described by pairing the "iommu-addresses"
>>>        property with a "reg" property. This is mostly useful for adopting
>>>        firmware-allocated buffers via identity mappings. One common use-
>>>        case where this is required is if early firmware or bootloaders
>>>        have set up a bootsplash framebuffer that a display controller is
>>>        actively scanning out from during the operating system boot
>>>        process.
>>>
>>>     2. If an "iommu-addresses" property exists without a "reg" property,
>>>        the reserved-memory node describes an IOVA reservation. Such memory
>>>        regions are excluded from the IOVA space available to operating
>>>        system drivers and can be used for regions that must not be used to
>>>        map arbitrary buffers.
>>
>> Bah, I've only just realised: don't we also need to change the "oneOf:
>> required: ..." schema to permit "iommu-addresses" without "reg" or "size"?
> 
> Hm... good point. I think at least we'll want another:
> 
>       - required:
>           - iommu-addresses
> 
> in there. I wonder if we also need to avoid the combination of "size"
> and "iommu-addresses". When "size" is specified, is it guaranteed that
> those regions will be allocated before the direct mapping needs to be
> created?

Well, it couldn't really be a direct mapping anyway. In general I don't 
think that combination makes any sense, since the presence of 
"iommu-addresses" means one of two things; either it says the IOVA range 
is carved out for some special purpose or just unusable, in which case 
allocating any memory to back it would surely be pointless, or it's 
saying don't touch these addresses because the device is already 
accessing them, thus the underlying physical memory must be allocated 
somewhere already.

Thanks,
Robin.

Re: [PATCH v9 1/5] dt-bindings: reserved-memory: Document iommu-addresses

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

On Fri, Oct 07, 2022 at 03:21:46PM +0100, Robin Murphy wrote:
> On 2022-10-07 14:54, Thierry Reding wrote:
> > On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
> > > On 2022-09-23 13:35, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > This adds the "iommu-addresses" property to reserved-memory nodes, which
> > > > allow describing the interaction of memory regions with IOMMUs. Two use-
> > > > cases are supported:
> > > > 
> > > >     1. Static mappings can be described by pairing the "iommu-addresses"
> > > >        property with a "reg" property. This is mostly useful for adopting
> > > >        firmware-allocated buffers via identity mappings. One common use-
> > > >        case where this is required is if early firmware or bootloaders
> > > >        have set up a bootsplash framebuffer that a display controller is
> > > >        actively scanning out from during the operating system boot
> > > >        process.
> > > > 
> > > >     2. If an "iommu-addresses" property exists without a "reg" property,
> > > >        the reserved-memory node describes an IOVA reservation. Such memory
> > > >        regions are excluded from the IOVA space available to operating
> > > >        system drivers and can be used for regions that must not be used to
> > > >        map arbitrary buffers.
> > > 
> > > Bah, I've only just realised: don't we also need to change the "oneOf:
> > > required: ..." schema to permit "iommu-addresses" without "reg" or "size"?
> > 
> > Hm... good point. I think at least we'll want another:
> > 
> >       - required:
> >           - iommu-addresses
> > 
> > in there. I wonder if we also need to avoid the combination of "size"
> > and "iommu-addresses". When "size" is specified, is it guaranteed that
> > those regions will be allocated before the direct mapping needs to be
> > created?
> 
> Well, it couldn't really be a direct mapping anyway. In general I don't
> think that combination makes any sense, since the presence of
> "iommu-addresses" means one of two things; either it says the IOVA range is
> carved out for some special purpose or just unusable, in which case
> allocating any memory to back it would surely be pointless, or it's saying
> don't touch these addresses because the device is already accessing them,
> thus the underlying physical memory must be allocated somewhere already.

I thought perhaps there could be cases where it is known that a
controller needs to access memory in a certain I/O virtual region but
doesn't actually care where that lives in physical memory and also does
not rely on that memory have been previously set up (pre-filled, or
whatever). Say you've got a micro-controller in a system that needs its
firmware in a given region, but the OS can set up that region without
any other limitations. One could use "size" and "iommu-addresses" to
make sure the region is allocated with a specific size and located in a
specific I/O virtual region. Not sure if that's perhaps a bit exotic,
though.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 2/5] iommu: Implement of_iommu_get_resv_regions()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7608 bytes --]

On Fri, Oct 07, 2022 at 02:47:23PM +0100, Robin Murphy wrote:
> On 2022-09-23 13:35, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > This is an implementation that IOMMU drivers can use to obtain reserved
> > memory regions from a device tree node. It uses the reserved-memory DT
> > bindings to find the regions associated with a given device. If these
> > regions are marked accordingly, identity mappings will be created for
> > them in the IOMMU domain that the devices will be attached to.
> > 
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v9:
> > - address review comments by Robin Murphy:
> >    - warn about non-direct mappings since they are not supported yet
> >    - cleanup code to require less indentation
> >    - narrow scope of variables
> > 
> > Changes in v8:
> > - cleanup set-but-unused variables
> > 
> > Changes in v6:
> > - remove reference to now unused dt-bindings/reserved-memory.h include
> > 
> > Changes in v5:
> > - update for new "iommu-addresses" device tree bindings
> > 
> > Changes in v4:
> > - fix build failure on !CONFIG_OF_ADDRESS
> > 
> > Changes in v3:
> > - change "active" property to identity mapping flag that is part of the
> >    memory region specifier (as defined by #memory-region-cells) to allow
> >    per-reference flags to be used
> > 
> > Changes in v2:
> > - use "active" property to determine whether direct mappings are needed
> > 
> >   drivers/iommu/of_iommu.c | 104 +++++++++++++++++++++++++++++++++++++++
> >   include/linux/of_iommu.h |   8 +++
> >   2 files changed, 112 insertions(+)
> > 
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 5696314ae69e..0bf2b08bca0a 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -11,6 +11,7 @@
> >   #include <linux/module.h>
> >   #include <linux/msi.h>
> >   #include <linux/of.h>
> > +#include <linux/of_address.h>
> >   #include <linux/of_iommu.h>
> >   #include <linux/of_pci.h>
> >   #include <linux/pci.h>
> > @@ -172,3 +173,106 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> >   	return ops;
> >   }
> > +
> > +static inline bool check_direct_mapping(struct device *dev, struct resource *phys,
> 
> Where "phys" is the virtual address, right? :(

No, phys is actually res passed in from of_iommu_get_resv_regions()
where it is the address read from the "reg" property. So that's the
physical address of the reserved region. Perhaps it'd be useful to
rename "res" to "phys" in that function to be a little more consistent.
It's actually the "start" and "end" values that are passed into this
function that refer to the I/O virtual addresses from iommu-addresses.

> 
> > +					phys_addr_t start, phys_addr_t end)
> > +{
> > +	if (start != phys->start || end != phys->end) {
> > +		dev_warn(dev, "treating non-direct mapping [%pr] -> [%pap-%pap] as reservation\n",
> > +			 &phys, &start, &end);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * of_iommu_get_resv_regions - reserved region driver helper for device tree
> > + * @dev: device for which to get reserved regions
> > + * @list: reserved region list
> > + *
> > + * IOMMU drivers can use this to implement their .get_resv_regions() callback
> > + * for memory regions attached to a device tree node. See the reserved-memory
> > + * device tree bindings on how to use these:
> > + *
> > + *   Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > + */
> > +void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
> > +{
> > +#if IS_ENABLED(CONFIG_OF_ADDRESS)
> > +	struct of_phandle_iterator it;
> > +	int err;
> > +
> > +	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
> > +		const __be32 *maps, *end;
> > +		struct resource res;
> > +		int size;
> > +
> > +		memset(&res, 0, sizeof(res));
> > +
> > +		/*
> > +		 * The "reg" property is optional and can be omitted by reserved-memory regions
> > +		 * that represent reservations in the IOVA space, which are regions that should
> > +		 * not be mapped.
> > +		 */
> > +		if (of_find_property(it.node, "reg", NULL)) {
> > +			err = of_address_to_resource(it.node, 0, &res);
> > +			if (err < 0) {
> > +				dev_err(dev, "failed to parse memory region %pOF: %d\n",
> > +					it.node, err);
> > +				continue;
> > +			}
> > +		}
> > +
> > +		maps = of_get_property(it.node, "iommu-addresses", &size);
> > +		if (!maps)
> > +			continue;
> > +
> > +		end = maps + size / sizeof(__be32);
> > +
> > +		while (maps < end) {
> > +			struct device_node *np;
> > +			u32 phandle;
> > +			int na, ns;
> > +
> > +			phandle = be32_to_cpup(maps++);
> > +			np = of_find_node_by_phandle(phandle);
> > +			na = of_n_addr_cells(np);
> > +			ns = of_n_size_cells(np);
> > +
> > +			if (np == dev->of_node) {
> > +				int prot = IOMMU_READ | IOMMU_WRITE;
> > +				struct iommu_resv_region *region;
> > +				enum iommu_resv_type type;
> > +				phys_addr_t start;
> > +				size_t length;
> > +
> > +				start = of_translate_dma_address(np, maps);
> > +				length = of_read_number(maps + na, ns);
> > +
> > +				/*
> > +				 * IOMMU regions without an associated physical region cannot be
> > +				 * mapped and are simply reservations.
> > +				 */
> > +				if (res.end > res.start) {
> > +					phys_addr_t end = start + length - 1;
> > +
> > +					if (check_direct_mapping(dev, &res, start, end))
> > +						type = IOMMU_RESV_DIRECT_RELAXABLE;
> 
> Again I really don't think we should assume relaxable by default.
> 
> Looking at the shape of things now, it seems like check_direct_mappings()
> wants to subsume the check on res as well and grow in to a more general
> function for determining the iommu_resv_type. Then we've got a clear place
> to start special-casing things like simple-framebuffer that we do know a bit
> more about.

Okay, I think I know where you're going with this. Let me see what I can
come up with.

Thierry

> 
> Thanks,
> Robin.
> 
> > +					else
> > +						type = IOMMU_RESV_RESERVED;
> > +				} else {
> > +					type = IOMMU_RESV_RESERVED;
> > +				}
> > +
> > +				region = iommu_alloc_resv_region(start, length, prot, type);
> > +				if (region)
> > +					list_add_tail(&region->list, list);
> > +			}
> > +
> > +			maps += na + ns;
> > +		}
> > +	}
> > +#endif
> > +}
> > +EXPORT_SYMBOL(of_iommu_get_resv_regions);
> > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> > index 55c1eb300a86..9a5e6b410dd2 100644
> > --- a/include/linux/of_iommu.h
> > +++ b/include/linux/of_iommu.h
> > @@ -12,6 +12,9 @@ extern const struct iommu_ops *of_iommu_configure(struct device *dev,
> >   					struct device_node *master_np,
> >   					const u32 *id);
> > +extern void of_iommu_get_resv_regions(struct device *dev,
> > +				      struct list_head *list);
> > +
> >   #else
> >   static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> > @@ -21,6 +24,11 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> >   	return NULL;
> >   }
> > +static inline void of_iommu_get_resv_regions(struct device *dev,
> > +					     struct list_head *list)
> > +{
> > +}
> > +
> >   #endif	/* CONFIG_OF_IOMMU */
> >   #endif /* __OF_IOMMU_H */

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 5/5] iommu/tegra-smmu: Support managed domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4127 bytes --]

On Fri, Oct 07, 2022 at 02:48:19PM +0100, Robin Murphy wrote:
> On 2022-09-23 13:35, Thierry Reding wrote:
> > From: Navneet Kumar <navneetk@nvidia.com>
> > 
> > Allow creating identity and DMA API compatible IOMMU domains. When
> > creating a DMA API compatible domain, make sure to also create the
> > required cookie.
> 
> Nit: this description is now confusingly outdated.
> 
> > Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v5:
> > - remove DMA cookie initialization that's now no longer needed
> > 
> >   drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
> >   1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index 57b4f2b37447..7ad993330634 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -20,6 +20,8 @@
> >   #include <soc/tegra/ahb.h>
> >   #include <soc/tegra/mc.h>
> > +#include "dma-iommu.h"
> > +
> >   struct tegra_smmu_group {
> >   	struct list_head list;
> >   	struct tegra_smmu *smmu;
> > @@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   {
> >   	struct tegra_smmu_as *as;
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> > +	    type != IOMMU_DOMAIN_DMA &&
> > +	    type != IOMMU_DOMAIN_IDENTITY)
> 
> Since there's apparently no actual handling of IOMMU_DOMAIN_IDENTITY being
> added anywhere, AFAICS it's still going to set up an address space for
> translation with nothing mapped in its pagetable, which is pretty much the
> opposite of what's required :/

Yeah, I think we can safely skip identity domains. I don't think I've
ever seen them get used on Tegra.

> 
> >   		return NULL;
> >   	as = kzalloc(sizeof(*as), GFP_KERNEL);
> > @@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
> >   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> > -	if (!as->pd) {
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->pd)
> > +		goto free_as;
> >   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
> > -	if (!as->count) {
> > -		__free_page(as->pd);
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->count)
> > +		goto free_pd_range;
> >   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
> > -	if (!as->pts) {
> > -		kfree(as->count);
> > -		__free_page(as->pd);
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->pts)
> > +		goto free_pts;
> 
> Nit: all this part is now just unrelated refactoring.

Okay, I'll see if I can pull this into a separate patch. Or perhaps just
drop it.

Frankly, at this point it's unlikely that all of this will ever be
deployed on Tegra210 (and earlier) that used this SMMU, so I've included
this particular patch here primarily because Tegra210 was originally
meant to be the primary target. This patch is now close to 4 years
old...

Thierry

> 
> Thanks,
> Robin.
> 
> >   	spin_lock_init(&as->lock);
> > @@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   	as->domain.geometry.force_aperture = true;
> >   	return &as->domain;
> > +
> > +free_pts:
> > +	kfree(as->pts);
> > +free_pd_range:
> > +	__free_page(as->pd);
> > +free_as:
> > +	kfree(as);
> > +
> > +	return NULL;
> >   }
> >   static void tegra_smmu_domain_free(struct iommu_domain *domain)
> > @@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
> >   	.probe_device = tegra_smmu_probe_device,
> >   	.release_device = tegra_smmu_release_device,
> >   	.device_group = tegra_smmu_device_group,
> > -	.get_resv_regions = of_iommu_get_resv_regions,
> > +	.get_resv_regions = iommu_dma_get_resv_regions,
> >   	.of_xlate = tegra_smmu_of_xlate,
> >   	.pgsize_bitmap = SZ_4K,
> >   	.default_domain_ops = &(const struct iommu_domain_ops) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 1/5] dt-bindings: reserved-memory: Document iommu-addresses

[Robin Murphy]

On 2022-10-07 16:22, Thierry Reding wrote:
> On Fri, Oct 07, 2022 at 03:21:46PM +0100, Robin Murphy wrote:
>> On 2022-10-07 14:54, Thierry Reding wrote:
>>> On Fri, Oct 07, 2022 at 02:45:31PM +0100, Robin Murphy wrote:
>>>> On 2022-09-23 13:35, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> This adds the "iommu-addresses" property to reserved-memory nodes, which
>>>>> allow describing the interaction of memory regions with IOMMUs. Two use-
>>>>> cases are supported:
>>>>>
>>>>>      1. Static mappings can be described by pairing the "iommu-addresses"
>>>>>         property with a "reg" property. This is mostly useful for adopting
>>>>>         firmware-allocated buffers via identity mappings. One common use-
>>>>>         case where this is required is if early firmware or bootloaders
>>>>>         have set up a bootsplash framebuffer that a display controller is
>>>>>         actively scanning out from during the operating system boot
>>>>>         process.
>>>>>
>>>>>      2. If an "iommu-addresses" property exists without a "reg" property,
>>>>>         the reserved-memory node describes an IOVA reservation. Such memory
>>>>>         regions are excluded from the IOVA space available to operating
>>>>>         system drivers and can be used for regions that must not be used to
>>>>>         map arbitrary buffers.
>>>>
>>>> Bah, I've only just realised: don't we also need to change the "oneOf:
>>>> required: ..." schema to permit "iommu-addresses" without "reg" or "size"?
>>>
>>> Hm... good point. I think at least we'll want another:
>>>
>>>        - required:
>>>            - iommu-addresses
>>>
>>> in there. I wonder if we also need to avoid the combination of "size"
>>> and "iommu-addresses". When "size" is specified, is it guaranteed that
>>> those regions will be allocated before the direct mapping needs to be
>>> created?
>>
>> Well, it couldn't really be a direct mapping anyway. In general I don't
>> think that combination makes any sense, since the presence of
>> "iommu-addresses" means one of two things; either it says the IOVA range is
>> carved out for some special purpose or just unusable, in which case
>> allocating any memory to back it would surely be pointless, or it's saying
>> don't touch these addresses because the device is already accessing them,
>> thus the underlying physical memory must be allocated somewhere already.
> 
> I thought perhaps there could be cases where it is known that a
> controller needs to access memory in a certain I/O virtual region but
> doesn't actually care where that lives in physical memory and also does
> not rely on that memory have been previously set up (pre-filled, or
> whatever). Say you've got a micro-controller in a system that needs its
> firmware in a given region, but the OS can set up that region without
> any other limitations. One could use "size" and "iommu-addresses" to
> make sure the region is allocated with a specific size and located in a
> specific I/O virtual region. Not sure if that's perhaps a bit exotic,
> though.

Yeah, that was the closest case I could think of as well, but I'd really 
rather not encourage people to abuse DT that way. If a kernel driver is 
loading firmware and initialising the device from scratch then it should 
be able to sort everything out at runtime without DT involvement. Even 
if the firmware is somehow massive enough to warrant an early dynamic 
carveout rather than a regular page/CMA allocation, there's still no 
good excuse for the driver not to manage its own address space constraints.

On the other hand if the device really does need its firmware at a 
specific hard-coded address than that would need a fixed physical 
reservation anyway, since the DT can't assume that the OS is definitely 
going to use IOMMU translation.

Thanks,
Robin.

Re: [PATCH v9 2/5] iommu: Implement of_iommu_get_resv_regions()

[Robin Murphy]

On 2022-10-07 16:28, Thierry Reding wrote:
[...]
>>> @@ -172,3 +173,106 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>>>    	return ops;
>>>    }
>>> +
>>> +static inline bool check_direct_mapping(struct device *dev, struct resource *phys,
>>
>> Where "phys" is the virtual address, right? :(
> 
> No, phys is actually res passed in from of_iommu_get_resv_regions()
> where it is the address read from the "reg" property. So that's the
> physical address of the reserved region. Perhaps it'd be useful to
> rename "res" to "phys" in that function to be a little more consistent.
> It's actually the "start" and "end" values that are passed into this
> function that refer to the I/O virtual addresses from iommu-addresses.

Oh, so it's the phys_addr_t's that aren't physical addresses - well, it 
had to be wrong one way or the other :)

I agree that s/res/phys/ in the main function, and maybe s/start/iova/ 
too, would be helpful.

Thanks,
Robin.

Re: [PATCH v9 2/5] iommu: Implement of_iommu_get_resv_regions()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6064 bytes --]

On Fri, Sep 23, 2022 at 02:35:54PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This is an implementation that IOMMU drivers can use to obtain reserved
> memory regions from a device tree node. It uses the reserved-memory DT
> bindings to find the regions associated with a given device. If these
> regions are marked accordingly, identity mappings will be created for
> them in the IOMMU domain that the devices will be attached to.
> 
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v9:
> - address review comments by Robin Murphy:
>   - warn about non-direct mappings since they are not supported yet
>   - cleanup code to require less indentation
>   - narrow scope of variables
> 
> Changes in v8:
> - cleanup set-but-unused variables
> 
> Changes in v6:
> - remove reference to now unused dt-bindings/reserved-memory.h include
> 
> Changes in v5:
> - update for new "iommu-addresses" device tree bindings
> 
> Changes in v4:
> - fix build failure on !CONFIG_OF_ADDRESS
> 
> Changes in v3:
> - change "active" property to identity mapping flag that is part of the
>   memory region specifier (as defined by #memory-region-cells) to allow
>   per-reference flags to be used
> 
> Changes in v2:
> - use "active" property to determine whether direct mappings are needed
> 
>  drivers/iommu/of_iommu.c | 104 +++++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h |   8 +++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5696314ae69e..0bf2b08bca0a 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_iommu.h>
>  #include <linux/of_pci.h>
>  #include <linux/pci.h>
> @@ -172,3 +173,106 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>  
>  	return ops;
>  }
> +
> +static inline bool check_direct_mapping(struct device *dev, struct resource *phys,
> +					phys_addr_t start, phys_addr_t end)
> +{
> +	if (start != phys->start || end != phys->end) {
> +		dev_warn(dev, "treating non-direct mapping [%pr] -> [%pap-%pap] as reservation\n",
> +			 &phys, &start, &end);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * of_iommu_get_resv_regions - reserved region driver helper for device tree
> + * @dev: device for which to get reserved regions
> + * @list: reserved region list
> + *
> + * IOMMU drivers can use this to implement their .get_resv_regions() callback
> + * for memory regions attached to a device tree node. See the reserved-memory
> + * device tree bindings on how to use these:
> + *
> + *   Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> + */
> +void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
> +{
> +#if IS_ENABLED(CONFIG_OF_ADDRESS)
> +	struct of_phandle_iterator it;
> +	int err;
> +
> +	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
> +		const __be32 *maps, *end;
> +		struct resource res;
> +		int size;
> +
> +		memset(&res, 0, sizeof(res));
> +
> +		/*
> +		 * The "reg" property is optional and can be omitted by reserved-memory regions
> +		 * that represent reservations in the IOVA space, which are regions that should
> +		 * not be mapped.
> +		 */
> +		if (of_find_property(it.node, "reg", NULL)) {
> +			err = of_address_to_resource(it.node, 0, &res);
> +			if (err < 0) {
> +				dev_err(dev, "failed to parse memory region %pOF: %d\n",
> +					it.node, err);
> +				continue;
> +			}
> +		}
> +
> +		maps = of_get_property(it.node, "iommu-addresses", &size);
> +		if (!maps)
> +			continue;
> +
> +		end = maps + size / sizeof(__be32);
> +
> +		while (maps < end) {
> +			struct device_node *np;
> +			u32 phandle;
> +			int na, ns;
> +
> +			phandle = be32_to_cpup(maps++);
> +			np = of_find_node_by_phandle(phandle);
> +			na = of_n_addr_cells(np);
> +			ns = of_n_size_cells(np);
> +
> +			if (np == dev->of_node) {
> +				int prot = IOMMU_READ | IOMMU_WRITE;
> +				struct iommu_resv_region *region;
> +				enum iommu_resv_type type;
> +				phys_addr_t start;
> +				size_t length;
> +
> +				start = of_translate_dma_address(np, maps);

I just came across an issue when extending the testing from simple-
framebuffer to the full display engine, with the main difference being
that the fill display engine is hooked up both to the IOMMU and to the
memory controller via the interconnects property ("dma-mem").

The latter seems to throw off the of_translate_dma_address() because we
have a top-level bus@0 node that sets #address-cells = <1> and #size-
cells = <1>, which is sufficient to represent the "reg" entries for the
devices. However, for the reserved-memory node needs #address-cells =
<2> and #size-cells = <2> to make sure we can describe memory regions
above the 4 GiB boundary (and potentially larger than 4 GiB, too).

What happens now is that of_translate_dma_address() will find the DMA
parent for the display engine, which is the memory controller, which
also has #address-cells = <2> and #size-cells = <2> for the same reason
as the reserved-memory node. In other words, what this tries to model is
that for DMA accesses, we span more than the 4 GiB range that is
sufficient to address registers for IP blocks.

However, of_translate_dma_address() then ends up getting #address-cells
and #size-cells from the *parent* of the DMA parent. And then everything
falls apart during translation.

Any idea if I'm doing something wrong? Or is the code wrong and it's not
actually using the right cell counts? Should it be using the cell counts
from the DMA parent rather than its parent bus?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 2/5] iommu: Implement of_iommu_get_resv_regions()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 7390 bytes --]

On Wed, Oct 19, 2022 at 08:03:31PM +0200, Thierry Reding wrote:
> On Fri, Sep 23, 2022 at 02:35:54PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > This is an implementation that IOMMU drivers can use to obtain reserved
> > memory regions from a device tree node. It uses the reserved-memory DT
> > bindings to find the regions associated with a given device. If these
> > regions are marked accordingly, identity mappings will be created for
> > them in the IOMMU domain that the devices will be attached to.
> > 
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v9:
> > - address review comments by Robin Murphy:
> >   - warn about non-direct mappings since they are not supported yet
> >   - cleanup code to require less indentation
> >   - narrow scope of variables
> > 
> > Changes in v8:
> > - cleanup set-but-unused variables
> > 
> > Changes in v6:
> > - remove reference to now unused dt-bindings/reserved-memory.h include
> > 
> > Changes in v5:
> > - update for new "iommu-addresses" device tree bindings
> > 
> > Changes in v4:
> > - fix build failure on !CONFIG_OF_ADDRESS
> > 
> > Changes in v3:
> > - change "active" property to identity mapping flag that is part of the
> >   memory region specifier (as defined by #memory-region-cells) to allow
> >   per-reference flags to be used
> > 
> > Changes in v2:
> > - use "active" property to determine whether direct mappings are needed
> > 
> >  drivers/iommu/of_iommu.c | 104 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_iommu.h |   8 +++
> >  2 files changed, 112 insertions(+)
> > 
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 5696314ae69e..0bf2b08bca0a 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/module.h>
> >  #include <linux/msi.h>
> >  #include <linux/of.h>
> > +#include <linux/of_address.h>
> >  #include <linux/of_iommu.h>
> >  #include <linux/of_pci.h>
> >  #include <linux/pci.h>
> > @@ -172,3 +173,106 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> >  
> >  	return ops;
> >  }
> > +
> > +static inline bool check_direct_mapping(struct device *dev, struct resource *phys,
> > +					phys_addr_t start, phys_addr_t end)
> > +{
> > +	if (start != phys->start || end != phys->end) {
> > +		dev_warn(dev, "treating non-direct mapping [%pr] -> [%pap-%pap] as reservation\n",
> > +			 &phys, &start, &end);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * of_iommu_get_resv_regions - reserved region driver helper for device tree
> > + * @dev: device for which to get reserved regions
> > + * @list: reserved region list
> > + *
> > + * IOMMU drivers can use this to implement their .get_resv_regions() callback
> > + * for memory regions attached to a device tree node. See the reserved-memory
> > + * device tree bindings on how to use these:
> > + *
> > + *   Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > + */
> > +void of_iommu_get_resv_regions(struct device *dev, struct list_head *list)
> > +{
> > +#if IS_ENABLED(CONFIG_OF_ADDRESS)
> > +	struct of_phandle_iterator it;
> > +	int err;
> > +
> > +	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
> > +		const __be32 *maps, *end;
> > +		struct resource res;
> > +		int size;
> > +
> > +		memset(&res, 0, sizeof(res));
> > +
> > +		/*
> > +		 * The "reg" property is optional and can be omitted by reserved-memory regions
> > +		 * that represent reservations in the IOVA space, which are regions that should
> > +		 * not be mapped.
> > +		 */
> > +		if (of_find_property(it.node, "reg", NULL)) {
> > +			err = of_address_to_resource(it.node, 0, &res);
> > +			if (err < 0) {
> > +				dev_err(dev, "failed to parse memory region %pOF: %d\n",
> > +					it.node, err);
> > +				continue;
> > +			}
> > +		}
> > +
> > +		maps = of_get_property(it.node, "iommu-addresses", &size);
> > +		if (!maps)
> > +			continue;
> > +
> > +		end = maps + size / sizeof(__be32);
> > +
> > +		while (maps < end) {
> > +			struct device_node *np;
> > +			u32 phandle;
> > +			int na, ns;
> > +
> > +			phandle = be32_to_cpup(maps++);
> > +			np = of_find_node_by_phandle(phandle);
> > +			na = of_n_addr_cells(np);
> > +			ns = of_n_size_cells(np);
> > +
> > +			if (np == dev->of_node) {
> > +				int prot = IOMMU_READ | IOMMU_WRITE;
> > +				struct iommu_resv_region *region;
> > +				enum iommu_resv_type type;
> > +				phys_addr_t start;
> > +				size_t length;
> > +
> > +				start = of_translate_dma_address(np, maps);
> 
> I just came across an issue when extending the testing from simple-
> framebuffer to the full display engine, with the main difference being
> that the fill display engine is hooked up both to the IOMMU and to the
> memory controller via the interconnects property ("dma-mem").
> 
> The latter seems to throw off the of_translate_dma_address() because we
> have a top-level bus@0 node that sets #address-cells = <1> and #size-
> cells = <1>, which is sufficient to represent the "reg" entries for the
> devices. However, for the reserved-memory node needs #address-cells =
> <2> and #size-cells = <2> to make sure we can describe memory regions
> above the 4 GiB boundary (and potentially larger than 4 GiB, too).
> 
> What happens now is that of_translate_dma_address() will find the DMA
> parent for the display engine, which is the memory controller, which
> also has #address-cells = <2> and #size-cells = <2> for the same reason
> as the reserved-memory node. In other words, what this tries to model is
> that for DMA accesses, we span more than the 4 GiB range that is
> sufficient to address registers for IP blocks.
> 
> However, of_translate_dma_address() then ends up getting #address-cells
> and #size-cells from the *parent* of the DMA parent. And then everything
> falls apart during translation.
> 
> Any idea if I'm doing something wrong? Or is the code wrong and it's not
> actually using the right cell counts? Should it be using the cell counts
> from the DMA parent rather than its parent bus?

I came up with the attached patch. This works for my case, but will
abort the DMA parent traversal early on some devices. I'm not sure how
much this would matter in practice.

A safer way would be to create a new variant of __of_get_dma_parent()
that doesn't have the of_get_parent() fallback. That's assuming that we
agree on the concept of having potentially different cell counts, and
effectively DMA busses that are separate from the traditional control
busses in DT.

Do we also need separate DMA cell counts so that one node can be a DMA
bus and a control bus at the same time? Or is this overcomplicating
things and a simpler approach would be to propagate the cell counts all
the way to the top level? I think this all might work with the existing
code if I make bus@0's cell count 2 & 2 for Tegra SoC DTSI files. It's a
lot of churn and seems more like a workaround rather than a correct
model of the busses.

Thierry

[-- Attachment #1.2: 0001-of-Stop-DMA-translation-at-last-DMA-parent.patch --]
[-- Type: text/plain, Size: 2633 bytes --]

From 7f63e7c86fa43f6c7d9254323606daeeb442cf48 Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Thu, 20 Oct 2022 15:21:10 +0200
Subject: [PATCH] of: Stop DMA translation at last DMA parent

DMA parent devices can define separate DMA busses via the "dma-ranges"
and "#address-cells" and "#size-cells" properties. If the DMA bus has
different cell counts than its parent, this can cause the translation
of DMA address to fails (e.g. truncation from 2 to 1 address cells).

Avoid this by stopping to search for DMA parents when a parent without
a "dma-ranges" property is encountered. Also, since it is the DMA parent
that defines the DMA bus, use the bus' cell counts instead of its parent
cell counts.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/address.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 14f137a21b0c..e2f45bdbc41a 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -475,6 +475,7 @@ static u64 __of_translate_address(struct device_node *dev,
 				  const __be32 *in_addr, const char *rprop,
 				  struct device_node **host)
 {
+	bool dma = rprop && !strcmp(rprop, "dma-ranges");
 	struct device_node *parent = NULL;
 	struct of_bus *bus, *pbus;
 	__be32 addr[OF_MAX_ADDR_CELLS];
@@ -494,7 +495,12 @@ static u64 __of_translate_address(struct device_node *dev,
 	bus = of_match_bus(parent);
 
 	/* Count address cells & copy address locally */
-	bus->count_cells(dev, &na, &ns);
+	if (dma) {
+		na = of_bus_n_addr_cells(parent);
+		ns = of_bus_n_size_cells(parent);
+	} else {
+		bus->count_cells(dev, &na, &ns);
+	}
 	if (!OF_CHECK_COUNTS(na, ns)) {
 		pr_debug("Bad cell count for %pOF\n", dev);
 		goto bail;
@@ -515,7 +521,7 @@ static u64 __of_translate_address(struct device_node *dev,
 		parent = get_parent(dev);
 
 		/* If root, we have finished */
-		if (parent == NULL) {
+		if (parent == NULL || (dma && !of_get_property(parent, "dma-ranges", NULL))) {
 			pr_debug("reached root node\n");
 			result = of_read_number(addr, na);
 			break;
@@ -536,7 +542,12 @@ static u64 __of_translate_address(struct device_node *dev,
 
 		/* Get new parent bus and counts */
 		pbus = of_match_bus(parent);
-		pbus->count_cells(dev, &pna, &pns);
+		if (dma) {
+			pna = of_bus_n_addr_cells(parent);
+			pns = of_bus_n_size_cells(parent);
+		} else {
+			pbus->count_cells(dev, &pna, &pns);
+		}
 		if (!OF_CHECK_COUNTS(pna, pns)) {
 			pr_err("Bad cell count for %pOF\n", dev);
 			break;
-- 
2.37.3


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]