Skip to content

sd-boot: Support installing new devicetree#19417

Merged
yuwata merged 1 commit intosystemd:mainfrom
esmil:main
Sep 10, 2021
Merged

sd-boot: Support installing new devicetree#19417
yuwata merged 1 commit intosystemd:mainfrom
esmil:main

Conversation

@esmil
Copy link
Copy Markdown
Contributor

@esmil esmil commented Apr 25, 2021

On systems using device tree it often happens that the device tree evolves with the kernel and eventually becomes out of sync with the device tree used by the bootloader (eg. u-boot). One such example is the HiFive Unleashed board where current kernels won't boot using the device tree in the latest u-boot. In such cases you want to be able to load a new device tree with the kernel you're booting and the Boot Loader Specification already has a stanza in the .conf files to specify it. Until now sd-boot just didn't do anything with it.

The UEFI spec says that the bootloader should have a private copy of the device tree so EFI apps are free to update the device tree in the EFI configuration table and specifies how it should be loaded into "EfiACPIReclaimMemory", so let's implement that.

We also need to be able to check that we're getting a valid device tree and for RISC-V we also need to modify it. I tried just using libfdt directly, but since it needs to run in EFI context I can't see any way out of modifying it slightly. Having accepted that I went all the way and cut out just the functionality we need into just a header file and an implementation. Carrying our own copy of libfdt is annoying, but at least we're in good company: linux, u-boot, grub all have their own copy too :/

Instead use the EFI_DT_FIXUP_PROTOCOL to validate and fixup the loaded device tree. To my knowledge this is so far only implemented by u-boot, but this is also the most common EFI implementation on ARM or RISC-V where device trees are needed.

This commit make sd-boot work properly on the BeagleV Starlight beta board with a recent u-boot. I'll also see if I can dig out an ARM board with a recent u-boot and test it there, but I thought I'd begin the review process early.

@bluca
Copy link
Copy Markdown
Member

bluca commented Apr 27, 2021

We also need to be able to check that we're getting a valid device tree and for RISC-V we also need to modify it. I tried just using libfdt directly, but since it needs to run in EFI context I can't see any way out of modifying it slightly. Having accepted that I went all the way and cut out just the functionality we need into just a header file and an implementation. Carrying our own copy of libfdt is annoying, but at least we're in good company: linux, u-boot, grub all have their own copy too :/

Have you tried making the changes you need in libfdt upstream instead of vendoring it?

@poettering
Copy link
Copy Markdown
Member

I'd feel more comfortable with this vendoring if there at least was at least some curl glue in meson to update the lib somehow

but quite frankly, can't the people behind fdt somehow provide an embeddedable version of their stuff. it can't be right that really everyone copies this and then it rots along?

@poettering
Copy link
Copy Markdown
Member

So i noticed that upstream libfdt already supports meson. Can't we make this a proper meson submodule we cna then import here?

i.e. work with the fdt people to make their lib embeddable in some way, and then pull it into our tree via git submodules + meson submodules.

@poettering
Copy link
Copy Markdown
Member

how many changes to fdt.c did you actually make? just add some glue to the front? if so, can't you rework this, so that we can take the naked file from upstream, but not compile it directly, but enclose it in a source file of our own, which defines the glue , and then #includes the unmodified file imported from libfdt? That way there's at least a theoretic way to pull in the stuff again.

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Apr 27, 2021
@esmil
Copy link
Copy Markdown
Contributor Author

esmil commented Apr 27, 2021

I'd feel more comfortable with this vendoring if there at least was at least some curl glue in meson to update the lib somehow

but quite frankly, can't the people behind fdt somehow provide an embeddedable version of their stuff. it can't be right that really everyone copies this and then it rots along?

I agree it's a sad state of things, but in our case the problem is that gnu-efi doesn't even provide a standard C environment with a reasonable subset of string.h that most "embedded" environments have.

@esmil
Copy link
Copy Markdown
Contributor Author

esmil commented Apr 27, 2021

how many changes to fdt.c did you actually make?

A lot. I went for the minimal amount of code to add. The real libfdt is 10 files and 4 headers, that I didn't just want to dump here.

just add some glue to the front? if so, can't you rework this, so that we can take the naked file from upstream, but not compile it directly, but enclose it in a source file of our own, which defines the glue , and then #includes the unmodified file imported from libfdt? That way there's at least a theoretic way to pull in the stuff again.

I can look into doing that, but it seems the compile flags given when compiling systemd-boot doesn't allow the compiler to throw away a lot of unused functions, so it's going to add some weight to systemd-boot that I also wanted to avoid.

@poettering
Copy link
Copy Markdown
Member

I can look into doing that, but it seems the compile flags given when compiling systemd-boot doesn't allow the compiler to throw away a lot of unused functions, so it's going to add some weight to systemd-boot that I also wanted to avoid.

hmm, we have those for the rest of the codebase. If we don't have them for sd-boot, that's probably something to fix

@mxre
Copy link
Copy Markdown
Contributor

mxre commented Aug 5, 2021

Shouldn't you use (or try to use) the DeviceTree fixup protocol ( https://github.com/U-Boot-EFI/EFI_DT_FIXUP_PROTOCOL )?
If you load a dtb directly UBoot can't add runtime information. (cf. https://www.kernel.org/doc/html/latest/admin-guide/efi-stub.html#the-dtb-option )
I don't have a HiFive Unleashed board, but doesn't UBoot manipulate the dtb for you if you load it before entering EFI mode? It does so for all Aarch64 boards that I've seen....

@esmil
Copy link
Copy Markdown
Contributor Author

esmil commented Aug 5, 2021

Shouldn't you use (or try to use) the DeviceTree fixup protocol ( https://github.com/U-Boot-EFI/EFI_DT_FIXUP_PROTOCOL )?

Thanks. That would definitely fix the need to fixup the DTS in sd-boot. Is this a common standard though or just u-boot specific?

I don't have a HiFive Unleashed board, but doesn't UBoot manipulate the dtb for you if you load it before entering EFI mode? It does so for all Aarch64 boards that I've seen....

It does. But only in the device tree used by u-boot and set as the default device tree. The whole point of this code is that we should be able to replace that device tree with a newer one specified by the devictree stanza and loaded from the EFI partition. Hence the code either needs to copy the /chosen/boot-hartid from the previous u-boot device tree or maybe use the protocol above.

@mxre
Copy link
Copy Markdown
Contributor

mxre commented Aug 5, 2021

Currently EFI_DT_FIXUP_PROTOCOL is UBoot only (but Its an open specification). I also had the a problem with the way UBoot loads devicetrees (mainly because they are not SecureBoot signed) and came to a different solution: I wrote a bootstub[[1]] (like the systemd-boot stub).
[1]: https://github.com/mxre/zloader/

But only in the device tree used by u-boot and set as the default device tree.
And why not load a device tree from the ESP, and pass it onto the efiboot command (which loads sd-boot)? You can load any dtb file you want...

@esmil esmil force-pushed the main branch 2 times, most recently from 3c717de to e2566d7 Compare August 27, 2021 20:28
@bluca bluca added needs-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Aug 27, 2021
@esmil esmil requested a review from poettering August 28, 2021 10:58
@esmil esmil force-pushed the main branch 3 times, most recently from 36f017b to 17582af Compare August 31, 2021 15:55
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed needs-review labels Sep 1, 2021
@poettering
Copy link
Copy Markdown
Member

Hmm, so how does this relate to the fdt stuff in #20601? Isn't there some code to share here?

@esmil
Copy link
Copy Markdown
Contributor Author

esmil commented Sep 1, 2021

Hmm, so how does this relate to the fdt stuff in #20601? Isn't there some code to share here?

As described above, this version no longer does any fdt manipulation, but instead uses the EFI_DT_FIXUP_PROTOCOL and hence doesn't need libfdt. However it looks like that pull request also installs a new device tree which could definitely use (parts of) the devicetree_install and cleanup functions implemented here so the original device tree will be reinstated on failure and the EfiACPIReclaimMemory freed.

@esmil esmil requested a review from poettering September 1, 2021 22:36
@esmil esmil force-pushed the main branch 2 times, most recently from c3c2bbc to 9b3be2a Compare September 1, 2021 23:16
@yuwata yuwata added needs-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Sep 4, 2021
@esmil
Copy link
Copy Markdown
Contributor Author

esmil commented Sep 8, 2021

Any chance we could merge this before #20601? It seems like that needs some more work and I think it would be helpful it if could easily re-use parts of this code to install a new device tree.

Copy link
Copy Markdown
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks pretty good. of course i have no idea about devicetree, so can just assume this all works, only commenting on the code here.

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed needs-review labels Sep 8, 2021
@bluca bluca added needs-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Sep 8, 2021
@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed needs-review labels Sep 8, 2021
@poettering
Copy link
Copy Markdown
Member

lgtm

The Bootloader Specification says "devicetree refers to the binary
device tree to use when executing the kernel..", but systemd-boot
didn't actually do anything when encountering this stanza until now.

Add support for loading, applying fixups if relevant, and installing the
new device tree before executing the kernel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed new-feature sd-boot/sd-stub/bootctl

Development

Successfully merging this pull request may close these issues.

7 participants