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

ovmf: expose EFI prefixes and refactor qemu-vm with it #187887

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Aug 22, 2022

Description of changes

In summary:

  • EDK2 mkDerivation supports functional-style mkDerivation ;
  • OVMF exposes EFI prefixes in passthru as firmware and variables
  • virtualisation/qemu-vm uses it to pass them in the machinery, consolidating it ;
  • adding myself as a maintainer of OVMF as I know well the ecosystem and am interested into maintaining it

How to test this? : nix-build nixos/tests/systemd-boot.nix -A basic, it if works, then QEMU's EFI still works fine. :)

It is pretty common to require the EFI firmware and variables of OVMF, this exposes it in passthru as suggested by @roberth on Matrix.
qemu-vm can be refactored with this, showing an example of usage.
This makes it easier to consolidate and add new architectures that OVMF supports, e.g. RISC-V, Raspberry Pi, etc.
I tested systemd-boot.basic test with this, which relies on EFI, so it should have tested this part of the code.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 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
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@danielfullmer danielfullmer left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

nixos/modules/virtualisation/qemu-vm.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/qemu-vm.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/OVMF/default.nix Outdated Show resolved Hide resolved
@roberth
Copy link
Member

roberth commented Aug 22, 2022

Could you add some tests to passthru.tests in the edk2 and ovmf packages?

I assume we already have tests for this, but I don't know.

@RaitoBezarius
Copy link
Member Author

Could you add some tests to passthru.tests in the edk2 and ovmf packages?

I assume we already have tests for this, but I don't know.

We do not have direct tests for this, except that using EFI in NixOS tests are using ovmf (and then edk2) indirectly, would you like me to add the indirect tests?

@roberth
Copy link
Member

roberth commented Aug 23, 2022

OfBorg doesn't seem to pick up on the lowercase commit message prefix.

@ofborg build OVMF OVMF.passthru.tests

@RaitoBezarius
Copy link
Member Author

@roberth
Copy link
Member

roberth commented Aug 23, 2022

@roberth unclear if https://logs.nix.ci/?key=nixos/nixpkgs.187887&attempt_id=a3efc083-4804-4b47-b326-aef7d9d3c51b is a failure or not to me.

It's a clean refactor producing the exact same derivation, so the problem isn't caused by this PR.

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

Successfully merging this pull request may close these issues.

None yet

4 participants