Message-ID: <CAGXv+5EWqzj3w=KaKBfviBXPms0baKx28S0n+bGZcLc3=fGK+g@mail.gmail.com>
Date: Thu, 13 Jun 2024 15:45:29 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Simon Glass <sjg@...omium.org>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH] scripts/make_fit: Support decomposing DTBs

On Wed, Jun 12, 2024 at 4:01 AM Simon Glass <sjg@...omium.org> wrote:
>
> Hi Chen-Yu,
>
> On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@...omium.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@...omium.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@...omium.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.

Seems reasonable. I'll make it use the capital -D then.

> >      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.

Me neither. Switching the format to an f string doesn't work:

  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 324, in <module>
    sys.exit(run_make_fit())
             ^^^^^^^^^^^^^^
  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 298, in run_make_fit
    out_data, count, size = build_fit(args)
                            ^^^^^^^^^^^^^^^
  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 288, in build_fit
    finish_fit(fsw, entries)
  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 161, in finish_fit
    fsw.property('fdt', b''.join([f'fdt-%d\x00' % x for x in files]))
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: sequence item 0: expected a bytes-like object, str found

> But do you need the inner [] ?

Nope. Will remove.

>
> >                  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 +

Will fix.

> > +            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?

I'll see what I can do. In that case I'll drop your Reviewed-by.


Thanks
ChenYu

> > +
> > +        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

Powered by blists - more mailing lists