[PATCH v2 3/8] binman: android_boot: vendor-dt support

[Sam Day]

'Ello Simon,

On Monday, 8 June 2026 at 11:27 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: 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.

Good point. I double checked and there's no need to add / enforce this
invariant. I simply added it purely out of paranoia, since it's right next
to the header_version and possibly some bootloader somewhere interprets
the "vendor DT" size as 64 bit. But then again, probably not ;)

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

mkbootimg-osm0sis doesn't enforce it, at any rate. So I've removed
the check from v3.

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

I opted for the cleaner alternative you proposed. In v3 the qcdt/dtbh
etypes traverse their ancestors to find an explicit page-size, falling
back to the local prop, or 2048 otherwise.

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

I did actually code-golf it that way, but I agree it's too clever ;)
(And by clever I mean obnoxiously obtuse)

I've introduced an explicit `overloaded_header_version` in v3, and
included a comment about this special/weird behaviour.

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

Not sure I follow this part. _BuildV0SectionData is only ever called when
self.header_version == 0.

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

Aye, oops. That's cleaned up properly in v3.

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

Ack - v3 will have 100% test coverage for all 3 newly introduced etypes.

Again, thanks for the very comprehensive review!

Kind regards,
-Sam

> 
> Regards,
> Simon
>