[U-Boot] [PATCH v2 1/4] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()

[U-Boot] [PATCH v2 1/4] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The change adding fdtdec_add_reserved_memory() already protected the added
phandle against the phandlep being NULL - making the phandlep var optional.

But in the early code checking for an already existing carveout this check
was not done and thus the phandle assignment could run into trouble,
so add a check there as well, which makes the function still return
successfully if a matching region is found, even though no-one wants to
work with the phandle.

Fixes: c9222a08b3f7 ("fdtdec: Implement fdtdec_add_reserved_memory()")
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v2:
- add comment about phandlep allowed to be NULL

 include/fdtdec.h | 1 +
 lib/fdtdec.c     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 635f53083b..54509a25ad 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -1078,6 +1078,7 @@ static inline int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
  * @param basename	base name of the node to create
  * @param carveout	information about the carveout region
  * @param phandlep	return location for the phandle of the carveout region
+ *			can be NULL
  * @return 0 on success or a negative error code on failure
  */
 int fdtdec_add_reserved_memory(void *blob, const char *basename,
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 74430c8b2f..d7c3684145 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1363,7 +1363,8 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 		}
 
 		if (addr == carveout->start && (addr + size) == carveout->end) {
-			*phandlep = fdt_get_phandle(blob, node);
+			if (phandlep)
+				*phandlep = fdt_get_phandle(blob, node);
 			return 0;
 		}
 	}
-- 
2.23.0

[U-Boot] [PATCH v2 2/4] fdtdec: only create phandle if caller wants it in fdtdec_add_reserved_memory()

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The phandlep pointer returning the phandle to the caller is optional
and if it is not set when calling fdtdec_add_reserved_memory() it is
highly likely that the caller is not interested in a phandle to the
created reserved-memory area and really just wants that area added.

So just don't create a phandle in that case.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v2:
- update function comment

 include/fdtdec.h |  2 +-
 lib/fdtdec.c     | 16 +++++++++-------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 54509a25ad..fe8e7770ec 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -1078,7 +1078,7 @@ static inline int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
  * @param basename	base name of the node to create
  * @param carveout	information about the carveout region
  * @param phandlep	return location for the phandle of the carveout region
- *			can be NULL
+ *			can be NULL if no phandle should be added
  * @return 0 on success or a negative error code on failure
  */
 int fdtdec_add_reserved_memory(void *blob, const char *basename,
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index d7c3684145..c841aafa2a 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -1393,13 +1393,15 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename,
 	if (node < 0)
 		return node;
 
-	err = fdt_generate_phandle(blob, &phandle);
-	if (err < 0)
-		return err;
-
-	err = fdtdec_set_phandle(blob, node, phandle);
-	if (err < 0)
-		return err;
+	if (phandlep) {
+		err = fdt_generate_phandle(blob, &phandle);
+		if (err < 0)
+			return err;
+
+		err = fdtdec_set_phandle(blob, node, phandle);
+		if (err < 0)
+			return err;
+	}
 
 	/* store one or two address cells */
 	if (na > 1)
-- 
2.23.0

[U-Boot] [PATCH v2 2/4] fdtdec: only create phandle if caller wants it in fdtdec_add_reserved_memory()

[Simon Glass]

On Tue, 22 Oct 2019 at 13:04, Heiko Stuebner <heiko@sntech.de> wrote:
>
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>
> The phandlep pointer returning the phandle to the caller is optional
> and if it is not set when calling fdtdec_add_reserved_memory() it is
> highly likely that the caller is not interested in a phandle to the
> created reserved-memory area and really just wants that area added.
>
> So just don't create a phandle in that case.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
> changes in v2:
> - update function comment
>
>  include/fdtdec.h |  2 +-
>  lib/fdtdec.c     | 16 +++++++++-------
>  2 files changed, 10 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 3/4] image: fdt: copy possible optee nodes to a loaded devicetree

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The loading convention for optee or any other tee on arm64 is as bl32
parameter to the trusted-firmware. So TF-A gets invoked with the TEE as
bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps
into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33.

All of them get passed a devicetree as parameter and all components often
get loaded from a FIT image.

OP-TEE will create additional nodes in that devicetree namely a firmware
node and possibly multiple reserved-memory nodes.

While this devicetree is used in main u-boot, in most cases it won't be
the one passed to the actual kernel. Instead most boot commands will load
a new devicetree from somewhere like mass storage of the network, so if
that happens u-boot should transfer the optee nodes to that new devicetree.

To make that happen introduce optee_copy_fdt_nodes() called from the dt
setup function in image-fdt which after checking for the optee presence
in the u-boot dt will make sure a optee node is present in the kernel dt
and transfer any reserved-memory regions it can find.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
changes in v2:
- don't create a new optee firmware-node, but instead copy the
  compatible+method properties from the old fdt blob.

 common/image-fdt.c  |   8 +++
 include/tee/optee.h |   9 +++
 lib/optee/optee.c   | 132 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)

diff --git a/common/image-fdt.c b/common/image-fdt.c
index 4247dcee0c..48388488d9 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -17,6 +17,7 @@
 #include <linux/libfdt.h>
 #include <mapmem.h>
 #include <asm/io.h>
+#include <tee/optee.h>
 
 #ifndef CONFIG_SYS_FDT_PAD
 #define CONFIG_SYS_FDT_PAD 0x3000
@@ -561,6 +562,13 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
 		}
 	}
 
+	fdt_ret = optee_copy_fdt_nodes(gd->fdt_blob, blob);
+	if (fdt_ret) {
+		printf("ERROR: transfer of optee nodes to new fdt failed: %s\n",
+		       fdt_strerror(fdt_ret));
+		goto err;
+	}
+
 	/* Delete the old LMB reservation */
 	if (lmb)
 		lmb_free(lmb, (phys_addr_t)(u32)(uintptr_t)blob,
diff --git a/include/tee/optee.h b/include/tee/optee.h
index 9446928fd4..121b30a303 100644
--- a/include/tee/optee.h
+++ b/include/tee/optee.h
@@ -67,4 +67,13 @@ static inline int optee_verify_bootm_image(unsigned long image_addr,
 }
 #endif
 
+#if defined(CONFIG_OPTEE) && defined(CONFIG_OF_LIBFDT)
+int optee_copy_fdt_nodes(const void *old_blob, void *new_blob);
+#else
+static inline int optee_copy_fdt_nodes(const void *old_blob, void *new_blob)
+{
+	return 0;
+}
+#endif
+
 #endif /* _OPTEE_H */
diff --git a/lib/optee/optee.c b/lib/optee/optee.c
index db92cd9af2..2f71ded536 100644
--- a/lib/optee/optee.c
+++ b/lib/optee/optee.c
@@ -5,6 +5,8 @@
  */
 
 #include <common.h>
+#include <malloc.h>
+#include <linux/libfdt.h>
 #include <tee/optee.h>
 
 #define optee_hdr_err_msg \
@@ -63,3 +65,133 @@ error:
 
 	return ret;
 }
+
+#if defined(CONFIG_OF_LIBFDT)
+static int optee_copy_firmware_node(const void *old_blob, void *fdt_blob)
+{
+	int old_offs, offs, ret, len;
+	const void *prop;
+
+	if (fdt_path_offset(fdt_blob, "/firmware/optee") >= 0) {
+		debug("OP-TEE Device Tree node already exists");
+		return 0;
+	}
+
+	old_offs = fdt_path_offset(old_blob, "/firmware/optee");
+	if (old_offs < 0) {
+		debug("Original OP-TEE Device Tree node not found");
+		return old_offs;
+	}
+
+	offs = fdt_path_offset(fdt_blob, "/firmware");
+	if (offs < 0) {
+		offs = fdt_path_offset(fdt_blob, "/");
+		if (offs < 0)
+			return offs;
+
+		offs = fdt_add_subnode(fdt_blob, offs, "firmware");
+		if (offs < 0)
+			return offs;
+	}
+
+	offs = fdt_add_subnode(fdt_blob, offs, "optee");
+	if (offs < 0)
+		return ret;
+
+	/* copy the compatible property */
+	prop = fdt_getprop(old_blob, old_offs, "compatible", &len);
+	if (!prop) {
+		debug("missing OP-TEE compatible property");
+		return -EINVAL;
+	}
+
+	ret = fdt_setprop(fdt_blob, offs, "compatible", prop, len);
+	if (ret < 0)
+		return ret;
+
+	/* copy the method property */
+	prop = fdt_getprop(old_blob, old_offs, "method", &len);
+	if (!prop) {
+		debug("missing OP-TEE method property");
+		return -EINVAL;
+	}
+
+	ret = fdt_setprop(fdt_blob, offs, "method", prop, len);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+int optee_copy_fdt_nodes(const void *old_blob, void *new_blob)
+{
+	int nodeoffset, subnode, ret;
+	struct fdt_resource res;
+
+	if (fdt_check_header(old_blob))
+		return -EINVAL;
+
+	/* only proceed if there is an /firmware/optee node */
+	if (fdt_path_offset(old_blob, "/firmware/optee") < 0) {
+		debug("No OP-TEE firmware node in old fdt, nothing to do");
+		return 0;
+	}
+
+	ret = optee_copy_firmware_node(old_blob, new_blob);
+	if (ret < 0) {
+		printf("Failed to add OP-TEE firmware node\n");
+		return ret;
+	}
+
+	/* optee inserts its memory regions as reserved-memory nodes */
+	nodeoffset = fdt_subnode_offset(old_blob, 0, "reserved-memory");
+	if (nodeoffset >= 0) {
+		subnode = fdt_first_subnode(old_blob, nodeoffset);
+		while (subnode >= 0) {
+			const char *name = fdt_get_name(old_blob,
+							subnode, NULL);
+			if (!name)
+				return -EINVAL;
+
+			/* only handle optee reservations */
+			if (strncmp(name, "optee", 5))
+				continue;
+
+			/* check if this subnode has a reg property */
+			ret = fdt_get_resource(old_blob, subnode, "reg", 0,
+					       &res);
+			if (!ret) {
+				struct fdt_memory carveout = {
+					.start = res.start,
+					.end = res.end,
+				};
+				char *oldname, *nodename, *tmp;
+
+				oldname = strdup(name);
+				if (!oldname)
+					return -ENOMEM;
+
+				tmp = oldname;
+				nodename = strsep(&tmp, "@");
+				if (!nodename) {
+					free(oldname);
+					return -EINVAL;
+				}
+
+				ret = fdtdec_add_reserved_memory(new_blob,
+								 nodename,
+								 &carveout,
+								 NULL);
+				free(oldname);
+
+				if (ret < 0)
+					return ret;
+			}
+
+			subnode = fdt_next_subnode(old_blob, subnode);
+		}
+	}
+
+	return 0;
+}
+#endif
-- 
2.23.0

[U-Boot] [PATCH v2 3/4] image: fdt: copy possible optee nodes to a loaded devicetree

[Jens Wiklander]

On Tue, Oct 22, 2019 at 09:04:27PM +0200, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> The loading convention for optee or any other tee on arm64 is as bl32
> parameter to the trusted-firmware. So TF-A gets invoked with the TEE as
> bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps
> into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33.
> 
> All of them get passed a devicetree as parameter and all components often
> get loaded from a FIT image.
> 
> OP-TEE will create additional nodes in that devicetree namely a firmware
> node and possibly multiple reserved-memory nodes.
> 
> While this devicetree is used in main u-boot, in most cases it won't be
> the one passed to the actual kernel. Instead most boot commands will load
> a new devicetree from somewhere like mass storage of the network, so if
> that happens u-boot should transfer the optee nodes to that new devicetree.
> 
> To make that happen introduce optee_copy_fdt_nodes() called from the dt
> setup function in image-fdt which after checking for the optee presence
> in the u-boot dt will make sure a optee node is present in the kernel dt
> and transfer any reserved-memory regions it can find.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
> changes in v2:
> - don't create a new optee firmware-node, but instead copy the
>   compatible+method properties from the old fdt blob.
> 
>  common/image-fdt.c  |   8 +++
>  include/tee/optee.h |   9 +++
>  lib/optee/optee.c   | 132 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
> 
[snip]

Looks good to me:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Cheers,
Jens

[U-Boot] [PATCH v2 3/4] image: fdt: copy possible optee nodes to a loaded devicetree

[Patrick DELAUNAY]

Hi Jens and Heiko,

> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Jens Wiklander
> Sent: mercredi 23 octobre 2019 08:46
> 
> On Tue, Oct 22, 2019 at 09:04:27PM +0200, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >
> > The loading convention for optee or any other tee on arm64 is as bl32
> > parameter to the trusted-firmware. So TF-A gets invoked with the TEE
> > as
> > bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps
> > into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33.
> >
> > All of them get passed a devicetree as parameter and all components
> > often get loaded from a FIT image.
> >
> > OP-TEE will create additional nodes in that devicetree namely a
> > firmware node and possibly multiple reserved-memory nodes.
> >
> > While this devicetree is used in main u-boot, in most cases it won't
> > be the one passed to the actual kernel. Instead most boot commands
> > will load a new devicetree from somewhere like mass storage of the
> > network, so if that happens u-boot should transfer the optee nodes to that new
> devicetree.
> >
> > To make that happen introduce optee_copy_fdt_nodes() called from the
> > dt setup function in image-fdt which after checking for the optee
> > presence in the u-boot dt will make sure a optee node is present in
> > the kernel dt and transfer any reserved-memory regions it can find.
> >
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > ---
> > changes in v2:
> > - don't create a new optee firmware-node, but instead copy the
> >   compatible+method properties from the old fdt blob.
> >
> >  common/image-fdt.c  |   8 +++
> >  include/tee/optee.h |   9 +++
> >  lib/optee/optee.c   | 132 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 149 insertions(+)
> >
> [snip]

On STM32MP1 platform (armv7 with TF-A support), 
we can use BL32 = OP-TEE or spmin provide by TF-A

So we are plan to have the same type of feature but with an inversed logical:

- Op-Tee nodes (firmwares and reserved) are present in kernel device tree
- U-Boot deactivated these nodes when TEE is not present, with the next function called in ft_system_setup:

static void stm32_fdt_disable_optee(void *blob)
{
	int off, node;

	off = fdt_node_offset_by_compatible(blob, -1, "linaro,optee-tz");
	if (off >= 0 && fdtdec_get_is_enabled(blob, off))
		fdt_status_disabled(blob, off);

	/* Disabled "optee at ..." reserved-memory node */
	off = fdt_path_offset(blob, "/reserved-memory/");
	if (off < 0)
		return;
	for (node = fdt_first_subnode(blob, off);
	     node >= 0;
	     node = fdt_next_subnode(blob, node)) {
		if (!strncmp(fdt_get_name(blob, node, NULL), "optee@", 6))
			fdt_status_disabled(blob, node);
	}
}

What is  the better for you ?
1/ Copy the U-Boot op-tee nodes in kernel device tree (where op-tee nodes was present)
or 
2/ Deactivate the op-tee nodes in kernel device tree (where op-tee nodes are present)

The advantage of us is to upstream a Linux Kernel device tree with the correct op-tee nodes...

Regards

> Looks good to me:
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> 
> Cheers,
> Jens

Patrick

[U-Boot] [PATCH v2 3/4] image: fdt: copy possible optee nodes to a loaded devicetree

[Heiko Stübner]

Hi Patrick,

Am Mittwoch, 23. Oktober 2019, 09:10:52 CEST schrieb Patrick DELAUNAY:
> Hi Jens and Heiko,
> 
> > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Jens Wiklander
> > Sent: mercredi 23 octobre 2019 08:46
> > 
> > On Tue, Oct 22, 2019 at 09:04:27PM +0200, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > >
> > > The loading convention for optee or any other tee on arm64 is as bl32
> > > parameter to the trusted-firmware. So TF-A gets invoked with the TEE
> > > as
> > > bl32 and main u-boot as bl33. Once it has done its startup TF-A jumps
> > > into the bl32 for the TEE startup, returns to TF-A and then jumps to bl33.
> > >
> > > All of them get passed a devicetree as parameter and all components
> > > often get loaded from a FIT image.
> > >
> > > OP-TEE will create additional nodes in that devicetree namely a
> > > firmware node and possibly multiple reserved-memory nodes.
> > >
> > > While this devicetree is used in main u-boot, in most cases it won't
> > > be the one passed to the actual kernel. Instead most boot commands
> > > will load a new devicetree from somewhere like mass storage of the
> > > network, so if that happens u-boot should transfer the optee nodes to that new
> > devicetree.
> > >
> > > To make that happen introduce optee_copy_fdt_nodes() called from the
> > > dt setup function in image-fdt which after checking for the optee
> > > presence in the u-boot dt will make sure a optee node is present in
> > > the kernel dt and transfer any reserved-memory regions it can find.
> > >
> > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > ---
> > > changes in v2:
> > > - don't create a new optee firmware-node, but instead copy the
> > >   compatible+method properties from the old fdt blob.
> > >
> > >  common/image-fdt.c  |   8 +++
> > >  include/tee/optee.h |   9 +++
> > >  lib/optee/optee.c   | 132 ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 149 insertions(+)
> > >
> > [snip]
> 
> On STM32MP1 platform (armv7 with TF-A support), 
> we can use BL32 = OP-TEE or spmin provide by TF-A

That is the same I'm hoping to have at some point for the RK3288
soc I'm also working on :-D .

But why do you need different handling then? Except BL32 being baked
into the TF-A binary on arm32, shouldn't the loading patch be somewhat
similar (SPL -> TF-A -> OP-Tee -> U-Boot) so that OP-Tee also can insert its
nodes there?

(Haven't looked too deeply at this yet)

> So we are plan to have the same type of feature but with an inversed logical:
> 
> - Op-Tee nodes (firmwares and reserved) are present in kernel device tree
> - U-Boot deactivated these nodes when TEE is not present, with the next function called in ft_system_setup:

Having the OP-Tee nodes hard-coded in the devicetree in kernel sources
sounds somewhat counter intuitive to me.

For the firmware node it shouldn't matter much, except that there may
also be other TEEs other than OP-Tee that people may want to load.

But when the reserved memory is hard-coded in the kernel, you bind
yourself to one specific version of your TEE. I.e. what happens when
a future version of your platform support wants to move the memory
region OP-Tee wants to occupy. With OP-Tee handling the reservation
this should be somewhat transparent to bootloader and kernel, as
just the reserved-memory changes.


> static void stm32_fdt_disable_optee(void *blob)
> {
> 	int off, node;
> 
> 	off = fdt_node_offset_by_compatible(blob, -1, "linaro,optee-tz");
> 	if (off >= 0 && fdtdec_get_is_enabled(blob, off))
> 		fdt_status_disabled(blob, off);
> 
> 	/* Disabled "optee at ..." reserved-memory node */
> 	off = fdt_path_offset(blob, "/reserved-memory/");
> 	if (off < 0)
> 		return;
> 	for (node = fdt_first_subnode(blob, off);
> 	     node >= 0;
> 	     node = fdt_next_subnode(blob, node)) {
> 		if (!strncmp(fdt_get_name(blob, node, NULL), "optee@", 6))
> 			fdt_status_disabled(blob, node);
> 	}
> }
> 
> What is  the better for you ?
> 1/ Copy the U-Boot op-tee nodes in kernel device tree (where op-tee nodes was present)
> or 
> 2/ Deactivate the op-tee nodes in kernel device tree (where op-tee nodes are present)
> 
> The advantage of us is to upstream a Linux Kernel device tree with the correct op-tee nodes...

In any case, the copying we're doing here should not affect any present
OP-Tee nodes in the kernel dts and only proceed if both
(a) U-Boot dts does contain an OP-Tee node
(b) Kernel dts does _not yet_ contain an OP-Tee node

are valid.


Heiko

> > Looks good to me:
> > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
> > 
> > Cheers,
> > Jens
> 
> Patrick
> 

[U-Boot] [PATCH v2 3/4] image: fdt: copy possible optee nodes to a loaded devicetree

[Patrick DELAUNAY]

Hi Heiko,

> 
> Hi Patrick,
> 
> Am Mittwoch, 23. Oktober 2019, 09:10:52 CEST schrieb Patrick DELAUNAY:
> > Hi Jens and Heiko,
> >
> > > From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Jens
> > > Wiklander
> > > Sent: mercredi 23 octobre 2019 08:46
> > >
> > > On Tue, Oct 22, 2019 at 09:04:27PM +0200, Heiko Stuebner wrote:
> > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > > >
> > > > The loading convention for optee or any other tee on arm64 is as
> > > > bl32 parameter to the trusted-firmware. So TF-A gets invoked with
> > > > the TEE as
> > > > bl32 and main u-boot as bl33. Once it has done its startup TF-A
> > > > jumps into the bl32 for the TEE startup, returns to TF-A and then jumps to
> bl33.
> > > >
> > > > All of them get passed a devicetree as parameter and all
> > > > components often get loaded from a FIT image.
> > > >
> > > > OP-TEE will create additional nodes in that devicetree namely a
> > > > firmware node and possibly multiple reserved-memory nodes.
> > > >
> > > > While this devicetree is used in main u-boot, in most cases it
> > > > won't be the one passed to the actual kernel. Instead most boot
> > > > commands will load a new devicetree from somewhere like mass
> > > > storage of the network, so if that happens u-boot should transfer
> > > > the optee nodes to that new
> > > devicetree.
> > > >
> > > > To make that happen introduce optee_copy_fdt_nodes() called from
> > > > the dt setup function in image-fdt which after checking for the
> > > > optee presence in the u-boot dt will make sure a optee node is
> > > > present in the kernel dt and transfer any reserved-memory regions it can find.
> > > >
> > > > Signed-off-by: Heiko Stuebner
> > > > <heiko.stuebner@theobroma-systems.com>
> > > > ---
> > > > changes in v2:
> > > > - don't create a new optee firmware-node, but instead copy the
> > > >   compatible+method properties from the old fdt blob.
> > > >
> > > >  common/image-fdt.c  |   8 +++
> > > >  include/tee/optee.h |   9 +++
> > > >  lib/optee/optee.c   | 132
> ++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 149 insertions(+)
> > > >
> > > [snip]
> >
> > On STM32MP1 platform (armv7 with TF-A support), we can use BL32 =
> > OP-TEE or spmin provide by TF-A
> 
> That is the same I'm hoping to have at some point for the RK3288 soc I'm also
> working on :-D .
> 
> But why do you need different handling then? Except BL32 being baked into the
> TF-A binary on arm32, shouldn't the loading patch be somewhat similar (SPL ->
> TF-A -> OP-Tee -> U-Boot) so that OP-Tee also can insert its nodes there?

We have official 2 boot chains support by STM32MP15 serie in U-Boot

ROM code => TF-A (BL2 & spmin) => U-Boot => kernel
= stm32mp15_trusted_defconfig

ROM code => TF-A (BL2) => OP-TEE => U-Boot => kernel
= stm32mp15_optee_defconfig

We have also SPL but only for U-Boot test and with 
minimal PSCI support in u-boot (no low-power mode,
no security support)

ROM code => SPL => U-Boot (install PSCI) => kernel
= stm32mp15_basic_defconfig

For details:
https://wiki.st.com/stm32mpu/index.php/Boot_chains_overview#STM32MP15_case


> (Haven't looked too deeply at this yet)
> 
> > So we are plan to have the same type of feature but with an inversed logical:
> >
> > - Op-Tee nodes (firmwares and reserved) are present in kernel device
> > tree
> > - U-Boot deactivated these nodes when TEE is not present, with the next
> function called in ft_system_setup:
> 
> Having the OP-Tee nodes hard-coded in the devicetree in kernel sources sounds
> somewhat counter intuitive to me.
> 
> For the firmware node it shouldn't matter much, except that there may also be
> other TEEs other than OP-Tee that people may want to load.
> 
> But when the reserved memory is hard-coded in the kernel, you bind yourself to
> one specific version of your TEE. I.e. what happens when a future version of your
> platform support wants to move the memory region OP-Tee wants to occupy. With
> OP-Tee handling the reservation this should be somewhat transparent to
> bootloader and kernel, as just the reserved-memory changes.

I don't check the code and architecture of OP-TEE since a long time
and with your answer I just discover the non-secure device tree 
modification done by the OP-Tee (even it is present since v2.1.0)

> 
> > static void stm32_fdt_disable_optee(void *blob) {
> > 	int off, node;
> >
> > 	off = fdt_node_offset_by_compatible(blob, -1, "linaro,optee-tz");
> > 	if (off >= 0 && fdtdec_get_is_enabled(blob, off))
> > 		fdt_status_disabled(blob, off);
> >
> > 	/* Disabled "optee at ..." reserved-memory node */
> > 	off = fdt_path_offset(blob, "/reserved-memory/");
> > 	if (off < 0)
> > 		return;
> > 	for (node = fdt_first_subnode(blob, off);
> > 	     node >= 0;
> > 	     node = fdt_next_subnode(blob, node)) {
> > 		if (!strncmp(fdt_get_name(blob, node, NULL), "optee@", 6))
> > 			fdt_status_disabled(blob, node);
> > 	}
> > }
> >
> > What is  the better for you ?
> > 1/ Copy the U-Boot op-tee nodes in kernel device tree (where op-tee
> > nodes was present) or 2/ Deactivate the op-tee nodes in kernel device
> > tree (where op-tee nodes are present)
> >
> > The advantage of us is to upstream a Linux Kernel device tree with the correct
> op-tee nodes...
> 
> In any case, the copying we're doing here should not affect any present OP-Tee
> nodes in the kernel dts and only proceed if both
> (a) U-Boot dts does contain an OP-Tee node
> (b) Kernel dts does _not yet_ contain an OP-Tee node
> 
> are valid.


Now I understood your patch....

I need to check to correctly handle this behavior on my side
as stm32mp15 TF-A don't using FIT and SPL is not used.

But I think that the U-Boot device tree need to be loaded by BL2 and no more integrated in
U-Boot image thus OP-Tee can modify it.

ROM code => TF-A (BL2) = Load TF-A spmin & U-Boot & Device Tree  
			   => spmin => U-Boot => kernel

ROM code => TF-A (BL2) = Load OP-TEE & U-Boot & Device Tree 
                                          => OP-TEE => U-Boot => kernel
 
> 
> Heiko
> 

Thanks again for the answer.

Patrick

[U-Boot] [PATCH v2 4/4] tests: add OP-TEE test suite

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

OP-TEE can get supplied with a devicetree and will then insert
its firmware node and reserved-memory sections into it.
As this devicetree often is not the one supplied to a later
loaded kernel, a previous commit added functionality to transfer
these nodes onto that new devicetree.

To make sure this functionality stays intact, also add a test
for the transfer functionality.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 Makefile                           |   1 +
 include/test/optee.h               |  14 +++
 include/test/suites.h              |   1 +
 test/Kconfig                       |   1 +
 test/cmd_ut.c                      |   6 ++
 test/optee/Kconfig                 |   7 ++
 test/optee/Makefile                |  13 +++
 test/optee/cmd_ut_optee.c          | 143 +++++++++++++++++++++++++++++
 test/optee/test-optee-base.dts     |  13 +++
 test/optee/test-optee-no-optee.dts |  13 +++
 test/optee/test-optee-optee.dts    |  32 +++++++
 11 files changed, 244 insertions(+)
 create mode 100644 include/test/optee.h
 create mode 100644 test/optee/Kconfig
 create mode 100644 test/optee/Makefile
 create mode 100644 test/optee/cmd_ut_optee.c
 create mode 100644 test/optee/test-optee-base.dts
 create mode 100644 test/optee/test-optee-no-optee.dts
 create mode 100644 test/optee/test-optee-optee.dts

diff --git a/Makefile b/Makefile
index 54da5cd51c..dc30dc8d93 100644
--- a/Makefile
+++ b/Makefile
@@ -746,6 +746,7 @@ libs-$(CONFIG_API) += api/
 libs-$(CONFIG_HAS_POST) += post/
 libs-$(CONFIG_UNIT_TEST) += test/ test/dm/
 libs-$(CONFIG_UT_ENV) += test/env/
+libs-$(CONFIG_UT_OPTEE) += test/optee/
 libs-$(CONFIG_UT_OVERLAY) += test/overlay/
 
 libs-y += $(if $(BOARDDIR),board/$(BOARDDIR)/)
diff --git a/include/test/optee.h b/include/test/optee.h
new file mode 100644
index 0000000000..a8c6e6395f
--- /dev/null
+++ b/include/test/optee.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
+ */
+
+#ifndef __TEST_OPTEE_H__
+#define __TEST_OPTEE_H__
+
+#include <test/test.h>
+
+/* Declare a new environment test */
+#define OPTEE_TEST(_name, _flags)	UNIT_TEST(_name, _flags, optee_test)
+
+#endif /* __TEST_OPTEE_H__ */
diff --git a/include/test/suites.h b/include/test/suites.h
index 01bee09346..20970f08d6 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -28,6 +28,7 @@ int do_ut_compression(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_dm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_env(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_lib(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
+int do_ut_optee(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_unicode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
diff --git a/test/Kconfig b/test/Kconfig
index 48a0e501f8..f53629aac5 100644
--- a/test/Kconfig
+++ b/test/Kconfig
@@ -33,4 +33,5 @@ config UT_UNICODE
 
 source "test/dm/Kconfig"
 source "test/env/Kconfig"
+source "test/optee/Kconfig"
 source "test/overlay/Kconfig"
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index e3b89504e7..2781f8bd56 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -43,6 +43,9 @@ static cmd_tbl_t cmd_ut_sub[] = {
 #if defined(CONFIG_UT_ENV)
 	U_BOOT_CMD_MKENT(env, CONFIG_SYS_MAXARGS, 1, do_ut_env, "", ""),
 #endif
+#ifdef CONFIG_UT_OPTEE
+	U_BOOT_CMD_MKENT(optee, CONFIG_SYS_MAXARGS, 1, do_ut_optee, "", ""),
+#endif
 #ifdef CONFIG_UT_OVERLAY
 	U_BOOT_CMD_MKENT(overlay, CONFIG_SYS_MAXARGS, 1, do_ut_overlay, "", ""),
 #endif
@@ -114,6 +117,9 @@ static char ut_help_text[] =
 #ifdef CONFIG_UT_LIB
 	"ut lib [test-name] - test library functions\n"
 #endif
+#ifdef CONFIG_UT_OPTEE
+	"ut optee [test-name]\n"
+#endif
 #ifdef CONFIG_UT_OVERLAY
 	"ut overlay [test-name]\n"
 #endif
diff --git a/test/optee/Kconfig b/test/optee/Kconfig
new file mode 100644
index 0000000000..2f6834aa3b
--- /dev/null
+++ b/test/optee/Kconfig
@@ -0,0 +1,7 @@
+config UT_OPTEE
+	bool "Enable OP-TEE Unit Tests"
+	depends on UNIT_TEST && OF_CONTROL && OPTEE
+	default y
+	help
+	  This enables the 'ut optee' command which runs a series of unit
+	  tests on the optee library code..
diff --git a/test/optee/Makefile b/test/optee/Makefile
new file mode 100644
index 0000000000..8793fd7ad6
--- /dev/null
+++ b/test/optee/Makefile
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
+
+# Test files
+obj-y += cmd_ut_optee.o
+
+DTC_FLAGS += -@
+
+# DT overlays
+obj-y += test-optee-base.dtb.o
+obj-y += test-optee-optee.dtb.o
+obj-y += test-optee-no-optee.dtb.o
diff --git a/test/optee/cmd_ut_optee.c b/test/optee/cmd_ut_optee.c
new file mode 100644
index 0000000000..b11fc51048
--- /dev/null
+++ b/test/optee/cmd_ut_optee.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
+ */
+
+#include <common.h>
+#include <command.h>
+#include <errno.h>
+#include <fdt_support.h>
+#include <malloc.h>
+#include <tee/optee.h>
+
+#include <linux/sizes.h>
+
+#include <test/ut.h>
+#include <test/optee.h>
+#include <test/suites.h>
+
+/* 4k ought to be enough for anybody */
+#define FDT_COPY_SIZE	(4 * SZ_1K)
+
+extern u32 __dtb_test_optee_base_begin;
+extern u32 __dtb_test_optee_optee_begin;
+extern u32 __dtb_test_optee_no_optee_begin;
+
+static void *fdt;
+static bool expect_success;
+
+static int optee_fdt_firmware(struct unit_test_state *uts)
+{
+	const void *prop;
+	int offs, len;
+
+	offs = fdt_path_offset(fdt, "/firmware/optee");
+	ut_assert(expect_success ? offs >= 0 : offs < 0);
+
+	/* only continue if we have an optee node */
+	if (offs < 0)
+		return CMD_RET_SUCCESS;
+
+	prop = fdt_getprop(fdt, offs, "compatible", &len);
+	ut_assertok(strncmp((const char *)prop, "linaro,optee-tz", len));
+
+	prop = fdt_getprop(fdt, offs, "method", &len);
+	ut_assert(strncmp(prop, "hvc", 3) == 0 || strncmp(prop, "smc", 3) == 0);
+
+	return CMD_RET_SUCCESS;
+}
+OPTEE_TEST(optee_fdt_firmware, 0);
+
+static int optee_fdt_protected_memory(struct unit_test_state *uts)
+{
+	const void *prop;
+	int offs, subnode, len;
+	bool found;
+
+	offs = fdt_path_offset(fdt, "/firmware/optee");
+	ut_assert(expect_success ? offs >= 0 : offs < 0);
+
+	/* only continue if we have an optee node */
+	if (offs < 0)
+		return CMD_RET_SUCCESS;
+
+	/* optee inserts its memory regions as reserved-memory nodes */
+	offs = fdt_subnode_offset(fdt, 0, "reserved-memory");
+	ut_assert(offs >= 0);
+
+	subnode = fdt_first_subnode(fdt, offs);
+	ut_assert(subnode);
+
+	found = 0;
+	while (subnode >= 0) {
+		const char *name = fdt_get_name(fdt, subnode, NULL);
+		struct fdt_resource res;
+
+		ut_assert(name);
+
+		/* only handle optee reservations */
+		if (strncmp(name, "optee", 5))
+			continue;
+
+		found = true;
+
+		/* check if this subnode has a reg property */
+		ut_assertok(fdt_get_resource(fdt, subnode, "reg", 0, &res));
+		subnode = fdt_next_subnode(fdt, subnode);
+	}
+
+	ut_assert(found);
+
+	return CMD_RET_SUCCESS;
+}
+OPTEE_TEST(optee_fdt_protected_memory, 0);
+
+int do_ut_optee(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct unit_test *tests = ll_entry_start(struct unit_test,
+						 optee_test);
+	const int n_ents = ll_entry_count(struct unit_test, optee_test);
+	struct unit_test_state *uts;
+	void *fdt_optee = &__dtb_test_optee_optee_begin;
+	void *fdt_no_optee = &__dtb_test_optee_no_optee_begin;
+	void *fdt_base = &__dtb_test_optee_base_begin;
+	int ret = -ENOMEM;
+
+	uts = calloc(1, sizeof(*uts));
+	if (!uts)
+		return -ENOMEM;
+
+	ut_assertok(fdt_check_header(fdt_base));
+	ut_assertok(fdt_check_header(fdt_optee));
+	ut_assertok(fdt_check_header(fdt_no_optee));
+
+	fdt = malloc(FDT_COPY_SIZE);
+	if (!fdt)
+		return ret;
+
+	/*
+	 * Resize the FDT to 4k so that we have room to operate on
+	 *
+	 * (and relocate it since the memory might be mapped
+	 * read-only)
+	 */
+	ut_assertok(fdt_open_into(fdt_base, fdt, FDT_COPY_SIZE));
+
+	/*
+	 * Try to copy optee nodes from empty dt.
+	 * This should still run successfully.
+	 */
+	ut_assertok(optee_copy_fdt_nodes(fdt_no_optee, fdt));
+
+	expect_success = false;
+	ret = cmd_ut_category("optee", tests, n_ents, argc, argv);
+
+	/* Try to copy optee nodes from prefilled dt */
+	ut_assertok(optee_copy_fdt_nodes(fdt_optee, fdt));
+
+	expect_success = true;
+	ret = cmd_ut_category("optee", tests, n_ents, argc, argv);
+
+	free(fdt);
+	return ret;
+}
diff --git a/test/optee/test-optee-base.dts b/test/optee/test-optee-base.dts
new file mode 100644
index 0000000000..3c1d0c60ef
--- /dev/null
+++ b/test/optee/test-optee-base.dts
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
+ */
+
+/dts-v1/;
+
+/ {
+	#address-cells = <2>;
+	#size-cells = <2>;
+};
+
+
diff --git a/test/optee/test-optee-no-optee.dts b/test/optee/test-optee-no-optee.dts
new file mode 100644
index 0000000000..3c1d0c60ef
--- /dev/null
+++ b/test/optee/test-optee-no-optee.dts
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
+ */
+
+/dts-v1/;
+
+/ {
+	#address-cells = <2>;
+	#size-cells = <2>;
+};
+
+
diff --git a/test/optee/test-optee-optee.dts b/test/optee/test-optee-optee.dts
new file mode 100644
index 0000000000..11e26a2728
--- /dev/null
+++ b/test/optee/test-optee-optee.dts
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019, Theobroma Systems Design und Consulting GmbH
+ */
+
+/dts-v1/;
+
+/ {
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	firmware {
+		optee {
+			compatible = "linaro,optee-tz";
+			method = "smc";
+		};
+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		optee_shm at 32000000 {
+			reg = <0x00 0x32000000 0x00 0x400000>;
+		};
+
+		optee_core at 30000000 {
+			reg = <0x00 0x30000000 0x00 0x2000000>;
+		};
+	};
+};
-- 
2.23.0

[U-Boot] [PATCH v2 4/4] tests: add OP-TEE test suite

[Simon Glass]

On Tue, 22 Oct 2019 at 13:04, Heiko Stuebner <heiko@sntech.de> wrote:
>
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>
> OP-TEE can get supplied with a devicetree and will then insert
> its firmware node and reserved-memory sections into it.
> As this devicetree often is not the one supplied to a later
> loaded kernel, a previous commit added functionality to transfer
> these nodes onto that new devicetree.
>
> To make sure this functionality stays intact, also add a test
> for the transfer functionality.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
>  Makefile                           |   1 +
>  include/test/optee.h               |  14 +++
>  include/test/suites.h              |   1 +
>  test/Kconfig                       |   1 +
>  test/cmd_ut.c                      |   6 ++
>  test/optee/Kconfig                 |   7 ++
>  test/optee/Makefile                |  13 +++
>  test/optee/cmd_ut_optee.c          | 143 +++++++++++++++++++++++++++++
>  test/optee/test-optee-base.dts     |  13 +++
>  test/optee/test-optee-no-optee.dts |  13 +++
>  test/optee/test-optee-optee.dts    |  32 +++++++
>  11 files changed, 244 insertions(+)
>  create mode 100644 include/test/optee.h
>  create mode 100644 test/optee/Kconfig
>  create mode 100644 test/optee/Makefile
>  create mode 100644 test/optee/cmd_ut_optee.c
>  create mode 100644 test/optee/test-optee-base.dts
>  create mode 100644 test/optee/test-optee-no-optee.dts
>  create mode 100644 test/optee/test-optee-optee.dts

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 1/4] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()

[Simon Glass]

On Tue, 22 Oct 2019 at 13:04, Heiko Stuebner <heiko@sntech.de> wrote:
>
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>
> The change adding fdtdec_add_reserved_memory() already protected the added
> phandle against the phandlep being NULL - making the phandlep var optional.
>
> But in the early code checking for an already existing carveout this check
> was not done and thus the phandle assignment could run into trouble,
> so add a check there as well, which makes the function still return
> successfully if a matching region is found, even though no-one wants to
> work with the phandle.
>
> Fixes: c9222a08b3f7 ("fdtdec: Implement fdtdec_add_reserved_memory()")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> ---
> changes in v2:
> - add comment about phandlep allowed to be NULL
>
>  include/fdtdec.h | 1 +
>  lib/fdtdec.c     | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>