[PATCH 0/6] SPL: FIT: Fix some omissions of SPL_LOAD_FIT_FULL and bootm

[PATCH 0/6] SPL: FIT: Fix some omissions of SPL_LOAD_FIT_FULL and bootm

[Alexandru Gagniuc]

Although this series focuses on SPL_LOAD_FIT_FULL, some of the fixes will
also apply to bootm, as they are sharing significant amounts of code.

Originally SPL_LOAD_FIT_FULL could not start either a linux FIT or a
u-boot image. It didn't even take FIT images generated automatically
by mkimage, as part of the u-boot build!!! The goal is to at the very
least, be able to boot autogenerated mkimage FITs.

This brings us much more in line with SPL_LOAD_FIT, and the
documentation. It's not perfect,  and the fpga 'compatible =' support
is still not implemented. That's all I currently have time for before
someone notices I'm working on u-boot again.

Alexandru Gagniuc (6):
  spl: LOAD_FIT_FULL: Fix selection of the "fdt" node
  spl: LOAD_FIT_FULL: Do not hard-code os to IH_OS_U_BOOT
  spl: LOAD_FIT_FULL: Relocate FDT for u-boot payloads
  spl: LOAD_FIT_FULL: Support 'kernel' and 'firmware' properties
  image-fit: Accept IH_TYPE_FIRMWARE in fit_image_load() as valid
  image-fit: Accept OP-TEE images when booting a FIT

 common/image-fit.c |  4 ++++
 common/spl/spl.c   | 40 +++++++++++++++++++++++++++++++++-------
 2 files changed, 37 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/6] spl: LOAD_FIT_FULL: Fix selection of the "fdt" node

[Alexandru Gagniuc]

The correct FDT to use is described by the "fdt" property of the
configuration node. When the fit_unamep argument to fit_image_load()
is "fdt", we get the "/images/fdt" node. This is incorrect, as it
ignores the "fdt" property of the config node, and in most cases,
the "/images/fdt" node doesn't exist.

When the 'fit_unamep' argument is NULL, fit_image_load() uses the
IH_TYPE_FLATDT value to read the config property "fdt", which points
to the correct FDT node(s).

fit_image_load() should probably be split into a function that reads
an image by name, and one that reads an image by config reference. I
don't make those decisions, I just point out the craziness.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index e3d84082f4..986cfbc6fd 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -201,7 +201,6 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
 {
 	bootm_headers_t images;
 	const char *fit_uname_config = NULL;
-	const char *fit_uname_fdt = FIT_FDT_PROP;
 	const char *uname;
 	ulong fw_data = 0, dt_data = 0, img_data = 0;
 	ulong fw_len = 0, dt_len = 0, img_len = 0;
@@ -230,8 +229,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
 #ifdef CONFIG_SPL_FIT_SIGNATURE
 	images.verify = 1;
 #endif
-	ret = fit_image_load(&images, (ulong)header,
-		       &fit_uname_fdt, &fit_uname_config,
+	ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
 		       IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
 		       FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
 	if (ret >= 0)
-- 
2.26.2

[PATCH 2/6] spl: LOAD_FIT_FULL: Do not hard-code os to IH_OS_U_BOOT

[Alexandru Gagniuc]

The information on the OS should be contained in the FIT, as the
self-explanatory "os" property of a node under /images. Hard-coding
this to U_BOOT might send us down the wrong path later in the boot
process.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 986cfbc6fd..8f6c8dba6f 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -220,8 +220,9 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
 	spl_image->size = fw_len;
 	spl_image->entry_point = fw_data;
 	spl_image->load_addr = fw_data;
-	spl_image->os = IH_OS_U_BOOT;
-	spl_image->name = "U-Boot";
+	if (!fit_image_get_os(header, ret, &spl_image->os))
+		spl_image->os = IH_OS_INVALID;
+	spl_image->name = genimg_get_os_name(spl_image->os);
 
 	debug(SPL_TPL_PROMPT "payload image: %32s load addr: 0x%lx size: %d\n",
 	      spl_image->name, spl_image->load_addr, spl_image->size);
-- 
2.26.2

[PATCH 3/6] spl: LOAD_FIT_FULL: Relocate FDT for u-boot payloads

[Alexandru Gagniuc]

U-Boot expects the FDT to be located right after the _end
linker symbol (see fdtdec.c: board_fdt_blob_setup())

The "basic" LOAD_FIT path is aware of this limitation, and relocates
the FDT at the expected location. Guessing the expected location
probably only works reliably on 32-bit arm, and it feels like a hack.
One proposal would be to pass the FDT address to u-boot
(e.g. using 'r2' on arm platforms).

The variable is named "fdt_hack" to remind future contributors that,
"hey! we should fix the underlying problem". However, that is beyond
the scope of this patch.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 8f6c8dba6f..e63f05bb33 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -201,6 +201,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
 {
 	bootm_headers_t images;
 	const char *fit_uname_config = NULL;
+	uintptr_t fdt_hack;
 	const char *uname;
 	ulong fw_data = 0, dt_data = 0, img_data = 0;
 	ulong fw_len = 0, dt_len = 0, img_len = 0;
@@ -233,9 +234,18 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
 	ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
 		       IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
 		       FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
-	if (ret >= 0)
+	if (ret >= 0) {
 		spl_image->fdt_addr = (void *)dt_data;
 
+		if (spl_image->os == IH_OS_U_BOOT) {
+			/* HACK: U-boot expects FDT at a specific address */
+			fdt_hack = spl_image->load_addr + spl_image->size;
+			fdt_hack = (fdt_hack + 3) & ~3;
+			debug("Relocating FDT to %p\n", spl_image->fdt_addr);
+			memcpy((void *)fdt_hack, spl_image->fdt_addr, dt_len);
+		}
+	}
+
 	conf_noffset = fit_conf_get_node((const void *)header,
 					 fit_uname_config);
 	if (conf_noffset <= 0)
-- 
2.26.2

[PATCH 4/6] spl: LOAD_FIT_FULL: Support 'kernel' and 'firmware' properties

[Alexandru Gagniuc]

The 'firmware' property of a config node takes precedence over the
'kernel' property. 'standalone' is deprecated. However, give users a
couple of releases where 'standalone' still works, but warns loudly.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/spl/spl.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index e63f05bb33..da4751b4ac 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -214,7 +214,24 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
 	ret = fit_image_load(&images, (ulong)header,
 			     NULL, &fit_uname_config,
 			     IH_ARCH_DEFAULT, IH_TYPE_STANDALONE, -1,
-			     FIT_LOAD_REQUIRED, &fw_data, &fw_len);
+			     FIT_LOAD_OPTIONAL, &fw_data, &fw_len);
+	if (ret >= 0) {
+		printf("DEPRECATED: 'standalone = ' property.");
+		printf("Please use either 'firmware =' or 'kernel ='\n");
+	} else {
+		ret = fit_image_load(&images, (ulong)header, NULL,
+				     &fit_uname_config, IH_ARCH_DEFAULT,
+				     IH_TYPE_FIRMWARE, -1, FIT_LOAD_OPTIONAL,
+				     &fw_data, &fw_len);
+	}
+
+	if (ret < 0) {
+		ret = fit_image_load(&images, (ulong)header, NULL,
+				     &fit_uname_config, IH_ARCH_DEFAULT,
+				     IH_TYPE_KERNEL, -1, FIT_LOAD_OPTIONAL,
+				     &fw_data, &fw_len);
+	}
+
 	if (ret < 0)
 		return ret;
 
-- 
2.26.2

[PATCH 5/6] image-fit: Accept IH_TYPE_FIRMWARE in fit_image_load() as valid

[Alexandru Gagniuc]

Consider the following FIT:

	images {
		whipple {};
	};
	configurations {
		conf-1 {
			firmware = "whipple";
		};
	};

Getting the 'firmware' image with fit_image_load() is not possible, as
it doesn't understand 'firmware =' properties. Although one could pass
IH_TYPE_FIRMWARE for 'image_type', this needs to be converted to a
"firmware" string for FDT lookup -- exactly what this change does.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-fit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index 28b3d2b191..8cd1621a18 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1955,6 +1955,8 @@ static const char *fit_get_image_type_property(int type)
 		return FIT_FDT_PROP;
 	case IH_TYPE_KERNEL:
 		return FIT_KERNEL_PROP;
+	case IH_TYPE_FIRMWARE:
+		return FIT_FIRMWARE_PROP;
 	case IH_TYPE_RAMDISK:
 		return FIT_RAMDISK_PROP;
 	case IH_TYPE_X86_SETUP:
-- 
2.26.2

[PATCH 6/6] image-fit: Accept OP-TEE images when booting a FIT

[Alexandru Gagniuc]

OP-TEE images are normally packaged with
	type = "tee;
	os = "tee";

However, fit_image_load() thinks that is somehow invalid. however if
they were declared as type = "kernel", os = "linux", fit_image_load()
would happily accept them and allow the boot to continue. There is no
technical limitation to excluding "tee".

Allowing "tee" images is useful in a boot flow where OP-TEE is
executed before linux.

In fact, I think it's unintuitive for a "load"ing function to also do
parsing and contain a bunch ad-hoc heuristics that only its caller
might know. But I don't make the rules, I just write fixes. In more
polite terms: refactoring the fit_image API is beyond the scope of
this change.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-fit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index 8cd1621a18..31cb73a870 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -2089,6 +2089,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL);
 	type_ok = fit_image_check_type(fit, noffset, image_type) ||
 		  fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) ||
+		  fit_image_check_type(fit, noffset, IH_TYPE_TEE) ||
 		  (image_type == IH_TYPE_KERNEL &&
 		   fit_image_check_type(fit, noffset, IH_TYPE_KERNEL_NOLOAD));
 
@@ -2096,6 +2097,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 		image_type == IH_TYPE_FPGA ||
 		fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
 		fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
+		fit_image_check_os(fit, noffset, IH_OS_TEE) ||
 		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
 		fit_image_check_os(fit, noffset, IH_OS_EFI) ||
 		fit_image_check_os(fit, noffset, IH_OS_VXWORKS);
-- 
2.26.2

[PATCH 1/6] spl: LOAD_FIT_FULL: Fix selection of the "fdt" node

[Simon Glass]

Hi Alexandru,

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The correct FDT to use is described by the "fdt" property of the
> configuration node. When the fit_unamep argument to fit_image_load()

fit_unamep (and below

> is "fdt", we get the "/images/fdt" node. This is incorrect, as it
> ignores the "fdt" property of the config node, and in most cases,
> the "/images/fdt" node doesn't exist.
>
> When the 'fit_unamep' argument is NULL, fit_image_load() uses the
> IH_TYPE_FLATDT value to read the config property "fdt", which points
> to the correct FDT node(s).
>
> fit_image_load() should probably be split into a function that reads
> an image by name, and one that reads an image by config reference. I
> don't make those decisions, I just point out the craziness.

Somewhere you need to add a simple sentence explaining what your change does.

>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>

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


> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index e3d84082f4..986cfbc6fd 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -201,7 +201,6 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>  {
>         bootm_headers_t images;
>         const char *fit_uname_config = NULL;
> -       const char *fit_uname_fdt = FIT_FDT_PROP;
>         const char *uname;
>         ulong fw_data = 0, dt_data = 0, img_data = 0;
>         ulong fw_len = 0, dt_len = 0, img_len = 0;
> @@ -230,8 +229,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>  #ifdef CONFIG_SPL_FIT_SIGNATURE
>         images.verify = 1;
>  #endif
> -       ret = fit_image_load(&images, (ulong)header,
> -                      &fit_uname_fdt, &fit_uname_config,
> +       ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
>                        IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
>                        FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
>         if (ret >= 0)
> --
> 2.26.2
>

[PATCH 2/6] spl: LOAD_FIT_FULL: Do not hard-code os to IH_OS_U_BOOT

[Simon Glass]

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The information on the OS should be contained in the FIT, as the
> self-explanatory "os" property of a node under /images. Hard-coding
> this to U_BOOT might send us down the wrong path later in the boot
> process.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 986cfbc6fd..8f6c8dba6f 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -220,8 +220,9 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>         spl_image->size = fw_len;
>         spl_image->entry_point = fw_data;
>         spl_image->load_addr = fw_data;
> -       spl_image->os = IH_OS_U_BOOT;

Again I wonder about the code-size impact of this. Should it depend on Kconfig?

> -       spl_image->name = "U-Boot";
> +       if (!fit_image_get_os(header, ret, &spl_image->os))
> +               spl_image->os = IH_OS_INVALID;
> +       spl_image->name = genimg_get_os_name(spl_image->os);
>
>         debug(SPL_TPL_PROMPT "payload image: %32s load addr: 0x%lx size: %d\n",
>               spl_image->name, spl_image->load_addr, spl_image->size);
> --
> 2.26.2
>

Regards,
Simon

[PATCH 3/6] spl: LOAD_FIT_FULL: Relocate FDT for u-boot payloads

[Simon Glass]

Hi Alexandru,

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> U-Boot expects the FDT to be located right after the _end
> linker symbol (see fdtdec.c: board_fdt_blob_setup())
>
> The "basic" LOAD_FIT path is aware of this limitation, and relocates
> the FDT at the expected location. Guessing the expected location
> probably only works reliably on 32-bit arm, and it feels like a hack.
> One proposal would be to pass the FDT address to u-boot
> (e.g. using 'r2' on arm platforms).

We could do that, but the current behaviour is OK. It is required that
you can joined U-Boot and its FDT and get a valid image.

>
> The variable is named "fdt_hack" to remind future contributors that,
> "hey! we should fix the underlying problem". However, that is beyond
> the scope of this patch.

I think it is fine to require it be after U-Boot, in fact that is
defined by U-Boot. So I don't think you need to think of it as a hack.

>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 8f6c8dba6f..e63f05bb33 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -201,6 +201,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>  {
>         bootm_headers_t images;
>         const char *fit_uname_config = NULL;
> +       uintptr_t fdt_hack;
>         const char *uname;
>         ulong fw_data = 0, dt_data = 0, img_data = 0;
>         ulong fw_len = 0, dt_len = 0, img_len = 0;
> @@ -233,9 +234,18 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>         ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
>                        IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
>                        FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
> -       if (ret >= 0)
> +       if (ret >= 0) {
>                 spl_image->fdt_addr = (void *)dt_data;
>
> +               if (spl_image->os == IH_OS_U_BOOT) {
> +                       /* HACK: U-boot expects FDT at a specific address */
> +                       fdt_hack = spl_image->load_addr + spl_image->size;
> +                       fdt_hack = (fdt_hack + 3) & ~3;

I don't think this is needed and it just confuses things. If U-Boot is
not an aligned sign, it can't boot because the DT ends up in the wrong
place. The build system makes sure this doesn't happen.

> +                       debug("Relocating FDT to %p\n", spl_image->fdt_addr);
> +                       memcpy((void *)fdt_hack, spl_image->fdt_addr, dt_len);
> +               }
> +       }
> +
>         conf_noffset = fit_conf_get_node((const void *)header,
>                                          fit_uname_config);
>         if (conf_noffset <= 0)
> --
> 2.26.2
>

Regards,
Simon

[PATCH 4/6] spl: LOAD_FIT_FULL: Support 'kernel' and 'firmware' properties

[Simon Glass]

HI Alexandru,

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The 'firmware' property of a config node takes precedence over the
> 'kernel' property. 'standalone' is deprecated. However, give users a
> couple of releases where 'standalone' still works, but warns loudly.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/spl/spl.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index e63f05bb33..da4751b4ac 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -214,7 +214,24 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>         ret = fit_image_load(&images, (ulong)header,
>                              NULL, &fit_uname_config,
>                              IH_ARCH_DEFAULT, IH_TYPE_STANDALONE, -1,
> -                            FIT_LOAD_REQUIRED, &fw_data, &fw_len);
> +                            FIT_LOAD_OPTIONAL, &fw_data, &fw_len);
> +       if (ret >= 0) {
> +               printf("DEPRECATED: 'standalone = ' property.");
> +               printf("Please use either 'firmware =' or 'kernel ='\n");
> +       } else {
> +               ret = fit_image_load(&images, (ulong)header, NULL,
> +                                    &fit_uname_config, IH_ARCH_DEFAULT,
> +                                    IH_TYPE_FIRMWARE, -1, FIT_LOAD_OPTIONAL,
> +                                    &fw_data, &fw_len);
> +       }
> +
> +       if (ret < 0) {
> +               ret = fit_image_load(&images, (ulong)header, NULL,
> +                                    &fit_uname_config, IH_ARCH_DEFAULT,
> +                                    IH_TYPE_KERNEL, -1, FIT_LOAD_OPTIONAL,
> +                                    &fw_data, &fw_len);

Should this only be for Falcon mode?

> +       }
> +
>         if (ret < 0)
>                 return ret;
>
> --
> 2.26.2
>

Regards,
Simon

[PATCH 5/6] image-fit: Accept IH_TYPE_FIRMWARE in fit_image_load() as valid

[Simon Glass]

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Consider the following FIT:
>
>         images {
>                 whipple {};
>         };
>         configurations {
>                 conf-1 {
>                         firmware = "whipple";
>                 };
>         };
>
> Getting the 'firmware' image with fit_image_load() is not possible, as
> it doesn't understand 'firmware =' properties. Although one could pass
> IH_TYPE_FIRMWARE for 'image_type', this needs to be converted to a
> "firmware" string for FDT lookup -- exactly what this change does.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-fit.c | 2 ++
>  1 file changed, 2 insertions(+)

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


>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 28b3d2b191..8cd1621a18 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1955,6 +1955,8 @@ static const char *fit_get_image_type_property(int type)
>                 return FIT_FDT_PROP;
>         case IH_TYPE_KERNEL:
>                 return FIT_KERNEL_PROP;
> +       case IH_TYPE_FIRMWARE:
> +               return FIT_FIRMWARE_PROP;
>         case IH_TYPE_RAMDISK:
>                 return FIT_RAMDISK_PROP;
>         case IH_TYPE_X86_SETUP:
> --
> 2.26.2
>

[PATCH 6/6] image-fit: Accept OP-TEE images when booting a FIT

[Simon Glass]

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> OP-TEE images are normally packaged with
>         type = "tee;
>         os = "tee";
>
> However, fit_image_load() thinks that is somehow invalid. however if
> they were declared as type = "kernel", os = "linux", fit_image_load()
> would happily accept them and allow the boot to continue. There is no
> technical limitation to excluding "tee".
>
> Allowing "tee" images is useful in a boot flow where OP-TEE is
> executed before linux.
>
> In fact, I think it's unintuitive for a "load"ing function to also do
> parsing and contain a bunch ad-hoc heuristics that only its caller
> might know. But I don't make the rules, I just write fixes. In more
> polite terms: refactoring the fit_image API is beyond the scope of
> this change.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-fit.c | 2 ++
>  1 file changed, 2 insertions(+)

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

[PATCH 3/6] spl: LOAD_FIT_FULL: Relocate FDT for u-boot payloads

[Alex G.]

On 3/29/21 2:43 AM, Simon Glass wrote:
> Hi Alexandru,

>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index 8f6c8dba6f..e63f05bb33 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -201,6 +201,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>>   {
>>          bootm_headers_t images;
>>          const char *fit_uname_config = NULL;
>> +       uintptr_t fdt_hack;
>>          const char *uname;
>>          ulong fw_data = 0, dt_data = 0, img_data = 0;
>>          ulong fw_len = 0, dt_len = 0, img_len = 0;
>> @@ -233,9 +234,18 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>>          ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
>>                         IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
>>                         FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
>> -       if (ret >= 0)
>> +       if (ret >= 0) {
>>                  spl_image->fdt_addr = (void *)dt_data;
>>
>> +               if (spl_image->os == IH_OS_U_BOOT) {
>> +                       /* HACK: U-boot expects FDT at a specific address */
>> +                       fdt_hack = spl_image->load_addr + spl_image->size;
>> +                       fdt_hack = (fdt_hack + 3) & ~3;
> 
> I don't think this is needed and it just confuses things. If U-Boot is
> not an aligned sign, it can't boot because the DT ends up in the wrong
> place. The build system makes sure this doesn't happen.

The correct alignment for an FDT is 8 bytes. For a while, this alignment 
was implemented [1], and that worked fine with everything but u-boot.

Now to the build system argument. I don't think It's wise the assume the 
conventions of the SPL build and of the u-boot build are the same. Even 
assuming the branch building the SPL is perfect, the FIT image could 
have been built from a buggy u-boot branch, or even assembled manually 
outside the u-boot build.

Consequently, we can't assume the FIT image will have a spl_image->size 
of the correct alignment. Thus, aligning things by hand is quite necessary.

The problem with [1] is that it broke booting u-boot with FIT. It had to 
be reverted [2]. There was a lengthy email discussion about, where I 
included an example of the failure [3]. Now, wish the actual problem 
could be fixed, but I don't have the bandwidth. The best I can do is 
document the problem.

Alex

[1] 
https://source.denx.de/u-boot/u-boot/-/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76
[2] 
https://source.denx.de/u-boot/u-boot/-/commit/5675ed7cb645f5ec13958726992daeeed16fd114
[3] https://lists.denx.de/pipermail/u-boot/2020-October/430058.html

[PATCH 4/6] spl: LOAD_FIT_FULL: Support 'kernel' and 'firmware' properties

[Alex G.]

On 3/29/21 2:43 AM, Simon Glass wrote:
> HI Alexandru,
> 
> On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> The 'firmware' property of a config node takes precedence over the
>> 'kernel' property. 'standalone' is deprecated. However, give users a
>> couple of releases where 'standalone' still works, but warns loudly.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   common/spl/spl.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index e63f05bb33..da4751b4ac 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -214,7 +214,24 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>>          ret = fit_image_load(&images, (ulong)header,
>>                               NULL, &fit_uname_config,
>>                               IH_ARCH_DEFAULT, IH_TYPE_STANDALONE, -1,
>> -                            FIT_LOAD_REQUIRED, &fw_data, &fw_len);
>> +                            FIT_LOAD_OPTIONAL, &fw_data, &fw_len);
>> +       if (ret >= 0) {
>> +               printf("DEPRECATED: 'standalone = ' property.");
>> +               printf("Please use either 'firmware =' or 'kernel ='\n");
>> +       } else {
>> +               ret = fit_image_load(&images, (ulong)header, NULL,
>> +                                    &fit_uname_config, IH_ARCH_DEFAULT,
>> +                                    IH_TYPE_FIRMWARE, -1, FIT_LOAD_OPTIONAL,
>> +                                    &fw_data, &fw_len);
>> +       }
>> +
>> +       if (ret < 0) {
>> +               ret = fit_image_load(&images, (ulong)header, NULL,
>> +                                    &fit_uname_config, IH_ARCH_DEFAULT,
>> +                                    IH_TYPE_KERNEL, -1, FIT_LOAD_OPTIONAL,
>> +                                    &fw_data, &fw_len);
> 
> Should this only be for Falcon mode?

I don't know. Falcon mode relies on u-boot proper preparing an FDT and 
saving it to storage. Then the falcon boot reads the kernel image, and 
the "raw" FDT (see "spl export").

On the one hand, yes, because falcon mode loads kernels directly.
On the other hand, no, because we're not using this prepared FDT.

There's a distinction between falcon mode and kernel boot from FIT. And 
it's very muddy. I'm hoping that people will stop using falcon mode with 
legacy images. Then and only then, we won't have to make this distinction.

Alex

> 
>> +       }
>> +
>>          if (ret < 0)
>>                  return ret;
>>
>> --
>> 2.26.2
>>
> 
> Regards,
> Simon
> 

[PATCH 2/6] spl: LOAD_FIT_FULL: Do not hard-code os to IH_OS_U_BOOT

[Alex G.]

On 3/29/21 2:43 AM, Simon Glass wrote:
> On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> The information on the OS should be contained in the FIT, as the
>> self-explanatory "os" property of a node under /images. Hard-coding
>> this to U_BOOT might send us down the wrong path later in the boot
>> process.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   common/spl/spl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index 986cfbc6fd..8f6c8dba6f 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -220,8 +220,9 @@ static int spl_load_fit_image(struct spl_image_info *spl_image,
>>          spl_image->size = fw_len;
>>          spl_image->entry_point = fw_data;
>>          spl_image->load_addr = fw_data;
>> -       spl_image->os = IH_OS_U_BOOT;
> 

+8 bytes. Down in the noise

Alex

>> -       spl_image->name = "U-Boot";
>> +       if (!fit_image_get_os(header, ret, &spl_image->os))
>> +               spl_image->os = IH_OS_INVALID;
>> +       spl_image->name = genimg_get_os_name(spl_image->os);
>>
>>          debug(SPL_TPL_PROMPT "payload image: %32s load addr: 0x%lx size: %d\n",
>>                spl_image->name, spl_image->load_addr, spl_image->size);
>> --
>> 2.26.2
>>
> 
> Regards,
> Simon
> 

[PATCH 2/6] spl: LOAD_FIT_FULL: Do not hard-code os to IH_OS_U_BOOT

[Simon Glass]

On Tue, 30 Mar 2021 at 18:54, Alex G. <mr.nuke.me@gmail.com> wrote:
>
>
>
> On 3/29/21 2:43 AM, Simon Glass wrote:
> > On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> >>
> >> The information on the OS should be contained in the FIT, as the
> >> self-explanatory "os" property of a node under /images. Hard-coding
> >> this to U_BOOT might send us down the wrong path later in the boot
> >> process.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >>   common/spl/spl.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>

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