[PATCH v4 17/20] spl: nor: add lzma decompression support for legacy image

[Weijie Gao]

On Wed, 2020-02-12 at 09:22 +0100, Simon Goldschmidt wrote:
> On Wed, Feb 12, 2020 at 8:49 AM Weijie Gao <weijie.gao at mediatek.com> wrote:
> >
> > This patch adds support for decompressing LZMA compressed u-boot payload in
> > legacy uImage format.
> >
> > Using this patch together with u-boot-lzma.img is useful for NOR flashes as
> > they can reduce the size and load time of u-boot payload.
> >
> > Reviewed-by: Stefan Roese <sr at denx.de>
> > Signed-off-by: Weijie Gao <weijie.gao at mediatek.com>
> > ---
> > Changes since v3: none
> > ---
> >  common/spl/spl_nor.c | 59 ++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 54 insertions(+), 5 deletions(-)
> >
> > diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> > index b1e79b9ded..7c81fb28f6 100644
> > --- a/common/spl/spl_nor.c
> > +++ b/common/spl/spl_nor.c
> > @@ -4,8 +4,19 @@
> >   */
> >
> >  #include <common.h>
> > +#include <cpu_func.h>
> >  #include <spl.h>
> >
> > +#if IS_ENABLED(CONFIG_SPL_LZMA)
> 
> Is this guard really necessary? What happens without it?

Actually nothing will happen.

> 
> > +#include <lzma/LzmaTypes.h>
> > +#include <lzma/LzmaDec.h>
> > +#include <lzma/LzmaTools.h>
> > +#endif
> > +
> > +#ifndef CONFIG_SYS_BOOTM_LEN
> > +#define CONFIG_SYS_BOOTM_LEN   (8 << 20)
> > +#endif
> 
> This looks strange. I think we should have a central place where this is defined
> to a default value. As it is now, you are adding the 3rd place where this is
> defined to a "fallback" value, each with a different value.

This is copied from common/bootm.c. It does exist in two files:
common/bootm.c and common/spl/spl_fit.c.

> 
> > +
> >  static ulong spl_nor_load_read(struct spl_load_info *load, ulong sector,
> >                                ulong count, void *buf)
> >  {
> > @@ -27,6 +38,9 @@ static int spl_nor_load_image(struct spl_image_info *spl_image,
> >         int ret;
> >         __maybe_unused const struct image_header *header;
> >         __maybe_unused struct spl_load_info load;
> > +       __maybe_unused SizeT lzma_len;
> > +       struct image_header hdr;
> > +       uintptr_t dataptr;
> >
> >         /*
> >          * Loading of the payload to SDRAM is done with skipping of
> > @@ -107,14 +121,49 @@ 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());
> > +       /* Payload image may not be aligned, so copy it for safety. */
> > +       memcpy(&hdr, (void *)spl_nor_get_uboot_base(), sizeof(hdr));
> > +       ret = spl_parse_image_header(spl_image, &hdr);
> >         if (ret)
> >                 return ret;
> >
> > -       memcpy((void *)(unsigned long)spl_image->load_addr,
> > -              (void *)(spl_nor_get_uboot_base() + sizeof(struct image_header)),
> > -              spl_image->size);
> > +       dataptr = spl_nor_get_uboot_base() + sizeof(struct image_header);
> > +
> > +       switch (image_get_comp(&hdr)) {
> > +       case IH_COMP_NONE:
> 
> I guess this will increase the size even when LZMA is not enabled, right?
> Do you have numbers for that?

Yes. about 8KiB on mips32r2 platform.

> 
> > +               memmove((void *)(unsigned long)spl_image->load_addr,
> > +                       (void *)dataptr, spl_image->size);
> > +               break;
> > +#if IS_ENABLED(CONFIG_SPL_LZMA)
> > +       case IH_COMP_LZMA:
> > +               lzma_len = CONFIG_SYS_BOOTM_LEN;
> > +
> > +               ret = lzmaBuffToBuffDecompress((void *)spl_image->load_addr,
> > +                                              &lzma_len, (void *)dataptr,
> > +                                              spl_image->size);
> > +
> > +               if (ret) {
> > +                       printf("LZMA decompression error: %d\n", ret);
> > +                       return ret;
> > +               }
> > +
> > +               spl_image->size = lzma_len;
> > +               break;
> > +#endif
> > +       default:
> > +               debug("Compression method %s is not supported\n",
> > +                     genimg_get_comp_short_name(image_get_comp(&hdr)));
> > +               return -EINVAL;
> > +       }
> > +
> > +       flush_cache((unsigned long)spl_image->load_addr, spl_image->size);
> 
> Why is this necessary? I can't see this from the commit message.

This is used for flushing the instruction cache. Without this the
payload may not be able to boot. For this patch series, this will happen
if the payload has a small size.

> 
> > +
> > +       /*
> > +        * If the image did not provide an entry point, assume the entry point
> > +        * is the same as its load address.
> > +        */
> > +       if (!spl_image->entry_point)
> > +               spl_image->entry_point = spl_image->load_addr;
> 
> And another change that is not described in the commit message.

I've checked this is no longer needed. This will be removed.

> 
> And more general: why do we need to code this in every loader? I think it would
> be preferrable to have the loader load the binary data and do the decompression
> (and entry_point assignment) in a central place so that all loaders can benefit
> from compression. As it is now, we are duplicating code when implementing LZMA
> in the next loader.

This feature is originally designed for the case that u-boot is stored
in a small capacity storage device, mostly NOR flashes, and the space
reserved for u-boot is very small. Most loaders (MMC, NAND, SATA, ...)
do not need this at all.

> 
> Regards,
> Simon
> 
> >
> >         return 0;
> >  }
> > --
> > 2.17.1