From cbaeea16020aeb02ffce5d9572688ceb55e69860 Mon Sep 17 00:00:00 2001 From: Wes Mason Date: Wed, 5 Aug 2026 20:24:20 +0100 Subject: [PATCH 1/3] fix(vhost-user-block): honour a readonly backend The VIRTIO_BLK_F_RO check read acked_features after it had been reassigned to acked_features & PROTOCOL_FEATURES, so it tested bit 5 against 0x4000_0000 and never matched. read_only was therefore always false, even though the device does ask the backend for VIRTIO_BLK_F_RO, and docs/api_requests/block-vhost-user.md documents the backend as the place a readonly vhost-user drive is configured. read_only feeds Block::read_only(), which decides whether a root device gets ro or rw on the guest kernel cmdline, so a readonly vhost-user root device was given rw. The guest still sees VIRTIO_BLK_F_RO and retries the root mount readonly, so it boots, but /proc/cmdline is wrong and a later remount,rw fails. There is no way to correct this from config: VhostUserBlockConfig::try_from rejects is_read_only, which leaves the backend as the only source of the flag. Compute read_only before acked_features is narrowed, so the shadowing cannot swallow the bit again. test_new_all_features already advertises VIRTIO_BLK_F_RO from its mock backend and asserted the broken result, so it now asserts read_only is set, and test_vhost_user_block asserts ro reaches the guest cmdline. Signed-off-by: Wes Mason --- CHANGELOG.md | 5 +++++ src/vmm/src/devices/virtio/block/vhost_user/device.rs | 6 ++++-- .../integration_tests/functional/test_drive_vhost_user.py | 7 +++++++ 3 files changed, 16 insertions(+), 2 deletions(-) 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/vhost_user/device.rs b/src/vmm/src/devices/virtio/block/vhost_user/device.rs index 65d0b0a08de..80e9683abd0 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -205,11 +205,13 @@ impl VhostUserBlockImpl { u64_to_usize(NUM_QUEUES)]; let device_state = DeviceState::Inactive; + // Read before `acked_features` is narrowed to the protocol bit below. + let read_only = acked_features & (1 << VIRTIO_BLK_F_RO) != 0; + // 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); @@ -657,7 +659,7 @@ mod tests { VhostUserHeaderFlag::empty().bits() ); assert!(!vhost_block.root_device); - assert!(!vhost_block.read_only); + assert!(vhost_block.read_only); assert_eq!(vhost_block.config_space, vec![0x69, 0x69, 0x69]); // Test some `VirtioDevice` methods 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( From 099cc7ec9a51523dfa985bb0df117454f9e4310f Mon Sep 17 00:00:00 2001 From: Wes Mason Date: Wed, 19 Aug 2026 12:36:36 +0100 Subject: [PATCH 2/3] feat(vhost-user): add a device-type agnostic frontend The vhost-user frontends duplicate everything that is not specific to the virtio device type they implement: connecting to the backend socket, negotiating virtio and protocol features, fetching the config space, allocating queues and eventfds, and setting up the backend's vrings on activation. Add VhostUserDevice to hold that shared state, built from a VhostUserDeviceSpec carrying the parts that do vary: socket path, queue count and size, the features to offer, the config space size, whether CONFIG is mandatory, and the metrics name. Device types embed it and keep their own state alongside. Two details worth noting for the callers that follow. Fetching the config space is skipped when the backend does not ack CONFIG, leaving an empty config space, since only a frontend with no device-specific fallback needs to insist on it. And activate() sets up the queues the guest marked ready rather than every allocated queue, because the number a driver initialises is dictated by the backend-owned config space, so initializing an unconfigured queue returns NotReady and aborts activation. Real vring indices are preserved: queues 0 and 2 being ready maps to vrings 0 and 2, not 0 and 1. Activating with no ready queues is rejected rather than quietly setting up nothing. The spec is validated before the socket is touched, since a frontend built from it may be configured at runtime rather than from constants: the queue count is bounded by what the PCI notification region can address, the queue size has to be a power of two, and the config space size has to fit the protocol's own limit. The config space size is a size rather than an upper bound, in that the backend has to return exactly that many bytes. The vhost crate checks the size the reply declares but not the length of the payload behind it, so the length is checked here too. Otherwise a backend could declare the size that was asked for, send fewer bytes, and leave the guest reading past the end of what it sent. No device type uses this yet. Signed-off-by: Wes Mason --- src/vmm/src/devices/virtio/vhost_user.rs | 615 ++++++++++++++++++++++- 1 file changed, 614 insertions(+), 1 deletion(-) 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( From 9a9a8f32442c90f666b2cd61bf87793c732214a8 Mon Sep 17 00:00:00 2001 From: Wes Mason Date: Wed, 19 Aug 2026 14:29:19 +0100 Subject: [PATCH 3/3] refactor(vhost-user-block): delegate to the generic frontend The block device carried its own copy of the feature negotiation, config space fetch, queue allocation and vring setup, none of which is specific to block. Move that state into VhostUserDevice and keep only the block-specific fields here: the drive id, partuuid, cache type and the root and readonly flags. Most of the behaviour is unchanged. Block still offers the same feature set, including FLUSH when the cache type is Writeback, still treats CONFIG as optional, and still reports metrics under block_{drive_id}. Activating without a ready queue reports QueueError::NotReady as before, though it now comes from the generic readiness check rather than from initializing queue 0. The double-activation assert moves into the generic device, which owns the state it guards. read_only is now read from the features the backend acked rather than from a local that a later shadowing narrowed, so the ordering trap fixed in "fix(vhost-user-block): honour a readonly backend" cannot come back. VhostUserBlockError keeps its existing variants and their messages, which surface through the API, and gains one for the generic errors block itself cannot produce. One behaviour does change. Fetching the config space now goes through the shared frontend, which requires the backend to return as many bytes as were asked for, so a backend that answers a 60 byte request with fewer now fails to attach where before it was accepted and left the rest of the config space short. Signed-off-by: Wes Mason --- src/vmm/src/devices/virtio/block/device.rs | 14 +- .../devices/virtio/block/vhost_user/device.rs | 285 ++++++------------ .../virtio/block/vhost_user/event_handler.rs | 6 +- .../devices/virtio/block/vhost_user/mod.rs | 26 +- 4 files changed, 126 insertions(+), 205 deletions(-) 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 80e9683abd0..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,80 +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; - - // Read before `acked_features` is narrowed to the protocol bit below. - let read_only = acked_features & (1 << VIRTIO_BLK_F_RO) != 0; - - // 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 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), + })?; + + // 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; - queues, - queue_evts, - device_state, + 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, }) } @@ -251,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()?) } } @@ -299,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]) { @@ -345,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 { @@ -400,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; @@ -523,6 +393,7 @@ mod tests { // no flags should be set. assert_eq!( vhost_block + .vu_device .vu_handle .vu .sock @@ -534,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] @@ -600,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)) } } @@ -628,6 +503,7 @@ mod tests { // should be negotiated and header flags should be set. assert_eq!( vhost_block + .vu_device .vu_handle .vu .sock @@ -639,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_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!( @@ -684,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 @@ -826,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), + } + } }