-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
nixos/grub: add support for loading deviceTree #446563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging-nixos
Are you sure you want to change the base?
Conversation
a00acb8 to
d25b074
Compare
|
jfly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#439700 and #446563 are independant implemention of device-tree support for grub. Just make sure we use the same option boot.loader.loadDeviceTree. #403746 will be closed and reimplemented after #446563. kernel maintainers do not agree on keeping extra *.dtsi files. Debian also does not keep *.dtsi files in their kbuild package. So i will seek other ways (e.g. using pkgs.srcOnly). |
|
It seems that adding a |
| @@ -465,6 +467,13 @@ sub addEntry { | |||
|
|
|||
| my $kernel = copyToKernelsDir(Cwd::abs_path("$path/kernel")); | |||
| my $initrd = copyToKernelsDir(Cwd::abs_path("$path/initrd")); | |||
| my $json_text = read_file("$path/boot.json"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are already consuming the bootspec file here, it would also make sense to pull the kernel and initrd from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You remind me, i think it is more appropriate we use bootspec "org.nixos.boot" instead of "org.nixos.device-tree" here, as it allows us to include more information (e.g. kernel initrd) in it.
However, this won’t be an easy migration. We still need backward compatibility for generations that currently place the kernel and initrd under $path, and we should warn users when we will drop the backward compatiblity. The same difficulty happen if we want migrate bootspec "org.nixos.systemd-boot".devicetree to "org.nixos.boot".devicetree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There already exists "org.nixos.bootspec.v1", we can put devicetree information under it.
nixpkgs/nixos/modules/system/boot/loader/generic-extlinux-compatible/default.nix Lines 107 to 111 in 0b4defa
by replacing (dtCfg.name != null) with config.boot.loader.loadDeviceTree we can make it somehow consistent. |
The insmod efi_gop and insmod efi_uga directives load two modules for EFI-based video support. On most systems the efi_gop module is enough. The efi_uga module is only useful and available for legacy system on i386 and x86_64. Therefore, we load the efi_uga module conditionally.
| @@ -35,6 +35,9 @@ let | |||
| } | |||
| // lib.optionalAttrs hasAtLeastOneInitrdSecret { | |||
| initrdSecrets = "${config.system.build.initialRamdiskSecretAppender}/bin/append-initrd-secrets"; | |||
| } | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (devicetree) is not a field in the V1 bootspec specification (https://github.com/NixOS/rfcs/blob/master/rfcs/0125-bootspec.md#bootspec-format-v1), so it should not be under the versioned org.nixos.bootspec.v1 key.
This should probably be an extension instead, or proposed as part of a V2 format of bootspec (there is an existing RFC for this at NixOS/rfcs#165).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the key to be under the field org.nixos.bootspec.v2, should i wait until the RFC NixOS/rfcs#165 been merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you'll want to wait for RFC 165 to merge if that's the case. However, since it's still a draft, you should not rely on it in this PR, as it may still change before it's merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeking other bootspec field other than "org.nixos.bootspec.v2" will make it hard for users to migrate their legacy configuration when we switch to bootspec v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean by "migrate their legacy configuration" -- old generations will have an extension for the devicetree (since it's not part of v1, and v2 has not been merged yet), and everything from Nixpkgs will understand it because it will be the same Nixpkgs revision that generated the boot.json with the extension for devicetree.
New generations (once v2 is merged) will have the "org.nixos.bootspec.v2".devicetree key accessible, and Nixpkgs will understand that at that point.
Additionally, there exists a synthesize tool whose job is to go from "no bootspec" or "bootspec vX" to "bootspec v(X+Y)" for the purposes of these bootloader generators, see:
| f"{BOOTSPEC_TOOLS}/bin/synthesize", |
It will be updated for bootspec v2 once bootspec v2 merges.
All that, in addition to the fact that this is exactly what extensions were intended for, makes me feel very strongly that this PR should either: 1) not use the bootspec document and pass the device tree in to the bootloader generators some other way; or 2) create an extension for it that does not abuse the versioned org.nixos.bootspec.vX namespace and access it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use the generation of grub.cfg as a example.
grub.cfg contains information of all the generation's kernel/initrd path (and devcietree path if this pr get merged).
It is generated by the current (most likely the latest) generation's nixpkgs install-grub.pl script. This script translate per generation's bootspec "org.nixos.somefield".devicetree to corresbonding devicetree /nix/store/xxxxx.dtb command. It is supposed to provide backward compatibility with old generation's boot.json when we switch to "org.nixos.bootsepc.v2".devicetree. It will be painfull we notify users when we will drop this backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect these generators to run on mismatched Nixpkgs versions (i.e. the system was created with a Nixpkgs with v2 of bootspec, but the generator only understands v1+this extension)?
I expect the old system was created with a Nixpkgs with v1+this extension, but the generator only understands v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still argue that the migration from org.nixos.systemd-boot.devicetree to org.nixos.bootspec.v2.devicetree migth (or not) be a problem
nixos/modules/system/boot/loader/systemd-boot/systemd-boot-builder.py
bootspec_json.get('org.nixos.systemd-boot', {}) -> bootspec_json.get('org.nixos.bootspec.v2', {})
I think this simple migration will drop backward compatibility with old generation that use org.nixos.systemd-boot.devicetree. (So we have to use fallback logical for a while)
What do you think about the right way to push this pr forward. also ping @jmbaur for suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For systemd-boot, I think it's probably not a problem because each boot entry can be generated individually.
I forgot about the grub "quirk" where it has to generate all the boot options every time (ugh).
That said, it sounds like it would probably be best to add an org.nixos.grub.deviceTree extension to the bootspec. Once v2 is adopted, then grub can use org.nixos.bootspec.v2.devicetree (or whatever it ends up being) if it exists, or org.nixos.grub.deviceTree if that doesn't exist. We'd keep generating org.nixos.grub.deviceTree, at least for a release or two.
This approach should keep everything working even once v2 is adopted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For systemd-boot, I think it's probably not a problem because each boot entry can be generated individually.
Got it. I will use org.nixos.grub.deviceTree as a temporaroy solution.
Btw, when we will reach an agreement on bootspec v2. Does community have any plan to disscuss NixOS/rfcs#165 in the meeting.
Rename `boot.loader.systemd-boot.installDeviceTree` to `boot.loader.loadDeviceTree`. This option provides generic support for loading device trees.
|
grub do have support for loading deviceTree from filesystem and pass it to linux.
See https://www.gnu.org/software/grub/manual/grub/html_node/devicetree.html.
Here we implement deviceTree support for grub module.
Things done
efi_uga module not availableon aarch64-linux platform when using grub. (efi_uga is only available on x86)Future plan
cc @jfly @jmbaur for your comment.
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.