Support for PVH boot protocol in Firecracker #5048

roypat · 2025-02-24T14:08:24Z

This is a rebased version of the feature/pvh feature branch contributed by @cperciva a while ago.

In Linux guests supporting the PVH protocol (which is all linux guests compiled with CONFIG_PVH=y, a default), Firecracker will now boot guests using the PVH boot protocol. Our performance testing has shown there to be no difference in performance to the old way of booting using the Linux boot protocol.

Functionally, this means Firecracker can now boot FreeBSD guests (which only support the PVH boot protocol).

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

I have read and understand CONTRIBUTING.md.
I have run tools/devtool checkstyle to verify that the PR passes the
automated style checks.
I have described what is done in these changes, why they are needed, and
how they are solving the problem in a clear and encompassing way.
I have updated any relevant documentation (both in code and in the docs)
in the PR.
I have mentioned all user-facing changes in CHANGELOG.md.
If a specific issue led to this PR, this PR closes the issue.
When making API changes, I have followed the
Runbook for Firecracker API changes.
I have tested all new and changed functionalities in unit tests and/or
integration tests.
I have linked an issue to every new TODO.

This functionality cannot be added in rust-vmm.

codecov · 2025-02-24T14:15:52Z

Codecov Report

Attention: Patch coverage is 87.22467% with 29 lines in your changes missing coverage. Please review.

Project coverage is 83.19%. Comparing base (dd0bf86) to head (cb790e4).

Files with missing lines	Patch %	Lines
src/vmm/src/arch/x86_64/mod.rs	92.45%	8 Missing ⚠️
src/vmm/src/arch/mod.rs	0.00%	7 Missing ⚠️
src/vmm/src/arch/x86_64/regs.rs	87.71%	7 Missing ⚠️
src/vmm/src/builder.rs	84.09%	7 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #5048    +/-   ##
========================================
  Coverage   83.19%   83.19%            
========================================
  Files         247      247            
  Lines       26641    26821   +180     
========================================
+ Hits        22163    22313   +150     
- Misses       4478     4508    +30

Flag	Coverage Δ
5.10-c5n.metal	`83.63% <85.91%> (-0.05%)`	⬇️
5.10-m5n.metal	`83.62% <85.91%> (-0.04%)`	⬇️
5.10-m6a.metal	`82.83% <85.91%> (-0.04%)`	⬇️
5.10-m6g.metal	`79.61% <65.11%> (-0.06%)`	⬇️
5.10-m6i.metal	`83.61% <85.91%> (-0.04%)`	⬇️
5.10-m7g.metal	`79.61% <65.11%> (-0.06%)`	⬇️
6.1-c5n.metal	`83.64% <85.91%> (-0.03%)`	⬇️
6.1-m5n.metal	`83.62% <85.91%> (-0.03%)`	⬇️
6.1-m6a.metal	`82.83% <85.91%> (-0.04%)`	⬇️
6.1-m6g.metal	`79.61% <65.11%> (-0.06%)`	⬇️
6.1-m6i.metal	`83.61% <85.91%> (-0.04%)`	⬇️
6.1-m7g.metal	`79.61% <65.11%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PR firecracker-microvm#5048 is the merge of the long-lived PVH feature branch. The commits on this branch were made long before we changes the gitlint rules to more closely follow Linux rules when it comes to sign-offs from co-authors (as used to not only not require them, but not allow them in the first place, meaning the first 3 commit in this PR are only signed by of Colin and me, but not the coauthor from a few years ago). Explicitly skip this test for this one PR. Signed-off-by: Patrick Roy <[email protected]>

ShadowCurse

How hard would it be to merge some commits and give them more standard names(feat:, ...)?

ShadowCurse · 2025-02-24T17:16:10Z

src/vmm/src/arch/x86_64/mod.rs

+fn add_memmap_entry(
+    memmap: &mut Vec<hvm_memmap_table_entry>,
+    addr: u64,
+    size: u64,
+    mem_type: u32,
+) -> Result<(), ConfigurationError> {
+    // Add the table entry to the vector
+    memmap.push(hvm_memmap_table_entry {
+        addr,
+        size,
+        type_: mem_type,
+        reserved: 0,
+    });
+
+    Ok(())
+}


just inline this

this really better to be inlined. You are only saving 3 lines from this on the very first call to the function. Other calls to this function are already vertically formatted, so they take same amount of lines.

I don't really see any strong argument for doing it either way. I kinda like it as is because there's less visual clutter. Inlining it would also mean doing the constructing the struct at all 8 call sites, which involves explicitly writing out all fields, because outside of this functions the local variable names don't match up with the struct names.I

It doesnt need to return Result though, I'll give you that.

@bchalios heya, could you weigh in here to resolve our deadlock? :D

docs/rootfs-and-kernel-setup.md

docs/pvh.md

CHANGELOG.md

tests/integration_tests/style/test_gitlint.py

roypat · 2025-02-25T06:47:18Z

How hard would it be to merge some commits and give them more standard names(feat:, ...)?

Well, it's the merge of a feature branch, they're ought to be a little bit different from what we're used to. We never rewrote feature branches on merge to make the commit history more like a normal PR. I guess we could here because its small, but I don't really think we need/should.

cperciva · 2025-02-25T07:03:12Z

How hard would it be to merge some commits and give them more standard names(feat:, ...)?

Well, it's the merge of a feature branch, they're ought to be a little bit different from what we're used to. We never rewrote feature branches on merge to make the commit history more like a normal PR. I guess we could here because its small, but I don't really think we need/should.

For the record: I'm totally fine with having my commit messages rewritten to match house style, if the Firecracker team wants to do this. I kept the original author's style for his commits that I reworked, and then just used FreeBSD commit message style for my own commits simply because that's what I'm used to.

I know I'm a guest here. :-)

ShadowCurse · 2025-02-25T13:49:04Z

Well, ok, let commit message be as they are. But I feel 5d37abd b95ea58 and aaf9609 commits should be merged with original commits. We have a unofficial rule that all commits should compile, so let's at least keep this.

PR firecracker-microvm#5048 is the merge of the long-lived PVH feature branch. The commits on this branch were made long before we changes the gitlint rules to more closely follow Linux rules when it comes to sign-offs from co-authors (as used to not only not require them, but not allow them in the first place, meaning the first 3 commit in this PR are only signed by of Colin and me, but not the coauthor from a few years ago). Explicitly skip this test for this one PR. Signed-off-by: Patrick Roy <[email protected]>

In order to properly configure the initial vCPU register state and boot parameters in guest memory, we must specify which boot protocol to use with the kernel entry point address. On x86-64 (the only architecture where multiple boot protocols are supported) we print the protocol used to load the kernel at the debug log level. Create an EntryPoint struct that contains the required information. This structure will later be used in the vCPU configuration methods to set the appropriate initial conditions for the guest. This commit also splits the load_kernel function into an x86-64 specific version and an aarch64 specific version. Signed-off-by: Colin Percival <[email protected]> Co-authored-by: Alejandro Jimenez <[email protected]> Signed-off-by: Patrick Roy <[email protected]>

Set the initial values of the KVM vCPU registers as specified in the PVH boot ABI: https://xenbits.xen.org/docs/unstable/misc/pvh.html Add stub bits for aarch64; PVH mode does not exist there. Signed-off-by: Colin Percival <[email protected]> Co-authored-by: Alejandro Jimenez <[email protected]>

PR firecracker-microvm#5048 is the merge of the long-lived PVH feature branch. The commits on this branch were made long before we changes the gitlint rules to more closely follow Linux rules when it comes to sign-offs from co-authors (as used to not only not require them, but not allow them in the first place, meaning the first 3 commit in this PR are only signed by of Colin and me, but not the coauthor from a few years ago). Explicitly skip this test for this one PR. Signed-off-by: Patrick Roy <[email protected]>

roypat · 2025-02-25T15:49:25Z

Well, ok, let commit message be as they are. But I feel 5d37abd b95ea58 and aaf9609 commits should be merged with original commits. We have a unofficial rule that all commits should compile, so let's at least keep this.

okay I've folded all the lint fixes/etc into their respective commits. probably should've just done that in the first place whenever I rebased this branch anyway.

PR firecracker-microvm#5048 is the merge of the long-lived PVH feature branch. The commits on this branch were made long before we changes the gitlint rules to more closely follow Linux rules when it comes to sign-offs from co-authors (as used to not only not require them, but not allow them in the first place, meaning the first 3 commit in this PR are only signed by of Colin and me, but not the coauthor from a few years ago). Explicitly skip this test for this one PR. Signed-off-by: Patrick Roy <[email protected]>

Fill the hvm_start_info and related structures as specified in the PVH boot protocol. Write the data structures to guest memory at the GPA that will be stored in %rbx when the guest starts. Signed-off-by: Colin Percival <[email protected]> Co-authored-by: Alejandro Jimenez <[email protected]> Signed-off-by: Patrick Roy <[email protected]>

The PVH boot support bits are under Oracle copyright. Signed-off-by: Colin Percival <[email protected]>

While I'm here, clarify that the existing instructions are for building a Linux kernel and rootfs. Signed-off-by: Colin Percival <[email protected]>

Brief description of the PVH boot mode. We defer to Xen for technical details of how CPU registers are set up upon kernel entry. Signed-off-by: Colin Percival <[email protected]> Signed-off-by: Patrick Roy <[email protected]>

Firecracker now supports PVH boot as an alternative to "Linux" boot on the x86_64 architecture. This makes it possible for FreeBSD to boot, and also affects how Linux kernels compiled with the CONFIG_PVH=y option boot. Signed-off-by: Colin Percival <[email protected]> Signed-off-by: Patrick Roy <[email protected]>

Use test_api_happy_start to assert that the log message that indicates usage of PVH boot protocol is present. Only x86_64 guests support PVH boot, and on ARM we do not emit any log messages, so restrict the assertion to x86_64 platforms. Signed-off-by: Patrick Roy <[email protected]>

Hardcoding main means the test looks at the wrong range of commits for feature branches, which can result in problem if feature branches are long-lived. Signed-off-by: Patrick Roy <[email protected]>

PR firecracker-microvm#5048 is the merge of the long-lived PVH feature branch. The commits on this branch were made long before we changes the gitlint rules to more closely follow Linux rules when it comes to sign-offs from co-authors (as used to not only not require them, but not allow them in the first place, meaning the first 3 commit in this PR are only signed by of Colin and me, but not the coauthor from a few years ago). Explicitly skip this test for this one PR. Signed-off-by: Patrick Roy <[email protected]>

roypat force-pushed the pvh-rebased branch from 2e02bbc to db536d7 Compare February 24, 2025 14:18

roypat force-pushed the pvh-rebased branch from e867de6 to 3b0d636 Compare February 24, 2025 14:34

roypat force-pushed the pvh-rebased branch from 3b0d636 to 9e3579f Compare February 24, 2025 14:36

roypat marked this pull request as ready for review February 24, 2025 15:51

roypat requested review from xmarcalx, kalyazin, pb8o and Manciukic as code owners February 24, 2025 15:51

roypat added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Feb 24, 2025

roypat requested a review from bchalios February 24, 2025 16:08

ShadowCurse reviewed Feb 24, 2025

View reviewed changes

roypat force-pushed the pvh-rebased branch from 5bcfcff to d17a274 Compare February 25, 2025 15:31

cperciva and others added 2 commits February 25, 2025 15:34

roypat force-pushed the pvh-rebased branch from d17a274 to 7248846 Compare February 25, 2025 15:44

roypat force-pushed the pvh-rebased branch from 7248846 to ecc01c7 Compare February 25, 2025 15:46

roypat force-pushed the pvh-rebased branch from ecc01c7 to 0e0ca9e Compare February 25, 2025 15:47

bchalios previously approved these changes Feb 26, 2025

View reviewed changes

roypat dismissed bchalios’s stale review via 70ff23e February 26, 2025 15:58

roypat force-pushed the pvh-rebased branch from 765456b to 70ff23e Compare February 26, 2025 15:58

roypat force-pushed the pvh-rebased branch from 70ff23e to bb4a860 Compare February 26, 2025 15:59

roypat requested review from ShadowCurse and bchalios February 26, 2025 15:59

cperciva and others added 8 commits February 27, 2025 06:50

test_licenses: Accept Oracle copyright

5182b9a

The PVH boot support bits are under Oracle copyright. Signed-off-by: Colin Percival <[email protected]>

Add FreeBSD kernel+rootfs build instructions

78912ef

While I'm here, clarify that the existing instructions are for building a Linux kernel and rootfs. Signed-off-by: Colin Percival <[email protected]>

Add docs/pvh.md

6f2e5ab

Brief description of the PVH boot mode. We defer to Xen for technical details of how CPU registers are set up upon kernel entry. Signed-off-by: Colin Percival <[email protected]> Signed-off-by: Patrick Roy <[email protected]>

fix: dont hardcode "main" in gitlint test

043a42c

Hardcoding main means the test looks at the wrong range of commits for feature branches, which can result in problem if feature branches are long-lived. Signed-off-by: Patrick Roy <[email protected]>

roypat force-pushed the pvh-rebased branch from 8028787 to 675249a Compare February 27, 2025 06:50

Merge branch 'main' into pvh-rebased

cb790e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for PVH boot protocol in Firecracker #5048

Support for PVH boot protocol in Firecracker #5048

roypat commented Feb 24, 2025 •

edited

Loading

codecov bot commented Feb 24, 2025 •

edited

Loading

ShadowCurse left a comment

ShadowCurse Feb 24, 2025

ShadowCurse Feb 26, 2025

roypat Feb 27, 2025 •

edited

Loading

roypat Feb 27, 2025

roypat commented Feb 25, 2025

cperciva commented Feb 25, 2025

ShadowCurse commented Feb 25, 2025

roypat commented Feb 25, 2025

Support for PVH boot protocol in Firecracker #5048

Are you sure you want to change the base?

Support for PVH boot protocol in Firecracker #5048

Conversation

roypat commented Feb 24, 2025 • edited Loading

License Acceptance

PR Checklist

codecov bot commented Feb 24, 2025 • edited Loading

Codecov Report

ShadowCurse left a comment

Choose a reason for hiding this comment

ShadowCurse Feb 24, 2025

Choose a reason for hiding this comment

ShadowCurse Feb 26, 2025

Choose a reason for hiding this comment

roypat Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

roypat Feb 27, 2025

Choose a reason for hiding this comment

roypat commented Feb 25, 2025

cperciva commented Feb 25, 2025

ShadowCurse commented Feb 25, 2025

roypat commented Feb 25, 2025

roypat commented Feb 24, 2025 •

edited

Loading

codecov bot commented Feb 24, 2025 •

edited

Loading

roypat Feb 27, 2025 •

edited

Loading