LinuxLists.cc - [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

2023-11-29 17:23:14

Subject: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

Add a script which produces a Flat Image Tree (FIT), a single file
containing the built kernel and associated devicetree files.
Compression defaults to gzip which gives a good balance of size and
performance.

The files compress from about 86MB to 24MB using this approach.

The FIT can be used by bootloaders which support it, such as U-Boot
and Linuxboot. It permits automatic selection of the correct
devicetree, matching the compatible string of the running board with
the closest compatible string in the FIT. There is no need for
filenames or other workarounds.

Add a 'make image.fit' build target for arm64, as well.

The FIT can be examined using 'dumpimage -l'.

This features requires pylibfdt (use 'pip install libfdt'). It also
requires compression utilities for the algorithm being used. Supported
compression options are the same as the Image.xxx files. For now there
is no way to change the compression other than by editing the rule for
$(obj)/image.fit

While FIT supports a ramdisk / initrd, no attempt is made to support
this here, since it must be built separately from the Linux build.

Signed-off-by: Simon Glass <[email protected]>
---

Changes in v7:
- Add Image as a dependency of image.fit
- Drop kbuild tag
- Add dependency on dtbs
- Drop unnecessary path separator for dtbs
- Rebase to -next

Changes in v5:
- Drop patch previously applied
- Correct compression rule which was broken in v4

Changes in v4:
- Use single quotes for UIMAGE_NAME

Changes in v3:
- Drop temporary file image.itk
- Drop patch 'Use double quotes for image name'
- Drop double quotes in use of UIMAGE_NAME
- Drop unnecessary CONFIG_EFI_ZBOOT condition for help
- Avoid hard-coding "arm64" for the DT architecture

Changes in v2:
- Drop patch previously applied
- Add .gitignore file
- Move fit rule to Makefile.lib using an intermediate file
- Drop dependency on CONFIG_EFI_ZBOOT
- Pick up .dtb files separately from the kernel
- Correct pylint too-many-args warning for write_kernel()
- Include the kernel image in the file count
- Add a pointer to the FIT spec and mention of its wide industry usage
- Mention the kernel version in the FIT description

MAINTAINERS | 7 +
arch/arm64/Makefile | 9 +-
arch/arm64/boot/.gitignore | 1 +
arch/arm64/boot/Makefile | 6 +-
scripts/Makefile.lib | 13 ++
scripts/make_fit.py | 289 +++++++++++++++++++++++++++++++++++++
6 files changed, 322 insertions(+), 3 deletions(-)
create mode 100755 scripts/make_fit.py

diff --git a/MAINTAINERS b/MAINTAINERS
index 14587be87a33..d609f0e8deb3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1585,6 +1585,13 @@ F: Documentation/process/maintainer-soc*.rst
F: arch/arm/boot/dts/Makefile
F: arch/arm64/boot/dts/Makefile

+ARM64 FIT SUPPORT
+M: Simon Glass <[email protected]>
+L: [email protected] (moderated for non-subscribers)
+S: Maintained
+F: arch/arm64/boot/Makefile
+F: scripts/make_fit.py
+
ARM ARCHITECTED TIMER DRIVER
M: Mark Rutland <[email protected]>
M: Marc Zyngier <[email protected]>
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 1bd4fae6e806..18e092de7cdb 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -36,6 +36,8 @@ ifeq ($(CONFIG_BROKEN_GAS_INST),y)
$(warning Detected assembler with broken .inst; disassembly will be unreliable)
endif

+KBUILD_DTBS := dtbs
+
KBUILD_CFLAGS += -mgeneral-regs-only \
$(compat_vdso) $(cc_has_k_constraint)
KBUILD_CFLAGS += $(call cc-disable-warning, psabi)
@@ -150,7 +152,7 @@ libs-$(CONFIG_EFI_STUB) += $(objtree)/drivers/firmware/efi/libstub/lib.a
# Default target when executing plain make
boot := arch/arm64/boot

-BOOT_TARGETS := Image vmlinuz.efi
+BOOT_TARGETS := Image vmlinuz.efi image.fit

PHONY += $(BOOT_TARGETS)

@@ -162,7 +164,9 @@ endif

all: $(notdir $(KBUILD_IMAGE))

-vmlinuz.efi: Image
+image.fit: $(KBUILD_DTBS)
+
+vmlinuz.efi image.fit: Image
$(BOOT_TARGETS): vmlinux
$(Q)$(MAKE) $(build)=$(boot) $(boot)/$@

@@ -215,6 +219,7 @@ virtconfig:
define archhelp
echo '* Image.gz - Compressed kernel image (arch/$(ARCH)/boot/Image.gz)'
echo ' Image - Uncompressed kernel image (arch/$(ARCH)/boot/Image)'
+ echo ' image.fit - Flat Image Tree (arch/$(ARCH)/boot/image.fit)'
echo ' install - Install uncompressed kernel'
echo ' zinstall - Install compressed kernel'
echo ' Install using (your) ~/bin/installkernel or'
diff --git a/arch/arm64/boot/.gitignore b/arch/arm64/boot/.gitignore
index af5dc61f8b43..abaae9de1bdd 100644
--- a/arch/arm64/boot/.gitignore
+++ b/arch/arm64/boot/.gitignore
@@ -2,3 +2,4 @@
Image
Image.gz
vmlinuz*
+image.fit
diff --git a/arch/arm64/boot/Makefile b/arch/arm64/boot/Makefile
index 1761f5972443..8d591fda078f 100644
--- a/arch/arm64/boot/Makefile
+++ b/arch/arm64/boot/Makefile
@@ -16,7 +16,8 @@

OBJCOPYFLAGS_Image :=-O binary -R .note -R .note.gnu.build-id -R .comment -S

-targets := Image Image.bz2 Image.gz Image.lz4 Image.lzma Image.lzo Image.zst
+targets := Image Image.bz2 Image.gz Image.lz4 Image.lzma Image.lzo \
+ Image.zst image.fit

$(obj)/Image: vmlinux FORCE
$(call if_changed,objcopy)
@@ -39,6 +40,9 @@ $(obj)/Image.lzo: $(obj)/Image FORCE
$(obj)/Image.zst: $(obj)/Image FORCE
$(call if_changed,zstd)

+$(obj)/image.fit: $(obj)/Image FORCE
+ $(call cmd,fit,gzip)
+
EFI_ZBOOT_PAYLOAD := Image
EFI_ZBOOT_BFD_TARGET := elf64-littleaarch64
EFI_ZBOOT_MACH_TYPE := ARM64
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 1a965fe68e01..e1c06ca3c847 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -496,6 +496,19 @@ quiet_cmd_uimage = UIMAGE $@
-a $(UIMAGE_LOADADDR) -e $(UIMAGE_ENTRYADDR) \
-n '$(UIMAGE_NAME)' -d $< $@

+# Flat Image Tree (FIT)
+# This allows for packaging of a kernel and all devicetrees files, using
+# compression.
+# ---------------------------------------------------------------------------
+
+MAKE_FIT := $(srctree)/scripts/make_fit.py
+
+quiet_cmd_fit = FIT $@
+ cmd_fit = $(MAKE_FIT) -f $@ --arch $(UIMAGE_ARCH) --os linux \
+ --name '$(UIMAGE_NAME)' \
+ --compress $(UIMAGE_COMPRESSION) -k $< \
+ $(dir $<)dts
+
# XZ
# ---------------------------------------------------------------------------
# Use xzkern to compress the kernel image and xzmisc to compress other things.
diff --git a/scripts/make_fit.py b/scripts/make_fit.py
new file mode 100755
index 000000000000..e1059825de9c
--- /dev/null
+++ b/scripts/make_fit.py
@@ -0,0 +1,289 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright 2023 Google LLC
+# Written by Simon Glass <[email protected]>
+#
+
+"""Build a FIT containing a lot of devicetree files
+
+Usage:
+ make_fit.py -A arm64 -n 'Linux-6.6' -O linux
+ -f arch/arm64/boot/image.fit -k /tmp/kern/arch/arm64/boot/image.itk
+ /tmp/kern/arch/arm64/boot/dts/ -E -c gzip
+
+Creates a FIT containing the supplied kernel and a directory containing the
+devicetree files.
+
+Use -E to generate an external FIT (where the data is placed after the
+FIT data structure). This allows parsing of the data without loading
+the entire FIT.
+
+Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
+zstd
+
+The resulting FIT can be booted by bootloaders which support FIT, such
+as U-Boot, Linuxboot, Tianocore, etc.
+
+Note that this tool does not yet support adding a ramdisk / initrd.
+"""
+
+import argparse
+import collections
+import os
+import subprocess
+import sys
+import tempfile
+import time
+
+import libfdt
+
+
+# Tool extension and the name of the command-line tools
+CompTool = collections.namedtuple('CompTool', 'ext,tools')
+
+COMP_TOOLS = {
+ 'bzip2': CompTool('.bz2', 'bzip2'),
+ 'gzip': CompTool('.gz', 'pigz,gzip'),
+ 'lz4': CompTool('.lz4', 'lz4'),
+ 'lzma': CompTool('.lzma', 'lzma'),
+ 'lzo': CompTool('.lzo', 'lzop'),
+ 'zstd': CompTool('.zstd', 'zstd'),
+}
+
+def parse_args():
+ """Parse the program ArgumentParser
+
+ Returns:
+ Namespace object containing the arguments
+ """
+ epilog = 'Build a FIT from a directory tree containing .dtb files'
+ parser = argparse.ArgumentParser(epilog=epilog)
+ parser.add_argument('-A', '--arch', type=str, required=True,
+ help='Specifies the architecture')
+ parser.add_argument('-c', '--compress', type=str, default='none',
+ help='Specifies the compression')
+ parser.add_argument('-E', '--external', action='store_true',
+ help='Convert the FIT to use external data')
+ parser.add_argument('-n', '--name', type=str, required=True,
+ help='Specifies the name')
+ parser.add_argument('-O', '--os', type=str, required=True,
+ help='Specifies the operating system')
+ parser.add_argument('-f', '--fit', type=str, required=True,
+ help='Specifies the output file (.fit)')
+ parser.add_argument('-k', '--kernel', type=str, required=True,
+ help='Specifies the (uncompressed) kernel input file (.itk)')
+ parser.add_argument('srcdir', type=str, nargs='*',
+ help='Specifies the directory tree that contains .dtb files')
+
+ return parser.parse_args()
+
+def setup_fit(fsw, name):
+ """Make a start on writing the FIT
+
+ Outputs the root properties and the 'images' node
+
+ Args:
+ fsw (libfdt.FdtSw): Object to use for writing
+ name (str): Name of kernel image
+ """
+ fsw.INC_SIZE = 65536
+ fsw.finish_reservemap()
+ fsw.begin_node('')
+ fsw.property_string('description', f'{name} with devicetree set')
+ fsw.property_u32('#address-cells', 1)
+
+ fsw.property_u32('timestamp', int(time.time()))
+ fsw.begin_node('images')
+
+
+def write_kernel(fsw, data, args):
+ """Write out the kernel image
+
+ Writes a kernel node along with the required properties
+
+ Args:
+ fsw (libfdt.FdtSw): Object to use for writing
+ data (bytes): Data to write (possibly compressed)
+ args (Namespace): Contains necessary strings:
+ arch: FIT architecture, e.g. 'arm64'
+ fit_os: Operating Systems, e.g. 'linux'
+ name: Name of OS, e.g. 'Linux-6.6.0-rc7'
+ compress: Compression algorithm to use, e.g. 'gzip'
+ """
+ with fsw.add_node('kernel'):
+ fsw.property_string('description', args.name)
+ fsw.property_string('type', 'kernel_noload')
+ fsw.property_string('arch', args.arch)
+ fsw.property_string('os', args.os)
+ fsw.property_string('compression', args.compress)
+ fsw.property('data', data)
+ fsw.property_u32('load', 0)
+ fsw.property_u32('entry', 0)
+
+
+def finish_fit(fsw, entries):
+ """Finish the FIT ready for use
+
+ Writes the /configurations node and subnodes
+
+ Args:
+ fsw (libfdt.FdtSw): Object to use for writing
+ entries (list of tuple): List of configurations:
+ str: Description of model
+ str: Compatible stringlist
+ """
+ fsw.end_node()
+ seq = 0
+ with fsw.add_node('configurations'):
+ for model, compat in entries:
+ seq += 1
+ with fsw.add_node(f'conf-{seq}'):
+ fsw.property('compatible', bytes(compat))
+ fsw.property_string('description', model)
+ fsw.property_string('fdt', f'fdt-{seq}')
+ fsw.property_string('kernel', 'kernel')
+ fsw.end_node()
+
+
+def compress_data(inf, compress):
+ """Compress data using a selected algorithm
+
+ Args:
+ inf (IOBase): Filename containing the data to compress
+ compress (str): Compression algorithm, e.g. 'gzip'
+
+ Return:
+ bytes: Compressed data
+ """
+ if compress == 'none':
+ return inf.read()
+
+ comp = COMP_TOOLS.get(compress)
+ if not comp:
+ raise ValueError(f"Unknown compression algorithm '{compress}'")
+
+ with tempfile.NamedTemporaryFile() as comp_fname:
+ with open(comp_fname.name, 'wb') as outf:
+ done = False
+ for tool in comp.tools.split(','):
+ try:
+ subprocess.call([tool, '-c'], stdin=inf, stdout=outf)
+ done = True
+ break
+ except FileNotFoundError:
+ pass
+ if not done:
+ raise ValueError(f'Missing tool(s): {comp.tools}\n')
+ with open(comp_fname.name, 'rb') as compf:
+ comp_data = compf.read()
+ return comp_data
+
+
+def output_dtb(fsw, seq, fname, arch, compress):
+ """Write out a single devicetree to the FIT
+
+ Args:
+ fsw (libfdt.FdtSw): Object to use for writing
+ seq (int): Sequence number (1 for first)
+ fmame (str): Filename containing the DTB
+ arch: FIT architecture, e.g. 'arm64'
+ compress (str): Compressed algorithm, e.g. 'gzip'
+
+ Returns:
+ tuple:
+ str: Model name
+ bytes: Compatible stringlist
+ """
+ with fsw.add_node(f'fdt-{seq}'):
+ # Get the compatible / model information
+ with open(fname, 'rb') as inf:
+ data = inf.read()
+ fdt = libfdt.FdtRo(data)
+ model = fdt.getprop(0, 'model').as_str()
+ compat = fdt.getprop(0, 'compatible')
+
+ fsw.property_string('description', model)
+ fsw.property_string('type', 'flat_dt')
+ fsw.property_string('arch', arch)
+ fsw.property_string('compression', compress)
+ fsw.property('compatible', bytes(compat))
+
+ with open(fname, 'rb') as inf:
+ compressed = compress_data(inf, compress)
+ fsw.property('data', compressed)
+ return model, compat
+
+
+def build_fit(args):
+ """Build the FIT from the provided files and arguments
+
+ Args:
+ args (Namespace): Program arguments
+
+ Returns:
+ tuple:
+ bytes: FIT data
+ int: Number of configurations generated
+ size: Total uncompressed size of data
+ """
+ fsw = libfdt.FdtSw()
+ setup_fit(fsw, args.name)
+ seq = 0
+ size = 0
+ entries = []
+
+ # Handle the kernel
+ with open(args.kernel, 'rb') as inf:
+ comp_data = compress_data(inf, args.compress)
+ size += os.path.getsize(args.kernel)
+ write_kernel(fsw, comp_data, args)
+
+ for path in args.srcdir:
+ # Handle devicetree files
+ if os.path.isdir(path):
+ for dirpath, _, fnames in os.walk(path):
+ for fname in fnames:
+ if os.path.splitext(fname)[1] != '.dtb':
+ continue
+ pathname = os.path.join(dirpath, fname)
+ seq += 1
+ size += os.path.getsize(pathname)
+ model, compat = output_dtb(fsw, seq, pathname,
+ args.arch, args.compress)
+ entries.append([model, compat])
+
+ finish_fit(fsw, entries)
+
+ # Include the kernel itself in the returned file count
+ return fsw.as_fdt().as_bytearray(), seq + 1, size
+
+
+def run_make_fit():
+ """Run the tool's main logic"""
+ args = parse_args()
+
+ out_data, count, size = build_fit(args)
+ with open(args.fit, 'wb') as outf:
+ outf.write(out_data)
+
+ ext_fit_size = None
+ if args.external:
+ mkimage = os.environ.get('MKIMAGE', 'mkimage')
+ subprocess.check_call([mkimage, '-E', '-F', args.fit],
+ stdout=subprocess.DEVNULL)
+
+ with open(args.fit, 'rb') as inf:
+ data = inf.read()
+ ext_fit = libfdt.FdtRo(data)
+ ext_fit_size = ext_fit.totalsize()
+
+ comp_size = len(out_data)
+ print(f'FIT size {comp_size:#x}/{comp_size / 1024 / 1024:.1f} MB', end='')
+ if ext_fit_size:
+ print(f', header {ext_fit_size:#x}/{ext_fit_size / 1024:.1f} KB', end='')
+ print(f', {count} files, uncompressed {size / 1024 / 1024:.1f} MB')
+
+
+if __name__ == "__main__":
+ sys.exit(run_make_fit())
--
2.43.0.rc2.451.g8631bc7472-goog

2023-11-29 18:35:23

by Ahmad Fatoum

[permalink] [raw]

Subject: Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

Hello Simon,

On 29.11.23 18:21, Simon Glass wrote:
> Add a script which produces a Flat Image Tree (FIT), a single file
> containing the built kernel and associated devicetree files.
> Compression defaults to gzip which gives a good balance of size and
> performance.

Thanks for working on this. I think it's useful to have the kernel
generate a FIT image out of the box. More complex use cases are always
free to call mkimage with a custom ITS.

> The files compress from about 86MB to 24MB using this approach.
>
> The FIT can be used by bootloaders which support it, such as U-Boot
> and Linuxboot. It permits automatic selection of the correct
> devicetree, matching the compatible string of the running board with
> the closest compatible string in the FIT. There is no need for
> filenames or other workarounds.
>
> Add a 'make image.fit' build target for arm64, as well.

not that it matters much, but should this maybe called Image.fit
as the other Image types are capitalized too?

> EFI_ZBOOT_PAYLOAD := Image
> EFI_ZBOOT_BFD_TARGET := elf64-littleaarch64
> EFI_ZBOOT_MACH_TYPE := ARM64
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 1a965fe68e01..e1c06ca3c847 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -496,6 +496,19 @@ quiet_cmd_uimage = UIMAGE $@
> -a $(UIMAGE_LOADADDR) -e $(UIMAGE_ENTRYADDR) \
> -n '$(UIMAGE_NAME)' -d $< $@

Doesn't hardcoding a load address and entry address here defeat the point
of having FIT as generic portable image format?

At least barebox will try to place the kernel image at physical address 0 and
will exit with an error message if no SDRAM is located at that address.
The recommendation in that case is to omit load and entry address altogether
to have barebox find a suitable location, but I see now that the FIT specification
requires a load and entry address. What would happen if U-Boot tries to load this
FIT image on a board that has no DRAM at address 0?

Please Cc me on subsequent revisions. I am interested in testing that this works for barebox
too.

Thanks,
Ahmad

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2023-11-29 18:59:23

by Ahmad Fatoum

[permalink] [raw]

Subject: Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

Hi,

a few more comments after decompiling the FIT image:

On 29.11.23 18:21, Simon Glass wrote:
> + with fsw.add_node('kernel'):
> + fsw.property_string('description', args.name)
> + fsw.property_string('type', 'kernel_noload')

The specification only says no loading done, but doesn't explain what it
means for a bootloader to _not_ load an image. Looking into the U-Boot commit
b9b50e89d317 ("image: Implement IH_TYPE_KERNEL_NOLOAD") that introduces this,
apparently no loading means ignoring load and entry address?

I presume missing load and entry is something older U-Boot versions
were unhappy about? Please let me know if the semantics are as I understood,
so I can prepare a barebox patch supporting it.

> + fsw.property_string('arch', args.arch)
> + fsw.property_string('os', args.os)
> + fsw.property_string('compression', args.compress)
> + fsw.property('data', data)
> + fsw.property_u32('load', 0)
> + fsw.property_u32('entry', 0)
> +
> +
> +def finish_fit(fsw, entries):
> + """Finish the FIT ready for use
> +
> + Writes the /configurations node and subnodes
> +
> + Args:
> + fsw (libfdt.FdtSw): Object to use for writing
> + entries (list of tuple): List of configurations:
> + str: Description of model
> + str: Compatible stringlist
> + """
> + fsw.end_node()
> + seq = 0
> + with fsw.add_node('configurations'):
> + for model, compat in entries:
> + seq += 1
> + with fsw.add_node(f'conf-{seq}'):
> + fsw.property('compatible', bytes(compat))

The specification says that this is the root U-Boot compatible,
which I presume to mean the top-level compatible, which makes sense to me.

The code here though adds all compatible strings from the device tree though,
is this intended?

> + fsw.property_string('description', model)
> + fsw.property_string('type', 'flat_dt')
> + fsw.property_string('arch', arch)
> + fsw.property_string('compression', compress)
> + fsw.property('compatible', bytes(compat))

I think I've never seen a compatible for a fdt node before.
What use does this serve?

Cheers,
Ahmad

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2023-11-29 19:02:48

by Simon Glass

[permalink] [raw]

Subject: Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

Hi Ahmad,

On Wed, 29 Nov 2023 at 11:35, Ahmad Fatoum <[email protected]> wrote:
>
> Hello Simon,
>
> On 29.11.23 18:21, Simon Glass wrote:
> > Add a script which produces a Flat Image Tree (FIT), a single file
> > containing the built kernel and associated devicetree files.
> > Compression defaults to gzip which gives a good balance of size and
> > performance.
>
> Thanks for working on this. I think it's useful to have the kernel
> generate a FIT image out of the box. More complex use cases are always
> free to call mkimage with a custom ITS.
>
>
> > The files compress from about 86MB to 24MB using this approach.
> >
> > The FIT can be used by bootloaders which support it, such as U-Boot
> > and Linuxboot. It permits automatic selection of the correct
> > devicetree, matching the compatible string of the running board with
> > the closest compatible string in the FIT. There is no need for
> > filenames or other workarounds.
> >
> > Add a 'make image.fit' build target for arm64, as well.
>
> not that it matters much, but should this maybe called Image.fit
> as the other Image types are capitalized too?
>
> > EFI_ZBOOT_PAYLOAD := Image
> > EFI_ZBOOT_BFD_TARGET := elf64-littleaarch64
> > EFI_ZBOOT_MACH_TYPE := ARM64
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 1a965fe68e01..e1c06ca3c847 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -496,6 +496,19 @@ quiet_cmd_uimage = UIMAGE $@
> > -a $(UIMAGE_LOADADDR) -e $(UIMAGE_ENTRYADDR) \
> > -n '$(UIMAGE_NAME)' -d $< $@
>
> Doesn't hardcoding a load address and entry address here defeat the point
> of having FIT as generic portable image format?
>
> At least barebox will try to place the kernel image at physical address 0 and
> will exit with an error message if no SDRAM is located at that address.
> The recommendation in that case is to omit load and entry address altogether
> to have barebox find a suitable location, but I see now that the FIT specification
> requires a load and entry address. What would happen if U-Boot tries to load this
> FIT image on a board that has no DRAM at address 0?

The 'kernel_noload' type indicates that the load/exec address are ignored.

>
> Please Cc me on subsequent revisions. I am interested in testing that this works for barebox
> too.

There has been some discussion about this recently in U-Boot too,
along with a series [1] which you could try if you like.

The FIT spec[2] does not provide enough detail on exactly what
kernel_noload means and we should improve this at some point.

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=382849
[2] https://github.com/open-source-firmware/flat-image-tree

>
> Thanks,
> Ahmad
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>

2023-11-29 19:03:27

On Wed, Nov 29, 2023 at 08:16:20PM +0100, Ahmad Fatoum wrote:
> Hello Tom,
>
> On 29.11.23 20:02, Tom Rini wrote:
> > On Wed, Nov 29, 2023 at 07:59:00PM +0100, Ahmad Fatoum wrote:
> >> Hi,
> >>
> >> a few more comments after decompiling the FIT image:
> >>
> >> On 29.11.23 18:21, Simon Glass wrote:
> >>> + with fsw.add_node('kernel'):
> >>> + fsw.property_string('description', args.name)
> >>> + fsw.property_string('type', 'kernel_noload')
> >>
> >> The specification only says no loading done, but doesn't explain what it
> >> means for a bootloader to _not_ load an image. Looking into the U-Boot commit
> >> b9b50e89d317 ("image: Implement IH_TYPE_KERNEL_NOLOAD") that introduces this,
> >> apparently no loading means ignoring load and entry address?
> >>
> >> I presume missing load and entry is something older U-Boot versions
> >> were unhappy about? Please let me know if the semantics are as I understood,
> >> so I can prepare a barebox patch supporting it.
> >
> > So the matching side for this series in U-Boot is:
> > https://patchwork.ozlabs.org/project/uboot/list/?series=382849&state=*
> >
> > And in short, for IH_TYPE_KERNEL_NOLOAD we do our best to use it
> > in-place. For decompression we allocate some space to decompress to.
>
> Thanks. I am still curious why "kernel" couldn't have been used back then
> with missing entry and load address to arrive at the same result?

Some level or another of historical oversight, yeah.

--
Tom

Attachments:

(No filename) (1.48 kB)
signature.asc (673.00 B)
Download all attachments

2023-11-29 20:35:30

by Ahmad Fatoum

[permalink] [raw]

Subject: Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

Hi Simon,

On 29.11.23 20:00, Simon Glass wrote:
> On Wed, 29 Nov 2023 at 11:35, Ahmad Fatoum <[email protected]> wrote:
>> Doesn't hardcoding a load address and entry address here defeat the point
>> of having FIT as generic portable image format?
>>
>> At least barebox will try to place the kernel image at physical address 0 and
>> will exit with an error message if no SDRAM is located at that address.
>> The recommendation in that case is to omit load and entry address altogether
>> to have barebox find a suitable location, but I see now that the FIT specification
>> requires a load and entry address. What would happen if U-Boot tries to load this
>> FIT image on a board that has no DRAM at address 0?
>
> The 'kernel_noload' type indicates that the load/exec address are ignored.

Can the script not insert load/exec addresses with dummy values to avoid confusion?

>> Please Cc me on subsequent revisions. I am interested in testing that this works for barebox
>> too.
>
> There has been some discussion about this recently in U-Boot too,
> along with a series [1] which you could try if you like.

Thanks for the pointer. I have just sent out a first patch to add support
for kernel_noload to the barebox mailing list[1]. With that change applied,
barebox can boot the FIT images generated by this series.

Once that's accepted, I'll reply with a Tested-by.

[1]: https://lore.barebox.org/barebox/[email protected]/T/#u

> The FIT spec[2] does not provide enough detail on exactly what
> kernel_noload means and we should improve this at some point.

Yes, that would be nice. Also straight references to e.g. U-Boot configuration
symbols could use some rewording.

Thanks,
Ahmad

>
> Regards,
> Simon
>
> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=382849
> [2] https://github.com/open-source-firmware/flat-image-tree
>
>
>
>>
>> Thanks,
>> Ahmad
>>
>> --
>> Pengutronix e.K. | |
>> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>>
>

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2023-11-30 08:26:55

Hello,

On 04.12.23 18:52, Doug Anderson wrote:> On Sat, Dec 2, 2023 at 8:37 AM Simon Glass <[email protected]> wrote:
>> On Thu, 30 Nov 2023 at 19:04, Ahmad Fatoum <[email protected]> wrote:
>>> On 30.11.23 21:30, Simon Glass wrote:
>>>> On Wed, 29 Nov 2023 at 12:54, Ahmad Fatoum <[email protected]> wrote:
>>>>> On 29.11.23 20:44, Simon Glass wrote:
>>>> I don't have an example to hand, but this is the required mechanism of
>>>> FIT. This feature has been in place for many years and is used by
>>>> ChromeOS, at least.
>>>
>>> I see the utility of a FIT configuration with
>>>
>>> compatible = "vendor,board-rev-a", "vendor,board-rev-b";
>>>
>>> I fail to see a utility for a configuration with
>>>
>>> compatible = "vendor,board", "vendor,SoM", "vendor,SoC";
>>>
>>> Any configuration that ends up being booted because "vendor,SoC" was matched is
>>> most likely doomed to fail. Therefore, I would suggest that only the top level
>>> configuration is written into the FIT configurations automatically.
>>
>> Firstly, I am not an expert on this.
>>
>> Say you have a board with variants. The compatible string in U-Boot
>> may be something like:
>>
>> "google,veyron-brain-rev1", "google,veyron-brain", "google,veyron",
>> "rockchip,rk3288";
>>
>> If you then have several FIT configurations, they may be something like:
>>
>> "google,veyron-brain-rev0", "google,veyron-brain", "google,veyron",
>> "rockchip,rk3288";
>> "google,veyron-brain-rev1", "google,veyron-brain", "google,veyron",
>> "rockchip,rk3288";
>> "google,veyron-brain-rev2", "google,veyron-brain", "google,veyron",
>> "rockchip,rk3288";
>>
>> You want to choose the second one, since it is a better match than the others.

Now imagine, you are building a kernel that has no DT support for the Veyron,
but instead has support for the Phytec RK3288 PCM-947:

phytec,rk3288-pcm-947", "phytec,rk3288-phycore-som", "rockchip,rk3288

As far as I understand U-Boot code, A veyron U-Boot would boot the Phytec DT
if CONFIG_FIT_BEST_MATCH is set, although it's a bad match and a boot failure
should rather have occurred.

>> +Doug Anderson who knows a lot more about this than me.
>
> Hopefully this is all explained by:
>
> https://docs.kernel.org/arch/arm/google/chromebook-boot-flow.html

Thanks Doug, this was helpful. The missing information to me was that
depthcharge only compares the top-level board compatible in addition
to runtime generated board compatibles, unlike what U-Boot seems to do.

barebox only compares its top-level compatible against the FIT configuration
compatibles, so adding a full compatible list to the configuration nodes as done
by this series should be ok there as well. I think U-Boot could run into
issues though as described above.

Out of curiosity: I only heard about Depthcharge before as exploitation toolkit
for U-Boot. Can you point me at some documentation on what the Depthcharge bootloader
does what U-Boot (which was presumably used before?) doesn't?

Thanks,
Ahmad

>

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2023-12-05 17:08:01

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

Hi,

On Tue, Dec 5, 2023 at 3:16 AM Ahmad Fatoum <[email protected]> wrote:
>
> Hello,
>
> On 04.12.23 18:52, Doug Anderson wrote:> On Sat, Dec 2, 2023 at 8:37 AM Simon Glass <[email protected]> wrote:
> >> On Thu, 30 Nov 2023 at 19:04, Ahmad Fatoum <[email protected]> wrote:
> >>> On 30.11.23 21:30, Simon Glass wrote:
> >>>> On Wed, 29 Nov 2023 at 12:54, Ahmad Fatoum <[email protected]> wrote:
> >>>>> On 29.11.23 20:44, Simon Glass wrote:
> >>>> I don't have an example to hand, but this is the required mechanism of
> >>>> FIT. This feature has been in place for many years and is used by
> >>>> ChromeOS, at least.
> >>>
> >>> I see the utility of a FIT configuration with
> >>>
> >>> compatible = "vendor,board-rev-a", "vendor,board-rev-b";
> >>>
> >>> I fail to see a utility for a configuration with
> >>>
> >>> compatible = "vendor,board", "vendor,SoM", "vendor,SoC";
> >>>
> >>> Any configuration that ends up being booted because "vendor,SoC" was matched is
> >>> most likely doomed to fail. Therefore, I would suggest that only the top level
> >>> configuration is written into the FIT configurations automatically.
> >>
> >> Firstly, I am not an expert on this.
> >>
> >> Say you have a board with variants. The compatible string in U-Boot
> >> may be something like:
> >>
> >> "google,veyron-brain-rev1", "google,veyron-brain", "google,veyron",
> >> "rockchip,rk3288";
> >>
> >> If you then have several FIT configurations, they may be something like:
> >>
> >> "google,veyron-brain-rev0", "google,veyron-brain", "google,veyron",
> >> "rockchip,rk3288";
> >> "google,veyron-brain-rev1", "google,veyron-brain", "google,veyron",
> >> "rockchip,rk3288";
> >> "google,veyron-brain-rev2", "google,veyron-brain", "google,veyron",
> >> "rockchip,rk3288";
> >>
> >> You want to choose the second one, since it is a better match than the others.
>
> Now imagine, you are building a kernel that has no DT support for the Veyron,
> but instead has support for the Phytec RK3288 PCM-947:
>
> phytec,rk3288-pcm-947", "phytec,rk3288-phycore-som", "rockchip,rk3288
>
> As far as I understand U-Boot code, A veyron U-Boot would boot the Phytec DT
> if CONFIG_FIT_BEST_MATCH is set, although it's a bad match and a boot failure
> should rather have occurred.

On depthcharge the bootloader never matches on just a SoC name.

> >> +Doug Anderson who knows a lot more about this than me.
> >
> > Hopefully this is all explained by:
> >
> > https://docs.kernel.org/arch/arm/google/chromebook-boot-flow.html
>
> Thanks Doug, this was helpful. The missing information to me was that
> depthcharge only compares the top-level board compatible in addition
> to runtime generated board compatibles, unlike what U-Boot seems to do.
>
> barebox only compares its top-level compatible against the FIT configuration
> compatibles, so adding a full compatible list to the configuration nodes as done
> by this series should be ok there as well. I think U-Boot could run into
> issues though as described above.
>
> Out of curiosity: I only heard about Depthcharge before as exploitation toolkit
> for U-Boot. Can you point me at some documentation on what the Depthcharge bootloader
> does what U-Boot (which was presumably used before?) doesn't?

I can only assume that the depthcharge you're talking about is
different. The one used by Chromebooks is basically:

https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/main

I assume you're asking: why are we using depthcharge in ChromeOS
instead of U-Boot?

There was much discussion about this back in the day. From what I
recall, part of the reason was that folks wanted the boot flow to be a
bit more standard between x86 Chromebooks and ARM Chromebooks. x86
Chromebooks were using coreboot for the initial hardware booting and
then needed a 2nd stage to actually load Linux and ended up creating
depthcharge. ...and then we switched to the same model for ARM boards.

I didn't personally make that decision and I'm also not on the
firmware team, so that's about all I'll say on the topic. ;-)

Oh, hmmm. Searching found me:

https://www.chromium.org/chromium-os/developer-information-for-chrome-os-devices/custom-firmware/

...which pointed at:

https://www.chromium.org/chromium-os/2014-firmware-summit/ChromeOS%20firmware%20summit%20-%20Depthcharge.pdf

-Doug