Re: [PATCH 3/3] arm64: boot: Support Flat Image Tree

From: Simon Glass <sjg@chromium.org>
To: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	U-Boot Mailing List <u-boot@lists.denx.de>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Nick Terrell <terrelln@fb.com>,
	Nicolas Schier <nicolas@fjasle.eu>, Will Deacon <will@kernel.org>,
	linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] arm64: boot: Support Flat Image Tree
Date: Sun, 29 Oct 2023 05:46:12 +1300	[thread overview]
Message-ID: <CAFLszThguWT0u0R0EHfpBro0f-pWDwLOGk+5pQZEVhFYNKH8fQ@mail.gmail.com> (raw)
In-Reply-To: <CAK7LNASATGRaS-6QxzqTEq7qNVkZPXOBE8pfRBg=2bQGyy3=yw@mail.gmail.com>

Hi Masahiro,

On Fri, Oct 27, 2023 at 2:25 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Thu, Oct 26, 2023 at 4:28 PM Simon Glass <sjg@chromium.org> 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.
> >
> > The files compress from about 85MB 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.
>
>
> Is this useful?
> For arm64, initrd is likely used.

It depends on your kernel configuration, though. I have found it
convenient to enable the drivers necessary to boot.

>
> FIT should be created in unbrellea projects
> such as OpenEmbedded,  Buildroot, etc.

Sure, but that is a separate issue, isn't it? We already support
various boot targets in arm64 but not one that includes the DTs, so
far as I can see. The old arm 'uImage' target is pretty out-of-date
now.

>
>
>
>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  MAINTAINERS              |   7 +
> >  arch/arm64/Makefile      |   3 +-
> >  arch/arm64/boot/Makefile |   8 +-
> >  scripts/Makefile.lib     |  16 ++-
> >  scripts/make_fit.py      | 285 +++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 315 insertions(+), 4 deletions(-)
> >  create mode 100755 scripts/make_fit.py
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2d13bbd69adb..d6955ebc3c24 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1575,6 +1575,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 <sjg@chromium.org>
> > +L:     linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> > +S:     Maintained
> > +F:     arch/arm64/boot/Makefile
> > +F:     scripts/make_fit.py
> > +
> >  ARM ARCHITECTED TIMER DRIVER
> >  M:     Mark Rutland <mark.rutland@arm.com>
> >  M:     Marc Zyngier <maz@kernel.org>
> > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > index 7b77b63e978f..d8290dcab6b6 100644
> > --- a/arch/arm64/Makefile
> > +++ b/arch/arm64/Makefile
> > @@ -150,7 +150,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.gz vmlinuz.efi
> > +BOOT_TARGETS   := Image.gz vmlinuz.efi image.fit
> >
> >  PHONY += $(BOOT_TARGETS)
> >
> > @@ -215,6 +215,7 @@ virtconfig:
> >  define archhelp
> >    echo  '* Image.gz      - Compressed kernel image (arch/$(ARCH)/boot/Image.gz)'
> >    echo  '  Image         - Uncompressed kernel image (arch/$(ARCH)/boot/Image)'
> > +$(if $(CONFIG_EFI_ZBOOT),,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/Makefile b/arch/arm64/boot/Makefile
> > index 1761f5972443..a6e5b20b22bd 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,11 @@ $(obj)/Image.lzo: $(obj)/Image FORCE
> >  $(obj)/Image.zst: $(obj)/Image FORCE
> >         $(call if_changed,zstd)
> >
> > +ifndef CONFIG_EFI_ZBOOT
> > +$(obj)/image.fit: $(obj)/Image $(obj)/dts FORCE
>
>
>
> This is wrong.
>
> $(obj)/dts is a directory.
>
> There is no point for comparing timestamps
> between $(obj)/image.fit and $(obj)/dts.
>
> Updates of *.dtb do not result in the
> update of the $(obj)/dts timestamp.
>
>
> if_changed never works correctly.

Yes I am aware that this doesn't actually deal with changes. I'll see
if I can figure out another way.

>
>
>
>
>
>
>
>
> > +       $(call if_changed,fit,gzip)
> > +endif
> > +
> >  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 68d0134bdbf9..4e4364ad641a 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -487,14 +487,26 @@ UIMAGE_OPTS-y ?=
> >  UIMAGE_TYPE ?= kernel
> >  UIMAGE_LOADADDR ?= arch_must_set_this
> >  UIMAGE_ENTRYADDR ?= $(UIMAGE_LOADADDR)
> > -UIMAGE_NAME ?= 'Linux-$(KERNELRELEASE)'
> > +UIMAGE_NAME ?= "Linux-$(KERNELRELEASE)"
>
>
> Unneeded noise change.

I'll move that to a separate patch. We don't want the single quotes in the name.

Regards,
Simon