Skip to content

Commit

Permalink
chore: upgrade kvm-bindings to 0.8.0
Browse files Browse the repository at this point in the history
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 <roypat@amazon.co.uk>
  • Loading branch information
roypat committed Apr 30, 2024
1 parent 189cbd8 commit f68a2ef
Show file tree
Hide file tree
Showing 8 changed files with 19 additions and 39 deletions.
20 changes: 6 additions & 14 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
2 changes: 1 addition & 1 deletion src/cpu-template-helper/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
2 changes: 1 addition & 1 deletion src/utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
4 changes: 2 additions & 2 deletions src/vmm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
12 changes: 6 additions & 6 deletions src/vmm/src/vstate/vcpu/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub struct VcpuConfig {
}

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

/// Error type for [`Vcpu::start_threaded`].
#[derive(Debug, derive_more::From, thiserror::Error)]
Expand Down Expand Up @@ -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(())
})
}
Expand All @@ -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(());
}
Expand All @@ -149,13 +149,13 @@ impl Vcpu {
/// dereferencing from pointer an already borrowed `Vcpu`.
unsafe fn run_on_thread_local<F>(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 {
Expand Down Expand Up @@ -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));
Expand Down
7 changes: 3 additions & 4 deletions src/vmm/src/vstate/vcpu/x86_64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
8 changes: 0 additions & 8 deletions src/vmm/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -294,13 +293,6 @@ fn test_snapshot_load_sanity_checks() {
snapshot_state_sanity_check(&microvm_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 {
Expand Down

0 comments on commit f68a2ef

Please sign in to comment.