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

[Simon Glass]

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.

> 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.

> 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.

> 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.

> 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

> 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?

> 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.

> 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 {'.

Regards,
Simon