From f68a2ef306807fdd298d85133d19eb595731d152 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 25 Apr 2024 10:32:08 +0100 Subject: [PATCH] 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 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 --- Cargo.lock | 20 ++++++-------------- Cargo.toml | 3 --- src/cpu-template-helper/Cargo.toml | 2 +- src/utils/Cargo.toml | 2 +- src/vmm/Cargo.toml | 4 ++-- src/vmm/src/vstate/vcpu/mod.rs | 12 ++++++------ src/vmm/src/vstate/vcpu/x86_64.rs | 7 +++---- src/vmm/tests/integration_tests.rs | 8 -------- 8 files changed, 19 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ec19e376921..523beae14c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -772,19 +772,20 @@ dependencies = [ [[package]] name = "kvm-bindings" -version = "0.7.0" -source = "git+https://github.com/firecracker-microvm/kvm-bindings?tag=v0.7.0-2#60cc04e2658646516f4e763eca77fbfa1cf5ec1f" +version = "0.8.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a82e7e8725a39a0015e511a46cc1f7d90cecc180db1610c4d0d4339a9e48bd21" dependencies = [ "serde", - "serde-big-array", "vmm-sys-util", + "zerocopy", ] [[package]] name = "kvm-ioctls" -version = "0.16.0" +version = "0.17.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9002dff009755414f22b962ec6ae6980b07d6d8b06e5297b1062019d72bd6a8c" +checksum = "bedae2ca4a531bebe311abaf9691f5cc14eaa21475243caa2e39c43bb872947d" dependencies = [ "bitflags 2.5.0", "kvm-bindings", @@ -1184,15 +1185,6 @@ dependencies = [ "serde_derive", ] -[[package]] -name = "serde-big-array" -version = "0.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11fc7cc2c76d73e0f27ee52abbd64eec84d46f370c88371120433196934e4b7f" -dependencies = [ - "serde", -] - [[package]] name = "serde_derive" version = "1.0.198" diff --git a/Cargo.toml b/Cargo.toml index 0ac71e19e0c..bc29988d5a3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,3 @@ panic = "abort" [profile.release] panic = "abort" lto = true - -[patch.crates-io] -kvm-bindings = { git = "https://github.com/firecracker-microvm/kvm-bindings", tag = "v0.7.0-2", features = ["fam-wrappers"] } diff --git a/src/cpu-template-helper/Cargo.toml b/src/cpu-template-helper/Cargo.toml index 619573dfde2..2c5b59afb0c 100644 --- a/src/cpu-template-helper/Cargo.toml +++ b/src/cpu-template-helper/Cargo.toml @@ -19,7 +19,7 @@ serde_json = "1.0.116" thiserror = "1.0.59" vmm = { path = "../vmm" } -vmm-sys-util = "0.12.1" +vmm-sys-util = { version = "0.12.1", features = ["with-serde"] } [features] tracing = ["log-instrument", "vmm/tracing"] diff --git a/src/utils/Cargo.toml b/src/utils/Cargo.toml index 6c7e7532071..992b068febe 100644 --- a/src/utils/Cargo.toml +++ b/src/utils/Cargo.toml @@ -16,7 +16,7 @@ log-instrument = { path = "../log-instrument", optional = true } serde = { version = "1.0.198", features = ["derive"] } thiserror = "1.0.59" vm-memory = { version = "0.14.1", features = ["backend-mmap", "backend-bitmap"] } -vmm-sys-util = "0.12.1" +vmm-sys-util = { version = "0.12.1", features = ["with-serde"] } [dev-dependencies] serde_json = "1.0.116" diff --git a/src/vmm/Cargo.toml b/src/vmm/Cargo.toml index 246a4c518b0..46f4ec9533d 100644 --- a/src/vmm/Cargo.toml +++ b/src/vmm/Cargo.toml @@ -19,8 +19,8 @@ crc64 = "2.0.0" derive_more = { version = "0.99.17", default-features = false, features = ["from", "display"] } displaydoc = "0.2.4" event-manager = "0.4.0" -kvm-bindings = { version = "0.7.0", features = ["fam-wrappers"] } -kvm-ioctls = "0.16.0" +kvm-bindings = { version = "0.8.0", features = ["fam-wrappers", "serde"] } +kvm-ioctls = "0.17.0" lazy_static = "1.4.0" libc = "0.2.117" linux-loader = "0.11.0" diff --git a/src/vmm/src/vstate/vcpu/mod.rs b/src/vmm/src/vstate/vcpu/mod.rs index 231234ffe60..2ecb8db6521 100644 --- a/src/vmm/src/vstate/vcpu/mod.rs +++ b/src/vmm/src/vstate/vcpu/mod.rs @@ -74,7 +74,7 @@ pub struct VcpuConfig { } // Using this for easier explicit type-casting to help IDEs interpret the code. -type VcpuCell = Cell>; +type VcpuCell = Cell>; /// Error type for [`Vcpu::start_threaded`]. #[derive(Debug, derive_more::From, thiserror::Error)] @@ -112,7 +112,7 @@ impl Vcpu { if cell.get().is_some() { return Err(VcpuError::VcpuTlsInit); } - cell.set(Some(self as *const Vcpu)); + cell.set(Some(self as *mut Vcpu)); Ok(()) }) } @@ -128,7 +128,7 @@ impl Vcpu { // _before_ running this, then there is nothing we can do. Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| { if let Some(vcpu_ptr) = cell.get() { - if vcpu_ptr == self as *const Vcpu { + if vcpu_ptr == self as *mut Vcpu { Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| cell.take()); return Ok(()); } @@ -149,13 +149,13 @@ impl Vcpu { /// dereferencing from pointer an already borrowed `Vcpu`. unsafe fn run_on_thread_local(func: F) -> Result<(), VcpuError> where - F: FnOnce(&Vcpu), + F: FnOnce(&mut Vcpu), { Self::TLS_VCPU_PTR.with(|cell: &VcpuCell| { if let Some(vcpu_ptr) = cell.get() { // Dereferencing here is safe since `TLS_VCPU_PTR` is populated/non-empty, // and it is being cleared on `Vcpu::drop` so there is no dangling pointer. - let vcpu_ref: &Vcpu = &*vcpu_ptr; + let vcpu_ref = &mut *vcpu_ptr; func(vcpu_ref); Ok(()) } else { @@ -1003,7 +1003,7 @@ pub mod tests { Vcpu::register_kick_signal_handler(); let (vm, mut vcpu, _) = setup_vcpu(0x1000); - let kvm_run = + let mut kvm_run = kvm_ioctls::KvmRunWrapper::mmap_from_fd(&vcpu.kvm_vcpu.fd, vm.fd().run_size()) .expect("cannot mmap kvm-run"); let success = Arc::new(std::sync::atomic::AtomicBool::new(false)); diff --git a/src/vmm/src/vstate/vcpu/x86_64.rs b/src/vmm/src/vstate/vcpu/x86_64.rs index bf3b21d95e8..d139b849c63 100644 --- a/src/vmm/src/vstate/vcpu/x86_64.rs +++ b/src/vmm/src/vstate/vcpu/x86_64.rs @@ -550,7 +550,7 @@ impl Peripherals { } /// Structure holding VCPU kvm state. -#[derive(Clone, Serialize, Deserialize)] +#[derive(Serialize, Deserialize)] pub struct VcpuState { /// CpuId. pub cpuid: CpuId, @@ -866,11 +866,10 @@ mod tests { // Test `is_tsc_scaling_required` as if it were on the same // CPU model as the one in the snapshot state. let (_vm, vcpu, _) = setup_vcpu(0x1000); - let orig_state = vcpu.save_state().unwrap(); { // The frequency difference is within tolerance. - let mut state = orig_state.clone(); + let mut state = vcpu.save_state().unwrap(); state.tsc_khz = Some( state.tsc_khz.unwrap() + state.tsc_khz.unwrap() * TSC_KHZ_TOL_NUMERATOR / TSC_KHZ_TOL_DENOMINATOR / 2, @@ -882,7 +881,7 @@ mod tests { { // The frequency difference is over the tolerance. - let mut state = orig_state; + let mut state = vcpu.save_state().unwrap(); state.tsc_khz = Some( state.tsc_khz.unwrap() + state.tsc_khz.unwrap() * TSC_KHZ_TOL_NUMERATOR / TSC_KHZ_TOL_DENOMINATOR * 2, diff --git a/src/vmm/tests/integration_tests.rs b/src/vmm/tests/integration_tests.rs index 8fb7395ab00..6067c252548 100644 --- a/src/vmm/tests/integration_tests.rs +++ b/src/vmm/tests/integration_tests.rs @@ -280,7 +280,6 @@ fn test_create_and_load_snapshot() { #[test] fn test_snapshot_load_sanity_checks() { use vmm::persist::SnapShotStateSanityCheckError; - use vmm::vmm_config::machine_config::MAX_SUPPORTED_VCPUS; let mut microvm_state = get_microvm_state_from_snapshot(); @@ -294,13 +293,6 @@ fn test_snapshot_load_sanity_checks() { snapshot_state_sanity_check(µvm_state), Err(SnapShotStateSanityCheckError::NoMemory) ); - - // Create MAX_SUPPORTED_VCPUS vCPUs starting from 1 vCPU. - for _ in 0..MAX_SUPPORTED_VCPUS.ilog2() { - microvm_state - .vcpu_states - .append(&mut microvm_state.vcpu_states.clone()); - } } fn get_microvm_state_from_snapshot() -> MicrovmState {