Re: [PATCH 2/4] fdtfactor: Add tool to split DTBs into a base DTB and overlays

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 62121 bytes --]

On Wed, Oct 28, 2026 at 02:57:27PM -0700, Doug Anderson wrote:
> Hi David,
> 
> On Sun, Oct 25, 2026 at 9:51 PM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > This patch is very large, which makes it fairly hard to review - a
> > danger with AI generated code.  However, from the premise there's not
> > an obvious (to me) logical way of splitting it up.
> 
> Yeah, it is a bit unwieldy. I didn't see any amazing ways to break it up,
> but I can definitely try harder. Let's see how it looks after addressing
> all of your feedback.

Ok.  I think my comments suggested at least some small pieces which
can be chipped off, even if there's not some big logical break points.

> > I'll see what
> > appears as I go through the implementation.
> 
> Thanks so much for your detailed review! I really appreciate it!
> 
> 
> > > +Options:
> > > +
> > > +* `-b`, `--base-dtb`: Output path for the common base `.dtb` (placed in
> > > +  `--outdir` if specified without a directory component).
> > > +* `-o`, `--outdir`: Output directory for generated `.dtbo` files (and for
> > > +  `--base-dtb` when given without a directory component).
> >
> > Maybe it's just me, but I find it slightly confusing that both -b and
> > -o are controlling the output names.  I'd be inclined to drop -b and
> > always send the generated based dtb to <outdir>/base.dtb, at least for
> > this first patch introducing the tool.  After all, you can always mv
> > it afterwards.
> 
> Yeah, I debated this a bit originally, and I agree it's a bit confusing.
> How about this: I'll keep the option, but force it to be just a filename,
> not a full path. At least to me that makes it less confusing. Hopefully
> that's OK?

Hm, the way the path is encoded isn't really what I dislike about it.
Something about the "base-dtb" name suggests an existing base tree,
rather than an output desination to me (particularly since in future
you might want an existing base tree to do recursive factoring).  What
I'd suggest is dropping the short option and renaming the long option
to include "output".

> > > +* `-S`, `--strip-local-fixups`: Omit `/__local_fixups__` from the generated
> > > +  `base.dtb` and final reconstructed device trees. When `-L` is passed to
> > > +  `dtc` solely so `fdtfactor` can locate phandle cells at build time, the
> > > +  original DTB's `/__local_fixups__` subtree is not needed at runtime by
> > > +  either the overlay loader (`fdt_overlay_apply()`) or the OS kernel;
> > > +  passing `-S` strips it to reduce the size of both `base.dtb` and the
> > > +  `.dtbo` overlays. Omit `-S` if downstream tooling or verification
> > > +  expects applying a board's `.dtbo` to `base.dtb` to reproduce the
> > > +  original `.dtb`'s `/__local_fixups__` subtree verbatim.
> >
> > Useful option, could be split to another patch to make this one less
> > huge.
> 
> Done.
> 
> 
> > > +* `--verify`: Verify that applying each generated `.dtbo` to `base.dtb`
> > > +  with `libfdt` recreates the original `.dtb`.
> >
> > Seems like something more for the test suite than built into the tool.
> 
> Removed and made sure the test suite covers.
> 
> 
> > > +* `-v`, `--verbose`: Print size savings summary.
> >
> > Something like --stats might be better for this.  -v tends to be more
> > for debugging / logging output.
> 
> Done.
> 
> 
> > > +### TODO
> > > +
> > > +* **Explore omitting `/__fixups__` and `/__symbols__` from overlays by
> > > +  hardcoding base DT phandles**: Normally a standalone `.dtbo` stores
> > > +  references to base tree nodes as `0xffffffff` with string entries in
> > > +  `/__fixups__` that the overlay loader resolves at runtime against
> > > +  `base.dtb`'s `/__symbols__`. Because `fdtfactor` generates `base.dtb`
> > > +  and all `.dtbo` overlays together, it already knows the exact numeric
> > > +  phandle values assigned to every node in `base.dtb`. Neither `libfdt` nor
> > > +  coreboot modifies property cells that are not listed in `/__fixups__` or
> > > +  `/__local_fixups__`. Writing the base phandle integers directly into
> > > +  overlay properties would allow omitting `/__fixups__` from the `.dtbo`
> > > +  files (and omitting `/__symbols__` when runtime symbol lookup is not
> > > +  needed), further reducing overlay size and speeding up boot-time overlay
> > > +  application.
> >
> > I really don't like that idea.  You'd essentially be creating not
> > really valid dtbo files that will work by accident against one
> > specific base file.
> 
> Dropped from the TODO.
> 
> 
> > > +* **Multi-level overlays**: Support hierarchical overlay layers (for
> > > +  example, SoC base -> board family intermediate overlay -> SKU leaf
> > > +  overlay) when multiple boards share large sub-trees not present across
> > > +  all boards.
> >
> > That sounds like a useful extension for the future.  An interim step
> > would be to extend to instead of taking a bunch of dtbs as input, take
> > a single (already generated) base dtb and a bunch of dtbos.  You'd
> > leave the given base alone, but find further common things between the
> > dtbs, in the context of that base.  The idea is you'd run fdtfactor
> > once on the raw dtbs, then run it again on a subset of the dtbos
> > emitted from the first pass to find further commonalities in that
> > subset.
> 
> Improved the TODO to incorporate this.
> 
> 
> > > +def split_dtb_filename(path):
> > > +    """Split a DTB path into its directory and filename stem.
> > > +
> > > +    Example:
> > > +        '/dir/board.dtb' -> ('/dir', 'board')
> > > +
> > > +    Args:
> > > +        path: File path to a .dtb or .dtbo file.
> > > +
> > > +    Returns:
> > > +        Tuple of (directory, stem) where `stem` has any trailing `.dtb` or
> > > +        `.dtbo` suffix removed.
> > > +    """
> > > +    directory, filename = os.path.split(path)
> > > +    stem, sep, ext = filename.rpartition('.')
> > > +    if not stem or not sep or ext not in ('dtb', 'dtbo'):
> > > +        stem = filename
> > > +    return directory, stem
> >
> > Hm, I'll have to see how this is actually used, but I'm generally a
> > bit leery (on non-Windows systems) of using the extension as anything
> > but a hint / default.
> 
> Removed from this patch. It comes back when compression / FIT image
> support is added but the code never relies on the extension being correct.
> 
> 
> > > +class NodeTree:
> > > +    """In-memory tree of nodes and properties for sequential FDT creation."""
> >
> > A "live" tree representation is the right data structure for this job.
> > On the other hand it's a pretty hefty piece of infrastructure to just
> > inline in this tool.
> >
> > I'd suggest adding this as a preliminary patch, with a more trivial
> > test case (say round trip from dtb to dtb via the in-memory
> > representation).  Then introduce the actual fdtfactor logic to use it.
> 
> I hope it's OK, but I ended up deciding to move NodeTree into
> pylibfdt/libfdt.i. I can move it back here if you want, but it felt
> like others could want to use this too. If you want it in a separate
> file or back in fdtfactor, let me know.

Ok, we'll see.

> > > +    def __init__(self):
> > > +        self.props = {}
> > > +        self.children = {}
> > > +
> > > +    def get_node(self, path):
> > > +        """Return the descendant NodeTree at `path`, creating missing nodes.
> >
> > I gess it's what you need here, but "creating missing nodes" seems
> > risky for what looks at first glance like a read interface.
> 
> I split out get_node() and create_path(), where create_path() creates the
> missing parts of the path and then also returns the final node. Hopefully
> that works for you.
> 
> 
> > > +        Looks up a child or deeper descendant `NodeTree` by following the
> > > +        slash-separated node names in `path` starting from `self` (leading and
> > > +        trailing `'/'` characters are ignored, so `'/soc/i2c@4000'` and
> > > +        `'soc/i2c@4000'` behave identically). If any node along `path` does not
> > > +        exist yet in `self.children`, a new empty `NodeTree` is created for it
> > > +        automatically (similar to `mkdir -p`). Passing `''` or `'/'` returns
> > > +        `self`.
> > > +
> > > +        Example:
> > > +            root = NodeTree()
> > > +            # The following automatically creates root.children['soc'] and
> > > +            # root.children['soc'].children['i2c@4000'], and returns the
> > > +            # 'i2c@4000' NodeTree.
> > > +            i2c = root.get_node('/soc/i2c@4000')
> > > +            i2c.props['status'] = b'okay\\x00'
> >
> > Should probably be b'status' - property names aren't Unicode.
> 
> That doesn't seem to match how the existing pylibfdt works. My agent
> says "Existing pylibfdt requires str for node names, paths, and property
> names, and bytes/bytearray for property values."
>
> I confirmed this myself by running this (my example uses the properties()
> method added by my next version of the patch series for testing, but
> that doesn't actually affect the result):
> 
>   fdtro = libfdt.FdtRo(open("sc7180-trogdor-lazor-r9.dtb", "rb").read())
>   [prop.name for prop in fdtro.properties(0)]
> 
> I got back:
>   ['interrupt-parent', '#address-cells', ... ]
> 
> Specifically, I didn't get back [b'interrupt-parent', ...]. That being
> said, while the existing bindings make the property names strings, the
> values seem to be bytes:
> 
>   [prop for prop in fdtro.properties(0)][3]
>   Property(b'Google Lazor (rev9)\x00')
> 
> So I think we're OK. My assumption is that the python bindings were
> written this way because:
> 
> 1. It would be a pain to use b'' strings everywhere.
> 2. Property names ought to be safe to convert back and forth to python
>    strings.
> 
> Please yell if I'm misunderstanding.

Ah, good point.  *thinks* In fact, this is perfectly safe because
propert and node)names are restricted to characters which all lie
within 7-bit ASCII, so UTF-8 and bytestrings are effectively
equivalent.  Sorry, my mistake (repeatedly).

> > > +        Args:
> > > +            path: Slash-separated node path starting from `self` (leading `'/'`
> > > +                is optional and ignored, e.g. `'/soc/i2c@4000'` or
> > > +                `'soc/i2c@4000'`).
> > > +
> > > +        Returns:
> > > +            The `NodeTree` instance at `path`.
> > > +        """
> > > +        if path == '/' or not path:
> > > +            return self
> >
> > Again, path should be treated as a bytestring not a string - node
> > names are not Unicode.  At least, internally that should be the case.
> > Accepting a string and converting it to a bytestring would be fine.
> 
> Again, I don't think this matches the existing python bindings.
>   first_off = [suboff for suboff in fdtro.subnodes(0)][0]
>   fdtro.get_path(first_off)
> 
> That gives me back: '/aliases', not b'/aliases'.
> 
> NOTE: the above uses the subnodes() method added by my next version of
> the patch series for testing, but that doesn't actually affect the result.
> 
> 
> > > +        curr = self
> > > +        for part in path.strip('/').split('/'):
> >
> > Hmm.  The strip() here means it will ignore not just one, but any
> > number of leading or trailing '/'.  However, internal repeated '/'
> > will result in subnodes named '', which is not desirable.
> >
> > Simpler and more consistent might be to drop the initial special case
> > handling of '/' and '' as full paths and instead go with:
> >
> >         for part in path.split('/'):
> >             if not part:
> >                 continue
> >             . . .
> 
> Done
> 
> 
> > > +            if part not in curr.children:
> > > +                curr.children[part] = NodeTree()
> > > +            curr = curr.children[part]
> > > +        return curr
> > > +
> > > +    def _write_to_sw(self, sw, name=''):
> >
> > Traditionally the process of converting an in-memory device tree
> > representation to a dtb is referred to as "flattening".  So I'd
> > suggest maybe _flatten_subtree() for this.  As an internal function,
> > I'd also drop the default name='' - it's rarely useful and incorrect
> > if called on anything except the root node.
> 
> Done.
> 
> 
> > > +        """Recursively write this NodeTree and its children into an FdtSw.
> > > +
> > > +        Args:
> > > +            sw: A `libfdt.FdtSw` sequential-write instance.
> > > +            name: Node name for this subtree (empty string for the root node).
> > > +        """
> > > +        sw.begin_node(name)
> > > +        for prop_name, val in self.props.items():
> > > +            sw.property(prop_name, bytes(val))
> > > +        for child_name, child_node in self.children.items():
> > > +            child_node._write_to_sw(sw, child_name)
> > > +        sw.end_node()
> > > +
> > > +    def to_fdt_blob(self, mem_rsv=(), boot_cpuid_phys=0, size_hint=65536):
> >
> > flatten()
> 
> Done.
> 
> 
> > > +        """Serialize this root NodeTree into a packed FDT byte string.
> > > +
> > > +        Args:
> > > +            mem_rsv: Iterable of (address, size) tuples for the FDT memory
> > > +                reservation block.
> > > +            boot_cpuid_phys: Physical boot CPU ID for the FDT header.
> > > +            size_hint: Initial buffer size hint in bytes for `libfdt.FdtSw`.
> > > +
> > > +        Returns:
> > > +            Serialized FDT blob as `bytes`.
> > > +        """
> > > +        sw = libfdt.FdtSw(size_hint)
> > > +        for addr, size in mem_rsv:
> > > +            sw.add_reservemap_entry(addr, size)
> > > +        sw.finish_reservemap()
> > > +        self._write_to_sw(sw, '')
> > > +        # Unlike other FdtSw methods, as_fdt() does not call check_space()
> > > +        # when fdt_finish() appends the 4-byte FDT_END token. If fewer than
> > > +        # 4 bytes remain in the buffer, resize once and retry.
> >
> > That seems like a bug in FdtSw.as_fdt().  We should fix that, rather
> > than working around it here.
> 
> Done.
> 
> 
> > > +def _open_fdt_ro(raw_bytes, filename):
> > > +    """Validate and wrap an uncompressed FDT blob in `libfdt.FdtRo`.
> > > +
> > > +    Args:
> > > +        raw_bytes: Uncompressed FDT binary contents as `bytes`.
> > > +        filename: Display name or file path for error reporting.
> >
> > This is a weird interface.  Why not take just the filename, and read
> > the bytes from it in here?
> 
> Function is gone in the new version.
> 
> 
> > > +class ParsedDtb:
> > > +    """Parsed representation of an input .dtb blob."""
> > > +
> > > +    def __init__(self, filename, raw_bytes):
> >
> > Again, why take filename and bytes separately?
> 
> It makes more sense when you get to the later patch that adds FIT image
> support (where DTBs are extracted from memory rather than a file).
> Actually, let me move raw_bytes to that patch to keep this patch smaller
> and then it will all hopefully make more sense.
> 
> 
> > > +        """Parse an uncompressed FDT blob into nodes, symbols, and fixups.
> > > +
> > > +        Args:
> > > +            filename: Display name or file path for error reporting.
> > > +            raw_bytes: Uncompressed FDT binary contents as `bytes`.
> > > +        """
> > > +        self.filename = filename
> > > +        self.raw_bytes = raw_bytes
> > > +        fdt = _open_fdt_ro(raw_bytes, filename)
> >
> > This is the only caller of _open_fdt_ro(), which itself is not very
> > long.  Any reason not to just open code it here?
> 
> Done.
> 
> 
> > > +        if _is_dtbo_overlay(fdt):
> > > +            raise ValueError(
> > > +                f'{filename}: Input is already a DTB overlay (.dtbo)'
> > > +            )
> > > +
> > > +        # Extract FDT header metadata (boot_cpuid_phys and /memreserve/ table)
> > > +        # stored outside the DT node/property tree; see _validate_dtb_headers()
> > > +        # for why these require special handling.
> > > +        self.boot_cpuid_phys = fdt.boot_cpuid_phys()
> > > +        self.mem_rsv = [
> > > +            tuple(fdt.get_mem_rsv(i)) for i in range(fdt.num_mem_rsv())
> > > +        ]
> >
> > I wonder if "get all mem reserves" would be a useful method to add to FdtRo.
> 
> Done. get_mem_rsv_list() added to FdtRo.
> 
> 
> > > +        self.has_symbols = False
> > > +        self.symbols = {}
> > > +        self.has_local_fixups = False
> > > +        self.local_fixups = {}
> >
> > What's the reason for the separate has_* fields, rather than encoding
> > that as an empty self.symbols or self.local_fixups?
> 
> Removed.
> 
> 
> > > +        self.nodes = {}
> > > +        self.phandles = {}
> > > +        self.has_phandle_prop = {}
> > > +        self.has_linux_phandle_prop = {}
> > > +        self.phandle_to_path = {}
> > > +        self.sym_props = {}
> > > +
> > > +        self._parse_tree(fdt)
> > > +
> > > +    @classmethod
> > > +    def from_file(cls, path):
> >
> > Why a class method rather than doing this in __init__()?
> 
> This is basically just the pythonic way of providing an alternate
> constructor, right? In v1, the main __init__ was used for the
> "--verify" option, which is now gone. So yeah, we can make
> constructing from a path be the only constructor and move to
> __init__.
> 
> 
> > > +    def _parse_tree(self, fdt):
> > > +        """Walk the FDT and extract nodes, properties, symbols, and fixups.
> > > +
> > > +        Args:
> > > +            fdt: A `libfdt.FdtRo` instance wrapping the input blob.
> > > +        """
> > > +        self._walk_regular(fdt, 0, '/')
> > > +
> > > +        sym_off = fdt.subnode_offset(0, '__symbols__',
> > > +                                     quiet=libfdt.QUIET_NOTFOUND)
> >
> > You already unflattened the tree, but deliberately excluded
> > __symbols__.  Both clearer and more efficient to *not* exclude it from
> > the unflattening, then read the symbols from the unflattened version,
> > rather than re-parsing.
> 
> Done. NodeTree.unflatten() now unflattens the entire FDT (including
> /__symbols__ and /__local_fixups__), and ParsedDtb pops __symbols__ and
> __local_fixups__ from self.root.children and processes them directly from
> the unflattened NodeTree nodes.

Great,.

> > > +        if sym_off >= 0:
> > > +            self.has_symbols = True
> > > +            poff = fdt.first_property_offset(sym_off,
> > > +                                             quiet=libfdt.QUIET_NOTFOUND)
> > > +            while poff >= 0:
> > > +                prop = fdt.get_property_by_offset(poff)
> > > +                if prop.name not in ('phandle', 'linux,phandle'):
> >
> > Huh, __symbols__ typically won't have a phandle, but yeah, we can't
> > exclude the possibility.  Another ugliness in the overlay format.
> 
> Sounds good. Keeping this.
> 
> 
> > > +                    target_path = prop.as_str() if prop else ''
> >
> > As everywhere, I think it would be safer to treat paths and node names
> > as bytestrings.
> 
> Not doing this. See above.
> 
> 
> > > +                    if target_path not in self.nodes:
> > > +                        raise ValueError(
> > > +                            f'{self.filename}: symbol {prop.name!r} references '
> > > +                            f'missing node {target_path!r}'
> > > +                        )
> > > +                    self.symbols[prop.name] = target_path
> > > +                poff = fdt.next_property_offset(poff,
> > > +                                                quiet=libfdt.QUIET_NOTFOUND)
> > > +
> > > +        lf_off = fdt.subnode_offset(0, '__local_fixups__',
> > > +                                    quiet=libfdt.QUIET_NOTFOUND)
> > > +        if lf_off >= 0:
> > > +            self.has_local_fixups = True
> > > +            self._walk_local_fixups(fdt, lf_off, '/')
> > > +
> > > +    def _walk_regular(self, fdt, nodeoffset, path):
> > > +        """Recursively collect regular nodes and their properties.
> >
> > I'd suggest "unflatten" as the name by analogy with similar code in
> > dtc and the kernel.
> 
> This became unflatten() in NodeTree, which is small/simple. Other work
> in ParsedDtb happens in separate passes.
> 
> 
> > > +        Args:
> > > +            fdt: A `libfdt.FdtRo` instance.
> > > +            nodeoffset: Offset of the current node in `fdt`.
> > > +            path: Full device tree path of the current node (e.g. '/soc').
> > > +        """
> > > +        props = {}
> > > +        ph_val = None
> > > +        has_ph = False
> > > +        has_linux_ph = False
> > > +
> > > +        poff = fdt.first_property_offset(nodeoffset,
> > > +                                         quiet=libfdt.QUIET_NOTFOUND)
> > > +        while poff >= 0:
> > > +            prop = fdt.get_property_by_offset(poff)
> > > +            raw = bytes(prop)
> > > +            if prop.name in ('phandle', 'linux,phandle'):
> >
> > It would be slightly less efficient, but I think recording the
> > phandles as a second pass rather than while you're parsing the
> > structure might be clearer.
> 
> Done.
> 
> 
> > > +                if len(raw) != 4:
> >
> > You can use len() directly on prop, as a subclass of bytearray.
> 
> Done (via NodeTree.get_u32()).
> 
> 
> > > +                    raise ValueError(
> > > +                        f'{self.filename}: {path}: {prop.name} length '
> > > +                        f'{len(raw)} != 4'
> > > +                    )
> > > +                val = struct.unpack('>I', raw)[0]
> >
> > You can use Property.as_uint32() rather than opencoding a
> > struct.unpack.
> 
> Done (via NodeTree.get_u32()).
> 
> 
> > > +                if ph_val is not None and ph_val != val:
> > > +                    raise ValueError(
> > > +                        f'{self.filename}: {path}: mismatched phandle '
> > > +                        f'({ph_val:#x} vs {val:#x})'
> > > +                    )
> > > +                ph_val = val
> > > +                if prop.name == 'phandle':
> > > +                    has_ph = True
> > > +                else:
> > > +                    has_linux_ph = True
> > > +            else:
> > > +                props[prop.name] = raw
> > > +            poff = fdt.next_property_offset(poff, quiet=libfdt.QUIET_NOTFOUND)
> > > +
> > > +        self.nodes[path] = props
> >
> > Wait... what?  You're not storing this as the NodeTree structure you
> > established above?  That's weird.
> >
> > > +        self.phandles[path] = ph_val
> > > +        self.has_phandle_prop[path] = has_ph
> > > +        self.has_linux_phandle_prop[path] = has_linux_ph
> >
> > Why parallel maps for all these, rather than a single Node object?
> > Or NodeTree, since you have that already.
> 
> Above two comments are handled and stuff is stored in NodeTree.
> 
> 
> > > +        if ph_val is not None:
> > > +            if ph_val in (0, 0xffffffff):
> > > +                raise ValueError(
> > > +                    f'{self.filename}: invalid phandle {ph_val:#x} on {path}'
> > > +                )
> > > +            if ph_val in self.phandle_to_path:
> > > +                raise ValueError(
> > > +                    f'{self.filename}: duplicate phandle {ph_val:#x} on '
> > > +                    f'{self.phandle_to_path[ph_val]} and {path}'
> > > +                )
> > > +            self.phandle_to_path[ph_val] = path
> > > +
> > > +        soff = fdt.first_subnode(nodeoffset, quiet=libfdt.QUIET_NOTFOUND)
> > > +        while soff >= 0:
> >
> > Hm, node and property iterators would probably be a good addition to
> > FdtRo (should be fairly easy using generators).
> 
> Done in an earlier patch in the series.
> 
> 
> > > +            name = fdt.get_name(soff)
> > > +            if path == '/' and name in ('__symbols__', '__local_fixups__'):
> > > +                soff = fdt.next_subnode(soff, quiet=libfdt.QUIET_NOTFOUND)
> > > +                continue
> >
> > I'd *definitely* suggest that parsing symbols and fixups be handled in
> > a later pass rather than linline during parsing.
> 
> _parse_symbols() is now a separate pass.
> 
> 
> > > +            child_path = '/' + name if path == '/' else f'{path}/{name}'
> > > +            self._walk_regular(fdt, soff, child_path)
> > > +            soff = fdt.next_subnode(soff, quiet=libfdt.QUIET_NOTFOUND)
> > > +
> > > +    def _walk_local_fixups(self, fdt, nodeoffset, rel_path):
> > > +        """Recursively collect phandle byte offsets from `/__local_fixups__`.
> > > +
> > > +        Args:
> > > +            fdt: A `libfdt.FdtRo` instance.
> > > +            nodeoffset: Offset of the current node inside `/__local_fixups__`.
> > > +            rel_path: Corresponding target node path in the device tree.
> >
> > 'rel_path' is a bad name, since this is an absolute path in the tree.
> 
> Updated.
> 
> 
> > > +        Raises:
> > > +            ValueError: If a fixup property length is not a multiple of 4.
> > > +        """
> > > +        poff = fdt.first_property_offset(nodeoffset,
> > > +                                         quiet=libfdt.QUIET_NOTFOUND)
> >
> > Again, this would be qeasier if you used the unflattened __local_fixups__.
> 
> Done.
> 
> 
> > > +        while poff >= 0:
> > > +            prop = fdt.get_property_by_offset(poff)
> > > +            raw = bytes(prop)
> > > +            if len(raw) % 4 != 0:
> >
> > Again, you can use len() directly on prop.
> 
> Done (via NodeTree.get_u32_list()).
> 
> 
> > > +                raise ValueError(
> > > +                    f'{self.filename}: malformed __local_fixups__ property '
> > > +                    f'{rel_path}:{prop.name} (length {len(raw)})'
> > > +                )
> > > +            count = len(raw) // 4
> > > +            offsets = struct.unpack(f'>{count}I', raw)
> >
> > and as_uint32_list().
> 
> Done (via NodeTree.get_u32_list()).
> 
> 
> > > +    def build_symbolic_props(self):
> > > +        """Build normalized property values for comparison across DTBs.
> > > +
> > > +        Populates `self.sym_props[path][prop_name]` with a tuple:
> > > +            `(masked_bytes, ((offset, target_node_path), ...))`
> > > +        where phandle cells listed in `self.local_fixups` are zeroed in
> > > +        `masked_bytes` and recorded symbolically by their target node path.
> > > +
> > > +        Raises:
> > > +            ValueError: If a fixup references a missing property, its offset is
> > > +                out of range, or it references an unknown phandle value.
> > > +        """
> > > +        for path, prop_name in self.local_fixups:
> > > +            if path not in self.nodes or prop_name not in self.nodes[path]:
> > > +                raise ValueError(
> > > +                    f'{self.filename}: local fixup references missing property '
> > > +                    f'{path}:{prop_name}'
> > > +                )
> >
> > Probably makes more sense for this check to happen while parsing
> > local_fixups than afterwards here.
> 
> Done.
> 
> 
> > > +        for path, props in self.nodes.items():
> > > +            node_sym_props = {}
> > > +            for prop_name, raw in props.items():
> > > +                offsets = self.local_fixups.get((path, prop_name), ())
> >
> > Walking the whole tree, then looking up if it is in local_fixups seems
> > the wrong way around.  Better to walk local_fixups and go to the right
> > place in the tree to process it.
> 
> Done.
> 
> 
> > > +                if not offsets:
> > > +                    node_sym_props[prop_name] = (raw, ())
> > > +                    continue
> > > +                masked = bytearray(raw)
> > > +                targets = []
> > > +                for off in offsets:
> > > +                    if off + 4 > len(raw):
> > > +                        raise ValueError(
> > > +                            f'{self.filename}: fixup offset {off} out of range '
> > > +                            f'on {path}:{prop_name} (len {len(raw)})'
> > > +                        )
> > > +                    ph = struct.unpack_from('>I', raw, off)[0]
> > > +                    target_path = self.phandle_to_path.get(ph)
> > > +                    if target_path is None:
> > > +                        raise ValueError(
> > > +                            f'{self.filename}: unknown phandle {ph:#x} at '
> > > +                            f'{path}:{prop_name}+{off}'
> > > +                        )
> > > +                    masked[off:off + 4] = b'\x00\x00\x00\x00'
> > > +                    targets.append((off, target_path))
> > > +                node_sym_props[prop_name] = (bytes(masked), tuple(targets))
> > > +            self.sym_props[path] = node_sym_props
> >
> > I don't love the name 'sym_props' here, since this is recording fixups
> > in the property, not symbols.  Also why store the masked property
> > value here in a tuple, rather than updating self.node[path][prop_name]
> > with the masked value?
> 
> Done. _apply_local_fixups() now zeros the phandle cells directly in
> target_node.props[prop_name] and stores the fixups in
> target_node.fixups[prop_name].
> 
> 
> > > +
> > > +
> > > +def select_best_candidate(candidates, byte_len_fn):
> > > +    """Choose the value that maximizes space savings across overlays.
> > > +
> > > +    Only candidates shared by at least two input DTBs (count > 1) are eligible
> > > +    for inclusion in the base DTB; if every candidate value is unique to a
> > > +    single DTB, placing one in the base DTB would yield zero net size savings
> > > +    (adding one copy to the base DTB to save one overlay copy).
> >
> > Beware a wrinkle here.  dtbos have no way of deleting a property from
> > the base tree.  So if *any* of the input trees is missing the
> > property, it cannot go into the base tree, even if it's present and
> > identical in all the others.
> 
> Right. _build_base_tree() only calls select_best_candidate() on
> common_prop_names (properties that exist in all input DTBs for that node).
> 
> 
> > > +    Args:
> > > +        candidates: Iterable of candidate values (in input DTB order).
> > > +        byte_len_fn: Function returning the byte cost of a candidate value in
> > > +            an overlay.
> >
> > AFAICT select_best_candidate() is always passed the same byte_len_fn,
> > so I'm not sure that it's a useful abstraction not to just it's
> > behaviour in here.
> 
> Done. Inlined the byte cost calculation directly into
> select_best_candidate().
> 
> 
> > > +    Returns:
> > > +        The chosen candidate value, or None if no value appears more than once.
> > > +    """
> > > +    counts = Counter(candidates)
> > > +    best_val = None
> > > +    best_key = None
> > > +    for val, count in counts.items():
> > > +        if count <= 1:
> > > +            continue
> > > +        length = byte_len_fn(val)
> > > +        # Maximize total bytes saved across overlays:
> > > +        # (count - 1) * (length + 12)
> >
> > Where is the + 12 coming from?
> 
> Added an explanation in the docstring: each FDT property in the structure
> block carries a 12-byte header (4-byte FDT_PROP tag + 4-byte length +
> 4-byte nameoff) in addition to its value bytes.
> 
> 
> > > +def _find_common_nodes_and_phandles(dtbs):
> > > +    """Identify common node paths and assign sequential base DTB phandles.
> > > +
> > > +    Any common node that carries a phandle in any input DTB is assigned a
> > > +    phandle in the base DTB so overlays never attempt to add a `phandle`
> > > +    property to an already-existing base DTB node (which Linux and
> > > +    coreboot/depthcharge disallow).
> > > +
> > > +    Args:
> > > +        dtbs: Sequence of `ParsedDtb` objects.
> > > +
> > > +    Returns:
> > > +        Tuple of `(base_dtb_nodes, common_node_set, base_dtb_phandles)` where:
> > > +        * `base_dtb_nodes`: Ordered list of node paths common to all `dtbs`.
> >
> > Does the order matter?  If not, I'd just say 'list'.
> >
> > > +        * `common_node_set`: Set of node paths in `base_dtb_nodes`.
> >
> >
> > Isn't this the same information as base_dtb_nodes?
> >
> > > +        * `base_dtb_phandles`: Dict mapping node path to assigned integer
> > > +          phandle in the base DTB.
> >
> > And isn't this redundant with the above.  Could we instead return a
> > single dictionary: All common nodes get an entry, with 'None' as the
> > value if it doesn't need the phandle, or an actual value if it does
> > need a phandle.
> 
> Done. _find_common_nodes_and_phandles() now directly builds and returns
> the common base_root NodeTree with each node's phandle attribute set to
> its assigned base DTB phandle (or None if it doesn't need one),
> eliminating the separate dicts/sets altogether.
> 
> 
> > > +def _find_common_symbols(dtbs, common_node_set, base_dtb_phandles):
> > > +    """Find symbols common to all input DTBs that point to the same base node.
> > > +
> > > +    Only symbols that exist in all DTBs and resolve to the same common base
> > > +    DTB node path are placed in the base DTB's `/__symbols__`, ensuring
> > > +    overlays only add new symbols and never need to overwrite existing ones
> > > +    (which coreboot/depthcharge merges with `upd=0`).
> > > +
> > > +    Args:
> > > +        dtbs: Sequence of `ParsedDtb` objects.
> > > +        common_node_set: Set of node paths present in all input DTBs.
> > > +        base_dtb_phandles: Dict mapping base DTB node paths to their assigned
> > > +            integer phandles.
> > > +    Returns:
> > > +        Tuple of `(base_dtb_symbols, path_to_base_dtb_symbol)` where:
> > > +        * `base_dtb_symbols`: Dict mapping symbol label to target node path.
> > > +        * `path_to_base_dtb_symbol`: Dict mapping target node path to its
> > > +          primary symbol label in `base_dtb_symbols`.
> > > +    """
> > > +    base_dtb_symbols = {}
> > > +    path_to_base_dtb_symbol = {}
> > > +
> > > +    common_sym_names = set(dtbs[0].symbols.keys())
> > > +    for d in dtbs[1:]:
> > > +        common_sym_names &= set(d.symbols.keys())
> > > +    for label in dtbs[0].symbols.keys():
> > > +        if label not in common_sym_names:
> > > +            continue
> >
> > for label in common_sym_names:
> >
> > would suffice and be clearer, no?
> 
> Done.
> 
> 
> > > +        target_paths = {d.symbols[label] for d in dtbs}
> > > +        if len(target_paths) == 1:
> >
> > To avoid excessive indentation, I'd prefer if len(target_paths) != 1: continue
> 
> Done.
> 
> 
> > > +            chosen_path = dtbs[0].symbols[label]
> > > +            if chosen_path in common_node_set:
> >
> > Ditto.
> 
> Done.
> 
> 
> > > +def _select_base_dtb_properties(dtbs, base_dtb_nodes, base_dtb_phandles,
> > > +                                preserve_local_fixups):
> > > +    """Select the common property values to place on each node in the base DTB.
> > > +
> > > +    Args:
> > > +        dtbs: Sequence of `ParsedDtb` objects with `sym_props` populated.
> > > +        base_dtb_nodes: Ordered list of node paths in the base DTB.
> > > +        base_dtb_phandles: Dict mapping base DTB node paths to their assigned
> > > +            integer phandles.
> > > +        preserve_local_fixups: Whether `/__local_fixups__` from the input DTBs
> > > +            must be preserved in the merged output.
> > > +
> > > +    Returns:
> > > +        Tuple of `(base_dtb_props, base_dtb_fixup_offsets)` where:
> > > +        * `base_dtb_props`: Dict mapping `path -> {prop_name: sym_val}` for
> > > +          properties placed in the base DTB.
> > > +        * `base_dtb_fixup_offsets`: Dict mapping `(path, prop_name)` to a tuple
> > > +          of byte offsets for the base DTB's `/__local_fixups__`.
> > > +    """
> > > +    base_dtb_props = {}
> > > +    base_dtb_fixup_offsets = {}
> > > +
> > > +    for path in base_dtb_nodes:
> > > +        common_prop_names = set(dtbs[0].nodes[path].keys())
> > > +        for d in dtbs[1:]:
> > > +            common_prop_names &= set(d.nodes[path].keys())
> > > +
> > > +        node_base_props = {}
> > > +        for prop_name in dtbs[0].nodes[path].keys():
> > > +            if prop_name not in common_prop_names:
> > > +                continue
> >
> > Again for prop_name in common_prop_names: should do the job, no?
> 
> Done.
> 
> 
> > > +
> > > +            all_have_prop_fixups = all(
> > > +                bool(d.sym_props[path][prop_name][1]) for d in dtbs
> > > +            )
> >
> > I'm not sure why you care that all the copies of the property have
> > fixups.  AFAICT all you care about is that the set of fixups is
> > identical for all of them, empty or not.
> 
> Sigh, this is where me not understanding 100% of the details of overlays
> bites me. Some background. I remember there being some talk a little while
> ago of (perhaps) shipping device trees compiled with '-L'. That would
> allow the OS to know for sure which properties represented phandles and
> that could help the OS figure out device dependencies.

[Not really relevant to this series, but that doesn't sound like a
 great idea to me]

> I wanted to make
> sure that running fdtfactor wouldn't break this and the OS would still
> have all the information it needed even after breaking things up into
> overlays and re-applying.
> 
> In order to test this, I wanted to make sure we tested re-combining the
> base devicetrees and overlays and we could confirm that everything was
> there. In order to make all that work, my agent added a bunch of extra
> '/fragment@0/__overlay__/__local_fixups__'. ...but it turns out that was
> a workaround for the fact that fdt_overlay_apply() didn't preserve
> __local_fixups__ in the base. :(

Ah, right.  Again, I'm not convinced this is a great idea, but if we
wanted it the right way to do it is to have fdt_overlay_apply() 
(optionally) regenerate __local_fixups__.  It's not really merely
"preserving", since applying the overlay might require that some
external fixups be converted to local fixups.

> I'll give a shot at adding a patch to fix that and hopefully things will
> be a lot cleaner.
> 
> 
> > > +            eligible = []
> > > +            for d in dtbs:
> > > +                sym_val = d.sym_props[path][prop_name]
> > > +                _, targets = sym_val
> >
> > These two lines could be one with no loss of clarity.
> 
> Code is pretty different now.
> 
> 
> > > +                if targets:
> > > +                    # Overlays cannot delete a property from /__local_fixups__,
> > > +                    # so a phandle-bearing value can only go into base.dtb when
> > > +                    # every board also has phandle fixups on this property.
> >
> > True, but more generally 1) you can't make *any* property common if it
> > doesn't exist in all the inputs and 2) the fixups must be identical
> > for all inputs, empty or otherwise.
> 
> With the fdt_overlay_apply() fix in place, all of this special-casing
> for preserve_local_fixups is gone.

Hooray.

> > > +                    if preserve_local_fixups and not all_have_prop_fixups:
> > > +                        continue
> > > +                    if not all(
> > > +                        t_path in base_dtb_phandles for _, t_path in targets
> > > +                    ):
> > > +                        continue
> > > +                eligible.append(sym_val)
> > > +
> > > +            fixup_entry_cost = len(path) + len(prop_name) + 28
> >
> > These needs an explanation of how you arrived at that formula.
> 
> Done. Documented the byte cost breakdown in select_best_candidate()'s
> docstring.
> 
> 
> > > +def _build_base_dtb(dtbs, base_dtb_nodes, base_dtb_props, base_dtb_phandles,
> > > +                    base_dtb_symbols, base_dtb_fixup_offsets):
> > > +    """Construct and serialize the common base DTB blob.
> > > +
> > > +    Args:
> > > +        dtbs: Sequence of `ParsedDtb` objects.
> > > +        base_dtb_nodes: Ordered list of node paths in the base DTB.
> > > +        base_dtb_props: Dict mapping `path -> {prop_name: sym_val}`.
> > > +        base_dtb_phandles: Dict mapping base DTB node paths to their assigned
> > > +            integer phandles.
> > > +        base_dtb_symbols: Dict mapping symbol label to target node path for the
> > > +            base DTB's `/__symbols__`.
> > > +        base_dtb_fixup_offsets: Dict mapping `(path, prop_name)` to phandle
> > > +            byte offsets for the base DTB's `/__local_fixups__`.
> > > +
> > > +    Returns:
> > > +        Serialized base DTB blob as `bytes`.
> > > +    """
> > > +    base_dtb_root = NodeTree()
> >
> > Using the NodeTree() representation only as an interim step to
> > flattening seems odd to me.  Either use NodeTree (or something like
> > it) throughout, or go directly from the internal cluster-of-maps
> > representation into a flattened tree.
> 
> Done. NodeTree is now used throughout.
> 
> 
> > > +    for path in base_dtb_nodes:
> > > +        node = base_dtb_root.get_node(path)
> > > +        for prop_name, (masked_bytes, targets) in base_dtb_props[path].items():
> > > +            if not targets:
> > > +                node.props[prop_name] = masked_bytes
> > > +            else:
> > > +                val = bytearray(masked_bytes)
> > > +                for off, t_path in targets:
> > > +                    struct.pack_into('>I', val, off, base_dtb_phandles[t_path])
> > > +                node.props[prop_name] = bytes(val)
> > > +
> > > +        if path in base_dtb_phandles:
> > > +            ph_bytes = struct.pack('>I', base_dtb_phandles[path])
> > > +            if any(d.has_phandle_prop[path] for d in dtbs):
> > > +                node.props['phandle'] = ph_bytes
> > > +            if any(d.has_linux_phandle_prop[path] for d in dtbs):
> > > +                node.props['linux,phandle'] = ph_bytes
> >
> > I'd suggest always just using 'phandle' since that's the modern form.
> 
> Done.
> 
> 
> > > +
> > > +    if base_dtb_symbols:
> > > +        sym_node = base_dtb_root.get_node('/__symbols__')
> > > +        for label, target_path in base_dtb_symbols.items():
> > > +            sym_node.props[label] = target_path.encode('utf-8') + b'\x00'
> >
> > Paths are not UTF-8, they're bytestrings to begin with.
> 
> No. See elsewhere.
> 
> 
> > > +def _encode_dtbo_property(path, prop_name, sym_val, base_dtb_phandles,
> > > +                          path_to_base_dtb_symbol, local_phandles, ext_fixups,
> > > +                          ov_local_fixups):
> > > +    """Encode a property value for `/fragment@0/__overlay__` and record fixups.
> > > +
> > > +    Args:
> > > +        path: Target node path in the device tree (e.g. '/soc/i2c@4000').
> > > +        prop_name: Property name.
> > > +        sym_val: Normalized `(masked_bytes, targets)` tuple for the property.
> > > +        base_dtb_phandles: Dict mapping base DTB node paths to their phandles.
> > > +        path_to_base_dtb_symbol: Dict mapping base DTB node paths to symbol
> > > +            names in the base DTB's `/__symbols__`.
> > > +        local_phandles: Dict mapping board-only node paths to overlay-local
> > > +            phandles.
> > > +        ext_fixups: Dict of `label -> [fixup_str, ...]` updated in-place for
> > > +            references to base DTB symbols.
> > > +        ov_local_fixups: Dict of `(path, prop_name) -> [offset, ...]` updated
> > > +            in-place for references to overlay-local phandles.
> > > +
> > > +    Returns:
> > > +        Encoded property value as `bytes`.
> > > +    """
> > > +    masked_bytes, targets = sym_val
> > > +    if not targets:
> > > +        return masked_bytes
> > > +
> > > +    val = bytearray(masked_bytes)
> > > +    for off, t_path in targets:
> > > +        if t_path in base_dtb_phandles:
> > > +            sym_label = path_to_base_dtb_symbol.get(t_path)
> > > +            if sym_label is not None:
> > > +                struct.pack_into('>I', val, off, 0xffffffff)
> > > +                ov_path = (
> > > +                    '/fragment@0/__overlay__'
> > > +                    if path == '/'
> > > +                    else f'/fragment@0/__overlay__{path}'
> > > +                )
> >
> > Putting everything in /fragment@0 is kind of nasty.  It would be
> > preferable to search for useful places to put fragments across the
> > input dtbs.  But, I'm fine if that is a later improvement, rather than
> > the original version.
> 
> I added a TODO item for this. I'm a bit curious, though. In general
> these overlays aren't _really_ expected to be human readable. It's
> just about using overlays to make things small. As I understand it,
> we won't really get any space savings by being smarter here. My
> agent thinks that in a few very targeted cases we could get a
> tiny space savings, but otherwise putting everything in '/fragment@0'
> is actually a better solution, even if it would be ugly for someone
> directly reading the overlay.

Hm, fair point.  It probably would be quite a lot of work, for minimal
gain.

I would still suggest a cut down version of this: in the case that all
the overlay changes are within a subtree, have the single fragment be
applied there, rather than at /.  It again won't gain a whole lot in
size, but it's much easier to implement and will make for a more
elegant output.  (Can still be after the first cut patch, though).

> > > +                entry = f'{ov_path}:{prop_name}:{off}'
> > > +                ext_fixups.setdefault(sym_label, []).append(entry)
> > > +            else:
> > > +                # Target is a common base DTB node without a symbol shared by
> > > +                # all input DTBs (e.g. an anonymous &{/path} reference or a
> > > +                # board-specific label); use its assigned base DTB phandle.
> > > +                struct.pack_into('>I', val, off, base_dtb_phandles[t_path])
> >
> > This creates what's arguably an invalid overlay - it assumes the value
> > of a phandle in the base tree.  I'd prefer to see this case rejected
> > outright.
> 
> FWIW, I'm a bit out of my league in overlay understanding here. I'm going
> to be lame and paste in my AI's answer here. Sorry. :( If this makes no
> sense, I'll go educate myself:
> 
> Agreed. In v2 I've removed that fallback so fdtfactor never emits an overlay
> property with a hardcoded base-tree phandle. Instead, if a non-root node has
> a phandle in some input DTB but lacks a common symbol in /__symbols__ across
> all input DTBs (which actually happens in the downstream Pixel 10 trees where
> two boards define /module_power_seq@2 with different label names),
> _find_common_nodes_and_phandles() keeps that node out of base.dtb so each
> board's .dtbo creates it with a normal overlay-local phandle and
> /__local_fixups__. If the root node / itself ever had a phandle without a
> common symbol and was referenced by an overlay property, fdtfactor rejects
> it outright with an error.

Ehh.. not sure it shows a total understanding, but it's probably good
enough to work with for now.

> > > +def _populate_dtbo_overlay_nodes(ov_root, dtb, common_node_set, base_dtb_props,
> > > +                                 base_dtb_phandles, path_to_base_dtb_symbol,
> > > +                                 local_phandles):
> > > +    """Populate `/fragment@0/__overlay__` with board-specific nodes and props.
> > > +
> > > +    Args:
> > > +        ov_root: The `NodeTree` for `/fragment@0/__overlay__`.
> > > +        dtb: The board's `ParsedDtb`.
> > > +        common_node_set: Set of node paths present in the base DTB.
> > > +        base_dtb_props: Dict mapping `path -> {prop_name: sym_val}` in the
> > > +            base DTB.
> > > +        base_dtb_phandles: Dict mapping base DTB node paths to their phandles.
> > > +        path_to_base_dtb_symbol: Dict mapping base DTB node paths to symbol
> > > +            names in the base DTB's `/__symbols__`.
> > > +        local_phandles: Dict mapping board-only node paths to overlay-local
> > > +            phandles.
> > > +
> > > +    Returns:
> > > +        Tuple of `(ext_fixups, ov_local_fixups)` collected across all overlay
> > > +        properties.
> > > +    """
> > > +    ext_fixups = {}
> > > +    ov_local_fixups = {}
> > > +
> > > +    for path, props in dtb.nodes.items():
> > > +        in_base_dtb = path in common_node_set
> > > +        base_node_props = base_dtb_props.get(path, {})
> > > +
> > > +        diff_props = [
> > > +            (prop_name, dtb.sym_props[path][prop_name])
> > > +            for prop_name in props.keys()
> > > +            if (not in_base_dtb or
> > > +                base_node_props.get(prop_name) !=
> > > +                dtb.sym_props[path][prop_name])
> > > +        ]
> > > +
> > > +        if not in_base_dtb or diff_props:
> >
> > diff_props is necessarily empty if not in_base_dtb, so the first term
> > is redundant.
> 
> Actually, a board-only node (not in_base_dtb) might have no properties of
> its own (e.g. an empty container node or one that only has subnodes), in
> which case diff_props is empty but we still need to create the node in the
> overlay.

Ah, yes, sorry.

> > > +            ov_node = ov_root.get_node(path)
> > > +            for prop_name, sym_val in diff_props:
> > > +                ov_node.props[prop_name] = _encode_dtbo_property(
> > > +                    path, prop_name, sym_val, base_dtb_phandles,
> > > +                    path_to_base_dtb_symbol, local_phandles, ext_fixups,
> > > +                    ov_local_fixups,
> > > +                )
> > > +
> > > +            if path in local_phandles:
> > > +                ph_bytes = struct.pack('>I', local_phandles[path])
> > > +                if dtb.has_phandle_prop[path]:
> > > +                    ov_node.props['phandle'] = ph_bytes
> > > +                if dtb.has_linux_phandle_prop[path]:
> > > +                    ov_node.props['linux,phandle'] = ph_bytes
> >
> > Again, I'd suggest just standardising on the modern 'phandle'.
> 
> Done.
> 
> 
> > > +    return ext_fixups, ov_local_fixups
> > > +
> > > +
> > > +def _populate_dtbo_metadata(dtbo_root, ov_root, dtb, base_dtb_symbols,
> > > +                            base_dtb_fixup_offsets, ext_fixups,
> > > +                            ov_local_fixups, preserve_local_fixups):
> > > +    """Populate `/__symbols__`, `/__fixups__`, and `/__local_fixups__` on DTBO.
> > > +
> > > +    Args:
> > > +        dtbo_root: Root `NodeTree` of the `.dtbo` overlay.
> > > +        ov_root: The `NodeTree` for `/fragment@0/__overlay__`.
> > > +        dtb: The board's `ParsedDtb`.
> > > +        base_dtb_symbols: Dict of symbols already present in the base DTB.
> > > +        base_dtb_fixup_offsets: Dict of `/__local_fixups__` offsets already
> > > +            present in the base DTB.
> > > +        ext_fixups: Dict of `label -> [fixup_str, ...]` referencing base DTB
> > > +            symbols.
> > > +        ov_local_fixups: Dict of `(path, prop_name) -> [offset, ...]`
> > > +            referencing overlay-local phandles.
> > > +        preserve_local_fixups: Whether to emit differing input
> > > +            `/__local_fixups__` entries into
> > > +            `/fragment@0/__overlay__/__local_fixups__`.
> >
> > That's not right.  The overlay should only have a global
> > /__local_fixups__, there shouldn't be __local_fixups__ for individual
> > fragments.
> 
> See above where I talk about fdt_overlay_apply() behavior. Gone for v2.
> 
> 
> > > +    """
> > > +    # If preserving input /__local_fixups__ in the merged tree, place any
> > > +    # differing fixup offset lists under
> > > +    # /fragment@0/__overlay__/__local_fixups__
> > > +    if preserve_local_fixups:
> > > +        for (path, prop_name), offs in dtb.local_fixups.items():
> > > +            if base_dtb_fixup_offsets.get((path, prop_name)) != offs:
> > > +                merged_lf_root = ov_root.get_node('/__local_fixups__')
> > > +                merged_lf_node = merged_lf_root.get_node(path)
> > > +                merged_lf_node.props[prop_name] = struct.pack(
> > > +                    f'>{len(offs)}I', *offs
> > > +                )
> > > +
> > > +    # Emit overlay root /__symbols__ for new or modified symbols
> > > +    diff_syms = [
> > > +        (label, t_path)
> > > +        for label, t_path in dtb.symbols.items()
> > > +        if base_dtb_symbols.get(label) != t_path
> > > +    ]
> > > +    if diff_syms:
> > > +        sym_node = dtbo_root.get_node('/__symbols__')
> > > +        for label, t_path in diff_syms:
> > > +            # Ensure target node exists in __overlay__ so dtc -I dtb -O dts
> > > +            # on the standalone .dtbo does not warn about a missing node.
> > > +            ov_root.get_node(t_path)
> > > +            ov_target = (
> > > +                '/fragment@0/__overlay__'
> > > +                if t_path == '/'
> > > +                else f'/fragment@0/__overlay__{t_path}'
> >
> > These path == '/' special cases appear in a bunch of places.  I think
> > you could avoid them all with some tweaks to your conventions about
> > paths (e.g. always keep them as relative paths to the root, allowing
> > '').
> 
> Done. Normalized paths now use '' for the root node and '/...' for
> subnodes, eliminating the path == '/' special cases.
> 
> 
> > > +            )
> > > +            sym_node.props[label] = ov_target.encode('utf-8') + b'\x00'
> >
> > Neither property names nor property values were every Unicode.
> 
> See above regarding pylibfdt's string conventions (this now uses
> NodeTree.set_str()).
> 
> 
> > > +def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> > > +                         strip_local_fixups):
> > > +    """Apply `dtbo_blob` to `base_dtb_blob` and verify it matches `orig_dtb`.
> > > +
> > > +    Args:
> > > +        orig_dtb: The original `ParsedDtb` instance for the board.
> > > +        base_dtb_blob: Uncompressed base DTB binary as `bytes`.
> > > +        dtbo_blob: Uncompressed board `.dtbo` overlay binary as `bytes`.
> > > +        strip_local_fixups: Whether `/__local_fixups__` was stripped.
> > > +
> > > +    Raises:
> > > +        RuntimeError: If `fdt_overlay_apply()` fails or any node, property,
> > > +            phandle, symbol, or header field in the merged DTB differs from
> > > +            `orig_dtb`.
> > > +    """
> > > +    merged_fdt = libfdt.Fdt(base_dtb_blob)
> > > +    merged_fdt.resize(len(base_dtb_blob) + len(dtbo_blob) * 2 + 65536)
> > > +    fdto = bytearray(dtbo_blob)
> > > +    err = libfdt.fdt_overlay_apply(merged_fdt._fdt, fdto)
> >
> > We should probably make a method wrapper for that in libfdt.Fdt.
> 
> verify_recreated_dtb() is gone now (replaced by fdtoverlay +
> dtbs_equal_unordered in tests/run_tests.sh), so fdtfactor no longer calls
> fdt_overlay_apply() from Python.
> 
> 
> > > +    if err != 0:
> > > +        raise RuntimeError(
> > > +            f'{orig_dtb.filename}: fdt_overlay_apply failed with error {err} '
> > > +            f'({libfdt.strerror(err)})'
> > > +        )
> > > +    merged_fdt.pack()
> > > +    recreated = ParsedDtb(
> > > +        f'{orig_dtb.filename} (recreated)', bytes(merged_fdt.as_bytearray())
> > > +    )
> > > +    if strip_local_fixups:
> > > +        if recreated.has_local_fixups:
> > > +            raise RuntimeError(
> > > +                f'{orig_dtb.filename}: /__local_fixups__ present despite '
> > > +                f'--strip-local-fixups'
> > > +            )
> > > +        recreated.local_fixups = orig_dtb.local_fixups
> > > +    else:
> > > +        if orig_dtb.local_fixups != recreated.local_fixups:
> > > +            raise RuntimeError(
> > > +                f'{orig_dtb.filename}: /__local_fixups__ mismatch'
> > > +            )
> > > +
> > > +    recreated.build_symbolic_props()
> > > +
> > > +    if set(orig_dtb.nodes.keys()) != set(recreated.nodes.keys()):
> > > +        missing = set(orig_dtb.nodes.keys()) - set(recreated.nodes.keys())
> > > +        extra = set(recreated.nodes.keys()) - set(orig_dtb.nodes.keys())
> > > +        raise RuntimeError(
> > > +            f'{orig_dtb.filename}: node mismatch (missing={missing}, '
> > > +            f'extra={extra})'
> > > +        )
> > > +
> > > +    for path in orig_dtb.nodes.keys():
> > > +        if orig_dtb.sym_props[path] != recreated.sym_props[path]:
> > > +            for p in (set(orig_dtb.sym_props[path]) |
> > > +                      set(recreated.sym_props[path])):
> > > +                if (orig_dtb.sym_props[path].get(p) !=
> > > +                        recreated.sym_props[path].get(p)):
> > > +                    raise RuntimeError(
> > > +                        f'{orig_dtb.filename}: property mismatch at {path}:{p}'
> > > +                    )
> > > +        # Common base nodes may gain a phandle if another board referenced them.
> > > +        if (orig_dtb.has_phandle_prop[path] and
> > > +                not recreated.has_phandle_prop[path]):
> > > +            raise RuntimeError(
> > > +                f'{orig_dtb.filename}: missing phandle at {path}'
> > > +            )
> > > +        if (orig_dtb.has_linux_phandle_prop[path] and
> > > +                not recreated.has_linux_phandle_prop[path]):
> > > +            raise RuntimeError(
> > > +                f'{orig_dtb.filename}: missing linux,phandle at {path}'
> > > +            )
> > > +
> > > +    if orig_dtb.symbols != recreated.symbols:
> > > +        raise RuntimeError(f'{orig_dtb.filename}: /__symbols__ mismatch')
> > > +    if orig_dtb.boot_cpuid_phys != recreated.boot_cpuid_phys:
> > > +        raise RuntimeError(
> > > +            f'{orig_dtb.filename}: boot_cpuid_phys mismatch '
> > > +            f'({orig_dtb.boot_cpuid_phys} != {recreated.boot_cpuid_phys})'
> > > +        )
> > > +    if orig_dtb.mem_rsv != recreated.mem_rsv:
> > > +        raise RuntimeError(
> > > +            f'{orig_dtb.filename}: /memreserve/ mismatch '
> > > +            f'({orig_dtb.mem_rsv} != {recreated.mem_rsv})'
> > > +        )
> >
> > Open coding the tree comparison when we already have
> > dtbs_equal_unordered is not ideal.
> 
> Removed verify_recreated_dtb() and switched the test suite to use
> dtbs_equal_unordered.
> 
> 
> > > diff --git a/tests/fdtfactor_board1.dts b/tests/fdtfactor_board1.dts
> > > new file mode 100644
> > > index 000000000000..cab6b3a4a3cd
> > > --- /dev/null
> > > +++ b/tests/fdtfactor_board1.dts
> > > @@ -0,0 +1,69 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/dts-v1/;
> >
> > fdt testcases generally try to be minimal.  So, I'd suggest using a
> > contrived tree along the lines of test_tree1.dts, rather than a
> > realistic board tree like this one.  It does need an example of all
> > the cases that should be fixed up, of course.
> 
> Done. Replaced the test .dts files with minimal contrived trees modeled on
> test_tree1.dts.
> 
> 
> > > @@ -239,6 +239,21 @@ run_fdtoverlay_test() {
> > >      base_run_test sh "$SRCDIR/fdtoverlay-runtest.sh" "$expect" "$@"
> > >  }
> > >
> > > +# Compare an original .dtb and a recreated .dtb by decompiling and sorting both
> > > +# with dtc -I dtb -O dts -qq -f -s (like dtdiff) and normalizing the numeric
> > > +# phandle properties assigned to target nodes. Because both blobs contain
> > > +# /__symbols__ and /__local_fixups__, dtc restores all node labels and converts
> > > +# all phandle references in properties back into symbolic <&label> references.
> > > +check_dtb_recreated () {
> > > +    orig="$1"
> > > +    recreated="$2"
> > > +    $DTC -I dtb -O dts -qq -f -s -o "$orig.raw.test.dts" "$orig" || return 1
> > > +    $DTC -I dtb -O dts -qq -f -s -o "$recreated.raw.test.dts" "$recreated" || return 1
> > > +    sed 's/phandle = <0x[0-9a-f]*>;/phandle = <0xffffffff>;/' "$orig.raw.test.dts" > "$orig.cmp.test.dts" || return 1
> > > +    sed 's/phandle = <0x[0-9a-f]*>;/phandle = <0xffffffff>;/' "$recreated.raw.test.dts" > "$recreated.cmp.test.dts" || return 1
> > > +    diff -u "$orig.cmp.test.dts" "$recreated.cmp.test.dts"
> >
> > I dislike tests comparing dts in general, since it's a highly
> > non-unique representation.  I much prefer to use the
> > dtbs_equal_ordered and dtbs_equal_unordered helpers.
> 
> Done. Removed check_dtb_recreated() and switched the tests to use
> dtbs_equal_unordered.

Great.

> > > +}
> > > +
> > >  BAD_FIXUP_TREES="bad_index \
> > >               empty \
> > >               empty_index \
> > > @@ -1089,6 +1104,50 @@ fdtoverlay_tests() {
> > >
> > >      shorten_echo "check phandle to noda d wasn't overwritten:        "
> > >      run_wrap_test test "$bd" = "$pd"
> >
> > I think it would be preferable to create a new group of tests for
> > fdtdactor, rather than lumping them in with fdtoverlay.
> 
> Done.
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]