Re: [PATCH 4/4] fdtfactor: Add compiled FIT image factoring support

[Simon Glass]

Hi Doug,

On 2026-10-15T18:21:33, Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
> fdtfactor: Add compiled FIT image factoring support
>
> Bootloaders such as depthcharge on Chromebooks and U-Boot use Flattened
> Image Tree (FIT) images to bundle a kernel image with multiple board FDT
> blobs (`fdt-1`, `fdt-2`, ...) and per-board configuration nodes.
>
> Add `-f`/`--fit`, `-F`/`--fit-out`, and `--single-base-dtb` options to
> `fdtfactor` to factor a compiled FIT image directly:
>
> - Parse non-overlay `type = "flat_dt"` nodes under `/images`,
>   decompressing their `data` blobs according to their `compression`
>   property (`none`, `lz4`, `lzma`, `gzip`).
> - Partition the `flat_dt` blobs into compatible factor groups (by
>   compression, architecture, FDT header fields, and node-path overlap)
>   so a single FIT image containing multiple board/SoC families produces
>   a separate `fdt-base-1`, `fdt-base-2`, etc. for each family.
> - Support `--single-base-dtb` to force all `flat_dt` blobs into a single
>   base DTB regardless of node-path overlap, failing only if their FDT
>   headers, architecture, or compression differ.
> - Insert each `fdt-base-N` node into `/images`, replace each factored
> [...]
>
>  README.md                  |  15 +
>  fdtfactor                  | 726 +++++++++++++++++++++++++++++++++++++++++++--
>  tests/Makefile.tests       |   2 +-
>  tests/fdtfactor_other1.dts |  69 +++++
>  tests/fdtfactor_other2.dts |  69 +++++
>  tests/fdtfactor_test.its   | 109 +++++++
>  tests/run_tests.sh         |  69 ++++-
>  7 files changed, 1033 insertions(+), 26 deletions(-)

David is leaving this patch until patch 2 has been reworked, so these
are just things to carry into the next version.

> diff --git a/fdtfactor b/fdtfactor
> @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> +    dtbo_comp_sizes = []
> +    for entry, dtbo_blob in zip(entries, dtbo_blobs):
> +        img_node = entry['node']
> +        dtbo_comp_bytes = compress_blob(dtbo_blob, comp)

One thing I'd like to check, since the README says this targets
U-Boot. In U-Boot, boot_get_fdt_fit() loads each overlay with
FIT_LOAD_IGNORED, and decomp_image() skips decompression in that case.
So a compressed overlay goes to fdt_open_into() as-is and fails, which
means the lz4 FIT that the test produces would not boot with U-Boot. I
suspect U-Boot should be fixed. Until then it would be worth having an
option to leave the overlays uncompressed (they are small anyway), or
at least a note in the README.

> diff --git a/fdtfactor b/fdtfactor
> @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> +            for sub_name in list(conf_node.children):
> +                if sub_name.startswith(('signature', 'hash')):
> +                    print(
> +                        f'Warning: Stripping invalidated {sub_name} subnode '

This only strips signatures from the configurations that are changed.
But the whole FIT is rewritten with FdtSw, which lays out the strings
block in a different order from dtc. A configuration signature also
covers the strings block (via 'hashed-strings'), so I believe
signatures on untouched configurations break too. The user has to
re-sign with mkimage after factoring anyway, so it might be simpler to
fail when any signature node is present and say so. What do you think?

> diff --git a/fdtfactor b/fdtfactor
> @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> +def _derive_base_dtb_description(entries, base_dtb_node_name, comp):
> +    """Derive a `description` string for a factored base FDT image node.

This is a lot of heuristics for a human-readable description, and the
results are not obvious (e.g. ['sc7180-trogdor', 'sc7280-herobrine']
gives 'sc-base.dtb'). It would be worth using something simple and
predictable instead, such as 'fdt-base-1.dtb' or a description listing
the images it serves. That would also simplify the tests.

> diff --git a/fdtfactor b/fdtfactor
> @@ -1211,6 +1261,588 @@ def verify_recreated_dtb(orig_dtb, base_dtb_blob, dtbo_blob,
> +        placed = False
> +        for grp in groups:
> +            if grp['key'] != header_key:
> +                continue
> +            ref_nodes = grp['ref_nodes']

'ref_nodes' is only the node set of the first entry in each group, so
the grouping depends on the order of the nodes in /images. For
example, if the first board in a family has many extra nodes, its
siblings can end up in a separate group. Comparing against the group's
common node set (which is what ends up in the base) might be more
robust. What do you think?

For the argparse description, you really have two completely different
cases...I suggest the FIT part of it should be worded differently so
it is clear that the result is an update / new FIT.

mkimage uses -F to indicate an inplace FIT - with -f used for the
source .its file. It might be better for your tool to use -F instead
of -f, and then -O (confusing for dtc?) or --outfile to specify
writing a separate file.

Regards,
Simon