qemu-vm: remove bootDisk, refactor using make-disk-image with better EFI support by RaitoBezarius · Pull Request #203641 · NixOS/nixpkgs

RaitoBezarius · 2022-11-29T18:30:37Z

Description of changes

This PR does multiple things :

make-disk-image refactoring in QEMU test infrastructure

As per #23052 — QEMU test infrastructure do not use ad-hoc code to build the boot disk image but reuse make-disk-image which was improved to support use-cases.

Doing this consolidates make-disk-image as one place to do these changes, it seems like the boot disk production code was doing old stuff with GRUB 0.97, I'm uncertain whether that should be cleaned up.

UEFI firmware infrastructure

As part of #187887 ; this uses consolidated UEFI firmware and variables and expose their variables to end users.

It also adds SecureBoot and System Management Mode enforcement support in the test infrastructure and the make-disk-image function.

EFIVARS can be modified as part of the disk image build process, enabling people to build a disk image with specific authenticated UEFI variables, e.g. SecureBoot keys.

Documentation

This PR adds documentation on what are the breaking changes in regard to the disk layout (storeImage is used only if needed at all.)

TODO/notes:

RaitoBezarius · 2022-11-29T18:31:10Z

Opened due to GitHub merging the old PR #191665

RaitoBezarius · 2022-12-02T13:34:16Z

With this, https://hydra.nixos.org/eval/1786610 ; this PR do not introduce any failing test beyond what there was on the base nixpkgs point of this PR.

Will fix the merge conflicts now.

nikstur

This is a very good change and I'm happy to see it. Most of my comments are just minor nitpicks.

nixos/modules/virtualisation/qemu-vm.nix

nixos/doc/manual/release-notes/rl-2211.section.md

nixos/doc/manual/release-notes/rl-2305.section.md

nixos/lib/make-disk-image.nix

samueldr · 2022-12-05T01:09:43Z

One overall note, still reading this whopper of a PR: I think the lack of separation of concerns by shoving filesystem creation duties with disk image creation duties in the same expression/derivation is likely to cause issues overall. And we're seeing even less separation of concern by addition of irrelevant machine state (EFI vars) within this, I don't really like this.

RaitoBezarius · 2022-12-05T01:51:59Z

One overall note, still reading this whopper of a PR: I think the lack of separation of concerns by shoving filesystem creation duties with disk image creation duties in the same expression/derivation is likely to cause issues overall. And we're seeing even less separation of concern by addition of irrelevant machine state (EFI vars) within this, I don't really like this.

I agree with you, but this PR has only a small ambition (which took already a lot of time to do "well"), that is, to remove the ad-hoc disk image creation that existed before and put the knobs into that super function (which is bad, I know.)

Once it lands, we can:

split disk image creation and filesystem creation (with machine state), to expose a lower level function
split filesystem creation and generic machine state recording during VM operations

IMHO, it's easier to deal with it in this way also because I have a use case (SecureBoot) which can nourish the adequate refactors, some of them were already mentioned in the N previous PRs (that were closed due to my mistakes), see also #187855 which is an interesting symptom exposing ad-hoc knobs to test images, but no generic tooling to do this across the ecosystem.

samueldr · 2022-12-05T04:53:02Z

Nevertheless, do you think it's futile spending time on making these images more deterministic?

Futile no. I think it's good to want to improve what is there.

I think my main thought is that this is the wrong model.

As a cleanup effort, a short-to-mid term effort, great! But building too large on top of this foundation is (in my opinion) not the way forward.

roberth · 2022-12-08T22:40:13Z

the lack of separation of concerns by shoving filesystem creation duties with disk image creation duties in the same expression/derivation is likely to cause issues overall.

There's a technical argument to be made for not producing a lot of garbage in the store. Such images fill up the store quickly, requiring more frequent garbage collection, reducing the utility of the local store as a cache. This gets even worse for CIs, which push build dependencies, including such images, to a persistent cache.

samueldr · 2022-12-08T23:54:32Z

There's a technical argument to be made for not producing a lot of garbage in the store. Such images fill up the store quickly, requiring more frequent garbage collection, reducing the utility of the local store as a cache. This gets even worse for CIs, which push build dependencies, including such images, to a persistent cache.

You're right. I have been slowly working on a scheme to make the concerns separate, but coalesce in a single derivation. So this should be a non-issue in the end.

In other words, the user interface separates concerns, the implementation builds on discrete blocks, but they all (can) assemble in a single derivation.

This is anyway a requirement for it to properly work with the org's hydra and the ARM sd image, as the discrete components are too big to be cached. And as you said, it causes a lot of unneeded churn.

RaitoBezarius · 2022-12-17T01:11:39Z

Ready for another round of review.

`bootDisk` was generated manually in a runVM command, this is now refactored using `make-disk-image` facility, enabling: - a better name: `systemImage` rather than `bootDisk`, i.e. bootDisk contained more than only the boot files in fact - support for arbitrary additional paths & contents with permissions & ownership, not exposed yet through public API though - UEFI firmware manipulation at make-disk-image time, e.g. SecureBoot can be tested - EFI firmware usage is simplified - bootDisk / rootDisk are clearly separated to support the case when we do not want the default filesystem, but still want a bootloader, or do not want bootloader but still want the default root filesystem, etc. - no multiple disk when it is not needed

- OVMF firmware can be compiled with SMM support ; - make-disk-image can enforce SMM ; - QEMU test infrastructure can enforce SMM ; - disable by default because it seems broken on OVMF

…e → rootDevice

Mic92 · 2022-12-20T08:49:27Z

nixos/lib/make-disk-image.nix

  # This will be undersized slightly, as this is actually the offset of
  # the end of the partition. Generally it will be 1MiB smaller.
-  bootSize ? "256MiB"
+  bootSize ? "256M"


In 2900974 you are not just changing documentation but also revert `

- bootSize ? "256MiB" + bootSize ? "256M"

This should be in 5380f26

Mic92 · 2022-12-20T08:50:01Z

nixos/tests/secureboot.nix

    boot.loader.systemd-boot.enable = true;
    boot.loader.efi.canTouchEfiVariables = true;
-    environment.systemPackages = [ pkgs.efibootmgr pkgs.sbsigntool pkgs.sbctl ];
+    environment.systemPackages = with pkgs; [ efibootmgr sbsigntool sbctl ];


This also should not be in a commit that changes documentation but in your secureboot test commit.

Mic92 · 2022-12-20T08:50:43Z

nixos/maintainers/scripts/ec2/amazon-image.nix

    sizeMB = mkOption {
      type = with types; either (enum [ "auto" ]) int;
-      default = 2048;
+      default = 2252;


The commit does not say why the disk size was bumped.

Indeed, the Amazon test was broken due to not enough disk space, I don't know if it was induced by me or in-between latest release and master. Will double check.

Mic92 · 2022-12-20T08:52:12Z

doc/builders/images.xml

 <xi:include href="images/ocitools.section.xml" />
 <xi:include href="images/snaptools.section.xml" />
 <xi:include href="images/portableservice.section.xml" />
+ <xi:include href="images/makediskimage.section.xml" />


Did you change anything about the interface? Otherwise I would pull this out into its own PR.

Yes, I added parameters.

Mic92 · 2022-12-20T08:53:08Z

nixos/modules/virtualisation/qemu-vm.nix

          '';
        };

+    virtualisation.bootPartition =


Was the legacy boot broken before or broken by your changes?

Broken by my changes

RaitoBezarius · 2022-12-21T23:53:50Z

Closed in favor of #207039 #207038 #207043.

RaitoBezarius requested review from blitz and nikstur November 29, 2022 18:36

RaitoBezarius mentioned this pull request Dec 2, 2022

boot: Development (& experimental) setup for SecureBoot #202497

Closed

23 tasks

RaitoBezarius requested a review from SuperSandro2000 December 2, 2022 13:36

RaitoBezarius force-pushed the simplified-qemu-boot-disks branch from 5af712a to eda877f Compare December 2, 2022 13:37

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 2, 2022

RaitoBezarius force-pushed the simplified-qemu-boot-disks branch 2 times, most recently from 7d302b7 to 3dfa547 Compare December 2, 2022 13:53

RaitoBezarius requested a review from roberth December 2, 2022 13:56

nikstur approved these changes Dec 2, 2022

View reviewed changes

RaitoBezarius force-pushed the simplified-qemu-boot-disks branch 2 times, most recently from 07da111 to a993ab2 Compare December 4, 2022 15:10

RaitoBezarius marked this pull request as ready for review December 4, 2022 15:12

RaitoBezarius commented Dec 4, 2022

View reviewed changes

nixos/doc/manual/release-notes/rl-2211.section.md Show resolved Hide resolved

RaitoBezarius commented Dec 4, 2022

View reviewed changes

nixos/doc/manual/release-notes/rl-2305.section.md Outdated Show resolved Hide resolved

RaitoBezarius mentioned this pull request Dec 4, 2022

make_ext4fs: init at unstable-2017-05-21 #67744

Closed

1 task

samueldr reviewed Dec 5, 2022

View reviewed changes

nixos/lib/make-disk-image.nix Outdated Show resolved Hide resolved

samueldr reviewed Dec 5, 2022

View reviewed changes

nixos/lib/make-disk-image.nix Outdated Show resolved Hide resolved

SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 9, 2022

RaitoBezarius force-pushed the simplified-qemu-boot-disks branch 3 times, most recently from f1ee5a0 to cec8916 Compare December 17, 2022 01:11

RaitoBezarius removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 17, 2022

RaitoBezarius and others added 11 commits December 18, 2022 00:11

nixos/tests: test that secureboot prevent reboot

b862ea4

nixos/tests(EFI): offer System Management Mode enforcement

b7a435e

- OVMF firmware can be compiled with SMM support ; - make-disk-image can enforce SMM ; - QEMU test infrastructure can enforce SMM ; - disable by default because it seems broken on OVMF

nixos/tests/systemd-boot: fix the new systemd-boot version format

410a3de

nixos/tests(treewide): vdb → vda, vdc → vdb, vdd → vdc, … / bootDevic…

3b4d1a3

…e → rootDevice

doc/builders/images: nixos/lib/make-disk-image.nix

2900974

nixos/tests: fix legacy BIOS tests

98fba75

nixos/tests: fix FIDO2 & TPM test

88f6e25

nixos/tests: fix LVM2

f36ab40

maintainers/scripts/ec2/amazon-image: bump disk size to 2252MB

de58e33

nixos/doc: move release notes from 22.11 to 23.05

7625574

RaitoBezarius force-pushed the simplified-qemu-boot-disks branch from cec8916 to 7625574 Compare December 17, 2022 23:11

Mic92 requested a review from Lassulus December 18, 2022 10:01

Mic92 reviewed Dec 20, 2022

View reviewed changes

RaitoBezarius mentioned this pull request Dec 20, 2022

make-disk-image: documentation, UEFI variables recording, improved determinism #207038

Merged

13 tasks

RaitoBezarius closed this Dec 21, 2022

Uh oh!

Conversation

RaitoBezarius commented Nov 29, 2022

Description of changes

make-disk-image refactoring in QEMU test infrastructure

UEFI firmware infrastructure

Documentation

Uh oh!

RaitoBezarius commented Nov 29, 2022

Uh oh!

RaitoBezarius commented Dec 2, 2022

Uh oh!

nikstur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samueldr commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaitoBezarius commented Dec 5, 2022

Uh oh!

samueldr commented Dec 5, 2022

Uh oh!

roberth commented Dec 8, 2022

Uh oh!

samueldr commented Dec 8, 2022

Uh oh!

RaitoBezarius commented Dec 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaitoBezarius commented Dec 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

samueldr commented Dec 5, 2022 •

edited

Loading