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

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

Questions:
- Should this be a separate entry type, or an option to the mkimage
  entry type that enables this behaviour?
- What kind of test(s) should I add?

(no changes since v1)

 tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

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

On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote:
> 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.

I did similar and came to the same conclusion, but I figured I'd ask
just in case it's used somewhere I hadn't found, or it's being relied
on outside of the U-Boot repository.

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

Similar thoughts here, especially since we couldn't find anything
relying on the concatenation behaviour.

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

Thanks for your review! I'll make these updates in the next version.