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

[Stefan Roese]

On 09.04.20 20:51, Daniel Schwierzeck wrote:

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 <s...@denx.de>
Cc: Weijie Gao <weijie....@mediatek.com>
Cc: Simon Goldschmidt <simon.k.r.goldschm...@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.

You are correct. I fat-fingered while rebasing the patches here. As you
will see, this assignment will be removed in the next patch [21/26].

I'll change it in v7.

+       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()?

Yes.

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

Thats what I had implemented in an internal version already. Not sure,
why I switched to not using it in this v6? I'll re-think about it and
will perhaps move it to spl_load_legacy_img() again.

Thanks,
Stefan