[PATCH v2 19/25] binman: Keep a separate list of entries for fit

2022-02-23 Thread Simon Glass
The current implementation sets up the FIT entries but then deletes the
'generator' ones so they don't appear in the final image.

This is a bit clumsy. We cannot build the image more than once, since the
generator entries are lost during the first build. Binman requires that
calling BuildSectionData() multiple times returns a valid result each
time.

Keep a separate, private list which includes the generator nodes and use
that where needed, to correct this problem. Ensure that the missing list
includes removed generator entries too.

Signed-off-by: Simon Glass 
---

(no changes since v1)

 tools/binman/etype/fit.py | 33 -
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index e1b056f56e..30b20a07a2 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -163,10 +163,15 @@ class Entry_fit(Entry_section):
 key: relative path to entry Node (from the base of the FIT)
 value: Entry_section object comprising the contents of this
 node
+_priv_entries: Internal copy of _entries which includes 'generator'
+entries which are used to create the FIT, but should not be
+processed as real entries. This is set up once we have the
+entries
 """
 super().__init__(section, etype, node)
 self._fit = None
 self._fit_props = {}
+self._priv_entries = {}
 
 for pname, prop in self._node.props.items():
 if pname.startswith('fit,'):
@@ -239,6 +244,10 @@ class Entry_fit(Entry_section):
 
 _add_entries(self._node, 0, self._node)
 
+# Keep a copy of all entries, including generator entries, since these
+# removed from self._entries later.
+self._priv_entries = dict(self._entries)
+
 def BuildSectionData(self, required):
 """Build FIT entry contents
 
@@ -415,11 +424,12 @@ class Entry_fit(Entry_section):
 
 has_images = depth == 2 and in_images
 if has_images:
-entry = self._entries[rel_path]
+entry = self._priv_entries[rel_path]
 data = entry.GetData()
 fsw.property('data', bytes(data))
 
 for subnode in node.subnodes:
+subnode_path = f'{rel_path}/{subnode.name}'
 if has_images and not (subnode.name.startswith('hash') or
subnode.name.startswith('signature')):
 # This subnode is a content node not meant to appear in
@@ -427,11 +437,11 @@ class Entry_fit(Entry_section):
 # fsw.add_node() or _add_node() for it.
 pass
 elif self.GetImage().generate and subnode.name.startswith('@'):
-subnode_path = f'{rel_path}/{subnode.name}'
-entry = self._entries.get(subnode_path)
 _gen_node(base_node, subnode, depth, in_images)
-if entry:
-del self._entries[subnode_path]
+# This is a generator (template) entry, so remove it from
+# the list of entries used by PackEntries(), etc. Otherwise
+# it will appear in the binman output
+to_remove.append(subnode_path)
 else:
 with fsw.add_node(subnode.name):
 _add_node(base_node, depth + 1, subnode)
@@ -440,10 +450,16 @@ class Entry_fit(Entry_section):
 # entry node
 fsw = libfdt.FdtSw()
 fsw.finish_reservemap()
+to_remove = []
 with fsw.add_node(''):
 _add_node(self._node, 0, self._node)
 fdt = fsw.as_fdt()
 
+# Remove generator entries from the main list
+for path in to_remove:
+if path in self._entries:
+del self._entries[path]
+
 # Pack this new FDT and scan it so we can add the data later
 fdt.pack()
 data = fdt.as_bytearray()
@@ -503,3 +519,10 @@ class Entry_fit(Entry_section):
 def AddBintools(self, btools):
 super().AddBintools(btools)
 self.mkimage = self.AddBintool(btools, 'mkimage')
+
+def CheckMissing(self, missing_list):
+# We must use our private entry list for this since generator notes
+# which are removed from self._entries will otherwise not show up as
+# missing
+for entry in self._priv_entries.values():
+entry.CheckMissing(missing_list)
-- 
2.35.1.574.g5d30c73bfb-goog



Re: [PATCH v2 19/25] binman: Keep a separate list of entries for fit

2022-03-03 Thread Alper Nebi Yasak
On 24/02/2022 02:00, Simon Glass wrote:
> The current implementation sets up the FIT entries but then deletes the
> 'generator' ones so they don't appear in the final image.

They still show up in the fdtmap if I add one to rockchip-u-boot.dtsi:

  $ binman ls -i u-boot-rockchip.bin
  NameImage-pos  Size Entry-typeOffset
  -
  main-section0   103520  section 0
mkimage   01a000  mkimage 0
fit   1a000e8d74  fit 1a000
  u-boot  1a644b1898  section   644
u-boot-nodtb  1a644b1898  u-boot-nodtb0
  @atf-SEQ00  section 0
atf-bl31  00  atf-bl310
  @tee-SEQ00  section 0
tee-os00  tee-os  0
  @fdt-SEQ00  section 0
fdtmap   102d74  79c  fdtmap 102d74

But not simple-bin.map:

  ImagePosOffset  Size  Name
      00103520  main-section
       0001a000  mkimage
  0001a000   0001a000  000e8d74  fit
  0001a6440644  000b1898  u-boot
  0001a644   000b1898  u-boot-nodtb
  00102d74   00102d74  079c  fdtmap

This happens in v1 as well, but I hadn't checked it then.

(The "fit" size and "fdtmap" offset are off by 0x10 too, but I'm not
sure why yet.)

> This is a bit clumsy. We cannot build the image more than once, since the
> generator entries are lost during the first build. Binman requires that
> calling BuildSectionData() multiple times returns a valid result each
> time.

I think the generator entries should be turned into concrete entries and
be removed in _gen_entries(), so BuildSectionData() should work on the
entries that were generated. This way the individual entries would show
up in fdtmap and could be binman-extracted/replaced as well.

For FDTs we can generate blob entries for each file, for ELFs maybe we
can split them into files like the script used to do (bl31_0x.bin)
and do the same?

Maybe some new entry types, "data" for arbitrary data unrelated to a
file whose contents we can set, or "elf-segment" that can extract a
segment from an ELF file (but I don't like the information loss there).

> Keep a separate, private list which includes the generator nodes and use
> that where needed, to correct this problem. Ensure that the missing list
> includes removed generator entries too.
> 
> Signed-off-by: Simon Glass 
> ---
> 
> (no changes since v1)

I'm more and more convinced that generator nodes needs a thorough
redesign, but the patch is consistent with status quo, so:

Reviewed-by: Alper Nebi Yasak 

>  tools/binman/etype/fit.py | 33 -
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index e1b056f56e..30b20a07a2 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -163,10 +163,15 @@ class Entry_fit(Entry_section):
>  key: relative path to entry Node (from the base of the FIT)
>  value: Entry_section object comprising the contents of this
>  node
> +_priv_entries: Internal copy of _entries which includes 
> 'generator'
> +entries which are used to create the FIT, but should not be
> +processed as real entries. This is set up once we have the
> +entries

Maybe this could be "_templates" or "_entry_generators" and only keep
track of the generator entries.

>  """
>  super().__init__(section, etype, node)
>  self._fit = None
>  self._fit_props = {}
> +self._priv_entries = {}
>  
>  for pname, prop in self._node.props.items():
>  if pname.startswith('fit,'):
>  
> [...]


Re: [PATCH v2 19/25] binman: Keep a separate list of entries for fit

2022-03-05 Thread Simon Glass
Hi Alper,

On Thu, 3 Mar 2022 at 14:17, Alper Nebi Yasak  wrote:
>
> On 24/02/2022 02:00, Simon Glass wrote:
> > The current implementation sets up the FIT entries but then deletes the
> > 'generator' ones so they don't appear in the final image.
>
> They still show up in the fdtmap if I add one to rockchip-u-boot.dtsi:
>
>   $ binman ls -i u-boot-rockchip.bin
>   NameImage-pos  Size Entry-typeOffset
>   -
>   main-section0   103520  section 0
> mkimage   01a000  mkimage 0
> fit   1a000e8d74  fit 1a000
>   u-boot  1a644b1898  section   644
> u-boot-nodtb  1a644b1898  u-boot-nodtb0
>   @atf-SEQ00  section 0
> atf-bl31  00  atf-bl310
>   @tee-SEQ00  section 0
> tee-os00  tee-os  0
>   @fdt-SEQ00  section 0
> fdtmap   102d74  79c  fdtmap 102d74
>
> But not simple-bin.map:
>
>   ImagePosOffset  Size  Name
>       00103520  main-section
>        0001a000  mkimage
>   0001a000   0001a000  000e8d74  fit
>   0001a6440644  000b1898  u-boot
>   0001a644   000b1898  u-boot-nodtb
>   00102d74   00102d74  079c  fdtmap
>
> This happens in v1 as well, but I hadn't checked it then.
>
> (The "fit" size and "fdtmap" offset are off by 0x10 too, but I'm not
> sure why yet.)

Well let's fix this after the series. We need tests for the map and so on.

>
> > This is a bit clumsy. We cannot build the image more than once, since the
> > generator entries are lost during the first build. Binman requires that
> > calling BuildSectionData() multiple times returns a valid result each
> > time.
>
> I think the generator entries should be turned into concrete entries and
> be removed in _gen_entries(), so BuildSectionData() should work on the
> entries that were generated. This way the individual entries would show
> up in fdtmap and could be binman-extracted/replaced as well.

That makes sense, but I'm not sure how to implement it. The split-elf
thing needs the contents of the entries which is not available at the
start. It is likely available before BuildSectionData() is called, but
not necessarily. So the template needs to hang around at least as long
as that.

I think there is something we could do here, but it isn't quite clear
to me. Perhaps we need a expand-nodes-based-on-contents phase in
control.py ? Eek...

>
> For FDTs we can generate blob entries for each file, for ELFs maybe we
> can split them into files like the script used to do (bl31_0x.bin)
> and do the same?

I'm not a big fan of adding files. Binman should be able to hold
everything in memory. It does generate files for later inspection
though, so yes we could add this feature.

>
> Maybe some new entry types, "data" for arbitrary data unrelated to a
> file whose contents we can set, or "elf-segment" that can extract a
> segment from an ELF file (but I don't like the information loss there).

Yes I quite like the idea of new entry types. In fact I was hoping to
turn split-elf into an entry type (one that generated multiple
entries). But I decided to stop before doing that since we really need
to gets the code in there, fix the bugs /move forward with some of the
ideas you and others have.

>
> > Keep a separate, private list which includes the generator nodes and use
> > that where needed, to correct this problem. Ensure that the missing list
> > includes removed generator entries too.
> >
> > Signed-off-by: Simon Glass 
> > ---
> >
> > (no changes since v1)
>
> I'm more and more convinced that generator nodes needs a thorough
> redesign, but the patch is consistent with status quo, so:
>
> Reviewed-by: Alper Nebi Yasak 
>
> >  tools/binman/etype/fit.py | 33 -
> >  1 file changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> > index e1b056f56e..30b20a07a2 100644
> > --- a/tools/binman/etype/fit.py
> > +++ b/tools/binman/etype/fit.py
> > @@ -163,10 +163,15 @@ class Entry_fit(Entry_section):
> >  key: relative path to entry Node (from the base of the FIT)
> >  value: Entry_section object comprising the contents of this
> >  node
> > +_priv_entries: Internal copy of _entries which includes 
> > 'generator'
> > +entries which are used to create the FIT, but should not be
> > +processed as real entries. This is set up once we have the
> > +entries
>
> Maybe this could be "_templates" or "_entry_generators" and only keep
> track of the generator 

Re: [PATCH v2 19/25] binman: Keep a separate list of entries for fit

2022-03-10 Thread Alper Nebi Yasak
On 06/03/2022 06:08, Simon Glass wrote:
> On Thu, 3 Mar 2022 at 14:17, Alper Nebi Yasak  
> wrote:
>>> This is a bit clumsy. We cannot build the image more than once, since the
>>> generator entries are lost during the first build. Binman requires that
>>> calling BuildSectionData() multiple times returns a valid result each
>>> time.
>>
>> I think the generator entries should be turned into concrete entries and
>> be removed in _gen_entries(), so BuildSectionData() should work on the
>> entries that were generated. This way the individual entries would show
>> up in fdtmap and could be binman-extracted/replaced as well.
> 
> That makes sense, but I'm not sure how to implement it. The split-elf
> thing needs the contents of the entries which is not available at the
> start. It is likely available before BuildSectionData() is called, but
> not necessarily. So the template needs to hang around at least as long
> as that.
> 
> I think there is something we could do here, but it isn't quite clear
> to me. Perhaps we need a expand-nodes-based-on-contents phase in
> control.py ? Eek...

I can't think of anything proper. I guess it could need architectural
changes, but I need to read more of the code and get a better grasp of
things.

>>
>> For FDTs we can generate blob entries for each file, for ELFs maybe we
>> can split them into files like the script used to do (bl31_0x.bin)
>> and do the same?
> 
> I'm not a big fan of adding files. Binman should be able to hold
> everything in memory. It does generate files for later inspection
> though, so yes we could add this feature.
> 
>>
>> Maybe some new entry types, "data" for arbitrary data unrelated to a
>> file whose contents we can set, or "elf-segment" that can extract a
>> segment from an ELF file (but I don't like the information loss there).
> 
> Yes I quite like the idea of new entry types. In fact I was hoping to
> turn split-elf into an entry type (one that generated multiple
> entries). But I decided to stop before doing that since we really need
> to gets the code in there, fix the bugs /move forward with some of the
> ideas you and others have.

After this and the other fixes and such, I hope I can set aside some
time and experiment more on the ideas I've been throwing at you.

Sorry if I ended up delaying this too much already...