iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
@ 2020-09-04 12:59 Thierry Reding
  2020-09-04 12:59 ` [PATCH v2 2/4] iommu: Implement of_iommu_get_resv_regions() Thierry Reding
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Thierry Reding @ 2020-09-04 12:59 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, Will Deacon

From: Thierry Reding <treding@nvidia.com>

Reserved memory regions can be marked as "active" if hardware is
expected to access the regions during boot and before the operating
system can take control. One example where this is useful is for the
operating system to infer whether the region needs to be identity-
mapped through an IOMMU.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
index 4dd20de6977f..163d2927e4fc 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
@@ -63,6 +63,13 @@ reusable (optional) - empty property
       able to reclaim it back. Typically that means that the operating
       system can use that region to store volatile or cached data that
       can be otherwise regenerated or migrated elsewhere.
+active (optional) - empty property
+    - If this property is set for a reserved memory region, it indicates
+      that some piece of hardware may be actively accessing this region.
+      Should the operating system want to enable IOMMU protection for a
+      device, all active memory regions must have been identity-mapped
+      in order to ensure that non-quiescent hardware during boot can
+      continue to access the memory.
 
 Linux implementation note:
 - If a "linux,cma-default" property is present, then Linux will use the
-- 
2.28.0

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/4] iommu: Implement of_iommu_get_resv_regions()
  2020-09-04 12:59 [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Thierry Reding
@ 2020-09-04 12:59 ` Thierry Reding
  2020-09-04 12:59 ` [PATCH v2 3/4] iommu: dma: Use of_iommu_get_resv_regions() Thierry Reding
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2020-09-04 12:59 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, Will Deacon

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. These
regions will be used to create 1:1 mappings in the IOMMU domain that
the devices will be attached to.

Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Hi Rob,

you had previously reviewed this patch, but I haven't included that here
because there's a new property now that you might not be okay with.

Thierry

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

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

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e505b9130a1c..3341d27fbbba 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>
@@ -245,3 +246,51 @@ 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)
+{
+	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;
+
+		/*
+		 * Active memory regions are expected to be accessed by
+		 * hardware during boot and must therefore have an identity
+		 * mapping created prior to the driver taking control of the
+		 * hardware. This ensures that non-quiescent hardware doesn't
+		 * cause IOMMU faults during boot.
+		 */
+		if (!of_property_read_bool(it.node, "active"))
+			continue;
+
+		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;
+		}
+
+		region = iommu_alloc_resv_region(res.start, resource_size(&res),
+						 IOMMU_READ | IOMMU_WRITE,
+						 IOMMU_RESV_DIRECT_RELAXABLE);
+		if (!region)
+			continue;
+
+		list_add_tail(&region->list, list);
+	}
+}
+EXPORT_SYMBOL(of_iommu_get_resv_regions);
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 16f4b3e87f20..8412437acaac 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -16,6 +16,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 int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -32,6 +35,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.28.0

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 3/4] iommu: dma: Use of_iommu_get_resv_regions()
  2020-09-04 12:59 [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Thierry Reding
  2020-09-04 12:59 ` [PATCH v2 2/4] iommu: Implement of_iommu_get_resv_regions() Thierry Reding
@ 2020-09-04 12:59 ` Thierry Reding
  2020-09-04 13:00 ` [RFC 4/4] iommu/tegra-smmu: Add support for reserved regions Thierry Reding
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2020-09-04 12:59 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, Will Deacon

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 5141d49a046b..1c36ca6ec2a3 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -20,6 +20,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/vmalloc.h>
@@ -164,6 +165,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.28.0

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [RFC 4/4] iommu/tegra-smmu: Add support for reserved regions
  2020-09-04 12:59 [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Thierry Reding
  2020-09-04 12:59 ` [PATCH v2 2/4] iommu: Implement of_iommu_get_resv_regions() Thierry Reding
  2020-09-04 12:59 ` [PATCH v2 3/4] iommu: dma: Use of_iommu_get_resv_regions() Thierry Reding
@ 2020-09-04 13:00 ` Thierry Reding
  2020-09-14 22:08 ` [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Rob Herring
  2020-09-24 13:23 ` Dmitry Osipenko
  4 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2020-09-04 13:00 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, Will Deacon

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>
---
I'm sending this out as RFC because there's a few hacks in here to make
this work properly and I'm not fully happy with this yet (see sections
marked with XXX).

Thierry

 drivers/iommu/tegra-smmu.c | 115 +++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 2574e716086b..33abc1527ac4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -9,6 +9,7 @@
 #include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -530,6 +531,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 (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;
@@ -542,6 +575,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)
@@ -882,6 +921,79 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	return group;
 }
 
+static void tegra_smmu_get_resv_regions(struct device *dev, struct list_head *list)
+{
+	struct of_phandle_iterator it;
+	int err;
+
+	if (!dev->of_node)
+		return;
+
+	of_for_each_phandle(&it, err, dev->of_node, "memory-region", NULL, 0) {
+		struct iommu_resv_region *region;
+		struct resource res;
+
+		err = of_address_to_resource(it.node, 0, &res);
+		if (err < 0) {
+			dev_err(dev, "failed to parse memory region %pOFn: %d\n",
+				it.node, err);
+			continue;
+		}
+
+		region = iommu_alloc_resv_region(res.start, resource_size(&res),
+						 IOMMU_READ | IOMMU_WRITE,
+						 IOMMU_RESV_DIRECT);
+		if (!region)
+			return;
+
+		dev_dbg(dev, "reserved region %pR\n", &res);
+		list_add_tail(&region->list, list);
+	}
+}
+
+static void tegra_smmu_put_resv_regions(struct device *dev,
+					struct list_head *list)
+{
+	struct iommu_resv_region *entry, *next;
+
+	list_for_each_entry_safe(entry, next, list, list)
+		kfree(entry);
+}
+
+static void tegra_smmu_apply_resv_region(struct device *dev,
+					 struct iommu_domain *domain,
+					 struct iommu_resv_region *region)
+{
+	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
+	struct tegra_smmu_as *as = to_smmu_as(domain);
+
+	/*
+	 * ->attach_dev() may not have been called yet at this point, so the
+	 * address space may not have been associated with an SMMU instance.
+	 * Set up the association here to make sure subsequent code can rely
+	 * on the SMMU instance being known.
+	 *
+	 * Also make sure that the SMMU instance doesn't conflict if an SMMU
+	 * has been associated with the address space already. This can happen
+	 * if a domain is shared between multiple devices.
+	 *
+	 * Note that this is purely theoretic because there are no known SoCs
+	 * with multiple instances of this SMMU.
+	 *
+	 * XXX Deal with this elsewhere. One possibility would be to pass the
+	 * struct iommu_domain that we're operating on to ->get_resv_regions()
+	 * and ->put_resv_regions() so that the connection between it and the
+	 * struct device (in order to find the SMMU instance) can already be
+	 * established at that time. This would be nicely symmetric because a
+	 * ->put_resv_regions() could undo that again so that ->attach_dev()
+	 * could start from a clean slate.
+	 */
+	if (as->smmu && as->smmu != smmu)
+		WARN(1, "conflicting SMMU instances\n");
+
+	as->smmu = smmu;
+}
+
 static int tegra_smmu_of_xlate(struct device *dev,
 			       struct of_phandle_args *args)
 {
@@ -902,6 +1014,9 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.map = tegra_smmu_map,
 	.unmap = tegra_smmu_unmap,
 	.iova_to_phys = tegra_smmu_iova_to_phys,
+	.get_resv_regions = tegra_smmu_get_resv_regions,
+	.put_resv_regions = tegra_smmu_put_resv_regions,
+	.apply_resv_region = tegra_smmu_apply_resv_region,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 };
-- 
2.28.0

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-04 12:59 [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Thierry Reding
                   ` (2 preceding siblings ...)
  2020-09-04 13:00 ` [RFC 4/4] iommu/tegra-smmu: Add support for reserved regions Thierry Reding
@ 2020-09-14 22:08 ` Rob Herring
  2020-09-15 12:36   ` Thierry Reding
  2020-09-24 13:23 ` Dmitry Osipenko
  4 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2020-09-14 22:08 UTC (permalink / raw)
  To: Thierry Reding
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu, Will Deacon

On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Reserved memory regions can be marked as "active" if hardware is
> expected to access the regions during boot and before the operating
> system can take control. One example where this is useful is for the
> operating system to infer whether the region needs to be identity-
> mapped through an IOMMU.

I like simple solutions, but this hardly seems adequate to solve the 
problem of passing IOMMU setup from bootloader/firmware to the OS. Like 
what is the IOVA that's supposed to be used if identity mapping is not 
used?

If you know enough about the regions to assume identity mapping, then 
can't you know if active or not?

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index 4dd20de6977f..163d2927e4fc 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>        able to reclaim it back. Typically that means that the operating
>        system can use that region to store volatile or cached data that
>        can be otherwise regenerated or migrated elsewhere.
> +active (optional) - empty property
> +    - If this property is set for a reserved memory region, it indicates
> +      that some piece of hardware may be actively accessing this region.
> +      Should the operating system want to enable IOMMU protection for a
> +      device, all active memory regions must have been identity-mapped
> +      in order to ensure that non-quiescent hardware during boot can
> +      continue to access the memory.
>  
>  Linux implementation note:
>  - If a "linux,cma-default" property is present, then Linux will use the
> -- 
> 2.28.0
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-14 22:08 ` [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Rob Herring
@ 2020-09-15 12:36   ` Thierry Reding
  2020-09-24 11:27     ` Thierry Reding
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-09-15 12:36 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu, Will Deacon


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

On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Reserved memory regions can be marked as "active" if hardware is
> > expected to access the regions during boot and before the operating
> > system can take control. One example where this is useful is for the
> > operating system to infer whether the region needs to be identity-
> > mapped through an IOMMU.
> 
> I like simple solutions, but this hardly seems adequate to solve the 
> problem of passing IOMMU setup from bootloader/firmware to the OS. Like 
> what is the IOVA that's supposed to be used if identity mapping is not 
> used?

The assumption here is that if the region is not active there is no need
for the IOVA to be specified because the kernel will allocate memory and
assign any IOVA of its choosing.

Also, note that this is not meant as a way of passing IOMMU setup from
the bootloader or firmware to the OS. The purpose of this is to specify
that some region of memory is actively being accessed during boot. The
particular case that I'm looking at is where the bootloader set up a
splash screen and keeps it on during boot. The bootloader has not set up
an IOMMU mapping and the identity mapping serves as a way of keeping the
accesses by the display hardware working during the transitional period
after the IOMMU translations have been enabled by the kernel but before
the kernel display driver has had a chance to set up its own IOMMU
mappings.

> If you know enough about the regions to assume identity mapping, then 
> can't you know if active or not?

We could alternatively add some property that describes the region as
requiring an identity mapping. But note that we can't make any
assumptions here about the usage of these regions because the IOMMU
driver simply has no way of knowing what they are being used for.

Some additional information is required in device tree for the IOMMU
driver to be able to make that decision.

Thierry

> 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > index 4dd20de6977f..163d2927e4fc 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > @@ -63,6 +63,13 @@ reusable (optional) - empty property
> >        able to reclaim it back. Typically that means that the operating
> >        system can use that region to store volatile or cached data that
> >        can be otherwise regenerated or migrated elsewhere.
> > +active (optional) - empty property
> > +    - If this property is set for a reserved memory region, it indicates
> > +      that some piece of hardware may be actively accessing this region.
> > +      Should the operating system want to enable IOMMU protection for a
> > +      device, all active memory regions must have been identity-mapped
> > +      in order to ensure that non-quiescent hardware during boot can
> > +      continue to access the memory.
> >  
> >  Linux implementation note:
> >  - If a "linux,cma-default" property is present, then Linux will use the
> > -- 
> > 2.28.0
> > 

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-15 12:36   ` Thierry Reding
@ 2020-09-24 11:27     ` Thierry Reding
  2020-11-05 16:43       ` Thierry Reding
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-09-24 11:27 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu, Will Deacon


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

On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Reserved memory regions can be marked as "active" if hardware is
> > > expected to access the regions during boot and before the operating
> > > system can take control. One example where this is useful is for the
> > > operating system to infer whether the region needs to be identity-
> > > mapped through an IOMMU.
> > 
> > I like simple solutions, but this hardly seems adequate to solve the 
> > problem of passing IOMMU setup from bootloader/firmware to the OS. Like 
> > what is the IOVA that's supposed to be used if identity mapping is not 
> > used?
> 
> The assumption here is that if the region is not active there is no need
> for the IOVA to be specified because the kernel will allocate memory and
> assign any IOVA of its choosing.
> 
> Also, note that this is not meant as a way of passing IOMMU setup from
> the bootloader or firmware to the OS. The purpose of this is to specify
> that some region of memory is actively being accessed during boot. The
> particular case that I'm looking at is where the bootloader set up a
> splash screen and keeps it on during boot. The bootloader has not set up
> an IOMMU mapping and the identity mapping serves as a way of keeping the
> accesses by the display hardware working during the transitional period
> after the IOMMU translations have been enabled by the kernel but before
> the kernel display driver has had a chance to set up its own IOMMU
> mappings.
> 
> > If you know enough about the regions to assume identity mapping, then 
> > can't you know if active or not?
> 
> We could alternatively add some property that describes the region as
> requiring an identity mapping. But note that we can't make any
> assumptions here about the usage of these regions because the IOMMU
> driver simply has no way of knowing what they are being used for.
> 
> Some additional information is required in device tree for the IOMMU
> driver to be able to make that decision.

Rob, can you provide any hints on exactly how you want to move this
forward? I don't know in what direction you'd like to proceed.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-04 12:59 [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Thierry Reding
                   ` (3 preceding siblings ...)
  2020-09-14 22:08 ` [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Rob Herring
@ 2020-09-24 13:23 ` Dmitry Osipenko
  2020-09-24 14:01   ` Thierry Reding
  4 siblings, 1 reply; 24+ messages in thread
From: Dmitry Osipenko @ 2020-09-24 13:23 UTC (permalink / raw)
  To: Thierry Reding, Joerg Roedel
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-tegra, Will Deacon

04.09.2020 15:59, Thierry Reding пишет:
> From: Thierry Reding <treding@nvidia.com>
> 
> Reserved memory regions can be marked as "active" if hardware is
> expected to access the regions during boot and before the operating
> system can take control. One example where this is useful is for the
> operating system to infer whether the region needs to be identity-
> mapped through an IOMMU.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index 4dd20de6977f..163d2927e4fc 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>        able to reclaim it back. Typically that means that the operating
>        system can use that region to store volatile or cached data that
>        can be otherwise regenerated or migrated elsewhere.
> +active (optional) - empty property
> +    - If this property is set for a reserved memory region, it indicates
> +      that some piece of hardware may be actively accessing this region.
> +      Should the operating system want to enable IOMMU protection for a
> +      device, all active memory regions must have been identity-mapped
> +      in order to ensure that non-quiescent hardware during boot can
> +      continue to access the memory.
>  
>  Linux implementation note:
>  - If a "linux,cma-default" property is present, then Linux will use the
> 

Hi,

Could you please explain what devices need this quirk? I see that you're
targeting Tegra SMMU driver, which means that it should be some pre-T186
device. Is this reservation needed for some device that has display
hardwired to a very specific IOMMU domain at the boot time?

If you're targeting devices that don't have IOMMU enabled by default at
the boot time, then this approach won't work for the existing devices
which won't ever get an updated bootloader. I think Robin Murphy already
suggested that we should simply create a dummy "identity" IOMMU domain
by default for the DRM/VDE devices and then replace it with an
explicitly created domain within the drivers.

Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
kernel's cmdline with the physical location of the framebuffer in
memory. Maybe we could support this option?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-24 13:23 ` Dmitry Osipenko
@ 2020-09-24 14:01   ` Thierry Reding
  2020-09-24 16:23     ` Dmitry Osipenko
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-09-24 14:01 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-tegra, Will Deacon


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

On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
> 04.09.2020 15:59, Thierry Reding пишет:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Reserved memory regions can be marked as "active" if hardware is
> > expected to access the regions during boot and before the operating
> > system can take control. One example where this is useful is for the
> > operating system to infer whether the region needs to be identity-
> > mapped through an IOMMU.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > index 4dd20de6977f..163d2927e4fc 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > @@ -63,6 +63,13 @@ reusable (optional) - empty property
> >        able to reclaim it back. Typically that means that the operating
> >        system can use that region to store volatile or cached data that
> >        can be otherwise regenerated or migrated elsewhere.
> > +active (optional) - empty property
> > +    - If this property is set for a reserved memory region, it indicates
> > +      that some piece of hardware may be actively accessing this region.
> > +      Should the operating system want to enable IOMMU protection for a
> > +      device, all active memory regions must have been identity-mapped
> > +      in order to ensure that non-quiescent hardware during boot can
> > +      continue to access the memory.
> >  
> >  Linux implementation note:
> >  - If a "linux,cma-default" property is present, then Linux will use the
> > 
> 
> Hi,
> 
> Could you please explain what devices need this quirk? I see that you're
> targeting Tegra SMMU driver, which means that it should be some pre-T186
> device.

Primarily I'm looking at Tegra210 and later, because on earlier devices
the bootloader doesn't consistently initialize display. I know that it
does on some devices, but not all of them. This same code should also
work on Tegra186 and later (with an ARM SMMU) although the situation is
slightly more complicated there because IOMMU translations will fault by
default long before these identity mappings can be established.

> Is this reservation needed for some device that has display
> hardwired to a very specific IOMMU domain at the boot time?

No, this is only used to convey information about the active framebuffer
to the kernel. In practice the DMA/IOMMU code will use this information
to establish a 1:1 mapping on whatever IOMMU domain that was picked for
display.

> If you're targeting devices that don't have IOMMU enabled by default at
> the boot time, then this approach won't work for the existing devices
> which won't ever get an updated bootloader.

If the devices don't use an IOMMU, then there should be no problem. The
extra reserved-memory nodes would still be necessary to ensure that the
kernel doesn't reuse the framebuffer memory for the slab allocator, but
if no IOMMU is used, then the display controller accessing the memory
isn't going to cause problems other than perhaps scanning out data that
is no longer a framebuffer.

There should also be no problem for devices with an old bootloader
because this code is triggered by the presence of a reserved-memory node
referenced via the memory-region property. Devices with an old
bootloader should continue to work as they did before. Although I
suppose they would start faulting once we enable DMA/IOMMU integration
for Tegra SMMU if they have a bootloader that does initialize display to
actively scan out during boot.

> I think Robin Murphy already suggested that we should simply create
> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
> then replace it with an explicitly created domain within the drivers.

I don't recall reading about that suggestion. So does this mean that for
certain devices we'd want to basically passthrough by default and then
at some point during boot take over with a properly managed IOMMU
domain?

The primary goal here is to move towards using the DMA API rather than
the IOMMU API directly, so we don't really have the option of replacing
with an explicitly created domain. Unless we have code in the DMA/IOMMU
code that does this somehow.

But I'm not sure what would be a good way to mark certain devices as
needing an identity domain by default. Do we still use the reserved-
memory node for that? That would still require some sort of flag to
specify which reserved-memory regions would need this identity mapping
because, as was pointed out in earlier review, some devices may have
reserved-memory regions that are not meant to be identity mapped.

> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
> kernel's cmdline with the physical location of the framebuffer in
> memory. Maybe we could support this option?

I'm not a big fan of that command-line option, but I also realize that
for older bootloaders that's probably the only option we have. I don't
suppose all of the devices support U-Boot? Because ideally we'd just
translate from tegra_fbmem=... to reserved-memory region there so that
we don't have to carry backwards-compatibility code for these purely
downstream bootloaders.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-24 14:01   ` Thierry Reding
@ 2020-09-24 16:23     ` Dmitry Osipenko
  2020-09-25 12:39       ` Robin Murphy
  2020-11-05 15:49       ` Thierry Reding
  0 siblings, 2 replies; 24+ messages in thread
From: Dmitry Osipenko @ 2020-09-24 16:23 UTC (permalink / raw)
  To: Thierry Reding
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-tegra, Will Deacon

24.09.2020 17:01, Thierry Reding пишет:
> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>> 04.09.2020 15:59, Thierry Reding пишет:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Reserved memory regions can be marked as "active" if hardware is
>>> expected to access the regions during boot and before the operating
>>> system can take control. One example where this is useful is for the
>>> operating system to infer whether the region needs to be identity-
>>> mapped through an IOMMU.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> index 4dd20de6977f..163d2927e4fc 100644
>>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>        able to reclaim it back. Typically that means that the operating
>>>        system can use that region to store volatile or cached data that
>>>        can be otherwise regenerated or migrated elsewhere.
>>> +active (optional) - empty property
>>> +    - If this property is set for a reserved memory region, it indicates
>>> +      that some piece of hardware may be actively accessing this region.
>>> +      Should the operating system want to enable IOMMU protection for a
>>> +      device, all active memory regions must have been identity-mapped
>>> +      in order to ensure that non-quiescent hardware during boot can
>>> +      continue to access the memory.
>>>  
>>>  Linux implementation note:
>>>  - If a "linux,cma-default" property is present, then Linux will use the
>>>
>>
>> Hi,
>>
>> Could you please explain what devices need this quirk? I see that you're
>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>> device.
> 
> Primarily I'm looking at Tegra210 and later, because on earlier devices
> the bootloader doesn't consistently initialize display. I know that it
> does on some devices, but not all of them.

AFAIK, all tablet devices starting with Tegra20 that have display panel
are initializing display at a boot time for showing splash screen. This
includes all T20/T30/T114 tablets that are already supported by upstream
kernel.

> This same code should also
> work on Tegra186 and later (with an ARM SMMU) although the situation is
> slightly more complicated there because IOMMU translations will fault by
> default long before these identity mappings can be established.
> 
>> Is this reservation needed for some device that has display
>> hardwired to a very specific IOMMU domain at the boot time?
> 
> No, this is only used to convey information about the active framebuffer
> to the kernel. In practice the DMA/IOMMU code will use this information
> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
> display.
> 
>> If you're targeting devices that don't have IOMMU enabled by default at
>> the boot time, then this approach won't work for the existing devices
>> which won't ever get an updated bootloader.
> 
> If the devices don't use an IOMMU, then there should be no problem. The
> extra reserved-memory nodes would still be necessary to ensure that the
> kernel doesn't reuse the framebuffer memory for the slab allocator, but
> if no IOMMU is used, then the display controller accessing the memory
> isn't going to cause problems other than perhaps scanning out data that
> is no longer a framebuffer.
> 
> There should also be no problem for devices with an old bootloader
> because this code is triggered by the presence of a reserved-memory node
> referenced via the memory-region property. Devices with an old
> bootloader should continue to work as they did before. Although I
> suppose they would start faulting once we enable DMA/IOMMU integration
> for Tegra SMMU if they have a bootloader that does initialize display to
> actively scan out during boot.
> 
>> I think Robin Murphy already suggested that we should simply create
>> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
>> then replace it with an explicitly created domain within the drivers.
> 
> I don't recall reading about that suggestion. So does this mean that for
> certain devices we'd want to basically passthrough by default and then
> at some point during boot take over with a properly managed IOMMU
> domain?

Yes, my understanding that this is what Robin suggested here:

https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/

> The primary goal here is to move towards using the DMA API rather than
> the IOMMU API directly, so we don't really have the option of replacing
> with an explicitly created domain. Unless we have code in the DMA/IOMMU
> code that does this somehow.
> 
> But I'm not sure what would be a good way to mark certain devices as
> needing an identity domain by default. Do we still use the reserved-
> memory node for that?

The reserved-memory indeed shouldn't be needed for resolving the
implicit IOMMU problem since we could mark certain devices within the
kernel IOMMU driver.

I haven't got around to trying to implement the implicit IOMMU support
yet, but I suppose we could implement the def_domain_type() hook in the
SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
devices. Then the Display/VDE drivers will take over the identity domain
and replace it with the explicit domain.

> That would still require some sort of flag to
> specify which reserved-memory regions would need this identity mapping
> because, as was pointed out in earlier review, some devices may have
> reserved-memory regions that are not meant to be identity mapped.

Please note that the reserved-memory approach also creates problem for
selection of a large CMA region if FB is located somewhere in a middle
of DRAM.

I already see that the FB's reserved-memory will break CMA for Nexus 7
and Acer A500 because CMA area overlaps with the bootloader's FB :)

Also keep in mind that initrd needs a location too and location usually
hardwired in a bootloader. Hence it increases pressure on the CMA selection.

>> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
>> kernel's cmdline with the physical location of the framebuffer in
>> memory. Maybe we could support this option?
> 
> I'm not a big fan of that command-line option, but I also realize that
> for older bootloaders that's probably the only option we have. I don't
> suppose all of the devices support U-Boot?

Majority of devices in a wild don't use u-boot and they have a
locked-down bootloader. Still it's possible to chain-load u-boot or
bypass the "security" and replace the bootloader, but these approaches
aren't widely supported because they take a lot of effort to be
implemented and maintained.

Even those devices that use proper u-boot usually never updating it and
are running some ancient version. You can't ignore all those people :)

> Because ideally we'd just
> translate from tegra_fbmem=... to reserved-memory region there so that
> we don't have to carry backwards-compatibility code for these purely
> downstream bootloaders.

IIRC, in the past Robin Murphy was suggesting to read out hardware state
early during kernel boot in order to find what regions are in use by
hardware.

I think it should be easy to do for the display controller since we
could check clock and PD states in order to decide whether DC's IO could
be accessed and then read out the FB pointer and size. I guess it should
take about hundred lines of code.

But the easiest way should be to ignore this trouble for devices that
have IOMMU disabled by default and simply allow display to show garbage.
Nobody ever complained about this for the past 7+ years :)

Hence implementing the dummy-identity domain support should be enough
for solving the problem, at least this should work for pre-T186 devices.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-24 16:23     ` Dmitry Osipenko
@ 2020-09-25 12:39       ` Robin Murphy
  2020-09-25 13:21         ` Dmitry Osipenko
                           ` (2 more replies)
  2020-11-05 15:49       ` Thierry Reding
  1 sibling, 3 replies; 24+ messages in thread
From: Robin Murphy @ 2020-09-25 12:39 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: devicetree, Frank Rowand, linux-kernel, iommu, Rob Herring,
	linux-tegra, Will Deacon

On 2020-09-24 17:23, Dmitry Osipenko wrote:
> 24.09.2020 17:01, Thierry Reding пишет:
>> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>>> 04.09.2020 15:59, Thierry Reding пишет:
>>>> From: Thierry Reding <treding@nvidia.com>
>>>>
>>>> Reserved memory regions can be marked as "active" if hardware is
>>>> expected to access the regions during boot and before the operating
>>>> system can take control. One example where this is useful is for the
>>>> operating system to infer whether the region needs to be identity-
>>>> mapped through an IOMMU.
>>>>
>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>> ---
>>>>   .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>> index 4dd20de6977f..163d2927e4fc 100644
>>>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>>         able to reclaim it back. Typically that means that the operating
>>>>         system can use that region to store volatile or cached data that
>>>>         can be otherwise regenerated or migrated elsewhere.
>>>> +active (optional) - empty property
>>>> +    - If this property is set for a reserved memory region, it indicates
>>>> +      that some piece of hardware may be actively accessing this region.
>>>> +      Should the operating system want to enable IOMMU protection for a
>>>> +      device, all active memory regions must have been identity-mapped
>>>> +      in order to ensure that non-quiescent hardware during boot can
>>>> +      continue to access the memory.
>>>>   
>>>>   Linux implementation note:
>>>>   - If a "linux,cma-default" property is present, then Linux will use the
>>>>
>>>
>>> Hi,
>>>
>>> Could you please explain what devices need this quirk? I see that you're
>>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>>> device.
>>
>> Primarily I'm looking at Tegra210 and later, because on earlier devices
>> the bootloader doesn't consistently initialize display. I know that it
>> does on some devices, but not all of them.
> 
> AFAIK, all tablet devices starting with Tegra20 that have display panel
> are initializing display at a boot time for showing splash screen. This
> includes all T20/T30/T114 tablets that are already supported by upstream
> kernel.
> 
>> This same code should also
>> work on Tegra186 and later (with an ARM SMMU) although the situation is
>> slightly more complicated there because IOMMU translations will fault by
>> default long before these identity mappings can be established.
>>
>>> Is this reservation needed for some device that has display
>>> hardwired to a very specific IOMMU domain at the boot time?
>>
>> No, this is only used to convey information about the active framebuffer
>> to the kernel. In practice the DMA/IOMMU code will use this information
>> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
>> display.
>>
>>> If you're targeting devices that don't have IOMMU enabled by default at
>>> the boot time, then this approach won't work for the existing devices
>>> which won't ever get an updated bootloader.
>>
>> If the devices don't use an IOMMU, then there should be no problem. The
>> extra reserved-memory nodes would still be necessary to ensure that the
>> kernel doesn't reuse the framebuffer memory for the slab allocator, but
>> if no IOMMU is used, then the display controller accessing the memory
>> isn't going to cause problems other than perhaps scanning out data that
>> is no longer a framebuffer.
>>
>> There should also be no problem for devices with an old bootloader
>> because this code is triggered by the presence of a reserved-memory node
>> referenced via the memory-region property. Devices with an old
>> bootloader should continue to work as they did before. Although I
>> suppose they would start faulting once we enable DMA/IOMMU integration
>> for Tegra SMMU if they have a bootloader that does initialize display to
>> actively scan out during boot.
>>
>>> I think Robin Murphy already suggested that we should simply create
>>> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
>>> then replace it with an explicitly created domain within the drivers.
>>
>> I don't recall reading about that suggestion. So does this mean that for
>> certain devices we'd want to basically passthrough by default and then
>> at some point during boot take over with a properly managed IOMMU
>> domain?
> 
> Yes, my understanding that this is what Robin suggested here:
> 
> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/

Just to clarify, what I was talking about there is largely orthogonal to 
the issue here. That was about systems with limited translation 
resources letting translation be specifically opt-in by IOMMU-aware 
drivers. It probably *would* happen to obviate the issue of disrupting 
live DMA at boot time on these particular Tegra platforms, but we still 
need something like Thierry's solution in general, since IOMMU drivers 
may have no other way to determine whether devices are active at boot 
and they have to take care to avoid breaking anything - e.g. SMMUv3 will 
at a bare minimum need to set up *some* form of valid stream table entry 
for the relevant device(s) right at the beginning where we first probe 
and reset the SMMU itself, regardless of what happens with domains and 
addresses later down the line.

>> The primary goal here is to move towards using the DMA API rather than
>> the IOMMU API directly, so we don't really have the option of replacing
>> with an explicitly created domain. Unless we have code in the DMA/IOMMU
>> code that does this somehow.
>>
>> But I'm not sure what would be a good way to mark certain devices as
>> needing an identity domain by default. Do we still use the reserved-
>> memory node for that?
> 
> The reserved-memory indeed shouldn't be needed for resolving the
> implicit IOMMU problem since we could mark certain devices within the
> kernel IOMMU driver.
> 
> I haven't got around to trying to implement the implicit IOMMU support
> yet, but I suppose we could implement the def_domain_type() hook in the
> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
> devices. Then the Display/VDE drivers will take over the identity domain
> and replace it with the explicit domain.

FWIW I've already cooked up identity domain support for tegra-gart; I 
was planning on tackling it for tegra-smmu as well for the next version 
of my arm default domains series (which will be after the next -rc1 now 
since I'm just about to take some long-overdue holiday).

>> That would still require some sort of flag to
>> specify which reserved-memory regions would need this identity mapping
>> because, as was pointed out in earlier review, some devices may have
>> reserved-memory regions that are not meant to be identity mapped.
> 
> Please note that the reserved-memory approach also creates problem for
> selection of a large CMA region if FB is located somewhere in a middle
> of DRAM.
> 
> I already see that the FB's reserved-memory will break CMA for Nexus 7
> and Acer A500 because CMA area overlaps with the bootloader's FB :)
> 
> Also keep in mind that initrd needs a location too and location usually
> hardwired in a bootloader. Hence it increases pressure on the CMA selection.
> 
>>> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
>>> kernel's cmdline with the physical location of the framebuffer in
>>> memory. Maybe we could support this option?
>>
>> I'm not a big fan of that command-line option, but I also realize that
>> for older bootloaders that's probably the only option we have. I don't
>> suppose all of the devices support U-Boot?
> 
> Majority of devices in a wild don't use u-boot and they have a
> locked-down bootloader. Still it's possible to chain-load u-boot or
> bypass the "security" and replace the bootloader, but these approaches
> aren't widely supported because they take a lot of effort to be
> implemented and maintained.
> 
> Even those devices that use proper u-boot usually never updating it and
> are running some ancient version. You can't ignore all those people :)
> 
>> Because ideally we'd just
>> translate from tegra_fbmem=... to reserved-memory region there so that
>> we don't have to carry backwards-compatibility code for these purely
>> downstream bootloaders.
> 
> IIRC, in the past Robin Murphy was suggesting to read out hardware state
> early during kernel boot in order to find what regions are in use by
> hardware.

I doubt I suggested that in general, because I've always firmly believed 
it to be a terrible idea. I've debugged too many cases where firmware or 
kexec has inadvertently left DMA running and corrupted kernel memory, so 
in general we definitely *don't* want to blindly trust random hardware 
state. Anything I may have said in relation to Qualcomm's fundamentally 
broken hypervisor/bootloader setup should not be considered outside that 
specific context ;)

Robin.

> I think it should be easy to do for the display controller since we
> could check clock and PD states in order to decide whether DC's IO could
> be accessed and then read out the FB pointer and size. I guess it should
> take about hundred lines of code.
> 
> But the easiest way should be to ignore this trouble for devices that
> have IOMMU disabled by default and simply allow display to show garbage.
> Nobody ever complained about this for the past 7+ years :)
> 
> Hence implementing the dummy-identity domain support should be enough
> for solving the problem, at least this should work for pre-T186 devices.
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-25 12:39       ` Robin Murphy
@ 2020-09-25 13:21         ` Dmitry Osipenko
  2020-11-05 16:23           ` Thierry Reding
  2020-09-25 13:52         ` Dmitry Osipenko
  2020-11-05 15:57         ` Thierry Reding
  2 siblings, 1 reply; 24+ messages in thread
From: Dmitry Osipenko @ 2020-09-25 13:21 UTC (permalink / raw)
  To: Robin Murphy, Thierry Reding
  Cc: devicetree, Frank Rowand, linux-kernel, iommu, Rob Herring,
	linux-tegra, Will Deacon

25.09.2020 15:39, Robin Murphy пишет:
...
>> IIRC, in the past Robin Murphy was suggesting to read out hardware state
>> early during kernel boot in order to find what regions are in use by
>> hardware.
> 
> I doubt I suggested that in general, because I've always firmly believed
> it to be a terrible idea. I've debugged too many cases where firmware or
> kexec has inadvertently left DMA running and corrupted kernel memory, so
> in general we definitely *don't* want to blindly trust random hardware
> state. Anything I may have said in relation to Qualcomm's fundamentally
> broken hypervisor/bootloader setup should not be considered outside that
> specific context ;)
> 
> Robin.
> 
>> I think it should be easy to do for the display controller since we
>> could check clock and PD states in order to decide whether DC's IO could
>> be accessed and then read out the FB pointer and size. I guess it should
>> take about hundred lines of code.

The active DMA is indeed very dangerous, but it's a bit less dangerous
in a case of read-only DMA.

I got another idea of how we could benefit from the active display
hardware. Maybe we could do the following:

1. Check whether display is active

2. Allocate CMA that matches the FB size

3. Create identity mapping for the CMA

4. Switch display framebuffer to our CMA

5. Create very early simple-framebuffer out of the CMA

6. Once Tegra DRM driver is loaded, it will kick out the simple-fb, and
thus, release temporal CMA and identity mapping.

This will provide us with a very early framebuffer output and it will
work on all devices out-of-the-box!
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-25 12:39       ` Robin Murphy
  2020-09-25 13:21         ` Dmitry Osipenko
@ 2020-09-25 13:52         ` Dmitry Osipenko
  2020-11-05 15:57         ` Thierry Reding
  2 siblings, 0 replies; 24+ messages in thread
From: Dmitry Osipenko @ 2020-09-25 13:52 UTC (permalink / raw)
  To: Robin Murphy, Thierry Reding
  Cc: devicetree, Frank Rowand, linux-kernel, iommu, Rob Herring,
	linux-tegra, Will Deacon

25.09.2020 15:39, Robin Murphy пишет:
...
>> Yes, my understanding that this is what Robin suggested here:
>>
>> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
>>
> 
> Just to clarify, what I was talking about there is largely orthogonal to
> the issue here. That was about systems with limited translation
> resources letting translation be specifically opt-in by IOMMU-aware
> drivers. It probably *would* happen to obviate the issue of disrupting
> live DMA at boot time on these particular Tegra platforms, but we still
> need something like Thierry's solution in general, since IOMMU drivers
> may have no other way to determine whether devices are active at boot
> and they have to take care to avoid breaking anything - e.g. SMMUv3 will
> at a bare minimum need to set up *some* form of valid stream table entry
> for the relevant device(s) right at the beginning where we first probe
> and reset the SMMU itself, regardless of what happens with domains and
> addresses later down the line.

Yes, I only meant that yours suggestion also should be useful here.
Anyways, thank you for the clarification :)

I agree that the Thierry's proposal is good! But it needs some more
thought yet because it's not very applicable to the current devices.

>>> The primary goal here is to move towards using the DMA API rather than
>>> the IOMMU API directly, so we don't really have the option of replacing
>>> with an explicitly created domain. Unless we have code in the DMA/IOMMU
>>> code that does this somehow.
>>>
>>> But I'm not sure what would be a good way to mark certain devices as
>>> needing an identity domain by default. Do we still use the reserved-
>>> memory node for that?
>>
>> The reserved-memory indeed shouldn't be needed for resolving the
>> implicit IOMMU problem since we could mark certain devices within the
>> kernel IOMMU driver.
>>
>> I haven't got around to trying to implement the implicit IOMMU support
>> yet, but I suppose we could implement the def_domain_type() hook in the
>> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
>> devices. Then the Display/VDE drivers will take over the identity domain
>> and replace it with the explicit domain.
> 
> FWIW I've already cooked up identity domain support for tegra-gart; I
> was planning on tackling it for tegra-smmu as well for the next version
> of my arm default domains series (which will be after the next -rc1 now
> since I'm just about to take some long-overdue holiday).

Very nice! Maybe we will have some more food for the discussion by the
time you'll return. Have a good time!
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-24 16:23     ` Dmitry Osipenko
  2020-09-25 12:39       ` Robin Murphy
@ 2020-11-05 15:49       ` Thierry Reding
  2021-01-12 19:00         ` Dmitry Osipenko
  1 sibling, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-11-05 15:49 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-tegra, Will Deacon


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

On Thu, Sep 24, 2020 at 07:23:34PM +0300, Dmitry Osipenko wrote:
> 24.09.2020 17:01, Thierry Reding пишет:
> > On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
> >> 04.09.2020 15:59, Thierry Reding пишет:
> >>> From: Thierry Reding <treding@nvidia.com>
> >>>
> >>> Reserved memory regions can be marked as "active" if hardware is
> >>> expected to access the regions during boot and before the operating
> >>> system can take control. One example where this is useful is for the
> >>> operating system to infer whether the region needs to be identity-
> >>> mapped through an IOMMU.
> >>>
> >>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >>> ---
> >>>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> >>>  1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >>> index 4dd20de6977f..163d2927e4fc 100644
> >>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
> >>>        able to reclaim it back. Typically that means that the operating
> >>>        system can use that region to store volatile or cached data that
> >>>        can be otherwise regenerated or migrated elsewhere.
> >>> +active (optional) - empty property
> >>> +    - If this property is set for a reserved memory region, it indicates
> >>> +      that some piece of hardware may be actively accessing this region.
> >>> +      Should the operating system want to enable IOMMU protection for a
> >>> +      device, all active memory regions must have been identity-mapped
> >>> +      in order to ensure that non-quiescent hardware during boot can
> >>> +      continue to access the memory.
> >>>  
> >>>  Linux implementation note:
> >>>  - If a "linux,cma-default" property is present, then Linux will use the
> >>>
> >>
> >> Hi,
> >>
> >> Could you please explain what devices need this quirk? I see that you're
> >> targeting Tegra SMMU driver, which means that it should be some pre-T186
> >> device.
> > 
> > Primarily I'm looking at Tegra210 and later, because on earlier devices
> > the bootloader doesn't consistently initialize display. I know that it
> > does on some devices, but not all of them.
> 
> AFAIK, all tablet devices starting with Tegra20 that have display panel
> are initializing display at a boot time for showing splash screen. This
> includes all T20/T30/T114 tablets that are already supported by upstream
> kernel.
> 
> > This same code should also
> > work on Tegra186 and later (with an ARM SMMU) although the situation is
> > slightly more complicated there because IOMMU translations will fault by
> > default long before these identity mappings can be established.
> > 
> >> Is this reservation needed for some device that has display
> >> hardwired to a very specific IOMMU domain at the boot time?
> > 
> > No, this is only used to convey information about the active framebuffer
> > to the kernel. In practice the DMA/IOMMU code will use this information
> > to establish a 1:1 mapping on whatever IOMMU domain that was picked for
> > display.
> > 
> >> If you're targeting devices that don't have IOMMU enabled by default at
> >> the boot time, then this approach won't work for the existing devices
> >> which won't ever get an updated bootloader.
> > 
> > If the devices don't use an IOMMU, then there should be no problem. The
> > extra reserved-memory nodes would still be necessary to ensure that the
> > kernel doesn't reuse the framebuffer memory for the slab allocator, but
> > if no IOMMU is used, then the display controller accessing the memory
> > isn't going to cause problems other than perhaps scanning out data that
> > is no longer a framebuffer.
> > 
> > There should also be no problem for devices with an old bootloader
> > because this code is triggered by the presence of a reserved-memory node
> > referenced via the memory-region property. Devices with an old
> > bootloader should continue to work as they did before. Although I
> > suppose they would start faulting once we enable DMA/IOMMU integration
> > for Tegra SMMU if they have a bootloader that does initialize display to
> > actively scan out during boot.
> > 
> >> I think Robin Murphy already suggested that we should simply create
> >> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
> >> then replace it with an explicitly created domain within the drivers.
> > 
> > I don't recall reading about that suggestion. So does this mean that for
> > certain devices we'd want to basically passthrough by default and then
> > at some point during boot take over with a properly managed IOMMU
> > domain?
> 
> Yes, my understanding that this is what Robin suggested here:
> 
> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
> 
> > The primary goal here is to move towards using the DMA API rather than
> > the IOMMU API directly, so we don't really have the option of replacing
> > with an explicitly created domain. Unless we have code in the DMA/IOMMU
> > code that does this somehow.
> > 
> > But I'm not sure what would be a good way to mark certain devices as
> > needing an identity domain by default. Do we still use the reserved-
> > memory node for that?
> 
> The reserved-memory indeed shouldn't be needed for resolving the
> implicit IOMMU problem since we could mark certain devices within the
> kernel IOMMU driver.
> 
> I haven't got around to trying to implement the implicit IOMMU support
> yet, but I suppose we could implement the def_domain_type() hook in the
> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
> devices. Then the Display/VDE drivers will take over the identity domain
> and replace it with the explicit domain.

Actually, the plan (and part of the reason for this patch) is to
transition the display driver over to using the DMA API rather than
creating IOMMU domains explicitly.

Explicit IOMMU usage currently works around this prior to Tegra186
because IOMMU translations are not enabled until after the device has
been attached to the IOMMU, at which point a mapping will already have
been created. It's also part of the reason why we don't support DMA-
type domains yet on Tegra SMMU, because as soon as we do, this would
cause a fault storm during boot.

On Tegra186 this same problem exists because the driver does support DMA
type domains and hence the kernel will try to set those up for display
during early boot and before the driver has had a chance to set up
mappings (or quiesce the display hardware).

> > That would still require some sort of flag to
> > specify which reserved-memory regions would need this identity mapping
> > because, as was pointed out in earlier review, some devices may have
> > reserved-memory regions that are not meant to be identity mapped.
> 
> Please note that the reserved-memory approach also creates problem for
> selection of a large CMA region if FB is located somewhere in a middle
> of DRAM.
> 
> I already see that the FB's reserved-memory will break CMA for Nexus 7
> and Acer A500 because CMA area overlaps with the bootloader's FB :)
> 
> Also keep in mind that initrd needs a location too and location usually
> hardwired in a bootloader. Hence it increases pressure on the CMA selection.

I do understand those issues, but there's really not a lot we can do
about it. It's wrong for CMA to overlap with the framebuffer from which
the display hardware just keeps scanning out, so all that these reserved
memory nodes do is actually fix a long-standing bug (depending on how
paranoid you are, you might even consider it a security hole).

For older devices that don't have a bootloader that properly defines
this memory as reserved, they should be able to continue to use a CMA
that overlaps with the framebuffer and work just like before.

> >> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
> >> kernel's cmdline with the physical location of the framebuffer in
> >> memory. Maybe we could support this option?
> > 
> > I'm not a big fan of that command-line option, but I also realize that
> > for older bootloaders that's probably the only option we have. I don't
> > suppose all of the devices support U-Boot?
> 
> Majority of devices in a wild don't use u-boot and they have a
> locked-down bootloader. Still it's possible to chain-load u-boot or
> bypass the "security" and replace the bootloader, but these approaches
> aren't widely supported because they take a lot of effort to be
> implemented and maintained.
> 
> Even those devices that use proper u-boot usually never updating it and
> are running some ancient version. You can't ignore all those people :)

I'm not trying to ignore all those people, but at the same time I don't
think legacy devices about which we can't do much should prevent new
devices from doing the right thing and properly reserving memory that
the kernel isn't supposed to use.

You already suggested that we could choose an identity domain by default
for VDE and display for this. That sounds like a good compromise to me,
but I think we should do that only on platforms where we can't implement
this properly.

> > Because ideally we'd just
> > translate from tegra_fbmem=... to reserved-memory region there so that
> > we don't have to carry backwards-compatibility code for these purely
> > downstream bootloaders.
> 
> IIRC, in the past Robin Murphy was suggesting to read out hardware state
> early during kernel boot in order to find what regions are in use by
> hardware.
> 
> I think it should be easy to do for the display controller since we
> could check clock and PD states in order to decide whether DC's IO could
> be accessed and then read out the FB pointer and size. I guess it should
> take about hundred lines of code.
> 
> But the easiest way should be to ignore this trouble for devices that
> have IOMMU disabled by default and simply allow display to show garbage.
> Nobody ever complained about this for the past 7+ years :)
> 
> Hence implementing the dummy-identity domain support should be enough
> for solving the problem, at least this should work for pre-T186 devices.

I'd still like to do this properly at least on Tegra210 as well. It
should be possible to come up with a set of criteria where the dummy
identity domain should be used and where it shouldn't. I think that's
the easiest part of this and can easily be implemented as a couple-of-
lines quirk in some kernel driver.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-25 12:39       ` Robin Murphy
  2020-09-25 13:21         ` Dmitry Osipenko
  2020-09-25 13:52         ` Dmitry Osipenko
@ 2020-11-05 15:57         ` Thierry Reding
  2 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2020-11-05 15:57 UTC (permalink / raw)
  To: Robin Murphy
  Cc: devicetree, Frank Rowand, linux-kernel, iommu, Rob Herring,
	linux-tegra, Dmitry Osipenko, Will Deacon


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

On Fri, Sep 25, 2020 at 01:39:07PM +0100, Robin Murphy wrote:
> On 2020-09-24 17:23, Dmitry Osipenko wrote:
> > 24.09.2020 17:01, Thierry Reding пишет:
> > > On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
> > > > 04.09.2020 15:59, Thierry Reding пишет:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > 
> > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > expected to access the regions during boot and before the operating
> > > > > system can take control. One example where this is useful is for the
> > > > > operating system to infer whether the region needs to be identity-
> > > > > mapped through an IOMMU.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > > ---
> > > > >   .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > index 4dd20de6977f..163d2927e4fc 100644
> > > > > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > > > @@ -63,6 +63,13 @@ reusable (optional) - empty property
> > > > >         able to reclaim it back. Typically that means that the operating
> > > > >         system can use that region to store volatile or cached data that
> > > > >         can be otherwise regenerated or migrated elsewhere.
> > > > > +active (optional) - empty property
> > > > > +    - If this property is set for a reserved memory region, it indicates
> > > > > +      that some piece of hardware may be actively accessing this region.
> > > > > +      Should the operating system want to enable IOMMU protection for a
> > > > > +      device, all active memory regions must have been identity-mapped
> > > > > +      in order to ensure that non-quiescent hardware during boot can
> > > > > +      continue to access the memory.
> > > > >   Linux implementation note:
> > > > >   - If a "linux,cma-default" property is present, then Linux will use the
> > > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > Could you please explain what devices need this quirk? I see that you're
> > > > targeting Tegra SMMU driver, which means that it should be some pre-T186
> > > > device.
> > > 
> > > Primarily I'm looking at Tegra210 and later, because on earlier devices
> > > the bootloader doesn't consistently initialize display. I know that it
> > > does on some devices, but not all of them.
> > 
> > AFAIK, all tablet devices starting with Tegra20 that have display panel
> > are initializing display at a boot time for showing splash screen. This
> > includes all T20/T30/T114 tablets that are already supported by upstream
> > kernel.
> > 
> > > This same code should also
> > > work on Tegra186 and later (with an ARM SMMU) although the situation is
> > > slightly more complicated there because IOMMU translations will fault by
> > > default long before these identity mappings can be established.
> > > 
> > > > Is this reservation needed for some device that has display
> > > > hardwired to a very specific IOMMU domain at the boot time?
> > > 
> > > No, this is only used to convey information about the active framebuffer
> > > to the kernel. In practice the DMA/IOMMU code will use this information
> > > to establish a 1:1 mapping on whatever IOMMU domain that was picked for
> > > display.
> > > 
> > > > If you're targeting devices that don't have IOMMU enabled by default at
> > > > the boot time, then this approach won't work for the existing devices
> > > > which won't ever get an updated bootloader.
> > > 
> > > If the devices don't use an IOMMU, then there should be no problem. The
> > > extra reserved-memory nodes would still be necessary to ensure that the
> > > kernel doesn't reuse the framebuffer memory for the slab allocator, but
> > > if no IOMMU is used, then the display controller accessing the memory
> > > isn't going to cause problems other than perhaps scanning out data that
> > > is no longer a framebuffer.
> > > 
> > > There should also be no problem for devices with an old bootloader
> > > because this code is triggered by the presence of a reserved-memory node
> > > referenced via the memory-region property. Devices with an old
> > > bootloader should continue to work as they did before. Although I
> > > suppose they would start faulting once we enable DMA/IOMMU integration
> > > for Tegra SMMU if they have a bootloader that does initialize display to
> > > actively scan out during boot.
> > > 
> > > > I think Robin Murphy already suggested that we should simply create
> > > > a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
> > > > then replace it with an explicitly created domain within the drivers.
> > > 
> > > I don't recall reading about that suggestion. So does this mean that for
> > > certain devices we'd want to basically passthrough by default and then
> > > at some point during boot take over with a properly managed IOMMU
> > > domain?
> > 
> > Yes, my understanding that this is what Robin suggested here:
> > 
> > https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
> 
> Just to clarify, what I was talking about there is largely orthogonal to the
> issue here. That was about systems with limited translation resources
> letting translation be specifically opt-in by IOMMU-aware drivers. It
> probably *would* happen to obviate the issue of disrupting live DMA at boot
> time on these particular Tegra platforms, but we still need something like
> Thierry's solution in general, since IOMMU drivers may have no other way to
> determine whether devices are active at boot and they have to take care to
> avoid breaking anything - e.g. SMMUv3 will at a bare minimum need to set up
> *some* form of valid stream table entry for the relevant device(s) right at
> the beginning where we first probe and reset the SMMU itself, regardless of
> what happens with domains and addresses later down the line.
> 
> > > The primary goal here is to move towards using the DMA API rather than
> > > the IOMMU API directly, so we don't really have the option of replacing
> > > with an explicitly created domain. Unless we have code in the DMA/IOMMU
> > > code that does this somehow.
> > > 
> > > But I'm not sure what would be a good way to mark certain devices as
> > > needing an identity domain by default. Do we still use the reserved-
> > > memory node for that?
> > 
> > The reserved-memory indeed shouldn't be needed for resolving the
> > implicit IOMMU problem since we could mark certain devices within the
> > kernel IOMMU driver.
> > 
> > I haven't got around to trying to implement the implicit IOMMU support
> > yet, but I suppose we could implement the def_domain_type() hook in the
> > SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
> > devices. Then the Display/VDE drivers will take over the identity domain
> > and replace it with the explicit domain.
> 
> FWIW I've already cooked up identity domain support for tegra-gart; I was
> planning on tackling it for tegra-smmu as well for the next version of my
> arm default domains series (which will be after the next -rc1 now since I'm
> just about to take some long-overdue holiday).
> 
> > > That would still require some sort of flag to
> > > specify which reserved-memory regions would need this identity mapping
> > > because, as was pointed out in earlier review, some devices may have
> > > reserved-memory regions that are not meant to be identity mapped.
> > 
> > Please note that the reserved-memory approach also creates problem for
> > selection of a large CMA region if FB is located somewhere in a middle
> > of DRAM.
> > 
> > I already see that the FB's reserved-memory will break CMA for Nexus 7
> > and Acer A500 because CMA area overlaps with the bootloader's FB :)
> > 
> > Also keep in mind that initrd needs a location too and location usually
> > hardwired in a bootloader. Hence it increases pressure on the CMA selection.
> > 
> > > > Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
> > > > kernel's cmdline with the physical location of the framebuffer in
> > > > memory. Maybe we could support this option?
> > > 
> > > I'm not a big fan of that command-line option, but I also realize that
> > > for older bootloaders that's probably the only option we have. I don't
> > > suppose all of the devices support U-Boot?
> > 
> > Majority of devices in a wild don't use u-boot and they have a
> > locked-down bootloader. Still it's possible to chain-load u-boot or
> > bypass the "security" and replace the bootloader, but these approaches
> > aren't widely supported because they take a lot of effort to be
> > implemented and maintained.
> > 
> > Even those devices that use proper u-boot usually never updating it and
> > are running some ancient version. You can't ignore all those people :)
> > 
> > > Because ideally we'd just
> > > translate from tegra_fbmem=... to reserved-memory region there so that
> > > we don't have to carry backwards-compatibility code for these purely
> > > downstream bootloaders.
> > 
> > IIRC, in the past Robin Murphy was suggesting to read out hardware state
> > early during kernel boot in order to find what regions are in use by
> > hardware.
> 
> I doubt I suggested that in general, because I've always firmly believed it
> to be a terrible idea. I've debugged too many cases where firmware or kexec
> has inadvertently left DMA running and corrupted kernel memory, so in
> general we definitely *don't* want to blindly trust random hardware state.
> Anything I may have said in relation to Qualcomm's fundamentally broken
> hypervisor/bootloader setup should not be considered outside that specific
> context ;)

I agree with this. I have no doubt that it could be done, technically,
but at the same time this is code that would have to run very early on
and therefore would have to be built-in. So even if that's just a couple
of hundred lines of code, it still means that we'd need that couple of
hundred lines of code for potentially each platform supported by a given
multi-platform kernel, and that's quickly going to add up.

So I think transporting this knowledge in device tree is a fair
compromise. The bootloader knows exactly whether the display hardware
was left active, so it's easy to update the device tree accordingly
before passing it to the kernel and it allows us to use generic code to
perform this quirking.

For Tegra specifically I'm not even sure this would work on all
generations. For example on Tegra186, the BPMP provides access to
clocks, resets and powergates. So the BPMP is needed to determine
whether or not the display hardware is active (you need to query clocks,
resets and powergates before accessing registers, because accessing
registers of an unclocked, in-reset or powergated hardware block
crashes). However, the BPMP is also typically behind the SMMU, so that
would make for a nice cyclic dependency.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-25 13:21         ` Dmitry Osipenko
@ 2020-11-05 16:23           ` Thierry Reding
  0 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2020-11-05 16:23 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: devicetree, Will Deacon, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-tegra, Frank Rowand


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

On Fri, Sep 25, 2020 at 04:21:17PM +0300, Dmitry Osipenko wrote:
> 25.09.2020 15:39, Robin Murphy пишет:
> ...
> >> IIRC, in the past Robin Murphy was suggesting to read out hardware state
> >> early during kernel boot in order to find what regions are in use by
> >> hardware.
> > 
> > I doubt I suggested that in general, because I've always firmly believed
> > it to be a terrible idea. I've debugged too many cases where firmware or
> > kexec has inadvertently left DMA running and corrupted kernel memory, so
> > in general we definitely *don't* want to blindly trust random hardware
> > state. Anything I may have said in relation to Qualcomm's fundamentally
> > broken hypervisor/bootloader setup should not be considered outside that
> > specific context ;)
> > 
> > Robin.
> > 
> >> I think it should be easy to do for the display controller since we
> >> could check clock and PD states in order to decide whether DC's IO could
> >> be accessed and then read out the FB pointer and size. I guess it should
> >> take about hundred lines of code.
> 
> The active DMA is indeed very dangerous, but it's a bit less dangerous
> in a case of read-only DMA.
> 
> I got another idea of how we could benefit from the active display
> hardware. Maybe we could do the following:
> 
> 1. Check whether display is active
> 
> 2. Allocate CMA that matches the FB size
> 
> 3. Create identity mapping for the CMA
> 
> 4. Switch display framebuffer to our CMA
> 
> 5. Create very early simple-framebuffer out of the CMA
> 
> 6. Once Tegra DRM driver is loaded, it will kick out the simple-fb, and
> thus, release temporal CMA and identity mapping.
> 
> This will provide us with a very early framebuffer output and it will
> work on all devices out-of-the-box!

Well that's already kind of what this is trying to achieve, only
skipping the CMA step because the memory is already there and actively
being scanned out from. The problem with your sequence above is first
that you have to allocate from CMA, which means that this has to wait
until CMA becomes available. That's fairly early, but it's not
immediately there. Until you get to that point, there's always the
potential for the display controller to read out from memory that may
now be used for something else. As you said, read-only active DMA isn't
as dangerous as write DMA, but it's not very nice either.

Furthermore, your point 5. above requires device-specific knowledge and
as I mentioned earlier that requires a small, but not necessarily
trivial, device-specific driver to work, which is very impractical for
multi-platform kernels.

There's nothing preventing these reserved-memory regions from being
reused to implement simple-framebuffer. I could in fact imagine a fairly
simple extension to the existing simple-framebuffer binding that could
look like this for Tegra:

	dc@52000000 {
		compatible = "nvidia,tegra210-display", "simple-framebuffer";
		...
		memory-region = <&framebuffer>;
		width = <1920>;
		height = <1080>;
		stride = <7680>;
		format = "r8g8b8";
		...
	};

That's not dissimilar to what you're proposing above, except that it
moves everything before step 5. into the bootloader's responsibility and
therefore avoids the need for hardware-specific early display code in
the kernel.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-09-24 11:27     ` Thierry Reding
@ 2020-11-05 16:43       ` Thierry Reding
  2020-11-05 17:47         ` Robin Murphy
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-11-05 16:43 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu, Will Deacon


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

On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
> On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> > On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Reserved memory regions can be marked as "active" if hardware is
> > > > expected to access the regions during boot and before the operating
> > > > system can take control. One example where this is useful is for the
> > > > operating system to infer whether the region needs to be identity-
> > > > mapped through an IOMMU.
> > > 
> > > I like simple solutions, but this hardly seems adequate to solve the 
> > > problem of passing IOMMU setup from bootloader/firmware to the OS. Like 
> > > what is the IOVA that's supposed to be used if identity mapping is not 
> > > used?
> > 
> > The assumption here is that if the region is not active there is no need
> > for the IOVA to be specified because the kernel will allocate memory and
> > assign any IOVA of its choosing.
> > 
> > Also, note that this is not meant as a way of passing IOMMU setup from
> > the bootloader or firmware to the OS. The purpose of this is to specify
> > that some region of memory is actively being accessed during boot. The
> > particular case that I'm looking at is where the bootloader set up a
> > splash screen and keeps it on during boot. The bootloader has not set up
> > an IOMMU mapping and the identity mapping serves as a way of keeping the
> > accesses by the display hardware working during the transitional period
> > after the IOMMU translations have been enabled by the kernel but before
> > the kernel display driver has had a chance to set up its own IOMMU
> > mappings.
> > 
> > > If you know enough about the regions to assume identity mapping, then 
> > > can't you know if active or not?
> > 
> > We could alternatively add some property that describes the region as
> > requiring an identity mapping. But note that we can't make any
> > assumptions here about the usage of these regions because the IOMMU
> > driver simply has no way of knowing what they are being used for.
> > 
> > Some additional information is required in device tree for the IOMMU
> > driver to be able to make that decision.
> 
> Rob, can you provide any hints on exactly how you want to move this
> forward? I don't know in what direction you'd like to proceed.

Hi Rob,

do you have any suggestions on how to proceed with this? I'd like to get
this moving again because it's something that's been nagging me for some
months now. It also requires changes across two levels in the bootloader
stack as well as Linux and it takes quite a bit of work to make all the
changes, so before I go and rewrite everything I'd like to get the DT
bindings sorted out first.

So just to summarize why I think this simple solution is good enough: it
tries to solve a very narrow and simple problem. This is not an attempt
at describing the firmware's full IOMMU setup to the kernel. In fact, it
is primarily targetted at cases where the firmware hasn't setup an IOMMU
at all, and we just want to make sure that when the kernel takes over
and does want to enable the IOMMU, that all the regions that are
actively being accessed by non-quiesced hardware (the most typical
example would be a framebuffer scanning out a splat screen or animation,
but it could equally well be some sort of welcoming tone or music being
played back) are described in device tree.

In other words, and this is perhaps better answering your second
question: in addition to describing reserved memory regions, we want to
add a bit of information here about the usage of these memory regions.
Some memory regions may contain information that the kernel may want to
use (such an external memory frequency scaling tables) and those I would
describe as "inactive" memory because it isn't being accessed by
hardware. The framebuffer in this case is the opposite and it is being
actively accessed (hence it is marked "active") by hardware while the
kernel is busy setting everything up so that it can reconfigure that
hardware and take over with its own framebuffer (for the console, for
example). It's also not so much that we know enough about the region to
assume it needs identity mapping. We don't really care about that from
the DT point of view. In fact, depending on the rest of the system
configuration, we may not need identity mapping (i.e. if none of the
users of the reserved memory region are behind an IOMMU). But the point
here is that the IOMMU drivers can use this "active" property to
determine that if a device is using an "active" region and it is behind
an IOMMU, then it must identity map that region in order for the
hardware, which is not under the kernel's control yet, to be able to
continue to access that memory through an IOMMU mapping.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-11-05 16:43       ` Thierry Reding
@ 2020-11-05 17:47         ` Robin Murphy
  2020-11-06 15:25           ` Thierry Reding
  0 siblings, 1 reply; 24+ messages in thread
From: Robin Murphy @ 2020-11-05 17:47 UTC (permalink / raw)
  To: Thierry Reding, Rob Herring
  Cc: devicetree, iommu, Frank Rowand, linux-kernel, Will Deacon

On 2020-11-05 16:43, Thierry Reding wrote:
> On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
>> On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
>>> On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
>>>> On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Reserved memory regions can be marked as "active" if hardware is
>>>>> expected to access the regions during boot and before the operating
>>>>> system can take control. One example where this is useful is for the
>>>>> operating system to infer whether the region needs to be identity-
>>>>> mapped through an IOMMU.
>>>>
>>>> I like simple solutions, but this hardly seems adequate to solve the
>>>> problem of passing IOMMU setup from bootloader/firmware to the OS. Like
>>>> what is the IOVA that's supposed to be used if identity mapping is not
>>>> used?
>>>
>>> The assumption here is that if the region is not active there is no need
>>> for the IOVA to be specified because the kernel will allocate memory and
>>> assign any IOVA of its choosing.
>>>
>>> Also, note that this is not meant as a way of passing IOMMU setup from
>>> the bootloader or firmware to the OS. The purpose of this is to specify
>>> that some region of memory is actively being accessed during boot. The
>>> particular case that I'm looking at is where the bootloader set up a
>>> splash screen and keeps it on during boot. The bootloader has not set up
>>> an IOMMU mapping and the identity mapping serves as a way of keeping the
>>> accesses by the display hardware working during the transitional period
>>> after the IOMMU translations have been enabled by the kernel but before
>>> the kernel display driver has had a chance to set up its own IOMMU
>>> mappings.
>>>
>>>> If you know enough about the regions to assume identity mapping, then
>>>> can't you know if active or not?
>>>
>>> We could alternatively add some property that describes the region as
>>> requiring an identity mapping. But note that we can't make any
>>> assumptions here about the usage of these regions because the IOMMU
>>> driver simply has no way of knowing what they are being used for.
>>>
>>> Some additional information is required in device tree for the IOMMU
>>> driver to be able to make that decision.
>>
>> Rob, can you provide any hints on exactly how you want to move this
>> forward? I don't know in what direction you'd like to proceed.
> 
> Hi Rob,
> 
> do you have any suggestions on how to proceed with this? I'd like to get
> this moving again because it's something that's been nagging me for some
> months now. It also requires changes across two levels in the bootloader
> stack as well as Linux and it takes quite a bit of work to make all the
> changes, so before I go and rewrite everything I'd like to get the DT
> bindings sorted out first.
> 
> So just to summarize why I think this simple solution is good enough: it
> tries to solve a very narrow and simple problem. This is not an attempt
> at describing the firmware's full IOMMU setup to the kernel. In fact, it
> is primarily targetted at cases where the firmware hasn't setup an IOMMU
> at all, and we just want to make sure that when the kernel takes over
> and does want to enable the IOMMU, that all the regions that are
> actively being accessed by non-quiesced hardware (the most typical
> example would be a framebuffer scanning out a splat screen or animation,
> but it could equally well be some sort of welcoming tone or music being
> played back) are described in device tree.
> 
> In other words, and this is perhaps better answering your second
> question: in addition to describing reserved memory regions, we want to
> add a bit of information here about the usage of these memory regions.
> Some memory regions may contain information that the kernel may want to
> use (such an external memory frequency scaling tables) and those I would
> describe as "inactive" memory because it isn't being accessed by
> hardware. The framebuffer in this case is the opposite and it is being
> actively accessed (hence it is marked "active") by hardware while the
> kernel is busy setting everything up so that it can reconfigure that
> hardware and take over with its own framebuffer (for the console, for
> example). It's also not so much that we know enough about the region to
> assume it needs identity mapping. We don't really care about that from
> the DT point of view. In fact, depending on the rest of the system
> configuration, we may not need identity mapping (i.e. if none of the
> users of the reserved memory region are behind an IOMMU). But the point
> here is that the IOMMU drivers can use this "active" property to
> determine that if a device is using an "active" region and it is behind
> an IOMMU, then it must identity map that region in order for the
> hardware, which is not under the kernel's control yet, to be able to
> continue to access that memory through an IOMMU mapping.

Hmm, "active" is not a property of the memory itself, though, it's 
really a property of the device accessing it. If several distinct 
devices share a carveout region, and for simplicity the bootloader marks 
it as active because one of those devices happens to be using some part 
of it at boot, we don't really want to have to do all the reserved 
region setup for all the other devices unnecessarily, when all that 
matters is not disrupting one of them when resetting the IOMMU.

That leads to another possible hiccup - some bindings already have a 
defined meaning for a "memory-region" property. If we use that to point 
to some small region for a temporary low-resolution bootsplash screen 
for visibility to an IOMMU driver, the device's own driver might also 
interpret it as a private carveout from which it is expected to allocate 
everything, and thus could end up failing to work well or at all.

I agree that we should only need a relatively simple binding, and that 
piggybacking off reserved-memory nodes seems like an ideal way of 
getting address range descriptions without too much extra complexity; 
the tricky part is how best to associate those with the other 
information needed, which is really the "iommus" property of the 
relevant device, and how to make it as generically discoverable as 
possible. Perhaps it might be workable to follow almost the same 
approach but with a dedicated property (e.g. "active-memory-region") 
that the IOMMU code can simply scan the DT for to determine relevant 
device nodes. Otherwise properties on the IOMMU node itself would seem 
the next most practical option.

We've also finally got things going on the IORT RMR side[1], which helps 
add a bit more shape to things too; beyond the actual firmware parsing, 
DT and ACPI systems should definitely be converging on the same internal 
implementation in the IOMMU layer.

Robin.

[1] 
https://lore.kernel.org/linux-iommu/20201027112646.44680-1-shameerali.kolothum.thodi@huawei.com/
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-11-05 17:47         ` Robin Murphy
@ 2020-11-06 15:25           ` Thierry Reding
  2020-11-10 19:33             ` Thierry Reding
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-11-06 15:25 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Rob Herring, devicetree, Will Deacon, linux-kernel, iommu, Frank Rowand


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

On Thu, Nov 05, 2020 at 05:47:21PM +0000, Robin Murphy wrote:
> On 2020-11-05 16:43, Thierry Reding wrote:
> > On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
> > > On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> > > > On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > > > > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > 
> > > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > > expected to access the regions during boot and before the operating
> > > > > > system can take control. One example where this is useful is for the
> > > > > > operating system to infer whether the region needs to be identity-
> > > > > > mapped through an IOMMU.
> > > > > 
> > > > > I like simple solutions, but this hardly seems adequate to solve the
> > > > > problem of passing IOMMU setup from bootloader/firmware to the OS. Like
> > > > > what is the IOVA that's supposed to be used if identity mapping is not
> > > > > used?
> > > > 
> > > > The assumption here is that if the region is not active there is no need
> > > > for the IOVA to be specified because the kernel will allocate memory and
> > > > assign any IOVA of its choosing.
> > > > 
> > > > Also, note that this is not meant as a way of passing IOMMU setup from
> > > > the bootloader or firmware to the OS. The purpose of this is to specify
> > > > that some region of memory is actively being accessed during boot. The
> > > > particular case that I'm looking at is where the bootloader set up a
> > > > splash screen and keeps it on during boot. The bootloader has not set up
> > > > an IOMMU mapping and the identity mapping serves as a way of keeping the
> > > > accesses by the display hardware working during the transitional period
> > > > after the IOMMU translations have been enabled by the kernel but before
> > > > the kernel display driver has had a chance to set up its own IOMMU
> > > > mappings.
> > > > 
> > > > > If you know enough about the regions to assume identity mapping, then
> > > > > can't you know if active or not?
> > > > 
> > > > We could alternatively add some property that describes the region as
> > > > requiring an identity mapping. But note that we can't make any
> > > > assumptions here about the usage of these regions because the IOMMU
> > > > driver simply has no way of knowing what they are being used for.
> > > > 
> > > > Some additional information is required in device tree for the IOMMU
> > > > driver to be able to make that decision.
> > > 
> > > Rob, can you provide any hints on exactly how you want to move this
> > > forward? I don't know in what direction you'd like to proceed.
> > 
> > Hi Rob,
> > 
> > do you have any suggestions on how to proceed with this? I'd like to get
> > this moving again because it's something that's been nagging me for some
> > months now. It also requires changes across two levels in the bootloader
> > stack as well as Linux and it takes quite a bit of work to make all the
> > changes, so before I go and rewrite everything I'd like to get the DT
> > bindings sorted out first.
> > 
> > So just to summarize why I think this simple solution is good enough: it
> > tries to solve a very narrow and simple problem. This is not an attempt
> > at describing the firmware's full IOMMU setup to the kernel. In fact, it
> > is primarily targetted at cases where the firmware hasn't setup an IOMMU
> > at all, and we just want to make sure that when the kernel takes over
> > and does want to enable the IOMMU, that all the regions that are
> > actively being accessed by non-quiesced hardware (the most typical
> > example would be a framebuffer scanning out a splat screen or animation,
> > but it could equally well be some sort of welcoming tone or music being
> > played back) are described in device tree.
> > 
> > In other words, and this is perhaps better answering your second
> > question: in addition to describing reserved memory regions, we want to
> > add a bit of information here about the usage of these memory regions.
> > Some memory regions may contain information that the kernel may want to
> > use (such an external memory frequency scaling tables) and those I would
> > describe as "inactive" memory because it isn't being accessed by
> > hardware. The framebuffer in this case is the opposite and it is being
> > actively accessed (hence it is marked "active") by hardware while the
> > kernel is busy setting everything up so that it can reconfigure that
> > hardware and take over with its own framebuffer (for the console, for
> > example). It's also not so much that we know enough about the region to
> > assume it needs identity mapping. We don't really care about that from
> > the DT point of view. In fact, depending on the rest of the system
> > configuration, we may not need identity mapping (i.e. if none of the
> > users of the reserved memory region are behind an IOMMU). But the point
> > here is that the IOMMU drivers can use this "active" property to
> > determine that if a device is using an "active" region and it is behind
> > an IOMMU, then it must identity map that region in order for the
> > hardware, which is not under the kernel's control yet, to be able to
> > continue to access that memory through an IOMMU mapping.
> 
> Hmm, "active" is not a property of the memory itself, though, it's really a
> property of the device accessing it. If several distinct devices share a
> carveout region, and for simplicity the bootloader marks it as active
> because one of those devices happens to be using some part of it at boot, we
> don't really want to have to do all the reserved region setup for all the
> other devices unnecessarily, when all that matters is not disrupting one of
> them when resetting the IOMMU.
> 
> That leads to another possible hiccup - some bindings already have a defined
> meaning for a "memory-region" property. If we use that to point to some
> small region for a temporary low-resolution bootsplash screen for visibility
> to an IOMMU driver, the device's own driver might also interpret it as a
> private carveout from which it is expected to allocate everything, and thus
> could end up failing to work well or at all.
> 
> I agree that we should only need a relatively simple binding, and that
> piggybacking off reserved-memory nodes seems like an ideal way of getting
> address range descriptions without too much extra complexity; the tricky
> part is how best to associate those with the other information needed, which
> is really the "iommus" property of the relevant device, and how to make it
> as generically discoverable as possible. Perhaps it might be workable to
> follow almost the same approach but with a dedicated property (e.g.
> "active-memory-region") that the IOMMU code can simply scan the DT for to
> determine relevant device nodes. Otherwise properties on the IOMMU node
> itself would seem the next most practical option.

We did recently introduce a "memory-region-names" property that's used
to add context for cases where multiple memory regions are used. Perhaps
the simplest to address the above would be to describe the region as
active by naming it "active". That has the disadvantage of restricting
the number of active regions to 1, though I suspect that may even be
enough for the vast majority of cases where we need this. This would be
similar to how we use the "dma-mem" string in the "interconnect-names"
property to specify the "DMA parent" of a device node.

Alternatively, we could perhaps support multiple occurrences of "active"
in the "memory-region-names" property. Or we could add a bit of
flexibility by considering all memory regions whose names have an
"active-" prefix as being active.

> We've also finally got things going on the IORT RMR side[1], which helps add
> a bit more shape to things too; beyond the actual firmware parsing, DT and
> ACPI systems should definitely be converging on the same internal
> implementation in the IOMMU layer.

Yeah, from a quick look at that series, this actually sounds really
close to what I'm trying to achieve here.

The patch set that I have would nicely complement the code added to
iommu_dma_get_resv_regions() for RMR regions. It's not exactly the same
code, but it's basically the DT equivalent of
iort_dev_rmr_get_resv_regions().

Thierry

> 
> Robin.
> 
> [1] https://lore.kernel.org/linux-iommu/20201027112646.44680-1-shameerali.kolothum.thodi@huawei.com/

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-11-06 15:25           ` Thierry Reding
@ 2020-11-10 19:33             ` Thierry Reding
  2020-12-17 15:00               ` Thierry Reding
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-11-10 19:33 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu, Will Deacon


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

On Fri, Nov 06, 2020 at 04:25:48PM +0100, Thierry Reding wrote:
> On Thu, Nov 05, 2020 at 05:47:21PM +0000, Robin Murphy wrote:
> > On 2020-11-05 16:43, Thierry Reding wrote:
> > > On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
> > > > On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> > > > > On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > > > > > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > > 
> > > > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > > > expected to access the regions during boot and before the operating
> > > > > > > system can take control. One example where this is useful is for the
> > > > > > > operating system to infer whether the region needs to be identity-
> > > > > > > mapped through an IOMMU.
> > > > > > 
> > > > > > I like simple solutions, but this hardly seems adequate to solve the
> > > > > > problem of passing IOMMU setup from bootloader/firmware to the OS. Like
> > > > > > what is the IOVA that's supposed to be used if identity mapping is not
> > > > > > used?
> > > > > 
> > > > > The assumption here is that if the region is not active there is no need
> > > > > for the IOVA to be specified because the kernel will allocate memory and
> > > > > assign any IOVA of its choosing.
> > > > > 
> > > > > Also, note that this is not meant as a way of passing IOMMU setup from
> > > > > the bootloader or firmware to the OS. The purpose of this is to specify
> > > > > that some region of memory is actively being accessed during boot. The
> > > > > particular case that I'm looking at is where the bootloader set up a
> > > > > splash screen and keeps it on during boot. The bootloader has not set up
> > > > > an IOMMU mapping and the identity mapping serves as a way of keeping the
> > > > > accesses by the display hardware working during the transitional period
> > > > > after the IOMMU translations have been enabled by the kernel but before
> > > > > the kernel display driver has had a chance to set up its own IOMMU
> > > > > mappings.
> > > > > 
> > > > > > If you know enough about the regions to assume identity mapping, then
> > > > > > can't you know if active or not?
> > > > > 
> > > > > We could alternatively add some property that describes the region as
> > > > > requiring an identity mapping. But note that we can't make any
> > > > > assumptions here about the usage of these regions because the IOMMU
> > > > > driver simply has no way of knowing what they are being used for.
> > > > > 
> > > > > Some additional information is required in device tree for the IOMMU
> > > > > driver to be able to make that decision.
> > > > 
> > > > Rob, can you provide any hints on exactly how you want to move this
> > > > forward? I don't know in what direction you'd like to proceed.
> > > 
> > > Hi Rob,
> > > 
> > > do you have any suggestions on how to proceed with this? I'd like to get
> > > this moving again because it's something that's been nagging me for some
> > > months now. It also requires changes across two levels in the bootloader
> > > stack as well as Linux and it takes quite a bit of work to make all the
> > > changes, so before I go and rewrite everything I'd like to get the DT
> > > bindings sorted out first.
> > > 
> > > So just to summarize why I think this simple solution is good enough: it
> > > tries to solve a very narrow and simple problem. This is not an attempt
> > > at describing the firmware's full IOMMU setup to the kernel. In fact, it
> > > is primarily targetted at cases where the firmware hasn't setup an IOMMU
> > > at all, and we just want to make sure that when the kernel takes over
> > > and does want to enable the IOMMU, that all the regions that are
> > > actively being accessed by non-quiesced hardware (the most typical
> > > example would be a framebuffer scanning out a splat screen or animation,
> > > but it could equally well be some sort of welcoming tone or music being
> > > played back) are described in device tree.
> > > 
> > > In other words, and this is perhaps better answering your second
> > > question: in addition to describing reserved memory regions, we want to
> > > add a bit of information here about the usage of these memory regions.
> > > Some memory regions may contain information that the kernel may want to
> > > use (such an external memory frequency scaling tables) and those I would
> > > describe as "inactive" memory because it isn't being accessed by
> > > hardware. The framebuffer in this case is the opposite and it is being
> > > actively accessed (hence it is marked "active") by hardware while the
> > > kernel is busy setting everything up so that it can reconfigure that
> > > hardware and take over with its own framebuffer (for the console, for
> > > example). It's also not so much that we know enough about the region to
> > > assume it needs identity mapping. We don't really care about that from
> > > the DT point of view. In fact, depending on the rest of the system
> > > configuration, we may not need identity mapping (i.e. if none of the
> > > users of the reserved memory region are behind an IOMMU). But the point
> > > here is that the IOMMU drivers can use this "active" property to
> > > determine that if a device is using an "active" region and it is behind
> > > an IOMMU, then it must identity map that region in order for the
> > > hardware, which is not under the kernel's control yet, to be able to
> > > continue to access that memory through an IOMMU mapping.
> > 
> > Hmm, "active" is not a property of the memory itself, though, it's really a
> > property of the device accessing it. If several distinct devices share a
> > carveout region, and for simplicity the bootloader marks it as active
> > because one of those devices happens to be using some part of it at boot, we
> > don't really want to have to do all the reserved region setup for all the
> > other devices unnecessarily, when all that matters is not disrupting one of
> > them when resetting the IOMMU.
> > 
> > That leads to another possible hiccup - some bindings already have a defined
> > meaning for a "memory-region" property. If we use that to point to some
> > small region for a temporary low-resolution bootsplash screen for visibility
> > to an IOMMU driver, the device's own driver might also interpret it as a
> > private carveout from which it is expected to allocate everything, and thus
> > could end up failing to work well or at all.
> > 
> > I agree that we should only need a relatively simple binding, and that
> > piggybacking off reserved-memory nodes seems like an ideal way of getting
> > address range descriptions without too much extra complexity; the tricky
> > part is how best to associate those with the other information needed, which
> > is really the "iommus" property of the relevant device, and how to make it
> > as generically discoverable as possible. Perhaps it might be workable to
> > follow almost the same approach but with a dedicated property (e.g.
> > "active-memory-region") that the IOMMU code can simply scan the DT for to
> > determine relevant device nodes. Otherwise properties on the IOMMU node
> > itself would seem the next most practical option.
> 
> We did recently introduce a "memory-region-names" property that's used
> to add context for cases where multiple memory regions are used. Perhaps
> the simplest to address the above would be to describe the region as
> active by naming it "active". That has the disadvantage of restricting
> the number of active regions to 1, though I suspect that may even be
> enough for the vast majority of cases where we need this. This would be
> similar to how we use the "dma-mem" string in the "interconnect-names"
> property to specify the "DMA parent" of a device node.
> 
> Alternatively, we could perhaps support multiple occurrences of "active"
> in the "memory-region-names" property. Or we could add a bit of
> flexibility by considering all memory regions whose names have an
> "active-" prefix as being active.
> 
> > We've also finally got things going on the IORT RMR side[1], which helps add
> > a bit more shape to things too; beyond the actual firmware parsing, DT and
> > ACPI systems should definitely be converging on the same internal
> > implementation in the IOMMU layer.
> 
> Yeah, from a quick look at that series, this actually sounds really
> close to what I'm trying to achieve here.
> 
> The patch set that I have would nicely complement the code added to
> iommu_dma_get_resv_regions() for RMR regions. It's not exactly the same
> code, but it's basically the DT equivalent of
> iort_dev_rmr_get_resv_regions().

Hi Rob,

what's your preference here for DT bindings? Do you want me to reuse the
existing memory-region/memory-region-names properties, or do you want
something completely separate?

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-11-10 19:33             ` Thierry Reding
@ 2020-12-17 15:00               ` Thierry Reding
  2020-12-18 22:15                 ` Rob Herring
  0 siblings, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2020-12-17 15:00 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu, Will Deacon


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

On Tue, Nov 10, 2020 at 08:33:09PM +0100, Thierry Reding wrote:
> On Fri, Nov 06, 2020 at 04:25:48PM +0100, Thierry Reding wrote:
> > On Thu, Nov 05, 2020 at 05:47:21PM +0000, Robin Murphy wrote:
> > > On 2020-11-05 16:43, Thierry Reding wrote:
> > > > On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
> > > > > On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> > > > > > On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > > > > > > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > > > 
> > > > > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > > > > expected to access the regions during boot and before the operating
> > > > > > > > system can take control. One example where this is useful is for the
> > > > > > > > operating system to infer whether the region needs to be identity-
> > > > > > > > mapped through an IOMMU.
> > > > > > > 
> > > > > > > I like simple solutions, but this hardly seems adequate to solve the
> > > > > > > problem of passing IOMMU setup from bootloader/firmware to the OS. Like
> > > > > > > what is the IOVA that's supposed to be used if identity mapping is not
> > > > > > > used?
> > > > > > 
> > > > > > The assumption here is that if the region is not active there is no need
> > > > > > for the IOVA to be specified because the kernel will allocate memory and
> > > > > > assign any IOVA of its choosing.
> > > > > > 
> > > > > > Also, note that this is not meant as a way of passing IOMMU setup from
> > > > > > the bootloader or firmware to the OS. The purpose of this is to specify
> > > > > > that some region of memory is actively being accessed during boot. The
> > > > > > particular case that I'm looking at is where the bootloader set up a
> > > > > > splash screen and keeps it on during boot. The bootloader has not set up
> > > > > > an IOMMU mapping and the identity mapping serves as a way of keeping the
> > > > > > accesses by the display hardware working during the transitional period
> > > > > > after the IOMMU translations have been enabled by the kernel but before
> > > > > > the kernel display driver has had a chance to set up its own IOMMU
> > > > > > mappings.
> > > > > > 
> > > > > > > If you know enough about the regions to assume identity mapping, then
> > > > > > > can't you know if active or not?
> > > > > > 
> > > > > > We could alternatively add some property that describes the region as
> > > > > > requiring an identity mapping. But note that we can't make any
> > > > > > assumptions here about the usage of these regions because the IOMMU
> > > > > > driver simply has no way of knowing what they are being used for.
> > > > > > 
> > > > > > Some additional information is required in device tree for the IOMMU
> > > > > > driver to be able to make that decision.
> > > > > 
> > > > > Rob, can you provide any hints on exactly how you want to move this
> > > > > forward? I don't know in what direction you'd like to proceed.
> > > > 
> > > > Hi Rob,
> > > > 
> > > > do you have any suggestions on how to proceed with this? I'd like to get
> > > > this moving again because it's something that's been nagging me for some
> > > > months now. It also requires changes across two levels in the bootloader
> > > > stack as well as Linux and it takes quite a bit of work to make all the
> > > > changes, so before I go and rewrite everything I'd like to get the DT
> > > > bindings sorted out first.
> > > > 
> > > > So just to summarize why I think this simple solution is good enough: it
> > > > tries to solve a very narrow and simple problem. This is not an attempt
> > > > at describing the firmware's full IOMMU setup to the kernel. In fact, it
> > > > is primarily targetted at cases where the firmware hasn't setup an IOMMU
> > > > at all, and we just want to make sure that when the kernel takes over
> > > > and does want to enable the IOMMU, that all the regions that are
> > > > actively being accessed by non-quiesced hardware (the most typical
> > > > example would be a framebuffer scanning out a splat screen or animation,
> > > > but it could equally well be some sort of welcoming tone or music being
> > > > played back) are described in device tree.
> > > > 
> > > > In other words, and this is perhaps better answering your second
> > > > question: in addition to describing reserved memory regions, we want to
> > > > add a bit of information here about the usage of these memory regions.
> > > > Some memory regions may contain information that the kernel may want to
> > > > use (such an external memory frequency scaling tables) and those I would
> > > > describe as "inactive" memory because it isn't being accessed by
> > > > hardware. The framebuffer in this case is the opposite and it is being
> > > > actively accessed (hence it is marked "active") by hardware while the
> > > > kernel is busy setting everything up so that it can reconfigure that
> > > > hardware and take over with its own framebuffer (for the console, for
> > > > example). It's also not so much that we know enough about the region to
> > > > assume it needs identity mapping. We don't really care about that from
> > > > the DT point of view. In fact, depending on the rest of the system
> > > > configuration, we may not need identity mapping (i.e. if none of the
> > > > users of the reserved memory region are behind an IOMMU). But the point
> > > > here is that the IOMMU drivers can use this "active" property to
> > > > determine that if a device is using an "active" region and it is behind
> > > > an IOMMU, then it must identity map that region in order for the
> > > > hardware, which is not under the kernel's control yet, to be able to
> > > > continue to access that memory through an IOMMU mapping.
> > > 
> > > Hmm, "active" is not a property of the memory itself, though, it's really a
> > > property of the device accessing it. If several distinct devices share a
> > > carveout region, and for simplicity the bootloader marks it as active
> > > because one of those devices happens to be using some part of it at boot, we
> > > don't really want to have to do all the reserved region setup for all the
> > > other devices unnecessarily, when all that matters is not disrupting one of
> > > them when resetting the IOMMU.
> > > 
> > > That leads to another possible hiccup - some bindings already have a defined
> > > meaning for a "memory-region" property. If we use that to point to some
> > > small region for a temporary low-resolution bootsplash screen for visibility
> > > to an IOMMU driver, the device's own driver might also interpret it as a
> > > private carveout from which it is expected to allocate everything, and thus
> > > could end up failing to work well or at all.
> > > 
> > > I agree that we should only need a relatively simple binding, and that
> > > piggybacking off reserved-memory nodes seems like an ideal way of getting
> > > address range descriptions without too much extra complexity; the tricky
> > > part is how best to associate those with the other information needed, which
> > > is really the "iommus" property of the relevant device, and how to make it
> > > as generically discoverable as possible. Perhaps it might be workable to
> > > follow almost the same approach but with a dedicated property (e.g.
> > > "active-memory-region") that the IOMMU code can simply scan the DT for to
> > > determine relevant device nodes. Otherwise properties on the IOMMU node
> > > itself would seem the next most practical option.
> > 
> > We did recently introduce a "memory-region-names" property that's used
> > to add context for cases where multiple memory regions are used. Perhaps
> > the simplest to address the above would be to describe the region as
> > active by naming it "active". That has the disadvantage of restricting
> > the number of active regions to 1, though I suspect that may even be
> > enough for the vast majority of cases where we need this. This would be
> > similar to how we use the "dma-mem" string in the "interconnect-names"
> > property to specify the "DMA parent" of a device node.
> > 
> > Alternatively, we could perhaps support multiple occurrences of "active"
> > in the "memory-region-names" property. Or we could add a bit of
> > flexibility by considering all memory regions whose names have an
> > "active-" prefix as being active.
> > 
> > > We've also finally got things going on the IORT RMR side[1], which helps add
> > > a bit more shape to things too; beyond the actual firmware parsing, DT and
> > > ACPI systems should definitely be converging on the same internal
> > > implementation in the IOMMU layer.
> > 
> > Yeah, from a quick look at that series, this actually sounds really
> > close to what I'm trying to achieve here.
> > 
> > The patch set that I have would nicely complement the code added to
> > iommu_dma_get_resv_regions() for RMR regions. It's not exactly the same
> > code, but it's basically the DT equivalent of
> > iort_dev_rmr_get_resv_regions().
> 
> Hi Rob,
> 
> what's your preference here for DT bindings? Do you want me to reuse the
> existing memory-region/memory-region-names properties, or do you want
> something completely separate?

Hi Rob,

I've been thinking about this some more and I think I've come up with an
alternative that I think you might like better than what we discussed so
far.

Rather than reusing memory-region-names and guessing from the name what
the intended purpose was, how about we add the concept of memory region
specifiers to describe additional properties of reserved memory regions
uses? This would allow us to address Robin's concerns about describing
what's essentially a device property within the reserved memory region.

The way I imagine that this would work is that the reserved memory
regions would gain a new property, "#memory-region-cells", that defines
the number of cells that make up a reserved memory region specifier,
much like we have #clock-cells, #reset-cells, #pwm-cells, etc. Since
these specifier are defined where the regions are used, they would allow
us to encode information about that specific use, rather than properties
of the regions themselves.

This should also allow for backwards-compatibility where a missing
#memory-region-cells would be interpreted as 0 specifier (i.e. no
additional data).

Here's how this would look for the specific example that I want to
solve:

	#define MEMORY_REGION_ACTIVE 0x1

	/ {
		reserved-memory {
			lut: lookup-table@96060000 {
				reg = <0x96060000 0x00010000>;
				#memory-region-cells = <1>;
			};

			fbc: framebuffer@96070000 {
				reg = <0x96070000 0x800000>;
				#memory-region-cells = <1>;
			};
		};

		...

		host1x@50000000 {
			...

			display@54200000 {
				...
				memory-regions = <&fbc MEMORY_REGION_ACTIVE>,
						 <&lut MEMORY_REGION_ACTIVE>;
				...
			};

			...
		};
	};

As you can see, the reserved memory region nodes only contain properties
that are immediately related to the regions themselves, whereas the
"active" attribute now applies only for the specific use of the region
within display@54200000.

What do you think?

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-12-17 15:00               ` Thierry Reding
@ 2020-12-18 22:15                 ` Rob Herring
  2020-12-19  0:49                   ` Thierry Reding
  0 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2020-12-18 22:15 UTC (permalink / raw)
  To: Thierry Reding
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel,
	Linux IOMMU, Will Deacon

On Thu, Dec 17, 2020 at 9:00 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Nov 10, 2020 at 08:33:09PM +0100, Thierry Reding wrote:
> > On Fri, Nov 06, 2020 at 04:25:48PM +0100, Thierry Reding wrote:
> > > On Thu, Nov 05, 2020 at 05:47:21PM +0000, Robin Murphy wrote:
> > > > On 2020-11-05 16:43, Thierry Reding wrote:
> > > > > On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
> > > > > > On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> > > > > > > On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > > > > > > > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > > > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > > > >
> > > > > > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > > > > > expected to access the regions during boot and before the operating
> > > > > > > > > system can take control. One example where this is useful is for the
> > > > > > > > > operating system to infer whether the region needs to be identity-
> > > > > > > > > mapped through an IOMMU.
> > > > > > > >
> > > > > > > > I like simple solutions, but this hardly seems adequate to solve the
> > > > > > > > problem of passing IOMMU setup from bootloader/firmware to the OS. Like
> > > > > > > > what is the IOVA that's supposed to be used if identity mapping is not
> > > > > > > > used?
> > > > > > >
> > > > > > > The assumption here is that if the region is not active there is no need
> > > > > > > for the IOVA to be specified because the kernel will allocate memory and
> > > > > > > assign any IOVA of its choosing.
> > > > > > >
> > > > > > > Also, note that this is not meant as a way of passing IOMMU setup from
> > > > > > > the bootloader or firmware to the OS. The purpose of this is to specify
> > > > > > > that some region of memory is actively being accessed during boot. The
> > > > > > > particular case that I'm looking at is where the bootloader set up a
> > > > > > > splash screen and keeps it on during boot. The bootloader has not set up
> > > > > > > an IOMMU mapping and the identity mapping serves as a way of keeping the
> > > > > > > accesses by the display hardware working during the transitional period
> > > > > > > after the IOMMU translations have been enabled by the kernel but before
> > > > > > > the kernel display driver has had a chance to set up its own IOMMU
> > > > > > > mappings.
> > > > > > >
> > > > > > > > If you know enough about the regions to assume identity mapping, then
> > > > > > > > can't you know if active or not?
> > > > > > >
> > > > > > > We could alternatively add some property that describes the region as
> > > > > > > requiring an identity mapping. But note that we can't make any
> > > > > > > assumptions here about the usage of these regions because the IOMMU
> > > > > > > driver simply has no way of knowing what they are being used for.
> > > > > > >
> > > > > > > Some additional information is required in device tree for the IOMMU
> > > > > > > driver to be able to make that decision.
> > > > > >
> > > > > > Rob, can you provide any hints on exactly how you want to move this
> > > > > > forward? I don't know in what direction you'd like to proceed.
> > > > >
> > > > > Hi Rob,
> > > > >
> > > > > do you have any suggestions on how to proceed with this? I'd like to get
> > > > > this moving again because it's something that's been nagging me for some
> > > > > months now. It also requires changes across two levels in the bootloader
> > > > > stack as well as Linux and it takes quite a bit of work to make all the
> > > > > changes, so before I go and rewrite everything I'd like to get the DT
> > > > > bindings sorted out first.
> > > > >
> > > > > So just to summarize why I think this simple solution is good enough: it
> > > > > tries to solve a very narrow and simple problem. This is not an attempt
> > > > > at describing the firmware's full IOMMU setup to the kernel. In fact, it
> > > > > is primarily targetted at cases where the firmware hasn't setup an IOMMU
> > > > > at all, and we just want to make sure that when the kernel takes over
> > > > > and does want to enable the IOMMU, that all the regions that are
> > > > > actively being accessed by non-quiesced hardware (the most typical
> > > > > example would be a framebuffer scanning out a splat screen or animation,
> > > > > but it could equally well be some sort of welcoming tone or music being
> > > > > played back) are described in device tree.
> > > > >
> > > > > In other words, and this is perhaps better answering your second
> > > > > question: in addition to describing reserved memory regions, we want to
> > > > > add a bit of information here about the usage of these memory regions.
> > > > > Some memory regions may contain information that the kernel may want to
> > > > > use (such an external memory frequency scaling tables) and those I would
> > > > > describe as "inactive" memory because it isn't being accessed by
> > > > > hardware. The framebuffer in this case is the opposite and it is being
> > > > > actively accessed (hence it is marked "active") by hardware while the
> > > > > kernel is busy setting everything up so that it can reconfigure that
> > > > > hardware and take over with its own framebuffer (for the console, for
> > > > > example). It's also not so much that we know enough about the region to
> > > > > assume it needs identity mapping. We don't really care about that from
> > > > > the DT point of view. In fact, depending on the rest of the system
> > > > > configuration, we may not need identity mapping (i.e. if none of the
> > > > > users of the reserved memory region are behind an IOMMU). But the point
> > > > > here is that the IOMMU drivers can use this "active" property to
> > > > > determine that if a device is using an "active" region and it is behind
> > > > > an IOMMU, then it must identity map that region in order for the
> > > > > hardware, which is not under the kernel's control yet, to be able to
> > > > > continue to access that memory through an IOMMU mapping.
> > > >
> > > > Hmm, "active" is not a property of the memory itself, though, it's really a
> > > > property of the device accessing it. If several distinct devices share a
> > > > carveout region, and for simplicity the bootloader marks it as active
> > > > because one of those devices happens to be using some part of it at boot, we
> > > > don't really want to have to do all the reserved region setup for all the
> > > > other devices unnecessarily, when all that matters is not disrupting one of
> > > > them when resetting the IOMMU.
> > > >
> > > > That leads to another possible hiccup - some bindings already have a defined
> > > > meaning for a "memory-region" property. If we use that to point to some
> > > > small region for a temporary low-resolution bootsplash screen for visibility
> > > > to an IOMMU driver, the device's own driver might also interpret it as a
> > > > private carveout from which it is expected to allocate everything, and thus
> > > > could end up failing to work well or at all.
> > > >
> > > > I agree that we should only need a relatively simple binding, and that
> > > > piggybacking off reserved-memory nodes seems like an ideal way of getting
> > > > address range descriptions without too much extra complexity; the tricky
> > > > part is how best to associate those with the other information needed, which
> > > > is really the "iommus" property of the relevant device, and how to make it
> > > > as generically discoverable as possible. Perhaps it might be workable to
> > > > follow almost the same approach but with a dedicated property (e.g.
> > > > "active-memory-region") that the IOMMU code can simply scan the DT for to
> > > > determine relevant device nodes. Otherwise properties on the IOMMU node
> > > > itself would seem the next most practical option.
> > >
> > > We did recently introduce a "memory-region-names" property that's used
> > > to add context for cases where multiple memory regions are used. Perhaps
> > > the simplest to address the above would be to describe the region as
> > > active by naming it "active". That has the disadvantage of restricting
> > > the number of active regions to 1, though I suspect that may even be
> > > enough for the vast majority of cases where we need this. This would be
> > > similar to how we use the "dma-mem" string in the "interconnect-names"
> > > property to specify the "DMA parent" of a device node.
> > >
> > > Alternatively, we could perhaps support multiple occurrences of "active"
> > > in the "memory-region-names" property. Or we could add a bit of
> > > flexibility by considering all memory regions whose names have an
> > > "active-" prefix as being active.
> > >
> > > > We've also finally got things going on the IORT RMR side[1], which helps add
> > > > a bit more shape to things too; beyond the actual firmware parsing, DT and
> > > > ACPI systems should definitely be converging on the same internal
> > > > implementation in the IOMMU layer.
> > >
> > > Yeah, from a quick look at that series, this actually sounds really
> > > close to what I'm trying to achieve here.
> > >
> > > The patch set that I have would nicely complement the code added to
> > > iommu_dma_get_resv_regions() for RMR regions. It's not exactly the same
> > > code, but it's basically the DT equivalent of
> > > iort_dev_rmr_get_resv_regions().
> >
> > Hi Rob,
> >
> > what's your preference here for DT bindings? Do you want me to reuse the
> > existing memory-region/memory-region-names properties, or do you want
> > something completely separate?

I think that's overloading memory-region-names as *-names is a name
local to a binding to augment an index.

>
> Hi Rob,
>
> I've been thinking about this some more and I think I've come up with an
> alternative that I think you might like better than what we discussed so
> far.
>
> Rather than reusing memory-region-names and guessing from the name what
> the intended purpose was, how about we add the concept of memory region
> specifiers to describe additional properties of reserved memory regions
> uses? This would allow us to address Robin's concerns about describing
> what's essentially a device property within the reserved memory region.
>
> The way I imagine that this would work is that the reserved memory
> regions would gain a new property, "#memory-region-cells", that defines
> the number of cells that make up a reserved memory region specifier,
> much like we have #clock-cells, #reset-cells, #pwm-cells, etc. Since
> these specifier are defined where the regions are used, they would allow
> us to encode information about that specific use, rather than properties
> of the regions themselves.
>
> This should also allow for backwards-compatibility where a missing
> #memory-region-cells would be interpreted as 0 specifier (i.e. no
> additional data).
>
> Here's how this would look for the specific example that I want to
> solve:
>
>         #define MEMORY_REGION_ACTIVE 0x1
>
>         / {
>                 reserved-memory {
>                         lut: lookup-table@96060000 {
>                                 reg = <0x96060000 0x00010000>;
>                                 #memory-region-cells = <1>;
>                         };
>
>                         fbc: framebuffer@96070000 {
>                                 reg = <0x96070000 0x800000>;
>                                 #memory-region-cells = <1>;
>                         };
>                 };
>
>                 ...
>
>                 host1x@50000000 {
>                         ...
>
>                         display@54200000 {
>                                 ...
>                                 memory-regions = <&fbc MEMORY_REGION_ACTIVE>,
>                                                  <&lut MEMORY_REGION_ACTIVE>;
>                                 ...
>                         };
>
>                         ...
>                 };
>         };
>
> As you can see, the reserved memory region nodes only contain properties
> that are immediately related to the regions themselves, whereas the
> "active" attribute now applies only for the specific use of the region
> within display@54200000.
>
> What do you think?

When would these regions ever not be active? Isn't just the fact that
you have the 'memory-regions' property enough to know that they are
active (possibly combined with seeing the display h/w is already
enabled)? I guess if the idea is to parse 'memory-regions' for the
whole DT and find the active ones, you'd need the flag (and presumably
an 'iommus' property too).

Do you have a usecase for this outside of the display enabled by
bootloader? Because we already have the simple-framebuffer binding of
which the memory region is just part of it. Wouldn't the presence of a
memory region there imply it's active as well?

Or maybe the iommu node(s) should just have a 'memory-regions' property?

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-12-18 22:15                 ` Rob Herring
@ 2020-12-19  0:49                   ` Thierry Reding
  0 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2020-12-19  0:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel,
	Linux IOMMU, Will Deacon


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

On Fri, Dec 18, 2020 at 04:15:45PM -0600, Rob Herring wrote:
> On Thu, Dec 17, 2020 at 9:00 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Tue, Nov 10, 2020 at 08:33:09PM +0100, Thierry Reding wrote:
> > > On Fri, Nov 06, 2020 at 04:25:48PM +0100, Thierry Reding wrote:
> > > > On Thu, Nov 05, 2020 at 05:47:21PM +0000, Robin Murphy wrote:
> > > > > On 2020-11-05 16:43, Thierry Reding wrote:
> > > > > > On Thu, Sep 24, 2020 at 01:27:25PM +0200, Thierry Reding wrote:
> > > > > > > On Tue, Sep 15, 2020 at 02:36:48PM +0200, Thierry Reding wrote:
> > > > > > > > On Mon, Sep 14, 2020 at 04:08:29PM -0600, Rob Herring wrote:
> > > > > > > > > On Fri, Sep 04, 2020 at 02:59:57PM +0200, Thierry Reding wrote:
> > > > > > > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > > > > > >
> > > > > > > > > > Reserved memory regions can be marked as "active" if hardware is
> > > > > > > > > > expected to access the regions during boot and before the operating
> > > > > > > > > > system can take control. One example where this is useful is for the
> > > > > > > > > > operating system to infer whether the region needs to be identity-
> > > > > > > > > > mapped through an IOMMU.
> > > > > > > > >
> > > > > > > > > I like simple solutions, but this hardly seems adequate to solve the
> > > > > > > > > problem of passing IOMMU setup from bootloader/firmware to the OS. Like
> > > > > > > > > what is the IOVA that's supposed to be used if identity mapping is not
> > > > > > > > > used?
> > > > > > > >
> > > > > > > > The assumption here is that if the region is not active there is no need
> > > > > > > > for the IOVA to be specified because the kernel will allocate memory and
> > > > > > > > assign any IOVA of its choosing.
> > > > > > > >
> > > > > > > > Also, note that this is not meant as a way of passing IOMMU setup from
> > > > > > > > the bootloader or firmware to the OS. The purpose of this is to specify
> > > > > > > > that some region of memory is actively being accessed during boot. The
> > > > > > > > particular case that I'm looking at is where the bootloader set up a
> > > > > > > > splash screen and keeps it on during boot. The bootloader has not set up
> > > > > > > > an IOMMU mapping and the identity mapping serves as a way of keeping the
> > > > > > > > accesses by the display hardware working during the transitional period
> > > > > > > > after the IOMMU translations have been enabled by the kernel but before
> > > > > > > > the kernel display driver has had a chance to set up its own IOMMU
> > > > > > > > mappings.
> > > > > > > >
> > > > > > > > > If you know enough about the regions to assume identity mapping, then
> > > > > > > > > can't you know if active or not?
> > > > > > > >
> > > > > > > > We could alternatively add some property that describes the region as
> > > > > > > > requiring an identity mapping. But note that we can't make any
> > > > > > > > assumptions here about the usage of these regions because the IOMMU
> > > > > > > > driver simply has no way of knowing what they are being used for.
> > > > > > > >
> > > > > > > > Some additional information is required in device tree for the IOMMU
> > > > > > > > driver to be able to make that decision.
> > > > > > >
> > > > > > > Rob, can you provide any hints on exactly how you want to move this
> > > > > > > forward? I don't know in what direction you'd like to proceed.
> > > > > >
> > > > > > Hi Rob,
> > > > > >
> > > > > > do you have any suggestions on how to proceed with this? I'd like to get
> > > > > > this moving again because it's something that's been nagging me for some
> > > > > > months now. It also requires changes across two levels in the bootloader
> > > > > > stack as well as Linux and it takes quite a bit of work to make all the
> > > > > > changes, so before I go and rewrite everything I'd like to get the DT
> > > > > > bindings sorted out first.
> > > > > >
> > > > > > So just to summarize why I think this simple solution is good enough: it
> > > > > > tries to solve a very narrow and simple problem. This is not an attempt
> > > > > > at describing the firmware's full IOMMU setup to the kernel. In fact, it
> > > > > > is primarily targetted at cases where the firmware hasn't setup an IOMMU
> > > > > > at all, and we just want to make sure that when the kernel takes over
> > > > > > and does want to enable the IOMMU, that all the regions that are
> > > > > > actively being accessed by non-quiesced hardware (the most typical
> > > > > > example would be a framebuffer scanning out a splat screen or animation,
> > > > > > but it could equally well be some sort of welcoming tone or music being
> > > > > > played back) are described in device tree.
> > > > > >
> > > > > > In other words, and this is perhaps better answering your second
> > > > > > question: in addition to describing reserved memory regions, we want to
> > > > > > add a bit of information here about the usage of these memory regions.
> > > > > > Some memory regions may contain information that the kernel may want to
> > > > > > use (such an external memory frequency scaling tables) and those I would
> > > > > > describe as "inactive" memory because it isn't being accessed by
> > > > > > hardware. The framebuffer in this case is the opposite and it is being
> > > > > > actively accessed (hence it is marked "active") by hardware while the
> > > > > > kernel is busy setting everything up so that it can reconfigure that
> > > > > > hardware and take over with its own framebuffer (for the console, for
> > > > > > example). It's also not so much that we know enough about the region to
> > > > > > assume it needs identity mapping. We don't really care about that from
> > > > > > the DT point of view. In fact, depending on the rest of the system
> > > > > > configuration, we may not need identity mapping (i.e. if none of the
> > > > > > users of the reserved memory region are behind an IOMMU). But the point
> > > > > > here is that the IOMMU drivers can use this "active" property to
> > > > > > determine that if a device is using an "active" region and it is behind
> > > > > > an IOMMU, then it must identity map that region in order for the
> > > > > > hardware, which is not under the kernel's control yet, to be able to
> > > > > > continue to access that memory through an IOMMU mapping.
> > > > >
> > > > > Hmm, "active" is not a property of the memory itself, though, it's really a
> > > > > property of the device accessing it. If several distinct devices share a
> > > > > carveout region, and for simplicity the bootloader marks it as active
> > > > > because one of those devices happens to be using some part of it at boot, we
> > > > > don't really want to have to do all the reserved region setup for all the
> > > > > other devices unnecessarily, when all that matters is not disrupting one of
> > > > > them when resetting the IOMMU.
> > > > >
> > > > > That leads to another possible hiccup - some bindings already have a defined
> > > > > meaning for a "memory-region" property. If we use that to point to some
> > > > > small region for a temporary low-resolution bootsplash screen for visibility
> > > > > to an IOMMU driver, the device's own driver might also interpret it as a
> > > > > private carveout from which it is expected to allocate everything, and thus
> > > > > could end up failing to work well or at all.
> > > > >
> > > > > I agree that we should only need a relatively simple binding, and that
> > > > > piggybacking off reserved-memory nodes seems like an ideal way of getting
> > > > > address range descriptions without too much extra complexity; the tricky
> > > > > part is how best to associate those with the other information needed, which
> > > > > is really the "iommus" property of the relevant device, and how to make it
> > > > > as generically discoverable as possible. Perhaps it might be workable to
> > > > > follow almost the same approach but with a dedicated property (e.g.
> > > > > "active-memory-region") that the IOMMU code can simply scan the DT for to
> > > > > determine relevant device nodes. Otherwise properties on the IOMMU node
> > > > > itself would seem the next most practical option.
> > > >
> > > > We did recently introduce a "memory-region-names" property that's used
> > > > to add context for cases where multiple memory regions are used. Perhaps
> > > > the simplest to address the above would be to describe the region as
> > > > active by naming it "active". That has the disadvantage of restricting
> > > > the number of active regions to 1, though I suspect that may even be
> > > > enough for the vast majority of cases where we need this. This would be
> > > > similar to how we use the "dma-mem" string in the "interconnect-names"
> > > > property to specify the "DMA parent" of a device node.
> > > >
> > > > Alternatively, we could perhaps support multiple occurrences of "active"
> > > > in the "memory-region-names" property. Or we could add a bit of
> > > > flexibility by considering all memory regions whose names have an
> > > > "active-" prefix as being active.
> > > >
> > > > > We've also finally got things going on the IORT RMR side[1], which helps add
> > > > > a bit more shape to things too; beyond the actual firmware parsing, DT and
> > > > > ACPI systems should definitely be converging on the same internal
> > > > > implementation in the IOMMU layer.
> > > >
> > > > Yeah, from a quick look at that series, this actually sounds really
> > > > close to what I'm trying to achieve here.
> > > >
> > > > The patch set that I have would nicely complement the code added to
> > > > iommu_dma_get_resv_regions() for RMR regions. It's not exactly the same
> > > > code, but it's basically the DT equivalent of
> > > > iort_dev_rmr_get_resv_regions().
> > >
> > > Hi Rob,
> > >
> > > what's your preference here for DT bindings? Do you want me to reuse the
> > > existing memory-region/memory-region-names properties, or do you want
> > > something completely separate?
> 
> I think that's overloading memory-region-names as *-names is a name
> local to a binding to augment an index.
> 
> >
> > Hi Rob,
> >
> > I've been thinking about this some more and I think I've come up with an
> > alternative that I think you might like better than what we discussed so
> > far.
> >
> > Rather than reusing memory-region-names and guessing from the name what
> > the intended purpose was, how about we add the concept of memory region
> > specifiers to describe additional properties of reserved memory regions
> > uses? This would allow us to address Robin's concerns about describing
> > what's essentially a device property within the reserved memory region.
> >
> > The way I imagine that this would work is that the reserved memory
> > regions would gain a new property, "#memory-region-cells", that defines
> > the number of cells that make up a reserved memory region specifier,
> > much like we have #clock-cells, #reset-cells, #pwm-cells, etc. Since
> > these specifier are defined where the regions are used, they would allow
> > us to encode information about that specific use, rather than properties
> > of the regions themselves.
> >
> > This should also allow for backwards-compatibility where a missing
> > #memory-region-cells would be interpreted as 0 specifier (i.e. no
> > additional data).
> >
> > Here's how this would look for the specific example that I want to
> > solve:
> >
> >         #define MEMORY_REGION_ACTIVE 0x1
> >
> >         / {
> >                 reserved-memory {
> >                         lut: lookup-table@96060000 {
> >                                 reg = <0x96060000 0x00010000>;
> >                                 #memory-region-cells = <1>;
> >                         };
> >
> >                         fbc: framebuffer@96070000 {
> >                                 reg = <0x96070000 0x800000>;
> >                                 #memory-region-cells = <1>;
> >                         };
> >                 };
> >
> >                 ...
> >
> >                 host1x@50000000 {
> >                         ...
> >
> >                         display@54200000 {
> >                                 ...
> >                                 memory-regions = <&fbc MEMORY_REGION_ACTIVE>,
> >                                                  <&lut MEMORY_REGION_ACTIVE>;
> >                                 ...
> >                         };
> >
> >                         ...
> >                 };
> >         };
> >
> > As you can see, the reserved memory region nodes only contain properties
> > that are immediately related to the regions themselves, whereas the
> > "active" attribute now applies only for the specific use of the region
> > within display@54200000.
> >
> > What do you think?
> 
> When would these regions ever not be active? Isn't just the fact that
> you have the 'memory-regions' property enough to know that they are
> active (possibly combined with seeing the display h/w is already
> enabled)? I guess if the idea is to parse 'memory-regions' for the
> whole DT and find the active ones, you'd need the flag (and presumably
> an 'iommus' property too).

The memory-region property can also be used to provide reserved regions
of memory for a device to use. This could be a special carveout to make
sure the device doesn't run out of physical memory to allocate from as
regular CMA fragments, for example.

Or it could be some special block of memory that a device has to use.

The difference here is that the "active" memory region in this case is
actively being used while the device is booting, which isn't necessarily
the norm for reserved memory regions.

So this is kind of like "regulator-boot-on" but for display controllers
and memory regions.

The reason why we want this defined as part of the device tree is
because this information is needed at a point very early in the boot
process where no display driver has been loaded yet, so it's unknown
whether the display controller is enabled or not. Furthermore, the SMMU
driver needs this information, so it needs to be a somewhat standard DT
binding so that it can be parsed generically because there is no device
specific driver in play at the time.

> Do you have a usecase for this outside of the display enabled by
> bootloader? Because we already have the simple-framebuffer binding of
> which the memory region is just part of it. Wouldn't the presence of a
> memory region there imply it's active as well?

I don't know of a use-case other than display where this is needed right
now. However, I could imagine that something similar could be happening
for audio where a mobile device would be playing some welcome sound or
music in a loop during boot.

I don't think simple-framebuffer would work in this case either because
it usually doesn't contain the iommus property. But even if we made that
part of simple-framebuffer, it'd mean that the SMMU driver would have to
special-case depending on the simple-framebuffer compatible string, and
potentially any future similar bindings for other use-cases.

> Or maybe the iommu node(s) should just have a 'memory-regions' property?

I don't think that's enough information. We need to know for which
device this memory region is active so that it can be mapped for the
correct address space.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property
  2020-11-05 15:49       ` Thierry Reding
@ 2021-01-12 19:00         ` Dmitry Osipenko
  0 siblings, 0 replies; 24+ messages in thread
From: Dmitry Osipenko @ 2021-01-12 19:00 UTC (permalink / raw)
  To: Thierry Reding
  Cc: devicetree, Frank Rowand, Robin Murphy, linux-kernel, iommu,
	Rob Herring, linux-tegra, Will Deacon

05.11.2020 18:49, Thierry Reding пишет:
> On Thu, Sep 24, 2020 at 07:23:34PM +0300, Dmitry Osipenko wrote:
>> 24.09.2020 17:01, Thierry Reding пишет:
>>> On Thu, Sep 24, 2020 at 04:23:59PM +0300, Dmitry Osipenko wrote:
>>>> 04.09.2020 15:59, Thierry Reding пишет:
>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>
>>>>> Reserved memory regions can be marked as "active" if hardware is
>>>>> expected to access the regions during boot and before the operating
>>>>> system can take control. One example where this is useful is for the
>>>>> operating system to infer whether the region needs to be identity-
>>>>> mapped through an IOMMU.
>>>>>
>>>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>>>> ---
>>>>>  .../bindings/reserved-memory/reserved-memory.txt           | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> index 4dd20de6977f..163d2927e4fc 100644
>>>>> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
>>>>> @@ -63,6 +63,13 @@ reusable (optional) - empty property
>>>>>        able to reclaim it back. Typically that means that the operating
>>>>>        system can use that region to store volatile or cached data that
>>>>>        can be otherwise regenerated or migrated elsewhere.
>>>>> +active (optional) - empty property
>>>>> +    - If this property is set for a reserved memory region, it indicates
>>>>> +      that some piece of hardware may be actively accessing this region.
>>>>> +      Should the operating system want to enable IOMMU protection for a
>>>>> +      device, all active memory regions must have been identity-mapped
>>>>> +      in order to ensure that non-quiescent hardware during boot can
>>>>> +      continue to access the memory.
>>>>>  
>>>>>  Linux implementation note:
>>>>>  - If a "linux,cma-default" property is present, then Linux will use the
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Could you please explain what devices need this quirk? I see that you're
>>>> targeting Tegra SMMU driver, which means that it should be some pre-T186
>>>> device.
>>>
>>> Primarily I'm looking at Tegra210 and later, because on earlier devices
>>> the bootloader doesn't consistently initialize display. I know that it
>>> does on some devices, but not all of them.
>>
>> AFAIK, all tablet devices starting with Tegra20 that have display panel
>> are initializing display at a boot time for showing splash screen. This
>> includes all T20/T30/T114 tablets that are already supported by upstream
>> kernel.
>>
>>> This same code should also
>>> work on Tegra186 and later (with an ARM SMMU) although the situation is
>>> slightly more complicated there because IOMMU translations will fault by
>>> default long before these identity mappings can be established.
>>>
>>>> Is this reservation needed for some device that has display
>>>> hardwired to a very specific IOMMU domain at the boot time?
>>>
>>> No, this is only used to convey information about the active framebuffer
>>> to the kernel. In practice the DMA/IOMMU code will use this information
>>> to establish a 1:1 mapping on whatever IOMMU domain that was picked for
>>> display.
>>>
>>>> If you're targeting devices that don't have IOMMU enabled by default at
>>>> the boot time, then this approach won't work for the existing devices
>>>> which won't ever get an updated bootloader.
>>>
>>> If the devices don't use an IOMMU, then there should be no problem. The
>>> extra reserved-memory nodes would still be necessary to ensure that the
>>> kernel doesn't reuse the framebuffer memory for the slab allocator, but
>>> if no IOMMU is used, then the display controller accessing the memory
>>> isn't going to cause problems other than perhaps scanning out data that
>>> is no longer a framebuffer.
>>>
>>> There should also be no problem for devices with an old bootloader
>>> because this code is triggered by the presence of a reserved-memory node
>>> referenced via the memory-region property. Devices with an old
>>> bootloader should continue to work as they did before. Although I
>>> suppose they would start faulting once we enable DMA/IOMMU integration
>>> for Tegra SMMU if they have a bootloader that does initialize display to
>>> actively scan out during boot.
>>>
>>>> I think Robin Murphy already suggested that we should simply create
>>>> a dummy "identity" IOMMU domain by default for the DRM/VDE devices and
>>>> then replace it with an explicitly created domain within the drivers.
>>>
>>> I don't recall reading about that suggestion. So does this mean that for
>>> certain devices we'd want to basically passthrough by default and then
>>> at some point during boot take over with a properly managed IOMMU
>>> domain?
>>
>> Yes, my understanding that this is what Robin suggested here:
>>
>> https://lore.kernel.org/linux-iommu/cb12808b-7316-19db-7413-b7f852a6f8ae@arm.com/
>>
>>> The primary goal here is to move towards using the DMA API rather than
>>> the IOMMU API directly, so we don't really have the option of replacing
>>> with an explicitly created domain. Unless we have code in the DMA/IOMMU
>>> code that does this somehow.
>>>
>>> But I'm not sure what would be a good way to mark certain devices as
>>> needing an identity domain by default. Do we still use the reserved-
>>> memory node for that?
>>
>> The reserved-memory indeed shouldn't be needed for resolving the
>> implicit IOMMU problem since we could mark certain devices within the
>> kernel IOMMU driver.
>>
>> I haven't got around to trying to implement the implicit IOMMU support
>> yet, but I suppose we could implement the def_domain_type() hook in the
>> SMMU driver and then return IOMMU_DOMAIN_IDENTITY for the Display/VDE
>> devices. Then the Display/VDE drivers will take over the identity domain
>> and replace it with the explicit domain.
> 
> Actually, the plan (and part of the reason for this patch) is to
> transition the display driver over to using the DMA API rather than
> creating IOMMU domains explicitly.
> 
> Explicit IOMMU usage currently works around this prior to Tegra186
> because IOMMU translations are not enabled until after the device has
> been attached to the IOMMU, at which point a mapping will already have
> been created. It's also part of the reason why we don't support DMA-
> type domains yet on Tegra SMMU, because as soon as we do, this would
> cause a fault storm during boot.
> 
> On Tegra186 this same problem exists because the driver does support DMA
> type domains and hence the kernel will try to set those up for display
> during early boot and before the driver has had a chance to set up
> mappings (or quiesce the display hardware).
> 
>>> That would still require some sort of flag to
>>> specify which reserved-memory regions would need this identity mapping
>>> because, as was pointed out in earlier review, some devices may have
>>> reserved-memory regions that are not meant to be identity mapped.
>>
>> Please note that the reserved-memory approach also creates problem for
>> selection of a large CMA region if FB is located somewhere in a middle
>> of DRAM.
>>
>> I already see that the FB's reserved-memory will break CMA for Nexus 7
>> and Acer A500 because CMA area overlaps with the bootloader's FB :)
>>
>> Also keep in mind that initrd needs a location too and location usually
>> hardwired in a bootloader. Hence it increases pressure on the CMA selection.
> 
> I do understand those issues, but there's really not a lot we can do
> about it. It's wrong for CMA to overlap with the framebuffer from which
> the display hardware just keeps scanning out, so all that these reserved
> memory nodes do is actually fix a long-standing bug (depending on how
> paranoid you are, you might even consider it a security hole).
> 
> For older devices that don't have a bootloader that properly defines
> this memory as reserved, they should be able to continue to use a CMA
> that overlaps with the framebuffer and work just like before.
> 
>>>> Secondly, all NVIDIA bootloaders are passing tegra_fbmem=... via
>>>> kernel's cmdline with the physical location of the framebuffer in
>>>> memory. Maybe we could support this option?
>>>
>>> I'm not a big fan of that command-line option, but I also realize that
>>> for older bootloaders that's probably the only option we have. I don't
>>> suppose all of the devices support U-Boot?
>>
>> Majority of devices in a wild don't use u-boot and they have a
>> locked-down bootloader. Still it's possible to chain-load u-boot or
>> bypass the "security" and replace the bootloader, but these approaches
>> aren't widely supported because they take a lot of effort to be
>> implemented and maintained.
>>
>> Even those devices that use proper u-boot usually never updating it and
>> are running some ancient version. You can't ignore all those people :)
> 
> I'm not trying to ignore all those people, but at the same time I don't
> think legacy devices about which we can't do much should prevent new
> devices from doing the right thing and properly reserving memory that
> the kernel isn't supposed to use.
> 
> You already suggested that we could choose an identity domain by default
> for VDE and display for this. That sounds like a good compromise to me,
> but I think we should do that only on platforms where we can't implement
> this properly.
> 
>>> Because ideally we'd just
>>> translate from tegra_fbmem=... to reserved-memory region there so that
>>> we don't have to carry backwards-compatibility code for these purely
>>> downstream bootloaders.
>>
>> IIRC, in the past Robin Murphy was suggesting to read out hardware state
>> early during kernel boot in order to find what regions are in use by
>> hardware.
>>
>> I think it should be easy to do for the display controller since we
>> could check clock and PD states in order to decide whether DC's IO could
>> be accessed and then read out the FB pointer and size. I guess it should
>> take about hundred lines of code.
>>
>> But the easiest way should be to ignore this trouble for devices that
>> have IOMMU disabled by default and simply allow display to show garbage.
>> Nobody ever complained about this for the past 7+ years :)
>>
>> Hence implementing the dummy-identity domain support should be enough
>> for solving the problem, at least this should work for pre-T186 devices.
> 
> I'd still like to do this properly at least on Tegra210 as well. It
> should be possible to come up with a set of criteria where the dummy
> identity domain should be used and where it shouldn't. I think that's
> the easiest part of this and can easily be implemented as a couple-of-
> lines quirk in some kernel driver.

Any solution should be fine as long as it doesn't break older devices.
If you'll have patches that will needs testing, then please feel free to
ping me.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-01-12 19:00 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 12:59 [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Thierry Reding
2020-09-04 12:59 ` [PATCH v2 2/4] iommu: Implement of_iommu_get_resv_regions() Thierry Reding
2020-09-04 12:59 ` [PATCH v2 3/4] iommu: dma: Use of_iommu_get_resv_regions() Thierry Reding
2020-09-04 13:00 ` [RFC 4/4] iommu/tegra-smmu: Add support for reserved regions Thierry Reding
2020-09-14 22:08 ` [PATCH v2 1/4] dt-bindings: reserved-memory: Document "active" property Rob Herring
2020-09-15 12:36   ` Thierry Reding
2020-09-24 11:27     ` Thierry Reding
2020-11-05 16:43       ` Thierry Reding
2020-11-05 17:47         ` Robin Murphy
2020-11-06 15:25           ` Thierry Reding
2020-11-10 19:33             ` Thierry Reding
2020-12-17 15:00               ` Thierry Reding
2020-12-18 22:15                 ` Rob Herring
2020-12-19  0:49                   ` Thierry Reding
2020-09-24 13:23 ` Dmitry Osipenko
2020-09-24 14:01   ` Thierry Reding
2020-09-24 16:23     ` Dmitry Osipenko
2020-09-25 12:39       ` Robin Murphy
2020-09-25 13:21         ` Dmitry Osipenko
2020-11-05 16:23           ` Thierry Reding
2020-09-25 13:52         ` Dmitry Osipenko
2020-11-05 15:57         ` Thierry Reding
2020-11-05 15:49       ` Thierry Reding
2021-01-12 19:00         ` Dmitry Osipenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).