Re: [PATCH v3 0/4] state: generic devicetree-overlay based state node injection

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 8/25/26 5:05 AM, chalianis1@gmail.com wrote:
> From: Chali Anis <chalianis1@gmail.com>
> 
> Boards that want a "barebox,state" node today have exactly one option:
> carry it in their own, statically compiled-in devicetree source. That's
> fine as long as barebox is built per-board with a maintained dts, but it
> is a harder fit for targets that don't have one to begin with: the EFI
> payload, deliberately meant to run unmodified across arbitrary
> x86/arm64 EFI platforms barebox itself knows nothing about at build
> time, and the generic BOARD_ARM_GENERIC_DT ("barebox-dt-2nd") image,
> which picks up whatever devicetree a first-stage bootloader or QEMU
> hands it in r2 at runtime the same way a Kernel would, rather than
> being built against a particular board's dts. Either way there is no
> single "board dts" being compiled for a state node to live in.

We indeed have nothing for the barebox-dt-2nd.img case, but this image
is meant to be used with a *barebox* device tree, not some random DT
that may use bindings barebox isn't compatible with.

This is intentionally narrower than what the kernel supports: The kernel
will keep supporting old bindings, but barebox will only support the
bindings it ships with.

With that background, barebox-dt-2nd.img should never be called with
some random DT that's not matched to what barebox expects and thus we
can expect that if someone wants a barebox state node they would add it.

As for QEMU, barebox already ships state overlays for it, but these are
built-in and applied early-on.

> 
> CONFIG_EXTERNAL_DTS_FRAGMENTS already covers a related need rather well:
> an external build system can append dts fragment files to a board's dts
> source at build time, scoped to specific boards via a per-dts
> preprocessor macro. That remains the more direct choice whenever a
> board's own dts is actually part of the build, and this series doesn't
> propose changing that. It runs into the same limit as static dts
> inclusion for the EFI payload

As mentioned on IRC, this is a limitation that can be fixed and not some
deliberate choice.

> and barebox-dt-2nd cases specifically,

As elaborated above, I have my reservations about starting barebox with
arbitrary DTs.

> though, since it operates at dts-source/build time on a particular
> "main dts" - which neither target, by design, has one of.

As the EFI payload needs the DT for nothing apart of state, we could
also ship an empty DT and make that extensible via fragments.

FYI, there was discussion when fragments were first added if they should
be overlays instead:

https://lore.barebox.org/barebox/CAMHeXxPZ9on3rZu92H1EeNQj79rUFBRbs5Qre=AS3U7_y=UueQ@mail.gmail.com/

> This series instead proposes a devicetree *overlay* (.dtso, applied at
> runtime via CONFIG_STATE_OVERLAY) for that gap. Applied to whichever
> devicetree barebox already ends up live with by boot time - statically
> compiled in, EFI-firmware-derived, passed in from a first-stage
> bootloader, or the EFI payload's own minimal stub root - it only ever
> adds one small node, so it doesn't need a "main dts" to attach to at
> build time, and it doesn't need to know a board's memory map or other
> devicetree content beyond one stable label (or, for EFI, just a
> partition UUID, patch 1) to hook its backend into.

If we were to allow supplying external overlays at build time that are
applied at runtime, why make it specific to only state?

Also to be a truly generic solution, we need some accounting for
multi-image (what if your build produces both a rpi3 and a rpi4 image
and you want different overlays for each?).

> In turn, that also
> means it never competes with an existing devicetree for ownership,
> which the one realistic alternative we considered - loading a full,
> standalone state.dtb at runtime - does run into: barebox_register_of()
> only accepts a new root if none is registered yet or the incoming tree
> is empty,

It only accepting empty state.dtbs is a bug! It's fixed on master now
though (and in the latest release).

> so a real state.dtb collides with whatever root the EFI
> payload already registered at boot and is rejected with -EBUSY, and a
> rejected tree's /aliases entries never reach the global alias cache
> of_alias_get() relies on either.

This argumentation follows from a bug, so it doesn't say anything to the
merit of this new feature.

> Happy to discuss trade-offs here, in particular whether it's worth
> teaching CONFIG_EXTERNAL_DTS_FRAGMENTS (or a variant of it) to handle
> the no-base-dts case instead of adding a separate mechanism - this
> series is meant as a concrete starting point for that conversation, not
> a claim that overlays are the only reasonable answer.

I appreciate you putting in the effort. As mentioned on IRC, I am in
favor of extending fragments as it meshes with what we already have, but
I agree overlays can cover use cases that fragments don't (while
overlays as implemented here can't cover all users that fragments provide).

> Patch 1 makes of_state_fixup() able to resolve a partuuid-referenced,
> non-hardware-backed backend node, and exports it so it can be called
> directly. Patch 2 adds CONFIG_STATE_OVERLAY itself, compiling an
> external .dtso into the barebox binary and applying it to the live
> devicetree at postcore_initcall time - guarding against there being no
> live devicetree yet, and refreshing the alias cache once applied. Patch
> 3 builds on both to publish the resolved state description as a UEFI
> variable once such a node exists. Patch 4 makes both of those actually
> reachable on x86: no code path there ever registered a live devicetree
> root pre-boot to begin with, since that registration only existed for
> the EFI_STUB entry point barebox uses on other architectures, not the
> EFI_PAYLOAD one x86 uses.


No need to recount the patch commit messages here. If at all, just
include a general description in the cover letter. This mail is already
very verbose, which makes following it a bit hard for me.
> 
> Changes since v1:
> - patch 1: fixed the compatible string on the synthesized fixed-partitions
>   node ("fixed-partitions", not the barebox-internal
>   "barebox,fixed-partitions" alias, which external consumers don't
>   recognize), fixed a phandle collision where the synthesized node kept
>   the phandle it had in barebox's own live devicetree instead of one
>   scoped to the target tree, and resolved non-partuuid backends via the
>   reproducible name cached at probe time again instead of recomputing it
>   against whatever tree is being fixed up.
> - patch 2: state_overlay_apply() now guards against there being no live
>   devicetree yet and skips cleanly instead of calling into the overlay
>   code with a NULL root, and calls of_alias_scan() afterward so the
>   overlay's /aliases entry becomes visible the same way a live overlay
>   applied via the interactive of_overlay command already does. Also
>   selects CONFIG_OFDEVICE, needed for a live devicetree root to exist
>   at all on some targets.
> - patch 3: publish "BareboxState" under efi_barebox_vendor_guid instead
>   of efi_systemd_vendor_guid - it's a barebox-defined variable, not part
>   of the systemd-boot loader protocol. state_to_efivars_export() and
>   efi_late_init() are both late_efi_initcall, and within one initcall
>   level execution follows definition order in the object file, so
>   state_to_efivars_export() is now defined after efi_late_init():
>   on boards with no state node in their own static devicetree,
>   efi_late_init() is what loads and registers the standalone state.dtb,
>   and only once that has had a chance to run does state_by_alias() (used
>   here instead of open-coding the equivalent of_find_node_by_alias() +
>   state_by_node()) have anything to find.
> - patch 4 is new: without it, CONFIG_STATE_OVERLAY silently never had a
>   devicetree to apply to on x86, and this series' EFI-payload rationale
>   didn't hold up for that architecture in practice.
> - cover letter: called out BOARD_ARM_GENERIC_DT ("barebox-dt-2nd") as a
>   second target that benefits from this alongside the EFI payload, since
>   it's in the same "no main dts at build time" situation.

I haven't checked out the earlier versions, so I will just gloss over this.

> Tested on a Raspberry Pi CM4 natively, and as the EFI payload on a
> Jetson Orin NX and under QEMU (x86, with a partuuid-referenced backend).

Thanks. This is useful info.

> 
> Chali Anis (4):
>   state: make of_state_fixup() usable outside common/state/
>   state: add CONFIG_STATE_OVERLAY to inject a state node via devicetree
>     overlay
>   efi: payload: export resolved state as a BareboxState UEFI variable
>   efi: payload: make CONFIG_STATE_OVERLAY (and BareboxState export) work
>     on x86
> 
>  .../bindings/barebox/barebox,state.rst        |  9 +++
>  Documentation/user/state.rst                  | 32 +++++++++
>  common/Kconfig                                | 41 +++++++++++
>  common/state/Makefile                         | 20 ++++++
>  common/state/state.c                          | 72 +++++++++++++++----
>  common/state/state_overlay.c                  | 29 ++++++++
>  efi/payload/Makefile                          |  1 +
>  efi/payload/init.c                            | 69 +++++++++++++++++-
>  include/state.h                               |  5 ++
>  9 files changed, 263 insertions(+), 15 deletions(-)
>  create mode 100644 common/state/state_overlay.c
> 
> 


Cheers,
Ahmad



-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |