Skip to content

Align device events registration after device activation #1687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iulianbarbu merged 6 commits into from
Apr 1, 2020
Merged

Align device events registration after device activation #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6409db6
devices: re-introduce mem in VirtioDevice activate
acatangiu Mar 19, 2020
8e21fa7
devices: block: activate through an activation event
acatangiu Mar 19, 2020
04f70de
devices: net: activate through an activation event
acatangiu Mar 30, 2020
24ed2f1
devices: vsock: pass memory during activate
acatangiu Mar 24, 2020
be73e7c
devices: vsock: only handle events after device activation
acatangiu Mar 30, 2020
a113b2e
Merge branch 'master' into device_activate
acatangiu Apr 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/devices/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,5 @@ pub enum Error {
PayloadExpected,
IoError(io::Error),
NoAvailBuffers,
SpuriousEvent,
}
85 changes: 49 additions & 36 deletions src/devices/src/virtio/block/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -88,15 +88,15 @@ pub struct Block {
avail_features: u64,
acked_features: u64,
config_space: Vec<u8>,
pub(crate) activate_evt: EventFd,

// Transport related fields.
queues: Vec<Queue>,
interrupt_status: Arc<AtomicUsize>,
interrupt_evt: EventFd,
pub(crate) queue_evts: [EventFd; 1],
mem: GuestMemoryMmap,

device_activated: bool,
pub(crate) device_state: DeviceState,

// Implementation specific fields.
pub(crate) rate_limiter: RateLimiter,
Expand All @@ -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,
Expand All @@ -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)?,
})
}

Expand All @@ -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.
Copy link
Contributor

@andreeaflorescu andreeaflorescu Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this should never happen, should we use a panic instead to avoid programming errors?

Copy link
Contributor Author

@acatangiu acatangiu Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm usually very apprehensive of adding crash conditions because of the risk of crashing in production.

In this area however, we have a lot of unit-tests so any future programming errors will be better caught with a panic as you say. Also because of the high-degree of coverage, I believe it is safe to say that we shouldn't crash in prod while tests are passing.

Per your suggestion, I've modified all instances of this check to have unreachable!() on the unreachable path.

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.
Expand Down Expand Up @@ -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) => {
Expand All @@ -212,15 +216,15 @@ impl Block {
};
// We use unwrap because the request parsing process already checked that the
// status_addr was valid.
self.mem.write_obj(status, request.status_addr).unwrap();
mem.write_obj(status, request.status_addr).unwrap();
}
Err(e) => {
error!("Failed to parse available descriptor chain: {:?}", e);
METRICS.block.execute_fails.inc();
len = 0;
}
}
queue.add_used(&self.mem, head.index, len);
queue.add_used(mem, head.index, len);
used_any = true;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

Expand All @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down
Loading