On Mon, Mar 2, 2020 at 10:41 AM Tom Rini <tr...@konsulko.com> wrote: > > On Fri, Feb 28, 2020 at 05:15:53PM -0800, Atish Patra wrote: > > On Thu, Feb 20, 2020 at 2:25 PM Atish Patra <ati...@atishpatra.org> wrote: > > > > > > On Thu, Feb 20, 2020 at 1:14 PM David Abdurachmanov > > > <david.abdurachma...@gmail.com> wrote: > > > > > > > > On Tue, Feb 18, 2020 at 10:38 PM Tom Rini <tr...@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 <tr...@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 <tr...@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.pa...@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.pa...@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?)
You are correct. The bigger issue is what if kernel is already loaded at a predefined address by previous stage loader. We will stick to kernel_comp_size for now. I will update the patch. 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 -- Regards, Atish