diff --git a/src/devices/src/lib.rs b/src/devices/src/lib.rs index 04cbc2f6d84..134348596dc 100644 --- a/src/devices/src/lib.rs +++ b/src/devices/src/lib.rs @@ -47,4 +47,5 @@ pub enum Error { PayloadExpected, IoError(io::Error), NoAvailBuffers, + SpuriousEvent, } diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 6692e59bc13..2ff760c4a22 100644 --- 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, + pub(crate) activate_evt: EventFd, // Transport related fields. queues: Vec, interrupt_status: Arc, 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 => unreachable!(), + }; 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,7 +216,7 @@ 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); @@ -220,7 +224,7 @@ impl Block { len = 0; } } - queue.add_used(&self.mem, head.index, len); + queue.add_used(mem, head.index, len); used_any = true; } @@ -318,17 +322,24 @@ 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) -> 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(()) } } #[cfg(test)] -mod tests { +pub(crate) mod tests { use std::fs::metadata; use std::os::unix::io::AsRawFd; use std::thread; @@ -352,7 +363,7 @@ mod tests { } impl Block { - fn set_queue(&mut self, idx: usize, q: Queue) { + pub(crate) fn set_queue(&mut self, idx: usize, q: Queue) { self.queues[idx] = q; } @@ -366,7 +377,7 @@ mod tests { } /// Create a default Block instance to be used in tests. - fn default_block() -> Block { + pub fn default_block() -> Block { // Create backing file. let f = TempFile::new().unwrap(); let block_file = f.into_file(); @@ -375,12 +386,14 @@ 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() + pub fn default_mem() -> GuestMemoryMmap { + GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x10000)]).unwrap() } - fn initialize_virtqueue(vq: &VirtQueue) { + pub fn initialize_virtqueue(vq: &VirtQueue) { let request_type_desc: usize = 0; let data_desc: usize = 1; let status_desc: usize = 2; @@ -496,10 +509,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -521,10 +534,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -579,10 +592,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -609,10 +622,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -668,10 +681,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -711,10 +724,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -777,10 +790,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); @@ -842,10 +855,10 @@ 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().unwrap(); + block.activate(mem.clone()).unwrap(); initialize_virtqueue(&vq); let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); diff --git a/src/devices/src/virtio/block/event_handler.rs b/src/devices/src/virtio/block/event_handler.rs index 20876c83d7e..131938b1f30 100644 --- a/src/devices/src/virtio/block/event_handler.rs +++ b/src/devices/src/virtio/block/event_handler.rs @@ -8,17 +8,48 @@ use utils::epoll::{EpollEvent, EventSet}; use crate::virtio::block::device::Block; use crate::virtio::VirtioDevice; -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(); - let queue_evt = self.queue_evts[0].as_raw_fd(); - let rate_limiter_evt = self.rate_limiter.as_raw_fd(); + 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 + ); + }); + + 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,117 @@ 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), + if self.is_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), + } + } else { + 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 { - 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, + )] + } +} + +#[cfg(test)] +pub mod tests { + use std::sync::{Arc, Mutex}; + + use super::*; + use crate::virtio::block::device::tests::*; + use crate::virtio::queue::tests::*; + use virtio_gen::virtio_blk::*; + use vm_memory::{Bytes, GuestAddress}; + + #[test] + fn test_event_handler() { + let mut event_manager = EventManager::new().unwrap(); + let mut block = default_block(); + let mem = default_mem(); + let vq = VirtQueue::new(GuestAddress(0), &mem, 16); + block.set_queue(0, vq.create_queue()); + initialize_virtqueue(&vq); + + let block = Arc::new(Mutex::new(block)); + event_manager.add_subscriber(block.clone()).unwrap(); + + let request_type_addr = GuestAddress(vq.dtable[0].addr.get()); + let data_addr = GuestAddress(vq.dtable[1].addr.get()); + let status_addr = GuestAddress(vq.dtable[2].addr.get()); + + // Push a 'Write' operation. + { + mem.write_obj::(VIRTIO_BLK_T_OUT, request_type_addr) + .unwrap(); + // Make data read only, 8 bytes in len, and set the actual value to be written. + vq.dtable[1].flags.set(VIRTQ_DESC_F_NEXT); + vq.dtable[1].len.set(8); + mem.write_obj::(123_456_789, data_addr).unwrap(); + + // Trigger the queue event. + block.lock().unwrap().queue_evts[0].write(1).unwrap(); + } + + // EventManager should report no events since block has only registered + // its activation event so far (even though queue event is pending). + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 0); + + // Manually force a queue event and check it's ignored pre-activation. + { + let mut b = block.lock().unwrap(); + let raw_q_evt = b.queue_evts[0].as_raw_fd() as u64; + // Artificially push event. + b.process( + &EpollEvent::new(EventSet::IN, raw_q_evt), + &mut event_manager, + ); + // Validate there was no queue operation. + assert_eq!( + b.interrupt_evt().read().unwrap_err().kind(), + std::io::ErrorKind::WouldBlock + ); + assert_eq!(vq.used.idx.get(), 0); + } + + // Now activate the device. + block.lock().unwrap().activate(mem.clone()).unwrap(); + // Process the activate event. + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 1); + + // Handle the pending queue event through EventManager. + event_manager + .run_with_timeout(100) + .expect("Metrics event timeout or error."); + // Validate the queue operation finished successfully. + assert_eq!(block.lock().unwrap().interrupt_evt().read().unwrap(), 1); + + assert_eq!(vq.used.idx.get(), 1); + assert_eq!(vq.used.ring[0].get().id, 0); + assert_eq!(vq.used.ring[0].get().len, 0); + assert_eq!(mem.read_obj::(status_addr).unwrap(), VIRTIO_BLK_S_OK); } } diff --git a/src/devices/src/virtio/device.rs b/src/devices/src/virtio/device.rs index 446bc2ced05..8be8d57fc63 100644 --- a/src/devices/src/virtio/device.rs +++ b/src/devices/src/virtio/device.rs @@ -10,6 +10,14 @@ use std::sync::{atomic::AtomicUsize, Arc}; use super::{ActivateResult, Queue}; 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. /// @@ -87,7 +95,7 @@ pub trait VirtioDevice: AsAny + Send { fn write_config(&mut self, offset: u64, data: &[u8]); /// Performs the formal activation for a device, which can be verified also with `is_activated`. - fn activate(&mut self) -> ActivateResult; + fn activate(&mut self, mem: GuestMemoryMmap) -> ActivateResult; /// Checks if the resources of this device are activated. fn is_activated(&self) -> bool; diff --git a/src/devices/src/virtio/mmio.rs b/src/devices/src/virtio/mmio.rs index f41011ffae8..6463a1a34cd 100644 --- a/src/devices/src/virtio/mmio.rs +++ b/src/devices/src/virtio/mmio.rs @@ -179,7 +179,7 @@ impl MmioTransport { let device_activated = self.locked_device().is_activated(); if !device_activated && self.are_queues_valid() { self.locked_device() - .activate() + .activate(self.mem.clone()) .expect("Failed to activate device"); } } @@ -403,7 +403,7 @@ mod tests { self.acked_features = acked_features; } - fn activate(&mut self) -> ActivateResult { + fn activate(&mut self, _: GuestMemoryMmap) -> ActivateResult { self.device_activated = true; Ok(()) } diff --git a/src/devices/src/virtio/net/device.rs b/src/devices/src/virtio/net/device.rs index bfe0d8d7520..069c690e4ab 100644 --- a/src/devices/src/virtio/net/device.rs +++ b/src/devices/src/virtio/net/device.rs @@ -8,7 +8,9 @@ use crate::virtio::net::Error; use crate::virtio::net::Result; use crate::virtio::net::{MAX_BUFFER_SIZE, QUEUE_SIZE, QUEUE_SIZES, RX_INDEX, TX_INDEX}; -use crate::virtio::{ActivateResult, Queue, VirtioDevice, TYPE_NET, VIRTIO_MMIO_INT_VRING}; +use crate::virtio::{ + ActivateResult, DeviceState, Queue, VirtioDevice, TYPE_NET, VIRTIO_MMIO_INT_VRING, +}; use crate::{report_net_event_fail, Error as DeviceError}; use dumbo::ns::MmdsNetworkStack; use dumbo::{EthernetFrame, MacAddr, MAC_ADDR_LEN}; @@ -59,8 +61,6 @@ pub struct Net { avail_features: u64, acked_features: u64, - mem: GuestMemoryMmap, - pub(crate) queues: Vec, pub(crate) queue_evts: Vec, @@ -82,7 +82,8 @@ pub struct Net { config_space: Vec, guest_mac: Option, - device_activated: bool, + pub(crate) device_state: DeviceState, + pub(crate) activate_evt: EventFd, mmds_ns: Option, @@ -95,7 +96,6 @@ impl Net { pub fn new_with_tap( tap: Tap, guest_mac: Option<&MacAddr>, - mem: GuestMemoryMmap, rx_rate_limiter: RateLimiter, tx_rate_limiter: RateLimiter, allow_mmds_requests: bool, @@ -148,7 +148,6 @@ impl Net { tap, avail_features, acked_features: 0u64, - mem, queues, queue_evts, rx_rate_limiter, @@ -161,7 +160,8 @@ impl Net { tx_iovec: Vec::with_capacity(QUEUE_SIZE as usize), interrupt_status: Arc::new(AtomicUsize::new(0)), interrupt_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(Error::EventFd)?, - device_activated: false, + device_state: DeviceState::Inactive, + activate_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(Error::EventFd)?, config_space, guest_mac, mmds_ns, @@ -219,8 +219,13 @@ impl Net { // if a buffer was used, and false if the frame must be deferred until a buffer // is made available by the driver. fn rx_single_frame(&mut self) -> 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 => unreachable!(), + }; let rx_queue = &mut self.queues[RX_INDEX]; - let mut next_desc = rx_queue.pop(&self.mem); + let mut next_desc = rx_queue.pop(mem); if next_desc.is_none() { METRICS.net.no_rx_avail_buffer.inc(); return false; @@ -240,7 +245,7 @@ impl Net { let limit = cmp::min(write_count + desc.len as usize, self.rx_bytes_read); let source_slice = &self.rx_frame_buf[write_count..limit]; - let write_result = self.mem.write_slice(source_slice, desc.addr); + let write_result = mem.write_slice(source_slice, desc.addr); match write_result { Ok(()) => { @@ -270,7 +275,7 @@ impl Net { } } - rx_queue.add_used(&self.mem, head_index, write_count as u32); + rx_queue.add_used(mem, head_index, write_count as u32); // Mark that we have at least one pending packet and we need to interrupt the guest. self.rx_deferred_irqs = true; @@ -406,6 +411,12 @@ impl Net { } fn process_tx(&mut self) -> result::Result<(), DeviceError> { + 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 => unreachable!(), + }; + // The MMDS network stack works like a state machine, based on synchronous calls, and // without being added to any event loop. If any frame is accepted by the MMDS, we also // trigger a process_rx() which checks if there are any new frames to be sent, starting @@ -414,7 +425,7 @@ impl Net { let mut raise_irq = false; let tx_queue = &mut self.queues[TX_INDEX]; - while let Some(head) = tx_queue.pop(&self.mem) { + while let Some(head) = tx_queue.pop(mem) { // If limiter.consume() fails it means there is no more TokenType::Ops // budget and rate limiting is in effect. if !self.tx_rate_limiter.consume(1, TokenType::Ops) { @@ -459,7 +470,7 @@ impl Net { for (desc_addr, desc_len) in self.tx_iovec.drain(..) { let limit = cmp::min((read_count + desc_len) as usize, self.tx_frame_buf.len()); - let read_result = self.mem.read_slice( + let read_result = mem.read_slice( &mut self.tx_frame_buf[read_count..limit as usize], desc_addr, ); @@ -491,7 +502,7 @@ impl Net { process_rx_for_mmds = true; } - tx_queue.add_used(&self.mem, head_index, 0); + tx_queue.add_used(mem, head_index, 0); raise_irq = true; } @@ -542,8 +553,13 @@ impl Net { } pub fn process_tap_rx_event(&mut self) { + 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 => unreachable!(), + }; METRICS.net.rx_tap_event_count.inc(); - if self.queues[RX_INDEX].is_empty(&self.mem) { + if self.queues[RX_INDEX].is_empty(mem) { METRICS.net.no_rx_avail_buffer.inc(); return; } @@ -676,20 +692,28 @@ impl VirtioDevice for Net { } fn is_activated(&self) -> bool { - self.device_activated + match self.device_state { + DeviceState::Inactive => false, + DeviceState::Activated(_) => true, + } } - fn activate(&mut self) -> ActivateResult { - self.device_activated = true; + fn activate(&mut self, mem: GuestMemoryMmap) -> ActivateResult { + if self.activate_evt.write(1).is_err() { + error!("Net: Cannot write to activate_evt"); + return Err(super::super::ActivateError::BadActivate); + } + self.device_state = DeviceState::Activated(mem); Ok(()) } } #[cfg(test)] -mod tests { +pub(crate) mod tests { use std::net::Ipv4Addr; use std::os::unix::io::AsRawFd; use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Mutex; use std::time::Duration; use std::{io, mem, thread}; @@ -741,7 +765,7 @@ mod tests { } } - trait TestUtil { + pub(crate) trait TestUtil { fn default_net(test_mutators: TestMutators) -> Net; fn default_guest_mac() -> MacAddr; fn default_guest_memory() -> GuestMemoryMmap; @@ -762,7 +786,6 @@ mod tests { let mut net = Net::new_with_tap( tap, Some(&guest_mac), - Net::default_guest_memory(), RateLimiter::default(), RateLimiter::default(), true, @@ -812,7 +835,6 @@ mod tests { self.queues.clear(); self.queues.push(rxq); self.queues.push(txq); - self.activate().unwrap(); } } @@ -931,12 +953,13 @@ mod tests { } #[test] - fn test_event_handling() { + fn test_event_processing() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); let daddr = 0x2000; assert!(daddr > txq.end().0); @@ -1213,9 +1236,10 @@ mod tests { fn test_process_error_cases() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); // RX rate limiter events should error since the limiter is not blocked. // Validate that the event failed and failure was properly accounted for. @@ -1242,15 +1266,27 @@ mod tests { fn test_invalid_event() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); + + let net = Arc::new(Mutex::new(net)); + event_manager.add_subscriber(net.clone()).unwrap(); + + // Process the activate event. + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 1); + // Inject invalid event. let invalid_event = EpollEvent::new(EventSet::IN, 1000); check_metric_after_block!( &METRICS.net.event_fails, 1, - net.process(&invalid_event, &mut event_manager) + net.lock() + .unwrap() + .process(&invalid_event, &mut event_manager) ); } @@ -1265,9 +1301,10 @@ mod tests { }; let mut net = Net::default_net(test_mutators); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); // The RX queue is empty. let tap_event = EpollEvent::new(EventSet::IN, net.tap.as_raw_fd() as u64); @@ -1290,9 +1327,10 @@ mod tests { fn test_rx_rate_limiter_handling() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); net.rx_rate_limiter = RateLimiter::new(0, None, 0, 0, None, 0).unwrap(); let rate_limiter_event = @@ -1308,9 +1346,10 @@ mod tests { fn test_tx_rate_limiter_handling() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); net.tx_rate_limiter = RateLimiter::new(0, None, 0, 0, None, 0).unwrap(); let rate_limiter_event = @@ -1327,9 +1366,10 @@ mod tests { fn test_bandwidth_rate_limiter() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); let daddr = 0x2000; assert!(daddr > txq.end().0); @@ -1445,9 +1485,10 @@ mod tests { fn test_ops_rate_limiter() { let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); let daddr = 0x2000; assert!(daddr > txq.end().0); @@ -1564,9 +1605,10 @@ mod tests { #[test] fn test_patch_rate_limiters() { let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); net.rx_rate_limiter = RateLimiter::new(10, None, 10, 2, None, 2).unwrap(); net.tx_rate_limiter = RateLimiter::new(10, None, 10, 2, None, 2).unwrap(); @@ -1600,9 +1642,10 @@ mod tests { // Regression test for https://github.com/firecracker-microvm/firecracker/issues/1436 . let mut event_manager = EventManager::new().unwrap(); let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); let daddr = 0x2000; assert!(daddr > txq.end().0); @@ -1626,9 +1669,10 @@ mod tests { #[test] fn test_virtio_device() { let mut net = Net::default_net(TestMutators::default()); - let mem_clone = net.mem.clone(); - let (rxq, txq) = Net::virtqueues(&mem_clone); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); net.assign_queues(rxq.create_queue(), txq.create_queue()); + net.activate(mem.clone()).unwrap(); // Test queues count (TX and RX). let queues = net.queues(); diff --git a/src/devices/src/virtio/net/event_handler.rs b/src/devices/src/virtio/net/event_handler.rs index 5cb480787b9..1ccf14f6381 100644 --- a/src/devices/src/virtio/net/event_handler.rs +++ b/src/devices/src/virtio/net/event_handler.rs @@ -10,13 +10,89 @@ use utils::epoll::{EpollEvent, EventSet}; use crate::virtio::net::device::Net; use crate::virtio::{VirtioDevice, RX_INDEX, TX_INDEX}; -impl Subscriber for Net { - 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 Net { + 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[RX_INDEX].as_raw_fd(), + EpollEvent::new(EventSet::IN, self.queue_evts[RX_INDEX].as_raw_fd() as u64), + self_subscriber.clone(), + ) + .unwrap_or_else(|e| { + error!( + "Failed to register net rx queue with event manager: {:?}", + e + ); + }); + + event_manager + .register( + self.queue_evts[TX_INDEX].as_raw_fd(), + EpollEvent::new(EventSet::IN, self.queue_evts[TX_INDEX].as_raw_fd() as u64), + self_subscriber.clone(), + ) + .unwrap_or_else(|e| { + error!( + "Failed to register net tx queue with event manager: {:?}", + e + ); + }); + + event_manager + .register( + self.tap.as_raw_fd(), + EpollEvent::new( + EventSet::IN | EventSet::EDGE_TRIGGERED, + self.tap.as_raw_fd() as u64, + ), + self_subscriber.clone(), + ) + .unwrap_or_else(|e| { + error!("Failed to register net tap with event manager: {:?}", e); + }); + + event_manager + .register( + self.tx_rate_limiter.as_raw_fd(), + EpollEvent::new(EventSet::IN, self.tx_rate_limiter.as_raw_fd() as u64), + self_subscriber.clone(), + ) + .unwrap_or_else(|e| { + error!( + "Failed to register net tx rate limiter with event manager: {:?}", + e + ); + }); + + event_manager + .register( + self.rx_rate_limiter.as_raw_fd(), + EpollEvent::new(EventSet::IN, self.rx_rate_limiter.as_raw_fd() as u64), + self_subscriber.clone(), + ) + .unwrap_or_else(|e| { + error!( + "Failed to register net rx rate limiter with event manager: {:?}", + e + ); + }); + event_manager + .unregister(self.activate_evt.as_raw_fd()) + .unwrap_or_else(|e| { + error!("Failed to unregister net activate evt: {:?}", e); + }) + } +} + +impl Subscriber for Net { + fn process(&mut self, event: &EpollEvent, evmgr: &mut EventManager) { let source = event.fd(); let event_set = event.event_set(); @@ -31,35 +107,102 @@ impl Subscriber for Net { return; } - let virtq_rx_ev_fd = self.queue_evts[RX_INDEX].as_raw_fd(); - let virtq_tx_ev_fd = self.queue_evts[TX_INDEX].as_raw_fd(); - let rx_rate_limiter_fd = self.rx_rate_limiter.as_raw_fd(); - let tx_rate_limiter_fd = self.tx_rate_limiter.as_raw_fd(); - let tap_fd = self.tap.as_raw_fd(); - - match source { - _ if source == virtq_rx_ev_fd => self.process_rx_queue_event(), - _ if source == tap_fd => self.process_tap_rx_event(), - _ if source == virtq_tx_ev_fd => self.process_tx_queue_event(), - _ if source == rx_rate_limiter_fd => self.process_rx_rate_limiter_event(), - _ if source == tx_rate_limiter_fd => self.process_tx_rate_limiter_event(), - _ => { - error!("Unknown event source."); - METRICS.net.event_fails.inc(); + if self.is_activated() { + let virtq_rx_ev_fd = self.queue_evts[RX_INDEX].as_raw_fd(); + let virtq_tx_ev_fd = self.queue_evts[TX_INDEX].as_raw_fd(); + let rx_rate_limiter_fd = self.rx_rate_limiter.as_raw_fd(); + let tx_rate_limiter_fd = self.tx_rate_limiter.as_raw_fd(); + let tap_fd = self.tap.as_raw_fd(); + let activate_fd = self.activate_evt.as_raw_fd(); + + // Looks better than C style if/else if/else. + match source { + _ if source == virtq_rx_ev_fd => self.process_rx_queue_event(), + _ if source == tap_fd => self.process_tap_rx_event(), + _ if source == virtq_tx_ev_fd => self.process_tx_queue_event(), + _ if source == rx_rate_limiter_fd => self.process_rx_rate_limiter_event(), + _ if source == tx_rate_limiter_fd => self.process_tx_rate_limiter_event(), + _ if activate_fd == source => self.process_activate_event(evmgr), + _ => { + warn!("Net: Spurious event received: {:?}", source); + METRICS.net.event_fails.inc(); + } } + } else { + warn!( + "Net: The device is not yet activated. Spurious event received: {:?}", + source + ); } } fn interest_list(&self) -> Vec { - vec![ - EpollEvent::new( - EventSet::IN | EventSet::EDGE_TRIGGERED, - self.tap.as_raw_fd() as u64, - ), - EpollEvent::new(EventSet::IN, self.queue_evts[RX_INDEX].as_raw_fd() as u64), - EpollEvent::new(EventSet::IN, self.queue_evts[TX_INDEX].as_raw_fd() as u64), - EpollEvent::new(EventSet::IN, self.rx_rate_limiter.as_raw_fd() as u64), - EpollEvent::new(EventSet::IN, self.tx_rate_limiter.as_raw_fd() as u64), - ] + vec![EpollEvent::new( + EventSet::IN, + self.activate_evt.as_raw_fd() as u64, + )] + } +} + +#[cfg(test)] +pub mod tests { + use std::sync::{Arc, Mutex}; + + use super::*; + use crate::virtio::net::device::tests::*; + + #[test] + fn test_event_handler() { + let mut event_manager = EventManager::new().unwrap(); + let mut net = Net::default_net(TestMutators::default()); + let mem = Net::default_guest_memory(); + let (rxq, txq) = Net::virtqueues(&mem); + net.assign_queues(rxq.create_queue(), txq.create_queue()); + + let net = Arc::new(Mutex::new(net)); + event_manager.add_subscriber(net.clone()).unwrap(); + + // Push a queue event, use the TX_QUEUE_EVENT in this test. + { + let daddr = 0x2000; + assert!(daddr > txq.end().0); + + txq.avail.idx.set(1); + txq.avail.ring[0].set(0); + txq.dtable[0].set(daddr, 0x1000, 0, 0); + + net.lock().unwrap().queue_evts[TX_INDEX].write(1).unwrap(); + } + + // EventManager should report no events since net has only registered + // its activation event so far (even though there is also a queue event pending). + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 0); + + // Manually force a queue event and check it's ignored pre-activation. + { + let mut n = net.lock().unwrap(); + let raw_txq_evt = n.queue_evts[TX_INDEX].as_raw_fd() as u64; + // Artificially push event. + n.process( + &EpollEvent::new(EventSet::IN, raw_txq_evt), + &mut event_manager, + ); + // Validate there was no queue operation. + assert_eq!(txq.used.idx.get(), 0); + } + + // Now activate the device. + net.lock().unwrap().activate(mem.clone()).unwrap(); + // Process the activate event. + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 1); + + // Handle the previously pushed queue event through EventManager. + event_manager + .run_with_timeout(100) + .expect("Metrics event timeout or error."); + // Make sure the data queue advanced. + assert_eq!(txq.used.idx.get(), 1); } } diff --git a/src/devices/src/virtio/queue.rs b/src/devices/src/virtio/queue.rs index 336bfdb1f25..57a4bb6e153 100644 --- a/src/devices/src/virtio/queue.rs +++ b/src/devices/src/virtio/queue.rs @@ -361,7 +361,7 @@ impl Queue { } #[cfg(test)] -pub mod tests { +pub(crate) mod tests { extern crate vm_memory; use std::marker::PhantomData; diff --git a/src/devices/src/virtio/vsock/device.rs b/src/devices/src/virtio/vsock/device.rs index 66cfc794ef3..f3506de66ee 100644 --- a/src/devices/src/virtio/vsock/device.rs +++ b/src/devices/src/virtio/vsock/device.rs @@ -29,7 +29,7 @@ use vm_memory::GuestMemoryMmap; use super::super::super::Error as DeviceError; use super::super::{ - ActivateError, ActivateResult, Queue as VirtQueue, VirtioDevice, VsockError, + ActivateError, ActivateResult, DeviceState, Queue as VirtQueue, VirtioDevice, VsockError, VIRTIO_MMIO_INT_VRING, }; use super::packet::VsockPacket; @@ -51,7 +51,6 @@ pub struct Vsock { cid: u64, pub(crate) queues: Vec, pub(crate) queue_events: Vec, - mem: GuestMemoryMmap, pub(crate) backend: B, avail_features: u64, acked_features: u64, @@ -63,7 +62,7 @@ pub struct Vsock { // mostly something we wanted to happen for the backend events, to prevent (potentially) // continuous triggers from happening before the device gets activated. pub(crate) activate_evt: EventFd, - device_activated: bool, + pub(crate) device_state: DeviceState, } // TODO: Detect / handle queue deadlock: @@ -77,7 +76,6 @@ where { pub(crate) fn with_queues( cid: u64, - mem: GuestMemoryMmap, backend: B, queues: Vec, ) -> super::Result> { @@ -90,24 +88,23 @@ where cid, queues, queue_events, - mem, backend, avail_features: AVAIL_FEATURES, acked_features: 0, interrupt_status: Arc::new(AtomicUsize::new(0)), interrupt_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(VsockError::EventFd)?, activate_evt: EventFd::new(libc::EFD_NONBLOCK).map_err(VsockError::EventFd)?, - device_activated: false, + device_state: DeviceState::Inactive, }) } /// Create a new virtio-vsock device with the given VM CID and vsock backend. - pub fn new(cid: u64, mem: GuestMemoryMmap, backend: B) -> super::Result> { + pub fn new(cid: u64, backend: B) -> super::Result> { let queues: Vec = defs::QUEUE_SIZES .iter() .map(|&max_size| VirtQueue::new(max_size)) .collect(); - Self::with_queues(cid, mem, backend, queues) + Self::with_queues(cid, backend, queues) } pub fn cid(&self) -> u64 { @@ -131,10 +128,15 @@ where /// otherwise. pub fn process_rx(&mut self) -> bool { debug!("vsock: process_rx()"); + 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 => unreachable!(), + }; let mut have_used = false; - while let Some(head) = self.queues[RXQ_INDEX].pop(&self.mem) { + while let Some(head) = self.queues[RXQ_INDEX].pop(mem) { let used_len = match VsockPacket::from_rx_virtq_head(&head) { Ok(mut pkt) => { if self.backend.recv_pkt(&mut pkt).is_ok() { @@ -153,7 +155,7 @@ where }; have_used = true; - self.queues[RXQ_INDEX].add_used(&self.mem, head.index, used_len); + self.queues[RXQ_INDEX].add_used(mem, head.index, used_len); } have_used @@ -164,16 +166,21 @@ where /// ring, and `false` otherwise. pub fn process_tx(&mut self) -> bool { debug!("vsock::process_tx()"); + 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 => unreachable!(), + }; let mut have_used = false; - while let Some(head) = self.queues[TXQ_INDEX].pop(&self.mem) { + while let Some(head) = self.queues[TXQ_INDEX].pop(mem) { let pkt = match VsockPacket::from_tx_virtq_head(&head) { Ok(pkt) => pkt, Err(e) => { error!("vsock: error reading TX packet: {:?}", e); have_used = true; - self.queues[TXQ_INDEX].add_used(&self.mem, head.index, 0); + self.queues[TXQ_INDEX].add_used(mem, head.index, 0); continue; } }; @@ -184,7 +191,7 @@ where } have_used = true; - self.queues[TXQ_INDEX].add_used(&self.mem, head.index, 0); + self.queues[TXQ_INDEX].add_used(mem, head.index, 0); } have_used @@ -252,7 +259,7 @@ where ); } - fn activate(&mut self) -> ActivateResult { + fn activate(&mut self, mem: GuestMemoryMmap) -> ActivateResult { if self.queues.len() != defs::NUM_QUEUES { error!( "Cannot perform activate. Expected {} queue(s), got {}", @@ -267,13 +274,16 @@ where return Err(ActivateError::BadActivate); } - self.device_activated = true; + self.device_state = DeviceState::Activated(mem); Ok(()) } fn is_activated(&self) -> bool { - self.device_activated + match self.device_state { + DeviceState::Inactive => false, + DeviceState::Activated(_) => true, + } } } @@ -351,6 +361,6 @@ mod tests { // } // Test a correct activation. - ctx.device.activate().unwrap(); + ctx.device.activate(ctx.mem.clone()).unwrap(); } } diff --git a/src/devices/src/virtio/vsock/event_handler.rs b/src/devices/src/virtio/vsock/event_handler.rs index bd882fdc61d..14e72ca84ac 100644 --- a/src/devices/src/virtio/vsock/event_handler.rs +++ b/src/devices/src/virtio/vsock/event_handler.rs @@ -29,6 +29,7 @@ use utils::epoll::{EpollEvent, EventSet}; use super::device::{Vsock, EVQ_INDEX, RXQ_INDEX, TXQ_INDEX}; use super::VsockBackend; +use crate::virtio::VirtioDevice; impl Vsock where @@ -191,23 +192,28 @@ where let backend = self.backend.as_raw_fd(); let activate_evt = self.activate_evt.as_raw_fd(); - let mut raise_irq = false; - - match source { - _ if source == rxq => raise_irq = self.handle_rxq_event(event), - _ if source == txq => raise_irq = self.handle_txq_event(event), - _ if source == evq => raise_irq = self.handle_evq_event(event), - _ if source == backend => { - raise_irq = self.notify_backend(event); + if self.is_activated() { + let mut raise_irq = false; + match source { + _ if source == rxq => raise_irq = self.handle_rxq_event(event), + _ if source == txq => raise_irq = self.handle_txq_event(event), + _ if source == evq => raise_irq = self.handle_evq_event(event), + _ if source == backend => { + raise_irq = self.notify_backend(event); + } + _ if source == activate_evt => { + self.handle_activate_event(event_manager); + } + _ => warn!("Unexpected vsock event received: {:?}", source), } - _ if source == activate_evt => { - self.handle_activate_event(event_manager); + if raise_irq { + self.signal_used_queue().unwrap_or_default(); } - _ => warn!("Unexpected vsock event received: {:?}", source), - } - - if raise_irq { - self.signal_used_queue().unwrap_or_default(); + } else { + warn!( + "Vsock: The device is not yet activated. Spurious event received: {:?}", + source + ); } } @@ -222,11 +228,13 @@ where #[cfg(test)] mod tests { use std::sync::atomic::Ordering; + use std::sync::{Arc, Mutex}; - use super::super::tests::TestContext; + use super::super::tests::{EventHandlerContext, TestContext}; use super::super::*; use super::*; + use crate::virtio::device::VirtioDevice; use crate::virtio::vsock::packet::VSOCK_PKT_HDR_SIZE; use crate::virtio::VIRTIO_MMIO_INT_VRING; use crate::Error as DeviceError; @@ -268,6 +276,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(false); ctx.signal_txq_event(); @@ -284,6 +293,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(true); ctx.signal_txq_event(); @@ -299,6 +309,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(false); ctx.device.backend.set_tx_err(Some(VsockError::NoData)); @@ -314,6 +325,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); // Invalidate the packet header descriptor, by setting its length to 0. ctx.guest_txvq.dtable[0].len.set(0); @@ -329,6 +341,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); assert!(!ctx .device @@ -345,6 +358,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(true); ctx.device.backend.set_rx_err(Some(VsockError::NoData)); @@ -361,6 +375,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(true); ctx.signal_rxq_event(); @@ -373,6 +388,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); // Invalidate the packet header descriptor, by setting its length to 0. ctx.guest_rxvq.dtable[0].len.set(0); @@ -387,6 +403,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(false); assert!(!ctx .device @@ -415,6 +432,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(true); ctx.device.notify_backend(&EpollEvent::new(EventSet::IN, 0)); @@ -433,6 +451,7 @@ mod tests { { let test_ctx = TestContext::new(); let mut ctx = test_ctx.create_event_handler_context(); + ctx.mock_activate(test_ctx.mem.clone()); ctx.device.backend.set_pending_rx(false); ctx.device.notify_backend(&EpollEvent::new(EventSet::IN, 0)); @@ -547,4 +566,70 @@ mod tests { GAP_SIZE as u32 + 100, ); } + + #[test] + fn test_event_handler() { + let mut event_manager = EventManager::new().unwrap(); + let test_ctx = TestContext::new(); + let EventHandlerContext { + device, + guest_rxvq, + guest_txvq, + .. + } = test_ctx.create_event_handler_context(); + + let vsock = Arc::new(Mutex::new(device)); + event_manager.add_subscriber(vsock.clone()).unwrap(); + + // Push a queue event + // - the driver has something to send (there's data in the TX queue); and + // - the backend also has some pending RX data. + { + let mut device = vsock.lock().unwrap(); + device.backend.set_pending_rx(true); + device.queue_events[TXQ_INDEX].write(1).unwrap(); + } + + // EventManager should report no events since vsock has only registered + // its activation event so far (even though there is also a queue event pending). + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 0); + + // Manually force a queue event and check it's ignored pre-activation. + { + let mut device = vsock.lock().unwrap(); + + let raw_txq_evt = device.queue_events[TXQ_INDEX].as_raw_fd() as u64; + // Artificially push event. + device.process( + &EpollEvent::new(EventSet::IN, raw_txq_evt), + &mut event_manager, + ); + + // Both available RX and TX descriptors should be untouched. + assert_eq!(guest_rxvq.used.idx.get(), 0); + assert_eq!(guest_txvq.used.idx.get(), 0); + } + + // Now activate the device. + vsock + .lock() + .unwrap() + .activate(test_ctx.mem.clone()) + .unwrap(); + // Process the activate event. + let ev_count = event_manager.run_with_timeout(50).unwrap(); + assert_eq!(ev_count, 1); + + // Handle the previously pushed queue event through EventManager. + { + let ev_count = event_manager + .run_with_timeout(100) + .expect("Metrics event timeout or error."); + assert_eq!(ev_count, 1); + // Both available RX and TX descriptors should have been used. + assert_eq!(guest_rxvq.used.idx.get(), 1); + assert_eq!(guest_txvq.used.idx.get(), 1); + } + } } diff --git a/src/devices/src/virtio/vsock/mod.rs b/src/devices/src/virtio/vsock/mod.rs index 203063841ed..79a317dcbd3 100644 --- a/src/devices/src/virtio/vsock/mod.rs +++ b/src/devices/src/virtio/vsock/mod.rs @@ -159,7 +159,7 @@ mod tests { use utils::eventfd::EventFd; use crate::virtio::queue::tests::VirtQueue as GuestQ; - use crate::virtio::{VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE}; + use crate::virtio::{VirtioDevice, VIRTQ_DESC_F_NEXT, VIRTQ_DESC_F_WRITE}; use utils::epoll::EpollEvent; use vm_memory::{GuestAddress, GuestMemoryMmap}; @@ -258,9 +258,9 @@ mod tests { let mem = GuestMemoryMmap::from_ranges(&[(GuestAddress(0), MEM_SIZE)]).unwrap(); Self { cid: CID, - mem: mem.clone(), + mem, mem_size: MEM_SIZE, - device: Vsock::new(CID, mem, TestBackend::new()).unwrap(), + device: Vsock::new(CID, TestBackend::new()).unwrap(), } } @@ -296,13 +296,8 @@ mod tests { guest_rxvq, guest_txvq, guest_evvq, - device: Vsock::with_queues( - self.cid, - self.mem.clone(), - TestBackend::new(), - vec![rxvq, txvq, evvq], - ) - .unwrap(), + device: Vsock::with_queues(self.cid, TestBackend::new(), vec![rxvq, txvq, evvq]) + .unwrap(), } } } @@ -315,6 +310,11 @@ mod tests { } impl<'a> EventHandlerContext<'a> { + pub fn mock_activate(&mut self, mem: GuestMemoryMmap) { + // Artificially activate the device. + self.device.activate(mem).unwrap(); + } + pub fn signal_txq_event(&mut self) { self.device.queue_events[TXQ_INDEX].write(1).unwrap(); self.device diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index 2ed483a15ad..94277939765 100644 --- 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(), @@ -759,7 +758,6 @@ fn attach_net_devices( devices::virtio::net::Net::new_with_tap( tap, cfg.guest_mac(), - vmm.guest_memory().clone(), rx_rate_limiter.unwrap_or_default(), tx_rate_limiter.unwrap_or_default(), allow_mmds_requests, @@ -796,12 +794,8 @@ fn attach_vsock_device( .map_err(CreateVsockBackend)?; let vsock_device = Arc::new(Mutex::new( - devices::virtio::Vsock::new( - u64::from(vsock.guest_cid), - vmm.guest_memory().clone(), - backend, - ) - .map_err(CreateVsockDevice)?, + devices::virtio::Vsock::new(u64::from(vsock.guest_cid), backend) + .map_err(CreateVsockDevice)?, )); event_manager diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index 41c94ace758..5acff33cfc7 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -389,7 +389,7 @@ mod tests { let _ = data; } - fn activate(&mut self) -> ActivateResult { + fn activate(&mut self, _: GuestMemoryMmap) -> ActivateResult { Ok(()) } @@ -471,7 +471,6 @@ mod tests { let mut dummy = DummyDevice::new(); assert_eq!(dummy.device_type(), 0); assert_eq!(dummy.queues().len(), QUEUE_SIZES.len()); - dummy.activate().unwrap(); } #[test] diff --git a/tests/integration_tests/build/test_coverage.py b/tests/integration_tests/build/test_coverage.py index 49aea545b44..157394cdfb0 100644 --- a/tests/integration_tests/build/test_coverage.py +++ b/tests/integration_tests/build/test_coverage.py @@ -19,7 +19,7 @@ import host_tools.cargo_build as host # pylint: disable=import-error -COVERAGE_TARGET_PCT = 82.53 +COVERAGE_TARGET_PCT = 82.78 COVERAGE_MAX_DELTA = 0.05 CARGO_KCOV_REL_PATH = os.path.join(host.CARGO_BUILD_REL_PATH, 'kcov')