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.

Sign up for GitHub

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

Jump to bottom

nixos/xen: refactor dom0 configuration #324911

Merged

emilazy merged 2 commits into NixOS:master from SigmaSquadron:xen-module

Sep 18, 2024

Contributor

SigmaSquadron commented Jul 5, 2024 •

edited

Loading

Description of changes

Refactors the Xen module for Domain 0.

Cleans up downstream systemd units in favour of using upstream units.
Xen 4.18 on Nixpkgs now supports EFI booting, so we have an EFI boot builder here that runs after systemd-boot-builder.py.
Add more options for setting up dom0 resource limits.
Adds options for the declarative configuration of oxenstored.
Disables the automatic bridge configuration, as it was broken.
Deprecates legacy boot.

Things done

Built on platform(s)
- x86_64-linux
Tested, as applicable:
- No automated tests for NixOS/xen yet.
24.11 Release Notes
- (Package updates) Added a release notes entry if the change is major or breaking
- (Module updates) Added a release notes entry if the change is significant
  - I added a release note under highlights. Let me know if this is too presumptuous.
Fits CONTRIBUTING.md.

Closes #129780, closes #127404.

Add a 👍 reaction to pull requests you find important.

SigmaSquadron requested a review from JulienMalka as a code owner

July 5, 2024 22:17

github-actions bot added 6.topic: nixos 8.has: documentation 8.has: changelog 8.has: module (update) labels

SigmaSquadron changed the title ~~Xen module~~ nixos/xen: refactor dom0 configuration

SigmaSquadron mentioned this pull request

xen: 4.15 -> {4.16,4.17,4.18,4.19} #324693

Merged

6 tasks

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels

SigmaSquadron force-pushed the xen-module branch 2 times, most recently from 5983d41 to 2a8bdec Compare

July 6, 2024 08:19

JulienMalka previously requested changes

View reviewed changes

Member

JulienMalka left a comment

This feels very intrusive to the systemd-boot-builder codebase to me, which will create maintenance burden. I've never worked with Xen, could you explain why this is necessary? Is it not possible to do the modification to the xen.cfg file at build time, like on the file passed to extraFiles?

This comment was marked as outdated.

Sign in to view

SigmaSquadron self-assigned this

SigmaSquadron requested a review from JulienMalka

July 8, 2024 14:29

SigmaSquadron force-pushed the xen-module branch 2 times, most recently from 1327508 to b7b29a5 Compare

July 10, 2024 19:25

Contributor Author

SigmaSquadron commented Jul 10, 2024

Removed instances of with lib; per review in #324693.

SigmaSquadron force-pushed the xen-module branch from b7b29a5 to 6e9b9b8 Compare

July 17, 2024 15:04

ofborg bot added the 2.status: merge conflict label

SigmaSquadron force-pushed the xen-module branch from 6e9b9b8 to a5f3491 Compare

July 17, 2024 22:00

ofborg bot removed the 2.status: merge conflict label

SigmaSquadron removed their assignment

SigmaSquadron force-pushed the xen-module branch from a5f3491 to c6ae504 Compare

July 19, 2024 20:07

Contributor Author

SigmaSquadron commented Jul 19, 2024 •

edited

Loading

I had some time, so I added more options to configure oxenstored.conf. The options haven't changed in a decade, so I think it's alright to skip building a new config generator for oxenstored.conf's custom syntax, and just use etc."xen/oxenstored.conf".text.

xl's next, but that's a major undertaking which is best left for a separate PR.

SigmaSquadron force-pushed the xen-module branch from c6ae504 to eba7bc4 Compare

July 19, 2024 21:14

SigmaSquadron requested a review from oxij

July 19, 2024 21:14

This comment was marked as outdated.

Sign in to view

SigmaSquadron force-pushed the xen-module branch 2 times, most recently from 95ecd4e to a03f9f6 Compare

July 20, 2024 19:49

nixos-discourse commented Sep 11, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4564

CertainLach approved these changes

View reviewed changes

SigmaSquadron added the 12.approvals: 1 label

This was referenced Sep 13, 2024

UKI direct boot #341357

Open

nixos/grub: add new implementation of install-grub.pl #317026

Open

Contributor Author

SigmaSquadron commented Sep 17, 2024

@emilazy I'm sorry to report that it has indeed been ~a week and only Lach has reviewed!

might I place this burden unto you?

Member

emilazy commented Sep 17, 2024

Yeah, I’ll take up the burden :) I am sick right now so my capacity is not at its best but I’ll try to do it tomorrow.

I notice that the module has gone through nixfmt; would you be able to reformat it in a separate commit before your substantive changes so that it’s easier to review exactly what changed, or do you think it has been sufficiently rewritten that comparing against the old state doesn’t even make sense?

SigmaSquadron force-pushed the xen-module branch from b7a8b06 to 980aedf Compare

September 17, 2024 20:21

Contributor Author

SigmaSquadron commented Sep 17, 2024

I notice that the module has gone through nixfmt; would you be able to reformat it in a separate commit before your substantive changes so that it’s easier to review exactly what changed, or do you think it has been sufficiently rewritten that comparing against the old state doesn’t even make sense?

I formatted the old version in a separate commit as requested. That said, the diff is still really hard to read because many options were moved around and config entries were deduplicated/nested, so I'd recommend looking at the full file instead of just the diff.

CertainLach reviewed

View reviewed changes

nixos/modules/virtualisation/xen-dom0.nix Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-dom0.nix Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-dom0.nix Outdated Show resolved Hide resolved


          nixos/xen: format with nixfmt-rfc-style

c3fa245

Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>

SigmaSquadron removed the 12.approvals: 1 label

CertainLach reviewed

View reviewed changes

nixos/modules/virtualisation/xen-dom0.nix Show resolved Hide resolved

SigmaSquadron force-pushed the xen-module branch from 980aedf to ad37ca0 Compare

September 17, 2024 21:45

CertainLach approved these changes

View reviewed changes

SigmaSquadron added the 12.approvals: 1 label

emilazy requested changes

View reviewed changes

Member

emilazy left a comment

This looks broadly good to me. I don’t know much about Xen, so my review here is mostly human linting and asking about things that stood out to me, but this shouldn’t interfere with setups that don’t use Xen and seems to be better quality code than what it’s replacing, which is good enough for me. A few things I’d like to see changed, but I’m deferring to @CertainLach’s approval as far as Xen matters go.

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

nixos/modules/virtualisation/xen-boot-builder.sh Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-boot-builder.sh Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-boot-builder.sh Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-dom0.nix Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-dom0.nix Outdated Show resolved Hide resolved

nixos/modules/virtualisation/xen-dom0.nix

    
                        Path to the Xen Store Daemon. This option is useful to

                        switch between the legacy C-based Xen Store Daemon, and

                        the newer OCaml-based Xen Store Daemon, `oxenstored`.

Member

emilazy Sep 18, 2024

If the C implementation is legacy and adds complexity to the implementation here, should we only support the OCaml one? Is there anything the OCaml one can’t do that the C one can?

Contributor Author

SigmaSquadron Sep 18, 2024

We can't, unfortunately. oxenstored does not support untrusted device domains, which is the main reason why Qubes OS (#341215) doesn't use it.

nixos/modules/virtualisation/xen-dom0.nix

Comment on lines +622 to +633

    
                    {

                      assertion = cfg.debug -> cfg.trace;

                      message = "Xen's debugging features are enabled, but logging is disabled. This is most likely not what you want.";

                    }

                    {

                      assertion = cfg.store.settings.quota.maxWatchEvents >= cfg.store.settings.quota.maxOutstanding;

                      message = ''

                        Upstream Xen recommends that maxWatchEvents be equal to or greater than maxOutstanding,

                        in order to mitigate denial of service attacks from malicious frontends.

                      '';

Member

emilazy Sep 18, 2024

Are these sufficiently bad configurations that we want them to be assertions, rather than warnings? (I’m perfectly fine with ruling out footguns like this; just want to check.)

Contributor Author

SigmaSquadron Sep 18, 2024

The quota stuff should certainly be kept, otherwise users might unknowingly open themselves to a known security vulnerability. The tracing stuff is more of a footgun, but could be replaced with a lib.warn with no issues. I don't see why someone would want to debug Xen without logs though.

nixos/modules/virtualisation/xen-dom0.nix Outdated Show resolved Hide resolved


          nixos/xen: refactor dom0 configuration

9e5f77a

- Cleans up downstream systemd units in favour of using upstream units.
- Xen 4.18 on Nixpkgs now supports EFI booting, so we have an EFI boot
  builder here that runs after systemd-boot-builder.py.
- Add more options for setting up dom0 resource limits.
- Adds options for the declarative configuration of oxenstored.
- Disables the automatic bridge configuration, as it was broken.
- Drops legacy BIOS boot
- Adds an EFI boot entry builder script.

Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Co-authored-by: Yaroslav Bolyukin <iam@lach.pw>

SigmaSquadron force-pushed the xen-module branch from ad37ca0 to 9e5f77a Compare

September 18, 2024 20:36

Contributor Author

SigmaSquadron commented Sep 18, 2024

Changes addressed, except for the removal of the footgun assertions.

emilazy approved these changes

View reviewed changes

Member

emilazy left a comment

Looks good to me! Any further improvements can be handled in future PRs. Thanks for all the work you’ve put into this.

@ofborg test simple

(just to make sure that NixOS eval is okay)

SigmaSquadron added 12.approvals: 2 and removed 12.approvals: 1 labels

Contributor Author

SigmaSquadron commented Sep 18, 2024

The simple test has passed!

emilazy merged commit 5320e21 into NixOS:master

24 checks passed

SigmaSquadron deleted the xen-module branch

September 18, 2024 22:23

Contributor Author

SigmaSquadron commented Sep 18, 2024

A sincere thank you to everyone who has helped in this Xen endeavour!

@CertainLach, @hehongbo, @digitalrayne, @radhus and @matdibu, I've sent everyone who has been involved in these Xen PRs an email which may be of interest to you. Let me know what you think!

(If you haven't received it, then I did a terrible job finding your address. Please do tell me if that's the case.)

hehongbo mentioned this pull request

Cannot build and deploy the new Xen Dom0 module #343522

Closed

SigmaSquadron added the 6.topic: xen-project label

CertainLach added a commit to CertainLach/lanzaboote that referenced this pull request


          feat: parse bootspec xen extension

347536a

As defined in NixOS/nixpkgs#324911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Kreyren Kreyren requested changes

radhus radhus left review comments

hehongbo hehongbo left review comments

emilazy emilazy approved these changes

CertainLach CertainLach approved these changes

JulienMalka JulienMalka left review comments

Labels

6.topic: nixos 6.topic: xen-project 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 12.approvals: 2