Re: [RFC PATCH v2 1/8] binman: mkimage: Support ':'-separated inputs

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 16/05/2022 14:07, Andrew Abbott wrote:
> mkimage supports combining multiple input binaries separating them
> with colons (':'). This is used at least for Rockchip platforms to
> encode payload offsets and sizes in the image header. It is required for
> Rockchip SPI boot since for the rkspi format, after the input binary
> combining, the entire image is spread across only the first 2K bytes of
> each 4K block.
> 
> Previous to this change, multiple inputs to a binman mkimage node would
> just be concatenated and the combined input would be passed as the -d
> argument to mkimage. Now, the inputs are instead passed colon-separated.
> 
> Signed-off-by: Andrew Abbott <andrew@mirx.dev>

Also see another attempt for this [1] and the comments to that for a
more complete picture, though I'll try writing all the points here anyway.

[1] binman: support mkimage separate files
https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipeout@gmail.com/

> ---
> This is a bit of a messy implementation for now and would probably break
> existing uses of mkimage that rely on the concatenation behaviour.

I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like
anything uses it yet. Except for binman/test/156_mkimage.dts, which
doesn't exactly test the concatenation.

> Questions:
> - Should this be a separate entry type, or an option to the mkimage
>   entry type that enables this behaviour?

You can add a 'separate-files' device-tree property like in [1]. I'm
actually OK with this separate-files being the default and only behavior
(concatenation can be done via an inner section), but I'm not sure Simon
would agree.

> - What kind of test(s) should I add?

At the minimum, a test using separate-files with multiple input entries.
You can do something like the _HandleGbbCommand in ftest.py to capture
and check the arguments that'll be passed to mkimage.

I think that'll be enough, but try to run `binman test -T` and check for
100% coverage with all tests succeeding.

> (no changes since v1)
> 
>  tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index 5f6def2287..8cea618fbd 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -51,21 +51,30 @@ class Entry_mkimage(Entry):

Expand the docstring with an explanation of the new behavior, and run
`binman entry-docs >tools/binman/entries.rst` to update it there as well.

>          self.ReadEntries()
>  
>      def ObtainContents(self):
> -        # Use a non-zero size for any fake files to keep mkimage happy
> -        data, input_fname, uniq = self.collect_contents_to_file(
> -            self._mkimage_entries.values(), 'mkimage', 1024)
> -        if data is None:
> -            return False
> -        output_fname = tools.get_output_filename('mkimage-out.%s' % uniq)
> -        if self.mkimage.run_cmd('-d', input_fname, *self._args,
> -                                output_fname) is not None:
> +        # For multiple inputs to mkimage, we want to separate them by colons.
> +        # This is needed for eg. the rkspi format, which treats the first data
> +        # file as the "init" and the second as "boot" and sets the image header
> +        # accordingly, then makes the image so that only the first 2 KiB of each
> +        # 4KiB block is used.
> +
> +        data_filenames = []
> +        for entry in self._mkimage_entries.values():
> +            # First get the input data and put it in a file. If any entry is not
> +            # available, try later.
> +            if not entry.ObtainContents():
> +                return False
> +
> +            input_fname = tools.get_output_filename('mkimage-in.%s' % entry.GetUniqueName())
> +            data_filenames.append(input_fname)
> +            tools.write_file(input_fname, entry.GetData())

Something like collect_contents_to_file([entry], f'mkimage-in-{idx}',
1024) would be better here. At least, the files must not be empty (or
mkimage exits with an error), where entry.GetData() can be b''.

> +
> +        output_fname = tools.get_output_filename('mkimage-out.%s' % self.GetUniqueName())

Should be an f-string.

> +        if self.mkimage.run_cmd('-d', ":".join(data_filenames), *self._args, output_fname):
>              self.SetContents(tools.read_file(output_fname))
> +            return True
>          else:
> -            # Bintool is missing; just use the input data as the output
>              self.record_missing_bintool(self.mkimage)
> -            self.SetContents(data)
> -
> -        return True
> +            return False

This case must set some dummy contents (I'd guess b'' is fine) and
return True. (False here roughly means "try again later".)

>  
>      def ReadEntries(self):
>          """Read the subnodes to find out what should go in this image"""