Message-ID: <CAGXv+5Gt5B35OEvQva7sh=MEHU=QYGD3fWzxbK_ZT-K13pf81w@mail.gmail.com>
Date: Thu, 13 Jun 2024 16:36:00 +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 Thu, Jun 13, 2024 at 3:45 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
>
> 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

    bytes(''.join(f'fdt-{x}\x00' for x in files), "ascii")

Seems to work.

ChenYu

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