Skip to content
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

Fix some pitfalls from the Raspberry Pi 4 specific sd image #90119

Merged
merged 3 commits into from Jun 20, 2020

Conversation

@samueldr
Copy link
Member

samueldr commented Jun 11, 2020

Motivation for this change

The SD image is not exactly trivial to use if you are not intimately knowledgeable of the way our default generic aarch64 image is built. That is, we have only shoe-horned the rpi4 kernel, and rpi boot chain on top of that default image.

This change aims to square the circle, and make it less trivial to get things wrong.

The main change is that we are naming the FAT32 partition something more obvious for this use case, NIXOS_BOOT, and we are also mounting it by default so new generations will boot as expected.

This is needed since I haven't managed to get u-boot booting NixOS on the pi4 yet, and the mainline kernel is (supposedly) still not there yet.

What does this change??

Well, mainly, it makes the difference in this image, compared to the default, much more obvious. Hopefully fixing the issue where users ended up confused with boot "reverting" to an old generation.

The main thing to look out for, is that we're going to have to ask users to list from /dev/disk/by-label to know the label of the filesystem, and then validate it's properly mounted.

I had a "first boot" boot once, though could not reproduce with a fresh image, where the neededForBoot /boot partition didn't end up mounting, and the boot didn't fail. It was mounted after a reboot. I don't think that if it is an issue, that is an issue specific to the changes here, but I don't know what it could be.

Things done

This was tested using cross-compilation, and using #89717 commits.

Verified to boot on a raspberry pi 4.

@samueldr samueldr requested review from tkerber and lopsided98 Jun 11, 2020
@samueldr samueldr changed the title Fix some pitfalls from the rpi4 specific sd image Fix some pitfalls from the Raspberry Pi 4 specific sd image Jun 11, 2020
@angerman
Copy link
Contributor

angerman commented Jun 11, 2020

Should probably link #78090 for good measure.

@samueldr
Copy link
Member Author

samueldr commented Jun 11, 2020

Oops, completely forgot that that PR existed even.

Though yeah, this is a more "proper" fix, in my opinion, as we are properly making the distinction between the "FIRMWARE" partition for mainline-based devices, and this "this is actually the boot partition" setup.

Copy link
Contributor

angerman left a comment

lgtm

# This is a hack to avoid replicating config.txt from boot.loader.raspberryPi
populateFirmwareCommands =
"${config.system.build.installBootLoader} ${config.system.build.toplevel} -d ./firmware";
# As the boot process is done entirely in the firmware partition.
populateRootCommands = "";
};

fileSystems."/boot/firmware" = {
# This effectively "renames" the loaOf entry set in sd-image.nix
mountPoint = "/boot";

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jun 11, 2020

Member

Does all of the rpi firmware need to be in /boot? I was hoping it was just that one .dtb fille.

This comment has been minimized.

Copy link
@samueldr

samueldr Jun 11, 2020

Author Member

This is not the "FIRMWARE", but the raspberry-pi-boot-chain specific config.txt, kernel, initrd, and old boot entries. And yes it does. While the script has an option for target, the module does not implement a way to change it.

Anyway, this is the expectation for a mutable raspberry pi image.

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer Jun 11, 2020

Member

I'm not sure if I understand. /boot/firmware is set in sd-image.nix, but this overwrites it to mount to /boot? Don't we want to avoid putting too much on the vfat filesystem?

This comment has been minimized.

Copy link
@samueldr

samueldr Jun 11, 2020

Author Member

sd-image.nix is built with the assumption that it builds a generic non-raspberry-pi image, which coincidentally ends up having the u-boot for the raspberry pi pre-installed. The "FIRMWARE" partition is u-boot for raspberry pi 3. Maybe it needs to be renamed "UBOOT4RPI" :).

Now, the confusing part, I think.

The sd-image-raspberrypi4 re-uses the sd-image.nix stuff. The FAT32 partition here is not for u-boot, but rather it is the usual raspberry pi boot chain, with the kernel and initrd on the partition.

What this thing does, is that it it re-uses the definition for the mountpoint /boot/firmware, as configured in sd-image.nix, and properly mounts the FAT32 partition on /boot as expected with the raspberry pi boot chain and our raspbperryPi "loader" options.

This is basically cheating and re-using something that wasn't exactly built for use that way.

We want to avoid putting too much in the FAT partition, but it is required with the raspberry pi toolchain.

samueldr added 3 commits Jun 11, 2020
The way this ends up being called with the raspberry pi 4 image builder
ends up not using the `-e` from the shebang.

In turn, the builds fails during cross-compilation. The wrong coreutils
ends up being used, but this is not made apparent.

The issue I faced is already fixed on master, but this ensures no one
ends up with a failed build "succeeding".
This will be helpful in the now too-long-lived image for the Raspberry
Pi 4. We'll be able to properly configure the partition to be useful.
This should have been done initially, as otherwise it gets awfully
awkward to boot into new generations by default.

This system-specific image wasn't expected to be long-lived, thus why it
didn't end up being polished much.

Reality shows us we may be stuck with it for a bit longer, so let's make
it easier to use for new users.
@samueldr samueldr force-pushed the samueldr:feature/rpi4-fixups branch from a8dd439 to 476c8e0 Jun 11, 2020
@samueldr samueldr merged commit f203b8b into NixOS:master Jun 20, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="476c8e0"; rev="476c8e0754968cc0d56479894ea5c3037dd70892"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@samueldr samueldr deleted the samueldr:feature/rpi4-fixups branch Jun 20, 2020
@samueldr samueldr mentioned this pull request Jun 21, 2020
1 of 10 tasks complete
@Profpatsch
Copy link
Member

Profpatsch commented Jun 21, 2020

Hmmmm, why is this not in nixos-hardware?

@samueldr
Copy link
Member Author

samueldr commented Jun 21, 2020

Because this is meant to produce an official image for a popular platform that is hard to get a setup.

Though it seems to me the question is loaded. Are you trying to mount an argument for NixOS/rfcs#70 out of this? Because I would agree this system-specific image shouldn't be part of Nixpkgs, but pragmatically speaking there was no other way to get the organization's Hydra building the images at the moment it was conceived. In my opinion this is a shining example of churn that I don't want to see inside of Nixpkgs.

Note that on the original PR adding this system I voiced my concerns.

While this is not something I want to see as part of Nixpkgs, and this is intended to be removed as soon as the generic image can boot on this dreadfully annoying piece of non-standard hardware, it is the most common gateway into getting a foothold in NixOS on ARM. It needed some maintenance, so I did it, instead of having to support user after user falling into the same pitfalls.

@Profpatsch
Copy link
Member

Profpatsch commented Jun 22, 2020

@samueldr
Copy link
Member Author

samueldr commented Jun 22, 2020

PLEASE leave the NixOS/rfcs#70 discussion into the RFC discussion and not shard it across my whole comments history.

@samueldr samueldr mentioned this pull request Jun 22, 2020
0 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
NixOS on ARM
Awaiting triage
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.