[PATCH v2 3/8] binman: android_boot: vendor-dt 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: vendor-dt support
>
> There's many Android bootloaders out there that expect non-standard
> abootimgs. Samsung and Qualcomm, and likely many others. This quirk has
> been codified in the widely used osm0sis fork/rewrite of mkbootimg.
>
> Presumably, these vendor-specific abootimgs were conceived before the v2
> format was widely available or specified. The evidence for this is their
> hijacking of the header_version field to specify the length of a payload
> that follows the main abootimg.
>
> At least QCDT and DTBH (both of which will be implemented as binman
> etypes in following commits) use this approach.
>
> Link: https://github.com/osm0sis/mkbootimg
> Signed-off-by: Sam Day <me at samcday.com>
>
> tools/binman/etype/android_boot.py                 | 37 ++++++++++++++++++++--
>  tools/binman/ftest.py                              | 11 ++++++-
>  .../binman/test/vendor/android_boot_vendor_dt.dts  | 27 ++++++++++++++++
>  3 files changed, 72 insertions(+), 3 deletions(-)

> diff --git a/tools/binman/etype/android_boot.py b/tools/binman/etype/android_boot.py
> @@ -131,17 +136,26 @@ class Entry_android_boot(Entry_section):
>                  self.Raise('page-size must fit the Android boot image header')
>              if 'dtb' in self._entries:
>                  self.Raise("Subnode 'dtb' requires header-version 2")
> +            if self.vendor_dt_node and self.os_version:
> +                self.Raise("os_version not allowed when vendor-dt is in use")

Two things. The message uses the Python attribute name; users see the
DT property, so this should read 'os-version'. Second, the commit
message doesn't explain why os-version becomes invalid in this mode -
in the v0 header layout, dt_size (the field we hijack) and os_version
are separate u32 fields, so there's no obvious conflict.

If a downstream bootloader interprets os_version differently when
dt_size is set, please document that - otherwise I'd drop the
restriction. Either way, please add a test for the Raise().

> diff --git a/tools/binman/etype/android_boot.py b/tools/binman/etype/android_boot.py
> @@ -152,6 +166,15 @@ class Entry_android_boot(Entry_section):
>              entry.SetPrefix(self._name_prefix)
>              self._entries[node.name] = entry
>
> +    def _ReadVendorDtEntries(self, vendor_dt_node):
> +        entry = Entry.Create(self, vendor_dt_node, etype='section',
> +                             expanded=self.GetImage().use_expanded,
> +                             missing_etype=self.GetImage().missing_etype)
> +        entry.page_size = fdt_util.GetInt(self._node, 'page-size', 2048)
> +        entry.ReadNode()
> +        entry.SetPrefix(self._name_prefix)
> +        self._entries[vendor_dt_node.name] = entry

Forcing etype='section' here works only because qcdt/dtbh later walks
up via getattr(self.section, 'page_size', 2048). That coupling is
invisible from this file. Please add a comment noting this attribute
is consumed by child etypes (qcdt/dtbh in later patches) - otherwise
it looks like dead code. A cleaner alternative would be for the child
etype to look up the android-boot ancestor directly, so page-size
doesn't need to be smuggled through an unrelated attribute.

Also 'page-size' is re-read from self._node here, but ReadNode() has
already cached it in self.page_size. Just use that.

> diff --git a/tools/binman/etype/android_boot.py b/tools/binman/etype/android_boot.py
> @@ -246,7 +278,7 @@ class Entry_android_boot(Entry_section):
>                               0, # second_offset
>                               self._GetAddr(self.tags_offset, 'tags'),
>                               self.page_size,
> -                             self.header_version,
> +                             len(vendor_dt),

This conflates two distinct fields. For a plain v0 image the slot is
header_version; for a vendor-dt image it's dt_size. The current code
only produces the right bytes because both collapse to 0 when there is
no vendor-dt. Please make it explicit:

    dt_size = len(vendor_dt) if self.vendor_dt_node else self.header_version

and add a comment that this slot is overloaded. As written, anyone
adding a new v0 header_version in future will silently break vendor-dt
builds.

> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> @@ -5662,7 +5662,16 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          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)])
> +    def testAndroidBootVendorDt(self):

You're silently dropping a duplicated assertion left over from patch
1/8. That cleanup belongs in its own patch (or a fixup to 1/8), not
buried in the vendor-dt commit.

> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> @@ -5662,7 +5662,16 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> +    def testAndroidBootVendorDt(self):
> +        """Test that android-boot can embed an arbitrary vendor-dt section"""
> +        data = self._DoReadFile('vendor/android_boot_vendor_dt.dts')
> +        header = struct.unpack_from('<8s10I16s512s32s', data, 0)
> +        vendor_dt = b'howdy'
> +        self.assertEqual(len(vendor_dt), header[9])
> +        self.assertEqual(0, header[10])
> +        self.assertEqual(self._AndroidBootId(U_BOOT_DATA, b'\0', b'',
> +                                             vendor_dt), header[13])
> +        self.assertEqual(vendor_dt, data[0x1800:0x1805])

Coverage is thin. Please add tests for the two new Raise() paths
('vendor-dt' on v2, os-version with vendor-dt), and assert that
vendor_dt is padded to page-size - slicing [0x1800:0x1805] only proves
the first five bytes landed, not that the next 0x7fb bytes are zero or
that the overall image length is right.

Regards,
Simon