openvmm: registering a time source while VM time runs from a nonzero start panics

Creating a time source after VM time has restarted from a nonzero value panics in TimerState::start, rather than returning the new source. This affects registration while the keeper is running, including after restoring nonzero VM time or stopping and restarting an advancing clock.

In vm/vmcore/src/vmtime.rs, VmTimeSourceBuilder::build initializes the new timer state at zero, then synchronizes it with the keeper's returned state:

        let mut state = Arc::new(RwLock::new(TimerState::new(driver, VmTime::from_100ns(0))));
        // Synchronize the time.
        {
            let state = Arc::get_mut(&mut state).unwrap().get_mut();
            match time {
                TimeState::Stopped(vmtime) => state.reset(vmtime),
                TimeState::Started(timestamp) => state.start(timestamp),
            }
        }

The stopped branch resets the fresh state to the keeper's time. The running branch instead calls start directly. TimerState::start reads the fresh state's stopped time and checks assert_eq!(now.vmtime, vmtime). When the keeper's start time is nonzero, that assertion compares the returned nonzero timestamp with zero and panics.

Trigger

Restore a VmTimeKeeper to VmTime::from_100ns(1_000_000), await keeper.start(), then await keeper.builder().build(&driver). The same mismatch occurs if VM time advances, is stopped and restarted, and a source is registered during that subsequent run. Registration while stopped already takes the reset branch.

Fix

fix.patch resets the fresh timer state to timestamp.vmtime before calling state.start(timestamp). This satisfies start's invariant while retaining the keeper's original running timestamp, so the new source joins the existing clock rather than creating a new time anchor. No keeper protocol or saved-state format change is needed.

Proposed patch

Patch against OpenVMM 998b9f2402050cd16d1faa897bc831ef1434be48. Save the contents below as fix.patch and run git apply fix.patch.

fix.patch

diff --git a/vm/vmcore/src/vmtime.rs b/vm/vmcore/src/vmtime.rs
index 7a5e82b6c0b29d593d5c2a9351e09423a876f4d4..9cbce9f2c0f9d4acd049dc2f5cd8b9009316e11f 100644
--- a/vm/vmcore/src/vmtime.rs
+++ b/vm/vmcore/src/vmtime.rs
@@ -628,7 +628,13 @@ pub async fn build(&self, driver: &impl SpawnDriver) -> Result<VmTimeSource, Tim
             let state = Arc::get_mut(&mut state).unwrap().get_mut();
             match time {
                 TimeState::Stopped(vmtime) => state.reset(vmtime),
-                TimeState::Started(timestamp) => state.start(timestamp),
+                TimeState::Started(timestamp) => {
+                    // The fresh state is stopped at 0, and a start begins at
+                    // the stopped time, so stop it at the running clock's
+                    // start time first.
+                    state.reset(timestamp.vmtime);
+                    state.start(timestamp);
+                }
             }
         }
         let mut keeper = SecondaryKeeper {
@@ -1258,4 +1264,152 @@ async fn test_timeout_nearly_half_the_ring_ahead(driver: DefaultDriver) {
         check_unrepresentable_timeout(&driver, &mut access).await;
         keeper.stop().await;
     }
+
+    /// Regression test for BUG-10: a source built while VM time runs from a
+    /// start time other than 0 (here after a restore) reads the running clock.
+    /// Before the fix, `build` started a fresh state stopped at 0 with the
+    /// running timestamp, and `TimerState::start`'s assertion panicked.
+    #[test]
+    fn build_while_running_from_nonzero_time_reads_the_running_clock() {
+        pal_async::DefaultPool::run_with(async |driver| {
+            let start = VmTime::from_100ns(1_000_000);
+            let mut keeper = VmTimeKeeper::new(&driver, VmTime::from_100ns(0));
+            keeper.restore(SavedState::from_vmtime(start)).await;
+            keeper.start().await;
+            let source = keeper.builder().build(&driver).await.unwrap();
+            assert!(!source.access("bug10").now().is_before(start));
+            keeper.stop().await;
+            assert_eq!(source.access("bug10").now(), keeper.save().vmtime);
+        })
+    }
+
+    /// The four keeper states a source can be built in (BUG-10): stopped at 0,
+    /// stopped at t != 0, running from 0, running from t != 0. Right after
+    /// `build`, and again after a later stop and start (or start and stop),
+    /// the new source reads the primary's VM time: its stopped time, or a
+    /// running time between two readings of a source built before.
+    #[test]
+    fn build_reads_the_primary_time_in_every_keeper_state() {
+        pal_async::DefaultPool::run_with(async |driver| {
+            let t = VmTime::from_100ns(7_000_000);
+            for (time, running) in [
+                (VmTime::from_100ns(0), false),
+                (t, false),
+                (VmTime::from_100ns(0), true),
+                (t, true),
+            ] {
+                let mut keeper = VmTimeKeeper::new(&driver, VmTime::from_100ns(0));
+                let reference = keeper.builder().build(&driver).await.unwrap();
+                keeper.restore(SavedState::from_vmtime(time)).await;
+                if running {
+                    keeper.start().await;
+                }
+                let source = keeper.builder().build(&driver).await.unwrap();
+                let (reference, source) = (reference.access("reference"), source.access("new"));
+                let check = |keeper: &VmTimeKeeper, running: bool, when: &str| {
+                    if running {
+                        let before = reference.now();
+                        let now = source.now();
+                        let after = reference.now();
+                        assert!(
+                            !now.is_before(before) && !after.is_before(now),
+                            "{time:?} {when}: {before:?} {now:?} {after:?}"
+                        );
+                        assert!(!now.is_before(time), "{time:?} {when}");
+                    } else {
+                        assert_eq!(source.now(), keeper.save().vmtime, "{time:?} {when}");
+                        assert_eq!(reference.now(), keeper.save().vmtime, "{time:?} {when}");
+                    }
+                };
+                check(&keeper, running, "after the build");
+                if running {
+                    keeper.stop().await;
+                    check(&keeper, false, "after a stop");
+                    keeper.start().await;
+                    check(&keeper, true, "after a start");
+                } else {
+                    keeper.start().await;
+                    check(&keeper, true, "after a start");
+                    keeper.stop().await;
+                    check(&keeper, false, "after a stop");
+                }
+            }
+        })
+    }
+
+    /// BUG-10 with concurrent builds: Start, Stop and Reset (through
+    /// `restore`) answered while sources are built concurrently, from other
+    /// tasks. After each answer, every source built so far, including those
+    /// whose registration raced with the request, reads the protocol's time:
+    /// after a stop, the saved stop time, with the stop clock's reading, and
+    /// no earlier than any time read before the stop; after a reset, the reset
+    /// time; after a start, a running time no earlier than it. (Before the
+    /// fix, a build that raced with a start from a nonzero time panicked.)
+    #[async_test]
+    async fn builds_concurrent_with_start_stop_and_reset_read_the_keeper_time(
+        driver: DefaultDriver,
+    ) {
+        use pal_async::task::Spawn;
+        let mut keeper = VmTimeKeeper::with_stop_clock(&driver, VmTime::from_100ns(0), || Some(42));
+        let builder = keeper.builder().clone();
+        let mut sources = Vec::new();
+        let spawn_builds = |sources: &mut Vec<_>| {
+            for _ in 0..4 {
+                let builder = builder.clone();
+                let task_driver = driver.clone();
+                sources.push(driver.spawn("bug10-build", async move {
+                    builder.build(&task_driver).await.unwrap()
+                }));
+            }
+        };
+        let mut built = Vec::new();
+        for round in 0..64u64 {
+            let reset_to = VmTime::from_100ns(round * 1_000_000);
+            spawn_builds(&mut sources);
+            keeper.restore(SavedState::from_vmtime(reset_to)).await;
+            for task in sources.drain(..) {
+                built.push(task.await);
+            }
+            for source in &built {
+                assert_eq!(
+                    source.access("bug10").now(),
+                    reset_to,
+                    "round {round}: after the reset"
+                );
+            }
+            spawn_builds(&mut sources);
+            keeper.start().await;
+            for task in sources.drain(..) {
+                built.push(task.await);
+            }
+            let running: Vec<_> = built
+                .iter()
+                .map(|source| source.access("bug10").now())
+                .collect();
+            for &time in &running {
+                assert!(!time.is_before(reset_to), "round {round}: after the start");
+            }
+            spawn_builds(&mut sources);
+            keeper.stop().await;
+            for task in sources.drain(..) {
+                built.push(task.await);
+            }
+            let saved = keeper.save();
+            assert_eq!(saved.stop_host_time(), Some(42), "round {round}");
+            for &time in &running {
+                assert!(
+                    !saved.vmtime.is_before(time),
+                    "round {round}: the stop time is the latest"
+                );
+            }
+            for source in &built {
+                assert_eq!(
+                    source.access("bug10").now(),
+                    saved.vmtime,
+                    "round {round}: after the stop"
+                );
+            }
+        }
+        assert_eq!(built.len(), 64 * 12);
+    }
 }

vmcore: join the running clock when building a VM time source

Navigation Menu

Repository navigation

openvmm: registering a time source while VM time runs from a nonzero start panics #1142

Trigger

Fix

Proposed patch

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

openvmm: registering a time source while VM time runs from a nonzero start panics #1142

Description

Trigger

Fix

Activity

tangruize commented on Nov 1, 2026

Proposed patch

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions