fix: do not panic if virtio device activation return Err(...)

src/vmm/src/device_manager/mmio.rs

@@ -193,8 +193,11 @@ impl MMIODeviceManager {
                 vm.register_ioevent(queue_evt, &io_addr, u32::try_from(i).unwrap())
                     .map_err(MmioError::RegisterIoEvent)?;
             }
-            vm.register_irqfd(locked_device.interrupt_evt(), device_info.irqs[0])
-                .map_err(MmioError::RegisterIrqFd)?;
+            vm.register_irqfd(
+                &locked_device.interrupt_trigger().irq_evt,
+                device_info.irqs[0],
+            )
+            .map_err(MmioError::RegisterIrqFd)?;
         }
 
         self.register_mmio_device(
@@ -513,13 +516,13 @@ impl DeviceInfoForFDT for MMIODeviceInfo {
 
 #[cfg(test)]
 mod tests {
-    use std::sync::atomic::AtomicU32;
+
     use std::sync::Arc;
 
     use utils::eventfd::EventFd;
 
     use super::*;
-    use crate::devices::virtio::device::VirtioDevice;
+    use crate::devices::virtio::device::{IrqTrigger, VirtioDevice};
     use crate::devices::virtio::queue::Queue;
     use crate::devices::virtio::ActivateError;
     use crate::utilities::test_utils::multi_region_mem;
@@ -566,7 +569,7 @@ mod tests {
         dummy: u32,
         queues: Vec<Queue>,
         queue_evts: [EventFd; 1],
-        interrupt_evt: EventFd,
+        interrupt_trigger: IrqTrigger,
     }
 
     impl DummyDevice {
@@ -575,7 +578,7 @@ mod tests {
                 dummy: 0,
                 queues: QUEUE_SIZES.iter().map(|&s| Queue::new(s)).collect(),
                 queue_evts: [EventFd::new(libc::EFD_NONBLOCK).expect("cannot create eventFD")],
-                interrupt_evt: EventFd::new(libc::EFD_NONBLOCK).expect("cannot create eventFD"),
+                interrupt_trigger: IrqTrigger::new().expect("cannot create eventFD"),
             }
         }
     }
@@ -607,12 +610,8 @@ mod tests {
             &self.queue_evts
         }
 
-        fn interrupt_evt(&self) -> &EventFd {
-            &self.interrupt_evt
-        }
-
-        fn interrupt_status(&self) -> Arc<AtomicU32> {
-            Arc::new(AtomicU32::new(0))
+        fn interrupt_trigger(&self) -> &IrqTrigger {
+            &self.interrupt_trigger
         }
 
         fn ack_features_by_page(&mut self, page: u32, value: u32) {

src/vmm/src/devices/virtio/balloon/device.rs

@@ -2,8 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use std::fmt;
-use std::sync::atomic::AtomicU32;
-use std::sync::Arc;
 use std::time::Duration;
 
 use log::error;
@@ -584,12 +582,8 @@ impl VirtioDevice for Balloon {
         &self.queue_evts
     }
 
-    fn interrupt_evt(&self) -> &EventFd {
-        &self.irq_trigger.irq_evt
-    }
-
-    fn interrupt_status(&self) -> Arc<AtomicU32> {
-        self.irq_trigger.irq_status.clone()
+    fn interrupt_trigger(&self) -> &IrqTrigger {
+        &self.irq_trigger
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {

src/vmm/src/devices/virtio/block/device.rs

@@ -1,17 +1,14 @@
 // Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::sync::atomic::AtomicU32;
-use std::sync::Arc;
-
 use event_manager::{EventOps, Events, MutEventSubscriber};
 use utils::eventfd::EventFd;
 
 use super::persist::{BlockConstructorArgs, BlockState};
 use super::vhost_user::device::{VhostUserBlock, VhostUserBlockConfig};
 use super::virtio::device::{VirtioBlock, VirtioBlockConfig};
 use super::BlockError;
-use crate::devices::virtio::device::VirtioDevice;
+use crate::devices::virtio::device::{IrqTrigger, VirtioDevice};
 use crate::devices::virtio::queue::Queue;
 use crate::devices::virtio::{ActivateError, TYPE_BLOCK};
 use crate::rate_limiter::BucketUpdate;
@@ -176,17 +173,10 @@
         }
     }
 
-    fn interrupt_evt(&self) -> &EventFd {
-        match self {
-            Self::Virtio(b) => &b.irq_trigger.irq_evt,
-            Self::VhostUser(b) => &b.irq_trigger.irq_evt,
-        }
-    }
-
-    fn interrupt_status(&self) -> Arc<AtomicU32> {
+    fn interrupt_trigger(&self) -> &IrqTrigger {
         match self {
-            Self::Virtio(b) => b.irq_trigger.irq_status.clone(),
-            Self::VhostUser(b) => b.irq_trigger.irq_status.clone(),
+            Self::Virtio(b) => &b.irq_trigger,
+            Self::VhostUser(b) => &b.irq_trigger,
         }
     }
 

src/vmm/src/devices/virtio/block/vhost_user/device.rs

@@ -4,7 +4,6 @@
 // Portions Copyright 2019 Intel Corporation. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::sync::atomic::AtomicU32;
 use std::sync::Arc;
 
 use log::error;
@@ -311,13 +310,8 @@ impl<T: VhostUserHandleBackend + Send + 'static> VirtioDevice for VhostUserBlock
         &self.queue_evts
     }
 
-    fn interrupt_evt(&self) -> &EventFd {
-        &self.irq_trigger.irq_evt
-    }
-
-    /// Returns the current device interrupt status.
-    fn interrupt_status(&self) -> Arc<AtomicU32> {
-        self.irq_trigger.irq_status.clone()
+    fn interrupt_trigger(&self) -> &IrqTrigger {
+        &self.irq_trigger
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {

src/vmm/src/devices/virtio/block/virtio/device.rs

@@ -11,7 +11,6 @@ use std::fs::{File, OpenOptions};
 use std::io::{Seek, SeekFrom, Write};
 use std::os::linux::fs::MetadataExt;
 use std::path::PathBuf;
-use std::sync::atomic::AtomicU32;
 use std::sync::Arc;
 
 use block_io::FileEngine;
@@ -609,13 +608,8 @@ impl VirtioDevice for VirtioBlock {
         &self.queue_evts
     }
 
-    fn interrupt_evt(&self) -> &EventFd {
-        &self.irq_trigger.irq_evt
-    }
-
-    /// Returns the current device interrupt status.
-    fn interrupt_status(&self) -> Arc<AtomicU32> {
-        self.irq_trigger.irq_status.clone()
+    fn interrupt_trigger(&self) -> &IrqTrigger {
+        &self.irq_trigger
     }
 
     fn read_config(&self, offset: u64, mut data: &mut [u8]) {

src/vmm/src/devices/virtio/device.rs

@@ -119,11 +119,12 @@ pub trait VirtioDevice: AsAny + Send {
     /// Returns the device queues event fds.
     fn queue_events(&self) -> &[EventFd];
 
-    /// Returns the device interrupt eventfd.
-    fn interrupt_evt(&self) -> &EventFd;
-
     /// Returns the current device interrupt status.
-    fn interrupt_status(&self) -> Arc<AtomicU32>;
+    fn interrupt_status(&self) -> Arc<AtomicU32> {
+        Arc::clone(&self.interrupt_trigger().irq_status)
+    }
+
+    fn interrupt_trigger(&self) -> &IrqTrigger;
 
     /// The set of feature bits shifted by `page * 32`.
     fn avail_features_by_page(&self, page: u32) -> u32 {
@@ -266,11 +267,7 @@ pub(crate) mod tests {
             todo!()
         }
 
-        fn interrupt_evt(&self) -> &EventFd {
-            todo!()
-        }
-
-        fn interrupt_status(&self) -> Arc<AtomicU32> {
+        fn interrupt_trigger(&self) -> &IrqTrigger {
             todo!()
         }
 

src/vmm/src/devices/virtio/mmio.rs

@@ -11,7 +11,7 @@ use std::sync::{Arc, Mutex, MutexGuard};
 
 use utils::byte_order;
 
-use crate::devices::virtio::device::VirtioDevice;
+use crate::devices::virtio::device::{IrqType, VirtioDevice};
 use crate::devices::virtio::device_status;
 use crate::devices::virtio::queue::Queue;
 use crate::logger::warn;
@@ -186,10 +186,18 @@ impl MmioTransport {
             DRIVER_OK if self.device_status == (ACKNOWLEDGE | DRIVER | FEATURES_OK) => {
                 self.device_status = status;
                 let device_activated = self.locked_device().is_activated();
-                if !device_activated && self.are_queues_valid() {
-                    self.locked_device()
-                        .activate(self.mem.clone())
-                        .expect("Failed to activate device");
+                if !device_activated
+                    && self.are_queues_valid()
+                    && self.locked_device().activate(self.mem.clone()).is_err()
+                {
+                    self.device_status |= DEVICE_NEEDS_RESET;
+
+                    // Section 2.1.2 of the specification states that we need to send a device
+                    // configuration change interrupt
+                    let _ = self
+                        .locked_device()
+                        .interrupt_trigger()
+                        .trigger_irq(IrqType::Config);
                 }
             }
             _ if (status & FAILED) != 0 => {
@@ -306,7 +314,9 @@ impl MmioTransport {
                     0x20 => {
                         if self.check_device_status(
                             device_status::DRIVER,
-                            device_status::FEATURES_OK | device_status::FAILED,
+                            device_status::FEATURES_OK
+                                | device_status::FAILED
+                                | device_status::DEVICE_NEEDS_RESET,
                         ) {
                             self.locked_device()
                                 .ack_features_by_page(self.acked_features_select, v);
@@ -339,7 +349,10 @@ impl MmioTransport {
                 }
             }
             0x100..=0xfff => {
-                if self.check_device_status(device_status::DRIVER, device_status::FAILED) {
+                if self.check_device_status(
+                    device_status::DRIVER,
+                    device_status::FAILED | device_status::DEVICE_NEEDS_RESET,
+                ) {
                     self.locked_device().write_config(offset - 0x100, data)
                 } else {
                     warn!("can not write to device config data area before driver is ready");
@@ -363,6 +376,8 @@ pub(crate) mod tests {
     use utils::u64_to_usize;
 
     use super::*;
+    use crate::devices::virtio::device::IrqTrigger;
+    use crate::devices::virtio::device_status::DEVICE_NEEDS_RESET;
     use crate::devices::virtio::ActivateError;
     use crate::utilities::test_utils::single_region_mem;
     use crate::vstate::memory::GuestMemoryMmap;
@@ -371,28 +386,28 @@ pub(crate) mod tests {
     pub(crate) struct DummyDevice {
         acked_features: u64,
         avail_features: u64,
-        interrupt_evt: EventFd,
-        interrupt_status: Arc<AtomicU32>,
+        interrupt_trigger: IrqTrigger,
         queue_evts: Vec<EventFd>,
         queues: Vec<Queue>,
         device_activated: bool,
         config_bytes: [u8; 0xeff],
+        activate_should_error: bool,
     }
 
     impl DummyDevice {
         pub(crate) fn new() -> Self {
             DummyDevice {
                 acked_features: 0,
                 avail_features: 0,
-                interrupt_evt: EventFd::new(libc::EFD_NONBLOCK).unwrap(),
-                interrupt_status: Arc::new(AtomicU32::new(0)),
+                interrupt_trigger: IrqTrigger::new().unwrap(),
                 queue_evts: vec![
                     EventFd::new(libc::EFD_NONBLOCK).unwrap(),
                     EventFd::new(libc::EFD_NONBLOCK).unwrap(),
                 ],
                 queues: vec![Queue::new(16), Queue::new(32)],
                 device_activated: false,
                 config_bytes: [0; 0xeff],
+                activate_should_error: false,
             }
         }
 
@@ -430,12 +445,8 @@ pub(crate) mod tests {
             &self.queue_evts
         }
 
-        fn interrupt_evt(&self) -> &EventFd {
-            &self.interrupt_evt
-        }
-
-        fn interrupt_status(&self) -> Arc<AtomicU32> {
-            self.interrupt_status.clone()
+        fn interrupt_trigger(&self) -> &IrqTrigger {
+            &self.interrupt_trigger
         }
 
         fn read_config(&self, offset: u64, data: &mut [u8]) {
@@ -450,7 +461,11 @@ pub(crate) mod tests {
 
         fn activate(&mut self, _: GuestMemoryMmap) -> Result<(), ActivateError> {
             self.device_activated = true;
-            Ok(())
+            if self.activate_should_error {
+                Err(ActivateError::BadActivate)
+            } else {
+                Ok(())
+            }
         }
 
         fn is_activated(&self) -> bool {
@@ -823,6 +838,63 @@ pub(crate) mod tests {
         assert_eq!(read_le_u32(&buf[..]), 1);
     }
 
+    #[test]
+    fn test_bus_device_activate_failure() {
+        let m = single_region_mem(0x1000);
+        let device = DummyDevice {
+            activate_should_error: true,
+            ..DummyDevice::new()
+        };
+        let mut d = MmioTransport::new(m, Arc::new(Mutex::new(device)), false);
+
+        set_device_status(&mut d, device_status::ACKNOWLEDGE);
+        set_device_status(&mut d, device_status::ACKNOWLEDGE | device_status::DRIVER);
+        set_device_status(
+            &mut d,
+            device_status::ACKNOWLEDGE | device_status::DRIVER | device_status::FEATURES_OK,
+        );
+
+        let mut buf = [0; 4];
+        let queue_len = d.locked_device().queues().len();
+        for q in 0..queue_len {
+            d.queue_select = q.try_into().unwrap();
+            write_le_u32(&mut buf[..], 16);
+            d.bus_write(0x38, &buf[..]);
+            write_le_u32(&mut buf[..], 1);
+            d.bus_write(0x44, &buf[..]);
+        }
+        assert!(d.are_queues_valid());
+        assert_eq!(
+            d.locked_device().interrupt_status().load(Ordering::SeqCst),
+            0
+        );
+
+        set_device_status(
+            &mut d,
+            device_status::ACKNOWLEDGE
+                | device_status::DRIVER
+                | device_status::FEATURES_OK
+                | device_status::DRIVER_OK,
+        );
+
+        // Failure in activate results in `DEVICE_NEEDS_RESET` status being set
+        assert_ne!(d.device_status & DEVICE_NEEDS_RESET, 0);
+        // We injected an interrupt of type "configuration change"
+        assert_eq!(
+            d.locked_device().interrupt_status().load(Ordering::SeqCst),
+            VIRTIO_MMIO_INT_CONFIG
+        );
+        // We actually wrote to the eventfd
+        assert_eq!(
+            d.locked_device()
+                .interrupt_trigger()
+                .irq_evt
+                .read()
+                .unwrap(),
+            1
+        );
+    }
+
     fn activate_device(d: &mut MmioTransport) {
         set_device_status(d, device_status::ACKNOWLEDGE);
         set_device_status(d, device_status::ACKNOWLEDGE | device_status::DRIVER);