[PATCH v2 1/8] binman: Android boot image support

[Sam Day]

Hello Simon,

On Monday, 8 June 2026 at 11:25 PM, Simon Glass <sjg at chromium.org> wrote:

> Hi Sam,
> 
> On 2026-06-07T23:37:09, Sam Day via B4 Relay
> <devnull+me.samcday.com at kernel.org> 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 <me at samcday.com>
> >
> > 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