chore: Update to kvm-bindings 0.8.0 #4581
Merged
Conversation
This file contains hidden or 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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4581 +/- ##
=======================================
Coverage 82.13% 82.14%
=======================================
Files 255 255
Lines 31274 31277 +3
=======================================
+ Hits 25688 25691 +3
Misses 5586 5586
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
force-pushed
the
kvm-bindings-0.8.0-again
branch
3 times, most recently
from
60ab983 to
6e6b4e6
Compare
roypat
added
the
Status: Awaiting review
roypat
force-pushed
the
kvm-bindings-0.8.0-again
branch
from
73c5678 to
01daf91
Compare
zulinx86
requested changes
Apr 29, 2024
ShadowCurse
reviewed
Apr 29, 2024
roypat
force-pushed
the
kvm-bindings-0.8.0-again
branch
from
f3abea7 to
a530a7a
Compare
This was referenced Apr 29, 2024
roypat
force-pushed
the
kvm-bindings-0.8.0-again
branch
2 times, most recently
from
78454d9 to
4e65114
Compare
ShadowCurse
reviewed
Apr 30, 2024
Separate the call to `KVM_RUN` from the handling of the result value. This makes the handling of the `VcpuExit` unit-testable without needing to hack in `cfg(not(test))` code that compiles out the `KVM_RUN` call at compile time. For this, separate the parts of KvmVcpu that the exit handler needs mutable access to into a `Peripherals` structure, which is passed to `handle_kvm_exit`. This is needed so that we can shared the `&mut` to `kvm_vcpu` into `fd` and "the rest". Co-Authored-By: Egor Lazarchuk <[email protected]> Signed-off-by: Patrick Roy <[email protected]>
Upgrade kvm-bindings to 0.8.0 to make use of upstream serialization support. New kvm-bindings no longer derives `Clone` on `kvm_xsave` (since this is not correct to do for all use cases post Linux 5.17, due to the structure now having a flexible array members). While Firecracker does not make use of this flexible array members (according to KVM documentation, it will only be present if `arch_prctl` is used [1]), meaning our usage of `Clone` was not problematic, it's easier for now to just clean up our use-sites of `kvm_xsave::clone` (which was only used in test code) - turns out they were either dead code, or should be replaced with successive calls to `save_state()`. Additionally, `VcpuFd::run` now takes `self` by mutable reference, which is the reason for the preceding refactors. The result of `run` holds references into the `kvm_run` structure contained by `VcpuFd`, meaning the mutable borrow on `Vcpu::fd` will stay alive until the return value is dropped. However, if the call to `run` is behind a function call that takes the entirety of `Vcpu` by mutable reference, this means that we cannot borrow any other fields of `Vcpu` mutably until we drop the `VcpuExit` - yet we need to do exactly this to actually handle the KVM_EXIT. So, instead inline the call to `run` so that the borrow checker understand we only borrow `Vcpu::fd` instead of the entire `Vcpu` object, meaning it allows us to access the _other_ fields of the `Vcpu` struct during handling. [1]: https://docs.kernel.org/virt/kvm/api.html#kvm-get-xsave2 Signed-off-by: Patrick Roy <[email protected]>
Delete various error variants in the x86_64 version of KvmVcpuError that were never constructed/used. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
kvm-bindings-0.8.0-again
branch
from
30b343b to
7d49849
Compare
ShadowCurse
approved these changes
Apr 30, 2024
zulinx86
approved these changes
Apr 30, 2024
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.
Changes
The goal of this PR is to update kvm-bindings from our custom fork to upstream version 0.8.0, as since rust-vmm/kvm-bindings#101 the serialization support we had our fork for is merged upstream.
Additionally, refactor our emulation code to be more easily unit-testable.
Reason
The refactors are necessary due to a change in kvm-ioctls. The
selfparameter toVcpuFd::runis now a mutable reference. Please see the final commit for a detailed breakdown of why this was incompatible with our existing mocking-based approach to testing vmexits.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
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.