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

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

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

this is another attempt at solving the problem of passing IOMMU
configuration via device tree. It has significantly evolved since the
last attempt, based on the discussion that followed. The discussion can
be found here:

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

Rather than using a memory-region specifier, this new version introduces
a new "iommu-addresses" property for the reserved-memory regions
themselves. These are used to describe either a static mapping or
reservation that should be created for a given device. If both "reg" and
"iommu-addresses" properties are given, a mapping will be created
(typically this would be an identity mapping) whereas if only an
"iommu-addresses" property is specified, a reservation for the specified
range will be installed.

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.

Adding Alyssa and Janne who have in the past tried to make these
bindings work on Apple M1. Also adding Sameer from the Tegra audio team
to look at the ADSP reservation and double-check that this is suitable
for our needs.

Thierry

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.txt       |  1 -
 .../reserved-memory/reserved-memory.yaml      | 62 +++++++++++++
 drivers/iommu/dma-iommu.c                     |  3 +
 drivers/iommu/of_iommu.c                      | 90 +++++++++++++++++++
 drivers/iommu/tegra-smmu.c                    | 82 +++++++++++++----
 include/dt-bindings/reserved-memory.h         |  8 ++
 include/linux/of_iommu.h                      |  8 ++
 7 files changed, 235 insertions(+), 19 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
 create mode 100644 include/dt-bindings/reserved-memory.h

-- 
2.36.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 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.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../reserved-memory/reserved-memory.txt       |  1 -
 .../reserved-memory/reserved-memory.yaml      | 62 +++++++++++++++++++
 include/dt-bindings/reserved-memory.h         |  8 +++
 3 files changed, 70 insertions(+), 1 deletion(-)
 delete mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
 create mode 100644 include/dt-bindings/reserved-memory.h

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
deleted file mode 100644
index 1810701a8509..000000000000
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+++ /dev/null
@@ -1 +0,0 @@
-This file has been moved to reserved-memory.yaml.
diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
index 7a0744052ff6..3a769aa66e1c 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,42 @@ oneOf:
 
 additionalProperties: true
 
+examples:
+  - |
+    reserved-memory {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      ranges;
+
+      adsp: 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 = <0x0 0x00000000 0x00 0x40000000>,
+                          <0x0 0x60000000 0xff 0xa0000000>;
+      };
+
+      fb: framebuffer@90000000 {
+        reg = <0x0 0x90000000 0x0 0x00800000>;
+        iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
+      };
+    };
+
+    bus@0 {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      adsp@2990000 {
+        reg = <0x0 0x2990000 0x0 0x2000>;
+        memory-region = <&adsp>;
+      };
+
+      display@15200000 {
+        reg = <0x0 0x15200000 0x0 0x10000>;
+        memory-region = <&fb>;
+      };
+    };
+
 ...
diff --git a/include/dt-bindings/reserved-memory.h b/include/dt-bindings/reserved-memory.h
new file mode 100644
index 000000000000..174ca3448342
--- /dev/null
+++ b/include/dt-bindings/reserved-memory.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+
+#ifndef _DT_BINDINGS_RESERVED_MEMORY_H
+#define _DT_BINDINGS_RESERVED_MEMORY_H
+
+#define MEMORY_REGION_IDENTITY_MAPPING 0x1
+
+#endif
-- 
2.36.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 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.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
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 | 90 ++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h |  8 ++++
 2 files changed, 98 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5696314ae69e..9e341b5e307f 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -11,12 +11,15 @@
 #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>
 #include <linux/slab.h>
 #include <linux/fsl/mc.h>
 
+#include <dt-bindings/reserved-memory.h>
+
 #define NO_IOMMU	1
 
 static int of_iommu_xlate(struct device *dev,
@@ -172,3 +175,90 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 
 	return ops;
 }
+
+/**
+ * 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) {
+		struct iommu_resv_region *region;
+		struct resource res;
+		const __be32 *maps;
+		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) {
+			const __be32 *end = maps + size / sizeof(__be32);
+			struct device_node *np;
+			unsigned int index = 0;
+			u32 phandle;
+			int na, ns;
+
+			while (maps < end) {
+				phys_addr_t start, end;
+				size_t length;
+
+				phandle = be32_to_cpup(maps++);
+				np = of_find_node_by_phandle(phandle);
+				na = of_n_addr_cells(np);
+				ns = of_n_size_cells(np);
+
+				start = of_translate_dma_address(np, maps);
+				length = of_read_number(maps + na, ns);
+				end = start + length - 1;
+
+				if (np == dev->of_node) {
+					int prot = IOMMU_READ | IOMMU_WRITE;
+					enum iommu_resv_type type;
+
+					/*
+					 * IOMMU regions without an associated physical region
+					 * cannot be mapped and are simply reservations.
+					 */
+					if (res.end > res.start)
+						type = IOMMU_RESV_DIRECT_RELAXABLE;
+					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;
+				index++;
+			}
+		}
+	}
+#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.36.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 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.

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 1ca85d37eeab..3a40ae7a450b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -22,6 +22,7 @@
 #include <linux/irq.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>
@@ -386,6 +387,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_msi_get_resv_regions(dev, list);
 
+	if (dev->of_node)
+		of_iommu_get_resv_regions(dev, list);
 }
 EXPORT_SYMBOL(iommu_dma_get_resv_regions);
 
-- 
2.36.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 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 | 47 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 2f2b12033618..93879c40056c 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,7 +903,9 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 	return &smmu->iommu;
 }
 
-static void tegra_smmu_release_device(struct device *dev) {}
+static void tegra_smmu_release_device(struct device *dev)
+{
+}
 
 static const struct tegra_smmu_group_soc *
 tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
@@ -968,6 +1009,8 @@ 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,
+	.put_resv_regions = generic_iommu_put_resv_regions,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.36.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v5 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 | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 93879c40056c..f8b2b863c111 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-iommu.h>
 
 #include <soc/tegra/ahb.h>
 #include <soc/tegra/mc.h>
@@ -277,7 +278,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 +290,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 +309,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)
@@ -1009,7 +1012,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,
 	.put_resv_regions = generic_iommu_put_resv_regions,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
-- 
2.36.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Rob Herring]

On Thu, 12 May 2022 21:00:48 +0200, 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.
> 
> 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.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../reserved-memory/reserved-memory.txt       |  1 -
>  .../reserved-memory/reserved-memory.yaml      | 62 +++++++++++++++++++
>  include/dt-bindings/reserved-memory.h         |  8 +++
>  3 files changed, 70 insertions(+), 1 deletion(-)
>  delete mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>  create mode 100644 include/dt-bindings/reserved-memory.h
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/reserved-memory/reserved-memory.example.dts:21.11-18: Warning (ranges_format): /example-0/reserved-memory:ranges: empty "ranges" property but its #address-cells (2) differs from /example-0 (1)
Documentation/devicetree/bindings/reserved-memory/reserved-memory.example.dts:21.11-18: Warning (ranges_format): /example-0/reserved-memory:ranges: empty "ranges" property but its #size-cells (2) differs from /example-0 (1)
Documentation/devicetree/bindings/reserved-memory/reserved-memory.example.dts:39.15-52.11: Warning (unit_address_vs_reg): /example-0/bus@0: node has a unit name, but no reg or ranges property

doc reference errors (make refcheckdocs):
Documentation/devicetree/bindings/display/arm,hdlcd.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/display/arm,komeda.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/display/arm,malidp.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/display/arm,pl11x.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/gpu/aspeed-gfx.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/media/amphion,vpu.yaml: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/media/aspeed-video.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/media/mediatek-vpu.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/remoteproc/ti,davinci-rproc.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/remoteproc/ti,keystone-rproc.txt: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
Documentation/devicetree/bindings/remoteproc/ti,omap-remoteproc.yaml: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
kernel/dma/Kconfig: Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Janne Grunau]

Hej,

I'm working on the display controller for Apple silicon SoCs and will 
add some comments with support for it in mind.

added asahi@lists.linux.dev to CC for the Apple silicon related aspects

On 2022-05-12 21:00:47 +0200, Thierry Reding wrote:
> 
> this is another attempt at solving the problem of passing IOMMU
> configuration via device tree. It has significantly evolved since the
> last attempt, based on the discussion that followed. The discussion can
> be found here:
> 
>   https://lore.kernel.org/all/20210423163234.3651547-1-thierry.reding@gmail.com/
> 
> Rather than using a memory-region specifier, this new version introduces
> a new "iommu-addresses" property for the reserved-memory regions
> themselves.

If experimented with both proposed bindings for dcp and I think this 
binding is easer to understand and to work with.

> These are used to describe either a static mapping or
> reservation that should be created for a given device. If both "reg" and
> "iommu-addresses" properties are given, a mapping will be created
> (typically this would be an identity mapping)

dcp on Apple silicon will not use identity mappings. The IOMMU supports 
identity mapping but the pre-configured mappings setup by Apple's system 
firmware will not work with identity mapping. It maps multiple regions 
which are incompatible with a linear identity mapping. In addition the 
embbeded aarch64 micro controllers used in the display subsystem appears 
to use a heap mapped at low IOVA space starting at 0.

> whereas if only an "iommu-addresses" property is specified, a 
> reservation for the specified range will be installed.
> 
> 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>;
> 		};

The binding supports mapping the same region to multiple devices. The 
code supports that and it will be used on Apple silicon. Not necessary 
to extend and complicate the example for I wanted to mention it 
explicitly.

> 
> 		/*
> 		 * 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.
> 
> Adding Alyssa and Janne who have in the past tried to make these
> bindings work on Apple M1. Also adding Sameer from the Tegra audio team
> to look at the ADSP reservation and double-check that this is suitable
> for our needs.

The binding itself is sufficient for the needs of the display subsystem 
on Apple silicon. The device tree parsing code for reserved regions is 
of limited use in it's current form. We will have either to extend or 
duplicate it to retrieve the non-identity mappings. That's our problem 
to solve.

Janne
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Janne Grunau]

On 2022-05-12 21:00:48 +0200, 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.
> 
> 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.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../reserved-memory/reserved-memory.txt       |  1 -
>  .../reserved-memory/reserved-memory.yaml      | 62 +++++++++++++++++++
>  include/dt-bindings/reserved-memory.h         |  8 +++
>  3 files changed, 70 insertions(+), 1 deletion(-)
>  delete mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>  create mode 100644 include/dt-bindings/reserved-memory.h
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> deleted file mode 100644
> index 1810701a8509..000000000000
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ /dev/null
> @@ -1 +0,0 @@
> -This file has been moved to reserved-memory.yaml.
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> index 7a0744052ff6..3a769aa66e1c 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,42 @@ oneOf:
>  
>  additionalProperties: true
>  
> +examples:
> +  - |
> +    reserved-memory {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      ranges;
> +
> +      adsp: 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 = <0x0 0x00000000 0x00 0x40000000>,
> +                          <0x0 0x60000000 0xff 0xa0000000>;

This misses the device's phandle. One could argue it's not necessary for 
reservations but it will complicate the parsing code and the current 
parsing code is not prepared for it.

> +      };
> +
> +      fb: framebuffer@90000000 {
> +        reg = <0x0 0x90000000 0x0 0x00800000>;
> +        iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
> +      };
> +    };
> +
> +    bus@0 {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      adsp@2990000 {
> +        reg = <0x0 0x2990000 0x0 0x2000>;
> +        memory-region = <&adsp>;
> +      };
> +
> +      display@15200000 {
> +        reg = <0x0 0x15200000 0x0 0x10000>;
> +        memory-region = <&fb>;
> +      };
> +    };
> +
>  ...
> diff --git a/include/dt-bindings/reserved-memory.h b/include/dt-bindings/reserved-memory.h
> new file mode 100644
> index 000000000000..174ca3448342
> --- /dev/null
> +++ b/include/dt-bindings/reserved-memory.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
> +
> +#ifndef _DT_BINDINGS_RESERVED_MEMORY_H
> +#define _DT_BINDINGS_RESERVED_MEMORY_H
> +
> +#define MEMORY_REGION_IDENTITY_MAPPING 0x1
> +
> +#endif

This appears to be an unused leftover from a previous version.

Janne
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Janne Grunau]

On 2022-05-12 21:00:49 +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.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> 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 | 90 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h |  8 ++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5696314ae69e..9e341b5e307f 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -11,12 +11,15 @@
>  #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>
>  #include <linux/slab.h>
>  #include <linux/fsl/mc.h>
>  
> +#include <dt-bindings/reserved-memory.h>
> +
>  #define NO_IOMMU	1
>  
>  static int of_iommu_xlate(struct device *dev,
> @@ -172,3 +175,90 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>  
>  	return ops;
>  }
> +
> +/**
> + * 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) {
> +		struct iommu_resv_region *region;
> +		struct resource res;
> +		const __be32 *maps;
> +		int size;

Adding 'if (!of_device_is_available(it.node)) continue;' here would help 
backwards compatibility. My plan was to add the reserved regions with 
"iommu-addresses" with all zero adresses and sizes with status = 
"disabled" to the devicetree. A bootloader update is required to fill 
those.

> +
> +		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) {
> +			const __be32 *end = maps + size / sizeof(__be32);
> +			struct device_node *np;
> +			unsigned int index = 0;
> +			u32 phandle;
> +			int na, ns;
> +
> +			while (maps < end) {
> +				phys_addr_t start, end;
> +				size_t length;
> +
> +				phandle = be32_to_cpup(maps++);
> +				np = of_find_node_by_phandle(phandle);
> +				na = of_n_addr_cells(np);
> +				ns = of_n_size_cells(np);
> +
> +				start = of_translate_dma_address(np, maps);
> +				length = of_read_number(maps + na, ns);

alternatively we could handle mappings/reservations with length 0 as 
error and skip them.

> +				end = start + length - 1;
> +
> +				if (np == dev->of_node) {
> +					int prot = IOMMU_READ | IOMMU_WRITE;
> +					enum iommu_resv_type type;
> +
> +					/*
> +					 * IOMMU regions without an associated physical region
> +					 * cannot be mapped and are simply reservations.
> +					 */
> +					if (res.end > res.start)
> +						type = IOMMU_RESV_DIRECT_RELAXABLE;
> +					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;
> +				index++;
> +			}
> +		}
> +	}
> +#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.36.1
> 

Janne
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Dmitry Osipenko]

On 5/12/22 22:00, Thierry Reding wrote:
> -277,7 +278,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;

Shouldn't at least pre-210 SoCs be guarded from IOMMU_DOMAIN_DMA? I
don't think that DRM and VDE drivers will work as-is today.

-- 
Best regards,
Dmitry
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Thierry Reding]

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

On Sun, May 15, 2022 at 01:10:38PM +0200, Janne Grunau wrote:
> On 2022-05-12 21:00:49 +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.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > 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 | 90 ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_iommu.h |  8 ++++
> >  2 files changed, 98 insertions(+)
> > 
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 5696314ae69e..9e341b5e307f 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -11,12 +11,15 @@
> >  #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>
> >  #include <linux/slab.h>
> >  #include <linux/fsl/mc.h>
> >  
> > +#include <dt-bindings/reserved-memory.h>
> > +
> >  #define NO_IOMMU	1
> >  
> >  static int of_iommu_xlate(struct device *dev,
> > @@ -172,3 +175,90 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> >  
> >  	return ops;
> >  }
> > +
> > +/**
> > + * 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) {
> > +		struct iommu_resv_region *region;
> > +		struct resource res;
> > +		const __be32 *maps;
> > +		int size;
> 
> Adding 'if (!of_device_is_available(it.node)) continue;' here would help 
> backwards compatibility. My plan was to add the reserved regions with 
> "iommu-addresses" with all zero adresses and sizes with status = 
> "disabled" to the devicetree. A bootloader update is required to fill 
> those.

Yes, good point. My plan was originally to have the bootloader/firmware
generate these nodes in their entirety, but yeah, prepopulating them and
having firmware just fill in updated values and setting status = "okay"
seems reasonable to me.

> > +
> > +		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) {
> > +			const __be32 *end = maps + size / sizeof(__be32);
> > +			struct device_node *np;
> > +			unsigned int index = 0;
> > +			u32 phandle;
> > +			int na, ns;
> > +
> > +			while (maps < end) {
> > +				phys_addr_t start, end;
> > +				size_t length;
> > +
> > +				phandle = be32_to_cpup(maps++);
> > +				np = of_find_node_by_phandle(phandle);
> > +				na = of_n_addr_cells(np);
> > +				ns = of_n_size_cells(np);
> > +
> > +				start = of_translate_dma_address(np, maps);
> > +				length = of_read_number(maps + na, ns);
> 
> alternatively we could handle mappings/reservations with length 0 as 
> error and skip them.

I think we could do both.

Thanks for the feedback,
Thierry

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

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Thierry Reding]

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

On Sun, May 15, 2022 at 12:45:54PM +0200, Janne Grunau wrote:
> On 2022-05-12 21:00:48 +0200, 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.
> > 
> > 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.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../reserved-memory/reserved-memory.txt       |  1 -
> >  .../reserved-memory/reserved-memory.yaml      | 62 +++++++++++++++++++
> >  include/dt-bindings/reserved-memory.h         |  8 +++
> >  3 files changed, 70 insertions(+), 1 deletion(-)
> >  delete mode 100644 Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >  create mode 100644 include/dt-bindings/reserved-memory.h
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > deleted file mode 100644
> > index 1810701a8509..000000000000
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +++ /dev/null
> > @@ -1 +0,0 @@
> > -This file has been moved to reserved-memory.yaml.
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
> > index 7a0744052ff6..3a769aa66e1c 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,42 @@ oneOf:
> >  
> >  additionalProperties: true
> >  
> > +examples:
> > +  - |
> > +    reserved-memory {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +      ranges;
> > +
> > +      adsp: 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 = <0x0 0x00000000 0x00 0x40000000>,
> > +                          <0x0 0x60000000 0xff 0xa0000000>;
> 
> This misses the device's phandle. One could argue it's not necessary for 
> reservations but it will complicate the parsing code and the current 
> parsing code is not prepared for it.

Ugh... I evidently messed this one up. It's not ever close to what I had
in my device trees for testing. I'll make sure to fix those up properly
and also that they pass validation.

> > +      };
> > +
> > +      fb: framebuffer@90000000 {
> > +        reg = <0x0 0x90000000 0x0 0x00800000>;
> > +        iommu-addresses = <&dc0 0x0 0x90000000 0x0 0x00800000>;
> > +      };
> > +    };
> > +
> > +    bus@0 {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      adsp@2990000 {
> > +        reg = <0x0 0x2990000 0x0 0x2000>;
> > +        memory-region = <&adsp>;
> > +      };
> > +
> > +      display@15200000 {
> > +        reg = <0x0 0x15200000 0x0 0x10000>;
> > +        memory-region = <&fb>;
> > +      };
> > +    };
> > +
> >  ...
> > diff --git a/include/dt-bindings/reserved-memory.h b/include/dt-bindings/reserved-memory.h
> > new file mode 100644
> > index 000000000000..174ca3448342
> > --- /dev/null
> > +++ b/include/dt-bindings/reserved-memory.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
> > +
> > +#ifndef _DT_BINDINGS_RESERVED_MEMORY_H
> > +#define _DT_BINDINGS_RESERVED_MEMORY_H
> > +
> > +#define MEMORY_REGION_IDENTITY_MAPPING 0x1
> > +
> > +#endif
> 
> This appears to be an unused leftover from a previous version.

Good catch. I'll drop this file from the patch set. It's no longer
needed.

Thanks,
Thierry

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

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Thierry Reding]

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

On Sun, May 15, 2022 at 12:35:44PM +0200, Janne Grunau wrote:
> Hej,
> 
> I'm working on the display controller for Apple silicon SoCs and will 
> add some comments with support for it in mind.
> 
> added asahi@lists.linux.dev to CC for the Apple silicon related aspects
> 
> On 2022-05-12 21:00:47 +0200, Thierry Reding wrote:
> > 
> > this is another attempt at solving the problem of passing IOMMU
> > configuration via device tree. It has significantly evolved since the
> > last attempt, based on the discussion that followed. The discussion can
> > be found here:
> > 
> >   https://lore.kernel.org/all/20210423163234.3651547-1-thierry.reding@gmail.com/
> > 
> > Rather than using a memory-region specifier, this new version introduces
> > a new "iommu-addresses" property for the reserved-memory regions
> > themselves.
> 
> If experimented with both proposed bindings for dcp and I think this 
> binding is easer to understand and to work with.
> 
> > These are used to describe either a static mapping or
> > reservation that should be created for a given device. If both "reg" and
> > "iommu-addresses" properties are given, a mapping will be created
> > (typically this would be an identity mapping)
> 
> dcp on Apple silicon will not use identity mappings. The IOMMU supports 
> identity mapping but the pre-configured mappings setup by Apple's system 
> firmware will not work with identity mapping. It maps multiple regions 
> which are incompatible with a linear identity mapping. In addition the 
> embbeded aarch64 micro controllers used in the display subsystem appears 
> to use a heap mapped at low IOVA space starting at 0.
> 
> > whereas if only an "iommu-addresses" property is specified, a 
> > reservation for the specified range will be installed.
> > 
> > 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>;
> > 		};
> 
> The binding supports mapping the same region to multiple devices. The 
> code supports that and it will be used on Apple silicon. Not necessary 
> to extend and complicate the example for I wanted to mention it 
> explicitly.
> 
> > 
> > 		/*
> > 		 * 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.
> > 
> > Adding Alyssa and Janne who have in the past tried to make these
> > bindings work on Apple M1. Also adding Sameer from the Tegra audio team
> > to look at the ADSP reservation and double-check that this is suitable
> > for our needs.
> 
> The binding itself is sufficient for the needs of the display subsystem 
> on Apple silicon. The device tree parsing code for reserved regions is 
> of limited use in it's current form. We will have either to extend or 
> duplicate it to retrieve the non-identity mappings. That's our problem 
> to solve.

I had looked at it a bit to see if I could easily implement that, but
the direct mapping support in the IOMMU subsystem currently only
supports either reservations or identity mappings, so arbitrary mappings
would either have to be added to that code, or it would have to take a
different code path that basically goes through the same steps, except
that it uses different physical and I/O virtual addresses.

The easiest, I think, would be for struct iommu_resv_region to be
extended with a pair of start/length fields for the I/O virtual address
and then the rest of the code should mostly work. This shouldn't even be
very invasive, maybe just adding a version of iommu_alloc_resv_region()
that takes the I/O virtual addresses as additional parameters.

Come to think of it, the current code could probably be improved a bit
by checking if the addresses in the reg and iommu-addresses properties
match. Currently the code just ignores the reserved memory region's
"reg" property, so one could technically set up a mapping that points to
physical memory that the device doesn't "own".

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu