[PATCH v2 0/5] binman: Improvements to FIT entry type
I've managed to build images like in doc/chromium/chainload.rst wtih binman, but ran into an issue with entry expansion in FIT and worked on it a bit. I also added SetImagePos() because that documentation asks for precise placement of u-boot.bin inside the FIT and I felt like doing it as an easier way to know the positions.
I would try to refactor and experiment with FIT things more, but I know Simon's currently working on converting SPL_FIT_GENERATOR to binman. Instead I'm just sending things I have already done with some tests added, hopefully without too many conflicts.
Changes in v2: - Split reused testSimpleFit code into a helper function - Check missing_bintools list instead of catching Fdt exceptions - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
v1: https://patchwork.ozlabs.org/project/uboot/list/?series=284714
Alper Nebi Yasak (5): binman: Fix subentry expansion for FIT entry type binman: Register and check bintools from FIT subentries binman: Check missing bintools of Section subclasses binman: Convert FIT entry type to a subclass of Section entry type binman: Update image positions of FIT subentries
tools/binman/etype/fit.py | 90 ++++++--- tools/binman/etype/section.py | 1 + tools/binman/ftest.py | 171 +++++++++++++++++- .../binman/test/220_fit_subentry_bintool.dts | 39 ++++ 4 files changed, 266 insertions(+), 35 deletions(-) create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts
Binman tries to expand some entries into parts that make it up, e.g. 'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb' and 'u-boot-dtb'. Entries with child entries must call ExpandEntries() on them to build a correct image, as it's possible that unexpanded child entries have no data of their own. The FIT entry type doesn't currently do this, which means putting a "u-boot" entry inside it doesn't work as expected.
Implement ExpandEntries() for FIT and add a copy of a simple FIT image test that checks subentry expansion in FIT entries.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---
Changes in v2: - Split reused testSimpleFit code into a helper function
tools/binman/etype/fit.py | 5 +++++ tools/binman/ftest.py | 33 ++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 6ad4a686df55..38bc2a2d784e 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -237,6 +237,11 @@ def _AddNode(base_node, depth, node): self._fdt = Fdt.FromData(fdt.as_bytearray()) self._fdt.Scan()
+ def ExpandEntries(self): + super().ExpandEntries() + for section in self._fit_sections.values(): + section.ExpandEntries() + def ObtainContents(self): """Obtain the contents of the FIT
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 5400f76c676a..626df786c8c9 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -61,6 +61,9 @@ U_BOOT_NODTB_DATA = b'nodtb with microcode pointer somewhere in here' U_BOOT_SPL_NODTB_DATA = b'splnodtb with microcode pointer somewhere in here' U_BOOT_TPL_NODTB_DATA = b'tplnodtb with microcode pointer somewhere in here' +U_BOOT_EXP_DATA = U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA +U_BOOT_SPL_EXP_DATA = U_BOOT_SPL_NODTB_DATA + U_BOOT_SPL_DTB_DATA +U_BOOT_TPL_EXP_DATA = U_BOOT_TPL_NODTB_DATA + U_BOOT_TPL_DTB_DATA FSP_DATA = b'fsp' CMC_DATA = b'cmc' VBT_DATA = b'vbt' @@ -3713,13 +3716,7 @@ def testPackOverlap(self): """Test that zero-size overlapping regions are ignored""" self._DoTestFile('160_pack_overlap_zero.dts')
- def testSimpleFit(self): - """Test an image with a FIT inside""" - data = self._DoReadFile('161_fit.dts') - self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)]) - self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):]) - fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)] - + def _CheckSimpleFitData(self, fit_data, kernel_data, fdt1_data): # The data should be inside the FIT dtb = fdt.Fdt.FromData(fit_data) dtb.Scan() @@ -3752,8 +3749,26 @@ def testSimpleFit(self): self.assertIsNotNone(data_sizes) self.assertEqual(2, len(data_sizes)) # Format is "4 Bytes = 0.00 KiB = 0.00 MiB" so take the first word - self.assertEqual(len(U_BOOT_DATA), int(data_sizes[0].split()[0])) - self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0])) + self.assertEqual(len(kernel_data), int(data_sizes[0].split()[0])) + self.assertEqual(len(fdt1_data), int(data_sizes[1].split()[0])) + + def testSimpleFit(self): + """Test an image with a FIT inside""" + data = self._DoReadFile('161_fit.dts') + self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)]) + self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):]) + fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)] + + self._CheckSimpleFitData(fit_data, U_BOOT_DATA, U_BOOT_SPL_DTB_DATA) + + def testSimpleFitExpandsSubentries(self): + """Test that FIT images expand their subentries""" + data = self._DoReadFileDtb('161_fit.dts', use_expanded=True)[0] + self.assertEqual(U_BOOT_EXP_DATA, data[:len(U_BOOT_EXP_DATA)]) + self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):]) + fit_data = data[len(U_BOOT_EXP_DATA):-len(U_BOOT_NODTB_DATA)] + + self._CheckSimpleFitData(fit_data, U_BOOT_EXP_DATA, U_BOOT_SPL_DTB_DATA)
def testFitExternal(self): """Test an image with an FIT with external images"""
On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
Binman tries to expand some entries into parts that make it up, e.g. 'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb' and 'u-boot-dtb'. Entries with child entries must call ExpandEntries() on them to build a correct image, as it's possible that unexpanded child entries have no data of their own. The FIT entry type doesn't currently do this, which means putting a "u-boot" entry inside it doesn't work as expected.
Implement ExpandEntries() for FIT and add a copy of a simple FIT image test that checks subentry expansion in FIT entries.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
Changes in v2:
- Split reused testSimpleFit code into a helper function
tools/binman/etype/fit.py | 5 +++++ tools/binman/ftest.py | 33 ++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
I missed the duplicate number on the dts, but will fix that when applying.
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 6ad4a686df55..38bc2a2d784e 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -237,6 +237,11 @@ def _AddNode(base_node, depth, node): self._fdt = Fdt.FromData(fdt.as_bytearray()) self._fdt.Scan()
- def ExpandEntries(self):
super().ExpandEntries()for section in self._fit_sections.values():section.ExpandEntries()- def ObtainContents(self): """Obtain the contents of the FIT
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 5400f76c676a..626df786c8c9 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -61,6 +61,9 @@ U_BOOT_NODTB_DATA = b'nodtb with microcode pointer somewhere in here' U_BOOT_SPL_NODTB_DATA = b'splnodtb with microcode pointer somewhere in here' U_BOOT_TPL_NODTB_DATA = b'tplnodtb with microcode pointer somewhere in here' +U_BOOT_EXP_DATA = U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA +U_BOOT_SPL_EXP_DATA = U_BOOT_SPL_NODTB_DATA + U_BOOT_SPL_DTB_DATA +U_BOOT_TPL_EXP_DATA = U_BOOT_TPL_NODTB_DATA + U_BOOT_TPL_DTB_DATA FSP_DATA = b'fsp' CMC_DATA = b'cmc' VBT_DATA = b'vbt' @@ -3713,13 +3716,7 @@ def testPackOverlap(self): """Test that zero-size overlapping regions are ignored""" self._DoTestFile('160_pack_overlap_zero.dts')
- def testSimpleFit(self):
"""Test an image with a FIT inside"""data = self._DoReadFile('161_fit.dts')self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]
- def _CheckSimpleFitData(self, fit_data, kernel_data, fdt1_data): # The data should be inside the FIT dtb = fdt.Fdt.FromData(fit_data) dtb.Scan()
@@ -3752,8 +3749,26 @@ def testSimpleFit(self): self.assertIsNotNone(data_sizes) self.assertEqual(2, len(data_sizes)) # Format is "4 Bytes = 0.00 KiB = 0.00 MiB" so take the first word
self.assertEqual(len(U_BOOT_DATA), int(data_sizes[0].split()[0]))self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0]))
self.assertEqual(len(kernel_data), int(data_sizes[0].split()[0]))self.assertEqual(len(fdt1_data), int(data_sizes[1].split()[0]))def testSimpleFit(self):
"""Test an image with a FIT inside"""data = self._DoReadFile('161_fit.dts')self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])fit_data = data[len(U_BOOT_DATA):-len(U_BOOT_NODTB_DATA)]self._CheckSimpleFitData(fit_data, U_BOOT_DATA, U_BOOT_SPL_DTB_DATA)def testSimpleFitExpandsSubentries(self):
"""Test that FIT images expand their subentries"""data = self._DoReadFileDtb('161_fit.dts', use_expanded=True)[0]self.assertEqual(U_BOOT_EXP_DATA, data[:len(U_BOOT_EXP_DATA)])self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])fit_data = data[len(U_BOOT_EXP_DATA):-len(U_BOOT_NODTB_DATA)]self._CheckSimpleFitData(fit_data, U_BOOT_EXP_DATA, U_BOOT_SPL_DTB_DATA)def testFitExternal(self): """Test an image with an FIT with external images"""
-- 2.34.1
On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
Binman tries to expand some entries into parts that make it up, e.g. 'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb' and 'u-boot-dtb'. Entries with child entries must call ExpandEntries() on them to build a correct image, as it's possible that unexpanded child entries have no data of their own. The FIT entry type doesn't currently do this, which means putting a "u-boot" entry inside it doesn't work as expected.
Implement ExpandEntries() for FIT and add a copy of a simple FIT image test that checks subentry expansion in FIT entries.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
Changes in v2:
- Split reused testSimpleFit code into a helper function
tools/binman/etype/fit.py | 5 +++++ tools/binman/ftest.py | 33 ++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
I missed the duplicate number on the dts, but will fix that when applying.
Applied to u-boot-dm, thanks!
Binman keeps track of binary tools each entry wants to use. The implementation of this for the FIT entry only adds "mkimage", but not the tools that would be used by its subentries.
Register the binary tools that FIT subentries will use in addition to the one FIT itself uses, and check their existence by copying the appropriate method from Section entry type. Also add tests that check if these subentries can use and warn about binary tools.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 14 +++++++ tools/binman/ftest.py | 25 ++++++++++++ .../binman/test/220_fit_subentry_bintool.dts | 39 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 38bc2a2d784e..9dffcd5adbd6 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -311,4 +311,18 @@ def SetAllowMissing(self, allow_missing): section.SetAllowMissing(allow_missing)
def AddBintools(self, tools): + for section in self._fit_sections.values(): + section.AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage') + + def check_missing_bintools(self, missing_list): + """Check if any entries in this section have missing bintools + + If there are missing bintools, these are added to the list + + Args: + missing_list: List of Bintool objects to be added to + """ + super().check_missing_bintools(missing_list) + for entry in self._fit_sections.values(): + entry.check_missing_bintools(missing_list) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 626df786c8c9..bc073587cc07 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5133,6 +5133,31 @@ def testListWithGenNode(self): finally: shutil.rmtree(tmpdir)
+ def testFitSubentryUsesBintool(self): + """Test that binman FIT subentries can use bintools""" + command.test_result = self._HandleGbbCommand + entry_args = { + 'keydir': 'devkeys', + 'bmpblk': 'bmpblk.bin', + } + data, _, _, _ = self._DoReadFileDtb('220_fit_subentry_bintool.dts', + entry_args=entry_args) + + expected = (GBB_DATA + GBB_DATA + tools.GetBytes(0, 8) + + tools.GetBytes(0, 0x2180 - 16)) + self.assertIn(expected, data) + + def testFitSubentryMissingBintool(self): + """Test that binman reports missing bintools for FIT subentries""" + entry_args = { + 'keydir': 'devkeys', + } + with test_util.capture_sys_output() as (_, stderr): + self._DoTestFile('220_fit_subentry_bintool.dts', + force_missing_bintools='futility', entry_args=entry_args) + err = stderr.getvalue() + self.assertRegex(err, + "Image 'main-section'.*missing bintools.*: futility")
if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/220_fit_subentry_bintool.dts b/tools/binman/test/220_fit_subentry_bintool.dts new file mode 100644 index 000000000000..6e29d41eeb33 --- /dev/null +++ b/tools/binman/test/220_fit_subentry_bintool.dts @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test-desc"; + #address-cells = <1>; + + images { + test { + description = "Something using a bintool"; + type = "kernel"; + arch = "arm"; + os = "linux"; + compression = "gzip"; + load = <00000000>; + entry = <00000000>; + + gbb { + size = <0x2180>; + }; + }; + }; + + configurations { + default = "conf-1"; + conf-1 { + description = "Boot bintool output"; + kernel = "kernel"; + }; + }; + }; + }; +};
Binman keeps track of binary tools each entry wants to use. The implementation of this for the FIT entry only adds "mkimage", but not the tools that would be used by its subentries.
Register the binary tools that FIT subentries will use in addition to the one FIT itself uses, and check their existence by copying the appropriate method from Section entry type. Also add tests that check if these subentries can use and warn about binary tools.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 14 +++++++ tools/binman/ftest.py | 25 ++++++++++++ .../binman/test/220_fit_subentry_bintool.dts | 39 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts
Applied to u-boot-dm, thanks!
Binman can check for missing binary tools and prints warnings if anything required for an image is missing. The implementation of this for the Section entry only checks the subentries, presumably because Section does not use any binary tools itself. However, this means the check is also skipped for subclasses of Section which might need binary tools.
Make sure missing binary tools are checked for subclasses of the Section entry type as well, by calling the parent class' implementation in the relevant Section method.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- This starts being tested by testFitMissing in the next patch when FIT becomes a Section subclass.
Changes in v2: - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/section.py | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index bb375e9063df..213a510a8d5b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -840,6 +840,7 @@ def check_missing_bintools(self, missing_list): Args: missing_list: List of Bintool objects to be added to """ + super().check_missing_bintools(missing_list) for entry in self._entries.values(): entry.check_missing_bintools(missing_list)
Binman can check for missing binary tools and prints warnings if anything required for an image is missing. The implementation of this for the Section entry only checks the subentries, presumably because Section does not use any binary tools itself. However, this means the check is also skipped for subclasses of Section which might need binary tools.
Make sure missing binary tools are checked for subclasses of the Section entry type as well, by calling the parent class' implementation in the relevant Section method.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org --- This starts being tested by testFitMissing in the next patch when FIT becomes a Section subclass.
Changes in v2: - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/section.py | 1 + 1 file changed, 1 insertion(+)
Applied to u-boot-dm, thanks!
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 70 +++++++++++---------------------------- tools/binman/ftest.py | 1 + 2 files changed, 20 insertions(+), 51 deletions(-)
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 9dffcd5adbd6..0f8c88a9720e 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -9,11 +9,12 @@ import libfdt
from binman.entry import Entry, EntryArg +from binman.etype.section import Entry_section from dtoc import fdt_util from dtoc.fdt import Fdt from patman import tools
-class Entry_fit(Entry): +class Entry_fit(Entry_section): """Flat Image Tree (FIT)
This calls mkimage to create a FIT (U-Boot Flat Image Tree) based on the @@ -112,15 +113,15 @@ def __init__(self, section, etype, node): """ Members: _fit: FIT file being built - _fit_sections: dict: + _entries: dict from Entry_section: key: relative path to entry Node (from the base of the FIT) value: Entry_section object comprising the contents of this node """ super().__init__(section, etype, node) self._fit = None - self._fit_sections = {} self._fit_props = {} + for pname, prop in self._node.props.items(): if pname.startswith('fit,'): self._fit_props[pname] = prop @@ -185,7 +186,7 @@ def _AddNode(base_node, depth, node): # 'data' property later. entry = Entry.Create(self.section, node, etype='section') entry.ReadNode() - self._fit_sections[rel_path] = entry + self._entries[rel_path] = entry
for subnode in node.subnodes: if has_images and not (subnode.name.startswith('hash') or @@ -237,18 +238,19 @@ def _AddNode(base_node, depth, node): self._fdt = Fdt.FromData(fdt.as_bytearray()) self._fdt.Scan()
- def ExpandEntries(self): - super().ExpandEntries() - for section in self._fit_sections.values(): - section.ExpandEntries() - - def ObtainContents(self): - """Obtain the contents of the FIT + 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 + + Returns: + Contents of the section (bytes) """ - # self._BuildInput() either returns bytes or raises an exception. data = self._BuildInput(self._fdt) uniq = self.GetUniqueName() input_fname = tools.GetOutputFilename('%s.itb' % uniq) @@ -264,14 +266,12 @@ def ObtainContents(self): 'pad': fdt_util.fdt32_to_cpu(ext_offset.value) } if self.mkimage.run(reset_timestamp=True, output_fname=output_fname, - **args) is not None: - self.SetContents(tools.ReadFile(output_fname)) - else: + **args) is None: # Bintool is missing; just use empty data as the output self.record_missing_bintool(self.mkimage) - self.SetContents(tools.GetBytes(0, 1024)) + return tools.GetBytes(0, 1024)
- return True + return tools.ReadFile(output_fname)
def _BuildInput(self, fdt): """Finish the FIT by adding the 'data' properties to it @@ -282,12 +282,8 @@ def _BuildInput(self, fdt): Returns: New fdt contents (bytes) """ - for path, section in self._fit_sections.items(): + for path, section in self._entries.items(): node = fdt.GetNode(path) - # Entry_section.ObtainContents() either returns True or - # raises an exception. - section.ObtainContents() - section.Pack(0) data = section.GetData() node.AddData('data', data)
@@ -295,34 +291,6 @@ def _BuildInput(self, fdt): data = fdt.GetContents() return data
- def CheckMissing(self, missing_list): - """Check if any entries in this FIT have missing external blobs - - If there are missing blobs, the entries are added to the list - - Args: - missing_list: List of Entry objects to be added to - """ - for path, section in self._fit_sections.items(): - section.CheckMissing(missing_list) - - def SetAllowMissing(self, allow_missing): - for section in self._fit_sections.values(): - section.SetAllowMissing(allow_missing) - def AddBintools(self, tools): - for section in self._fit_sections.values(): - section.AddBintools(tools) + super().AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage') - - def check_missing_bintools(self, missing_list): - """Check if any entries in this section have missing bintools - - If there are missing bintools, these are added to the list - - Args: - missing_list: List of Bintool objects to be added to - """ - super().check_missing_bintools(missing_list) - for entry in self._fit_sections.values(): - entry.check_missing_bintools(missing_list) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index bc073587cc07..f16798960b84 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3850,6 +3850,7 @@ def testPadInSections(self):
def testFitImageSubentryAlignment(self): """Test relative alignability of FIT image subentries""" + self._SetupSplElf() entry_args = { 'test-id': TEXT_DATA, }
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 70 +++++++++++---------------------------- tools/binman/ftest.py | 1 + 2 files changed, 20 insertions(+), 51 deletions(-)
Applied to u-boot-dm, thanks!
On 07.02.22 23:08, Alper Nebi Yasak wrote:
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v2:
- Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 70 +++++++++++---------------------------- tools/binman/ftest.py | 1 + 2 files changed, 20 insertions(+), 51 deletions(-)
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 9dffcd5adbd6..0f8c88a9720e 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -9,11 +9,12 @@ import libfdt
from binman.entry import Entry, EntryArg +from binman.etype.section import Entry_section from dtoc import fdt_util from dtoc.fdt import Fdt from patman import tools
-class Entry_fit(Entry): +class Entry_fit(Entry_section): """Flat Image Tree (FIT)
This calls mkimage to create a FIT (U-Boot Flat Image Tree) based on the@@ -112,15 +113,15 @@ def __init__(self, section, etype, node): """ Members: _fit: FIT file being built
_fit_sections: dict:
_entries: dict from Entry_section: key: relative path to entry Node (from the base of the FIT) value: Entry_section object comprising the contents of this node """ super().__init__(section, etype, node) self._fit = None
self._fit_sections = {} self._fit_props = {}
for pname, prop in self._node.props.items(): if pname.startswith('fit,'): self._fit_props[pname] = prop@@ -185,7 +186,7 @@ def _AddNode(base_node, depth, node): # 'data' property later. entry = Entry.Create(self.section, node, etype='section') entry.ReadNode()
self._fit_sections[rel_path] = entry
self._entries[rel_path] = entry for subnode in node.subnodes: if has_images and not (subnode.name.startswith('hash') or@@ -237,18 +238,19 @@ def _AddNode(base_node, depth, node): self._fdt = Fdt.FromData(fdt.as_bytearray()) self._fdt.Scan()
- def ExpandEntries(self):
super().ExpandEntries()for section in self._fit_sections.values():section.ExpandEntries()- def ObtainContents(self):
"""Obtain the contents of the FIT
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 toreturn NoneReturns:Contents of the section (bytes) """
# self._BuildInput() either returns bytes or raises an exception. data = self._BuildInput(self._fdt) uniq = self.GetUniqueName() input_fname = tools.GetOutputFilename('%s.itb' % uniq)@@ -264,14 +266,12 @@ def ObtainContents(self): 'pad': fdt_util.fdt32_to_cpu(ext_offset.value) } if self.mkimage.run(reset_timestamp=True, output_fname=output_fname,
**args) is not None:self.SetContents(tools.ReadFile(output_fname))else:
**args) is None: # Bintool is missing; just use empty data as the output self.record_missing_bintool(self.mkimage)
self.SetContents(tools.GetBytes(0, 1024))
return tools.GetBytes(0, 1024)
return True
return tools.ReadFile(output_fname)def _BuildInput(self, fdt): """Finish the FIT by adding the 'data' properties to it
@@ -282,12 +282,8 @@ def _BuildInput(self, fdt): Returns: New fdt contents (bytes) """
for path, section in self._fit_sections.items():
for path, section in self._entries.items(): node = fdt.GetNode(path)
# Entry_section.ObtainContents() either returns True or# raises an exception.section.ObtainContents()section.Pack(0) data = section.GetData() node.AddData('data', data)@@ -295,34 +291,6 @@ def _BuildInput(self, fdt): data = fdt.GetContents() return data
- def CheckMissing(self, missing_list):
"""Check if any entries in this FIT have missing external blobsIf there are missing blobs, the entries are added to the listArgs:missing_list: List of Entry objects to be added to"""for path, section in self._fit_sections.items():section.CheckMissing(missing_list)- def SetAllowMissing(self, allow_missing):
for section in self._fit_sections.values():section.SetAllowMissing(allow_missing)- def AddBintools(self, tools):
for section in self._fit_sections.values():section.AddBintools(tools)
super().AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage')
- def check_missing_bintools(self, missing_list):
"""Check if any entries in this section have missing bintoolsIf there are missing bintools, these are added to the listArgs:missing_list: List of Bintool objects to be added to"""super().check_missing_bintools(missing_list)for entry in self._fit_sections.values():entry.check_missing_bintools(missing_list)diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index bc073587cc07..f16798960b84 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3850,6 +3850,7 @@ def testPadInSections(self):
def testFitImageSubentryAlignment(self): """Test relative alignability of FIT image subentries"""
self._SetupSplElf() entry_args = { 'test-id': TEXT_DATA, }
Only came to testing this now, and it causes a regression.
Before this commit (I've added fdtmap to arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size ---------------------------------------------------------------------------------- main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 280000 f445f fit@0x280000 280000 fdtmap 37445f cf9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
With this commit:
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size ------------------------------------------------------------------------------------------- main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 fit@0x280000 280000 u-boot section u-boot-nodtb u-boot-nodtb fdt-iot2050-basic section blob blob fdt-iot2050-basic-pg2 section blob blob fdt-iot2050-advanced section blob blob fdt-iot2050-advanced-pg2 section blob blob k3-rti-wdt-firmware section blob-ext blob-ext fdtmap 37445f cd9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
The fit is nicely shown but its Image-pos column is now empty. And that leads to:
$ source/tools/binman/binman -D extract -i flash.bin -f extr.bin fit@0x280000 binman: unsupported operand type(s) for +: 'int' and 'NoneType'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "/data/u-boot/tools/binman/control.py", line 639, in Binman not args.uncompressed, args.format) File "/data/u-boot/tools/binman/control.py", line 260, in ExtractEntries data = entry.ReadData(decomp, alt_format) File "/data/u-boot/tools/binman/etype/section.py", line 763, in ReadData data = parent_data[offset:offset + self.size] TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
Similar breakages for "replace" and likely more sub-commands.
I have to revert this locally for now - no time to debug.
Jan
On 14/02/2022 12:09, Jan Kiszka wrote:
On 07.02.22 23:08, Alper Nebi Yasak wrote:
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Only came to testing this now, and it causes a regression.
Before this commit (I've added fdtmap to arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 280000 f445f fit@0x280000 280000 fdtmap 37445f cf9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
With this commit:
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 fit@0x280000 280000 u-boot section u-boot-nodtb u-boot-nodtb fdt-iot2050-basic section blob blob fdt-iot2050-basic-pg2 section blob blob fdt-iot2050-advanced section blob blob fdt-iot2050-advanced-pg2 section blob blob k3-rti-wdt-firmware section blob-ext blob-ext fdtmap 37445f cd9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
[3] "binman: Update image positions of FIT subentries" https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alper...
The fit is nicely shown but its Image-pos column is now empty. And that leads to:
$ source/tools/binman/binman -D extract -i flash.bin -f extr.bin fit@0x280000 binman: unsupported operand type(s) for +: 'int' and 'NoneType'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "/data/u-boot/tools/binman/control.py", line 639, in Binman not args.uncompressed, args.format) File "/data/u-boot/tools/binman/control.py", line 260, in ExtractEntries data = entry.ReadData(decomp, alt_format) File "/data/u-boot/tools/binman/etype/section.py", line 763, in ReadData data = parent_data[offset:offset + self.size] TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
Similar breakages for "replace" and likely more sub-commands.
OTOH, Entry_section docstring does say subentry offsets can be None, so it might be a good idea to fix these TypeErrors regardless.
I have to revert this locally for now - no time to debug.
Jan
On 15.02.22 13:27, Alper Nebi Yasak wrote:
On 14/02/2022 12:09, Jan Kiszka wrote:
On 07.02.22 23:08, Alper Nebi Yasak wrote:
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Only came to testing this now, and it causes a regression.
Before this commit (I've added fdtmap to arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 280000 f445f fit@0x280000 280000 fdtmap 37445f cf9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
With this commit:
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 fit@0x280000 280000 u-boot section u-boot-nodtb u-boot-nodtb fdt-iot2050-basic section blob blob fdt-iot2050-basic-pg2 section blob blob fdt-iot2050-advanced section blob blob fdt-iot2050-advanced-pg2 section blob blob k3-rti-wdt-firmware section blob-ext blob-ext fdtmap 37445f cd9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
This one helped, indeed.
Thanks, Jan
[3] "binman: Update image positions of FIT subentries" https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alper...
The fit is nicely shown but its Image-pos column is now empty. And that leads to:
$ source/tools/binman/binman -D extract -i flash.bin -f extr.bin fit@0x280000 binman: unsupported operand type(s) for +: 'int' and 'NoneType'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "/data/u-boot/tools/binman/control.py", line 639, in Binman not args.uncompressed, args.format) File "/data/u-boot/tools/binman/control.py", line 260, in ExtractEntries data = entry.ReadData(decomp, alt_format) File "/data/u-boot/tools/binman/etype/section.py", line 763, in ReadData data = parent_data[offset:offset + self.size] TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
Similar breakages for "replace" and likely more sub-commands.
OTOH, Entry_section docstring does say subentry offsets can be None, so it might be a good idea to fix these TypeErrors regardless.
I have to revert this locally for now - no time to debug.
Jan
On 15.02.22 17:50, Jan Kiszka wrote:
On 15.02.22 13:27, Alper Nebi Yasak wrote:
On 14/02/2022 12:09, Jan Kiszka wrote:
On 07.02.22 23:08, Alper Nebi Yasak wrote:
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Only came to testing this now, and it causes a regression.
Before this commit (I've added fdtmap to arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 280000 f445f fit@0x280000 280000 fdtmap 37445f cf9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
With this commit:
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 fit@0x280000 280000 u-boot section u-boot-nodtb u-boot-nodtb fdt-iot2050-basic section blob blob fdt-iot2050-basic-pg2 section blob blob fdt-iot2050-advanced section blob blob fdt-iot2050-advanced-pg2 section blob blob k3-rti-wdt-firmware section blob-ext blob-ext fdtmap 37445f cd9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
This one helped, indeed.
...not completely:
$ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Jan
On 15.02.22 18:06, Jan Kiszka wrote:
On 15.02.22 17:50, Jan Kiszka wrote:
On 15.02.22 13:27, Alper Nebi Yasak wrote:
On 14/02/2022 12:09, Jan Kiszka wrote:
On 07.02.22 23:08, Alper Nebi Yasak wrote:
The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.
Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Only came to testing this now, and it causes a regression.
Before this commit (I've added fdtmap to arch/arm/dts/k3-am65-iot2050-boot-image.dtsi for this):
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 280000 f445f fit@0x280000 280000 fdtmap 37445f cf9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
With this commit:
$ source/tools/binman/binman -D ls -i flash.bin Name Image-pos Size Entry-type Offset Uncomp-size
main-section 0 8c0000 section 0 blob-ext@0x000000 0 37373 blob-ext@0x000000 0 blob@0x080000 80000 84484 blob@0x080000 80000 fit@0x280000 fit@0x280000 280000 u-boot section u-boot-nodtb u-boot-nodtb fdt-iot2050-basic section blob blob fdt-iot2050-basic-pg2 section blob blob fdt-iot2050-advanced section blob blob fdt-iot2050-advanced-pg2 section blob blob k3-rti-wdt-firmware section blob-ext blob-ext fdtmap 37445f cd9 fdtmap 37445f fill@0x680000 680000 20000 fill@0x680000 680000 fill@0x6a0000 6a0000 20000 fill@0x6a0000 6a0000 blob-ext@0x6c0000 6c0000 415de blob-ext@0x6c0000 6c0000 blob-ext@0x740000 740000 43952 blob-ext@0x740000 740000 blob-ext@0x7c0000 7c0000 415e2 blob-ext@0x7c0000 7c0000 blob-ext@0x840000 840000 4395a blob-ext@0x840000 840000
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
This one helped, indeed.
...not completely:
$ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Ping.
$ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000 binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "u-boot/tools/binman/control.py", line 644, in Binman allow_resize=not args.fix_size, write_map=args.map) File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage AfterReplace(image, allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 333, in AfterReplace get_contents=False, allow_resize=allow_resize) File "u-boot/tools/binman/control.py", line 560, in ProcessImage image.PackEntries() File "u-boot/tools/binman/image.py", line 155, in PackEntries super().Pack(0) File "u-boot/tools/binman/etype/section.py", line 385, in Pack self._PackEntries() File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries offset = entry.Pack(offset) File "u-boot/tools/binman/etype/section.py", line 390, in Pack data = self.BuildSectionData(True) File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData tools.write_file(input_fname, data) File "u-boot/tools/patman/tools.py", line 482, in write_file with open(filename(fname), binary and 'wb' or 'w') as fd: PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Something seems fairly broken here. That '/.' does not come from the output directory name, it's generated by Entry.GetUniqueName. Looks like this path should not been taken under these conditions.
Jan
On 18/02/2022 19:50, Jan Kiszka wrote:
On 15.02.22 18:06, Jan Kiszka wrote:
On 15.02.22 17:50, Jan Kiszka wrote:
On 15.02.22 13:27, Alper Nebi Yasak wrote:
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
This one helped, indeed.
...not completely:
$ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Ping.
$ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000 binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "u-boot/tools/binman/control.py", line 644, in Binman allow_resize=not args.fix_size, write_map=args.map) File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage AfterReplace(image, allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 333, in AfterReplace get_contents=False, allow_resize=allow_resize) File "u-boot/tools/binman/control.py", line 560, in ProcessImage image.PackEntries() File "u-boot/tools/binman/image.py", line 155, in PackEntries super().Pack(0) File "u-boot/tools/binman/etype/section.py", line 385, in Pack self._PackEntries() File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries offset = entry.Pack(offset) File "u-boot/tools/binman/etype/section.py", line 390, in Pack data = self.BuildSectionData(True) File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData tools.write_file(input_fname, data) File "u-boot/tools/patman/tools.py", line 482, in write_file with open(filename(fname), binary and 'wb' or 'w') as fd: PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Something seems fairly broken here. That '/.' does not come from the output directory name, it's generated by Entry.GetUniqueName. Looks like this path should not been taken under these conditions.
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
Here's the diff I got so far in case it helps:
diff --git a/tools/binman/control.py b/tools/binman/control.py index a179f7812988..24ec89b26302 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -390,6 +390,7 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths, """ image_fname = os.path.abspath(image_fname) image = Image.FromFile(image_fname) + image.CollectBintools()
# Replace an entry from a single file, as a special case if input_fname: diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 631215dfc88a..8173e91af96a 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -767,7 +767,7 @@ def GetUniqueName(self): node = self._node while node.parent: node = node.parent - if node.name == 'binman': + if node.name == 'binman' or node.name == '/': break name = '%s.%s' % (node.name, name) return name diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 1e957023f354..9b2c33bc2b34 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -137,10 +137,6 @@ def __init__(self, section, etype, node): str)])[0] self.mkimage = None
- def ReadNode(self): - self.ReadEntries() - super().ReadNode() - def ReadEntries(self): def _AddNode(base_node, depth, node): """Add a node to the FIT @@ -184,11 +180,12 @@ def _AddNode(base_node, depth, node): # 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 = Entry.Create(self, node, etype='section') entry.ReadNode() # The hash subnodes here are for mkimage, not binman. entry.SetUpdateHash(False) - self._entries[rel_path] = entry + image_name = rel_path[len('/images/'):] + self._entries[image_name] = entry
for subnode in node.subnodes: if has_images and not (subnode.name.startswith('hash') or @@ -284,7 +281,8 @@ def _BuildInput(self, fdt): Returns: New fdt contents (bytes) """ - for path, section in self._entries.items(): + for image_name, section in self._entries.items(): + path = f"/images/{image_name}" node = fdt.GetNode(path) data = section.GetData() node.AddData('data', data) @@ -312,7 +310,8 @@ def SetImagePos(self, image_pos): fdt = Fdt.FromData(self.GetData()) fdt.Scan()
- for path, section in self._entries.items(): + for image_name, section in self._entries.items(): + path = f"/images/{image_name}" node = fdt.GetNode(path)
data_prop = node.props.get("data")
Hi Alper,
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 18/02/2022 19:50, Jan Kiszka wrote:
On 15.02.22 18:06, Jan Kiszka wrote:
On 15.02.22 17:50, Jan Kiszka wrote:
On 15.02.22 13:27, Alper Nebi Yasak wrote:
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
This one helped, indeed.
...not completely:
$ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Ping.
$ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000 binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "u-boot/tools/binman/control.py", line 644, in Binman allow_resize=not args.fix_size, write_map=args.map) File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage AfterReplace(image, allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 333, in AfterReplace get_contents=False, allow_resize=allow_resize) File "u-boot/tools/binman/control.py", line 560, in ProcessImage image.PackEntries() File "u-boot/tools/binman/image.py", line 155, in PackEntries super().Pack(0) File "u-boot/tools/binman/etype/section.py", line 385, in Pack self._PackEntries() File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries offset = entry.Pack(offset) File "u-boot/tools/binman/etype/section.py", line 390, in Pack data = self.BuildSectionData(True) File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData tools.write_file(input_fname, data) File "u-boot/tools/patman/tools.py", line 482, in write_file with open(filename(fname), binary and 'wb' or 'w') as fd: PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Something seems fairly broken here. That '/.' does not come from the output directory name, it's generated by Entry.GetUniqueName. Looks like this path should not been taken under these conditions.
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
Regards, Simon
Here's the diff I got so far in case it helps:
diff --git a/tools/binman/control.py b/tools/binman/control.py index a179f7812988..24ec89b26302 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -390,6 +390,7 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths, """ image_fname = os.path.abspath(image_fname) image = Image.FromFile(image_fname)
image.CollectBintools()
# Replace an entry from a single file, as a special case if input_fname:
diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 631215dfc88a..8173e91af96a 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -767,7 +767,7 @@ def GetUniqueName(self): node = self._node while node.parent: node = node.parent
if node.name == 'binman':
if node.name == 'binman' or node.name == '/': break name = '%s.%s' % (node.name, name) return namediff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 1e957023f354..9b2c33bc2b34 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -137,10 +137,6 @@ def __init__(self, section, etype, node): str)])[0] self.mkimage = None
- def ReadNode(self):
self.ReadEntries()super().ReadNode()- def ReadEntries(self): def _AddNode(base_node, depth, node): """Add a node to the FIT
@@ -184,11 +180,12 @@ def _AddNode(base_node, depth, node): # 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 = Entry.Create(self, node, etype='section') entry.ReadNode() # The hash subnodes here are for mkimage, not binman. entry.SetUpdateHash(False)
self._entries[rel_path] = entry
image_name = rel_path[len('/images/'):]self._entries[image_name] = entry for subnode in node.subnodes: if has_images and not (subnode.name.startswith('hash') or@@ -284,7 +281,8 @@ def _BuildInput(self, fdt): Returns: New fdt contents (bytes) """
for path, section in self._entries.items():
for image_name, section in self._entries.items():path = f"/images/{image_name}" node = fdt.GetNode(path) data = section.GetData() node.AddData('data', data)@@ -312,7 +310,8 @@ def SetImagePos(self, image_pos): fdt = Fdt.FromData(self.GetData()) fdt.Scan()
for path, section in self._entries.items():
for image_name, section in self._entries.items():path = f"/images/{image_name}" node = fdt.GetNode(path) data_prop = node.props.get("data")
Hi again,
On Sat, 19 Feb 2022 at 08:53, Simon Glass sjg@chromium.org wrote:
Hi Alper,
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 18/02/2022 19:50, Jan Kiszka wrote:
On 15.02.22 18:06, Jan Kiszka wrote:
On 15.02.22 17:50, Jan Kiszka wrote:
On 15.02.22 13:27, Alper Nebi Yasak wrote:
The AddMissingProperties() and SetCalculatedProperties() methods were disabled for FIT as a fixup to this patch, that's why image-pos etc. aren't available. See comments at [1] for some context.
Hopefully my two patches [2][3] fix things, can you test with them?
[1] "binman: Correct the error message for a bad hash algorithm" https://patchwork.ozlabs.org/project/uboot/patch/20220208105941.1.I8f212a115...
[2] "binman: Skip processing "hash" subnodes of FIT subsections" https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpe...
This one helped, indeed.
...not completely:
$ source/tools/binman/binman replace -i flash.bin -f fit@0x380000.fit fit@0x380000binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Ping.
$ source/tools/binman/binman -D replace -i flash.bin -f fit@0x380000.fit fit@0x380000 binman: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Traceback (most recent call last): File "source/tools/binman/binman", line 141, in RunBinman ret_code = control.Binman(args) File "u-boot/tools/binman/control.py", line 644, in Binman allow_resize=not args.fix_size, write_map=args.map) File "u-boot/tools/binman/control.py", line 404, in ReplaceEntries allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 341, in WriteEntryToImage AfterReplace(image, allow_resize=allow_resize, write_map=write_map) File "u-boot/tools/binman/control.py", line 333, in AfterReplace get_contents=False, allow_resize=allow_resize) File "u-boot/tools/binman/control.py", line 560, in ProcessImage image.PackEntries() File "u-boot/tools/binman/image.py", line 155, in PackEntries super().Pack(0) File "u-boot/tools/binman/etype/section.py", line 385, in Pack self._PackEntries() File "u-boot/tools/binman/etype/section.py", line 403, in _PackEntries offset = entry.Pack(offset) File "u-boot/tools/binman/etype/section.py", line 390, in Pack data = self.BuildSectionData(True) File "u-boot/tools/binman/etype/fit.py", line 265, in BuildSectionData tools.write_file(input_fname, data) File "u-boot/tools/patman/tools.py", line 482, in write_file with open(filename(fname), binary and 'wb' or 'w') as fd: PermissionError: [Errno 13] Permission denied: '/.fit@0x380000.itb'
Something seems fairly broken here. That '/.' does not come from the output directory name, it's generated by Entry.GetUniqueName. Looks like this path should not been taken under these conditions.
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
It refactors the fit implementation to separate scanning from emitting the tree and I think this might help quite a bit. I'll send out the series when I get a chance in the next few days or so.
Regards, Simon
[..]
On 21/02/2022 07:40, Simon Glass wrote:
On Sat, 19 Feb 2022 at 08:53, Simon Glass sjg@chromium.org wrote:
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
Yeah, this causes an error in image.FindEntryPath() while trying to replace e.g. "/fit@0x280000/images/u-boot" since there is no "images" entry in the FIT. Changing the key to the node name works, but then the "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
It refactors the fit implementation to separate scanning from emitting the tree and I think this might help quite a bit. I'll send out the series when I get a chance in the next few days or so.
I've also managed to somewhat fix the rest of the issues I wrote, so now I can replace a FIT entry with a modified one (having a different u-boot file), or replace a subentry of the FIT with an arbitrary file.
I couldn't look at your new version much but I'll try to see how good my fixes apply on top of it, will probably take me longer to patchify things.
Hi Alper,
On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 21/02/2022 07:40, Simon Glass wrote:
On Sat, 19 Feb 2022 at 08:53, Simon Glass sjg@chromium.org wrote:
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
Yeah, this causes an error in image.FindEntryPath() while trying to replace e.g. "/fit@0x280000/images/u-boot" since there is no "images" entry in the FIT. Changing the key to the node name works, but then the "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
It refactors the fit implementation to separate scanning from emitting the tree and I think this might help quite a bit. I'll send out the series when I get a chance in the next few days or so.
I've also managed to somewhat fix the rest of the issues I wrote, so now I can replace a FIT entry with a modified one (having a different u-boot file), or replace a subentry of the FIT with an arbitrary file.
I couldn't look at your new version much but I'll try to see how good my fixes apply on top of it, will probably take me longer to patchify things.
OK I'm going to send a new series with (most of) your suggested fixes a new patches, then my refactoring. Just need to get things through CI.
Regards, Simon
On 23.02.22 23:59, Simon Glass wrote:
Hi Alper,
On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 21/02/2022 07:40, Simon Glass wrote:
On Sat, 19 Feb 2022 at 08:53, Simon Glass sjg@chromium.org wrote:
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
Yeah, this causes an error in image.FindEntryPath() while trying to replace e.g. "/fit@0x280000/images/u-boot" since there is no "images" entry in the FIT. Changing the key to the node name works, but then the "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
It refactors the fit implementation to separate scanning from emitting the tree and I think this might help quite a bit. I'll send out the series when I get a chance in the next few days or so.
I've also managed to somewhat fix the rest of the issues I wrote, so now I can replace a FIT entry with a modified one (having a different u-boot file), or replace a subentry of the FIT with an arbitrary file.
I couldn't look at your new version much but I'll try to see how good my fixes apply on top of it, will probably take me longer to patchify things.
OK I'm going to send a new series with (most of) your suggested fixes a new patches, then my refactoring. Just need to get things through CI.
What's the status here? I've just rebased over master, a simple revert of this commit no longer works, and the regression is still present. Are there any pending patches that fixes this and I should pick locally in order to rebase/test my pending things?
Thanks, Jan
On 28/02/2022 14:48, Jan Kiszka wrote:
On 23.02.22 23:59, Simon Glass wrote:
On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
I've also managed to somewhat fix the rest of the issues I wrote, so now I can replace a FIT entry with a modified one (having a different u-boot file), or replace a subentry of the FIT with an arbitrary file.
I couldn't look at your new version much but I'll try to see how good my fixes apply on top of it, will probably take me longer to patchify things.
OK I'm going to send a new series with (most of) your suggested fixes a new patches, then my refactoring. Just need to get things through CI.
What's the status here? I've just rebased over master, a simple revert of this commit no longer works, and the regression is still present. Are there any pending patches that fixes this and I should pick locally in order to rebase/test my pending things?
Sorry, I didn't get enough time to work on this at all. I'm drafting up replies to Simon's series [1] now, after I'm done with those I'll try to finish and send fixes for this.
[1] "binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script" https://lore.kernel.org/u-boot/20220223230040.159317-1-sjg@chromium.org/T/
Hi Jan,
On Mon, 28 Feb 2022 at 04:48, Jan Kiszka jan.kiszka@siemens.com wrote:
On 23.02.22 23:59, Simon Glass wrote:
Hi Alper,
On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 21/02/2022 07:40, Simon Glass wrote:
On Sat, 19 Feb 2022 at 08:53, Simon Glass sjg@chromium.org wrote:
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
I can reproduce this and tried a few things, but more issues just kept popping up (outside u-boot as well). I got it to a point where the command re-packs the FIT and the image but quite wrongly. The offset and image-pos properties get added in the FIT, and the image main-section just concatenates all entries without regard to set offsets. I'll need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
Yeah, this causes an error in image.FindEntryPath() while trying to replace e.g. "/fit@0x280000/images/u-boot" since there is no "images" entry in the FIT. Changing the key to the node name works, but then the "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
It refactors the fit implementation to separate scanning from emitting the tree and I think this might help quite a bit. I'll send out the series when I get a chance in the next few days or so.
I've also managed to somewhat fix the rest of the issues I wrote, so now I can replace a FIT entry with a modified one (having a different u-boot file), or replace a subentry of the FIT with an arbitrary file.
I couldn't look at your new version much but I'll try to see how good my fixes apply on top of it, will probably take me longer to patchify things.
OK I'm going to send a new series with (most of) your suggested fixes a new patches, then my refactoring. Just need to get things through CI.
What's the status here? I've just rebased over master, a simple revert of this commit no longer works, and the regression is still present. Are there any pending patches that fixes this and I should pick locally in order to rebase/test my pending things?
Please see this series and review if you can:
https://patchwork.ozlabs.org/user/todo/uboot/?series=287681
I did not add a test for your issue though. Can you take a look?
Regards, Simon
On 28.02.22 14:56, Simon Glass wrote:
Hi Jan,
On Mon, 28 Feb 2022 at 04:48, Jan Kiszka jan.kiszka@siemens.com wrote:
On 23.02.22 23:59, Simon Glass wrote:
Hi Alper,
On Tue, 22 Feb 2022 at 11:58, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 21/02/2022 07:40, Simon Glass wrote:
On Sat, 19 Feb 2022 at 08:53, Simon Glass sjg@chromium.org wrote:
On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak alpernebiyasak@gmail.com wrote: > I can reproduce this and tried a few things, but more issues just kept > popping up (outside u-boot as well). I got it to a point where the > command re-packs the FIT and the image but quite wrongly. The offset and > image-pos properties get added in the FIT, and the image main-section > just concatenates all entries without regard to set offsets. I'll > need more time to work those out, then to add tests and send patches.
I am going to try to merge my fit generator series today.
One issue I notice is that the conversion to use entry_Section changes the contents of the self._fit_entries dict. Before it was keyed by relative path, but entry_section keys self._entries by node name.
Yeah, this causes an error in image.FindEntryPath() while trying to replace e.g. "/fit@0x280000/images/u-boot" since there is no "images" entry in the FIT. Changing the key to the node name works, but then the "binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".
We may need to split it up. I will see if I can at least merge my series, which should not make things any worse, then see if I can come up with ideas.
Thanks for the diff.
I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
It refactors the fit implementation to separate scanning from emitting the tree and I think this might help quite a bit. I'll send out the series when I get a chance in the next few days or so.
I've also managed to somewhat fix the rest of the issues I wrote, so now I can replace a FIT entry with a modified one (having a different u-boot file), or replace a subentry of the FIT with an arbitrary file.
I couldn't look at your new version much but I'll try to see how good my fixes apply on top of it, will probably take me longer to patchify things.
OK I'm going to send a new series with (most of) your suggested fixes a new patches, then my refactoring. Just need to get things through CI.
What's the status here? I've just rebased over master, a simple revert of this commit no longer works, and the regression is still present. Are there any pending patches that fixes this and I should pick locally in order to rebase/test my pending things?
Please see this series and review if you can:
https://patchwork.ozlabs.org/project/uboot/list/?series=287681
I did not add a test for your issue though. Can you take a look?
I cannot comment on details. However, a quick test did not indicate that the series by itself changes the situation: same error.
Jan
Binman keeps track of positions of each entry in the final image, but currently this data is wrong for things included in FIT entries, especially since a previous patch makes FIT a subclass of Section and inherit its implementation.
There are three ways to put data into a FIT image. It can be directly included as a "data" property, or it can be external to the FIT image represented by an offset-size pair of properties. This external offset is either "data-position" from the start of the FIT or "data-offset" from the end of the FIT, and the size is "data-size" for both. However, binman doesn't use the "data-offset" method while building FIT entries.
According to the Section docstring, its subclasses should calculate and set the correct offsets and sizes in SetImagePos() method. Do this for FIT subentries for the three ways mentioned above, and add tests for the two ways binman can pack them in.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Check missing_bintools list instead of catching Fdt exceptions - Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 51 +++++++++++++++++ tools/binman/ftest.py | 112 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+)
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 0f8c88a9720e..5497f036a26d 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -291,6 +291,57 @@ def _BuildInput(self, fdt): data = fdt.GetContents() return data
+ def SetImagePos(self, image_pos): + """Set the position in the image + + This sets each subentry's offsets, sizes and positions-in-image + according to where they ended up in the packed FIT file. + + Args: + image_pos: Position of this entry in the image + """ + super().SetImagePos(image_pos) + + # If mkimage is missing we'll have empty data, + # which will cause a FDT_ERR_BADMAGIC error + if self.mkimage in self.missing_bintools: + return + + fdt = Fdt.FromData(self.GetData()) + fdt.Scan() + + for path, section in self._entries.items(): + node = fdt.GetNode(path) + + data_prop = node.props.get("data") + data_pos = fdt_util.GetInt(node, "data-position") + data_offset = fdt_util.GetInt(node, "data-offset") + data_size = fdt_util.GetInt(node, "data-size") + + # Contents are inside the FIT + if data_prop is not None: + # GetOffset() returns offset of a fdt_property struct, + # which has 3 fdt32_t members before the actual data. + offset = data_prop.GetOffset() + 12 + size = len(data_prop.bytes) + + # External offset from the base of the FIT + elif data_pos is not None: + offset = data_pos + size = data_size + + # External offset from the end of the FIT, not used in binman + elif data_offset is not None: # pragma: no cover + offset = fdt.GetFdtObj().totalsize() + data_offset + size = data_size + + # This should never happen + else: # pragma: no cover + self.Raise("%s: missing data properties" % (path)) + + section.SetOffsetSize(offset, size) + section.SetImagePos(self.image_pos) + def AddBintools(self, tools): super().AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage') diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index f16798960b84..ab988565c335 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3770,6 +3770,62 @@ def testSimpleFitExpandsSubentries(self):
self._CheckSimpleFitData(fit_data, U_BOOT_EXP_DATA, U_BOOT_SPL_DTB_DATA)
+ def testSimpleFitImagePos(self): + """Test that we have correct image-pos for FIT subentries""" + data, _, _, out_dtb_fname = self._DoReadFileDtb('161_fit.dts', + update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, BASE_DTB_PROPS + REPACK_DTB_PROPS) + + self.assertEqual({ + 'image-pos': 0, + 'offset': 0, + 'size': 1890, + + 'u-boot:image-pos': 0, + 'u-boot:offset': 0, + 'u-boot:size': 4, + + 'fit:image-pos': 4, + 'fit:offset': 4, + 'fit:size': 1840, + + 'fit/images/kernel:image-pos': 160, + 'fit/images/kernel:offset': 156, + 'fit/images/kernel:size': 4, + + 'fit/images/kernel/u-boot:image-pos': 160, + 'fit/images/kernel/u-boot:offset': 0, + 'fit/images/kernel/u-boot:size': 4, + + 'fit/images/fdt-1:image-pos': 456, + 'fit/images/fdt-1:offset': 452, + 'fit/images/fdt-1:size': 6, + + 'fit/images/fdt-1/u-boot-spl-dtb:image-pos': 456, + 'fit/images/fdt-1/u-boot-spl-dtb:offset': 0, + 'fit/images/fdt-1/u-boot-spl-dtb:size': 6, + + 'u-boot-nodtb:image-pos': 1844, + 'u-boot-nodtb:offset': 1844, + 'u-boot-nodtb:size': 46, + }, props) + + # Actually check the data is where we think it is + for node, expected in [ + ("u-boot", U_BOOT_DATA), + ("fit/images/kernel", U_BOOT_DATA), + ("fit/images/kernel/u-boot", U_BOOT_DATA), + ("fit/images/fdt-1", U_BOOT_SPL_DTB_DATA), + ("fit/images/fdt-1/u-boot-spl-dtb", U_BOOT_SPL_DTB_DATA), + ("u-boot-nodtb", U_BOOT_NODTB_DATA), + ]: + image_pos = props[f"{node}:image-pos"] + size = props[f"{node}:size"] + self.assertEqual(len(expected), size) + self.assertEqual(expected, data[image_pos:image_pos+size]) + def testFitExternal(self): """Test an image with an FIT with external images""" data = self._DoReadFile('162_fit_external.dts') @@ -3798,6 +3854,62 @@ def testFitExternal(self): self.assertEqual(U_BOOT_DATA + b'aa', data[actual_pos:actual_pos + external_data_size])
+ def testFitExternalImagePos(self): + """Test that we have correct image-pos for external FIT subentries""" + data, _, _, out_dtb_fname = self._DoReadFileDtb('162_fit_external.dts', + update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, BASE_DTB_PROPS + REPACK_DTB_PROPS) + + self.assertEqual({ + 'image-pos': 0, + 'offset': 0, + 'size': 1082, + + 'u-boot:image-pos': 0, + 'u-boot:offset': 0, + 'u-boot:size': 4, + + 'fit:size': 1032, + 'fit:offset': 4, + 'fit:image-pos': 4, + + 'fit/images/kernel:size': 4, + 'fit/images/kernel:offset': 1024, + 'fit/images/kernel:image-pos': 1028, + + 'fit/images/kernel/u-boot:size': 4, + 'fit/images/kernel/u-boot:offset': 0, + 'fit/images/kernel/u-boot:image-pos': 1028, + + 'fit/images/fdt-1:size': 2, + 'fit/images/fdt-1:offset': 1028, + 'fit/images/fdt-1:image-pos': 1032, + + 'fit/images/fdt-1/_testing:size': 2, + 'fit/images/fdt-1/_testing:offset': 0, + 'fit/images/fdt-1/_testing:image-pos': 1032, + + 'u-boot-nodtb:image-pos': 1036, + 'u-boot-nodtb:offset': 1036, + 'u-boot-nodtb:size': 46, + }, props) + + # Actually check the data is where we think it is + for node, expected in [ + ("u-boot", U_BOOT_DATA), + ("fit/images/kernel", U_BOOT_DATA), + ("fit/images/kernel/u-boot", U_BOOT_DATA), + ("fit/images/fdt-1", b'aa'), + ("fit/images/fdt-1/_testing", b'aa'), + ("u-boot-nodtb", U_BOOT_NODTB_DATA), + ]: + image_pos = props[f"{node}:image-pos"] + size = props[f"{node}:size"] + self.assertEqual(len(expected), size) + self.assertEqual(expected, data[image_pos:image_pos+size]) + def testFitMissing(self): """Test that binman still produces a FIT image if mkimage is missing""" with test_util.capture_sys_output() as (_, stderr):
Hi Alper,
On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
Binman keeps track of positions of each entry in the final image, but currently this data is wrong for things included in FIT entries, especially since a previous patch makes FIT a subclass of Section and inherit its implementation.
There are three ways to put data into a FIT image. It can be directly included as a "data" property, or it can be external to the FIT image represented by an offset-size pair of properties. This external offset is either "data-position" from the start of the FIT or "data-offset" from the end of the FIT, and the size is "data-size" for both. However, binman doesn't use the "data-offset" method while building FIT entries.
According to the Section docstring, its subclasses should calculate and set the correct offsets and sizes in SetImagePos() method. Do this for FIT subentries for the three ways mentioned above, and add tests for the two ways binman can pack them in.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v2:
- Check missing_bintools list instead of catching Fdt exceptions
- Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 51 +++++++++++++++++ tools/binman/ftest.py | 112 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+)
As mentioned I had to change the previous patch in a minor way to get it to apply.
I'd really like to get this in if possible, too. The issue is the handling of hash nodes in a FIT, as I mentioned.
If you are able to rework this, please let me know.
I've gone ahead sent my fit series but will rebase it onto this patch if you are able to fix it up.
Regards, Simon
Hi Alper,
On Mon, 7 Feb 2022 at 15:08, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
Binman keeps track of positions of each entry in the final image, but currently this data is wrong for things included in FIT entries, especially since a previous patch makes FIT a subclass of Section and inherit its implementation.
There are three ways to put data into a FIT image. It can be directly included as a "data" property, or it can be external to the FIT image represented by an offset-size pair of properties. This external offset is either "data-position" from the start of the FIT or "data-offset" from the end of the FIT, and the size is "data-size" for both. However, binman doesn't use the "data-offset" method while building FIT entries.
According to the Section docstring, its subclasses should calculate and set the correct offsets and sizes in SetImagePos() method. Do this for FIT subentries for the three ways mentioned above, and add tests for the two ways binman can pack them in.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v2:
- Check missing_bintools list instead of catching Fdt exceptions
- Add tag: "Reviewed-by: Simon Glass sjg@chromium.org"
tools/binman/etype/fit.py | 51 +++++++++++++++++ tools/binman/ftest.py | 112 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+)
As mentioned I had to change the previous patch in a minor way to get it to apply.
I'd really like to get this in if possible, too. The issue is the handling of hash nodes in a FIT, as I mentioned.
If you are able to rework this, please let me know.
I've gone ahead sent my fit series but will rebase it onto this patch if you are able to fix it up.
Regards, Simon
Applied to u-boot-dm, thanks!
participants (3)
-
Alper Nebi Yasak -
Jan Kiszka -
Simon Glass