fix(kvm-clock): do not jump monotonic clock on restore

Firecracker has never advanced the clock on restore for any of the
supported clocksources. Since Linux 5.16, the KVM_CLOCK_REALTIME has
been passed to kvm-clock, causing the monotonic time in the guest to
jump when using kvm-clock as clock source.

Despite being unexpected and not what Firecracker should do, we
recognize this may be a valid usecase so this patch adds a way to
configure it, keeping the default to the expected documented behaviour.

This patch adds a new API flag to LoadSnapshot, clock_realtime, that
advances the clock on restore when set (default is False). Rather than
the clock flags being decided at snapshot time, the restore path ignores
those flags and decides what to do depending on the clock_realtime flag.
This is because the other available flag (KVM_CLOCK_TSC_STABLE) cannot
even be passed to `set_clock`, meaning the only valid flag is
KVM_CLOCK_REALTIME.

The name of the flag was kept generic as we may add this behaviour for
the other clock sources in the future, if the need arises.

Signed-off-by: Riccardo Mancini <mancio@amazon.com>

Author: Manciukic

CHANGELOG.md

@@ -50,6 +50,11 @@ and this project adheres to
   balloon statistics descriptor length to prevent a guest-controlled oversized
   descriptor from temporarily stalling the VMM event loop. Only affects microVMs
   with `stats_polling_interval_s > 0`.
+- [#5809](https://github.com/firecracker-microvm/firecracker/pull/5809): Fixed a
+  bug on host Linux >= 5.16 for x86_64 guests using the `kvm-clock` clock source
+  causing the monotonic clock to jump on restore. Users that want to keep the
+  previous behaviour can opt back in using the new `clock_realtime` flag in
+  `LoadSnapshot` API.
 
 ## [1.15.0]
 

docs/snapshotting/snapshot-support.md

@@ -493,6 +493,10 @@ resumed with the guest OS wall-clock continuing from the moment of the snapshot
 creation. For this reason, the wall-clock should be updated to the current time,
 on the guest-side. More details on how you could do this can be found at a
 [related FAQ](../../FAQ.md#my-guest-wall-clock-is-drifting-how-can-i-fix-it).
+When using `kvm-clock` as clock source on `x86_64`, it's possible to optionally
+set the `clock_realtime: true` in the `LoadSnapshot` request to advance the
+clock source on the guest at restore time. Note that this may cause issues
+within the guest as the clock source will appear to suddenly jump.
 
 ## Provisioning host disk space for snapshots
 

src/firecracker/src/api_server/request/snapshot.rs

@@ -111,6 +111,7 @@ fn parse_put_snapshot_load(body: &Body) -> Result<ParsedRequest, RequestError> {
         resume_vm: snapshot_config.resume_vm,
         network_overrides: snapshot_config.network_overrides,
         vsock_override: snapshot_config.vsock_override,
+        clock_realtime: snapshot_config.clock_realtime,
     };
 
     // Construct the `ParsedRequest` object.
@@ -189,6 +190,7 @@ mod tests {
             resume_vm: false,
             network_overrides: vec![],
             vsock_override: None,
+            clock_realtime: false,
         };
         let mut parsed_request = parse_put_snapshot(&Body::new(body), Some("load")).unwrap();
         assert!(
@@ -220,6 +222,7 @@ mod tests {
             resume_vm: false,
             network_overrides: vec![],
             vsock_override: None,
+            clock_realtime: false,
         };
         let mut parsed_request = parse_put_snapshot(&Body::new(body), Some("load")).unwrap();
         assert!(
@@ -251,6 +254,7 @@ mod tests {
             resume_vm: true,
             network_overrides: vec![],
             vsock_override: None,
+            clock_realtime: false,
         };
         let mut parsed_request = parse_put_snapshot(&Body::new(body), Some("load")).unwrap();
         assert!(
@@ -291,6 +295,7 @@ mod tests {
                 host_dev_name: String::from("vmtap2"),
             }],
             vsock_override: None,
+            clock_realtime: false,
         };
         let mut parsed_request = parse_put_snapshot(&Body::new(body), Some("load")).unwrap();
         assert!(
@@ -319,6 +324,7 @@ mod tests {
             resume_vm: true,
             network_overrides: vec![],
             vsock_override: None,
+            clock_realtime: false,
         };
         let parsed_request = parse_put_snapshot(&Body::new(body), Some("load")).unwrap();
         assert_eq!(

src/firecracker/swagger/firecracker.yaml

@@ -1602,7 +1602,7 @@ definitions:
         type: string
         description:
           The new path for the backing Unix Domain Socket.
-          
+
   SnapshotLoadParams:
     type: object
     description:
@@ -1650,6 +1650,13 @@ definitions:
           for restoring a snapshot with a different socket path than the one used
           when the snapshot was created. For example, when the original socket path
           is no longer available or when deploying to a different environment.
+      clock_realtime:
+        type: boolean
+        description:
+          "[x86_64 only] When set to true, passes KVM_CLOCK_REALTIME to
+          KVM_SET_CLOCK on restore, advancing kvmclock by the wall-clock time
+          elapsed since the snapshot was taken. When false (default), kvmclock resumes
+          from where it was at snapshot time."
 
 
   TokenBucket:

src/vmm/src/arch/x86_64/vm.rs

@@ -5,7 +5,7 @@ use std::fmt;
 use std::sync::{Arc, Mutex};
 
 use kvm_bindings::{
-    KVM_CLOCK_TSC_STABLE, KVM_IRQCHIP_IOAPIC, KVM_IRQCHIP_PIC_MASTER, KVM_IRQCHIP_PIC_SLAVE,
+    KVM_CLOCK_REALTIME, KVM_IRQCHIP_IOAPIC, KVM_IRQCHIP_PIC_MASTER, KVM_IRQCHIP_PIC_SLAVE,
     KVM_PIT_SPEAKER_DUMMY, MsrList, kvm_clock_data, kvm_irqchip, kvm_pit_config, kvm_pit_state2,
 };
 use kvm_ioctls::Cap;
@@ -127,13 +127,21 @@ impl ArchVm {
     /// - [`kvm_ioctls::VmFd::set_irqchip`] errors.
     /// - [`kvm_ioctls::VmFd::set_irqchip`] errors.
     /// - [`kvm_ioctls::VmFd::set_irqchip`] errors.
-    pub fn restore_state(&mut self, state: &VmState) -> Result<(), ArchVmError> {
+    pub fn restore_state(
+        &mut self,
+        state: &VmState,
+        clock_realtime: bool,
+    ) -> Result<(), ArchVmError> {
         self.fd()
             .set_pit2(&state.pitstate)
             .map_err(ArchVmError::SetPit2)?;
-        self.fd()
-            .set_clock(&state.clock)
-            .map_err(ArchVmError::SetClock)?;
+        let mut clock = state.clock;
+        clock.flags = if clock_realtime {
+            KVM_CLOCK_REALTIME
+        } else {
+            0
+        };
+        self.fd().set_clock(&clock).map_err(ArchVmError::SetClock)?;
         self.fd()
             .set_irqchip(&state.pic_master)
             .map_err(ArchVmError::SetIrqChipPicMaster)?;
@@ -167,9 +175,7 @@ impl ArchVm {
     pub fn save_state(&self) -> Result<VmState, ArchVmError> {
         let pitstate = self.fd().get_pit2().map_err(ArchVmError::VmGetPit2)?;
 
-        let mut clock = self.fd().get_clock().map_err(ArchVmError::VmGetClock)?;
-        // This bit is not accepted in SET_CLOCK, clear it.
-        clock.flags &= !KVM_CLOCK_TSC_STABLE;
+        let clock = self.fd().get_clock().map_err(ArchVmError::VmGetClock)?;
 
         let mut pic_master = kvm_irqchip {
             chip_id: KVM_IRQCHIP_PIC_MASTER,
@@ -248,8 +254,7 @@ impl fmt::Debug for VmState {
 #[cfg(test)]
 mod tests {
     use kvm_bindings::{
-        KVM_CLOCK_TSC_STABLE, KVM_IRQCHIP_IOAPIC, KVM_IRQCHIP_PIC_MASTER, KVM_IRQCHIP_PIC_SLAVE,
-        KVM_PIT_SPEAKER_DUMMY,
+        KVM_IRQCHIP_IOAPIC, KVM_IRQCHIP_PIC_MASTER, KVM_IRQCHIP_PIC_SLAVE, KVM_PIT_SPEAKER_DUMMY,
     };
 
     use crate::vstate::vm::VmState;
@@ -270,15 +275,26 @@ mod tests {
             vm_state.pitstate.flags | KVM_PIT_SPEAKER_DUMMY,
             KVM_PIT_SPEAKER_DUMMY
         );
-        assert_eq!(vm_state.clock.flags & KVM_CLOCK_TSC_STABLE, 0);
         assert_eq!(vm_state.pic_master.chip_id, KVM_IRQCHIP_PIC_MASTER);
         assert_eq!(vm_state.pic_slave.chip_id, KVM_IRQCHIP_PIC_SLAVE);
         assert_eq!(vm_state.ioapic.chip_id, KVM_IRQCHIP_IOAPIC);
 
         let (_, mut vm) = setup_vm_with_memory(0x1000);
         vm.setup_irqchip().unwrap();
 
-        vm.restore_state(&vm_state).unwrap();
+        vm.restore_state(&vm_state, false).unwrap();
+    }
+
+    #[cfg(target_arch = "x86_64")]
+    #[test]
+    fn test_vm_save_restore_state_kvm_clock_realtime() {
+        let (_, vm) = setup_vm_with_memory(0x1000);
+        vm.setup_irqchip().unwrap();
+        let vm_state = vm.save_state().unwrap();
+
+        let (_, mut vm) = setup_vm_with_memory(0x1000);
+        vm.setup_irqchip().unwrap();
+        vm.restore_state(&vm_state, true).unwrap();
     }
 
     #[cfg(target_arch = "x86_64")]
@@ -296,18 +312,18 @@ mod tests {
         // Try to restore an invalid PIC Master chip ID
         let orig_master_chip_id = vm_state.pic_master.chip_id;
         vm_state.pic_master.chip_id = KVM_NR_IRQCHIPS;
-        vm.restore_state(&vm_state).unwrap_err();
+        vm.restore_state(&vm_state, false).unwrap_err();
         vm_state.pic_master.chip_id = orig_master_chip_id;
 
         // Try to restore an invalid PIC Slave chip ID
         let orig_slave_chip_id = vm_state.pic_slave.chip_id;
         vm_state.pic_slave.chip_id = KVM_NR_IRQCHIPS;
-        vm.restore_state(&vm_state).unwrap_err();
+        vm.restore_state(&vm_state, false).unwrap_err();
         vm_state.pic_slave.chip_id = orig_slave_chip_id;
 
         // Try to restore an invalid IOPIC chip ID
         vm_state.ioapic.chip_id = KVM_NR_IRQCHIPS;
-        vm.restore_state(&vm_state).unwrap_err();
+        vm.restore_state(&vm_state, false).unwrap_err();
     }
 
     #[cfg(target_arch = "x86_64")]
@@ -321,6 +337,6 @@ mod tests {
         let serialized_data = bitcode::serialize(&state).unwrap();
         let restored_state: VmState = bitcode::deserialize(&serialized_data).unwrap();
 
-        vm.restore_state(&restored_state).unwrap();
+        vm.restore_state(&restored_state, false).unwrap();
     }
 }

src/vmm/src/builder.rs

@@ -438,6 +438,7 @@ pub fn build_microvm_from_snapshot(
     uffd: Option<Uffd>,
     seccomp_filters: &BpfThreadMap,
     vm_resources: &mut VmResources,
+    clock_realtime: bool,
 ) -> Result<Arc<Mutex<Vmm>>, BuildMicrovmFromSnapshotError> {
     // Build Vmm.
     debug!("event_start: build microvm from snapshot");
@@ -480,13 +481,15 @@ pub fn build_microvm_from_snapshot(
     #[cfg(target_arch = "aarch64")]
     {
         let mpidrs = construct_kvm_mpidrs(&microvm_state.vcpu_states);
+        // Ignore unused_variables check for this x86_64-specific flag
+        let _ = clock_realtime;
         // Restore kvm vm state.
         vm.restore_state(&mpidrs, &microvm_state.vm_state)?;
     }
 
     // Restore kvm vm state.
     #[cfg(target_arch = "x86_64")]
-    vm.restore_state(&microvm_state.vm_state)?;
+    vm.restore_state(&microvm_state.vm_state, clock_realtime)?;
 
     // Restore the boot source config paths.
     vm_resources.boot_source.config = microvm_state.vm_info.boot_source;

src/vmm/src/persist.rs

@@ -466,6 +466,7 @@ pub fn restore_from_snapshot(
         uffd,
         seccomp_filters,
         vm_resources,
+        params.clock_realtime,
     )
     .map_err(RestoreFromSnapshotError::Build)
 }

src/vmm/src/rpc_interface.rs

@@ -1286,6 +1286,7 @@ mod tests {
                 resume_vm: false,
                 network_overrides: vec![],
                 vsock_override: None,
+                clock_realtime: false,
             },
         )));
         check_unsupported(runtime_request(VmmAction::SetEntropyDevice(

src/vmm/src/vmm_config/snapshot.rs

@@ -81,6 +81,11 @@ pub struct LoadSnapshotParams {
     pub network_overrides: Vec<NetworkOverride>,
     /// When set, the vsock backend UDS path will be overridden
     pub vsock_override: Option<VsockOverride>,
+    /// [x86_64 only] When set to true, passes `KVM_CLOCK_REALTIME` to `KVM_SET_CLOCK` on restore,
+    /// advancing kvmclock by the wall-clock time elapsed since the snapshot was taken. This is
+    /// intended for live-migration scenarios. When false (default), kvmclock resumes from where it
+    /// was at snapshot time.
+    pub clock_realtime: bool,
 }
 
 /// Stores the configuration for loading a snapshot that is provided by the user.
@@ -113,6 +118,9 @@ pub struct LoadSnapshotConfig {
     /// Whether or not to override the vsock backend UDS path.
     #[serde(skip_serializing_if = "Option::is_none")]
     pub vsock_override: Option<VsockOverride>,
+    /// [x86_64 only] When set to true, passes `KVM_CLOCK_REALTIME` to `KVM_SET_CLOCK` on restore.
+    #[serde(default)]
+    pub clock_realtime: bool,
 }
 
 /// Stores the configuration used for managing snapshot memory.

src/vmm/src/vstate/vm.rs

@@ -879,7 +879,7 @@ pub(crate) mod tests {
         let serialized_data = bitcode::serialize(&state).unwrap();
 
         let restored_state: VmState = bitcode::deserialize(&serialized_data).unwrap();
-        vm.restore_state(&restored_state).unwrap();
+        vm.restore_state(&restored_state, false).unwrap();
 
         let mut resource_allocator = vm.resource_allocator();
         let gsi_new = resource_allocator.allocate_gsi_msi(1).unwrap()[0];