[RFC] binman: add support for creating dummy files for external blobs

[Heiko Thiery]

Hi Simon,

Am Di., 23. Nov. 2021 um 17:15 Uhr schrieb Simon Glass <sjg at chromium.org>:
>
> Hi Heiko,
>
> On Tue, 16 Nov 2021 at 03:47, Heiko Thiery <heiko.thiery at gmail.com> wrote:
> >
> > While converting to binman for an imx8mq board, it has been found that
> > building in the u-boot CI fails. This is because an imx8mq requires an
> > external binary (signed_hdmi_imx8m.bin). If this file cannot be found
> > mkimage fails.
> > To be able to build this board in the u-boot CI a binman option
> > (--fake-ext-blobs) is introduced that can be switched on via the u-boot
> > makefile option BINMAN_FAKE_EXT_BLOBS. With that the needed dummy files are
> > created.
> >
> > Signed-off-by: Heiko Thiery <heiko.thiery at gmail.com>
> > ---
> >  Makefile                       |  1 +
> >  tools/binman/cmdline.py        |  2 ++
> >  tools/binman/control.py        | 16 ++++++++++++++++
> >  tools/binman/entry.py          |  1 +
> >  tools/binman/etype/blob_ext.py |  1 +
> >  5 files changed, 21 insertions(+)
>
> This seems like a reasonable solution to me.
>
> >
> > diff --git a/Makefile b/Makefile
> > index f911f70344..1a833a1637 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1307,6 +1307,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
> >                 -a tpl-bss-pad=$(if $(CONFIG_TPL_SEPARATE_BSS),,1) \
> >                 -a spl-dtb=$(CONFIG_SPL_OF_REAL) \
> >                 -a tpl-dtb=$(CONFIG_SPL_OF_REAL) \
> > +               $(if $(BINMAN_FAKE_EXT_BLOBS),--fake-ext-blobs) \
> >                 $(BINMAN_$(@F))

This will only work if we add the BINMAN_FAKE_EXT_BLOBS=y to the make
call as argument/option. This means we also have to change the
buildman for the CI. Or do you know if it is also possible to use an
environment variable to achieve this? Then we could simply set the env
var for the CI builds.

currently:
make CROSS_COMPILE=aarch64-linux-gnu-  -j4 BINMAN_FAKE_EXT_BLOBS=y

nice to have
export BINMAN_FAKE_EXT_BLOBS=y
make CROSS_COMPILE=aarch64-linux-gnu-  -j4

What do you think?

> >
> >  OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex
> > diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py
> > index d6156df408..2b29981cb4 100644
> > --- a/tools/binman/cmdline.py
> > +++ b/tools/binman/cmdline.py
> > @@ -52,6 +52,8 @@ controlled by a description in the board device tree.'''
> >              help='Configuration file (.dtb) to use')
> >      build_parser.add_argument('--fake-dtb', action='store_true',
> >              help='Use fake device tree contents (for testing only)')
> > +    build_parser.add_argument('--fake-ext-blobs', action='store_true',
> > +            help='Create fake ext blobs with dummy content (for testing only)')
> >      build_parser.add_argument('-i', '--image', type=str, action='append',
> >              help='Image filename to build (if not specified, build all)')
> >      build_parser.add_argument('-I', '--indir', action='append',
> > diff --git a/tools/binman/control.py b/tools/binman/control.py
> > index 0dbcbc28e9..f95aadd1f3 100644
> > --- a/tools/binman/control.py
> > +++ b/tools/binman/control.py
> > @@ -8,6 +8,7 @@
> >  from collections import OrderedDict
> >  import glob
> >  import os
> > +import pathlib
> >  import pkg_resources
> >  import re
> >
> > @@ -401,6 +402,17 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths,
> >      return image
> >
> >
> > +def PrepareFakeExtBlobs(images):
>
> Please add a function comment
>
> > +    for (_, image) in images.items():
> > +        for entry in image._entries.values():
> > +            if entry.allow_dummy:
> > +                if not pathlib.Path(entry._filename).is_file():
> > +                    tout.Warning("Missing external blob '%s', fake it" %
> > +                                 entry._filename)
> > +                    with open(entry._filename, "wb") as out:
> > +                        out.truncate(1024)
>
> Won't this create files for all external blobs that are missing? Is
> that the intent? If so, we can do this in Entry_blob.ObtainContents().

Yes this will create the dummy files for all external blobs.

>
> Should we use a size of 0? Or will that make some tools complain?

mkimage will complain about files that are too small.

> > +
> > +
> >  def PrepareImagesAndDtbs(dtb_fname, select_images, update_fdt, use_expanded):
> >      """Prepare the images to be processed and select the device tree
> >
> > @@ -629,6 +641,10 @@ def Binman(args):
> >
> >              images = PrepareImagesAndDtbs(dtb_fname, args.image,
> >                                            args.update_fdt, use_expanded)
> > +
> > +            if args.fake_ext_blobs:
> > +                PrepareFakeExtBlobs(images)
>
> This should go in ProcessImage(). See the SetAllowMissing()  and see
> if you can follow the same pattern. Basically we tell each section
> that it should allow missing blobs, then check that in
> Entry_blot.ObtainContents()

I changed this to the proposed. I had to add this also to the
'Entry_mkimage' when I add the external blob inside this node.

spl {
  filename = "spl.bin";

  mkimage {
    args = "-n spl/u-boot-spl.cfgout -T imx8mimage -e 0x7e1000";

    blob {
      filename = "u-boot-spl-ddr.bin";
    };
    hdmi_fw: blob-ext at 5 {
      filename = "signed_hdmi_imx8m.bin";
    };
  };
};


> > +
> >              if args.test_section_timeout:
> >                  # Set the first image to timeout, used in testThreadTimeout()
> >                  images[list(images.keys())[0]].test_section_timeout = True
> > diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> > index 70222718ea..2f5c38d15a 100644
> > --- a/tools/binman/entry.py
> > +++ b/tools/binman/entry.py
> > @@ -100,6 +100,7 @@ class Entry(object):
> >          self.missing = False
> >          self.external = False
> >          self.allow_missing = False
> > +        self.allow_dummy = False
>
> Please add a comment about this above.

Ok.

>
> >
> >      @staticmethod
> >      def Lookup(node_path, etype, expanded):
> > diff --git a/tools/binman/etype/blob_ext.py b/tools/binman/etype/blob_ext.py
> > index d6b0ca17c3..85bc9fe6b8 100644
> > --- a/tools/binman/etype/blob_ext.py
> > +++ b/tools/binman/etype/blob_ext.py
> > @@ -26,3 +26,4 @@ class Entry_blob_ext(Entry_blob):
> >      def __init__(self, section, etype, node):
> >          Entry_blob.__init__(self, section, etype, node)
> >          self.external = True
> > +        self.allow_dummy = True
> > --
> > 2.30.2
> >
>
> Also please run 'binman test -T' as you need a test for this.

Currently I'm struggling doing that.

-- 
Heiko