fix: resolve TODO to stop failed virtio devices

aaron-ang · aaron-ang · commit 8d6d69cdce38 · 2026-01-05T23:33:11.000-08:00
Implemented VirtioDevice reset methods to run on failure.

Signed-off-by: Aaron Ang &lt;aaron.angyd@gmail.com&gt;
diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs
@@ -545,6 +545,10 @@ pub(crate) mod tests {
         fn is_activated(&self) -> bool {
             false
         }
+
+        fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+            None
+        }
     }
 
     #[test]
diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/src/vmm/src/devices/virtio/balloon/device.rs
@@ -949,6 +949,16 @@ impl VirtioDevice for Balloon {
         self.device_state.is_activated()
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_evts.len());
+        for evt in &self.queue_evts {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
+
     fn kick(&mut self) {
         // If device is activated, kick the balloon queue(s) to make up for any
         // pending or in-flight epoll events we may have not captured in snapshot.
diff --git a/src/vmm/src/devices/virtio/block/device.rs b/src/vmm/src/devices/virtio/block/device.rs
@@ -209,6 +209,13 @@ impl VirtioDevice for Block {
         }
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        match self {
+            Self::Virtio(b) => b.reset(),
+            Self::VhostUser(b) => b.reset(),
+        }
+    }
+
     fn kick(&mut self) {
         // If device is activated, kick the block queue(s) to make up for any
         // pending or in-flight epoll events we may have not captured in
diff --git a/src/vmm/src/devices/virtio/block/vhost_user/device.rs b/src/vmm/src/devices/virtio/block/vhost_user/device.rs
@@ -373,6 +373,16 @@ impl<T: VhostUserHandleBackend + Send + 'static> VirtioDevice for VhostUserBlock
     fn is_activated(&self) -> bool {
         self.device_state.is_activated()
     }
+
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_evts.len());
+        for evt in &self.queue_evts {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
 }
 
 #[cfg(test)]
diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs
@@ -671,6 +671,16 @@ impl VirtioDevice for VirtioBlock {
     fn is_activated(&self) -> bool {
         self.device_state.is_activated()
     }
+
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_evts.len());
+        for evt in &self.queue_evts {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
 }
 
 impl Drop for VirtioBlock {
diff --git a/src/vmm/src/devices/virtio/device.rs b/src/vmm/src/devices/virtio/device.rs
@@ -155,9 +155,7 @@ pub trait VirtioDevice: AsAny + Send {
 
     /// Optionally deactivates this device and returns ownership of the guest memory map, interrupt
     /// event, and queue events.
-    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
-        None
-    }
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)>;
 
     /// Mark pages used by queues as dirty.
     fn mark_queue_memory_dirty(&mut self, mem: &GuestMemoryMmap) -> Result<(), QueueError> {
@@ -254,6 +252,10 @@ pub(crate) mod tests {
         fn is_activated(&self) -> bool {
             todo!()
         }
+
+        fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+            todo!()
+        }
     }
 
     #[test]
diff --git a/src/vmm/src/devices/virtio/mem/device.rs b/src/vmm/src/devices/virtio/mem/device.rs
@@ -658,6 +658,16 @@ impl VirtioDevice for VirtioMem {
         self.device_state.is_activated()
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_events.len());
+        for evt in &self.queue_events {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
+
     fn activate(
         &mut self,
         mem: GuestMemoryMmap,
diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs
@@ -1039,6 +1039,16 @@ impl VirtioDevice for Net {
         self.device_state.is_activated()
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_evts.len());
+        for evt in &self.queue_evts {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
+
     fn kick(&mut self) {
         // If device is activated, kick the net queue(s) to make up for any
         // pending or in-flight epoll events we may have not captured in snapshot.
diff --git a/src/vmm/src/devices/virtio/pmem/device.rs b/src/vmm/src/devices/virtio/pmem/device.rs
@@ -399,6 +399,16 @@ impl VirtioDevice for Pmem {
         self.device_state.is_activated()
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_events.len());
+        for evt in &self.queue_events {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
+
     fn kick(&mut self) {
         if self.is_activated() {
             info!("kick pmem {}.", self.config.id);
diff --git a/src/vmm/src/devices/virtio/rng/device.rs b/src/vmm/src/devices/virtio/rng/device.rs
@@ -296,6 +296,16 @@ impl VirtioDevice for Entropy {
         self.device_state.is_activated()
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_events.len());
+        for evt in &self.queue_events {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
+
     fn activate(
         &mut self,
         mem: GuestMemoryMmap,
diff --git a/src/vmm/src/devices/virtio/transport/mmio.rs b/src/vmm/src/devices/virtio/transport/mmio.rs
@@ -158,6 +158,15 @@ impl MmioTransport {
         }
     }
 
+    fn drain_eventfds(interrupt: &Arc<dyn VirtioInterrupt>, queue_evts: &mut [EventFd]) {
+        if let Some(evt) = interrupt.notifier(VirtioInterruptType::Config) {
+            evt.read();
+        }
+        for evt in queue_evts {
+            evt.read();
+        }
+    }
+
     /// Update device status according to the state machine defined by VirtIO Spec 1.0.
     /// Please refer to VirtIO Spec 1.0, section 2.1.1 and 3.1.1.
     ///
@@ -199,7 +208,13 @@ impl MmioTransport {
                 }
             }
             _ if (status & FAILED) != 0 => {
-                // TODO: notify backend driver to stop the device
+                let mut locked_device = self.device.lock().expect("Poisoned lock");
+                if locked_device.is_activated()
+                    && let Some((interrupt_evt, mut queue_evts)) = locked_device.reset()
+                {
+                    Self::drain_eventfds(&interrupt_evt, &mut queue_evts);
+                }
+
                 self.device_status |= FAILED;
             }
             _ if status == 0 => {
@@ -209,7 +224,9 @@ impl MmioTransport {
                         let mut device_status = self.device_status;
                         let reset_result = locked_device.reset();
                         match reset_result {
-                            Some((_interrupt_evt, mut _queue_evts)) => {}
+                            Some((interrupt_evt, mut queue_evts)) => {
+                                Self::drain_eventfds(&interrupt_evt, &mut queue_evts);
+                            }
                             None => {
                                 device_status |= FAILED;
                             }
@@ -588,6 +605,20 @@ pub(crate) mod tests {
         fn is_activated(&self) -> bool {
             self.device_activated
         }
+
+        fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+            if !self.device_activated {
+                return None;
+            }
+            let interrupt = self.interrupt_trigger.as_ref()?.clone();
+            let mut queue_evts = Vec::with_capacity(self.queue_evts.len());
+            for evt in &self.queue_evts {
+                queue_evts.push(evt.try_clone().ok()?);
+            }
+            self.device_activated = false;
+            self.interrupt_trigger = None;
+            Some((interrupt, queue_evts))
+        }
     }
 
     fn set_device_status(d: &mut MmioTransport, status: u32) {
@@ -1070,23 +1101,20 @@ pub(crate) mod tests {
             Arc::new(Mutex::new(DummyDevice::new())),
             false,
         );
-        let mut buf = [0; 4];
 
         assert!(!d.locked_device().is_activated());
         assert_eq!(d.device_status, 0);
         activate_device(&mut d);
 
-        // Marking device as FAILED should not affect device_activated state
-        write_le_u32(&mut buf[..], 0x8f);
-        d.write(0x0, 0x70, &buf[..]);
+        // Marking device as FAILED deactivates the device but keeps the FAILED bit set.
+        set_device_status(&mut d, 0x8f);
         assert_eq!(d.device_status, 0x8f);
-        assert!(d.locked_device().is_activated());
+        assert!(!d.locked_device().is_activated());
 
-        // Nothing happens when backend driver doesn't support reset
-        write_le_u32(&mut buf[..], 0x0);
-        d.write(0x0, 0x70, &buf[..]);
+        // Reset keeps the FAILED bit set.
+        set_device_status(&mut d, 0);
         assert_eq!(d.device_status, 0x8f);
-        assert!(d.locked_device().is_activated());
+        assert!(!d.locked_device().is_activated());
     }
 
     #[test]
diff --git a/src/vmm/src/devices/virtio/transport/mod.rs b/src/vmm/src/devices/virtio/transport/mod.rs
@@ -40,9 +40,7 @@ pub trait VirtioInterrupt: std::fmt::Debug + Send + Sync {
     }
 
     /// Get the `EventFd` (if any) that backs the underlying interrupt.
-    fn notifier(&self, _interrupt_type: VirtioInterruptType) -> Option<&EventFd> {
-        None
-    }
+    fn notifier(&self, interrupt_type: VirtioInterruptType) -> Option<&EventFd>;
 
     /// Get the current device interrupt status.
     fn status(&self) -> Arc<AtomicU32>;
diff --git a/src/vmm/src/devices/virtio/vsock/device.rs b/src/vmm/src/devices/virtio/vsock/device.rs
@@ -379,6 +379,16 @@ where
         self.device_state.is_activated()
     }
 
+    fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {
+        let interrupt = self.device_state.active_state()?.interrupt.clone();
+        let mut queue_evts = Vec::with_capacity(self.queue_events.len());
+        for evt in &self.queue_events {
+            queue_evts.push(evt.try_clone().ok()?);
+        }
+        self.device_state = DeviceState::Inactive;
+        Some((interrupt, queue_evts))
+    }
+
     fn kick(&mut self) {
         // Vsock has complicated protocol that isn't resilient to any packet loss,
         // so for Vsock we don't support connection persistence through snapshot.

Original file line number	Diff line number	Diff line change
`@@ -545,6 +545,10 @@ pub(crate) mod tests {`
`545`	`545`	`fn is_activated(&self) -> bool {`
`546`	`546`	`false`
`547`	`547`	`}`
	`548`	`+`
	`549`	`+ fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {`
	`550`	`+ None`
	`551`	`+ }`
`548`	`552`	`}`
`549`	`553`
`550`	`554`	`#[test]`
Original file line number	Diff line number	Diff line change
`@@ -209,6 +209,13 @@ impl VirtioDevice for Block {`
`209`	`209`	`}`
`210`	`210`	`}`
`211`	`211`
	`212`	`+ fn reset(&mut self) -> Option<(Arc<dyn VirtioInterrupt>, Vec<EventFd>)> {`
	`213`	`+ match self {`
	`214`	`+ Self::Virtio(b) => b.reset(),`
	`215`	`+ Self::VhostUser(b) => b.reset(),`
	`216`	`+ }`
	`217`	`+ }`
	`218`	`+`
`212`	`219`	`fn kick(&mut self) {`
`213`	`220`	`// If device is activated, kick the block queue(s) to make up for any`
`214`	`221`	`// pending or in-flight epoll events we may have not captured in`