* [PATCH v2 1/5] binman: Fix subentry expansion for FIT entry type
2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
2022-02-08 15:05 ` Simon Glass
2022-02-07 22:08 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Alper Nebi Yasak
` (3 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak
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"""
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 1/5] binman: Fix subentry expansion for FIT entry type
2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
@ 2022-02-08 15:05 ` Simon Glass
2022-02-08 20:39 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-08 15:05 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka
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
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 1/5] binman: Fix subentry expansion for FIT entry type
2022-02-08 15:05 ` Simon Glass
@ 2022-02-08 20:39 ` Simon Glass
0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka, Alper Nebi Yasak
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!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 2/5] binman: Register and check bintools from FIT subentries
2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
2022-02-08 20:39 ` Simon Glass
2022-02-07 22:08 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Alper Nebi Yasak
` (2 subsequent siblings)
4 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak
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";
+ };
+ };
+ };
+ };
+};
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/5] binman: Register and check bintools from FIT subentries
2022-02-07 22:08 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Alper Nebi Yasak
@ 2022-02-08 20:39 ` Simon Glass
0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, u-boot
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!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 3/5] binman: Check missing bintools of Section subclasses
2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 1/5] binman: Fix subentry expansion for " Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 2/5] binman: Register and check bintools from FIT subentries Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
2022-02-08 20:39 ` Simon Glass
2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
4 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak
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)
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 3/5] binman: Check missing bintools of Section subclasses
2022-02-07 22:08 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Alper Nebi Yasak
@ 2022-02-08 20:39 ` Simon Glass
0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, u-boot
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!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
` (2 preceding siblings ...)
2022-02-07 22:08 ` [PATCH v2 3/5] binman: Check missing bintools of Section subclasses Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
2022-02-08 20:39 ` Simon Glass
2022-02-14 9:09 ` Jan Kiszka
2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
4 siblings, 2 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak
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,
}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
@ 2022-02-08 20:39 ` Simon Glass
2022-02-14 9:09 ` Jan Kiszka
1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:39 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, u-boot
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!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
2022-02-08 20:39 ` Simon Glass
@ 2022-02-14 9:09 ` Jan Kiszka
2022-02-15 12:27 ` Alper Nebi Yasak
1 sibling, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-14 9:09 UTC (permalink / raw)
To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass
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 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,
> }
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
--
Siemens AG, Technology
Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-14 9:09 ` Jan Kiszka
@ 2022-02-15 12:27 ` Alper Nebi Yasak
2022-02-15 16:50 ` Jan Kiszka
0 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-15 12:27 UTC (permalink / raw)
To: Jan Kiszka, u-boot; +Cc: Heiko Thiery, Simon Glass
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
[2] "binman: Skip processing "hash" subnodes of FIT subsections"
https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
[3] "binman: Update image positions of FIT subentries"
https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alpernebiyasak@gmail.com/
> 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
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-15 12:27 ` Alper Nebi Yasak
@ 2022-02-15 16:50 ` Jan Kiszka
2022-02-15 17:06 ` Jan Kiszka
0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-15 16:50 UTC (permalink / raw)
To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>
> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>
This one helped, indeed.
Thanks,
Jan
> [3] "binman: Update image positions of FIT subentries"
> https://patchwork.ozlabs.org/project/uboot/patch/20220207220809.4497-6-alpernebiyasak@gmail.com/
>
>> 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
>>
--
Siemens AG, Technology
Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-15 16:50 ` Jan Kiszka
@ 2022-02-15 17:06 ` Jan Kiszka
2022-02-18 16:50 ` Jan Kiszka
0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-15 17:06 UTC (permalink / raw)
To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>>
>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>>
>
> 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
--
Siemens AG, Technology
Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-15 17:06 ` Jan Kiszka
@ 2022-02-18 16:50 ` Jan Kiszka
2022-02-18 17:34 ` Alper Nebi Yasak
0 siblings, 1 reply; 27+ messages in thread
From: Jan Kiszka @ 2022-02-18 16:50 UTC (permalink / raw)
To: Alper Nebi Yasak, u-boot; +Cc: Heiko Thiery, Simon Glass
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>>>
>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>>>
>>
>> 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
--
Siemens AG, Technology
Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-18 16:50 ` Jan Kiszka
@ 2022-02-18 17:34 ` Alper Nebi Yasak
2022-02-19 15:53 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-18 17:34 UTC (permalink / raw)
To: Jan Kiszka, u-boot; +Cc: Heiko Thiery, Simon Glass
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
>>>>
>>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
>>>>
>>>
>>> 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")
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-18 17:34 ` Alper Nebi Yasak
@ 2022-02-19 15:53 ` Simon Glass
2022-02-21 4:40 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-19 15:53 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
> >>>>
> >>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
> >>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
> >>>>
> >>>
> >>> 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 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")
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-19 15:53 ` Simon Glass
@ 2022-02-21 4:40 ` Simon Glass
2022-02-22 18:58 ` Alper Nebi Yasak
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-21 4:40 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery
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.I8f212a1150defebaf8b7b15a79f7a2fc62c276b2@changeid/
> > >>>>
> > >>>> [2] "binman: Skip processing "hash" subnodes of FIT subsections"
> > >>>> https://patchwork.ozlabs.org/project/uboot/patch/20220209190236.26479-1-alpernebiyasak@gmail.com/
> > >>>>
> > >>>
> > >>> 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
[..]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-21 4:40 ` Simon Glass
@ 2022-02-22 18:58 ` Alper Nebi Yasak
2022-02-23 22:59 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-22 18:58 UTC (permalink / raw)
To: Simon Glass; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-22 18:58 ` Alper Nebi Yasak
@ 2022-02-23 22:59 ` Simon Glass
2022-02-28 11:48 ` Jan Kiszka
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-23 22:59 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: Jan Kiszka, U-Boot Mailing List, Heiko Thiery
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-23 22:59 ` Simon Glass
@ 2022-02-28 11:48 ` Jan Kiszka
2022-02-28 13:51 ` Alper Nebi Yasak
2022-02-28 13:56 ` Simon Glass
0 siblings, 2 replies; 27+ messages in thread
From: Jan Kiszka @ 2022-02-28 11:48 UTC (permalink / raw)
To: Simon Glass, Alper Nebi Yasak; +Cc: U-Boot Mailing List, Heiko Thiery
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
--
Siemens AG, Technology
Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-28 11:48 ` Jan Kiszka
@ 2022-02-28 13:51 ` Alper Nebi Yasak
2022-02-28 13:56 ` Simon Glass
1 sibling, 0 replies; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-28 13:51 UTC (permalink / raw)
To: Jan Kiszka, Simon Glass; +Cc: U-Boot Mailing List, Heiko Thiery
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/
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-28 11:48 ` Jan Kiszka
2022-02-28 13:51 ` Alper Nebi Yasak
@ 2022-02-28 13:56 ` Simon Glass
2022-02-28 14:14 ` Jan Kiszka
1 sibling, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-28 13:56 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Alper Nebi Yasak, U-Boot Mailing List, Heiko Thiery
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
2022-02-28 13:56 ` Simon Glass
@ 2022-02-28 14:14 ` Jan Kiszka
0 siblings, 0 replies; 27+ messages in thread
From: Jan Kiszka @ 2022-02-28 14:14 UTC (permalink / raw)
To: Simon Glass; +Cc: Alper Nebi Yasak, U-Boot Mailing List, Heiko Thiery
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/user/todo/uboot/?series=287681
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
--
Siemens AG, Technology
Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 5/5] binman: Update image positions of FIT subentries
2022-02-07 22:08 [PATCH v2 0/5] binman: Improvements to FIT entry type Alper Nebi Yasak
` (3 preceding siblings ...)
2022-02-07 22:08 ` [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type Alper Nebi Yasak
@ 2022-02-07 22:08 ` Alper Nebi Yasak
2022-02-08 20:43 ` Simon Glass
4 siblings, 1 reply; 27+ messages in thread
From: Alper Nebi Yasak @ 2022-02-07 22:08 UTC (permalink / raw)
To: u-boot; +Cc: Heiko Thiery, Jan Kiszka, Simon Glass, Alper Nebi Yasak
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):
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v2 5/5] binman: Update image positions of FIT subentries
2022-02-07 22:08 ` [PATCH v2 5/5] binman: Update image positions of FIT subentries Alper Nebi Yasak
@ 2022-02-08 20:43 ` Simon Glass
2022-02-23 2:35 ` Simon Glass
0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2022-02-08 20:43 UTC (permalink / raw)
To: Alper Nebi Yasak; +Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/5] binman: Update image positions of FIT subentries
2022-02-08 20:43 ` Simon Glass
@ 2022-02-23 2:35 ` Simon Glass
0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2022-02-23 2:35 UTC (permalink / raw)
To: Simon Glass
Cc: U-Boot Mailing List, Heiko Thiery, Jan Kiszka, Alper Nebi Yasak
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!
^ permalink raw reply [flat|nested] 27+ messages in thread