Received-SPF: pass (google.com: domain of linux-kernel+bounces-210552-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240605094843.4141730-1-wenst@chromium.org>
In-Reply-To: <20240605094843.4141730-1-wenst@chromium.org>
From: Simon Glass <sjg@chromium.org>
Date: Tue, 11 Jun 2024 14:01:07 -0600
Message-ID: <CAFLszTiZ8PUvxHx4kfLZf18RqSczRwxCmCxZ_y6J2rpu03pA=w@mail.gmail.com>
Subject: Re: [PATCH] scripts/make_fit: Support decomposing DTBs
To: Chen-Yu Tsai <wenst@chromium.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>, Nathan Chancellor <nathan@kernel.org>, 
	Nicolas Schier <nicolas@fjasle.eu>, linux-arm-kernel@lists.infradead.org, 
	linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi Chen-Yu,

On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> The kernel tree builds some "composite" DTBs, where the final DTB is the
> result of applying one or more DTB overlays on top of a base DTB with
> fdtoverlay.
>
> The FIT image specification already supports configurations having one
> base DTB and overlays applied on top. It is then up to the bootloader to
> apply said overlays and either use or pass on the final result. This
> allows the FIT image builder to reuse the same FDT images for multiple
> configurations, if such cases exist.
>
> The decomposition function depends on the kernel build system, reading
> back the .cmd files for the to-be-packaged DTB files to check for the
> fdtoverlay command being called. This will not work outside the kernel
> tree. The function is off by default to keep compatibility with possible
> existing users.
>
> To facilitate the decomposition and keep the code clean, the model and
> compatitble string extraction have been moved out of the output_dtb
> function. The FDT image description is replaced with the base file name
> of the included image.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This is a feature I alluded to in my replies to Simon's original
> submission of the make_fit.py script [1].
>
> This is again made a runtime argument as not all firmware out there
> that boot FIT images support applying overlays. Like my previous
> submission for disabling compression for included FDT images, the
> bootloader found in RK3399 and MT8173 Chromebooks do not support
> applying overlays. Another case of this is U-boot shipped by development
> board vendors in binary form (without upstream) in an image or in
> SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> These would fail to boot FIT images with DT overlays. One such
> example is my Hummingboard Pulse. In these cases the firmware is
> either not upgradable or very hard to upgrade.
>
> I believe there is value in supporting these cases. A common script
> shipped with the kernel source that can be shared by distros means
> the distro people don't have to reimplement this in their downstream
> repos or meta-packages. For ChromeOS this means reducing the amount
> of package code we have in shell script.
>
> [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> [2]
>
>  scripts/Makefile.lib |  1 +
>  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 49 insertions(+), 22 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Some possible nits / changes below

>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 9f06f6aaf7fc..d78b5d38beaa 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
>        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
>                 --name '$(UIMAGE_NAME)' \
>                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
>                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
>
>  # XZ
> diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> index 263147df80a4..120f13e1323c 100755
> --- a/scripts/make_fit.py
> +++ b/scripts/make_fit.py
> @@ -22,6 +22,11 @@ the entire FIT.
>  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
>  zstd algorithms.
>
> +Use -d to decompose "composite" DTBs into their base components and
> +deduplicate the resulting base DTBs and DTB overlays. This requires the
> +DTBs to be sourced from the kernel build directory, as the implementation
> +looks at the .cmd files produced by the kernel build.
> +
>  The resulting FIT can be booted by bootloaders which support FIT, such
>  as U-Boot, Linuxboot, Tianocore, etc.
>
> @@ -64,6 +69,8 @@ def parse_args():
>            help='Specifies the architecture')
>      parser.add_argument('-c', '--compress', type=str, default='none',
>            help='Specifies the compression')
> +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> +          help='Decompose composite DTBs into base DTB and overlays')

I wonder if we should reserve -d for --debug? I don't have a strong
opinion though.

>      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,
> @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
>      fsw.end_node()
>      seq = 0
>      with fsw.add_node('configurations'):
> -        for model, compat in entries:
> +        for model, compat, files 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('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))

This looks right to me. It would be nice to use an f string but I
don't know how to do that with bytes.

But do you need the inner [] ?

>                  fsw.property_string('kernel', 'kernel')
>      fsw.end_node()
>
> @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
>          fname (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('description', os.path.basename(fname))
>          fsw.property_string('type', 'flat_dt')
>          fsw.property_string('arch', arch)
>          fsw.property_string('compression', compress)
> @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
>          with open(fname, 'rb') as inf:
>              compressed = compress_data(inf, compress)
>          fsw.property('data', compressed)
> -    return model, compat
>
>
>  def build_fit(args):
> @@ -235,6 +229,7 @@ def build_fit(args):
>      fsw = libfdt.FdtSw()
>      setup_fit(fsw, args.name)
>      entries = []
> +    fdts = collections.OrderedDict()
>
>      # Handle the kernel
>      with open(args.kernel, 'rb') as inf:
> @@ -243,12 +238,43 @@ def build_fit(args):
>      write_kernel(fsw, comp_data, args)
>
>      for fname in args.dtbs:
> -        # Ignore overlay (.dtbo) files
> -        if os.path.splitext(fname)[1] == '.dtb':
> -            seq += 1
> -            size += os.path.getsize(fname)
> -            model, compat = output_dtb(fsw, seq, fname, args.arch, args.compress)
> -            entries.append([model, compat])
> +        # Ignore non-DTB (*.dtb) files
> +        if os.path.splitext(fname)[1] != '.dtb':
> +            continue
> +
> +        # 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')
> +
> +        if args.decompose_dtbs:
> +            # Check if the DTB needs to be decomposed
> +            path, basename = os.path.split(fname)
> +            cmd_fname = os.path.join(path, f'.{basename}.cmd')
> +            with open(cmd_fname, 'r', encoding='ascii') as inf:
> +                cmd = inf.read()
> +
> +            if 'scripts/dtc/fdtoverlay' in cmd:
> +                # This depends on the structure of the composite DTB command
> +                files = cmd.split()
> +                files = files[files.index('-i')+1:]

spaces around +

> +            else:
> +                files = [fname]
> +        else:
> +            files = [fname]

I do wonder if the code from '# Get the compatible' to here would be
better in a separate, documented function, to keep things easier to
understand?

> +
> +        for fn in files:
> +            if fn not in fdts:
> +                seq += 1
> +                size += os.path.getsize(fn)
> +                output_dtb(fsw, seq, fn, args.arch, args.compress)
> +                fdts[fn] = seq
> +
> +        files_seq = [fdts[fn] for fn in files]
> +
> +        entries.append([model, compat, files_seq])
>
>      finish_fit(fsw, entries)
>
> --
> 2.45.1.288.g0e0cd299f1-goog
>

Regards,
Simon