Re: [PATCH 20/26 v6] spl: spl_nor: Move legacy image loading into spl_legacy.c

Am 08.04.20 um 10:09 schrieb Stefan Roese:

Move the legacy image loading into spl_legacy.c. This makes it easier to extend the legacy image handling with new features that other SPL loaders might use (e.g. spl_spi.c etc).

No functional change indended.

Signed-off-by: Stefan Roese sr@denx.de Cc: Weijie Gao weijie.gao@mediatek.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com

---

Changes in v6:

• New patch

common/spl/spl_legacy.c | 17 +++++++++++++++++ common/spl/spl_nor.c | 34 +++++++++++++++++----------------- include/spl.h | 14 ++++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c index 772135193e..d0fc4b111e 100644 --- a/common/spl/spl_legacy.c +++ b/common/spl/spl_legacy.c @@ -51,3 +51,20 @@ int spl_parse_legacy_header(struct spl_image_info *spl_image,

return 0; }

+int spl_load_legacy_img(struct spl_image_info *spl_image,

• 	const struct image_header *header,
  

• 	uintptr_t dataptr, struct spl_load_info *info)
  

+{

• int ret;
• ret = spl_parse_image_header(spl_image, header);
• if (ret)

• return ret;
  

• dataptr = (uintptr_t)header + sizeof(struct image_header);

how should this work? You call this function with

spl_load_legacy_img(spl_image, &hdr, dataptr, &load);

so "header" points to the copy on the stack. "dataptr" already holds the address of the SPL payload. If you overwrite "dataptr" now, it will point to the stack area after the header copy.

• info->read(info, dataptr, spl_image->size,

•    (void *)(unsigned long)spl_image->load_addr);
  

• return 0;

+} diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c index cb8e06fe1f..d55165b772 100644 --- a/common/spl/spl_nor.c +++ b/common/spl/spl_nor.c @@ -24,11 +24,8 @@ 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) {

• int ret; __maybe_unused const struct image_header *header; __maybe_unused struct spl_load_info load;

• struct image_header hdr;

• uintptr_t dataptr;

  /*

  • Loading of the payload to SDRAM is done with skipping of

@@ -45,6 +42,8 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, header = (const struct image_header *)CONFIG_SYS_OS_BASE; #ifdef CONFIG_SPL_LOAD_FIT if (image_get_magic(header) == FDT_MAGIC) {

• 	int ret;
  

• debug("Found FIT\n");
  load.bl_len = 1;
  load.read = spl_nor_load_read;
  

@@ -63,6 +62,7 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, #endif if (image_get_os(header) == IH_OS_LINUX) { /* happy - was a Linux */

• 	int ret;
  
  ret = spl_parse_image_header(spl_image, header);
  if (ret)
  

@@ -95,11 +95,9 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, debug("Found FIT format U-Boot\n"); load.bl_len = 1; load.read = spl_nor_load_read;

• ret = spl_load_simple_fit(spl_image, &load,
  

• 			  spl_nor_get_uboot_base(),
  

• 			  (void *)header);
  

• return ret;
  

• return spl_load_simple_fit(spl_image, &load,
  

• 			   spl_nor_get_uboot_base(),
  

• 			   (void *)header);
  

  }

#endif if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) { @@ -109,17 +107,19 @@ static int spl_nor_load_image(struct spl_image_info *spl_image, spl_nor_get_uboot_base()); }

• ret = spl_parse_image_header(spl_image,

• 	(const struct image_header *)spl_nor_get_uboot_base());
  

• if (ret)

• return ret;
  

• if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_SUPPORT)) {

• struct image_header hdr;
  

• uintptr_t dataptr;
  
  

• /* Payload image may not be aligned, so copy it for safety */
• memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
• dataptr = spl_nor_get_uboot_base() + sizeof(struct image_header);

• /* Payload image may not be aligned, so copy it for safety */
  

• memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
  

• dataptr = spl_nor_get_uboot_base() + sizeof(hdr);
  
  

• memcpy((void *)(unsigned long)spl_image->load_addr,

•       (void *)dataptr, spl_image->size);
  

• /* Legacy image handling */
  

• load.bl_len = 1;
  

• load.read = spl_nor_load_read;
  

• return spl_load_legacy_img(spl_image, &hdr, dataptr, &load);
  

• }

I don't understand it fully. Is the purpose of the header copy to avoid unaligned access to the header in spl_parse_image_header()?

If so then the copy should be made in spl_load_legacy_img() itself.

return 0; } diff --git a/include/spl.h b/include/spl.h index 6087cd793c..5331591db5 100644 --- a/include/spl.h +++ b/include/spl.h @@ -219,6 +219,20 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, #define SPL_COPY_PAYLOAD_ONLY 1 #define SPL_FIT_FOUND 2

+/**

• • spl_load_legacy_img() - Loads a legacy image from a device.
• • @spl_image: Image description to set up
• • @dataptr: Source location of the image
• • @header: Image header to parse
• • Reads an legacy image from the device. Loads u-boot image to
• • specified load address.
• • Returns 0 on success.
• */

+int spl_load_legacy_img(struct spl_image_info *spl_image,

• 	const struct image_header *header,
  

• 	uintptr_t dataptr, struct spl_load_info *load);
  

/**

• spl_load_imx_container() - Loads a imx container image from a device.
• @spl_image: Image description to set up