Skip to content

grub2_pvgrub_image, grub: Refactor, add PVH support #374753

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

grub2_pvgrub_image, grub: Refactor, add PVH support #374753

wants to merge 2 commits into from

Conversation

hehongbo
Copy link
Contributor

@hehongbo hehongbo commented Jan 18, 2025 •
edited
Loading

This one is revived from #121514 by @radhus, with the GRUB configuration files and memdisk stuff being reused from the grub2_pvgrub_image instead of duplicated, since the making of PvGrub and PvhGrub image shares the same logic.

The result, is a new grub2_pvhgrub_image being provided along with the old grub2_pvgrub_image, which can be used to boot PVH guests. And this, hopefully, provides an upgrade path for PV guests since PV is old, and has performance overhead for historical reasons.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@hehongbo hehongbo mentioned this pull request Jan 18, 2025
Closed
13 tasks
@hehongbo hehongbo marked this pull request as draft January 18, 2025 08:50
@hehongbo
Copy link
Contributor Author

ping @NixOS/xen-project

I still have to iron out these errors from format checks, meanwhile, as this is also discussed on William's original PR, should we skip the arm/aarch64 architectures for now? I can't imagine if PvhGrub is useful here in Nixpkgs if we don't have the hypervisor itself working properly. (Maybe after #345217?)

@hehongbo hehongbo requested review from a team, CertainLach and SigmaSquadron January 18, 2025 09:00
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 18, 2025
SigmaSquadron
SigmaSquadron reviewed Jan 20, 2025
View reviewed changes
@hehongbo hehongbo marked this pull request as ready for review January 22, 2025 10:48
SigmaSquadron
SigmaSquadron reviewed Jan 22, 2025
View reviewed changes
@hehongbo hehongbo added the 6.topic: xen-project Issues and PRs related to the Xen Project Hypervisor. label Feb 12, 2025
SigmaSquadron
SigmaSquadron reviewed Mar 26, 2025
View reviewed changes
SigmaSquadron
SigmaSquadron reviewed Mar 26, 2025
View reviewed changes
@github-actions github-actions bot removed the 6.topic: xen-project Issues and PRs related to the Xen Project Hypervisor. label Mar 27, 2025
@hehongbo hehongbo requested a review from SigmaSquadron March 27, 2025 03:01
SigmaSquadron
SigmaSquadron reviewed Mar 27, 2025
View reviewed changes
@SigmaSquadron SigmaSquadron mentioned this pull request Mar 27, 2025
Closed
47 tasks
@hehongbo hehongbo force-pushed the xen-grub-pvh branch 2 times, most recently from e0953a3 to 24063a7 Compare March 27, 2025 07:25
@hehongbo hehongbo requested a review from SigmaSquadron March 27, 2025 07:49
@hehongbo
Copy link
Contributor Author

And then a final nitpicky point I want to get rid of: The entire thing has nothing to do with EFI, but the attrset efiSystemsBuild is there and was moved from the original grub2_pvgrub_image.

I did a brief archaeology, it looks like it's there when being created, and maybe was derived from the main package of GRUB for reuse purposes.

Changed to avoid further confusion.

SigmaSquadron
SigmaSquadron approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for picking this up.

@hehongbo hehongbo added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 28, 2025
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@hehongbo hehongbo force-pushed the xen-grub-pvh branch 3 times, most recently from 13c65b3 to 1dd1ff0 Compare April 3, 2025 02:20
@hehongbo
Copy link
Contributor Author

hehongbo commented Apr 3, 2025

No changes; just rebase and resolve conflicts.

@hehongbo
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 374753

x86_64-linux

✅ 4 packages built:
  • grub2_pvgrub_image
  • grub2_pvhgrub_image
  • grub2_xen_pvh
  • grub2_xen_pvh.debug

SigmaSquadron
SigmaSquadron reviewed Apr 26, 2025
View reviewed changes
@SigmaSquadron SigmaSquadron mentioned this pull request Apr 26, 2025
Open
@hehongbo hehongbo added the 6.topic: xen-project Issues and PRs related to the Xen Project Hypervisor. label May 27, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 24, 2025
@hehongbo hehongbo mentioned this pull request Jul 27, 2025
Merged
13 tasks
hehongbo and others added 2 commits August 6, 2025 10:38
@hehongbo @radhus
grub: build grub2_xen_pvh
2c3ce3e 
This enables the GRUB package to be built for the `xen_pvh` platform, which is
needed to generate a corresponding GRUB image and boot PVH DomUs.

Signed-off-by: Hongbo <hehongbo@mail.com>
Co-authored-by: William Johansson <radar@radhuset.org>
@hehongbo @radhus
grub: build grub2_pvhgrub_image together with grub2_pvgrub_image
47881f9 
This enables the GRUB package to be built for the `xen_pvh` platform, which is
needed to generate a corresponding GRUB image and boot PVH DomUs.

Also, modifications made to the original `grub2_pvgrub_image` package:
- Use split phases and avoid overriding `buildCommand` during the build.
- Remove logic that excludes `all_video.mod` (already ironed out by upstream).
- Fix `lib.platforms` and remove platforms that cannot build.
- Update the description.
- Add maintainer information of the Xen Project team.

Signed-off-by: Hongbo <hehongbo@mail.com>
Reviewed-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Co-authored-by: William Johansson <radar@radhuset.org>
@nixpkgs-ci nixpkgs-ci bot removed 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: xen-project Issues and PRs related to the Xen Project Hypervisor. labels Aug 6, 2025
@hehongbo
Copy link
Contributor Author

hehongbo commented Aug 6, 2025

Rebase again to resolve conflicts, and I realized that I should fix platform support declarations/meta after considering the actual support matrix, which was #427180 and #428878 that had prompted me to do so.

  • xenPvhSystemsBuild will be used to present meta.platforms when building pkgs.grub2_xen_pvh. It was previously set to platforms.gnu plus platforms.linux, which is not accurate. The grub2: refactor platforms logic, avoid abusing meta.broken #428878 fixed that for pkgs.grub2_xen (PvGrub package), and now we just align with that.
  • And once again, I renamed xenSystemsBuild into targets in grub2_pvhgrub_image, similar to William's original PR. Because now we have xenSystemsBuild and xenPvhSystemsBuild separately in the GRUB main package, we should either align with that, or move the attrset's name to something else to avoid future confusion.
  • And similarly, chain meta.platforms to the support targets attrset in grub2_pvhgrub_image. According to configure.ac, only the two x86 platforms are supported in PvGrub and PvhGrub, with the latter one sharing the i386 binary/image for both platforms.

It's mysterious that arm, aarch64, riscv32, riscv64 were in efiSystemsBuild of pkgs.grub2_pvgrub_image (and by the way, the "efi" is because of copy-pasted code and has nothing to do with EFI). It turned out to be a fix of evaluation errors in #213612, all because that meta.platforms was set incorrectly in the first place. Now we should make it clear, only two x86 platforms are supported, ARM and RISC-V are not, at least for now.

@hehongbo hehongbo mentioned this pull request Aug 6, 2025
Merged
14 tasks
@hehongbo hehongbo changed the title grub: build PVH variant of Grub2 for Xen grub2_pvgrub_image, grub: Refactor, add PVH support Aug 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SigmaSquadron SigmaSquadron SigmaSquadron approved these changes

@CertainLach CertainLach Awaiting requested review from CertainLach CertainLach was automatically assigned from NixOS/xen-project

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hehongbo @SigmaSquadron @wegank