Re: [PATCH v2 10/25] binman: Refactor fit to generate output at the end

[Simon Glass <sjg@chromium.org>]

Hi Alper,

On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 24/02/2022 02:00, Simon Glass wrote:
> > At present the fit implementation creates the output tree while
> > scanning the FIT description. Then it updates the tree later when the
> > data is known.
> >
> > This works, but is a bit confusing, since it requires mixing the scanning
> > code with the generation code, with a fix-up step at the end.
> >
> > It is actually possible to do this in two phases, one to scan everything
> > and the other to generate the FIT. Thus the FIT is generated in one pass,
> > when everything is known.
>
> Doing it in one go makes sense to me as well. In general I like the way
> distinct processing actions/steps are being split into their own blocks
> or so, and I think this helps move things toward that.

OK good.

>
> > Update the code accordingly. The only functional change is that the 'data'
> > property for each node are now last instead of first, which is really a
> > more natural position. Update the affected test to deal with this.
> >
> > One wrinkle is that the calculated properties (image-pos, size and offset)
> > are now added before the FIT is generated. so we must filter these out
> > when copying properties from the binman description to the FIT.
> >
> > Most of the change here is splitting out some of the code from the
> > ReadEntries() implementation into _BuildInput(). So despite the large
> > diff, most of the code is the same. It is not feasible to split this patch
> > up, so far as I can tell.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Add new patch to refactor fit to generate output at the end
> >
> >  tools/binman/etype/fit.py              | 178 ++++++++++++++-----------
> >  tools/binman/ftest.py                  |  13 +-
> >  tools/binman/test/224_fit_bad_oper.dts |   2 -
> >  3 files changed, 109 insertions(+), 84 deletions(-)
>
> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>
> I still wrote some weird ideas below, mostly for the future, since this
> patch is mostly moving code around which is fine as is.
>
> > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> > index 2d4c5f6545..61c72780e9 100644
> > --- a/tools/binman/etype/fit.py
> > +++ b/tools/binman/etype/fit.py
> > @@ -209,6 +209,81 @@ class Entry_fit(Entry_section):
> >          return oper
> >
> >      def ReadEntries(self):
> > +        def _add_entries(base_node, depth, node):
> > +            """Add entries for any nodes that need them
> > +
> > +            Args:
> > +                base_node: Base Node of the FIT (with 'description' property)
> > +                depth: Current node depth (0 is the base 'fit' node)
> > +                node: Current node to process
> > +
> > +            He we only need to provide binman entries which are used to define
>
> He -> Here ?
>
> > +            the 'data' for each image. We create an entry_Section for each.
> > +            """
> > +            rel_path = node.path[len(base_node.path):]
> > +            in_images = rel_path.startswith('/images')
> > +            has_images = depth == 2 and in_images
> > +            if has_images:
> > +                # This node is a FIT subimage node (e.g. "/images/kernel")
> > +                # containing content nodes. We collect the subimage nodes and
> > +                # section entries for them here to merge the content subnodes
> > +                # together and put the merged contents in the subimage node's
> > +                # 'data' property later.
> > +                entry = Entry.Create(self.section, node, etype='section')
> > +                entry.ReadNode()
>
> I plan to change 'self.section' to 'self' here later, fixes extracting
> wrong contents for FIT subentries.

OK

>
> > +                # The hash subnodes here are for mkimage, not binman.
> > +                entry.SetUpdateHash(False)
> > +                self._entries[rel_path] = entry
> > +
>
> I also plan to change this to a single-level node name instead of the
> relative path, lets 'binman extract fit/u-boot' etc. run at all.

That would be good.

>
> > +            for subnode in node.subnodes:
> > +                _add_entries(base_node, depth + 1, subnode)
> > +
> > +        _add_entries(self._node, 0, self._node)
>
> I think it's especially visible here what I meant by switching away from
> recursion: this recurses through every node but only does anything on
> immediate subnodes of "/images" (for now?).

Fair enough.

>
> > +
> > +    def BuildSectionData(self, required):
> > +        """Build FIT entry contents
> > +
> > +        This adds the 'data' properties to the input ITB (Image-tree Binary)
> > +        then runs mkimage to process it.
> > +
> > +        Args:
> > +            required: True if the data must be present, False if it is OK to
> > +                return None
>
> I forgot to handle 'required' while converting FIT to section...
>
> > +
> > +        Returns:
> > +            Contents of the section (bytes)
> > +        """
> > +        data = self._BuildInput()
> > +        uniq = self.GetUniqueName()
> > +        input_fname = tools.get_output_filename('%s.itb' % uniq)
> > +        output_fname = tools.get_output_filename('%s.fit' % uniq)
> > +        tools.write_file(input_fname, data)
> > +        tools.write_file(output_fname, data)
> > +
> > +        args = {}
> > +        ext_offset = self._fit_props.get('fit,external-offset')
> > +        if ext_offset is not None:
> > +            args = {
> > +                'external': True,
> > +                'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
> > +                }
> > +        if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
> > +                            **args) is None:
>
> I have an idea for the far future, to let /image/* nodes sometimes be
> Entry_collection to handle external offsets in binman so we can take
> mkimage completely out of this, but no clue how feasible/desirable that
> end goal is.

Well I would really prefer to avoid duplicating mkimage functionality.
I think it is something to look at when binman is more widespread, but
as you can see we are still sorting out the FIT issues.

>
> > +            # Bintool is missing; just use empty data as the output
> > +            self.record_missing_bintool(self.mkimage)
> > +            return tools.get_bytes(0, 1024)
> > +
> > +        return tools.read_file(output_fname)
> > +
> > +    def _BuildInput(self):
> > +        """Finish the FIT by adding the 'data' properties to it
> > +
> > +        Arguments:
> > +            fdt: FIT to update
> > +
> > +        Returns:
> > +            New fdt contents (bytes)
> > +        """
> >          def _process_prop(pname, prop):
> >              """Process special properties
> >
> > @@ -236,9 +311,15 @@ class Entry_fit(Entry_section):
> >                      val = val[1:].replace('DEFAULT-SEQ', str(seq + 1))
> >                      fsw.property_string(pname, val)
> >                      return
> > +            elif pname.startswith('fit,'):
> > +                # Ignore these, which are commands for binman to process
> > +                return
> > +            elif pname in ['offset', 'size', 'image-pos']:
> > +                # Don't add binman's calculated properties
> > +                return
>
> This is one of the things I was thinking of doing, thanks. I encountered
> the same issue when replacing a FIT entry with the same f"{uniq}.fit"
> that was used to build it, will try adding a test for that later.

Yes good.

>
> >              fsw.property(pname, prop.bytes)
> >
> > -        def _scan_gen_fdt_nodes(subnode, depth, in_images):
> > +        def _gen_fdt_nodes(subnode, depth, in_images):
> >              """Generate FDT nodes
> >
> >              This creates one node for each member of self._fdts using the
> > [...]
> >
> > diff --git a/tools/binman/test/224_fit_bad_oper.dts b/tools/binman/test/224_fit_bad_oper.dts
> > index cee801e2ea..8a8014ea33 100644
> > --- a/tools/binman/test/224_fit_bad_oper.dts
> > +++ b/tools/binman/test/224_fit_bad_oper.dts
> > @@ -21,7 +21,5 @@
> >                               };
> >                       };
> >               };
> > -             fdtmap {
> > -             };
>
> This looked unrelated at first, but as I understand it, changing the
> order of things made the bad-operation error happen later and exposed a
> breakage from the fdtmap entry trying to parse the mock dtb data.

Yes, that's it.

Regards,
Simon