[U-Boot,3/3] image: fdt: copy possible optee nodes to a loaded devicetree
diff mbox series

Message ID 20191008002207.14396-3-heiko@sntech.de
State Superseded
Delegated to: Simon Glass
Headers show
Series
  • [U-Boot,1/3] fdtdec: protect against another NULL phandlep in fdtdec_add_reserved_memory()
Related show

Commit Message

Heiko Stuebner Oct. 8, 2019, 12:22 a.m. UTC
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@sntech.de>
---
This goes together with my bl32 work for the spl_atf loader in
https://patchwork.ozlabs.org/patch/1172565/

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

Comments

Simon Glass Oct. 22, 2019, 12:17 a.m. UTC | #1
Hi Heiko,

On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote:
>
> 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@sntech.de>
> ---
> This goes together with my bl32 work for the spl_atf loader in
> https://patchwork.ozlabs.org/patch/1172565/
>
>  common/image-fdt.c  |   8 ++++
>  include/tee/optee.h |   9 ++++
>  lib/optee/optee.c   | 112 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)

Could we please get a test for this new functionality?

Regards,
Simon
Jens Wiklander Oct. 22, 2019, 6:16 a.m. UTC | #2
Hi Heiko,

[+Igor]

On Tue, Oct 8, 2019 at 2:22 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> 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@sntech.de>
> ---
> This goes together with my bl32 work for the spl_atf loader in
> https://patchwork.ozlabs.org/patch/1172565/
>
>  common/image-fdt.c  |   8 ++++
>  include/tee/optee.h |   9 ++++
>  lib/optee/optee.c   | 112 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 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..f484b12e67 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,113 @@ error:
>
>         return ret;
>  }
> +
> +#if defined(CONFIG_OF_LIBFDT)
> +static int optee_add_firmware_node(void *fdt_blob)
> +{
> +       int offs, ret;
> +
> +       if (fdt_path_offset(fdt_blob, "/firmware/optee") >= 0) {
> +               debug("OP-TEE Device Tree node already exists");
> +               return 0;
> +       }
> +
> +       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;
> +
> +       ret = fdt_setprop_string(fdt_blob, offs, "compatible",
> +                                "linaro,optee-tz");
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = fdt_setprop_string(fdt_blob, offs, "method", "smc");
> +       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_add_firmware_node(new_blob);

I think it's more safe to copy the OP-TEE node instead. The values of
"compatible" and "method" may be different on other platforms, "hvc"
instead of "smc" for instance.

Thanks,
Jens


> +       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
>
Heiko Stuebner Oct. 22, 2019, 12:08 p.m. UTC | #3
Hi Simon,

Am Dienstag, 22. Oktober 2019, 02:17:00 CEST schrieb Simon Glass:
> On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > 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@sntech.de>
> > ---
> > This goes together with my bl32 work for the spl_atf loader in
> > https://patchwork.ozlabs.org/patch/1172565/
> >
> >  common/image-fdt.c  |   8 ++++
> >  include/tee/optee.h |   9 ++++
> >  lib/optee/optee.c   | 112 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 129 insertions(+)
> 
> Could we please get a test for this new functionality?

We can try ;-) , but I think I'll need some pointers how this should be done.

On first glance, test/overlay/* seems to be a good inspiration. Aka the new
function relies on an opaque OP-TEE binary modifying the .itb's dt-blob
between SPL and main-U-Boot, so working with stub devicetrees for
testing seems reasonable.

Aka checking cases of optee-nodes in old_fdt and not having them.

Looking at the README though points me to "TODO: convert to pytest" it
seems. Though that README.md only talks about interacting with the
u-boot console, so I'm not sure how that should work.


Thanks
Heiko
Heiko Stuebner Oct. 22, 2019, 8:02 p.m. UTC | #4
Hi Simon,

Am Dienstag, 22. Oktober 2019, 14:08:04 CEST schrieb Heiko Stübner:
> Am Dienstag, 22. Oktober 2019, 02:17:00 CEST schrieb Simon Glass:
> > On Mon, 7 Oct 2019 at 18:22, Heiko Stuebner <heiko@sntech.de> wrote:
> > >
> > > 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@sntech.de>
> > > ---
> > > This goes together with my bl32 work for the spl_atf loader in
> > > https://patchwork.ozlabs.org/patch/1172565/
> > >
> > >  common/image-fdt.c  |   8 ++++
> > >  include/tee/optee.h |   9 ++++
> > >  lib/optee/optee.c   | 112 ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 129 insertions(+)
> > 
> > Could we please get a test for this new functionality?
> 
> We can try ;-) , but I think I'll need some pointers how this should be done.
> 
> On first glance, test/overlay/* seems to be a good inspiration. Aka the new
> function relies on an opaque OP-TEE binary modifying the .itb's dt-blob
> between SPL and main-U-Boot, so working with stub devicetrees for
> testing seems reasonable.
> 
> Aka checking cases of optee-nodes in old_fdt and not having them.

I just did go ahead and implemented a test like that, see v2 series.
Is this what you had in mind?

Heiko

Patch
diff mbox series

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..f484b12e67 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,113 @@  error:
 
 	return ret;
 }
+
+#if defined(CONFIG_OF_LIBFDT)
+static int optee_add_firmware_node(void *fdt_blob)
+{
+	int offs, ret;
+
+	if (fdt_path_offset(fdt_blob, "/firmware/optee") >= 0) {
+		debug("OP-TEE Device Tree node already exists");
+		return 0;
+	}
+
+	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;
+
+	ret = fdt_setprop_string(fdt_blob, offs, "compatible",
+				 "linaro,optee-tz");
+	if (ret < 0)
+		return ret;
+
+	ret = fdt_setprop_string(fdt_blob, offs, "method", "smc");
+	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_add_firmware_node(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