refactor: Simplify VirtioDevice trait by combining methods

Combine the `interrupt_evt` and `interrupt_status` methods into a single method `interrupt_trigger` that returns a `IrqTrigger` reference (which essentially combines the two objects originally returned by the status and evt methods). The advantage to this is that `IrqTrigger` exposes a `trigger_irq` method, which I'd like to use in the next commit. Signed-off-by: Patrick Roy <[email protected]>
roypat · Jul 3, 2024 · 65b927e · 65b927e
1 parent dc17a23
commit 65b927e
Show file tree

Hide file tree

Showing 10 changed files with 39 additions and 93 deletions.
diff --git a/src/vmm/src/device_manager/mmio.rs b/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) {

diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/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]) {

diff --git a/src/vmm/src/devices/virtio/block/device.rs b/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 @@ impl VirtioDevice for Block {
         }
     }
 
-    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,
         }
     }
 

diff --git a/src/vmm/src/devices/virtio/block/vhost_user/device.rs b/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]) {

diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/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]) {

diff --git a/src/vmm/src/devices/virtio/device.rs b/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!()
         }
 

diff --git a/src/vmm/src/devices/virtio/mmio.rs b/src/vmm/src/devices/virtio/mmio.rs
@@ -363,6 +363,7 @@ pub(crate) mod tests {
     use utils::u64_to_usize;
 
     use super::*;
+    use crate::devices::virtio::device::IrqTrigger;
     use crate::devices::virtio::ActivateError;
     use crate::utilities::test_utils::single_region_mem;
     use crate::vstate::memory::GuestMemoryMmap;
@@ -371,8 +372,7 @@ 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,
@@ -384,8 +384,7 @@ pub(crate) mod tests {
             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(),
@@ -430,12 +429,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]) {

diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs
@@ -9,7 +9,6 @@
 use std::io::Read;
 use std::mem;
 use std::net::Ipv4Addr;
-use std::sync::atomic::AtomicU32;
 use std::sync::{Arc, Mutex};
 
 use libc::EAGAIN;
@@ -823,14 +822,9 @@ impl VirtioDevice for Net {
         &self.queue_evts
     }
 
-    fn interrupt_evt(&self) -> &EventFd {
-        &self.irq_trigger.irq_evt
+    fn interrupt_trigger(&self) -> &IrqTrigger {
+        &self.irq_trigger
     }
-
-    fn interrupt_status(&self) -> Arc<AtomicU32> {
-        self.irq_trigger.irq_status.clone()
-    }
-
     fn read_config(&self, offset: u64, data: &mut [u8]) {
         if let Some(config_space_bytes) = self.config_space.as_slice().get(u64_to_usize(offset)..) {
             let len = config_space_bytes.len().min(data.len());

diff --git a/src/vmm/src/devices/virtio/rng/device.rs b/src/vmm/src/devices/virtio/rng/device.rs
@@ -259,12 +259,8 @@ impl VirtioDevice for Entropy {
         &self.queue_events
     }
 
-    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 avail_features(&self) -> u64 {

diff --git a/src/vmm/src/devices/virtio/vsock/device.rs b/src/vmm/src/devices/virtio/vsock/device.rs
@@ -6,8 +6,8 @@
 // found in the THIRD-PARTY file.
 
 use std::fmt::Debug;
+
 /// This is the `VirtioDevice` implementation for our vsock device. It handles the virtio-level
-/// device logic: feature negociation, device configuration, and device activation.
 ///
 /// We aim to conform to the VirtIO v1.1 spec:
 /// https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.html
@@ -20,9 +20,6 @@ use std::fmt::Debug;
 /// - a TX queue FD;
 /// - an event queue FD; and
 /// - a backend FD.
-use std::sync::atomic::AtomicU32;
-use std::sync::Arc;
-
 use log::{error, warn};
 use utils::byte_order;
 use utils::eventfd::EventFd;
@@ -290,12 +287,8 @@ where
         &self.queue_events
     }
 
-    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]) {