Skip to content

Commit 01daf91

Browse files
committed
chore: upgrade kvm-bindings to 0.8.0
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 our 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]>
1 parent 08cfdd6 commit 01daf91

File tree

9 files changed

+20
-40
lines changed

9 files changed

+20
-40
lines changed

Cargo.lock

Lines changed: 6 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,3 @@ panic = "abort"
2828
[profile.release]
2929
panic = "abort"
3030
lto = true
31-
32-
[patch.crates-io]
33-
kvm-bindings = { git = "https://github.com/firecracker-microvm/kvm-bindings", tag = "v0.7.0-2", features = ["fam-wrappers"] }

src/cpu-template-helper/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ serde_json = "1.0.116"
1919
thiserror = "1.0.59"
2020

2121
vmm = { path = "../vmm" }
22-
vmm-sys-util = "0.12.1"
22+
vmm-sys-util = { version = "0.12.1", features = ["with-serde"] }
2323

2424
[features]
2525
tracing = ["log-instrument", "vmm/tracing"]

src/utils/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ log-instrument = { path = "../log-instrument", optional = true }
1616
serde = { version = "1.0.198", features = ["derive"] }
1717
thiserror = "1.0.59"
1818
vm-memory = { version = "0.14.1", features = ["backend-mmap", "backend-bitmap"] }
19-
vmm-sys-util = "0.12.1"
19+
vmm-sys-util = { version = "0.12.1", features = ["with-serde"] }
2020

2121
[dev-dependencies]
2222
serde_json = "1.0.116"

src/vmm/Cargo.toml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ crc64 = "2.0.0"
1919
derive_more = { version = "0.99.17", default-features = false, features = ["from", "display"] }
2020
displaydoc = "0.2.4"
2121
event-manager = "0.4.0"
22-
kvm-bindings = { version = "0.7.0", features = ["fam-wrappers"] }
23-
kvm-ioctls = "0.16.0"
22+
kvm-bindings = { version = "0.8.0", features = ["fam-wrappers", "serde"] }
23+
kvm-ioctls = "0.17.0"
2424
lazy_static = "1.4.0"
2525
libc = "0.2.117"
2626
linux-loader = "0.11.0"

src/vmm/src/vstate/vcpu/aarch64.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ mod tests {
434434
// This should fail with ENOEXEC.
435435
// https://elixir.bootlin.com/linux/v5.10.176/source/arch/arm64/kvm/arm.c#L1165
436436
let (mut vm, _) = setup_vm(0x1000);
437-
let mut vcpu = Vcpu::new(0, &vm, EventFd::new(libc::EFD_NONBLOCK).unwrap()).unwrap();
437+
let vcpu = Vcpu::new(0, &vm, EventFd::new(libc::EFD_NONBLOCK).unwrap()).unwrap();
438438
vm.setup_irqchip(1).unwrap();
439439

440440
vcpu.dump_cpu_config().unwrap_err();

src/vmm/src/vstate/vcpu/mod.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ pub struct VcpuConfig {
7676
}
7777

7878
// Using this for easier explicit type-casting to help IDEs interpret the code.
79-
type VcpuCell = Cell<Option<*const Vcpu>>;
79+
type VcpuCell = Cell<Option<*mut Vcpu>>;
8080

8181
/// Error type for [`Vcpu::start_threaded`].
8282
#[derive(Debug, derive_more::From, thiserror::Error)]
@@ -117,7 +117,7 @@ impl Vcpu {
117117
if cell.get().is_some() {
118118
return Err(VcpuError::VcpuTlsInit);
119119
}
120-
cell.set(Some(self as *const Vcpu));
120+
cell.set(Some(self as *mut Vcpu));
121121
Ok(())
122122
})
123123
}
@@ -133,7 +133,7 @@ impl Vcpu {
133133
// _before_ running this, then there is nothing we can do.
134134
Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| {
135135
if let Some(vcpu_ptr) = cell.get() {
136-
if vcpu_ptr == self as *const Vcpu {
136+
if vcpu_ptr == self as *mut Vcpu {
137137
Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| cell.take());
138138
return Ok(());
139139
}
@@ -154,13 +154,13 @@ impl Vcpu {
154154
/// dereferencing from pointer an already borrowed `Vcpu`.
155155
unsafe fn run_on_thread_local<F>(func: F) -> Result<(), VcpuError>
156156
where
157-
F: FnOnce(&Vcpu),
157+
F: FnOnce(&mut Vcpu),
158158
{
159159
Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| {
160160
if let Some(vcpu_ptr) = cell.get() {
161161
// Dereferencing here is safe since `TLS_VCPU_PTR` is populated/non-empty,
162162
// and it is being cleared on `Vcpu::drop` so there is no dangling pointer.
163-
let vcpu_ref: &Vcpu = &*vcpu_ptr;
163+
let vcpu_ref = &mut *vcpu_ptr;
164164
func(vcpu_ref);
165165
Ok(())
166166
} else {
@@ -1001,7 +1001,7 @@ pub mod tests {
10011001
Vcpu::register_kick_signal_handler();
10021002
let (vm, mut vcpu, _) = setup_vcpu(0x1000);
10031003

1004-
let kvm_run = kvm_ioctls::KvmRunWrapper::mmap_from_fd(&vcpu.fd, vm.fd().run_size())
1004+
let mut kvm_run = kvm_ioctls::KvmRunWrapper::mmap_from_fd(&vcpu.fd, vm.fd().run_size())
10051005
.expect("cannot mmap kvm-run");
10061006
let success = Arc::new(std::sync::atomic::AtomicBool::new(false));
10071007
let vcpu_success = success.clone();

src/vmm/src/vstate/vcpu/x86_64.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,7 @@ impl Vcpu {
543543
}
544544

545545
/// Structure holding VCPU kvm state.
546-
#[derive(Clone, Serialize, Deserialize)]
546+
#[derive(Serialize, Deserialize)]
547547
pub struct VcpuState {
548548
/// CpuId.
549549
pub cpuid: CpuId,
@@ -860,11 +860,10 @@ mod tests {
860860
// Test `is_tsc_scaling_required` as if it were on the same
861861
// CPU model as the one in the snapshot state.
862862
let (_vm, vcpu, _) = setup_vcpu(0x1000);
863-
let orig_state = vcpu.save_state().unwrap();
864863

865864
{
866865
// The frequency difference is within tolerance.
867-
let mut state = orig_state.clone();
866+
let mut state = vcpu.save_state().unwrap();
868867
state.tsc_khz = Some(
869868
state.tsc_khz.unwrap()
870869
+ state.tsc_khz.unwrap() * TSC_KHZ_TOL_NUMERATOR / TSC_KHZ_TOL_DENOMINATOR / 2,
@@ -876,7 +875,7 @@ mod tests {
876875

877876
{
878877
// The frequency difference is over the tolerance.
879-
let mut state = orig_state;
878+
let mut state = vcpu.save_state().unwrap();
880879
state.tsc_khz = Some(
881880
state.tsc_khz.unwrap()
882881
+ state.tsc_khz.unwrap() * TSC_KHZ_TOL_NUMERATOR / TSC_KHZ_TOL_DENOMINATOR * 2,

src/vmm/tests/integration_tests.rs

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,6 @@ fn test_create_and_load_snapshot() {
280280
#[test]
281281
fn test_snapshot_load_sanity_checks() {
282282
use vmm::persist::SnapShotStateSanityCheckError;
283-
use vmm::vmm_config::machine_config::MAX_SUPPORTED_VCPUS;
284283

285284
let mut microvm_state = get_microvm_state_from_snapshot();
286285

@@ -294,13 +293,6 @@ fn test_snapshot_load_sanity_checks() {
294293
snapshot_state_sanity_check(&microvm_state),
295294
Err(SnapShotStateSanityCheckError::NoMemory)
296295
);
297-
298-
// Create MAX_SUPPORTED_VCPUS vCPUs starting from 1 vCPU.
299-
for _ in 0..MAX_SUPPORTED_VCPUS.ilog2() {
300-
microvm_state
301-
.vcpu_states
302-
.append(&mut microvm_state.vcpu_states.clone());
303-
}
304296
}
305297

306298
fn get_microvm_state_from_snapshot() -> MicrovmState {

0 commit comments

Comments
 (0)