Hello Simon, On Monday, 8 June 2026 at 11:25 PM, Simon Glass <[email protected]> wrote:
> Hi Sam, > > On 2026-06-07T23:37:09, Sam Day via B4 Relay > <[email protected]> wrote: > > binman: Android boot image support > > > > Introduce initial support for Android boot images (abootimgs). This > > initial stab supports both v0 and v2 headers. The AOSP implementation > > was used as a reference. > > > > Since we're targeting U-Boot use cases here, a couple of things were > > omitted from this impl, namely 'second' and recovery_dtbo support. > > > > Link: https://android.googlesource.com/platform/system/tools/mkbootimg/ > > Signed-off-by: Sam Day <[email protected]> > > > > tools/binman/etype/android_boot.py | 313 > > +++++++++++++++++++++++++++ > > tools/binman/ftest.py | 66 ++++++ > > tools/binman/test/vendor/android_boot_v0.dts | 29 +++ > > tools/binman/test/vendor/android_boot_v2.dts | 46 ++++ > > 4 files changed, 454 insertions(+) > > > diff --git a/tools/binman/etype/android_boot.py > > b/tools/binman/etype/android_boot.py > > @@ -0,0 +1,313 @@ > > + @staticmethod > > + def _CheckFit(name, data, size): > > + if len(data) > size: > > + raise ValueError('%s is %d bytes, maximum is %d' % > > + (name, len(data), size)) > > + > > + return data + b'\0' * (size - len(data)) > > Please raise via self.Raise() so the user gets the node path. A bare > ValueError makes it hard to track down which entry overflowed when > several android-boot images are present. Ack, done in v3. > > > diff --git a/tools/binman/etype/android_boot.py > > b/tools/binman/etype/android_boot.py > > @@ -0,0 +1,313 @@ > > + def _GetEntryData(self, name, required): > > + entry = self._entries.get(name) > > + data = entry.GetData(required) > > + if data is None and not required: > > + return None > > + > > + return data > > + > > + def _GetOptionalEntryData(self, name, required, default=b''): > > + entry = self._entries.get(name) > > + if not entry: > > + return default > > + > > + data = entry.GetData(required) > > + if data is None and not required: > > + return None > > + > > + return data > > These two are almost identical and the names don't distinguish them, > i.e. _GetEntryData() will AttributeError if the name isn't present, > while _GetOptionalEntryData() returns the default. Please fold them > into one with a default=None parameter: None means required, anything > else means optional (function comment). Also, the 'if data is None and > not required' branch in _GetEntryData() does nothing - both arms > return data. Good catch, collapsed and simplified _GetEntryData() in v3. > > > diff --git a/tools/binman/etype/android_boot.py > > b/tools/binman/etype/android_boot.py > > @@ -0,0 +1,313 @@ > > + def ReadNode(self): > > + super().ReadNode() > > + self.header_version = fdt_util.GetInt(self._node, > > 'header-version', 0) > > ... > > + if self.header_version not in (0, 2): > > + self.Raise('Only Android boot image header versions 0 and 2 > > are ' > > + 'supported') > > Please call this out in the commit message - AOSP defines v0..v4 and > readers will wonder why v1/v3/v4 are missing. Something like "v1, v3, > v4 are not implemented as no use case has come up yet" would help. Noted, added the explicit callout in v3. > > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > > @@ -5598,6 +5598,72 @@ fdt fdtmap Extract the > > devicetree blob from the fdtmap > > + self.assertEqual(U_BOOT_DATA, data[0x800:0x800 + len(U_BOOT_DATA)]) > > + self.assertEqual(U_BOOT_DTB_DATA, > > + data[0x1000:0x1000 + len(U_BOOT_DTB_DATA)]) > > + > > + self.assertEqual(U_BOOT_DATA, data[0x800:0x800 + len(U_BOOT_DATA)]) > > Last assertion duplicates the first - please drop it. Apologies, this was junk I didn't properly catch in the zillionth rebasing session. It was already cleaned up in a subsequent commit. v3 will clean it up properly. > > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > > @@ -5598,6 +5598,72 @@ fdt fdtmap Extract the > > devicetree blob from the fdtmap > > + def testAndroidBootV0(self): > > + """Test that binman can produce a plain legacy Android boot > > image""" > > + data = self._DoReadFile('vendor/android_boot_v0.dts') > > Missing coverage for several error paths - U haven't run this but > there is Raise() for unsupported header_version, non-power-of-two > page-size, etc. Binman aims for full branch coverage - please add > tests for these, and verify 'binman test -T' still shows 100% coverage > for your etype As discussed in IRC, I agree this is very reasonable... I didn't appreciate just how far the proposed code was from that 100% goal, though ;) v3 introduces a much more comprehensive suite that hits 100% coverage. > > > diff --git a/tools/binman/test/vendor/android_boot_v2.dts > > b/tools/binman/test/vendor/android_boot_v2.dts > > @@ -0,0 +1,46 @@ > > + kernel-offset = <0x00008000>; > > + ramdisk-offset = <0x01000000>; > > + second-offset = <0x00f00000>; > > + tags-offset = <0x00000100>; > > 'second-offset' is silently ignored - the etype doesn't parse it and > the commit message says 'second' support is dropped. Do you need it? Nope, another vestige of a previous iteration that I didn't clean up properly in the rebasing, sorry about that! Removed in v3. > > > diff --git a/tools/binman/test/vendor/android_boot_v0.dts > > b/tools/binman/test/vendor/android_boot_v0.dts > > @@ -0,0 +1,29 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +/dts-v1/; > > Why a new 'vendor/' subdirectory? The contents here are plain Android > boot images, not anything vendor-specific. I'd suggest 'android/' (or > leaving them in test/ alongside other etype .dts files), which also > leaves a clearer naming pattern for QCDT/DTBH later. Fair call - I've yanked everything up to the top level in v3. > > > diff --git a/tools/binman/etype/android_boot.py > > b/tools/binman/etype/android_boot.py > > @@ -0,0 +1,313 @@ > > + Example:: > > + A v2 abootimg with control FDT placed in the DTB section: > > + > > + android-boot { > > ... > > + A v0 abootimg with embedded control FDT (v0 doesn't support DTBs) > > and > > + an empty ramdisk (some bootloaders insist on a ramdisk being > > present): > > + android-boot { > > These render as one big code block in the auto-generated docs, with > the v0 description folded into the literal. Please split into two > 'Example::' blocks, with a blank line between the v0 prose and its > 'android-boot {'. Ack, fixed in v3. > > Regards, > Simon > Thanks for the thorough review Simon, much appreciated. Cheers, -Sam
