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

[PATCH v3 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.

Changes since v2:
  - spl_load_fit_image: Use "if (fit_image_get_os())" -- without the "!"

Changes since v1:
  - Update commit message of "spl: LOAD_FIT_FULL: Fix selection of the "fdt" node"

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

[PATCH v3 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.

Use NULL for the 'fit_unamep' argument. With 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 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.3

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

[Tom Rini]

On Thu, Apr 01, 2021 at 01:25:26PM -0500, Alexandru Gagniuc 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()
> 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.
> 
> Use NULL for the 'fit_unamep' argument. With 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210416/104f909f/attachment.sig>

[PATCH v3 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..f3b4c3a036 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.3

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

[Tom Rini]

On Thu, Apr 01, 2021 at 01:25:27PM -0500, Alexandru Gagniuc 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210416/3ff231f5/attachment.sig>

[PATCH v3 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 f3b4c3a036..5a059c6dc5 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.3

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

[Tom Rini]

On Thu, Apr 01, 2021 at 01:25:28PM -0500, Alexandru Gagniuc 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).
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210416/d008bc9e/attachment.sig>

[PATCH v3 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 5a059c6dc5..4e028abf77 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.3

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

[Tom Rini]

On Thu, Apr 01, 2021 at 01:25:29PM -0500, Alexandru Gagniuc 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210416/8aa7170e/attachment.sig>

[PATCH v3 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-fit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index 94501b1071..970e3f89da 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.3

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

[Tom Rini]

On Thu, Apr 01, 2021 at 01:25:30PM -0500, Alexandru Gagniuc 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210416/58314a1f/attachment.sig>

[PATCH v3 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 common/image-fit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index 970e3f89da..ee51f5e76f 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.3

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

[Tom Rini]

On Thu, Apr 01, 2021 at 01:25:31PM -0500, Alexandru Gagniuc 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210416/8729a5ed/attachment.sig>