From 6409db620c2638ec8ef052eeb3f5ef4d70a89837 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Thu, 19 Mar 2020 13:55:49 +0200 Subject: [PATCH 1/5] devices: re-introduce mem in VirtioDevice activate Instead of passing memory at device creation, bring it in during device activation. This enables future scenarios where devices can be created prior to guest memory configuration. Signed-off-by: Adrian Catangiu --- src/devices/src/virtio/block/device.rs | 18 +++++++++--------- src/devices/src/virtio/device.rs | 3 ++- src/devices/src/virtio/mmio.rs | 4 ++-- src/devices/src/virtio/net/device.rs | 4 ++-- src/devices/src/virtio/vsock/device.rs | 4 ++-- src/vmm/src/device_manager/mmio.rs | 3 +-- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 6692e59bc13..7aca58b26f5 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -321,7 +321,7 @@ impl VirtioDevice for Block { self.device_activated } - fn activate(&mut self) -> ActivateResult { + fn activate(&mut self, _: GuestMemoryMmap) -> ActivateResult { self.device_activated = true; Ok(()) } @@ -499,7 +499,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -524,7 +524,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -582,7 +582,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -612,7 +612,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -671,7 +671,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -714,7 +714,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -780,7 +780,7 @@ mod tests { let mem = block.mem.clone(); 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()); @@ -845,7 +845,7 @@ mod tests { let mem = block.mem.clone(); 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/device.rs b/src/devices/src/virtio/device.rs index 446bc2ced05..591fdfdddaf 100644 --- a/src/devices/src/virtio/device.rs +++ b/src/devices/src/virtio/device.rs @@ -10,6 +10,7 @@ use std::sync::{atomic::AtomicUsize, Arc}; use super::{ActivateResult, Queue}; use crate::virtio::AsAny; use utils::eventfd::EventFd; +use vm_memory::GuestMemoryMmap; /// Trait for virtio devices to be driven by a virtio transport. /// @@ -87,7 +88,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..7a7b6eda93e 100644 --- a/src/devices/src/virtio/net/device.rs +++ b/src/devices/src/virtio/net/device.rs @@ -679,7 +679,7 @@ impl VirtioDevice for Net { self.device_activated } - fn activate(&mut self) -> ActivateResult { + fn activate(&mut self, _: GuestMemoryMmap) -> ActivateResult { self.device_activated = true; Ok(()) } @@ -812,7 +812,7 @@ mod tests { self.queues.clear(); self.queues.push(rxq); self.queues.push(txq); - self.activate().unwrap(); + self.activate(self.mem.clone()).unwrap(); } } diff --git a/src/devices/src/virtio/vsock/device.rs b/src/devices/src/virtio/vsock/device.rs index 66cfc794ef3..9c34d25436c 100644 --- a/src/devices/src/virtio/vsock/device.rs +++ b/src/devices/src/virtio/vsock/device.rs @@ -252,7 +252,7 @@ where ); } - fn activate(&mut self) -> ActivateResult { + fn activate(&mut self, _: GuestMemoryMmap) -> ActivateResult { if self.queues.len() != defs::NUM_QUEUES { error!( "Cannot perform activate. Expected {} queue(s), got {}", @@ -351,6 +351,6 @@ mod tests { // } // Test a correct activation. - ctx.device.activate().unwrap(); + ctx.device.activate(ctx.mem.clone()).unwrap(); } } 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] From 8e21fa756f68ebd21ad4799d49cf7fdd0c64e361 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Thu, 19 Mar 2020 16:21:06 +0200 Subject: [PATCH 2/5] devices: block: activate through an activation event 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 --- src/devices/src/virtio/block/device.rs | 69 +++++--- src/devices/src/virtio/block/event_handler.rs | 163 +++++++++++++++--- src/devices/src/virtio/device.rs | 7 + src/devices/src/virtio/queue.rs | 2 +- src/vmm/src/builder.rs | 1 - 5 files changed, 192 insertions(+), 50 deletions(-) diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 7aca58b26f5..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, _: 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(()) } } #[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,7 +509,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(); @@ -521,7 +534,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(); @@ -579,7 +592,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(); @@ -609,7 +622,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(); @@ -668,7 +681,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(); @@ -711,7 +724,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(); @@ -777,7 +790,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(); @@ -842,7 +855,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 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 591fdfdddaf..8be8d57fc63 100644 --- 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/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/vmm/src/builder.rs b/src/vmm/src/builder.rs index 2ed483a15ad..51c613a5081 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(), From 04f70de0fa6737c1509dd4be78055971c35c5d2e Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 30 Mar 2020 22:50:15 +0300 Subject: [PATCH 3/5] devices: net: activate through an activation event Postpone net external events registration to device activation time. This makes the net 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 --- src/devices/src/lib.rs | 1 + src/devices/src/virtio/net/device.rs | 134 ++++++++----- src/devices/src/virtio/net/event_handler.rs | 205 +++++++++++++++++--- src/vmm/src/builder.rs | 1 - 4 files changed, 264 insertions(+), 77 deletions(-) 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/net/device.rs b/src/devices/src/virtio/net/device.rs index 7a7b6eda93e..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, _: GuestMemoryMmap) -> 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(self.mem.clone()).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/vmm/src/builder.rs b/src/vmm/src/builder.rs index 51c613a5081..7c8cadca15b 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -758,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, From 24ed2f1bdb9da66d9c47cef1c02641d25c20832d Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 24 Mar 2020 19:02:57 +0200 Subject: [PATCH 4/5] devices: vsock: pass memory during activate Allow Vsock creation without guest memory. Access to memory will be given to the device during its activation. Signed-off-by: Adrian Catangiu --- src/devices/src/virtio/vsock/device.rs | 42 ++++++++++++------- src/devices/src/virtio/vsock/event_handler.rs | 11 +++++ src/devices/src/virtio/vsock/mod.rs | 20 ++++----- src/vmm/src/builder.rs | 8 +--- 4 files changed, 49 insertions(+), 32 deletions(-) diff --git a/src/devices/src/virtio/vsock/device.rs b/src/devices/src/virtio/vsock/device.rs index 9c34d25436c..c75f418b1d1 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 => return false, + }; 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 => return false, + }; 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, _: GuestMemoryMmap) -> 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, + } } } diff --git a/src/devices/src/virtio/vsock/event_handler.rs b/src/devices/src/virtio/vsock/event_handler.rs index bd882fdc61d..1c1ec0e0109 100644 --- a/src/devices/src/virtio/vsock/event_handler.rs +++ b/src/devices/src/virtio/vsock/event_handler.rs @@ -268,6 +268,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 +285,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 +301,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 +317,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 +333,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 +350,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 +367,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 +380,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 +395,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 +424,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 +443,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)); 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 7c8cadca15b..94277939765 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -794,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 From be73e7cf6a23448a6434bf30d34491d73aa49e82 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 30 Mar 2020 22:04:54 +0300 Subject: [PATCH 5/5] devices: vsock: only handle events after device activation Make sure all events are ignored prior to device activation. Added test for vsock event handling through EventManager. Signed-off-by: Adrian Catangiu --- src/devices/src/virtio/vsock/device.rs | 4 +- src/devices/src/virtio/vsock/event_handler.rs | 106 +++++++++++++++--- .../integration_tests/build/test_coverage.py | 2 +- 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/src/devices/src/virtio/vsock/device.rs b/src/devices/src/virtio/vsock/device.rs index c75f418b1d1..f3506de66ee 100644 --- a/src/devices/src/virtio/vsock/device.rs +++ b/src/devices/src/virtio/vsock/device.rs @@ -131,7 +131,7 @@ where 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, + DeviceState::Inactive => unreachable!(), }; let mut have_used = false; @@ -169,7 +169,7 @@ where 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, + DeviceState::Inactive => unreachable!(), }; let mut have_used = false; diff --git a/src/devices/src/virtio/vsock/event_handler.rs b/src/devices/src/virtio/vsock/event_handler.rs index 1c1ec0e0109..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; @@ -558,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/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')