From 6fddf45e90fffcae9503f24c841a2b72a14c8b0e Mon Sep 17 00:00:00 2001 From: Alexandru Agache Date: Mon, 14 Dec 2020 16:35:25 +0200 Subject: [PATCH] refactor common code Signed-off-by: Alexandru Agache --- Cargo.lock | 10 +- Cargo.toml | 3 - rust-vmm-ci | 2 +- src/devices/Cargo.toml | 2 +- src/devices/src/virtio/block/device.rs | 337 ++++++------------------- src/devices/src/virtio/block/mod.rs | 97 +++++-- src/devices/src/virtio/mod.rs | 277 +++++++++++++++++++- src/vm-vcpu/Cargo.toml | 3 +- src/vmm/Cargo.toml | 2 +- src/vmm/src/lib.rs | 7 +- 10 files changed, 443 insertions(+), 297 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 30bf5874..c01f7730 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -116,9 +116,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.87" +version = "0.2.81" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "265d751d31d6780a3f956bb5b8022feba2d94eeee5a84ba64f4212eedca42213" +checksum = "1482821306169ec4d07f6aca392a4681f66c75c9918aa49641a2595db64053cb" [[package]] name = "linux-loader" @@ -209,7 +209,7 @@ dependencies = [ [[package]] name = "vm-virtio" version = "0.1.0" -source = "git+https://github.com/alexandruag/vm-virtio.git?branch=proto_wip4#e0a0d5dcbdb0c15f5a245934313db6dd2845bb7f" +source = "git+https://github.com/rust-vmm/vm-virtio.git#7b5fd44d802408212d746eaa3e41ad2f22477a7a" dependencies = [ "byteorder", "libc", @@ -246,9 +246,9 @@ dependencies = [ [[package]] name = "vmm-sys-util" -version = "0.6.1" +version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "183d25b56a61a6f518ef464ac578e790f04added34dfaab59a453d8a03cb7bd0" +checksum = "d1cdd1d72e262bbfb014de65ada24c1ac50e10a2e3b1e8ec052df188c2ee5dfa" dependencies = [ "bitflags", "libc", diff --git a/Cargo.toml b/Cargo.toml index 56d65fa5..ca82d13b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,3 @@ panic = "abort" [patch.crates-io] event-manager = { git = "https://github.com/rust-vmm/event-manager.git", rev = "19ddbaa" } linux-loader = { git = "https://github.com/rust-vmm/linux-loader.git", rev ="acf9c21"} - -[patch."https://github.com/rust-vmm/vm-virtio.git"] -vm-virtio = { git = "https://github.com/alexandruag/vm-virtio.git", branch = "proto_wip4" } diff --git a/rust-vmm-ci b/rust-vmm-ci index ebc70164..e1108f1c 160000 --- a/rust-vmm-ci +++ b/rust-vmm-ci @@ -1 +1 @@ -Subproject commit ebc701641fa57f78d03f3f5ecac617b7bf7470b4 +Subproject commit e1108f1c908a255f74d9150bf48f094fd5f57b3e diff --git a/src/devices/Cargo.toml b/src/devices/Cargo.toml index 522c7443..bdaef24f 100644 --- a/src/devices/Cargo.toml +++ b/src/devices/Cargo.toml @@ -12,7 +12,7 @@ libc = "0.2.76" linux-loader = "0.2.0" log = "0.4.6" vm-memory = "0.4.0" -vmm-sys-util = "0.6.1" +vmm-sys-util = "0.7.0" # vm-device is not yet published on crates.io. # To make sure that breaking changes to vm-device are not breaking the diff --git a/src/devices/src/virtio/block/device.rs b/src/devices/src/virtio/block/device.rs index 78bd3ece..c04f03e0 100644 --- a/src/devices/src/virtio/block/device.rs +++ b/src/devices/src/virtio/block/device.rs @@ -1,27 +1,22 @@ // Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause +use std::borrow::{Borrow, BorrowMut}; use std::fs::OpenOptions; use std::ops::DerefMut; use std::path::PathBuf; use std::sync::{Arc, Mutex}; -use event_manager::{MutEventSubscriber, RemoteEndpoint, Result as EvmgrResult, SubscriberId}; -use kvm_ioctls::{IoEventAddress, VmFd}; use vm_device::bus::MmioAddress; use vm_device::device_manager::MmioManager; use vm_device::{DeviceMmio, MutDeviceMmio}; -use vm_memory::{GuestAddress, GuestAddressSpace}; +use vm_memory::GuestAddressSpace; use vm_virtio::block::stdio_executor::StdIoBackend; -use vm_virtio::device::{VirtioConfig, VirtioMmioDevice, WithDeviceOps, WithVirtioConfig}; +use vm_virtio::device::{VirtioConfig, VirtioDeviceActions, VirtioDeviceType, VirtioMmioDevice}; use vm_virtio::Queue; -use vmm_sys_util::eventfd::{EventFd, EFD_NONBLOCK}; -use crate::virtio::block::{BLOCK_DEVICE_ID, VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_RO}; -use crate::virtio::features::{VIRTIO_F_IN_ORDER, VIRTIO_F_RING_EVENT_IDX, VIRTIO_F_VERSION_1}; -use crate::virtio::{ - MmioConfig, SingleFdSignalQueue, QUEUE_MAX_SIZE, VIRTIO_MMIO_QUEUE_NOTIFY_OFFSET, -}; +use crate::virtio::block::{BLOCK_DEVICE_ID, VIRTIO_BLK_F_RO}; +use crate::virtio::{CommonConfig, Env, SingleFdSignalQueue, QUEUE_MAX_SIZE}; use super::inorder_handler::InOrderQueueHandler; use super::queue_handler::QueueHandler; @@ -31,11 +26,7 @@ use super::{build_config_space, BlockArgs, Error, Result}; // the functionality when we implement virtio PCI as well, for example by having a base generic // type, and then separate concrete instantiations for `MmioConfig` and `PciConfig`. pub struct Block { - virtio_cfg: VirtioConfig, - mmio_cfg: MmioConfig, - endpoint: RemoteEndpoint>>, - vm_fd: Arc, - irqfd: Arc, + cfg: CommonConfig, file_path: PathBuf, read_only: bool, // We'll prob need to remember this for state save/restore unless we pass the info from @@ -47,149 +38,76 @@ impl Block where M: GuestAddressSpace + Clone + Send + 'static, { - pub fn new(mut args: BlockArgs) -> Result>> - where - // We're using this (more convoluted) bound so we can pass both references and smart - // pointers such as mutex guards here. - B: DerefMut, - B::Target: MmioManager>, - { - // The queue handling logic for this device uses the buffers in order, so we enable the - // corresponding feature as well. - let mut device_features = - 1 << VIRTIO_F_VERSION_1 | 1 << VIRTIO_F_IN_ORDER | 1 << VIRTIO_F_RING_EVENT_IDX; - - if args.read_only { - device_features |= 1 << VIRTIO_BLK_F_RO; - } - - if args.advertise_flush { - device_features |= 1 << VIRTIO_BLK_F_FLUSH; - } + // Helper method that only creates a `Block` object. + fn create_block(env: &mut Env, args: &BlockArgs) -> Result { + let device_features = args.device_features(); // A block device has a single queue. - let queues = vec![Queue::new(args.common.mem, QUEUE_MAX_SIZE)]; + let queues = vec![Queue::new(env.mem.clone(), QUEUE_MAX_SIZE)]; let config_space = build_config_space(&args.file_path)?; let virtio_cfg = VirtioConfig::new(device_features, queues, config_space); - // Used to send notifications to the driver. - let irqfd = EventFd::new(EFD_NONBLOCK).map_err(Error::EventFd)?; + let common_cfg = CommonConfig::new(virtio_cfg, env).map_err(Error::Generic)?; - args.common - .vm_fd - .register_irqfd(&irqfd, args.common.mmio_cfg.gsi) - .map_err(Error::RegisterIrqfd)?; - - let mmio_cfg = args.common.mmio_cfg; - - let block = Arc::new(Mutex::new(Block { - virtio_cfg, - mmio_cfg, - endpoint: args.common.event_mgr.remote_endpoint(), - vm_fd: args.common.vm_fd, - irqfd: Arc::new(irqfd), - file_path: args.file_path, + Ok(Block { + cfg: common_cfg, + file_path: args.file_path.clone(), read_only: args.read_only, _root_device: args.root_device, - })); + }) + } + + // Create `Block` object, register it on the MMIO bus, and add any extra required info to + // the kernel cmdline from the environment. + pub fn new(env: &mut Env, args: &BlockArgs) -> Result>> + where + // We're using this (more convoluted) bound so we can pass both references and smart + // pointers such as mutex guards here. + B: DerefMut, + B::Target: MmioManager>, + { + let block = Arc::new(Mutex::new(Self::create_block(env, args)?)); // Register the device on the MMIO bus. - args.common - .mmio_mgr - .register_mmio(mmio_cfg.range, block.clone()) - .map_err(Error::Bus)?; - - // Extra parameters have to be appended to the cmdline passed to the kernel because - // there's no active enumeration/discovery mechanism for virtio over MMIO. In the future, - // we might rely on a device tree representation instead. - - args.common - .kernel_cmdline - .add_virtio_mmio_device( - mmio_cfg.range.size(), - GuestAddress(mmio_cfg.range.base().0), - mmio_cfg.gsi, - None, - ) - .map_err(Error::Cmdline)?; - - if args.root_device { - args.common - .kernel_cmdline - .insert_str("root=/dev/vda") - .map_err(Error::Cmdline)?; - - if args.read_only { - args.common - .kernel_cmdline - .insert_str("ro") - .map_err(Error::Cmdline)?; - } - } + env.register_mmio_device(block.clone()) + .map_err(Error::Generic)?; + + env.insert_cmdline_str(args.cmdline_config_substring()) + .map_err(Error::Generic)?; Ok(block) } } -// We now implement `WithVirtioConfig` and `WithDeviceOps` to get the automatic implementation -// for `VirtioDevice`. -impl WithVirtioConfig for Block { - fn device_type(&self) -> u32 { - BLOCK_DEVICE_ID +impl Borrow> for Block { + fn borrow(&self) -> &VirtioConfig { + &self.cfg.virtio } +} - fn virtio_config(&self) -> &VirtioConfig { - &self.virtio_cfg +impl BorrowMut> for Block { + fn borrow_mut(&mut self) -> &mut VirtioConfig { + &mut self.cfg.virtio } +} - fn virtio_config_mut(&mut self) -> &mut VirtioConfig { - &mut self.virtio_cfg +impl VirtioDeviceType for Block { + fn device_type(&self) -> u32 { + BLOCK_DEVICE_ID } } -impl WithDeviceOps for Block { +impl VirtioDeviceActions for Block { type E = Error; fn activate(&mut self) -> Result<()> { - if self.virtio_cfg.device_activated { - return Err(Error::AlreadyActivated); - } - - if !self.queues_valid() { - return Err(Error::QueuesNotValid); - } - - // We do not support legacy drivers. - if self.virtio_cfg.driver_features & (1 << VIRTIO_F_VERSION_1) == 0 { - return Err(Error::BadFeatures(self.virtio_cfg.driver_features)); - } - - // Set the appropriate queue configuration flag if the `EVENT_IDX` features has been - // negotiated. - if self.virtio_cfg.driver_features & (1 << VIRTIO_F_RING_EVENT_IDX) != 0 { - self.virtio_cfg.queues[0].set_event_idx(true); - } - - let ioeventfd = EventFd::new(EFD_NONBLOCK).map_err(Error::EventFd)?; - - // Register the queue event fd. - self.vm_fd - .register_ioevent( - &ioeventfd, - &IoEventAddress::Mmio( - self.mmio_cfg.range.base().0 + VIRTIO_MMIO_QUEUE_NOTIFY_OFFSET, - ), - 0u32, - ) - .map_err(Error::RegisterIoevent)?; - let file = OpenOptions::new() .read(true) .write(!self.read_only) .open(&self.file_path) .map_err(Error::OpenFile)?; - let mut features = self.virtio_cfg.driver_features; + let mut features = self.cfg.virtio.driver_features; if self.read_only { // Not sure if the driver is expected to explicitly acknowledge the `RO` feature, // so adding it explicitly here when present just in case. @@ -200,31 +118,24 @@ impl WithDeviceOps for Block { let disk = StdIoBackend::new(file, features).map_err(Error::Backend)?; let driver_notify = SingleFdSignalQueue { - irqfd: self.irqfd.clone(), - interrupt_status: self.virtio_cfg.interrupt_status.clone(), + irqfd: self.cfg.irqfd.clone(), + interrupt_status: self.cfg.virtio.interrupt_status.clone(), }; let inner = InOrderQueueHandler { driver_notify, - queue: self.virtio_cfg.queues[0].clone(), + queue: self.cfg.virtio.queues[0].clone(), disk, }; - let handler = Arc::new(Mutex::new(QueueHandler { inner, ioeventfd })); - - // Register the queue handler with the `EventManager`. We could record the `sub_id` - // (and/or keep a handler clone) for further interaction (i.e. to remove the subscriber at - // a later time, retrieve state, etc). - let _sub_id = self - .endpoint - .call_blocking(move |mgr| -> EvmgrResult { - Ok(mgr.add_subscriber(handler)) - }) - .map_err(Error::Endpoint)?; + let mut ioevents = self.cfg.prepare_activate().map_err(Error::Generic)?; - self.virtio_cfg.device_activated = true; + let handler = Arc::new(Mutex::new(QueueHandler { + inner, + ioeventfd: ioevents.remove(0), + })); - Ok(()) + self.cfg.finalize_activate(handler).map_err(Error::Generic) } fn reset(&mut self) -> Result<()> { @@ -249,127 +160,45 @@ impl MutDeviceMmio for Block { mod tests { use vmm_sys_util::tempfile::TempFile; - use crate::virtio::tests::CommonArgsMock; + use crate::virtio::tests::EnvMock; + use super::super::VIRTIO_BLK_F_FLUSH; use super::*; // Restricting this for now, because registering irqfds does not work on Arm without properly - // setting up the equivalent of the irqchip first (as part of `CommonArgsContext::new`). + // setting up the equivalent of the irqchip first (as part of `EnvMock::new`). #[cfg_attr(target_arch = "aarch64", ignore)] #[test] fn test_device() { let tmp = TempFile::new().unwrap(); - { - let mut mock = CommonArgsMock::new(); - let common_args = mock.args(); - let args = BlockArgs { - common: common_args, - file_path: tmp.as_path().to_path_buf(), - read_only: false, - root_device: true, - advertise_flush: true, - }; - - let block_mutex = Block::new(args).unwrap(); - let mut block = block_mutex.lock().unwrap(); - - // The read-only feature should not be present. - assert_eq!(block.virtio_cfg.device_features & (1 << VIRTIO_BLK_F_RO), 0); - // The flush feature should be present. - assert_ne!( - block.virtio_cfg.device_features & (1 << VIRTIO_BLK_F_FLUSH), - 0 - ); - - // Some quick sanity checks. Most of the functionality around the device should be - // exercised/validated via integration tests. - - let range = mock.mmio_cfg.range; - - let bus_range = mock.mmio_mgr.mmio_device(range.base()).unwrap().0; - assert_eq!(bus_range.base(), range.base()); - assert_eq!(bus_range.size(), range.size()); - - assert_eq!( - mock.kernel_cmdline.as_str(), - format!( - "virtio_mmio.device=4K@0x{:x}:{} root=/dev/vda", - range.base().0, - mock.mmio_cfg.gsi - ) - ); - - assert_eq!(block.device_type(), BLOCK_DEVICE_ID); - - assert!(matches!(block.activate(), Err(Error::QueuesNotValid))); - - block.virtio_config_mut().device_activated = true; - assert_eq!(block.virtio_config().device_activated, true); - assert!(matches!(block.activate(), Err(Error::AlreadyActivated))); - } + let mut mock = EnvMock::new(); + let mut env = mock.env(); + let args = BlockArgs { + file_path: tmp.as_path().to_path_buf(), + read_only: true, + root_device: true, + advertise_flush: true, + }; - // Test a read-only root device. - { - let mut mock = CommonArgsMock::new(); - let common_args = mock.args(); - let args = BlockArgs { - common: common_args, - file_path: tmp.as_path().to_path_buf(), - read_only: true, - root_device: true, - advertise_flush: true, - }; - - let block_mutex = Block::new(args).unwrap(); - let block = block_mutex.lock().unwrap(); - - // The read-only feature should be present. - assert_ne!(block.virtio_cfg.device_features & (1 << VIRTIO_BLK_F_RO), 0); - - assert_eq!( - mock.kernel_cmdline.as_str(), - format!( - "virtio_mmio.device=4K@0x{:x}:{} root=/dev/vda ro", - mock.mmio_cfg.range.base().0, - mock.mmio_cfg.gsi - ) - ); - } + let block_mutex = Block::new(&mut env, &args).unwrap(); + let block = block_mutex.lock().unwrap(); - // Test a block device with root and advertise flush not enabled. - { - { - let mut mock = CommonArgsMock::new(); - let common_args = mock.args(); - let args = BlockArgs { - common: common_args, - file_path: tmp.as_path().to_path_buf(), - read_only: true, - root_device: false, - advertise_flush: false, - }; - - let block_mutex = Block::new(args).unwrap(); - let block = block_mutex.lock().unwrap(); - - // The read-only feature should be present. - assert_ne!(block.virtio_cfg.device_features & (1 << VIRTIO_BLK_F_RO), 0); - // The flush feature should not be present. - assert_eq!( - block.virtio_cfg.device_features & (1 << VIRTIO_BLK_F_FLUSH), - 0 - ); - - assert_eq!( - mock.kernel_cmdline.as_str(), - format!( - "virtio_mmio.device=4K@0x{:x}:{}", - mock.mmio_cfg.range.base().0, - mock.mmio_cfg.gsi - ) - ); - } - } + assert_eq!(block.device_type(), BLOCK_DEVICE_ID); + + assert_eq!( + mock.kernel_cmdline.as_str(), + format!( + "virtio_mmio.device=4K@0x{:x}:{} root=/dev/vda ro", + mock.mmio_cfg.range.base().0, + mock.mmio_cfg.gsi + ) + ); + + assert_ne!(block.cfg.virtio.device_features & (1 << VIRTIO_BLK_F_RO), 0); + assert_ne!( + block.cfg.virtio.device_features & (1 << VIRTIO_BLK_F_FLUSH), + 0 + ); } } diff --git a/src/devices/src/virtio/block/mod.rs b/src/devices/src/virtio/block/mod.rs index a943711a..b7a21c98 100644 --- a/src/devices/src/virtio/block/mod.rs +++ b/src/devices/src/virtio/block/mod.rs @@ -9,12 +9,9 @@ use std::fs::File; use std::io::{self, Seek, SeekFrom}; use std::path::{Path, PathBuf}; -use event_manager::Error as EvmgrError; -use vm_device::bus; use vm_virtio::block::stdio_executor; -use vmm_sys_util::errno; -use crate::virtio::CommonArgs; +use crate::virtio::features::{VIRTIO_F_IN_ORDER, VIRTIO_F_RING_EVENT_IDX, VIRTIO_F_VERSION_1}; pub use device::Block; @@ -33,17 +30,9 @@ const SECTOR_SHIFT: u8 = 9; #[derive(Debug)] pub enum Error { - AlreadyActivated, Backend(stdio_executor::Error), - BadFeatures(u64), - Bus(bus::Error), - Cmdline(linux_loader::cmdline::Error), - Endpoint(EvmgrError), - EventFd(io::Error), + Generic(crate::virtio::Error), OpenFile(io::Error), - QueuesNotValid, - RegisterIoevent(errno::Error), - RegisterIrqfd(errno::Error), Seek(io::Error), } @@ -67,14 +56,47 @@ fn build_config_space>(path: P) -> Result> { } // Arguments required when building a block device. -pub struct BlockArgs<'a, M, B> { - pub common: CommonArgs<'a, M, B>, +pub struct BlockArgs { pub file_path: PathBuf, pub read_only: bool, pub root_device: bool, pub advertise_flush: bool, } +impl BlockArgs { + // Generate device features based on the configuration options. + pub fn device_features(&self) -> u64 { + // The queue handling logic for the device uses the buffers in order, so we enable the + // corresponding feature as well. + let mut features = + 1 << VIRTIO_F_VERSION_1 | 1 << VIRTIO_F_IN_ORDER | 1 << VIRTIO_F_RING_EVENT_IDX; + + if self.read_only { + features |= 1 << VIRTIO_BLK_F_RO; + } + + if self.advertise_flush { + features |= 1 << VIRTIO_BLK_F_FLUSH; + } + + features + } + + // Generate additional info that needs to be appended to the kernel command line based + // on the current arg configuration. + pub fn cmdline_config_substring(&self) -> String { + let mut s = String::new(); + if self.root_device { + s.push_str("root=/dev/vda"); + + if self.read_only { + s.push_str(" ro"); + } + } + s + } +} + #[cfg(test)] mod tests { use std::io::Write; @@ -84,6 +106,17 @@ mod tests { use super::*; + impl Default for BlockArgs { + fn default() -> Self { + BlockArgs { + file_path: "".into(), + read_only: false, + root_device: false, + advertise_flush: false, + } + } + } + #[test] fn test_build_config_space() { let tmp = TempFile::new().unwrap(); @@ -113,4 +146,38 @@ mod tests { assert_eq!(config_space[..8], num_sectors.to_le_bytes()); } } + + #[test] + fn test_device_features() { + let mut args = BlockArgs::default(); + + let base = + 1u64 << VIRTIO_F_VERSION_1 | 1 << VIRTIO_F_IN_ORDER | 1 << VIRTIO_F_RING_EVENT_IDX; + + assert_eq!(args.device_features(), base); + + args.read_only = true; + assert_eq!(args.device_features(), base | 1 << VIRTIO_BLK_F_RO); + + args.read_only = false; + args.advertise_flush = true; + assert_eq!(args.device_features(), base | 1 << VIRTIO_BLK_F_FLUSH); + } + + #[test] + fn test_cmdline_string() { + let mut args = BlockArgs::default(); + + assert_eq!(args.cmdline_config_substring(), ""); + + args.read_only = true; + // There's no effect unless `root_device` is also `true`. + assert_eq!(args.cmdline_config_substring(), ""); + + args.root_device = true; + assert_eq!(args.cmdline_config_substring(), "root=/dev/vda ro"); + + args.read_only = false; + assert_eq!(args.cmdline_config_substring(), "root=/dev/vda"); + } } diff --git a/src/devices/src/virtio/mod.rs b/src/devices/src/virtio/mod.rs index fe7af102..df474c2f 100644 --- a/src/devices/src/virtio/mod.rs +++ b/src/devices/src/virtio/mod.rs @@ -5,14 +5,25 @@ pub mod block; +use std::convert::TryFrom; +use std::io; +use std::ops::DerefMut; use std::sync::atomic::{AtomicU8, Ordering}; use std::sync::{Arc, Mutex}; -use event_manager::{EventManager, MutEventSubscriber}; -use kvm_ioctls::VmFd; +use event_manager::{ + Error as EvmgrError, EventManager, MutEventSubscriber, RemoteEndpoint, Result as EvmgrResult, + SubscriberId, +}; +use kvm_ioctls::{IoEventAddress, VmFd}; use linux_loader::cmdline::Cmdline; -use vm_device::bus::MmioRange; -use vmm_sys_util::eventfd::EventFd; +use vm_device::bus::{self, MmioRange}; +use vm_device::device_manager::MmioManager; +use vm_device::DeviceMmio; +use vm_memory::{GuestAddress, GuestAddressSpace}; +use vm_virtio::device::VirtioConfig; +use vmm_sys_util::errno; +use vmm_sys_util::eventfd::{EventFd, EFD_NONBLOCK}; // TODO: Move virtio-related defines from the local modules to the `vm-virtio` crate upstream. @@ -38,6 +49,23 @@ const VIRTIO_MMIO_QUEUE_NOTIFY_OFFSET: u64 = 0x50; // TODO: Make configurable for each device maybe? const QUEUE_MAX_SIZE: u16 = 256; +// Common errors encountered during device creation, configuration, and operation. +#[derive(Debug)] +pub enum Error { + AlreadyActivated, + BadFeatures(u64), + Bus(bus::Error), + Cmdline(linux_loader::cmdline::Error), + Endpoint(EvmgrError), + EventFd(io::Error), + QueuesNotValid, + RegisterIoevent(errno::Error), + RegisterIrqfd(errno::Error), +} + +type Result = std::result::Result; +pub type Subscriber = Arc>; + #[derive(Copy, Clone)] pub struct MmioConfig { pub range: MmioRange, @@ -45,9 +73,11 @@ pub struct MmioConfig { pub gsi: u32, } -// These arguments are common for all virtio devices. We're always passing a mmio_cfg object -// for now, and we'll re-evaluate the layout of this struct when adding more transport options. -pub struct CommonArgs<'a, M, B> { +// Represents the environment the devices in this crate current expect in order to be created +// and registered with the appropriate buses/handlers/etc. We're always passing a mmio_cfg object +// for now, and we'll re-evaluate the mechanism for exposing environment (i.e. maybe we'll do it +// through an object that implements a number of traits the devices are aware of). +pub struct Env<'a, M, B> { // The objects used for guest memory accesses and other operations. pub mem: M, // Used by the devices to register ioevents and irqfds. @@ -67,6 +97,132 @@ pub struct CommonArgs<'a, M, B> { pub kernel_cmdline: &'a mut Cmdline, } +impl<'a, M, B> Env<'a, M, B> +where + // We're using this (more convoluted) bound so we can pass both references and smart + // pointers such as mutex guards here. + B: DerefMut, + B::Target: MmioManager>, +{ + // Registers an MMIO device with the inner bus and kernel cmdline. + pub fn register_mmio_device( + &mut self, + device: Arc, + ) -> Result<()> { + self.mmio_mgr + .register_mmio(self.mmio_cfg.range, device) + .map_err(Error::Bus)?; + + self.kernel_cmdline + .add_virtio_mmio_device( + self.mmio_cfg.range.size(), + GuestAddress(self.mmio_cfg.range.base().0), + self.mmio_cfg.gsi, + None, + ) + .map_err(Error::Cmdline)?; + + Ok(()) + } + + // Appends a string to the inner kernel cmdline. + pub fn insert_cmdline_str>(&mut self, t: T) -> Result<()> { + self.kernel_cmdline + .insert_str(t.as_ref()) + .map_err(Error::Cmdline) + } +} + +// Holds configuration objects which are common to all current devices. +pub struct CommonConfig { + pub virtio: VirtioConfig, + pub mmio: MmioConfig, + pub endpoint: RemoteEndpoint, + pub vm_fd: Arc, + pub irqfd: Arc, +} + +impl CommonConfig { + pub fn new(virtio_cfg: VirtioConfig, env: &Env) -> Result { + let irqfd = Arc::new(EventFd::new(EFD_NONBLOCK).map_err(Error::EventFd)?); + + env.vm_fd + .register_irqfd(&irqfd, env.mmio_cfg.gsi) + .map_err(Error::RegisterIrqfd)?; + + Ok(CommonConfig { + virtio: virtio_cfg, + mmio: env.mmio_cfg, + endpoint: env.event_mgr.remote_endpoint(), + vm_fd: env.vm_fd.clone(), + irqfd, + }) + } + + // Perform common initial steps for device activation based on the configuration, and return + // a `Vec` that contains `EventFd`s registered as ioeventfds, which are used to convey queue + // notifications coming from the driver. + pub fn prepare_activate(&self) -> Result> { + if !self.virtio.queues_valid() { + return Err(Error::QueuesNotValid); + } + + if self.virtio.device_activated { + return Err(Error::AlreadyActivated); + } + + // We do not support legacy drivers. + if self.virtio.driver_features & (1 << features::VIRTIO_F_VERSION_1) == 0 { + return Err(Error::BadFeatures(self.virtio.driver_features)); + } + + let mut ioevents = Vec::new(); + + // Right now, we operate under the assumption all queues are marked ready by the device + // (which is true until we start supporting devices that can optionally make use of + // additional queues on top of the defaults). + for i in 0..self.virtio.queues.len() { + let fd = EventFd::new(EFD_NONBLOCK).map_err(Error::EventFd)?; + + // Register the queue event fd. + self.vm_fd + .register_ioevent( + &fd, + &IoEventAddress::Mmio( + self.mmio.range.base().0 + VIRTIO_MMIO_QUEUE_NOTIFY_OFFSET, + ), + // The maximum number of queues should fit within an `u16` according to the + // standard, so the conversion below is always expected to succeed. + u32::try_from(i).unwrap(), + ) + .map_err(Error::RegisterIoevent)?; + + ioevents.push(fd); + } + + Ok(ioevents) + } + + // Perform the final steps of device activation based on the inner configuration and the + // provided subscriber that's going to handle the device queues. We'll extend this when + // we start support devices that make use of multiple handlers (i.e. for multiple queues). + pub fn finalize_activate(&mut self, handler: Subscriber) -> Result<()> { + // Register the queue handler with the `EventManager`. We could record the `sub_id` + // (and/or keep a handler clone) for further interaction (i.e. to remove the subscriber at + // a later time, retrieve state, etc). + let _sub_id = self + .endpoint + .call_blocking(move |mgr| -> EvmgrResult { + Ok(mgr.add_subscriber(handler)) + }) + .map_err(Error::Endpoint)?; + + self.virtio.device_activated = true; + + Ok(()) + } +} + /// Simple trait to model the operation of signalling the driver about used events /// for the specified queue. // TODO: Does this need renaming to be relevant for packed queues as well? @@ -97,8 +253,13 @@ impl SignalUsedQueue for SingleFdSignalQueue { pub(crate) mod tests { use vm_device::bus::MmioAddress; use vm_device::device_manager::IoManager; + use vm_device::MutDeviceMmio; use vm_memory::{GuestAddress, GuestMemoryMmap}; + use event_manager::{EventOps, Events}; + use vm_virtio::Queue; + + use super::features::VIRTIO_F_VERSION_1; use super::*; pub type MockMem = Arc; @@ -106,7 +267,7 @@ pub(crate) mod tests { // Can be used in other modules to test functionality that requires a `CommonArgs` struct as // input. The `args` method below generates an instance of `CommonArgs` based on the members // below. - pub struct CommonArgsMock { + pub struct EnvMock { pub mem: MockMem, pub vm_fd: Arc, pub event_mgr: EventManager>>, @@ -115,7 +276,7 @@ pub(crate) mod tests { pub kernel_cmdline: Cmdline, } - impl CommonArgsMock { + impl EnvMock { pub fn new() -> Self { let mem = Arc::new(GuestMemoryMmap::from_ranges(&[(GuestAddress(0), 0x1000_0000)]).unwrap()); @@ -129,7 +290,7 @@ pub(crate) mod tests { // Required so the vm_fd can be used to register irqfds. vm_fd.create_irq_chip().unwrap(); - CommonArgsMock { + EnvMock { mem, vm_fd, event_mgr: EventManager::new().unwrap(), @@ -140,8 +301,8 @@ pub(crate) mod tests { } } - pub fn args(&mut self) -> CommonArgs { - CommonArgs { + pub fn env(&mut self) -> Env { + Env { mem: self.mem.clone(), vm_fd: self.vm_fd.clone(), event_mgr: &mut self.event_mgr, @@ -151,4 +312,96 @@ pub(crate) mod tests { } } } + + #[test] + fn test_env() { + // Just a dummy device we're going to register on the bus. + struct Dummy; + + impl MutDeviceMmio for Dummy { + fn mmio_read(&mut self, _base: MmioAddress, _offset: u64, _data: &mut [u8]) {} + + fn mmio_write(&mut self, _base: MmioAddress, _offset: u64, _data: &[u8]) {} + } + + let mut mock = EnvMock::new(); + + let dummy = Arc::new(Mutex::new(Dummy)); + + mock.env().register_mmio_device(dummy).unwrap(); + + let range = mock.mmio_cfg.range; + let bus_range = mock.mmio_mgr.mmio_device(range.base()).unwrap().0; + assert_eq!(bus_range.base(), range.base()); + assert_eq!(bus_range.size(), range.size()); + + assert_eq!( + mock.kernel_cmdline.as_str(), + format!( + "virtio_mmio.device=4K@0x{:x}:{}", + range.base().0, + mock.mmio_cfg.gsi + ) + ); + + mock.env().insert_cmdline_str("ending_string").unwrap(); + assert!(mock.kernel_cmdline.as_str().ends_with("ending_string")); + } + + #[test] + fn test_common_config() { + let mut mock = EnvMock::new(); + let env = mock.env(); + + let device_features = 0; + let queues = vec![Queue::new(env.mem.clone(), 256)]; + let config_space = Vec::new(); + let virtio_cfg = VirtioConfig::new(device_features, queues, config_space); + + let mut cfg = CommonConfig::new(virtio_cfg, &env).unwrap(); + assert_eq!(cfg.virtio.device_activated, false); + + assert!(matches!(cfg.prepare_activate(), Err(Error::QueuesNotValid))); + + // Let's pretend the queue has been configured such that the `is_valid` check passes. + cfg.virtio.queues[0].ready = true; + cfg.virtio.queues[0].size = 256; + + // This will fail because the "driver" didn't acknowledge `VIRTIO_F_VERSION_1`. + assert!(matches!(cfg.prepare_activate(), Err(Error::BadFeatures(0)))); + + cfg.virtio.driver_features = 1 << VIRTIO_F_VERSION_1; + + cfg.virtio.device_activated = true; + assert!(matches!( + cfg.prepare_activate(), + Err(Error::AlreadyActivated) + )); + + cfg.virtio.device_activated = false; + let ioevents = cfg.prepare_activate().unwrap(); + assert_eq!(ioevents.len(), cfg.virtio.queues.len()); + + // Let's define a dummy subscriber to invoke `finalize_activate`. + struct Dummy; + + impl MutEventSubscriber for Dummy { + fn process(&mut self, _events: Events, _ops: &mut EventOps) {} + + fn init(&mut self, _ops: &mut EventOps) {} + } + + // `finalize_activate` attempts to register the subscriber using a remote endpoint and + // the associated `call_blocking` method, so let's start up a separate thread while + // waiting for one `EventManager` run loop to finish on the current one. + + let t = std::thread::spawn(move || { + cfg.finalize_activate(Arc::new(Mutex::new(Dummy))).unwrap(); + assert_eq!(cfg.virtio.device_activated, true); + }); + + assert_eq!(mock.event_mgr.run(), Ok(1)); + + t.join().unwrap(); + } } diff --git a/src/vm-vcpu/Cargo.toml b/src/vm-vcpu/Cargo.toml index a600246d..175d3a4c 100644 --- a/src/vm-vcpu/Cargo.toml +++ b/src/vm-vcpu/Cargo.toml @@ -11,7 +11,8 @@ libc = "0.2.76" kvm-bindings = { version = "0.3.0", features = ["fam-wrappers"] } kvm-ioctls = "0.5.0" vm-memory = { version = "0.4.0" } -vmm-sys-util = "0.6.1" +vmm-sys-util = "0.7.0" + utils = { path = "../utils" } # vm-device is not yet published on crates.io. diff --git a/src/vmm/Cargo.toml b/src/vmm/Cargo.toml index 0cec88d1..13d7f970 100644 --- a/src/vmm/Cargo.toml +++ b/src/vmm/Cargo.toml @@ -13,7 +13,7 @@ libc = "0.2.87" linux-loader = { version = "0.2.0", features = ["bzimage", "elf"] } vm-memory = { version = "0.4.0", features = ["backend-mmap"] } vm-superio = "0.1.1" -vmm-sys-util = "0.6.1" +vmm-sys-util = "0.7.0" # vm-device is not yet published on crates.io. # To make sure that breaking changes to vm-device are not breaking the diff --git a/src/vmm/src/lib.rs b/src/vmm/src/lib.rs index 6bd042e6..112f1685 100644 --- a/src/vmm/src/lib.rs +++ b/src/vmm/src/lib.rs @@ -42,7 +42,7 @@ use vmm_sys_util::{epoll::EventSet, eventfd::EventFd, terminal::Terminal}; use boot::build_bootparams; pub use config::*; use devices::virtio::block::{self, BlockArgs}; -use devices::virtio::{CommonArgs, MmioConfig}; +use devices::virtio::{Env, MmioConfig}; use serial::SerialWrapper; use vm_vcpu::vcpu::{cpuid::filter_cpuid, VcpuState}; use vm_vcpu::vm::{self, ExitHandler, KvmVm, VmState}; @@ -410,7 +410,7 @@ impl VMM { let mut guard = self.device_mgr.lock().unwrap(); - let common = CommonArgs { + let mut env = Env { mem, vm_fd: self.vm.vm_fd(), event_mgr: &mut self.event_mgr, @@ -420,7 +420,6 @@ impl VMM { }; let args = BlockArgs { - common, file_path: PathBuf::from(&cfg.path), read_only: false, root_device: true, @@ -428,7 +427,7 @@ impl VMM { }; // We can also hold this somewhere if we need to keep the handle for later. - let block = Block::new(args).map_err(Error::Block)?; + let block = Block::new(&mut env, &args).map_err(Error::Block)?; self.block_devices.push(block); Ok(())