From 5d3ff6493a5900c020fabd363872603c974c5744 Mon Sep 17 00:00:00 2001
From: Noah Gold <nkgold@google.com>
Date: Fri, 25 Aug 2023 18:15:42 -0700
Subject: [PATCH] devices: virtio: pvclock: fix feature acks.

Similar to virtio-balloon, feature acks were not being tracked properly
and caused snapshotting to fail. This CL applies the same fix we used on
the balloon device.

BUG=b:297294476
TEST=snapshotting doesn't fail on this device anymore.

Change-Id: I6a4faa088eadf043fcc49479e3e565af18dbaa96
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4814771
Reviewed-by: Frederick Mayle <fmayle@google.com>
Commit-Queue: Noah Gold <nkgold@google.com>
---
 devices/src/virtio/pvclock.rs | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/devices/src/virtio/pvclock.rs b/devices/src/virtio/pvclock.rs
index 9b191a79d5..7ae6dc35b9 100644
--- a/devices/src/virtio/pvclock.rs
+++ b/devices/src/virtio/pvclock.rs
@@ -209,6 +209,7 @@ pub struct PvClock {
     /// by the PvClockWorker thread but read by PvClock from the mmio bus in the main thread.
     total_suspend_ns: Arc<AtomicU64>,
     features: u64,
+    acked_features: u64,
     worker_thread: Option<WorkerThread<WorkerReturn>>,
     /// If the device is sleeping, a [PvClockWorkerSnapshot] that can re-create the worker
     /// will be stored here. (We can't just store the worker itself as it contains an object
@@ -223,6 +224,7 @@ struct PvClockSnapshot {
     paused_worker: Option<PvClockWorkerSnapshot>,
     total_suspend_ns: u64,
     features: u64,
+    acked_features: u64,
 }
 
 impl PvClock {
@@ -235,6 +237,7 @@ impl PvClock {
                 | 1 << VIRTIO_PVCLOCK_F_TSC_STABLE
                 | 1 << VIRTIO_PVCLOCK_F_INJECT_SLEEP
                 | 1 << VIRTIO_PVCLOCK_F_CLOCKSOURCE_RATING,
+            acked_features: 0,
             worker_thread: None,
             paused_worker: None,
         }
@@ -704,11 +707,12 @@ impl VirtioDevice for PvClock {
         self.features
     }
 
-    // TODO(b/237300012): `self.features` should not be mutated here. Also need
-    // to check if `value` is subset of `self.features`.
-    // Example: https://source.chromium.org/chromium/chromiumos/platform/crosvm/+/main:devices/src/virtio/balloon.rs;l=690-693;drc=d2ca9e04c7e477c09a8e14eff392b2d90f52e295
-    fn ack_features(&mut self, value: u64) {
-        self.features &= value;
+    fn ack_features(&mut self, mut value: u64) {
+        if value & !self.features != 0 {
+            warn!("virtio-pvclock got unknown feature ack {:x}", value);
+            value &= self.features;
+        }
+        self.acked_features |= value;
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {
@@ -780,6 +784,7 @@ impl VirtioDevice for PvClock {
     fn virtio_snapshot(&self) -> anyhow::Result<serde_json::Value> {
         serde_json::to_value(PvClockSnapshot {
             features: self.features,
+            acked_features: self.acked_features,
             total_suspend_ns: self.total_suspend_ns.load(Ordering::SeqCst),
             tsc_frequency: self.tsc_frequency,
             paused_worker: self.paused_worker.clone(),
@@ -796,6 +801,7 @@ impl VirtioDevice for PvClock {
                 snap.features,
             );
         }
+        self.acked_features = snap.acked_features;
         self.total_suspend_ns
             .store(snap.total_suspend_ns, Ordering::SeqCst);
         self.paused_worker = snap.paused_worker;