Hi Sam,
On Tue, 9 Jun 2026 at 08:20, Sam Day <[email protected]> wrote:
>
> 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 ;)
Indeed...in particularly we are not used to adding tests for failure
cases and it can be a bit tedious. It does make you think though,
whether an error is needed, whether a condition should be removed,
etc. Also it becomes very hard for future changes to break your etype.
>
> 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.
You're welcome.
Regards,
Simon
