diff --git a/CHANGELOG.md b/CHANGELOG.md index 75077c0c652..eb3a0617c82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,11 @@ and this project adheres to Terminating a connection now also discards its TX buffer, so the device stops advertising `EPOLLOUT` for a host stream it will never write to again, which could otherwise busy-spin the event thread indefinitely. +- [#6083](https://github.com/firecracker-microvm/firecracker/pull/6083): Fixed a + vhost-user-block device backed by a readonly backend not being treated as + readonly. The `VIRTIO_BLK_F_RO` check read the acked feature set after it had + been narrowed to the vhost-user protocol bit, so it never matched, and a + readonly vhost-user root device was given `rw` on the guest kernel cmdline. - [#6086](https://github.com/firecracker-microvm/firecracker/pull/6086), [#6143](https://github.com/firecracker-microvm/firecracker/pull/6143): Fixed a deadlock in the logger: a signal handler that logs while the interrupted diff --git a/src/vmm/src/devices/virtio/block/device.rs b/src/vmm/src/devices/virtio/block/device.rs index 50a8084448e..ea1aaf22dc0 100644 --- a/src/vmm/src/devices/virtio/block/device.rs +++ b/src/vmm/src/devices/virtio/block/device.rs @@ -129,42 +129,42 @@ impl VirtioDevice for Block { fn avail_features(&self) -> u64 { match self { Self::Virtio(b) => b.avail_features, - Self::VhostUser(b) => b.avail_features, + Self::VhostUser(b) => b.avail_features(), } } fn acked_features(&self) -> u64 { match self { Self::Virtio(b) => b.acked_features, - Self::VhostUser(b) => b.acked_features, + Self::VhostUser(b) => b.acked_features(), } } fn set_acked_features(&mut self, acked_features: u64) { match self { Self::Virtio(b) => b.acked_features = acked_features, - Self::VhostUser(b) => b.acked_features = acked_features, + Self::VhostUser(b) => b.set_acked_features(acked_features), } } fn queues(&self) -> &[Queue] { match self { Self::Virtio(b) => &b.queues, - Self::VhostUser(b) => &b.queues, + Self::VhostUser(b) => b.queues(), } } fn queues_mut(&mut self) -> &mut [Queue] { match self { Self::Virtio(b) => &mut b.queues, - Self::VhostUser(b) => &mut b.queues, + Self::VhostUser(b) => b.queues_mut(), } } fn queue_events(&self) -> &[EventFd] { match self { Self::Virtio(b) => &b.queue_evts, - Self::VhostUser(b) => &b.queue_evts, + Self::VhostUser(b) => b.queue_events(), } } @@ -203,7 +203,7 @@ impl VirtioDevice for Block { fn is_activated(&self) -> bool { match self { Self::Virtio(b) => b.device_state.is_activated(), - Self::VhostUser(b) => b.device_state.is_activated(), + Self::VhostUser(b) => b.is_activated(), } } diff --git a/src/vmm/src/devices/virtio/block/vhost_user/device.rs b/src/vmm/src/devices/virtio/block/vhost_user/device.rs index 65d0b0a08de..0c6bd580c3f 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -4,10 +4,8 @@ // Portions Copyright 2019 Intel Corporation. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::ops::Deref; use std::sync::Arc; -use utils::time::{ClockType, get_time_us}; use vhost::vhost_user::Frontend; use vhost::vhost_user::message::*; use vmm_sys_util::eventfd::EventFd; @@ -15,18 +13,16 @@ use vmm_sys_util::eventfd::EventFd; use super::{NUM_QUEUES, QUEUE_SIZE, VhostUserBlockError}; use crate::devices::virtio::ActivateError; use crate::devices::virtio::block::CacheType; -use crate::devices::virtio::device::{ActiveState, DeviceState, VirtioDevice, VirtioDeviceType}; +use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceType}; use crate::devices::virtio::generated::virtio_blk::{VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_RO}; use crate::devices::virtio::generated::virtio_config::VIRTIO_F_VERSION_1; use crate::devices::virtio::generated::virtio_ring::VIRTIO_RING_F_EVENT_IDX; use crate::devices::virtio::queue::Queue; -use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; -use crate::devices::virtio::vhost_user::{VhostUserHandleBackend, VhostUserHandleImpl}; -use crate::devices::virtio::vhost_user_metrics::{ - VhostUserDeviceMetrics, VhostUserMetricsPerDevice, +use crate::devices::virtio::transport::VirtioInterrupt; +use crate::devices::virtio::vhost_user::{ + VhostUserDevice, VhostUserDeviceSpec, VhostUserHandleBackend, }; -use crate::logger::{IncMetric, StoreMetric, log_dev_preview_warning}; -use crate::utils::u64_to_usize; +use crate::logger::log_dev_preview_warning; use crate::vmm_config::drive::BlockDeviceConfig; use crate::vstate::memory::GuestMemoryMmap; use crate::{MutEventSubscriber, impl_device_type}; @@ -110,16 +106,8 @@ pub type VhostUserBlock = VhostUserBlockImpl; /// vhost-user block device. pub struct VhostUserBlockImpl { - // Virtio fields. - pub avail_features: u64, - pub acked_features: u64, - pub config_space: Vec, - pub activate_evt: EventFd, - - // Transport related fields. - pub queues: Vec, - pub queue_evts: [EventFd; u64_to_usize(NUM_QUEUES)], - pub device_state: DeviceState, + // Everything that is not specific to block living in the generic frontend. + pub vu_device: VhostUserDevice, // Implementation specific fields. pub id: String, @@ -127,35 +115,18 @@ pub struct VhostUserBlockImpl { pub cache_type: CacheType, pub root_device: bool, pub read_only: bool, - - // Vhost user protocol handle - pub vu_handle: VhostUserHandleImpl, - pub vu_acked_protocol_features: u64, - pub metrics: Arc, } // Need custom implementation because otherwise `Debug` is required for `vhost::Master` impl std::fmt::Debug for VhostUserBlockImpl { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("VhostUserBlockImpl") - .field("avail_features", &self.avail_features) - .field("acked_features", &self.acked_features) - .field("config_space", &self.config_space) - .field("activate_evt", &self.activate_evt) - .field("queues", &self.queues) - .field("queue_evts", &self.queue_evts) - .field("device_state", &self.device_state) + .field("vu_device", &self.vu_device) .field("id", &self.id) .field("partuuid", &self.partuuid) .field("cache_type", &self.cache_type) .field("root_device", &self.root_device) .field("read_only", &self.read_only) - .field("vu_handle", &self.vu_handle) - .field( - "vu_acked_protocol_features", - &self.vu_acked_protocol_features, - ) - .field("metrics", &self.metrics) .finish() } } @@ -163,78 +134,36 @@ impl std::fmt::Debug for VhostUserBlockImpl { impl VhostUserBlockImpl { pub fn new(config: VhostUserBlockConfig) -> Result { log_dev_preview_warning("vhost-user-blk device", Option::None); - let start_time = get_time_us(ClockType::Monotonic); - let mut requested_features = AVAILABLE_FEATURES; + let mut avail_features = AVAILABLE_FEATURES; if config.cache_type == CacheType::Writeback { - requested_features |= 1 << VIRTIO_BLK_F_FLUSH; + avail_features |= 1 << VIRTIO_BLK_F_FLUSH; } - let requested_protocol_features = VhostUserProtocolFeatures::CONFIG; - - let mut vu_handle = VhostUserHandleImpl::::new(&config.socket, NUM_QUEUES) - .map_err(VhostUserBlockError::VhostUser)?; - let (acked_features, acked_protocol_features) = vu_handle - .negotiate_features(requested_features, requested_protocol_features) - .map_err(VhostUserBlockError::VhostUser)?; - - // Get config from backend if CONFIG is acked or use empty buffer. - let config_space = - if acked_protocol_features & VhostUserProtocolFeatures::CONFIG.bits() != 0 { - // This buffer is used for config size check in vhost crate. - let buffer = [0u8; BLOCK_CONFIG_SPACE_SIZE as usize]; - let (_, new_config_space) = vu_handle - .vu - .get_config( - 0, - BLOCK_CONFIG_SPACE_SIZE, - VhostUserConfigFlags::WRITABLE, - &buffer, - ) - .map_err(VhostUserBlockError::Vhost)?; - new_config_space - } else { - vec![] - }; - - let activate_evt = - EventFd::new(libc::EFD_NONBLOCK).map_err(VhostUserBlockError::EventFd)?; - - let queues = vec![Queue::new(QUEUE_SIZE)]; - let queue_evts = [EventFd::new(libc::EFD_NONBLOCK).map_err(VhostUserBlockError::EventFd)?; - u64_to_usize(NUM_QUEUES)]; - let device_state = DeviceState::Inactive; - - // We negotiated features with backend. Now these acked_features - // are available for guest driver to choose from. - let avail_features = acked_features; - let acked_features = acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits(); - let read_only = acked_features & (1 << VIRTIO_BLK_F_RO) != 0; - let vhost_user_block_metrics_name = format!("block_{}", config.drive_id); - - let metrics = VhostUserMetricsPerDevice::alloc(vhost_user_block_metrics_name); - let delta_us = get_time_us(ClockType::Monotonic) - start_time; - metrics.init_time_us.store(delta_us); - - Ok(Self { + let vu_device = VhostUserDevice::::new(VhostUserDeviceSpec { + socket: config.socket, + num_queues: NUM_QUEUES, + queue_size: QUEUE_SIZE, avail_features, - acked_features, - config_space, - activate_evt, + config_space_size: BLOCK_CONFIG_SPACE_SIZE, + // A backend that does not implement CONFIG leaves the config space + // empty, which the guest driver reads as a zero-capacity disk. + require_config: false, + metrics_name: format!("block_{}", config.drive_id), + })?; - queues, - queue_evts, - device_state, + // What the backend acked is what the guest driver gets offered, so this + // is where a readonly backend shows up. + let read_only = vu_device.avail_features & (1 << VIRTIO_BLK_F_RO) != 0; + + Ok(Self { + vu_device, id: config.drive_id, partuuid: config.partuuid, cache_type: config.cache_type, read_only, root_device: config.is_root_device, - - vu_handle, - vu_acked_protocol_features: acked_protocol_features, - metrics, }) } @@ -249,40 +178,12 @@ impl VhostUserBlockImpl { partuuid: self.partuuid.clone(), is_root_device: self.root_device, cache_type: self.cache_type, - socket: self.vu_handle.socket_path.clone(), + socket: self.vu_device.socket_path().to_string(), } } pub fn config_update(&mut self) -> Result<(), VhostUserBlockError> { - let start_time = get_time_us(ClockType::Monotonic); - let interrupt = self - .device_state - .active_state() - .expect("Device is not initialized") - .interrupt - .clone(); - - // This buffer is used for config size check in vhost crate. - let buffer = [0u8; BLOCK_CONFIG_SPACE_SIZE as usize]; - let (_, new_config_space) = self - .vu_handle - .vu - .get_config( - 0, - BLOCK_CONFIG_SPACE_SIZE, - VhostUserConfigFlags::WRITABLE, - &buffer, - ) - .map_err(VhostUserBlockError::Vhost)?; - self.config_space = new_config_space; - interrupt - .trigger(VirtioInterruptType::Config) - .map_err(VhostUserBlockError::Interrupt)?; - - let delta_us = get_time_us(ClockType::Monotonic) - start_time; - self.metrics.config_change_time_us.store(delta_us); - - Ok(()) + Ok(self.vu_device.refresh_config()?) } } @@ -297,39 +198,35 @@ where } fn avail_features(&self) -> u64 { - self.avail_features + self.vu_device.avail_features } fn acked_features(&self) -> u64 { - self.acked_features + self.vu_device.acked_features } fn set_acked_features(&mut self, acked_features: u64) { - self.acked_features = acked_features; + self.vu_device.acked_features = acked_features; } fn queues(&self) -> &[Queue] { - &self.queues + &self.vu_device.queues } fn queues_mut(&mut self) -> &mut [Queue] { - &mut self.queues + &mut self.vu_device.queues } fn queue_events(&self) -> &[EventFd] { - &self.queue_evts + &self.vu_device.queue_evts } fn interrupt_trigger(&self) -> &dyn VirtioInterrupt { - self.device_state - .active_state() - .expect("Device is not initialized") - .interrupt - .deref() + self.vu_device.interrupt() } fn config_as_bytes(&self) -> &[u8] { - self.config_space.as_slice() + self.vu_device.config_space.as_slice() } fn write_config(&mut self, _offset: u64, _data: &[u8]) { @@ -343,41 +240,15 @@ where mem: GuestMemoryMmap, interrupt: Arc, ) -> Result<(), ActivateError> { - assert!(!self.is_activated()); - - for q in self.queues.iter_mut() { - q.initialize(&mem) - .map_err(ActivateError::QueueMemoryError)?; - } - - let start_time = get_time_us(ClockType::Monotonic); - // Setting features again, because now we negotiated them - // with guest driver as well. - self.vu_handle - .set_features(self.acked_features) - .and_then(|()| { - self.vu_handle.setup_backend( - &mem, - &[(0, &self.queues[0], &self.queue_evts[0])], - interrupt.clone(), - ) - }) - .map_err(|err| { - self.metrics.activate_fails.inc(); - ActivateError::VhostUser(err) - })?; - self.device_state = DeviceState::Activated(ActiveState { mem, interrupt }); - let delta_us = get_time_us(ClockType::Monotonic) - start_time; - self.metrics.activate_time_us.store(delta_us); - Ok(()) + self.vu_device.activate(mem, interrupt) } fn is_activated(&self) -> bool { - self.device_state.is_activated() + self.vu_device.is_activated() } fn deactivate(&mut self) { - self.device_state = DeviceState::Inactive; + self.vu_device.deactivate(); } fn _reset(&mut self) -> bool { @@ -398,6 +269,7 @@ mod tests { use super::*; use crate::devices::virtio::block::virtio::device::FileEngineType; + use crate::devices::virtio::device::{ActiveState, DeviceState}; use crate::devices::virtio::test_utils::{VirtQueue, default_interrupt, default_mem}; use crate::devices::virtio::transport::mmio::VIRTIO_MMIO_INT_CONFIG; use crate::devices::virtio::vhost_user::tests::create_mem; @@ -521,6 +393,7 @@ mod tests { // no flags should be set. assert_eq!( vhost_block + .vu_device .vu_handle .vu .sock @@ -532,18 +405,18 @@ mod tests { .unwrap(), &tmp_socket_path, ); - assert_eq!(vhost_block.vu_handle.vu.max_queue_num, NUM_QUEUES); - assert!(unsafe { *vhost_block.vu_handle.vu.is_owner.get() }); - assert_eq!(vhost_block.avail_features, 0); - assert_eq!(vhost_block.acked_features, 0); - assert_eq!(vhost_block.vu_acked_protocol_features, 0); + assert_eq!(vhost_block.vu_device.vu_handle.vu.max_queue_num, NUM_QUEUES); + assert!(unsafe { *vhost_block.vu_device.vu_handle.vu.is_owner.get() }); + assert_eq!(vhost_block.vu_device.avail_features, 0); + assert_eq!(vhost_block.vu_device.acked_features, 0); + assert_eq!(vhost_block.vu_device.vu_acked_protocol_features, 0); assert_eq!( - unsafe { &*vhost_block.vu_handle.vu.hdr_flags.get() }.bits(), + unsafe { &*vhost_block.vu_device.vu_handle.vu.hdr_flags.get() }.bits(), VhostUserHeaderFlag::empty().bits() ); assert!(!vhost_block.root_device); assert!(!vhost_block.read_only); - assert_eq!(vhost_block.config_space, Vec::::new()); + assert_eq!(vhost_block.vu_device.config_space, Vec::::new()); } #[test] @@ -598,11 +471,15 @@ mod tests { fn get_config( &mut self, _offset: u32, - _size: u32, + size: u32, _flags: VhostUserConfigFlags, _buf: &[u8], ) -> Result<(VhostUserConfig, VhostUserConfigPayload), vhost::Error> { - Ok((VhostUserConfig::default(), vec![0x69, 0x69, 0x69])) + // The frontend requires the backend to answer with as many + // bytes as were asked for. + let mut config = vec![0x69, 0x69, 0x69]; + config.resize(size as usize, 0); + Ok((VhostUserConfig::default(), config)) } } @@ -626,6 +503,7 @@ mod tests { // should be negotiated and header flags should be set. assert_eq!( vhost_block + .vu_device .vu_handle .vu .sock @@ -637,28 +515,35 @@ mod tests { .unwrap(), &tmp_socket_path, ); - assert_eq!(vhost_block.vu_handle.vu.max_queue_num, NUM_QUEUES); - assert!(unsafe { *vhost_block.vu_handle.vu.is_owner.get() }); + assert_eq!(vhost_block.vu_device.vu_handle.vu.max_queue_num, NUM_QUEUES); + assert!(unsafe { *vhost_block.vu_device.vu_handle.vu.is_owner.get() }); assert_eq!( - vhost_block.avail_features, + vhost_block.vu_device.avail_features, AVAILABLE_FEATURES | (1 << VIRTIO_BLK_F_FLUSH) ); assert_eq!( - vhost_block.acked_features, + vhost_block.vu_device.acked_features, VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits() ); assert_eq!( - vhost_block.vu_acked_protocol_features, + vhost_block.vu_device.vu_acked_protocol_features, VhostUserProtocolFeatures::CONFIG.bits() ); assert_eq!( - unsafe { &*vhost_block.vu_handle.vu.hdr_flags.get() }.bits(), + unsafe { &*vhost_block.vu_device.vu_handle.vu.hdr_flags.get() }.bits(), VhostUserHeaderFlag::empty().bits() ); assert!(!vhost_block.root_device); - assert!(!vhost_block.read_only); - assert_eq!(vhost_block.config_space, vec![0x69, 0x69, 0x69]); + assert!(vhost_block.read_only); + assert_eq!( + vhost_block.vu_device.config_space.len(), + BLOCK_CONFIG_SPACE_SIZE as usize + ); + assert_eq!( + &vhost_block.vu_device.config_space[..3], + &[0x69, 0x69, 0x69] + ); // Test some `VirtioDevice` methods assert_eq!( @@ -682,16 +567,30 @@ mod tests { // Writing to the config does nothing vhost_block.write_config(0x69, &[0]); - assert_eq!(vhost_block.config_space, vec![0x69, 0x69, 0x69]); + assert_eq!( + vhost_block.vu_device.config_space.len(), + BLOCK_CONFIG_SPACE_SIZE as usize + ); + assert_eq!( + &vhost_block.vu_device.config_space[..3], + &[0x69, 0x69, 0x69] + ); // Testing [`config_update`] - vhost_block.device_state = DeviceState::Activated(ActiveState { + vhost_block.vu_device.device_state = DeviceState::Activated(ActiveState { mem: default_mem(), interrupt: default_interrupt(), }); - vhost_block.config_space = vec![]; + vhost_block.vu_device.config_space = vec![]; vhost_block.config_update().unwrap(); - assert_eq!(vhost_block.config_space, vec![0x69, 0x69, 0x69]); + assert_eq!( + vhost_block.vu_device.config_space.len(), + BLOCK_CONFIG_SPACE_SIZE as usize + ); + assert_eq!( + &vhost_block.vu_device.config_space[..3], + &[0x69, 0x69, 0x69] + ); assert_eq!( vhost_block.interrupt_status().load(Ordering::SeqCst), VIRTIO_MMIO_INT_CONFIG @@ -824,14 +723,14 @@ mod tests { let regions = vec![(GuestAddress(0x0), region_size)]; let guest_memory = create_mem(file, ®ions); let q = VirtQueue::new(GuestAddress(0), &guest_memory, 16); - vhost_block.queues[0] = q.create_queue(); + vhost_block.vu_device.queues[0] = q.create_queue(); let interrupt = default_interrupt(); // During actiavion of the device features, memory and queues should be set and activated. vhost_block.activate(guest_memory, interrupt).unwrap(); - assert!(unsafe { *vhost_block.vu_handle.vu.features_are_set.get() }); - assert!(unsafe { *vhost_block.vu_handle.vu.memory_is_set.get() }); - assert!(unsafe { *vhost_block.vu_handle.vu.vring_enabled.get() }); + assert!(unsafe { *vhost_block.vu_device.vu_handle.vu.features_are_set.get() }); + assert!(unsafe { *vhost_block.vu_device.vu_handle.vu.memory_is_set.get() }); + assert!(unsafe { *vhost_block.vu_device.vu_handle.vu.vring_enabled.get() }); assert!(vhost_block.is_activated()); } } diff --git a/src/vmm/src/devices/virtio/block/vhost_user/event_handler.rs b/src/vmm/src/devices/virtio/block/vhost_user/event_handler.rs index 4f143995630..57d32839a73 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/event_handler.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/event_handler.rs @@ -12,7 +12,7 @@ impl VhostUserBlock { fn register_activate_event(&self, ops: &mut EventOps) { if let Err(err) = ops.add(Events::with_data( - &self.activate_evt, + &self.vu_device.activate_evt, Self::PROCESS_ACTIVATE, EventSet::IN, )) { @@ -21,11 +21,11 @@ impl VhostUserBlock { } fn process_activate_event(&self, ops: &mut EventOps) { - if let Err(err) = self.activate_evt.read() { + if let Err(err) = self.vu_device.activate_evt.read() { error!("Failed to consume block activate event: {:?}", err); } if let Err(err) = ops.remove(Events::with_data( - &self.activate_evt, + &self.vu_device.activate_evt, Self::PROCESS_ACTIVATE, EventSet::IN, )) { diff --git a/src/vmm/src/devices/virtio/block/vhost_user/mod.rs b/src/vmm/src/devices/virtio/block/vhost_user/mod.rs index 0cd6d46f0ae..54b2c004233 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/mod.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/mod.rs @@ -6,7 +6,7 @@ pub mod event_handler; pub mod persist; use self::device::VhostUserBlock; -use crate::devices::virtio::vhost_user::VhostUserError; +use crate::devices::virtio::vhost_user::{VhostUserDeviceError, VhostUserError}; use crate::vstate::interrupts::InterruptError; /// Number of queues for the vhost-user block device. @@ -30,4 +30,28 @@ pub enum VhostUserBlockError { EventFd(std::io::Error), /// Error creating irqfd: {0} Interrupt(InterruptError), + /// Vhost-user device error: {0} + VhostUserDevice(VhostUserDeviceError), +} + +impl From for VhostUserBlockError { + fn from(err: VhostUserDeviceError) -> Self { + match err { + VhostUserDeviceError::VhostUser(err) => Self::VhostUser(err), + VhostUserDeviceError::GetConfig(err) => Self::Vhost(err), + VhostUserDeviceError::EventFd(err) => Self::EventFd(err), + VhostUserDeviceError::Interrupt(err) => Self::Interrupt(err), + // Block builds its spec from constants and treats CONFIG as + // optional, so the only one of these it can actually hit is a + // backend under-filling the config space. They are listed rather + // than caught by a wildcard so that a new variant on the generic + // error does not compile until it has been considered here. + err @ (VhostUserDeviceError::InvalidNumQueues(_) + | VhostUserDeviceError::InvalidConfigSpaceSize(_) + | VhostUserDeviceError::InvalidQueueSize(_) + | VhostUserDeviceError::TooManyQueues(..) + | VhostUserDeviceError::ShortConfigSpace(..) + | VhostUserDeviceError::ConfigFeatureNotNegotiated) => Self::VhostUserDevice(err), + } + } } diff --git a/src/vmm/src/devices/virtio/vhost_user.rs b/src/vmm/src/devices/virtio/vhost_user.rs index 1792824ae83..c42dcc49b08 100644 --- a/src/vmm/src/devices/virtio/vhost_user.rs +++ b/src/vmm/src/devices/virtio/vhost_user.rs @@ -4,18 +4,28 @@ // Portions Copyright 2019 Intel Corporation. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +use std::ops::Deref; use std::os::fd::AsRawFd; use std::os::unix::net::UnixStream; use std::sync::Arc; +use utils::time::{ClockType, get_time_us}; use vhost::vhost_user::message::*; use vhost::vhost_user::{Frontend, VhostUserFrontend}; use vhost::{Error as VhostError, VhostBackend, VhostUserMemoryRegionInfo, VringConfigData}; use vm_memory::{Address, GuestMemoryBackend, GuestMemoryError, GuestMemoryRegion}; use vmm_sys_util::eventfd::EventFd; -use crate::devices::virtio::queue::Queue; +use crate::devices::virtio::ActivateError; +use crate::devices::virtio::device::{ActiveState, DeviceState}; +use crate::devices::virtio::queue::{Queue, QueueError}; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; +use crate::devices::virtio::vhost_user_metrics::{ + VhostUserDeviceMetrics, VhostUserMetricsPerDevice, +}; +use crate::logger::{IncMetric, StoreMetric, debug}; +use crate::utils::u64_to_usize; +use crate::vstate::interrupts::InterruptError; use crate::vstate::memory::GuestMemoryMmap; /// vhost-user error. @@ -466,6 +476,348 @@ impl VhostUserHandleImpl { } } +/// Largest number of queues a vhost-user device can be given. +/// +/// The binding constraint is the PCI notification region: a dword per queue +/// in a 4KiB capability, so 1024 queues. MSI-X is looser, one vector per +/// queue plus one for configuration out of the 2048 a device may have. +const MAX_QUEUES: u64 = 1024; + +/// How a device type configures its vhost-user frontend. +/// +/// Everything that varies by virtio device type is supplied here, so that +/// [`VhostUserDevice`] itself stays device-type agnostic. +#[derive(Debug)] +pub struct VhostUserDeviceSpec { + /// Path of the backend's Unix socket. + pub socket: String, + /// Number of virtqueues to allocate. + pub num_queues: u64, + /// Size of each virtqueue. + pub queue_size: u16, + /// Virtio features to offer the backend, device-specific bits included. + /// Whatever the backend acks is what the guest driver is then offered. + pub avail_features: u64, + /// Size of the config space to fetch from the backend, which has to return + /// exactly this many bytes. So this is the device type's config space size + /// and not an upper bound. + pub config_space_size: u32, + /// Whether the CONFIG protocol feature is mandatory. Frontends with no + /// device-specific fallback for the config space require it. + pub require_config: bool, + /// Name to report this device's metrics under. + pub metrics_name: String, +} + +/// Error building a vhost-user frontend. +#[derive(Debug, thiserror::Error, displaydoc::Display)] +pub enum VhostUserDeviceError { + /// A vhost-user device needs at least one queue, got {0} + InvalidNumQueues(u64), + /// Config space size must be between 1 and 4096 bytes, got {0} + InvalidConfigSpaceSize(u32), + /// A vhost-user device supports at most {1} queues, got {0} + TooManyQueues(u64, u64), + /// Queue size must be a power of two, got {0} + InvalidQueueSize(u16), + /// Backend returned {0} bytes of config space, expected {1} + ShortConfigSpace(usize, u32), + /// Backend did not negotiate the mandatory CONFIG protocol feature + ConfigFeatureNotNegotiated, + /// Vhost-user: {0} + VhostUser(VhostUserError), + /// Failed to get config space from the backend: {0} + GetConfig(VhostError), + /// Failed to create eventfd: {0} + EventFd(std::io::Error), + /// Failed to signal the guest driver: {0} + Interrupt(InterruptError), +} + +/// Device-type agnostic vhost-user frontend. +/// +/// Owns the parts of a vhost-user frontend that do not depend on which virtio +/// device type is being implemented: the backend handle, the negotiated +/// features and config space, and the virtqueues. Device types embed this and +/// add their own state alongside it. +pub struct VhostUserDevice { + pub avail_features: u64, + pub acked_features: u64, + /// Config space fetched from the backend, empty if CONFIG was not acked. + pub config_space: Vec, + config_space_size: u32, + pub activate_evt: EventFd, + pub queues: Vec, + pub queue_evts: Vec, + pub device_state: DeviceState, + pub vu_handle: VhostUserHandleImpl, + pub vu_acked_protocol_features: u64, + pub metrics: Arc, +} + +// Custom because `Debug` is not derivable through `vhost`'s `Frontend`. +impl std::fmt::Debug for VhostUserDevice { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("VhostUserDevice") + .field("avail_features", &self.avail_features) + .field("acked_features", &self.acked_features) + .field("config_space", &self.config_space) + .field("config_space_size", &self.config_space_size) + .field("activate_evt", &self.activate_evt) + .field("queues", &self.queues) + .field("queue_evts", &self.queue_evts) + .field("device_state", &self.device_state) + .field("vu_handle", &self.vu_handle) + .field( + "vu_acked_protocol_features", + &self.vu_acked_protocol_features, + ) + .field("metrics", &self.metrics) + .finish() + } +} + +impl VhostUserDevice { + /// Connect to the backend, negotiate features, fetch the config space and + /// allocate the queues. + pub fn new(spec: VhostUserDeviceSpec) -> Result { + // Device-specific minimums (virtio-fs wants a hiprio queue plus at + // least one request queue, say) are the caller's business. + if spec.num_queues == 0 { + return Err(VhostUserDeviceError::InvalidNumQueues(spec.num_queues)); + } + + // One MSI-X vector per queue plus one for configuration, and the PCI + // transport allows 2048 vectors per device. Rejecting this here turns + // what would otherwise be an eventfd per queue followed by a failed + // u16 conversion during activation into an error the caller sees. + if spec.num_queues > MAX_QUEUES { + return Err(VhostUserDeviceError::TooManyQueues( + spec.num_queues, + MAX_QUEUES, + )); + } + + // Virtio requires a power of two. Nothing else checks the size a + // device is built with, only the smaller size a driver later selects, + // so an unusable queue would otherwise surface as a guest that + // silently refuses to probe the device. + if !spec.queue_size.is_power_of_two() { + return Err(VhostUserDeviceError::InvalidQueueSize(spec.queue_size)); + } + + // Both ends are rejected by the vhost crate, the lower one only once a + // backend acks CONFIG. Checking here keeps that from depending on which + // backend we are talking to, and avoids allocating the buffer first. + if spec.config_space_size == 0 || spec.config_space_size > VHOST_USER_CONFIG_SIZE { + return Err(VhostUserDeviceError::InvalidConfigSpaceSize( + spec.config_space_size, + )); + } + + let start_time = get_time_us(ClockType::Monotonic); + + let mut vu_handle = VhostUserHandleImpl::::new(&spec.socket, spec.num_queues) + .map_err(VhostUserDeviceError::VhostUser)?; + let (acked_features, vu_acked_protocol_features) = vu_handle + .negotiate_features(spec.avail_features, VhostUserProtocolFeatures::CONFIG) + .map_err(VhostUserDeviceError::VhostUser)?; + + let config_acked = + vu_acked_protocol_features & VhostUserProtocolFeatures::CONFIG.bits() != 0; + if spec.require_config && !config_acked { + return Err(VhostUserDeviceError::ConfigFeatureNotNegotiated); + } + + let config_space = if config_acked { + get_config_space(&mut vu_handle, spec.config_space_size)? + } else { + vec![] + }; + + let activate_evt = + EventFd::new(libc::EFD_NONBLOCK).map_err(VhostUserDeviceError::EventFd)?; + + let num_queues = u64_to_usize(spec.num_queues); + let queues = vec![Queue::new(spec.queue_size); num_queues]; + let queue_evts = (0..num_queues) + .map(|_| EventFd::new(libc::EFD_NONBLOCK).map_err(VhostUserDeviceError::EventFd)) + .collect::, _>>()?; + + // What the backend acked is what the guest driver may choose from. + let avail_features = acked_features; + let acked_features = acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits(); + + let metrics = VhostUserMetricsPerDevice::alloc(spec.metrics_name); + metrics + .init_time_us + .store(get_time_us(ClockType::Monotonic) - start_time); + + Ok(Self { + avail_features, + acked_features, + config_space, + config_space_size: spec.config_space_size, + activate_evt, + queues, + queue_evts, + device_state: DeviceState::Inactive, + vu_handle, + vu_acked_protocol_features, + metrics, + }) + } + + /// Set up the backend's vrings for the queues the guest marked ready. + pub fn activate( + &mut self, + mem: GuestMemoryMmap, + interrupt: Arc, + ) -> Result<(), ActivateError> { + assert!(!self.is_activated()); + + // A driver only initialises the queues it intends to use, and how many + // that is comes from the backend-owned config space rather than from + // the configured queue count (virtio-fs scales its request queues to + // the vCPU count, for example). Initializing a queue the guest never + // configured returns NotReady and aborts activation, so only the ready + // ones are set up here. Real vring indices are preserved, so queues 0 + // and 2 being ready maps to vrings 0 and 2 rather than 0 and 1. + let ready: Vec = self + .queues + .iter() + .enumerate() + .filter(|(_, queue)| queue.ready) + .map(|(i, _)| i) + .collect(); + + // Activating with nothing ready would otherwise set up no vrings at + // all and report success. + if ready.is_empty() { + return Err(ActivateError::QueueMemoryError(QueueError::NotReady)); + } + + if ready.len() < self.queues.len() { + // The queue count a driver uses comes from the backend-owned config + // space, so being given more than it wants is normal rather than a + // problem worth warning about. + debug!( + "vhost-user: setting up {} of {} configured vrings, the guest driver did not \ + ready the rest", + ready.len(), + self.queues.len() + ); + } + + for &i in &ready { + self.queues[i] + .initialize(&mem) + .map_err(ActivateError::QueueMemoryError)?; + } + + let start_time = get_time_us(ClockType::Monotonic); + let queue_refs: Vec<(usize, &Queue, &EventFd)> = ready + .iter() + .map(|&i| (i, &self.queues[i], &self.queue_evts[i])) + .collect(); + + // Set the features again, now they are negotiated with the guest + // driver as well. + self.vu_handle + .set_features(self.acked_features) + .and_then(|()| { + self.vu_handle + .setup_backend(&mem, &queue_refs, interrupt.clone()) + }) + .map_err(|err| { + self.metrics.activate_fails.inc(); + ActivateError::VhostUser(err) + })?; + + self.device_state = DeviceState::Activated(ActiveState { mem, interrupt }); + self.metrics + .activate_time_us + .store(get_time_us(ClockType::Monotonic) - start_time); + Ok(()) + } + + pub fn is_activated(&self) -> bool { + self.device_state.is_activated() + } + + pub fn socket_path(&self) -> &str { + &self.vu_handle.socket_path + } + + pub fn deactivate(&mut self) { + self.device_state = DeviceState::Inactive; + } + + /// Interrupt of the activated device. + /// + /// # Panics + /// + /// Panics if the device is not activated. + pub fn interrupt(&self) -> &dyn VirtioInterrupt { + self.device_state + .active_state() + .expect("Device is not initialized") + .interrupt + .deref() + } + + /// Re-read the config space from the backend and tell the guest driver it + /// changed. + /// + /// # Panics + /// + /// Panics if the device is not activated. + pub fn refresh_config(&mut self) -> Result<(), VhostUserDeviceError> { + let start_time = get_time_us(ClockType::Monotonic); + let interrupt = self + .device_state + .active_state() + .expect("Device is not initialized") + .interrupt + .clone(); + + self.config_space = get_config_space(&mut self.vu_handle, self.config_space_size)?; + + interrupt + .trigger(VirtioInterruptType::Config) + .map_err(VhostUserDeviceError::Interrupt)?; + + self.metrics + .config_change_time_us + .store(get_time_us(ClockType::Monotonic) - start_time); + + Ok(()) + } +} + +fn get_config_space( + vu_handle: &mut VhostUserHandleImpl, + size: u32, +) -> Result, VhostUserDeviceError> { + let buffer = vec![0u8; u64_to_usize(u64::from(size))]; + let (_, config_space) = vu_handle + .vu + .get_config(0, size, VhostUserConfigFlags::WRITABLE, &buffer) + .map_err(VhostUserDeviceError::GetConfig)?; + + // The vhost crate checks the size the reply declares, but not the length of + // the payload that follows it, so a backend can declare the size we asked + // for and send fewer bytes. Short of this check the guest would read + // whatever the config space was not long enough to cover. + if config_space.len() != u64_to_usize(u64::from(size)) { + return Err(VhostUserDeviceError::ShortConfigSpace( + config_space.len(), + size, + )); + } + + Ok(config_space) +} #[cfg(test)] pub(crate) mod tests { #![allow(clippy::undocumented_unsafe_blocks)] @@ -480,6 +832,267 @@ pub(crate) mod tests { use crate::vstate::memory; use crate::vstate::memory::{GuestAddress, GuestRegionMmapExt}; + /// Backend that records the vring index of every per-vring call, so a test + /// can tell an index-preserving setup from an index-compacting one. + #[derive(Default)] + pub(crate) struct VringCalls { + pub num: Vec, + pub addr: Vec, + pub base: Vec, + pub call: Vec, + pub kick: Vec, + pub enable: Vec, + } + + pub(crate) struct MockRecorder { + pub calls: std::cell::UnsafeCell, + } + + impl VhostUserHandleBackend for MockRecorder { + fn from_stream(_sock: UnixStream, _max_queue_num: u64) -> Self { + Self { + calls: std::cell::UnsafeCell::new(VringCalls::default()), + } + } + + fn set_owner(&self) -> Result<(), vhost::Error> { + Ok(()) + } + + fn set_hdr_flags(&self, _flags: VhostUserHeaderFlag) {} + + fn get_features(&self) -> Result { + Ok(0) + } + + fn get_protocol_features(&mut self) -> Result { + Ok(VhostUserProtocolFeatures::empty()) + } + + fn set_protocol_features( + &mut self, + _features: VhostUserProtocolFeatures, + ) -> Result<(), vhost::Error> { + Ok(()) + } + + fn set_features(&self, _features: u64) -> Result<(), vhost::Error> { + Ok(()) + } + + fn set_mem_table( + &self, + _regions: &[VhostUserMemoryRegionInfo], + ) -> Result<(), vhost::Error> { + Ok(()) + } + + fn set_vring_num(&self, queue_index: usize, _num: u16) -> Result<(), vhost::Error> { + unsafe { (*self.calls.get()).num.push(queue_index) }; + Ok(()) + } + + fn set_vring_addr( + &self, + queue_index: usize, + _config_data: &VringConfigData, + ) -> Result<(), vhost::Error> { + unsafe { (*self.calls.get()).addr.push(queue_index) }; + Ok(()) + } + + fn set_vring_base(&self, queue_index: usize, _base: u16) -> Result<(), vhost::Error> { + unsafe { (*self.calls.get()).base.push(queue_index) }; + Ok(()) + } + + fn set_vring_call(&self, queue_index: usize, _fd: &EventFd) -> Result<(), vhost::Error> { + unsafe { (*self.calls.get()).call.push(queue_index) }; + Ok(()) + } + + fn set_vring_kick(&self, queue_index: usize, _fd: &EventFd) -> Result<(), vhost::Error> { + unsafe { (*self.calls.get()).kick.push(queue_index) }; + Ok(()) + } + + fn set_vring_enable( + &mut self, + queue_index: usize, + _enable: bool, + ) -> Result<(), vhost::Error> { + unsafe { (*self.calls.get()).enable.push(queue_index) }; + Ok(()) + } + } + + fn recording_device(socket: String, num_queues: u64) -> VhostUserDevice { + VhostUserDevice::::new(VhostUserDeviceSpec { + socket, + num_queues, + queue_size: 128, + avail_features: 0, + config_space_size: 8, + require_config: false, + metrics_name: format!("test_generic_{num_queues}"), + }) + .unwrap() + } + + fn ready_queue(device: &mut VhostUserDevice, index: usize) { + device.queues[index].ready = true; + device.queues[index].size = device.queues[index].max_size; + } + + fn test_mem() -> GuestMemoryMmap { + let region_size = 0x10000; + let file = TempFile::new().unwrap().into_file(); + file.set_len(region_size as u64).unwrap(); + create_mem(file, &[(GuestAddress(0x0), region_size)]) + } + + #[test] + fn test_activate_preserves_vring_indices() { + let (_tmp_dir, tmp_socket_path) = create_tmp_socket(); + let mut device = recording_device(tmp_socket_path, 3); + + // The guest readies vrings 0 and 2 and leaves 1 alone, which is what a + // driver using fewer queues than were configured looks like. + ready_queue(&mut device, 0); + ready_queue(&mut device, 2); + + device.activate(test_mem(), default_interrupt()).unwrap(); + + // Vring 2 has to be set up as vring 2, not renumbered to 1. + let calls = unsafe { &*device.vu_handle.vu.calls.get() }; + assert_eq!(calls.num, vec![0, 2]); + assert_eq!(calls.addr, vec![0, 2]); + assert_eq!(calls.base, vec![0, 2]); + assert_eq!(calls.call, vec![0, 2]); + assert_eq!(calls.kick, vec![0, 2]); + assert_eq!(calls.enable, vec![0, 2]); + } + + #[test] + fn test_activate_without_ready_queues() { + let (_tmp_dir, tmp_socket_path) = create_tmp_socket(); + let mut device = recording_device(tmp_socket_path, 2); + + // Nothing ready has to fail rather than set up no vrings and report + // success. + assert!(matches!( + device.activate(test_mem(), default_interrupt()), + Err(ActivateError::QueueMemoryError(QueueError::NotReady)) + )); + assert!(!device.is_activated()); + + let calls = unsafe { &*device.vu_handle.vu.calls.get() }; + assert!(calls.num.is_empty()); + } + + #[test] + fn test_new_rejects_invalid_spec() { + let spec = |num_queues, config_space_size| VhostUserDeviceSpec { + socket: "no-such-socket".to_string(), + num_queues, + queue_size: 128, + avail_features: 0, + config_space_size, + require_config: false, + metrics_name: "test_invalid_spec".to_string(), + }; + + // All three are rejected before the socket is touched, so the bogus + // path above never gets in the way. + assert!(matches!( + VhostUserDevice::::new(spec(0, 8)), + Err(VhostUserDeviceError::InvalidNumQueues(0)) + )); + assert!(matches!( + VhostUserDevice::::new(spec(1, 0)), + Err(VhostUserDeviceError::InvalidConfigSpaceSize(0)) + )); + assert!(matches!( + VhostUserDevice::::new(spec(1, VHOST_USER_CONFIG_SIZE + 1)), + Err(VhostUserDeviceError::InvalidConfigSpaceSize(_)) + )); + assert!(matches!( + VhostUserDevice::::new(spec(MAX_QUEUES + 1, 8)), + Err(VhostUserDeviceError::TooManyQueues(_, MAX_QUEUES)) + )); + + let odd_queue_size = VhostUserDeviceSpec { + queue_size: 100, + ..spec(1, 8) + }; + assert!(matches!( + VhostUserDevice::::new(odd_queue_size), + Err(VhostUserDeviceError::InvalidQueueSize(100)) + )); + } + + #[test] + fn test_new_rejects_short_config_space() { + /// Backend that acks CONFIG and then under-fills the config space. + struct MockShortConfig; + + impl VhostUserHandleBackend for MockShortConfig { + fn from_stream(_sock: UnixStream, _max_queue_num: u64) -> Self { + Self + } + + fn set_owner(&self) -> Result<(), vhost::Error> { + Ok(()) + } + + fn set_hdr_flags(&self, _flags: VhostUserHeaderFlag) {} + + fn get_features(&self) -> Result { + Ok(VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits()) + } + + fn get_protocol_features(&mut self) -> Result { + Ok(VhostUserProtocolFeatures::CONFIG) + } + + fn set_protocol_features( + &mut self, + _features: VhostUserProtocolFeatures, + ) -> Result<(), vhost::Error> { + Ok(()) + } + + fn get_config( + &mut self, + _offset: u32, + _size: u32, + _flags: VhostUserConfigFlags, + _buf: &[u8], + ) -> Result<(VhostUserConfig, VhostUserConfigPayload), vhost::Error> { + // Asked for 8 bytes, answers with 3. + Ok((VhostUserConfig::default(), vec![0x69, 0x69, 0x69])) + } + } + + let (_tmp_dir, tmp_socket_path) = create_tmp_socket(); + let result = VhostUserDevice::::new(VhostUserDeviceSpec { + socket: tmp_socket_path, + num_queues: 1, + queue_size: 128, + avail_features: VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits(), + config_space_size: 8, + require_config: true, + metrics_name: "test_short_config".to_string(), + }); + + // A guest reading the bytes the backend did not send would otherwise be + // reading whatever the config space was too short to cover. + assert!(matches!( + result, + Err(VhostUserDeviceError::ShortConfigSpace(3, 8)) + )); + } + pub(crate) fn create_mem(file: File, regions: &[(GuestAddress, usize)]) -> GuestMemoryMmap { GuestMemoryMmap::from_regions( memory::create( diff --git a/tests/integration_tests/functional/test_drive_vhost_user.py b/tests/integration_tests/functional/test_drive_vhost_user.py index 8b9e7da5274..a65887ee5a5 100644 --- a/tests/integration_tests/functional/test_drive_vhost_user.py +++ b/tests/integration_tests/functional/test_drive_vhost_user.py @@ -104,6 +104,13 @@ def test_vhost_user_block(uvm_vhost_user_booted_ro): "1-6": "/dev/vda", } _check_drives(vm, assert_dict, assert_dict.keys()) + + # The backend is readonly, so Firecracker should have negotiated + # VIRTIO_BLK_F_RO and passed `ro` for the root device. + _, stdout, stderr = vm.ssh.run("cat /proc/cmdline") + assert stderr == "" + assert " ro " in f" {stdout.strip()} " + vhost_user_block_metrics.validate(vm) with pytest.raises(