refactor: move arch specific code from vm.rs into modules #5031
+604
−646
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roypat
requested review from
ShadowCurse,
bchalios and
kalyazin
and removed request for
ShadowCurse
roypat
force-pushed
the
vm-refactor-pr
branch
4 times, most recently
from
21779af to
4e0dca0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5031 +/- ##
==========================================
+ Coverage 83.16% 83.20% +0.04%
==========================================
Files 245 247 +2
Lines 26645 26639 -6
==========================================
+ Hits 22159 22166 +7
+ Misses 4486 4473 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ShadowCurse
reviewed
Feb 14, 2025
roypat
force-pushed
the
vm-refactor-pr
branch
from
4e0dca0 to
c0d5c9d
Compare
bchalios
previously approved these changes
Feb 18, 2025
ShadowCurse
previously approved these changes
Feb 18, 2025
Follow a similar approach we already take for the vcpu module, where all the architecture specific stuff is inside aarch64/x86_64 modules, keeping only the common code in the `vm` module. No functional change intended. Signed-off-by: Patrick Roy <[email protected]>
Introduce `enum ArchVmError` which contains architecture specific variants for VM operations (mostly related to snapshot creation/restore). Do this by adapting exiting architecture specific enums that were used for snapshot restoration (x86_64)/GIC creation (aarch64). While we're at it, remove unused/duplicated enum variants, and on x86 use the SetIrqChip enum variant for failures inside `setup_irq_chip` instead of producing some generic `VmError`. Admittedly, there's a lot more cleanup that can be done here, but for now this is enough to attain my goals (separating architecture specific code). No functional change intended (apart from error messages changing). Signed-off-by: Patrick Roy <[email protected]>
Similarly to what we do in the vcpu module for the Vcpu/KvmVcpu structs. The difference is that we do not implement any function on `ArchVm`, so that common fields like `fd` can live in `Vm`, instead of needing to be duplicated into all the `ArchVm` types. Signed-off-by: Patrick Roy <[email protected]>
This allows us to not need to feed `struct Kvm` all the way down to Vcpu creation. While we're at it, abstract away the `MsrList` type, and just have a helper that returns a slice of u32 (as every call site converts the MsrList to this anyway). Signed-off-by: Patrick Roy <[email protected]>
This abstracts away the weird irqchip setup dance that builder.rs has been doing (where depending on architecture, irqchip needs to be created before or after vcpu creation. Removes a whole bunch of code that builder.rs that didn't really have too much business being there Signed-off-by: Patrick Roy <[email protected]>
Instead of doing the irqchip dance inside unit testing code, just call vm.create_vcpus, which does it for us. Signed-off-by: Patrick Roy <[email protected]>
We have some utility functions for creating dummy VMs/etc. in unittests, so let's try to use them instead of copy-pasting the same few lines around. Signed-off-by: Patrick Roy <[email protected]>
There's no need to use the GuestMemoryMmap to resolve the pointer to the start of the GuestMemoryRegion by resolving the guest physical address it starts at - because we literally store that pointer inside the GuestMemoryRegion, so can just use that. Signed-off-by: Patrick Roy <[email protected]>
Since we issue this ioctl with a constant address, it doesn't really matter _when_ we do it. Internally, it causes a hidden memslot of size 3 pages to be created. If we create it _after_ all other memslots, then this ioctl can fail with EEXIST if it would overlap some pre-existing memslot. Now, instead the creation of the overlapping memslot would fail with EEXIST. But this is a moot point, because if Firecracker ever tried to create a memslot that overlaps this one, something is fundamentally broken. Signed-off-by: Patrick Roy <[email protected]>
All of these were dead code (but the compiler isn't able to tell us because the vmm crate is a library crate where everything is `pub`, and it doesn't know that no downstream users outside of the `firecracker` crate exist, and that everything that's not used in `firecracker` is dead code). Signed-off-by: Patrick Roy <[email protected]>
Dropping a GuestMemoryMmap causes the memory to get unmapped, so if we try to KVM_RUN after dropping it, we'll just get EFAULT. Admittedly no idea how we didn't run into this issue before. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
vm-refactor-pr
branch
from
e829487 to
8a5cf78
Compare
ShadowCurse
approved these changes
Feb 18, 2025
bchalios
approved these changes
Feb 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow a similar approach we already take for the vcpu module, where all
the architecture specific stuff is inside aarch64/x86_64 modules,
keeping only the common code in the
vmmodule.No functional change intended.
Signed-off-by: Patrick Roy [email protected]
Some more unmotivated-at-face-value code I had lying around, which I wrote while messing around for #4522, but which constitutes an improvement standalone anyway (at least imo it makes the code a bit easier to read due to less cfg-ing)
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
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.Follow a similar approach we already take for the vcpu module, where all
the architecture specific stuff is inside aarch64/x86_64 modules,
keeping only the common code in the
vmmodule.No functional change intended.
Signed-off-by: Patrick Roy [email protected]