'Ello Simon, On Monday, 8 June 2026 at 11:27 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: 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 <[email protected]> > > > > 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 >
