refactor: set VirtIO interrupt during activation

The MMIO transport for VirtIO devices uses an `IrqTrigger` object as the
object that models the logic for sending interrupts from the device to
the guest. We create one such object for every VirtIO device when
creating it. The MMIO device manager associates this object with an IRQ
number and registers it with KVM.

This commit changes the timing of association of an `IrqTrigger` with a
VirtIO-mmio device. It only assigns such an object to the device during
its activation. We do this to prepare for supporting a PCI transport for
VirtIO devices. The cloud hypervisor implementation for these passes the
interrupt objects used by the device during activation, so we make this
change to have a uniform way to handle interrupts for both transports.
Functionally, nothing changes for MMIO devices, as before activation we
don't trigger any interrupts.

Signed-off-by: Babis Chalios <[email protected]>

Author: bchalios

src/vmm/src/builder.rs

@@ -44,7 +44,7 @@ use crate::devices::virtio::block::device::Block;
 use crate::devices::virtio::device::VirtioDevice;
 use crate::devices::virtio::net::Net;
 use crate::devices::virtio::rng::Entropy;
-use crate::devices::virtio::transport::mmio::MmioTransport;
+use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport};
 use crate::devices::virtio::vsock::{Vsock, VsockUnixBackend};
 #[cfg(feature = "gdb")]
 use crate::gdb;
@@ -597,8 +597,14 @@ fn attach_virtio_device<T: 'static + VirtioDevice + MutEventSubscriber + Debug>(
 ) -> Result<(), MmioError> {
     event_manager.add_subscriber(device.clone());
 
+    let interrupt = Arc::new(IrqTrigger::new()?);
     // The device mutex mustn't be locked here otherwise it will deadlock.
-    let device = MmioTransport::new(vmm.vm.guest_memory().clone(), device, is_vhost_user);
+    let device = MmioTransport::new(
+        vmm.vm.guest_memory().clone(),
+        interrupt,
+        device,
+        is_vhost_user,
+    );
     vmm.mmio_device_manager
         .register_mmio_virtio_for_boot(
             vmm.vm.fd(),

src/vmm/src/device_manager/mmio.rs

@@ -53,6 +53,8 @@ pub enum MmioError {
     InvalidDeviceType,
     /// {0}
     InternalDeviceError(String),
+    /// Could not create IRQ for MMIO device: {0}
+    CreateIrq(#[from] std::io::Error),
     /// Invalid MMIO IRQ configuration.
     InvalidIrqConfig,
     /// Failed to register IO event: {0}
@@ -205,7 +207,7 @@ impl MMIODeviceManager {
                 vm.register_ioevent(queue_evt, &io_addr, u32::try_from(i).unwrap())
                     .map_err(MmioError::RegisterIoEvent)?;
             }
-            vm.register_irqfd(&locked_device.interrupt_trigger().irq_evt, irq.get())
+            vm.register_irqfd(&mmio_device.interrupt.irq_evt, irq.get())
                 .map_err(MmioError::RegisterIrqFd)?;
         }
 
@@ -549,7 +551,8 @@ mod tests {
             cmdline: &mut kernel_cmdline::Cmdline,
             dev_id: &str,
         ) -> Result<u64, MmioError> {
-            let mmio_device = MmioTransport::new(guest_mem, device, false);
+            let interrupt = Arc::new(IrqTrigger::new().unwrap());
+            let mmio_device = MmioTransport::new(guest_mem, interrupt, device, false);
             let device_info = self.register_mmio_virtio_for_boot(
                 vm,
                 resource_allocator,
@@ -576,7 +579,7 @@ mod tests {
         dummy: u32,
         queues: Vec<Queue>,
         queue_evts: [EventFd; 1],
-        interrupt_trigger: IrqTrigger,
+        interrupt_trigger: Option<Arc<IrqTrigger>>,
     }
 
     impl DummyDevice {
@@ -585,7 +588,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_trigger: IrqTrigger::new().expect("cannot create eventFD"),
+                interrupt_trigger: None,
             }
         }
     }
@@ -617,8 +620,11 @@ mod tests {
             &self.queue_evts
         }
 
-        fn interrupt_trigger(&self) -> &IrqTrigger {
-            &self.interrupt_trigger
+        fn interrupt_trigger(&self) -> Arc<IrqTrigger> {
+            self.interrupt_trigger
+                .as_ref()
+                .expect("Device is not activated")
+                .clone()
         }
 
         fn ack_features_by_page(&mut self, page: u32, value: u32) {
@@ -636,7 +642,11 @@ mod tests {
             let _ = data;
         }
 
-        fn activate(&mut self, _: GuestMemoryMmap) -> Result<(), ActivateError> {
+        fn activate(
+            &mut self,
+            _: GuestMemoryMmap,
+            _: Arc<IrqTrigger>,
+        ) -> Result<(), ActivateError> {
             Ok(())
         }
 

src/vmm/src/device_manager/persist.rs

@@ -34,7 +34,7 @@ use crate::devices::virtio::rng::Entropy;
 use crate::devices::virtio::rng::persist::{
     EntropyConstructorArgs, EntropyPersistError as EntropyError, EntropyState,
 };
-use crate::devices::virtio::transport::mmio::MmioTransport;
+use crate::devices::virtio::transport::mmio::{IrqTrigger, MmioTransport};
 use crate::devices::virtio::vsock::persist::{
     VsockConstructorArgs, VsockState, VsockUdsConstructorArgs,
 };
@@ -476,8 +476,22 @@ impl<'a> Persist<'a> for MMIODeviceManager {
                                   device_info: &MMIODeviceInfo,
                                   event_manager: &mut EventManager|
          -> Result<(), Self::Error> {
+            // If the device was already activated, an interrupt was created and assigned to it. In
+            // that case grab a reference to it and pass it to the transport state. Otherwise,
+            // create a fresh one. Like this both the transport and the device will hold a
+            // reference to the same interrupt object, at all times.
+            let interrupt = {
+                let locked_dev = device.lock().expect("Poisoned lock");
+                if locked_dev.is_activated() {
+                    locked_dev.interrupt_trigger()
+                } else {
+                    Arc::new(IrqTrigger::new().expect("Could not create IRQ for MMIO device"))
+                }
+            };
+
             let restore_args = MmioTransportConstructorArgs {
                 mem: mem.clone(),
+                interrupt,
                 device,
                 is_vhost_user,
             };

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

@@ -1,7 +1,7 @@
 // Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::fmt;
+use std::sync::Arc;
 use std::time::Duration;
 
 use log::error;
@@ -149,6 +149,7 @@ impl BalloonStats {
     }
 }
 
+#[derive(Debug)]
 /// Virtio balloon device.
 pub struct Balloon {
     // Virtio fields.
@@ -161,7 +162,6 @@ pub struct Balloon {
     pub(crate) queues: Vec<Queue>,
     pub(crate) queue_evts: [EventFd; BALLOON_NUM_QUEUES],
     pub(crate) device_state: DeviceState,
-    pub(crate) irq_trigger: IrqTrigger,
 
     // Implementation specific fields.
     pub(crate) restored_from_file: bool,
@@ -175,29 +175,6 @@ pub struct Balloon {
     pub(crate) pfn_buffer: [u32; MAX_PAGE_COMPACT_BUFFER],
 }
 
-// TODO Use `#[derive(Debug)]` when a new release of
-// [rust-timerfd](https://github.com/main--/rust-timerfd) is published that includes
-// https://github.com/main--/rust-timerfd/pull/12.
-impl fmt::Debug for Balloon {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.debug_struct("Balloon")
-            .field("avail_features", &self.avail_features)
-            .field("acked_features", &self.acked_features)
-            .field("config_space", &self.config_space)
-            .field("activate_evt", &self.activate_evt)
-            .field("queues", &self.queues)
-            .field("queue_evts", &self.queue_evts)
-            .field("device_state", &self.device_state)
-            .field("irq_trigger", &self.irq_trigger)
-            .field("restored_from_file", &self.restored_from_file)
-            .field("stats_polling_interval_s", &self.stats_polling_interval_s)
-            .field("stats_desc_index", &self.stats_desc_index)
-            .field("latest_stats", &self.latest_stats)
-            .field("pfn_buffer", &self.pfn_buffer)
-            .finish()
-    }
-}
-
 impl Balloon {
     /// Instantiate a new balloon device.
     pub fn new(
@@ -242,7 +219,6 @@ impl Balloon {
             },
             queue_evts,
             queues,
-            irq_trigger: IrqTrigger::new().map_err(BalloonError::EventFd)?,
             device_state: DeviceState::Inactive,
             activate_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(BalloonError::EventFd)?,
             restored_from_file,
@@ -282,7 +258,7 @@ impl Balloon {
 
     pub(crate) fn process_inflate(&mut self) -> Result<(), BalloonError> {
         // This is safe since we checked in the event handler that the device is activated.
-        let mem = self.device_state.mem().unwrap();
+        let (mem, _) = self.device_state.active_state().unwrap();
         METRICS.inflate_count.inc();
 
         let queue = &mut self.queues[INFLATE_INDEX];
@@ -389,7 +365,7 @@ impl Balloon {
 
     pub(crate) fn process_stats_queue(&mut self) -> Result<(), BalloonError> {
         // This is safe since we checked in the event handler that the device is activated.
-        let mem = self.device_state.mem().unwrap();
+        let (mem, _) = self.device_state.active_state().unwrap();
         METRICS.stats_updates_count.inc();
 
         while let Some(head) = self.queues[STATS_INDEX].pop() {
@@ -426,10 +402,12 @@ impl Balloon {
     }
 
     pub(crate) fn signal_used_queue(&self) -> Result<(), BalloonError> {
-        self.irq_trigger.trigger_irq(IrqType::Vring).map_err(|err| {
-            METRICS.event_fails.inc();
-            BalloonError::InterruptError(err)
-        })
+        self.interrupt_trigger()
+            .trigger_irq(IrqType::Vring)
+            .map_err(|err| {
+                METRICS.event_fails.inc();
+                BalloonError::InterruptError(err)
+            })
     }
 
     /// Process device virtio queue(s).
@@ -461,7 +439,7 @@ impl Balloon {
     pub fn update_size(&mut self, amount_mib: u32) -> Result<(), BalloonError> {
         if self.is_activated() {
             self.config_space.num_pages = mib_to_pages(amount_mib)?;
-            self.irq_trigger
+            self.interrupt_trigger()
                 .trigger_irq(IrqType::Config)
                 .map_err(BalloonError::InterruptError)
         } else {
@@ -573,8 +551,11 @@ impl VirtioDevice for Balloon {
         &self.queue_evts
     }
 
-    fn interrupt_trigger(&self) -> &IrqTrigger {
-        &self.irq_trigger
+    fn interrupt_trigger(&self) -> Arc<IrqTrigger> {
+        self.device_state
+            .active_state()
+            .expect("Device is not activated")
+            .1
     }
 
     fn read_config(&self, offset: u64, data: &mut [u8]) {
@@ -601,13 +582,17 @@ impl VirtioDevice for Balloon {
         dst.copy_from_slice(data);
     }
 
-    fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> {
+    fn activate(
+        &mut self,
+        mem: GuestMemoryMmap,
+        interrupt: Arc<IrqTrigger>,
+    ) -> Result<(), ActivateError> {
         for q in self.queues.iter_mut() {
             q.initialize(&mem)
                 .map_err(ActivateError::QueueMemoryError)?;
         }
 
-        self.device_state = DeviceState::Activated(mem);
+        self.device_state = DeviceState::Activated((mem, interrupt));
         if self.activate_evt.write(1).is_err() {
             METRICS.activate_fails.inc();
             self.device_state = DeviceState::Inactive;
@@ -813,10 +798,11 @@ pub(crate) mod tests {
     fn test_invalid_request() {
         let mut balloon = Balloon::new(0, true, 0, false).unwrap();
         let mem = default_mem();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
         // Only initialize the inflate queue to demonstrate invalid request handling.
         let infq = VirtQueue::new(GuestAddress(0), &mem, 16);
         balloon.set_queue(INFLATE_INDEX, infq.create_queue());
-        balloon.activate(mem.clone()).unwrap();
+        balloon.activate(mem.clone(), interrupt).unwrap();
 
         // Fill the second page with non-zero bytes.
         for i in 0..0x1000 {
@@ -872,9 +858,10 @@ pub(crate) mod tests {
     fn test_inflate() {
         let mut balloon = Balloon::new(0, true, 0, false).unwrap();
         let mem = default_mem();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
         let infq = VirtQueue::new(GuestAddress(0), &mem, 16);
         balloon.set_queue(INFLATE_INDEX, infq.create_queue());
-        balloon.activate(mem.clone()).unwrap();
+        balloon.activate(mem.clone(), interrupt).unwrap();
 
         // Fill the third page with non-zero bytes.
         for i in 0..0x1000 {
@@ -942,9 +929,10 @@ pub(crate) mod tests {
     fn test_deflate() {
         let mut balloon = Balloon::new(0, true, 0, false).unwrap();
         let mem = default_mem();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
         let defq = VirtQueue::new(GuestAddress(0), &mem, 16);
         balloon.set_queue(DEFLATE_INDEX, defq.create_queue());
-        balloon.activate(mem.clone()).unwrap();
+        balloon.activate(mem.clone(), interrupt).unwrap();
 
         let page_addr = 0x10;
 
@@ -990,9 +978,10 @@ pub(crate) mod tests {
     fn test_stats() {
         let mut balloon = Balloon::new(0, true, 1, false).unwrap();
         let mem = default_mem();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
         let statsq = VirtQueue::new(GuestAddress(0), &mem, 16);
         balloon.set_queue(STATS_INDEX, statsq.create_queue());
-        balloon.activate(mem.clone()).unwrap();
+        balloon.activate(mem.clone(), interrupt).unwrap();
 
         let page_addr = 0x100;
 
@@ -1068,7 +1057,7 @@ pub(crate) mod tests {
                 assert!(balloon.stats_desc_index.is_some());
                 balloon.process_stats_timer_event().unwrap();
                 assert!(balloon.stats_desc_index.is_none());
-                assert!(balloon.irq_trigger.has_pending_irq(IrqType::Vring));
+                assert!(balloon.interrupt_trigger().has_pending_irq(IrqType::Vring));
             });
         }
     }
@@ -1077,21 +1066,23 @@ pub(crate) mod tests {
     fn test_process_balloon_queues() {
         let mut balloon = Balloon::new(0x10, true, 0, false).unwrap();
         let mem = default_mem();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
         let infq = VirtQueue::new(GuestAddress(0), &mem, 16);
         let defq = VirtQueue::new(GuestAddress(0), &mem, 16);
 
         balloon.set_queue(INFLATE_INDEX, infq.create_queue());
         balloon.set_queue(DEFLATE_INDEX, defq.create_queue());
 
-        balloon.activate(mem).unwrap();
+        balloon.activate(mem, interrupt).unwrap();
         balloon.process_virtio_queues()
     }
 
     #[test]
     fn test_update_stats_interval() {
         let mut balloon = Balloon::new(0, true, 0, false).unwrap();
         let mem = default_mem();
-        balloon.activate(mem).unwrap();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
+        balloon.activate(mem, interrupt).unwrap();
         assert_eq!(
             format!("{:?}", balloon.update_stats_polling_interval(1)),
             "Err(StatisticsStateChange)"
@@ -1100,7 +1091,8 @@ pub(crate) mod tests {
 
         let mut balloon = Balloon::new(0, true, 1, false).unwrap();
         let mem = default_mem();
-        balloon.activate(mem).unwrap();
+        let interrupt = Arc::new(IrqTrigger::new().unwrap());
+        balloon.activate(mem, interrupt).unwrap();
         assert_eq!(
             format!("{:?}", balloon.update_stats_polling_interval(0)),
             "Err(StatisticsStateChange)"
@@ -1120,7 +1112,8 @@ pub(crate) mod tests {
     fn test_num_pages() {
         let mut balloon = Balloon::new(0, true, 0, false).unwrap();
         // Switch the state to active.
-        balloon.device_state = DeviceState::Activated(single_region_mem(0x1));
+        balloon.device_state =
+            DeviceState::Activated((single_region_mem(0x1), Arc::new(IrqTrigger::new().unwrap())));
 
         assert_eq!(balloon.num_pages(), 0);
         assert_eq!(balloon.actual_pages(), 0);