[U-Boot] [RFC/RFT PATCH v4 3/3] image: Add compressed Image parsing support in booti.

Tom Rini trini at konsulko.com
Mon Mar 2 19:41:30 CET 2020


On Fri, Feb 28, 2020 at 05:15:53PM -0800, Atish Patra wrote:
> On Thu, Feb 20, 2020 at 2:25 PM Atish Patra <atishp at atishpatra.org> wrote:
> >
> > On Thu, Feb 20, 2020 at 1:14 PM David Abdurachmanov
> > <david.abdurachmanov at gmail.com> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 10:38 PM Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Sun, Feb 16, 2020 at 04:48:22PM -0800, Atish Patra wrote:
> > > > > On Fri, Feb 14, 2020 at 8:43 AM Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Feb 13, 2020 at 11:32:52PM +0200, David Abdurachmanov wrote:
> > > > > > > On Thu, Feb 13, 2020 at 6:17 PM Tom Rini <trini at konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Feb 05, 2020 at 12:01:38AM +0000, Atish Patra wrote:
> > > > > > > > > On Fri, 2019-11-22 at 18:19 -0800, Atish Patra wrote:
> > > > > > > > > > On Wed, 2019-11-13 at 11:47 -0800, Atish Patra wrote:
> > > > > > > > > > > On Wed, 2019-11-13 at 15:36 +0200, David Abdurachmanov wrote:
> > > > > > > > > > > > On Sat, Nov 9, 2019 at 2:14 AM Atish Patra <atish.patra at wdc.com>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > Add compressed Image parsing support so that booti can parse
> > > > > > > > > > > > > both
> > > > > > > > > > > > > flat and compressed Image to boot Linux. Currently, it is
> > > > > > > > > > > > > difficult
> > > > > > > > > > > > > to calculate a safe address for every board where the
> > > > > > > > > > > > > compressed
> > > > > > > > > > > > > image can be decompressed. It is also not possible to figure
> > > > > > > > > > > > > out
> > > > > > > > > > > > > the
> > > > > > > > > > > > > size of the compressed file as well. Thus, user need to set two
> > > > > > > > > > > > > additional environment variables kernel_comp_addr_r and
> > > > > > > > > > > > > filesize
> > > > > > > > > > > > > to
> > > > > > > > > > > > > make this work.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Following compression methods are supported for now.
> > > > > > > > > > > > > lzma, lzo, bzip2, gzip.
> > > > > > > > > > > > >
> > > > > > > > > > > > > lz4 support is not added as ARM64 kernel generates a lz4
> > > > > > > > > > > > > compressed
> > > > > > > > > > > > > image with legacy header which U-Boot doesn't know how to parse
> > > > > > > > > > > > > and
> > > > > > > > > > > > > decompress.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Tested on HiFive Unleashed and Qemu for RISC-V.
> > > > > > > > > > > > > Tested on Qemu for ARM64.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Atish Patra <atish.patra at wdc.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > I could not test this patch on any ARM64 boards due to lack of
> > > > > > > > > > > > > access to any ARM64 board. If anybody can test it on ARM64,
> > > > > > > > > > > > > that
> > > > > > > > > > > > > would be great.
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  cmd/booti.c                | 40 ++++++++++++++++++++++++++-
> > > > > > > > > > > > >  doc/README.distro          | 12 +++++++++
> > > > > > > > > > > > >  doc/board/sifive/fu540.rst | 55
> > > > > > > > > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > > >  3 files changed, 106 insertions(+), 1 deletion(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/cmd/booti.c b/cmd/booti.c
> > > > > > > > > > > > > index c36b0235df8c..cd8670a9a8db 100644
> > > > > > > > > > > > > --- a/cmd/booti.c
> > > > > > > > > > > > > +++ b/cmd/booti.c
> > > > > > > > > > > > > @@ -13,6 +13,7 @@
> > > > > > > > > > > > >  #include <linux/kernel.h>
> > > > > > > > > > > > >  #include <linux/sizes.h>
> > > > > > > > > > > > >
> > > > > > > > > > > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > > > > > > > > > >  /*
> > > > > > > > > > > > >   * Image booting support
> > > > > > > > > > > > >   */
> > > > > > > > > > > > > @@ -23,6 +24,12 @@ static int booti_start(cmd_tbl_t *cmdtp, int
> > > > > > > > > > > > > flag, int argc,
> > > > > > > > > > > > >         ulong ld;
> > > > > > > > > > > > >         ulong relocated_addr;
> > > > > > > > > > > > >         ulong image_size;
> > > > > > > > > > > > > +       uint8_t *temp;
> > > > > > > > > > > > > +       ulong dest;
> > > > > > > > > > > > > +       ulong dest_end;
> > > > > > > > > > > > > +       unsigned long comp_len;
> > > > > > > > > > > > > +       unsigned long decomp_len;
> > > > > > > > > > > > > +       int ctype;
> > > > > > > > > > > > >
> > > > > > > > > > > > >         ret = do_bootm_states(cmdtp, flag, argc, argv,
> > > > > > > > > > > > > BOOTM_STATE_START,
> > > > > > > > > > > > >                               images, 1);
> > > > > > > > > > > > > @@ -37,6 +44,33 @@ static int booti_start(cmd_tbl_t *cmdtp, int
> > > > > > > > > > > > > flag, int argc,
> > > > > > > > > > > > >                 debug("*  kernel: cmdline image address =
> > > > > > > > > > > > > 0x%08lx\n", ld);
> > > > > > > > > > > > >         }
> > > > > > > > > > > > >
> > > > > > > > > > > > > +       temp = map_sysmem(ld, 0);
> > > > > > > > > > > > > +       ctype = image_decomp_type(temp, 2);
> > > > > > > > > > > > > +       if (ctype > 0) {
> > > > > > > > > > > > > +               dest = env_get_ulong("kernel_comp_addr_r", 16,
> > > > > > > > > > > > > 0);
> > > > > > > > > > > > > +               comp_len = env_get_ulong("filesize", 16, 0);
> > > > > > > > > > > > > +               if (!dest || !comp_len) {
> > > > > > > > > > > > > +                       puts("kernel_comp_addr_r or filesize is
> > > > > > > > > > > > > not
> > > > > > > > > > > > > provided!\n");
> > > > > > > > > > > > > +                       return -EINVAL;
> > > > > > > > > > > > > +               }
> > > > > > > > > > > > > +               if (dest < gd->ram_base || dest > gd->ram_top)
> > > > > > > > > > > > > {
> > > > > > > > > > > > > +                       puts("kernel_comp_addr_r is outside of
> > > > > > > > > > > > > DRAM
> > > > > > > > > > > > > range!\n");
> > > > > > > > > > > > > +                       return -EINVAL;
> > > > > > > > > > > > > +               }
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +               debug("kernel image compression type %d size =
> > > > > > > > > > > > > 0x%08lx address = 0x%08lx\n",
> > > > > > > > > > > > > +                       ctype, comp_len, (ulong)dest);
> > > > > > > > > > > > > +               decomp_len = comp_len * 10;
> > > > > > > > > > > > > +               ret = image_decomp(ctype, 0, ld,
> > > > > > > > > > > > > IH_TYPE_KERNEL,
> > > > > > > > > > > > > +                                (void *)dest, (void *)ld,
> > > > > > > > > > > > > comp_len,
> > > > > > > > > > > > > +                                decomp_len, &dest_end);
> > > > > > > > > > > > > +               if (ret)
> > > > > > > > > > > > > +                       return ret;
> > > > > > > > > > > > > +               /* dest_end contains the uncompressed Image
> > > > > > > > > > > > > size
> > > > > > > > > > > > > */
> > > > > > > > > > > > > +               memmove((void *) ld, (void *)dest, dest_end);
> > > > > > > > > > > > > +       }
> > > > > > > > > > > > > +       unmap_sysmem((void *)ld);
> > > > > > > > > > > > > +
> > > > > > > > > > > > >         ret = booti_setup(ld, &relocated_addr, &image_size,
> > > > > > > > > > > > > false);
> > > > > > > > > > > > >         if (ret != 0)
> > > > > > > > > > > > >                 return 1;
> > > > > > > > > > > > > @@ -96,10 +130,14 @@ int do_booti(cmd_tbl_t *cmdtp, int flag,
> > > > > > > > > > > > > int
> > > > > > > > > > > > > argc, char * const argv[])
> > > > > > > > > > > > >  #ifdef CONFIG_SYS_LONGHELP
> > > > > > > > > > > > >  static char booti_help_text[] =
> > > > > > > > > > > > >         "[addr [initrd[:size]] [fdt]]\n"
> > > > > > > > > > > > > -       "    - boot Linux 'Image' stored at 'addr'\n"
> > > > > > > > > > > > > +       "    - boot Linux flat or compressed 'Image' stored at
> > > > > > > > > > > > > 'addr'\n"
> > > > > > > > > > > > >         "\tThe argument 'initrd' is optional and specifies the
> > > > > > > > > > > > > address\n"
> > > > > > > > > > > > >         "\tof an initrd in memory. The optional parameter
> > > > > > > > > > > > > ':size'
> > > > > > > > > > > > > allows\n"
> > > > > > > > > > > > >         "\tspecifying the size of a RAW initrd.\n"
> > > > > > > > > > > > > +       "\tCurrently only booting from gz, bz2, lzma and lz4
> > > > > > > > > > > > > compression\n"
> > > > > > > > > > > > > +       "\ttypes are supported. In order to boot from any of
> > > > > > > > > > > > > these
> > > > > > > > > > > > > compressed\n"
> > > > > > > > > > > > > +       "\timages, user have to set kernel_comp_addr_r and
> > > > > > > > > > > > > filesize
> > > > > > > > > > > > > enviornment\n"
> > > > > > > > > > > > > +       "\tvariables beforehand.\n"
> > > > > > > > > > > > >  #if defined(CONFIG_OF_LIBFDT)
> > > > > > > > > > > > >         "\tSince booting a Linux kernel requires a flat device-
> > > > > > > > > > > > > tree, a\n"
> > > > > > > > > > > > >         "\tthird argument providing the address of the device-
> > > > > > > > > > > > > tree
> > > > > > > > > > > > > blob\n"
> > > > > > > > > > > > > diff --git a/doc/README.distro b/doc/README.distro
> > > > > > > > > > > > > index ab6e6f4e74be..67b49e1e4b6a 100644
> > > > > > > > > > > > > --- a/doc/README.distro
> > > > > > > > > > > > > +++ b/doc/README.distro
> > > > > > > > > > > > > @@ -246,6 +246,18 @@ kernel_addr_r:
> > > > > > > > > > > > >
> > > > > > > > > > > > >    A size of 16MB for the kernel is likely adequate.
> > > > > > > > > > > > >
> > > > > > > > > > > > > +kernel_comp_addr_r:
> > > > > > > > > > > > > +  Optional. This is only required if user wants to boot Linux
> > > > > > > > > > > > > from
> > > > > > > > > > > > > a compressed
> > > > > > > > > > > > > +  Image(.gz, .bz2, .lzma, .lzo) using booti command. It
> > > > > > > > > > > > > represents
> > > > > > > > > > > > > the location
> > > > > > > > > > > > > +  in RAM where the compressed Image will be decompressed
> > > > > > > > > > > > > temporarily. Once the
> > > > > > > > > > > > > +  decompression is complete, decompressed data will be moved
> > > > > > > > > > > > > kernel_addr_r for
> > > > > > > > > > > > > +  booting.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +filesize:
> > > > > > > > > > > > > +  Optional. This is only required if user wants to boot Linux
> > > > > > > > > > > > > from
> > > > > > > > > > > > > a compressed
> > > > > > > > > > > > > +  Image using booti command. It represents the size of the
> > > > > > > > > > > > > compressed file. The
> > > > > > > > > > > > > +  size has to at least the size of loaded image for
> > > > > > > > > > > > > decompression
> > > > > > > > > > > > > to succeed.
> > > > > > > > > > > > > +
> > > > > > > > > > > >
> > > > > > > > > > > > I am not sure I like using filesize here compared to
> > > > > > > > > > > > kernel_gz_size.
> > > > > > > > > > > > It's true that $filesize will be set once your load the binary,
> > > > > > > > > > > > e.g.
> > > > > > > > > > > > from mmc.
> > > > > > > > > > > > But in EXTLINUX/PXE situation $filesize could be wrong.
> > > > > > > > > > > >
> > > > > > > > > > > > The load happens in this order:
> > > > > > > > > > > > - initrd
> > > > > > > > > > > > - kernel
> > > > > > > > > > > > - fdt
> > > > > > > > > > > >
> > > > > > > > > > > > So if I specify FDT in my EXTLINUX/PXE config the $filesize will
> > > > > > > > > > > > be
> > > > > > > > > > > > wrong
> > > > > > > > > > > > while attempting to boot the kernel. Unless we save filesize of
> > > > > > > > > > > > kernel after
> > > > > > > > > > > > loading and set it again before calling do_booti() in pxe.c.
> > > > > > > > > > > >
> > > > > > > > > > > > Currently I set kernel_gz_size in board environment, which is set
> > > > > > > > > > > > to
> > > > > > > > > > > > max
> > > > > > > > > > > > allowed size.
> > > > > > > > > > > >
> > > > > > > > > > > > david
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Ahh okay. There are two ways to fix it.
> > > > > > > > > > >
> > > > > > > > > > > 1. Fix the pxe implementation but save the filesize to be used
> > > > > > > > > > > later.
> > > > > > > > > > >
> > > > > > > > > > > But some other user may fall into the same issue without realizing
> > > > > > > > > > > it
> > > > > > > > > > > if the user loads any other image after compressed kernel Image.
> > > > > > > > > > >
> > > > > > > > > > > We need clear documentation saying that, compressed kernel Image
> > > > > > > > > > > should
> > > > > > > > > > > be loaded last before executing booti or $filesize must be set
> > > > > > > > > > > manually
> > > > > > > > > > > before calling booti.
> > > > > > > > > > >
> > > > > > > > > > > 2. Just change it back to kernel_comp_size (compressed kernel image
> > > > > > > > > > > size) to avoid any confusion.
> > > > > > > > > > >
> > > > > > > > > > > Any preference ?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Any thoughts ?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I think this patch got lost during holidays :). Any suggestion on what
> > > > > > > > > should be the best approach to resolve the issue with pxe
> > > > > > > > > implementation ?
> > > > > > > >
> > > > > > > > I think that we really need to be careful when adding "automagic"
> > > > > > > > features like this.  Thinking harder about it all, we can't re-use any
> > > > > > > > existing variable as there's going to be some case of a setup breaking
> > > > > > > > in strange silent ways.
> > > > > > > >
> > > > > > > > Can we uncompress a single chunk / page / something of the compressed
> > > > > > > > image to get the header and thus know the uncompressed size?  We would
> > > > > > > > then know the compressed size is no larger than that and can progress
> > > > > > > > from there?
> > > > > > >
> > > > > > > After a quick google on various compression formats I get impression this
> > > > > > > is not a common thing to record in the headers. The kernels could be
> > > > > > > compressed with gzip, bzip2, lz4, lzma and lzo (I only checked Makefile for
> > > > > > >  riscv).
> > > > > >
> > > > > > Sorry I wasn't clear enough.  The contents in question here contain a
> > > > > > header at the start which does contain the uncompressed image size.
> > > > > >
> > > > >
> > > > > The objective of the patch was to support all the compressed image
> > > > > type that kernel supports.
> > > > > i.e. gz, bz2, lzma, lzo.
> > > > >
> > > > > uncompressed image size in compressed header in the following file formats:
> > > > >
> > > > > 1. gz: Uncompressed size is at the footer (http://www.zlib.org/rfc-gzip.html)
> > > > > 2. bz2: No information in the header
> > > > > 3. lzma: Present in the header
> > > > > (https://svn.python.org/projects/external/xz-5.0.3/doc/lzma-file-format.txt)
> > > > > 4. lzo: Present in the header (https://gist.github.com/jledet/1333896)
> > > > >
> > > > > To summarize, if we want to parse the compressed file to determine the
> > > > > uncompressed size,
> > > > > we have to add the support to generic compression library for all file
> > > > > formats rather than just doing it here.
> > > > > Additionally, not all format mandates the uncompressed size which
> > > > > makes it difficult.
> > > >
> > > > Here's my confusion, and perhaps dumb question.  What happens if we:
> > > > 1. Uncompress the first "chunk" of the data, which will let us at the
> > > > Image header.
> > >
> 
> Theoretically that should work. However, current zlib implementation
> doesn't allow
> decompressing few chunks of data. I tried a few different sizes. But
> any size less
> than compressed file size results in following error.
> 
> Error: inflate() returned -5
> 
> There may be some way to modify zlib implementation to allow that but I am not a
> compression expert to make such changes.

Thanks for looking in to this.

> Let us know what other alternative approaches you prefer.
> 
> 1. Save $filesize to kernel_comp_size after loading kernel binary and
> pass it around in C code.
> or
> 2.  Just use an environment variable other than filesize that users can set it.

I think we need to go with #2 here as I fear #1 will be error prone (how
do we know we just loaded the kernel?) over just adapting the existing
general logic to add 'setenv kernel_comp_size $filesize' after loading
the kernel image.  I'm open to reviewing #1 all the same however.
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/20200302/7aad66d1/attachment.sig>


More information about the U-Boot mailing list