[PATCH v3 0/9] spl: Use common function for loading/parsing images

[PATCH v3 0/9] spl: Use common function for loading/parsing images

[Sean Anderson]

This series adds support for loading all image types (Legacy, FIT (with
and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
and EXT load methods. It does this by introducing a helper function
which handles the minutiae of invoking the proper parsing function, and
reading the rest of the image.

Hopefully, this will make it easier for load methods to support all
image types that U-Boot supports, without having undocumented
unsupported image types. I applied this to several loaders which were
invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
not use it with others (e.g. DFU/RAM) which had complications in the
mix.

Here's some bloat-o-meter for j7200_evm_a72_defconfig with ext4 support
enabled:

add/remove: 1/0 grow/shrink: 2/4 up/down: 224/-236 (-12)
Function                                     old     new   delta
spl_load                                       -     176    +176
spl_fit_read                                  60     104     +44
spl_load_image_ext                           364     368      +4
spl_nor_load_image                           120     108     -12
spl_spi_load_image                           280     228     -52
spl_load_image_fat                           320     264     -56
spl_mmc_load                                 716     600    -116
Total: Before=264556, After=264544, chg -0.00%

ext4 support is +48 bytes, because the original image support was so
bare-bones (just legacy/raw images). For most boards with a few load
methods (where one of them isn't ext4), this series should be no bloat
or a net negative. However, in the worst case this series will add
150-180 bytes.

I have only tested EXT and MMC raw loaders. Please try booting your
favorite board with NOR/SPI flash or SPI falcon mode.

Changes in v3:
- Fix using ffs instead of fls
- Fix using not initializing bl_len when info->filename was NULL
- Fix failing on success

Changes in v2:
- Use reverse-xmas-tree style for locals in spl_simple_read. This is not
  complete, since overhead depends on bl_mask.
- Convert semihosting as well
- Consolidate spi_load_image_os into spl_spi_load_image

Sean Anderson (9):
  spl: Add generic spl_load function
  spl: Convert ext to use spl_load
  spl: Convert fat to spl_load
  spl: Convert mmc to spl_load
  spl: Convert net to spl_load
  spl: Convert nor to spl_load
  spl: Convert semihosting to spl_load
  spl: Convert spi to spl_load
  spl: spi: Consolidate spi_load_image_os into spl_spi_load_image

 common/spl/spl.c             |  68 ++++++++++++++++++
 common/spl/spl_ext.c         |  24 +++++--
 common/spl/spl_fat.c         |  40 +++--------
 common/spl/spl_mmc.c         |  73 ++-----------------
 common/spl/spl_net.c         |  24 ++-----
 common/spl/spl_nor.c         |  35 ++--------
 common/spl/spl_semihosting.c |  39 +++++------
 common/spl/spl_spi.c         | 131 ++++++++++-------------------------
 include/spl.h                |  30 +++++++-
 9 files changed, 193 insertions(+), 271 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 1/9] spl: Add generic spl_load function

[Sean Anderson]

Implementers of SPL_LOAD_IMAGE_METHOD have to correctly determine what
type of image is being loaded and then call the appropriate image load
function correctly. This is tricky, because some image load functions
expect the whole image to already be loaded (CONFIG_SPL_LOAD_FIT_FULL),
some will load the image automatically using spl_load_info.read()
(CONFIG_SPL_LOAD_FIT/CONFIG_SPL_LOAD_IMX_CONTAINER), and some just parse
the header and expect the caller to do the actual loading afterwards
(legacy/raw images). Load methods often only support a subset of the
above methods, meaning that not all image types can be used with all
load methods. Further, the code to invoke these functions is
duplicated between different load functions.

To address this problem, this commit introduces a "spl_load" function.
It aims to handle image detection and correct invocation of each of the
parse/load functions. spl_simple_read is a wrapper around
spl_load_info.read with get_aligned_image* functions inlined for size
purposes. Additionally, we assume that bl_len is a power of 2 so we can
do bitshifts instead of divisions (which is smaller and faster).

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

Changes in v3:
- Fix using ffs instead of fls
- Fix using not initializing bl_len when info->filename was NULL

Changes in v2:
- Use reverse-xmas-tree style for locals in spl_simple_read. This is not
  complete, since overhead depends on bl_mask.

 common/spl/spl.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/spl.h    | 30 ++++++++++++++++++++-
 2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index c9750ee163..f9a1cfc71e 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -399,6 +399,74 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 	return 0;
 }
 
+static int spl_simple_read(struct spl_load_info *info, void *buf, size_t size,
+			   size_t offset)
+{
+	size_t bl_len = info->filename ? ARCH_DMA_MINALIGN : info->bl_len;
+	size_t bl_mask = bl_len - 1;
+	size_t overhead = offset & bl_mask;
+	size_t bl_shift = fls(bl_mask);
+	int ret;
+
+	debug("%s: buf=%p size=%lx offset=%lx\n", __func__, buf, (long)size,
+	      (long)offset);
+	debug("%s: bl_len=%lx bl_mask=%lx bl_shift=%lx\n", __func__, bl_len,
+	      bl_mask, bl_shift);
+
+	buf -= overhead;
+	size = (size + overhead + bl_mask) >> bl_shift;
+	offset = offset >> bl_shift;
+
+	debug("info->read(info, %lx, %lx, %p)\n", (ulong)offset, (ulong)size,
+	      buf);
+	ret = info->read(info, offset, size, buf);
+	return ret == size ? 0 : -EIO;
+}
+
+int spl_load(struct spl_image_info *spl_image,
+	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
+	     struct image_header *header, size_t size, size_t sector)
+{
+	int ret;
+	size_t offset = sector * info->bl_len;
+
+	if (image_get_magic(header) == FDT_MAGIC) {
+		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
+			void *buf;
+
+			/*
+			 * In order to support verifying images in the FIT, we
+			 * need to load the whole FIT into memory. Try and
+			 * guess how much we need to load by using the total
+			 * size. This will fail for FITs with external data,
+			 * but there's not much we can do about that.
+			 */
+			if (!size)
+				size = roundup(fdt_totalsize(header), 4);
+			buf = spl_get_load_buffer(0, size);
+			ret = spl_simple_read(info, buf, size, offset);
+			if (ret)
+				return ret;
+
+			return spl_parse_image_header(spl_image, bootdev, buf);
+		}
+
+		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
+			return spl_load_simple_fit(spl_image, info, sector,
+						   header);
+	}
+
+	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
+		return spl_load_imx_container(spl_image, info, sector);
+
+	ret = spl_parse_image_header(spl_image, bootdev, header);
+	if (ret)
+		return ret;
+
+	return spl_simple_read(info, (void *)spl_image->load_addr,
+			       spl_image->size, offset + spl_image->offset);
+}
+
 __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
 	typedef void __noreturn (*image_entry_noargs_t)(void);
diff --git a/include/spl.h b/include/spl.h
index 6134aba857..025fffb895 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -237,7 +237,7 @@ struct spl_image_info {
  *
  * @dev: Pointer to the device, e.g. struct mmc *
  * @priv: Private data for the device
- * @bl_len: Block length for reading in bytes
+ * @bl_len: Block length for reading in bytes; must be a power of 2
  * @filename: Name of the fit image file.
  * @read: Function to call to read from the device
  */
@@ -609,6 +609,34 @@ int spl_load_image_ext_os(struct spl_image_info *spl_image,
 			  struct spl_boot_device *bootdev,
 			  struct blk_desc *block_dev, int partition);
 
+/**
+ * spl_load() - Parse a header and load the image
+ * @spl_image: Image data which will be filled in by this function
+ * @bootdev: The device to load from
+ * @info: Describes how to load additional information from @bootdev. At the
+ *        minimum, read() and bl_len must be populated.
+ * @header: The image header. This should already have been loaded. It may be
+ *          clobbered by the load process (if e.g. the load address overlaps).
+ * @size: The size of the image, if it is known in advance. Some boot devices
+ *        (such as filesystems) know how big an image is before parsing the
+ *        header. If this information is unknown, then the size will be
+ *        determined from the header.
+ * @sectors: The offset from the start if @bootdev, in units of @info->bl_len.
+ *           This should have the offset @header was loaded from. It will be
+ *           added to any offsets passed to @info->read().
+ *
+ * This function determines the image type (FIT, legacy, i.MX, raw, etc), calls
+ * the appropriate parsing function, determines the load address, and the loads
+ * the image from storage. It is designed to replace ad-hoc image loading which
+ * may not support all image types (especially when config options are
+ * involved).
+ *
+ * Return: 0 on success, or a negative error on failure
+ */
+int spl_load(struct spl_image_info *spl_image,
+	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
+	     struct image_header *header, size_t size, size_t sector);
+
 /**
  * spl_early_init() - Set up device tree and driver model in SPL if enabled
  *
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 2/9] spl: Convert ext to use spl_load

[Sean Anderson]

This converts the ext load method to use spl_load. As a consequence, it
also adds support for FIT and IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

(no changes since v1)

 common/spl/spl_ext.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
index ebd914c492..1384842776 100644
--- a/common/spl/spl_ext.c
+++ b/common/spl/spl_ext.c
@@ -9,6 +9,18 @@
 #include <errno.h>
 #include <image.h>
 
+static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
+			  ulong size, void *buf)
+{
+	int ret;
+	loff_t actlen;
+
+	ret = ext4fs_read(buf, file_offset, size, &actlen);
+	if (ret)
+		return ret;
+	return actlen;
+}
+
 int spl_load_image_ext(struct spl_image_info *spl_image,
 		       struct spl_boot_device *bootdev,
 		       struct blk_desc *block_dev, int partition,
@@ -18,6 +30,10 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
 	struct image_header *header;
 	loff_t filelen, actlen;
 	struct disk_partition part_info = {};
+	struct spl_load_info load = {
+		.read = spl_fit_read,
+		.bl_len = 1,
+	};
 
 	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
 
@@ -47,13 +63,7 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
 		goto end;
 	}
 
-	err = spl_parse_image_header(spl_image, bootdev, header);
-	if (err < 0) {
-		puts("spl: ext: failed to parse image header\n");
-		goto end;
-	}
-
-	err = ext4fs_read((char *)spl_image->load_addr, 0, filelen, &actlen);
+	err = spl_load(spl_image, bootdev, &load, header, filelen, 0);
 
 end:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 3/9] spl: Convert fat to spl_load

[Sean Anderson]

This converts the fat loader to use spl_load.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

Changes in v3:
- Fix failing on success

 common/spl/spl_fat.c | 40 ++++++++--------------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
index 5b270541fc..f25f1a19ac 100644
--- a/common/spl/spl_fat.c
+++ b/common/spl/spl_fat.c
@@ -61,6 +61,11 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
 {
 	int err;
 	struct image_header *header;
+	struct spl_load_info load = {
+		.read = spl_fit_read,
+		.bl_len = 1,
+		.filename = filename,
+	};
 
 	err = spl_register_fat_device(block_dev, partition);
 	if (err)
@@ -72,45 +77,16 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
 	if (err <= 0)
 		goto end;
 
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		err = file_fat_read(filename, (void *)CONFIG_SYS_LOAD_ADDR, 0);
-		if (err <= 0)
-			goto end;
-		err = spl_parse_image_header(spl_image, bootdev,
-				(struct image_header *)CONFIG_SYS_LOAD_ADDR);
-		if (err == -EAGAIN)
-			return err;
-		if (err == 0)
-			err = 1;
-	} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.read = spl_fit_read;
-		load.bl_len = 1;
-		load.filename = (void *)filename;
-		load.priv = NULL;
-
-		return spl_load_simple_fit(spl_image, &load, 0, header);
-	} else {
-		err = spl_parse_image_header(spl_image, bootdev, header);
-		if (err)
-			goto end;
-
-		err = file_fat_read(filename,
-				    (u8 *)(uintptr_t)spl_image->load_addr, 0);
-	}
+	err = spl_load(spl_image, bootdev, &load, header, err, 0);
 
 end:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-	if (err <= 0)
+	if (err < 0)
 		printf("%s: error reading image %s, err - %d\n",
 		       __func__, filename, err);
 #endif
 
-	return (err <= 0);
+	return err;
 }
 
 #if CONFIG_IS_ENABLED(OS_BOOT)
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 4/9] spl: Convert mmc to spl_load

[Sean Anderson]

This converts the mmc loader to spl_load. Legacy images are handled by
spl_load (via spl_parse_image_header), so mmc_load_legacy can be
omitted.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

(no changes since v1)

 common/spl/spl_mmc.c | 73 ++++----------------------------------------
 1 file changed, 6 insertions(+), 67 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 6116a68371..93a28cdaa9 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -17,48 +17,6 @@
 #include <mmc.h>
 #include <image.h>
 
-static int mmc_load_legacy(struct spl_image_info *spl_image,
-			   struct spl_boot_device *bootdev,
-			   struct mmc *mmc,
-			   ulong sector, struct image_header *header)
-{
-	u32 image_offset_sectors;
-	u32 image_size_sectors;
-	unsigned long count;
-	u32 image_offset;
-	int ret;
-
-	ret = spl_parse_image_header(spl_image, bootdev, header);
-	if (ret)
-		return ret;
-
-	/* convert offset to sectors - round down */
-	image_offset_sectors = spl_image->offset / mmc->read_bl_len;
-	/* calculate remaining offset */
-	image_offset = spl_image->offset % mmc->read_bl_len;
-
-	/* convert size to sectors - round up */
-	image_size_sectors = (spl_image->size + mmc->read_bl_len - 1) /
-			     mmc->read_bl_len;
-
-	/* Read the header too to avoid extra memcpy */
-	count = blk_dread(mmc_get_blk_desc(mmc),
-			  sector + image_offset_sectors,
-			  image_size_sectors,
-			  (void *)(ulong)spl_image->load_addr);
-	debug("read %x sectors to %lx\n", image_size_sectors,
-	      spl_image->load_addr);
-	if (count != image_size_sectors)
-		return -EIO;
-
-	if (image_offset)
-		memmove((void *)(ulong)spl_image->load_addr,
-			(void *)(ulong)spl_image->load_addr + image_offset,
-			spl_image->size);
-
-	return 0;
-}
-
 static ulong h_spl_load_read(struct spl_load_info *load, ulong sector,
 			     ulong count, void *buf)
 {
@@ -86,6 +44,11 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
 	struct image_header *header;
 	struct blk_desc *bd = mmc_get_blk_desc(mmc);
 	int ret = 0;
+	struct spl_load_info load = {
+		.dev = mmc,
+		.bl_len = mmc->read_bl_len,
+		.read = h_spl_load_read,
+	};
 
 	header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
 
@@ -97,31 +60,7 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
 		goto end;
 	}
 
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.dev = mmc;
-		load.priv = NULL;
-		load.filename = NULL;
-		load.bl_len = mmc->read_bl_len;
-		load.read = h_spl_load_read;
-		ret = spl_load_simple_fit(spl_image, &load, sector, header);
-	} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
-		struct spl_load_info load;
-
-		load.dev = mmc;
-		load.priv = NULL;
-		load.filename = NULL;
-		load.bl_len = mmc->read_bl_len;
-		load.read = h_spl_load_read;
-
-		ret = spl_load_imx_container(spl_image, &load, sector);
-	} else {
-		ret = mmc_load_legacy(spl_image, bootdev, mmc, sector, header);
-	}
-
+	ret = spl_load(spl_image, bootdev, &load, header, 0, sector);
 end:
 	if (ret) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 5/9] spl: Convert net to spl_load

[Sean Anderson]

This converts the net load method to use spl_load. As a result, it also
adds support for LOAD_FIT_FULL and IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

(no changes since v1)

 common/spl/spl_net.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/common/spl/spl_net.c b/common/spl/spl_net.c
index a853e6aead..3b4374add6 100644
--- a/common/spl/spl_net.c
+++ b/common/spl/spl_net.c
@@ -29,6 +29,10 @@ static int spl_net_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
 	struct image_header *header = (struct image_header *)image_load_addr;
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_net_load_read,
+	};
 	int rv;
 
 	env_init();
@@ -47,25 +51,7 @@ static int spl_net_load_image(struct spl_image_info *spl_image,
 		return rv;
 	}
 
-	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-	    image_get_magic(header) == FDT_MAGIC) {
-		struct spl_load_info load;
-
-		debug("Found FIT\n");
-		load.bl_len = 1;
-		load.read = spl_net_load_read;
-		rv = spl_load_simple_fit(spl_image, &load, 0, header);
-	} else {
-		debug("Legacy image\n");
-
-		rv = spl_parse_image_header(spl_image, bootdev, header);
-		if (rv)
-			return rv;
-
-		memcpy((void *)spl_image->load_addr, header, spl_image->size);
-	}
-
-	return rv;
+	return spl_load(spl_image, bootdev, &load, header, 0, 0);
 }
 #endif
 
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 6/9] spl: Convert nor to spl_load

[Sean Anderson]

This converts the nor load method to use spl_load. As a result it also
adds support for LOAD_FIT_FULL.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

(no changes since v1)

 common/spl/spl_nor.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
index 0f4fff8493..90ece77af1 100644
--- a/common/spl/spl_nor.c
+++ b/common/spl/spl_nor.c
@@ -26,8 +26,11 @@ unsigned long __weak spl_nor_get_uboot_base(void)
 static int spl_nor_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
-	__maybe_unused const struct image_header *header;
-	__maybe_unused struct spl_load_info load;
+	struct image_header *header = (void *)spl_nor_get_uboot_base();
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_nor_load_read,
+	};
 
 	/*
 	 * Loading of the payload to SDRAM is done with skipping of
@@ -91,32 +94,6 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
 	 * Load real U-Boot from its location in NOR flash to its
 	 * defined location in SDRAM
 	 */
-#ifdef CONFIG_SPL_LOAD_FIT
-	header = (const struct image_header *)spl_nor_get_uboot_base();
-	if (image_get_magic(header) == FDT_MAGIC) {
-		debug("Found FIT format U-Boot\n");
-		load.bl_len = 1;
-		load.read = spl_nor_load_read;
-		return spl_load_simple_fit(spl_image, &load,
-					   spl_nor_get_uboot_base(),
-					   (void *)header);
-	}
-#endif
-	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
-		load.bl_len = 1;
-		load.read = spl_nor_load_read;
-		return spl_load_imx_container(spl_image, &load,
-					      spl_nor_get_uboot_base());
-	}
-
-	/* Legacy image handling */
-	if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_SUPPORT)) {
-		load.bl_len = 1;
-		load.read = spl_nor_load_read;
-		return spl_load_legacy_img(spl_image, bootdev, &load,
-					   spl_nor_get_uboot_base());
-	}
-
-	return 0;
+	return spl_load(spl_image, bootdev, &load, header, 0, 0);
 }
 SPL_LOAD_IMAGE_METHOD("NOR", 0, BOOT_DEVICE_NOR, spl_nor_load_image);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 7/9] spl: Convert semihosting to spl_load

[Sean Anderson]

This converts the semihosting load method to use spl_load. As a result, it
also adds support for LOAD_FIT_FULL and IMX images.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v2)

Changes in v2:
- New

 common/spl/spl_semihosting.c | 39 +++++++++++++++---------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
index df6aeb2951..35fbc2ee5e 100644
--- a/common/spl/spl_semihosting.c
+++ b/common/spl/spl_semihosting.c
@@ -9,16 +9,16 @@
 #include <semihosting.h>
 #include <spl.h>
 
-static int smh_read_full(long fd, void *memp, size_t len)
+static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
+			      ulong count, void *buf)
 {
-	long read;
+	int ret, fd = *(int *)load->priv;
 
-	read = smh_read(fd, memp, len);
-	if (read < 0)
-		return read;
-	if (read != len)
-		return -EIO;
-	return 0;
+	if (smh_seek(fd, sector))
+		return 0;
+
+	ret = smh_read(fd, buf, count);
+	return ret < 0 ? 0 : ret;
 }
 
 static int spl_smh_load_image(struct spl_image_info *spl_image,
@@ -29,12 +29,17 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
 	long fd, len;
 	struct image_header *header =
 		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_smh_fit_read,
+	};
 
 	fd = smh_open(filename, MODE_READ | MODE_BINARY);
 	if (fd < 0) {
 		log_debug("could not open %s: %ld\n", filename, fd);
 		return fd;
 	}
+	load.priv = &fd;
 
 	ret = smh_flen(fd);
 	if (ret < 0) {
@@ -43,25 +48,13 @@ static int spl_smh_load_image(struct spl_image_info *spl_image,
 	}
 	len = ret;
 
-	ret = smh_read_full(fd, header, sizeof(struct image_header));
-	if (ret) {
+	ret = smh_read(fd, header, sizeof(struct image_header));
+	if (ret != sizeof(struct image_header)) {
 		log_debug("could not read image header: %d\n", ret);
 		goto out;
 	}
 
-	ret = spl_parse_image_header(spl_image, bootdev, header);
-	if (ret) {
-		log_debug("failed to parse image header: %d\n", ret);
-		goto out;
-	}
-
-	ret = smh_seek(fd, 0);
-	if (ret) {
-		log_debug("could not seek to start of image: %d\n", ret);
-		goto out;
-	}
-
-	ret = smh_read_full(fd, (void *)spl_image->load_addr, len);
+	ret = spl_load(spl_image, bootdev, &load, header, len, 0);
 	if (ret)
 		log_debug("could not read %s: %d\n", filename, ret);
 out:
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 8/9] spl: Convert spi to spl_load

[Sean Anderson]

This converts the spi load method to use spl_load. As a consequence, it
also adds support for LOAD_FIT_FULL.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
Reviewed-by: Stefan Roese <sr@denx.de>
---

(no changes since v1)

 common/spl/spl_spi.c | 48 +++++++-------------------------------------
 1 file changed, 7 insertions(+), 41 deletions(-)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index cf3f7ef4c0..037db1a19f 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -83,6 +83,10 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 	unsigned int payload_offs;
 	struct spi_flash *flash;
 	struct image_header *header;
+	struct spl_load_info load = {
+		.bl_len = 1,
+		.read = spl_spi_fit_read,
+	};
 
 	/*
 	 * Load U-Boot image from SPI flash into RAM
@@ -99,6 +103,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 		return -ENODEV;
 	}
 
+	load.dev = flash;
 	payload_offs = spl_spi_get_uboot_offs(flash);
 
 	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
@@ -121,47 +126,8 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 			return err;
 		}
 
-		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
-		    image_get_magic(header) == FDT_MAGIC) {
-			err = spi_flash_read(flash, payload_offs,
-					     roundup(fdt_totalsize(header), 4),
-					     (void *)CONFIG_SYS_LOAD_ADDR);
-			if (err)
-				return err;
-			err = spl_parse_image_header(spl_image, bootdev,
-					(struct image_header *)CONFIG_SYS_LOAD_ADDR);
-		} else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
-			   image_get_magic(header) == FDT_MAGIC) {
-			struct spl_load_info load;
-
-			debug("Found FIT\n");
-			load.dev = flash;
-			load.priv = NULL;
-			load.filename = NULL;
-			load.bl_len = 1;
-			load.read = spl_spi_fit_read;
-			err = spl_load_simple_fit(spl_image, &load,
-						  payload_offs,
-						  header);
-		} else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
-			struct spl_load_info load;
-
-			load.dev = flash;
-			load.priv = NULL;
-			load.filename = NULL;
-			load.bl_len = 1;
-			load.read = spl_spi_fit_read;
-
-			err = spl_load_imx_container(spl_image, &load,
-						     payload_offs);
-		} else {
-			err = spl_parse_image_header(spl_image, bootdev, header);
-			if (err)
-				return err;
-			err = spi_flash_read(flash, payload_offs + spl_image->offset,
-					     spl_image->size,
-					     (void *)spl_image->load_addr);
-		}
+		err = spl_load(spl_image, bootdev, &load, header, 0,
+			       payload_offs);
 	}
 
 	return err;
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 9/9] spl: spi: Consolidate spi_load_image_os into spl_spi_load_image

[Sean Anderson]

spi_load_image_os performs almost the same steps as the non-falcon-boot
path of spl_spi_load_image. The load address is different, and it also
loads a device tree, but that's it. Refactor the boot process so that
they can both use the same load function.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v2)

Changes in v2:
- New

 common/spl/spl_spi.c | 87 +++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 54 deletions(-)

diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index 037db1a19f..e724a74783 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -18,41 +18,6 @@
 #include <asm/global_data.h>
 #include <dm/ofnode.h>
 
-#if CONFIG_IS_ENABLED(OS_BOOT)
-/*
- * Load the kernel, check for a valid header we can parse, and if found load
- * the kernel and then device tree.
- */
-static int spi_load_image_os(struct spl_image_info *spl_image,
-			     struct spl_boot_device *bootdev,
-			     struct spi_flash *flash,
-			     struct image_header *header)
-{
-	int err;
-
-	/* Read for a header, parse or error out. */
-	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS, sizeof(*header),
-		       (void *)header);
-
-	if (image_get_magic(header) != IH_MAGIC)
-		return -1;
-
-	err = spl_parse_image_header(spl_image, bootdev, header);
-	if (err)
-		return err;
-
-	spi_flash_read(flash, CONFIG_SYS_SPI_KERNEL_OFFS,
-		       spl_image->size, (void *)spl_image->load_addr);
-
-	/* Read device tree. */
-	spi_flash_read(flash, CONFIG_SYS_SPI_ARGS_OFFS,
-		       CONFIG_SYS_SPI_ARGS_SIZE,
-		       (void *)CONFIG_SYS_SPL_ARGS_ADDR);
-
-	return 0;
-}
-#endif
-
 static ulong spl_spi_fit_read(struct spl_load_info *load, ulong sector,
 			      ulong count, void *buf)
 {
@@ -71,6 +36,29 @@ unsigned int __weak spl_spi_get_uboot_offs(struct spi_flash *flash)
 	return CONFIG_SYS_SPI_U_BOOT_OFFS;
 }
 
+static int spi_do_load_image(struct spl_image_info *spl_image,
+			     struct spl_boot_device *bootdev,
+			     struct spl_load_info *load,
+			     unsigned int payload_offs)
+{
+	int ret;
+	struct spi_flash *flash = load->dev;
+	struct image_header *header =
+		spl_get_load_buffer(-sizeof(*header), sizeof(*header));
+
+	/* mkimage header is 64 bytes. */
+	ret = spi_flash_read(flash, payload_offs, sizeof(*header),
+			     (void *)header);
+	if (ret) {
+		debug("%s: Failed to read from SPI flash (err=%d)\n",
+		      __func__, ret);
+		return ret;
+	}
+
+	return spl_load(spl_image, bootdev, load, header, 0,
+			payload_offs);
+}
+
 /*
  * The main entry for SPI booting. It's necessary that SDRAM is already
  * configured and available since this code loads the main U-Boot image
@@ -79,10 +67,8 @@ unsigned int __weak spl_spi_get_uboot_offs(struct spi_flash *flash)
 static int spl_spi_load_image(struct spl_image_info *spl_image,
 			      struct spl_boot_device *bootdev)
 {
-	int err = 0;
 	unsigned int payload_offs;
 	struct spi_flash *flash;
-	struct image_header *header;
 	struct spl_load_info load = {
 		.bl_len = 1,
 		.read = spl_spi_fit_read,
@@ -106,31 +92,24 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
 	load.dev = flash;
 	payload_offs = spl_spi_get_uboot_offs(flash);
 
-	header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
-
 	if (CONFIG_IS_ENABLED(OF_REAL)) {
 		payload_offs = ofnode_conf_read_int("u-boot,spl-payload-offset",
 						    payload_offs);
 	}
 
 #if CONFIG_IS_ENABLED(OS_BOOT)
-	if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev, flash, header))
-#endif
-	{
-		/* Load u-boot, mkimage header is 64 bytes. */
-		err = spi_flash_read(flash, payload_offs, sizeof(*header),
-				     (void *)header);
-		if (err) {
-			debug("%s: Failed to read from SPI flash (err=%d)\n",
-			      __func__, err);
-			return err;
-		}
-
-		err = spl_load(spl_image, bootdev, &load, header, 0,
-			       payload_offs);
+	if (spl_start_uboot()) {
+		int err = spi_do_load_image(spl_image, bootdev, &load,
+					    CONFIG_SYS_SPI_KERNEL_OFFS);
+		if (!err)
+			/* Read device tree. */
+			return spi_flash_read(flash, CONFIG_SYS_SPI_ARGS_OFFS,
+					      CONFIG_SYS_SPI_ARGS_SIZE,
+					      (void *)CONFIG_SYS_SPL_ARGS_ADDR);
 	}
+#endif
 
-	return err;
+	return spi_do_load_image(spl_image, bootdev, &load, payload_offs);
 }
 /* Use priorty 1 so that boards can override this */
 SPL_LOAD_IMAGE_METHOD("SPI", 1, BOOT_DEVICE_SPI, spl_spi_load_image);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v3 3/9] spl: Convert fat to spl_load

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

On Thu, May 05, 2022 at 04:16:49PM -0400, Sean Anderson wrote:

> This converts the fat loader to use spl_load.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Stefan Roese <sr@denx.de>

On omap2plus and rpi:
Tested-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 4/9] spl: Convert mmc to spl_load

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Thu, May 05, 2022 at 04:16:50PM -0400, Sean Anderson wrote:

> This converts the mmc loader to spl_load. Legacy images are handled by
> spl_load (via spl_parse_image_header), so mmc_load_legacy can be
> omitted.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Stefan Roese <sr@denx.de>

On mx6cuboxi, pine_a64 and libretech-cc:
Tested-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 8/9] spl: Convert spi to spl_load

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Thu, May 05, 2022 at 04:16:54PM -0400, Sean Anderson wrote:
> This converts the spi load method to use spl_load. As a consequence, it

> also adds support for LOAD_FIT_FULL.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Stefan Roese <sr@denx.de>

I had hoped to test this, but I can't convince the platform I was going
to use to boot from SPI period (it required some odd combination of DIP
switches I can't replicate anymore), so I couldn't test this afterall.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 5/9] spl: Convert net to spl_load

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Thu, May 05, 2022 at 04:16:51PM -0400, Sean Anderson wrote:

> This converts the net load method to use spl_load. As a result, it also
> adds support for LOAD_FIT_FULL and IMX images.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Stefan Roese <sr@denx.de>

Given my previous hardware problems testing SPI and that for the
platform I have this requires a different set of fun switches (and I
_think_ technically wasn't supported, in the end on the EVM, only some
production designs) I haven't tested this either.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 8/9] spl: Convert spi to spl_load

[Sean Anderson]

On 5/6/22 12:18 PM, Tom Rini wrote:
> On Thu, May 05, 2022 at 04:16:54PM -0400, Sean Anderson wrote:
>> This converts the spi load method to use spl_load. As a consequence, it
> 
>> also adds support for LOAD_FIT_FULL.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> I had hoped to test this, but I can't convince the platform I was going
> to use to boot from SPI period (it required some odd combination of DIP
> switches I can't replicate anymore), so I couldn't test this afterall.
> 

[...]

> On Thu, May 05, 2022 at 04:16:51PM -0400, Sean Anderson wrote:
> 
>> This converts the net load method to use spl_load. As a result, it also
>> adds support for LOAD_FIT_FULL and IMX images.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Given my previous hardware problems testing SPI and that for the
> platform I have this requires a different set of fun switches (and I
> _think_ technically wasn't supported, in the end on the EVM, only some
> production designs) I haven't tested this either.

+CC some people who've either authored a spi/net patch recently or have tested one.

Re: [PATCH v3 0/9] spl: Use common function for loading/parsing images

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Thu, May 05, 2022 at 04:16:46PM -0400, Sean Anderson wrote:

> This series adds support for loading all image types (Legacy, FIT (with
> and without LOAD_FIT_FULL), and i.MX) to the MMC, SPI, NOR, NET, FAT,
> and EXT load methods. It does this by introducing a helper function
> which handles the minutiae of invoking the proper parsing function, and
> reading the rest of the image.
> 
> Hopefully, this will make it easier for load methods to support all
> image types that U-Boot supports, without having undocumented
> unsupported image types. I applied this to several loaders which were
> invoking spl_load_simple_fit and/or spl_parse_image_header, but I did
> not use it with others (e.g. DFU/RAM) which had complications in the
> mix.

This needs to be re-based, sorry.  I manually updated 6/9 but now my
mx6cuboxi fails to MMC boot and it's more likely a merge error than a
new problem, since we iterated over this before I think.  Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 1/9] spl: Add generic spl_load function

[Xavier Drudis Ferran]

Hello. 

Thank you for your work, simplifying and generalizing code, 
and sorry that I hadn't seen this series before.

I'm new to U-Boot so I'm sorry if I waste your time with silly
questions, but I can't seem to understand some details.

1- Does some info->read implementation ever want its buffer aligned to
  ARCH_DMA_MINALIGN ? I thought so, because of some code using aligned
  buffers, but I can't find it documented. Must be too obvious except
  for me ?

2- What constraints do we expect about the buffer returned by 
   spl_get_load_buffer(0, size)  ?  From what I see it would seem 
   to be often just CONFIG_SYS_TEXT_BASE ? 
   Do we know CONFIG_SYS_TEXT_BASE is DMA aligned ? (I think it will be).
   Does it need to be in "the middle" of RAM, with room before it?
      grep -E 'CONFIG_SYS_TEXT_BASE=0(x0+)?\s*$' configs/*
   returns some 35 boards with CONFIG_SYS_TEXT_BASE=0
   Can we assume we can write before the buffer and after buffer+size?

3- do all implementations of info->read expect the size to be in
   ARCH_DMA_ALIGN  units, not a size in bytes 
   when there's info->filename ?

   spl_fat has filename, bl_len=1 but expects size in bytes, 
   not in blocks of length ARCH_DMA_MINALIGN (which could be >1)

   on the other hand (doesn't seem to be touched by this series yet?)
    spl_imx_romapi has no filename but expects size in bytes, 
                   not in bl_len=pagesize units ? 
     
El Thu, May 05, 2022 at 04:16:47PM -0400, Sean Anderson deia:
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index c9750ee163..f9a1cfc71e 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -399,6 +399,74 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>  	return 0;
>  }
>  
> +static int spl_simple_read(struct spl_load_info *info, void *buf, size_t size,
> +			   size_t offset)
> +{
> +	size_t bl_len = info->filename ? ARCH_DMA_MINALIGN : info->bl_len;

does it work for spl_fat (and spl_imx_romapi if ever needed)? 
and should this or those be fixed ?

> +	size_t bl_mask = bl_len - 1;
> +	size_t overhead = offset & bl_mask;
> +	size_t bl_shift = fls(bl_mask);
> +	int ret;
> +
> +	debug("%s: buf=%p size=%lx offset=%lx\n", __func__, buf, (long)size,
> +	      (long)offset);
> +	debug("%s: bl_len=%lx bl_mask=%lx bl_shift=%lx\n", __func__, bl_len,
> +	      bl_mask, bl_shift);
> +
> +	buf -= overhead;

buf could be 0 ? 
If buf was aligned on entry can it be unaligned now, 
and does it need to be aligned ? 

> +	size = (size + overhead + bl_mask) >> bl_shift;

ditto for spl_fat (and spl_imx_romapi) ? 

> +	offset = offset >> bl_shift;
> +
> +	debug("info->read(info, %lx, %lx, %p)\n", (ulong)offset, (ulong)size,
> +	      buf);
> +	ret = info->read(info, offset, size, buf);

This could read some bytes before the buf pointer that was given to us
and beyond buf+size values received as arguments.

We were given bytes and read multiples of bl_len (or
ARCH_DMA_MINALIGN) bytes, and then there's overhead.

Is this always ok ? 

> +	return ret == size ? 0 : -EIO;
> +}
> +
> +int spl_load(struct spl_image_info *spl_image,
> +	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
> +	     struct image_header *header, size_t size, size_t sector)
> +{
> +	int ret;
> +	size_t offset = sector * info->bl_len;
> +
> +	if (image_get_magic(header) == FDT_MAGIC) {
> +		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
> +			void *buf;
> +
> +			/*
> +			 * In order to support verifying images in the FIT, we
> +			 * need to load the whole FIT into memory. Try and
> +			 * guess how much we need to load by using the total
> +			 * size. This will fail for FITs with external data,
> +			 * but there's not much we can do about that.
> +			 */
> +			if (!size)
> +				size = roundup(fdt_totalsize(header), 4);
> +			buf = spl_get_load_buffer(0, size);

maybe 
   extra = max(info->bl_len, ARCH_DMA_MINALIGN) - 1; /* could be more precise, less wasteful */
   buf = spl_get_load_buffer(extra , size + extra) ;

or maybe better

   buf = spl_get_load_buffer(0, size + 2 * extra) + extra ;

or something, to prevent buf being 0 and make sure we have almost 1 buffer
before and one buffer after the image size to cater for images unaligned in media ? 

Also any consideration needed for the (DMA?) alignment of buf ?

> +			ret = spl_simple_read(info, buf, size, offset);
> +			if (ret)
> +				return ret;
> +
> +			return spl_parse_image_header(spl_image, bootdev, buf);
> +		}
> +
> +		if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
> +			return spl_load_simple_fit(spl_image, info, sector,
> +						   header);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
> +		return spl_load_imx_container(spl_image, info, sector);
> +
> +	ret = spl_parse_image_header(spl_image, bootdev, header);
> +	if (ret)
> +		return ret;
> +
> +	return spl_simple_read(info, (void *)spl_image->load_addr,
> +			       spl_image->size, offset + spl_image->offset);

This looks like maybe it could run into the same problem as spl_load_fit_image

http://patchwork.ozlabs.org/project/uboot/patch/20220609152414.1518919-1-jerome.forissier@linaro.org/

Namely that the extra data in the media blocks before and after the
image can get written outside [load_addr, load_addr+length) and give
trouble (in the case Jerome Forissier found, writes to 0xff3b2XYZ were
overwriting 0xff3b0XYZ on rk3399 because INTMEM1 was only a 8 KiB
SRAM).  That was solved by optionally reading into an aligned buffer
and copying from there to load_addr without overflow (in chunks, but
it could be at once if there's room).

But I'm no longer sure in which case I am, and when can we reach this. 
Non FIT image that is not inside a FIT image? I don't think current 
Rock Pi 4 would get here, but maybe loading from something else or 
in the future going to binman, or something ? 

> +}
> +
>  __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>  {
>  	typedef void __noreturn (*image_entry_noargs_t)(void);
> diff --git a/include/spl.h b/include/spl.h
> index 6134aba857..025fffb895 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -237,7 +237,7 @@ struct spl_image_info {
>   *
>   * @dev: Pointer to the device, e.g. struct mmc *
>   * @priv: Private data for the device
> - * @bl_len: Block length for reading in bytes
> + * @bl_len: Block length for reading in bytes; must be a power of 2
>   * @filename: Name of the fit image file.
>   * @read: Function to call to read from the device
>   */
> @@ -609,6 +609,34 @@ int spl_load_image_ext_os(struct spl_image_info *spl_image,
>  			  struct spl_boot_device *bootdev,
>  			  struct blk_desc *block_dev, int partition);
>  
> +/**
> + * spl_load() - Parse a header and load the image
> + * @spl_image: Image data which will be filled in by this function
> + * @bootdev: The device to load from
> + * @info: Describes how to load additional information from @bootdev. At the
> + *        minimum, read() and bl_len must be populated.

declare whether read must be able to read from unaligned buffers ? 

> + * @header: The image header. This should already have been loaded. It may be
> + *          clobbered by the load process (if e.g. the load address overlaps).
> + * @size: The size of the image, if it is known in advance. Some boot devices
> + *        (such as filesystems) know how big an image is before parsing the
> + *        header. If this information is unknown, then the size will be
> + *        determined from the header.

The size in bytes.
If 0 , then the size will be

> + * @sectors: The offset from the start if @bootdev, in units of @info->bl_len.

if -> of 
maybe (nitpick)
in units of @info->bl_len. -> in units of @info->bl_len bytes.

> + *           This should have the offset @header was loaded from. It will be
> + *           added to any offsets passed to @info->read().
> + *
> + * This function determines the image type (FIT, legacy, i.MX, raw, etc), calls
> + * the appropriate parsing function, determines the load address, and the loads
> + * the image from storage. It is designed to replace ad-hoc image loading which
> + * may not support all image types (especially when config options are
> + * involved).
> + *
> + * Return: 0 on success, or a negative error on failure
> + */
> +int spl_load(struct spl_image_info *spl_image,
> +	     const struct spl_boot_device *bootdev, struct spl_load_info *info,
> +	     struct image_header *header, size_t size, size_t sector);
> +
>  /**
>   * spl_early_init() - Set up device tree and driver model in SPL if enabled
>   *

Thanks for your patience, on top of for your code.

Re: [PATCH v3 1/9] spl: Add generic spl_load function

[Sean Anderson]

Hi Xavier,

On 6/16/22 5:42 AM, Xavier Drudis Ferran wrote:
> [You don't often get email from xdrudis@tinet.cat. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello.
> 
> Thank you for your work, simplifying and generalizing code,
> and sorry that I hadn't seen this series before.
> 
> I'm new to U-Boot so I'm sorry if I waste your time with silly
> questions, but I can't seem to understand some details.
> 
> 1- Does some info->read implementation ever want its buffer aligned to
>   ARCH_DMA_MINALIGN ? I thought so, because of some code using aligned
>   buffers, but I can't find it documented. Must be too obvious except
>   for me ?

I believe that some storage drivers expect the buffer to be DMA aligned.
See e.g. spl_simple_fit_read which is what I based spl_simple_read off of.

> 2- What constraints do we expect about the buffer returned by
>    spl_get_load_buffer(0, size)  ?  From what I see it would seem
>    to be often just CONFIG_SYS_TEXT_BASE ?
>    Do we know CONFIG_SYS_TEXT_BASE is DMA aligned ? (I think it will be).

It should be.

>    Does it need to be in "the middle" of RAM, with room before it?
>       grep -E 'CONFIG_SYS_TEXT_BASE=0(x0+)?\s*$' configs/*
>    returns some 35 boards with CONFIG_SYS_TEXT_BASE=0

Yes. Presumably these boards did not support legacy images before. So this
will not break anything, but it will not work either.

>    Can we assume we can write before the buffer and after buffer+size

We can assume that we can write from buffer+offset to buffer+offset+size.
It's up to spl_get_load_buffer to ensure this is the case (but as you can
see it doesn't always do a good job).

For some context, AIUI the idea of using a negative offset is that for
memory-mapped storage this will ensure that the kernel is aligned to some
known base address (which is typically the load address). 

> 3- do all implementations of info->read expect the size to be in
>    ARCH_DMA_ALIGN  units, not a size in bytes
>    when there's info->filename ?

From the docs for struct spl_load_info, info->read uses units of
info->bl_len. For many types of storage this is 1 (byte aligned). For
others, it is the block size (typically 512 bytes, but maybe more for
flash). Whether or not there is a filename just controls whether we need
to be aligned to some other value.

>    spl_fat has filename, bl_len=1 but expects size in bytes,
>    not in blocks of length ARCH_DMA_MINALIGN (which could be >1)
> 
>    on the other hand (doesn't seem to be touched by this series yet?)
>     spl_imx_romapi has no filename but expects size in bytes,
>                    not in bl_len=pagesize units ?
> 
> El Thu, May 05, 2022 at 04:16:47PM -0400, Sean Anderson deia:
>>
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index c9750ee163..f9a1cfc71e 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -399,6 +399,74 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>>       return 0;
>>  }
>>
>> +static int spl_simple_read(struct spl_load_info *info, void *buf, size_t size,
>> +                        size_t offset)
>> +{
>> +     size_t bl_len = info->filename ? ARCH_DMA_MINALIGN : info->bl_len;
> 
> does it work for spl_fat

Yes (on my system). TBH I'm not really sure about why the DMA align was
added in the first place. I think you need to ask Simon about that.

> (and spl_imx_romapi if ever needed)?

No clue.

> and should this or those be fixed ?

Maybe? I think it might be better to add a flag or a field to specify
whether we need to be aligned.

>> +     size_t bl_mask = bl_len - 1;
>> +     size_t overhead = offset & bl_mask;
>> +     size_t bl_shift = fls(bl_mask);
>> +     int ret;
>> +
>> +     debug("%s: buf=%p size=%lx offset=%lx\n", __func__, buf, (long)size,
>> +           (long)offset);
>> +     debug("%s: bl_len=%lx bl_mask=%lx bl_shift=%lx\n", __func__, bl_len,
>> +           bl_mask, bl_shift);
>> +
>> +     buf -= overhead;
> 
> buf could be 0 ?

Yeah, maybe. The problem is that generally we don't control the address
where the final image will be loaded. Something like
spl_parse_image_header is going to determine where we load the image,
and we have to work around that. We could perhaps warn here with DEBUG
enabled, but at this point everything is already laid out. The only way
to do this properly would be to load everything to an aligned location,
and then memmove it so the load_addr is correct. For a completely correct
load function, that's probably the only way to do it. I'll look into how
that affects the size.

spl_simple_fit_read gets around this because it gets to choose the load
address.

> If buf was aligned on entry can it be unaligned now,
> and does it need to be aligned ?

It will be unchanged by this 

>> +     size = (size + overhead + bl_mask) >> bl_shift;
> 
> ditto for spl_fat (and spl_imx_romapi) ?
> 
>> +     offset = offset >> bl_shift;
>> +
>> +     debug("info->read(info, %lx, %lx, %p)\n", (ulong)offset, (ulong)size,
>> +           buf);
>> +     ret = info->read(info, offset, size, buf);
> 
> This could read some bytes before the buf pointer that was given to us
> and beyond buf+size values received as arguments.

Correct. 

> We were given bytes and read multiples of bl_len (or
> ARCH_DMA_MINALIGN) bytes, and then there's overhead.
> 
> Is this always ok ?

No. But all the existing code is broken in this same way too.

>> +     return ret == size ? 0 : -EIO;
>> +}
>> +
>> +int spl_load(struct spl_image_info *spl_image,
>> +          const struct spl_boot_device *bootdev, struct spl_load_info *info,
>> +          struct image_header *header, size_t size, size_t sector)
>> +{
>> +     int ret;
>> +     size_t offset = sector * info->bl_len;
>> +
>> +     if (image_get_magic(header) == FDT_MAGIC) {
>> +             if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
>> +                     void *buf;
>> +
>> +                     /*
>> +                      * In order to support verifying images in the FIT, we
>> +                      * need to load the whole FIT into memory. Try and
>> +                      * guess how much we need to load by using the total
>> +                      * size. This will fail for FITs with external data,
>> +                      * but there's not much we can do about that.
>> +                      */
>> +                     if (!size)
>> +                             size = roundup(fdt_totalsize(header), 4);
>> +                     buf = spl_get_load_buffer(0, size);
> 
> maybe
>    extra = max(info->bl_len, ARCH_DMA_MINALIGN) - 1; /* could be more precise, less wasteful */
>    buf = spl_get_load_buffer(extra , size + extra) ;
> 
> or maybe better
> 
>    buf = spl_get_load_buffer(0, size + 2 * extra) + extra ;
> 
> or something, to prevent buf being 0 and make sure we have almost 1 buffer
> before and one buffer after the image size to cater for images unaligned in media ?
> 
> Also any consideration needed for the (DMA?) alignment of buf ?

Yes, probably.

>> +                     ret = spl_simple_read(info, buf, size, offset);
>> +                     if (ret)
>> +                             return ret;
>> +
>> +                     return spl_parse_image_header(spl_image, bootdev, buf);
>> +             }
>> +
>> +             if (IS_ENABLED(CONFIG_SPL_LOAD_FIT))
>> +                     return spl_load_simple_fit(spl_image, info, sector,
>> +                                                header);
>> +     }
>> +
>> +     if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER))
>> +             return spl_load_imx_container(spl_image, info, sector);
>> +
>> +     ret = spl_parse_image_header(spl_image, bootdev, header);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return spl_simple_read(info, (void *)spl_image->load_addr,
>> +                            spl_image->size, offset + spl_image->offset);
> 
> This looks like maybe it could run into the same problem as spl_load_fit_image
> 
> http://patchwork.ozlabs.org/project/uboot/patch/20220609152414.1518919-1-jerome.forissier@linaro.org/
> 
> Namely that the extra data in the media blocks before and after the
> image can get written outside [load_addr, load_addr+length) and give
> trouble (in the case Jerome Forissier found, writes to 0xff3b2XYZ were
> overwriting 0xff3b0XYZ on rk3399 because INTMEM1 was only a 8 KiB
> SRAM).  That was solved by optionally reading into an aligned buffer
> and copying from there to load_addr without overflow (in chunks, but
> it could be at once if there's room).
> 
> But I'm no longer sure in which case I am, and when can we reach this.
> Non FIT image that is not inside a FIT image? I don't think current
> Rock Pi 4 would get here, but maybe loading from something else or
> in the future going to binman, or something ?

This code in particular is effectively translating old semantics to new.
The negative (unaligned) offset stuff is done by all loaders supporting
legacy images. So it will not always work, but it *will* work for things
where it did before.

>> +}
>> +
>>  __weak void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>>  {
>>       typedef void __noreturn (*image_entry_noargs_t)(void);
>> diff --git a/include/spl.h b/include/spl.h
>> index 6134aba857..025fffb895 100644
>> --- a/include/spl.h
>> +++ b/include/spl.h
>> @@ -237,7 +237,7 @@ struct spl_image_info {
>>   *
>>   * @dev: Pointer to the device, e.g. struct mmc *
>>   * @priv: Private data for the device
>> - * @bl_len: Block length for reading in bytes
>> + * @bl_len: Block length for reading in bytes; must be a power of 2
>>   * @filename: Name of the fit image file.
>>   * @read: Function to call to read from the device
>>   */
>> @@ -609,6 +609,34 @@ int spl_load_image_ext_os(struct spl_image_info *spl_image,
>>                         struct spl_boot_device *bootdev,
>>                         struct blk_desc *block_dev, int partition);
>>
>> +/**
>> + * spl_load() - Parse a header and load the image
>> + * @spl_image: Image data which will be filled in by this function
>> + * @bootdev: The device to load from
>> + * @info: Describes how to load additional information from @bootdev. At the
>> + *        minimum, read() and bl_len must be populated.
> 
> declare whether read must be able to read from unaligned buffers ?
> 
>> + * @header: The image header. This should already have been loaded. It may be
>> + *          clobbered by the load process (if e.g. the load address overlaps).
>> + * @size: The size of the image, if it is known in advance. Some boot devices
>> + *        (such as filesystems) know how big an image is before parsing the
>> + *        header. If this information is unknown, then the size will be
>> + *        determined from the header.
> 
> The size in bytes.
> If 0 , then the size will be

Will update.

>> + * @sectors: The offset from the start if @bootdev, in units of @info->bl_len.
> 
> if -> of

Will fix.

> maybe (nitpick)
> in units of @info->bl_len. -> in units of @info->bl_len bytes.

I like the original wording here.

>> + *           This should have the offset @header was loaded from. It will be
>> + *           added to any offsets passed to @info->read().
>> + *
>> + * This function determines the image type (FIT, legacy, i.MX, raw, etc), calls
>> + * the appropriate parsing function, determines the load address, and the loads
>> + * the image from storage. It is designed to replace ad-hoc image loading which
>> + * may not support all image types (especially when config options are
>> + * involved).
>> + *
>> + * Return: 0 on success, or a negative error on failure
>> + */
>> +int spl_load(struct spl_image_info *spl_image,
>> +          const struct spl_boot_device *bootdev, struct spl_load_info *info,
>> +          struct image_header *header, size_t size, size_t sector);
>> +
>>  /**
>>   * spl_early_init() - Set up device tree and driver model in SPL if enabled
>>   *
> 
> Thanks for your patience, on top of for your code.

Sure. I suppose the real question is how badly we really need things to be DMA aligned...

I think Simon really needs to weigh in on this, but he's been pretty quiet recently.

--Sean