devices: block: activate through an activation event

acatangiu · acatangiu · commit ec157e18e405 · 2020-03-19T20:26:30.000+02:00
Postpone block external events registration to device activation time.
This makes the block device unaware of external events prior to
its activation.

During creation register a dedicated activation event which will notify
the device when it's time to register the other external events sources.

This activation event is unregistered after successful device
activation.

Signed-off-by: Adrian Catangiu &lt;acatan@amazon.com&gt;
diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs
@@ -21,7 +21,7 @@ use virtio_gen::virtio_blk::*;
 use vm_memory::{Bytes, GuestMemoryMmap};
 
 use super::{
-    super::{ActivateResult, Queue, VirtioDevice, TYPE_BLOCK, VIRTIO_MMIO_INT_VRING},
+    super::{ActivateResult, DeviceState, Queue, VirtioDevice, TYPE_BLOCK, VIRTIO_MMIO_INT_VRING},
     request::*,
     Error, CONFIG_SPACE_SIZE, QUEUE_SIZES, SECTOR_SHIFT, SECTOR_SIZE,
 };
@@ -88,15 +88,15 @@ pub struct Block {
     avail_features: u64,
     acked_features: u64,
     config_space: Vec<u8>,
+    pub(crate) activate_evt: EventFd,
 
     // Transport related fields.
     queues: Vec<Queue>,
     interrupt_status: Arc<AtomicUsize>,
     interrupt_evt: EventFd,
     pub(crate) queue_evts: [EventFd; 1],
-    mem: GuestMemoryMmap,
 
-    device_activated: bool,
+    pub(crate) device_state: DeviceState,
 
     // Implementation specific fields.
     pub(crate) rate_limiter: RateLimiter,
@@ -107,7 +107,6 @@ impl Block {
     ///
     /// The given file must be seekable and sizable.
     pub fn new(
-        mem: GuestMemoryMmap,
         mut disk_image: File,
         is_disk_read_only: bool,
         rate_limiter: RateLimiter,
@@ -132,12 +131,12 @@ impl Block {
             acked_features: 0u64,
             config_space: build_config_space(disk_size),
             rate_limiter,
-            mem,
             interrupt_status: Arc::new(AtomicUsize::new(0)),
             interrupt_evt: EventFd::new(libc::EFD_NONBLOCK)?,
             queue_evts,
             queues,
-            device_activated: false,
+            device_state: DeviceState::Inactive,
+            activate_evt: EventFd::new(libc::EFD_NONBLOCK)?,
         })
     }
 
@@ -161,11 +160,16 @@ impl Block {
     }
 
     pub(crate) fn process_queue(&mut self, queue_index: usize) -> bool {
+        let mem = match self.device_state {
+            DeviceState::Activated(ref mem) => mem,
+            // This should never happen, it's been already validated in the event handler.
+            DeviceState::Inactive => return false,
+        };
         let queue = &mut self.queues[queue_index];
         let mut used_any = false;
-        while let Some(head) = queue.pop(&self.mem) {
+        while let Some(head) = queue.pop(mem) {
             let len;
-            match Request::parse(&head, &self.mem) {
+            match Request::parse(&head, mem) {
                 Ok(request) => {
                     // If limiter.consume() fails it means there is no more TokenType::Ops
                     // budget and rate limiting is in effect.
@@ -196,7 +200,7 @@ impl Block {
                     let status = match request.execute(
                         &mut self.disk_image,
                         self.disk_nsectors,
-                        &self.mem,
+                        mem,
                         &self.disk_image_id,
                     ) {
                         Ok(l) => {
@@ -212,15 +216,15 @@ impl Block {
                     };
                     // We use unwrap because the request parsing process already checked that the
                     // status_addr was valid.
-                    self.mem.write_obj(status, request.status_addr).unwrap();
+                    mem.write_obj(status, request.status_addr).unwrap();
                 }
                 Err(e) => {
                     error!("Failed to parse available descriptor chain: {:?}", e);
                     METRICS.block.execute_fails.inc();
                     len = 0;
                 }
             }
-            queue.add_used(&self.mem, head.index, len);
+            queue.add_used(mem, head.index, len);
             used_any = true;
         }
 
@@ -314,11 +318,18 @@ impl VirtioDevice for Block {
     }
 
     fn is_activated(&self) -> bool {
-        self.device_activated
+        match self.device_state {
+            DeviceState::Inactive => false,
+            DeviceState::Activated(_) => true,
+        }
     }
 
-    fn activate(&mut self, _: GuestMemoryMmap) -> ActivateResult {
-        self.device_activated = true;
+    fn activate(&mut self, mem: GuestMemoryMmap) -> ActivateResult {
+        if self.activate_evt.write(1).is_err() {
+            error!("Block: Cannot write to activate_evt");
+            return Err(super::super::ActivateError::BadActivate);
+        }
+        self.device_state = DeviceState::Activated(mem);
         Ok(())
     }
 }
@@ -371,9 +382,11 @@ mod tests {
         // Rate limiting is enabled but with a high operation rate (10 million ops/s).
         let rate_limiter = RateLimiter::new(0, None, 0, 100_000, None, 10).unwrap();
 
-        let mem = GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap();
+        Block::new(block_file, true, rate_limiter).unwrap()
+    }
 
-        Block::new(mem, block_file, true, rate_limiter).unwrap()
+    fn default_mem() -> GuestMemoryMmap {
+        GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap()
     }
 
     fn initialize_virtqueue(vq: &VirtQueue) {
@@ -492,7 +505,7 @@ mod tests {
     #[test]
     fn test_invalid_request() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -517,7 +530,7 @@ mod tests {
     #[test]
     fn test_request_execute_failures() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -575,7 +588,7 @@ mod tests {
     #[test]
     fn test_unsupported_request_type() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -605,7 +618,7 @@ mod tests {
     #[test]
     fn test_read_write() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -664,7 +677,7 @@ mod tests {
     #[test]
     fn test_flush() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -707,7 +720,7 @@ mod tests {
     #[test]
     fn test_get_device_id() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -773,7 +786,7 @@ mod tests {
     #[test]
     fn test_bandwidth_rate_limiter() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
@@ -838,7 +851,7 @@ mod tests {
     #[test]
     fn test_ops_rate_limiter() {
         let mut block = default_block();
-        let mem = block.mem.clone();
+        let mem = default_mem();
         let vq = VirtQueue::new(GuestAddress(0), &mem, 16);
         block.set_queue(0, vq.create_queue());
         block.activate(mem.clone()).unwrap();
diff --git a/src/devices/src/virtio/block/event_handler.rs b/src/devices/src/virtio/block/event_handler.rs
@@ -6,19 +6,50 @@ use polly::event_manager::{EventManager, Subscriber};
 use utils::epoll::{EpollEvent, EventSet};
 
 use crate::virtio::block::device::Block;
-use crate::virtio::VirtioDevice;
+use crate::virtio::DeviceState;
 
-impl Subscriber for Block {
-    // Handle an event for queue or rate limiter.
-    fn process(&mut self, event: &EpollEvent, _: &mut EventManager) {
-        if !self.is_activated() {
-            warn!("The device is not yet activated. Events can not be handled.");
-            return;
-        }
+impl Block {
+    fn process_activate_event(&self, event_manager: &mut EventManager) {
+        // The subscriber must exist as we previously registered activate_evt via
+        // `interest_list()`.
+        let self_subscriber = event_manager
+            .subscriber(self.activate_evt.as_raw_fd())
+            .unwrap();
+
+        event_manager
+            .register(
+                self.queue_evts[0].as_raw_fd(),
+                EpollEvent::new(EventSet::IN, self.queue_evts[0].as_raw_fd() as u64),
+                self_subscriber.clone(),
+            )
+            .unwrap_or_else(|e| {
+                error!("Failed to register block queue with event manager: {:?}", e);
+            });
+
+        event_manager
+            .register(
+                self.rate_limiter.as_raw_fd(),
+                EpollEvent::new(EventSet::IN, self.rate_limiter.as_raw_fd() as u64),
+                self_subscriber.clone(),
+            )
+            .unwrap_or_else(|e| {
+                error!(
+                    "Failed to register block rate limiter with event manager: {:?}",
+                    e
+                );
+            });
 
-        let queue_evt = self.queue_evts[0].as_raw_fd();
-        let rate_limiter_evt = self.rate_limiter.as_raw_fd();
+        event_manager
+            .unregister(self.activate_evt.as_raw_fd())
+            .unwrap_or_else(|e| {
+                error!("Failed to unregister block activate evt: {:?}", e);
+            })
+    }
+}
 
+impl Subscriber for Block {
+    // Handle an event for queue or rate limiter.
+    fn process(&mut self, event: &EpollEvent, evmgr: &mut EventManager) {
         let source = event.fd();
         let event_set = event.event_set();
 
@@ -27,25 +58,38 @@ impl Subscriber for Block {
         let supported_events = EventSet::IN;
         if !supported_events.contains(event_set) {
             warn!(
-                "Received unknown event: {:?} from source: {:?}",
+                "Block: Received unknown event: {:?} from source: {:?}",
                 event_set, source
             );
             return;
         }
 
-        // Looks better than C style if/else if/else.
-        match source {
-            _ if queue_evt == source => self.process_queue_event(),
-            _ if rate_limiter_evt == source => self.process_rate_limiter_event(),
-            _ => warn!("Spurious event received: {:?}", source),
-        }
+        match self.device_state {
+            DeviceState::Activated(_) => {
+                let queue_evt = self.queue_evts[0].as_raw_fd();
+                let rate_limiter_evt = self.rate_limiter.as_raw_fd();
+                let activate_fd = self.activate_evt.as_raw_fd();
+
+                // Looks better than C style if/else if/else.
+                match source {
+                    _ if queue_evt == source => self.process_queue_event(),
+                    _ if rate_limiter_evt == source => self.process_rate_limiter_event(),
+                    _ if activate_fd == source => self.process_activate_event(evmgr),
+                    _ => warn!("Block: Spurious event received: {:?}", source),
+                }
+            }
+            DeviceState::Inactive => warn!(
+                "Block: The device is not yet activated. Spurious event received: {:?}",
+                source
+            ),
+        };
     }
 
     // Returns the rate_limiter and queue event fds.
     fn interest_list(&self) -> Vec<EpollEvent> {
-        vec![
-            EpollEvent::new(EventSet::IN, self.rate_limiter.as_raw_fd() as u64),
-            EpollEvent::new(EventSet::IN, self.queue_evts[0].as_raw_fd() as u64),
-        ]
+        vec![EpollEvent::new(
+            EventSet::IN,
+            self.activate_evt.as_raw_fd() as u64,
+        )]
     }
 }
diff --git a/src/devices/src/virtio/device.rs b/src/devices/src/virtio/device.rs
@@ -12,6 +12,13 @@ use crate::virtio::AsAny;
 use utils::eventfd::EventFd;
 use vm_memory::GuestMemoryMmap;
 
+/// Enum that indicates if a VirtioDevice is inactive or has been activated
+/// and memory attached to it.
+pub enum DeviceState {
+    Inactive,
+    Activated(GuestMemoryMmap),
+}
+
 /// Trait for virtio devices to be driven by a virtio transport.
 ///
 /// The lifecycle of a virtio device is to be moved to a virtio transport, which will then query the
diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs
@@ -708,7 +708,6 @@ fn attach_block_devices(
 
         let block_device = Arc::new(Mutex::new(
             devices::virtio::Block::new(
-                vmm.guest_memory.clone(),
                 block_file,
                 drive_config.is_read_only,
                 rate_limiter.unwrap_or_default(),
