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

[PATCH v10 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 v9, which can be found
here:

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

Most notably this addresses an issue with DMA address translations
failing across bus boundaries with mismatching #address-cells (2 vs. 1)
properties. The proposed fix is to treat DMA parents as creating a DMA
bus that is separate from the control bus, and therefore stop address
translation for DMA at the last DMA parent.

Other than that this includes minor fixups to the DT bindings as well as
some rework to the OF/IOMMU core code to be more easily extensible in
the future.

I've also dropped the tegra-smmu patches for now since they have
accumulated some bitrot and probably need more revision and testing.

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/

Thierry Reding (5):
  of: Introduce of_translate_dma_region()
  of: Stop DMA translation at last DMA parent
  dt-bindings: reserved-memory: Document iommu-addresses
  iommu: Implement of_iommu_get_resv_regions()
  iommu: dma: Use of_iommu_get_resv_regions()

 .../reserved-memory/reserved-memory.yaml      | 73 ++++++++++++++
 drivers/iommu/dma-iommu.c                     |  3 +
 drivers/iommu/of_iommu.c                      | 94 +++++++++++++++++++
 drivers/of/address.c                          | 58 +++++++++++-
 include/linux/of_address.h                    |  2 +
 include/linux/of_iommu.h                      |  8 ++
 6 files changed, 235 insertions(+), 3 deletions(-)

-- 
2.38.1

[PATCH v10 1/5] of: Introduce of_translate_dma_region()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function is similar to of_translate_dma_address() but also reads a
length in addition to an address from a device tree property.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v10:
- new patch

 drivers/of/address.c       | 41 ++++++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index c34ac33b7338..14f137a21b0c 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -626,6 +626,47 @@ u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr)
 }
 EXPORT_SYMBOL(of_translate_dma_address);
 
+/**
+ * of_translate_dma_region - Translate device tree address and size tuple
+ * @dev: device tree node for which to translate
+ * @prop: pointer into array of cells
+ * @start: return value for the start of the DMA range
+ * @length: return value for the length of the DMA range
+ *
+ * Returns a pointer to the cell immediately following the translated DMA region.
+ */
+const __be32 *of_translate_dma_region(struct device_node *dev, const __be32 *prop,
+				      phys_addr_t *start, size_t *length)
+{
+	struct device_node *parent;
+	u64 address, size;
+	int na, ns;
+
+	parent = __of_get_dma_parent(dev);
+	if (!parent)
+		return NULL;
+
+	na = of_bus_n_addr_cells(parent);
+	ns = of_bus_n_size_cells(parent);
+
+	of_node_put(parent);
+
+	address = of_translate_dma_address(dev, prop);
+	if (address == OF_BAD_ADDR)
+		return NULL;
+
+	size = of_read_number(prop + na, ns);
+
+	if (start)
+		*start = address;
+
+	if (length)
+		*length = size;
+
+	return prop + na + ns;
+}
+EXPORT_SYMBOL(of_translate_dma_region);
+
 const __be32 *__of_get_address(struct device_node *dev, int index, int bar_no,
 			       u64 *size, unsigned int *flags)
 {
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 265f26eeaf6b..376671594746 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -38,6 +38,8 @@ struct of_pci_range {
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);
+extern const __be32 *of_translate_dma_region(struct device_node *dev, const __be32 *addr,
+					     phys_addr_t *start, size_t *length);
 
 #ifdef CONFIG_OF_ADDRESS
 extern u64 of_translate_address(struct device_node *np, const __be32 *addr);
-- 
2.38.1

[PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

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>
---
Changes in v10:
- new patch to avoid address truncation when traversing a bus hierarchy
  with mismatching #address-cells properties

Example from Tegra194 (redacted for clarity):

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

		framebuffer@0,0 {
			compatible = "framebuffer";
			reg = <0x2 0x57320000 0x0 0x00800000>;
			iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>;
		};
	};

	bus@0 {
		/* truncation happens here */
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0x0 0x0 0x0 0x40000000>;

		mc: memory-controller@2c00000 {
			#address-cells = <2>;
			#size-cells = <2>;

			/*
			 * memory controller provides access to 512 GiB
			 * of system RAM (root of the DMA bus)
			 */
			dma-ranges = <0x0 0x0 0x0 0x80 0x0>;
		};

		host1x@13e00000 {
			display-hub@15200000 {
				display@15200000 {
					interconnect-names = "dma-mem", ...;
					interconnects = <&mc ...>;
					memory-region = <&fb>;
				};
			};
		};
	};

During DMA address translation, the framebuffer address (0x257320000)
will first be translated to the DMA parent's DMA bus, which yields the
same value. After that, the current translation code will switch to the
control bus of bus@0 and then the address will be truncated to
0x57320000 due to #address-cells = <1>.

The idea of this patch is to interrupt DMA address translation at &mc
because it is the root of the DMA bus (i.e. its parent does not have a
dma-ranges property) so that the control bus' #address-cells doesn't
truncate the DMA address.

 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.38.1

Re: [PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Rob Herring]

On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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).

My assumption in this case was that the parent cell sizes should be 
increased to 2 cells. That tends to be what people want to do anyways 
(64-bit everywhere on 64-bit CPUs).

> 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.

We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec 
even says that because I hit that case.

Is this going to work for 'dma-device' with something like this?:

  bus@0 {
    dma-ranges = <...>;
    child-bus@... {
      dma-device@... {
      };
    };
  };

> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v10:
> - new patch to avoid address truncation when traversing a bus hierarchy
>   with mismatching #address-cells properties
> 
> Example from Tegra194 (redacted for clarity):
> 
> 	reserved-memory {
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		ranges;
> 
> 		framebuffer@0,0 {
> 			compatible = "framebuffer";
> 			reg = <0x2 0x57320000 0x0 0x00800000>;
> 			iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>;
> 		};
> 	};
> 
> 	bus@0 {
> 		/* truncation happens here */
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0x0 0x0 0x0 0x40000000>;
> 
> 		mc: memory-controller@2c00000 {
> 			#address-cells = <2>;
> 			#size-cells = <2>;

I think this is wrong. The parent should have more or equal number of 
cells.


> 			/*
> 			 * memory controller provides access to 512 GiB
> 			 * of system RAM (root of the DMA bus)
> 			 */
> 			dma-ranges = <0x0 0x0 0x0 0x80 0x0>;
> 		};
> 
> 		host1x@13e00000 {
> 			display-hub@15200000 {
> 				display@15200000 {
> 					interconnect-names = "dma-mem", ...;
> 					interconnects = <&mc ...>;
> 					memory-region = <&fb>;
> 				};
> 			};
> 		};
> 	};
> 
> During DMA address translation, the framebuffer address (0x257320000)
> will first be translated to the DMA parent's DMA bus, which yields the
> same value. After that, the current translation code will switch to the
> control bus of bus@0 and then the address will be truncated to
> 0x57320000 due to #address-cells = <1>.
> 
> The idea of this patch is to interrupt DMA address translation at &mc
> because it is the root of the DMA bus (i.e. its parent does not have a
> dma-ranges property) so that the control bus' #address-cells doesn't
> truncate the DMA address.
> 
>  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.38.1
> 
> 

Re: [PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Thierry Reding]

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

On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote:
> On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > 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).
> 
> My assumption in this case was that the parent cell sizes should be 
> increased to 2 cells. That tends to be what people want to do anyways 
> (64-bit everywhere on 64-bit CPUs).
> 
> > 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.
> 
> We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec 
> even says that because I hit that case.
> 
> Is this going to work for 'dma-device' with something like this?:
> 
>   bus@0 {
>     dma-ranges = <...>;
>     child-bus@... {
>       dma-device@... {
>       };
>     };
>   };
> 
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v10:
> > - new patch to avoid address truncation when traversing a bus hierarchy
> >   with mismatching #address-cells properties
> > 
> > Example from Tegra194 (redacted for clarity):
> > 
> > 	reserved-memory {
> > 		#address-cells = <2>;
> > 		#size-cells = <2>;
> > 		ranges;
> > 
> > 		framebuffer@0,0 {
> > 			compatible = "framebuffer";
> > 			reg = <0x2 0x57320000 0x0 0x00800000>;
> > 			iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>;
> > 		};
> > 	};
> > 
> > 	bus@0 {
> > 		/* truncation happens here */
> > 		#address-cells = <1>;
> > 		#size-cells = <1>;
> > 		ranges = <0x0 0x0 0x0 0x40000000>;
> > 
> > 		mc: memory-controller@2c00000 {
> > 			#address-cells = <2>;
> > 			#size-cells = <2>;
> 
> I think this is wrong. The parent should have more or equal number of 
> cells.

I was half suspecting that. The reason why I hesitated is that I recall
having the opposite discussion a while ago when we were adding bus@0 to
64-bit Tegra devices. We had at some point (probably around Tegra114 or
Tegra124, 32-bit ARM chips that support LPAE) started to set #address-
cells = <2> precisely because the CPU could address more than 32-bit
addresses. We then did the same thing transitioning to 64-bit ARM. When
we then started discussing bus@0, someone (might have been you) had
argued that all these peripherals could be addressed with a single cell
so there'd be no need for #address-cells = <2>, so then we went with
that.

Reverting back to #address-cells = <2> is now going to cause quite a bit
of churn, but I guess if it's the right thing, so be it.

Another possible alternative would be to move the memory-controller node
from the bus@0 to the top-level. Not sure if that's any better.

Thierry

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

Re: [PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Rob Herring]

On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote:
> > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > 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).
> >
> > My assumption in this case was that the parent cell sizes should be
> > increased to 2 cells. That tends to be what people want to do anyways
> > (64-bit everywhere on 64-bit CPUs).
> >
> > > 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.
> >
> > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec
> > even says that because I hit that case.
> >
> > Is this going to work for 'dma-device' with something like this?:
> >
> >   bus@0 {
> >     dma-ranges = <...>;
> >     child-bus@... {
> >       dma-device@... {
> >       };
> >     };
> >   };
> >
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > > Changes in v10:
> > > - new patch to avoid address truncation when traversing a bus hierarchy
> > >   with mismatching #address-cells properties
> > >
> > > Example from Tegra194 (redacted for clarity):
> > >
> > >     reserved-memory {
> > >             #address-cells = <2>;
> > >             #size-cells = <2>;
> > >             ranges;
> > >
> > >             framebuffer@0,0 {
> > >                     compatible = "framebuffer";
> > >                     reg = <0x2 0x57320000 0x0 0x00800000>;
> > >                     iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>;
> > >             };
> > >     };
> > >
> > >     bus@0 {
> > >             /* truncation happens here */
> > >             #address-cells = <1>;
> > >             #size-cells = <1>;
> > >             ranges = <0x0 0x0 0x0 0x40000000>;
> > >
> > >             mc: memory-controller@2c00000 {
> > >                     #address-cells = <2>;
> > >                     #size-cells = <2>;
> >
> > I think this is wrong. The parent should have more or equal number of
> > cells.
>
> I was half suspecting that. The reason why I hesitated is that I recall
> having the opposite discussion a while ago when we were adding bus@0 to
> 64-bit Tegra devices. We had at some point (probably around Tegra114 or
> Tegra124, 32-bit ARM chips that support LPAE) started to set #address-
> cells = <2> precisely because the CPU could address more than 32-bit
> addresses. We then did the same thing transitioning to 64-bit ARM. When
> we then started discussing bus@0, someone (might have been you) had
> argued that all these peripherals could be addressed with a single cell
> so there'd be no need for #address-cells = <2>, so then we went with
> that.

I may have not thinking about the DMA side of things.

> Reverting back to #address-cells = <2> is now going to cause quite a bit
> of churn, but I guess if it's the right thing, so be it.
>
> Another possible alternative would be to move the memory-controller node
> from the bus@0 to the top-level. Not sure if that's any better.

I stumbled upon 'ibm,#dma-address-cells' and 'ibm,#dma-size-cells'
while reviewing this. Those seem to be for the same purpose AFAICT. We
could consider adding those (w/o 'ibm') to handle this situation.

Rob

Re: [PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Lucas Stach]

Am Dienstag, dem 08.11.2022 um 10:25 -0600 schrieb Rob Herring:
> On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote:
> > > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > 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).
> > > 
> > > My assumption in this case was that the parent cell sizes should be
> > > increased to 2 cells. That tends to be what people want to do anyways
> > > (64-bit everywhere on 64-bit CPUs).
> > > 
> > > > 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.
> > > 
> > > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec
> > > even says that because I hit that case.
> > > 
> > > Is this going to work for 'dma-device' with something like this?:
> > > 
> > >   bus@0 {
> > >     dma-ranges = <...>;
> > >     child-bus@... {
> > >       dma-device@... {
> > >       };
> > >     };
> > >   };
> > > 
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > > Changes in v10:
> > > > - new patch to avoid address truncation when traversing a bus hierarchy
> > > >   with mismatching #address-cells properties
> > > > 
> > > > Example from Tegra194 (redacted for clarity):
> > > > 
> > > >     reserved-memory {
> > > >             #address-cells = <2>;
> > > >             #size-cells = <2>;
> > > >             ranges;
> > > > 
> > > >             framebuffer@0,0 {
> > > >                     compatible = "framebuffer";
> > > >                     reg = <0x2 0x57320000 0x0 0x00800000>;
> > > >                     iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>;
> > > >             };
> > > >     };
> > > > 
> > > >     bus@0 {
> > > >             /* truncation happens here */
> > > >             #address-cells = <1>;
> > > >             #size-cells = <1>;
> > > >             ranges = <0x0 0x0 0x0 0x40000000>;
> > > > 
> > > >             mc: memory-controller@2c00000 {
> > > >                     #address-cells = <2>;
> > > >                     #size-cells = <2>;
> > > 
> > > I think this is wrong. The parent should have more or equal number of
> > > cells.
> > 
> > I was half suspecting that. The reason why I hesitated is that I recall
> > having the opposite discussion a while ago when we were adding bus@0 to
> > 64-bit Tegra devices. We had at some point (probably around Tegra114 or
> > Tegra124, 32-bit ARM chips that support LPAE) started to set #address-
> > cells = <2> precisely because the CPU could address more than 32-bit
> > addresses. We then did the same thing transitioning to 64-bit ARM. When
> > we then started discussing bus@0, someone (might have been you) had
> > argued that all these peripherals could be addressed with a single cell
> > so there'd be no need for #address-cells = <2>, so then we went with
> > that.
> 
> I may have not thinking about the DMA side of things.
> 
> > Reverting back to #address-cells = <2> is now going to cause quite a bit
> > of churn, but I guess if it's the right thing, so be it.
> > 
> > Another possible alternative would be to move the memory-controller node
> > from the bus@0 to the top-level. Not sure if that's any better.
> 
> I stumbled upon 'ibm,#dma-address-cells' and 'ibm,#dma-size-cells'
> while reviewing this. Those seem to be for the same purpose AFAICT. We
> could consider adding those (w/o 'ibm') to handle this situation.

I would appreciate this. We have the same situation on some of the NXP
i.MX8 SoCs right now: all the MMIO is addressable with 32bit, so all
the busses have a single address and size cell right now, but we would
need to extend the address-cells to 64bit just to properly describe the
DMA addressing capabilities of the devices.

Regards,
Lucas

Re: [PATCH v10 2/5] of: Stop DMA translation at last DMA parent

[Thierry Reding]

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

On Wed, Nov 09, 2022 at 11:07:02AM +0100, Lucas Stach wrote:
> Am Dienstag, dem 08.11.2022 um 10:25 -0600 schrieb Rob Herring:
> > On Tue, Nov 8, 2022 at 8:33 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > On Mon, Nov 07, 2022 at 01:30:35PM -0600, Rob Herring wrote:
> > > > On Thu, Nov 03, 2022 at 02:38:57PM +0100, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > 
> > > > > 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).
> > > > 
> > > > My assumption in this case was that the parent cell sizes should be
> > > > increased to 2 cells. That tends to be what people want to do anyways
> > > > (64-bit everywhere on 64-bit CPUs).
> > > > 
> > > > > 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.
> > > > 
> > > > We treat no 'dma-ranges' as equivalent to 'dma-ranges;'. IIRC, the spec
> > > > even says that because I hit that case.
> > > > 
> > > > Is this going to work for 'dma-device' with something like this?:
> > > > 
> > > >   bus@0 {
> > > >     dma-ranges = <...>;
> > > >     child-bus@... {
> > > >       dma-device@... {
> > > >       };
> > > >     };
> > > >   };
> > > > 
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > > ---
> > > > > Changes in v10:
> > > > > - new patch to avoid address truncation when traversing a bus hierarchy
> > > > >   with mismatching #address-cells properties
> > > > > 
> > > > > Example from Tegra194 (redacted for clarity):
> > > > > 
> > > > >     reserved-memory {
> > > > >             #address-cells = <2>;
> > > > >             #size-cells = <2>;
> > > > >             ranges;
> > > > > 
> > > > >             framebuffer@0,0 {
> > > > >                     compatible = "framebuffer";
> > > > >                     reg = <0x2 0x57320000 0x0 0x00800000>;
> > > > >                     iommu-addresses = <&dc0 0x2 0x57320000 0x0 0x00800000>;
> > > > >             };
> > > > >     };
> > > > > 
> > > > >     bus@0 {
> > > > >             /* truncation happens here */
> > > > >             #address-cells = <1>;
> > > > >             #size-cells = <1>;
> > > > >             ranges = <0x0 0x0 0x0 0x40000000>;
> > > > > 
> > > > >             mc: memory-controller@2c00000 {
> > > > >                     #address-cells = <2>;
> > > > >                     #size-cells = <2>;
> > > > 
> > > > I think this is wrong. The parent should have more or equal number of
> > > > cells.
> > > 
> > > I was half suspecting that. The reason why I hesitated is that I recall
> > > having the opposite discussion a while ago when we were adding bus@0 to
> > > 64-bit Tegra devices. We had at some point (probably around Tegra114 or
> > > Tegra124, 32-bit ARM chips that support LPAE) started to set #address-
> > > cells = <2> precisely because the CPU could address more than 32-bit
> > > addresses. We then did the same thing transitioning to 64-bit ARM. When
> > > we then started discussing bus@0, someone (might have been you) had
> > > argued that all these peripherals could be addressed with a single cell
> > > so there'd be no need for #address-cells = <2>, so then we went with
> > > that.
> > 
> > I may have not thinking about the DMA side of things.
> > 
> > > Reverting back to #address-cells = <2> is now going to cause quite a bit
> > > of churn, but I guess if it's the right thing, so be it.
> > > 
> > > Another possible alternative would be to move the memory-controller node
> > > from the bus@0 to the top-level. Not sure if that's any better.
> > 
> > I stumbled upon 'ibm,#dma-address-cells' and 'ibm,#dma-size-cells'
> > while reviewing this. Those seem to be for the same purpose AFAICT. We
> > could consider adding those (w/o 'ibm') to handle this situation.
> 
> I would appreciate this. We have the same situation on some of the NXP
> i.MX8 SoCs right now: all the MMIO is addressable with 32bit, so all
> the busses have a single address and size cell right now, but we would
> need to extend the address-cells to 64bit just to properly describe the
> DMA addressing capabilities of the devices.

Alright, I'll see if I can come up with some code to deal with this.

Thierry

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

[PATCH v10 3/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 v10:
- mark iommu-addresses as required in the absence of reg and size

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      | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
index 44f72bcf1782..07711fb30518 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: >
@@ -95,6 +119,55 @@ oneOf:
   - required:
       - size
 
+  - required:
+      - iommu-addresses
+
 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.38.1

[PATCH v10 4/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>
---
Rob,

I've dropped your Reviewed-by: for now since there were some significant
changes since you looked at this. Please take another look to confirm
that it still applies.

Changes in v10:
- extract more code into the new iommu_resv_region_get_type() function
- rename variables for physical and I/O virtual addresses for clarity
- default to IOMMU_RESV_DIRECT instead of IOMMU_RESV_DIRECT_RELAXABLE
- use newly introduced of_translate_dma_region()

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 | 94 ++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h |  8 ++++
 2 files changed, 102 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5696314ae69e..fa7c63a4abbf 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,96 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 
 	return ops;
 }
+
+static enum iommu_resv_type iommu_resv_region_get_type(struct device *dev, struct resource *phys,
+						       phys_addr_t start, size_t length)
+{
+	phys_addr_t end = start + length - 1;
+
+	/*
+	 * IOMMU regions without an associated physical region cannot be
+	 * mapped and are simply reservations.
+	 */
+	if (phys->start >= phys->end)
+		return IOMMU_RESV_RESERVED;
+
+	/* may be IOMMU_RESV_DIRECT_RELAXABLE for certain cases */
+	if (start == phys->start && end == phys->end)
+		return IOMMU_RESV_DIRECT;
+
+	dev_warn(dev, "treating non-direct mapping [%pr] -> [%pap-%pap] as reservation\n", &phys,
+		 &start, &end);
+	return IOMMU_RESV_RESERVED;
+}
+
+/**
+ * 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 phys;
+		int size;
+
+		memset(&phys, 0, sizeof(phys));
+
+		/*
+		 * 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, &phys);
+			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;
+
+			phandle = be32_to_cpup(maps++);
+			np = of_find_node_by_phandle(phandle);
+
+			if (np == dev->of_node) {
+				int prot = IOMMU_READ | IOMMU_WRITE;
+				struct iommu_resv_region *region;
+				enum iommu_resv_type type;
+				phys_addr_t iova;
+				size_t length;
+
+				maps = of_translate_dma_region(np, maps, &iova, &length);
+				type = iommu_resv_region_get_type(dev, &phys, iova, length);
+
+				region = iommu_alloc_resv_region(iova, length, prot, type,
+								 GFP_KERNEL);
+				if (region)
+					list_add_tail(&region->list, list);
+			}
+		}
+	}
+#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.38.1

[PATCH v10 5/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.38.1