diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index 68dfa3dfc8d..577632c728f 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -193,8 +193,11 @@ impl MMIODeviceManager { vm.register_ioevent(queue_evt, &io_addr, u32::try_from(i).unwrap()) .map_err(MmioError::RegisterIoEvent)?; } - vm.register_irqfd(locked_device.interrupt_evt(), device_info.irqs[0]) - .map_err(MmioError::RegisterIrqFd)?; + vm.register_irqfd( + &locked_device.interrupt_trigger().irq_evt, + device_info.irqs[0], + ) + .map_err(MmioError::RegisterIrqFd)?; } self.register_mmio_device( @@ -513,13 +516,13 @@ impl DeviceInfoForFDT for MMIODeviceInfo { #[cfg(test)] mod tests { - use std::sync::atomic::AtomicU32; + use std::sync::Arc; use utils::eventfd::EventFd; use super::*; - use crate::devices::virtio::device::VirtioDevice; + use crate::devices::virtio::device::{IrqTrigger, VirtioDevice}; use crate::devices::virtio::queue::Queue; use crate::devices::virtio::ActivateError; use crate::utilities::test_utils::multi_region_mem; @@ -566,7 +569,7 @@ mod tests { dummy: u32, queues: Vec, queue_evts: [EventFd; 1], - interrupt_evt: EventFd, + interrupt_trigger: IrqTrigger, } impl DummyDevice { @@ -575,7 +578,7 @@ mod tests { dummy: 0, queues: QUEUE_SIZES.iter().map(|&s| Queue::new(s)).collect(), queue_evts: [EventFd::new(libc::EFD_NONBLOCK).expect("cannot create eventFD")], - interrupt_evt: EventFd::new(libc::EFD_NONBLOCK).expect("cannot create eventFD"), + interrupt_trigger: IrqTrigger::new().expect("cannot create eventFD"), } } } @@ -607,12 +610,8 @@ mod tests { &self.queue_evts } - fn interrupt_evt(&self) -> &EventFd { - &self.interrupt_evt - } - - fn interrupt_status(&self) -> Arc { - Arc::new(AtomicU32::new(0)) + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.interrupt_trigger } fn ack_features_by_page(&mut self, page: u32, value: u32) { diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/src/vmm/src/devices/virtio/balloon/device.rs index 77a13902cc2..065d9a1fb2f 100644 --- a/src/vmm/src/devices/virtio/balloon/device.rs +++ b/src/vmm/src/devices/virtio/balloon/device.rs @@ -2,8 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 use std::fmt; -use std::sync::atomic::AtomicU32; -use std::sync::Arc; use std::time::Duration; use log::error; @@ -584,12 +582,8 @@ impl VirtioDevice for Balloon { &self.queue_evts } - fn interrupt_evt(&self) -> &EventFd { - &self.irq_trigger.irq_evt - } - - fn interrupt_status(&self) -> Arc { - self.irq_trigger.irq_status.clone() + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.irq_trigger } fn read_config(&self, offset: u64, data: &mut [u8]) { @@ -619,10 +613,9 @@ impl VirtioDevice for Balloon { fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> { self.device_state = DeviceState::Activated(mem); if self.activate_evt.write(1).is_err() { - error!("Balloon: Cannot write to activate_evt"); METRICS.activate_fails.inc(); self.device_state = DeviceState::Inactive; - return Err(ActivateError::BadActivate); + return Err(ActivateError::EventFd); } if self.stats_enabled() { diff --git a/src/vmm/src/devices/virtio/block/device.rs b/src/vmm/src/devices/virtio/block/device.rs index c4c77a746b3..fe201f7246d 100644 --- a/src/vmm/src/devices/virtio/block/device.rs +++ b/src/vmm/src/devices/virtio/block/device.rs @@ -1,9 +1,6 @@ // Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::sync::atomic::AtomicU32; -use std::sync::Arc; - use event_manager::{EventOps, Events, MutEventSubscriber}; use utils::eventfd::EventFd; @@ -11,7 +8,7 @@ use super::persist::{BlockConstructorArgs, BlockState}; use super::vhost_user::device::{VhostUserBlock, VhostUserBlockConfig}; use super::virtio::device::{VirtioBlock, VirtioBlockConfig}; use super::BlockError; -use crate::devices::virtio::device::VirtioDevice; +use crate::devices::virtio::device::{IrqTrigger, VirtioDevice}; use crate::devices::virtio::queue::Queue; use crate::devices::virtio::{ActivateError, TYPE_BLOCK}; use crate::rate_limiter::BucketUpdate; @@ -176,17 +173,10 @@ impl VirtioDevice for Block { } } - fn interrupt_evt(&self) -> &EventFd { - match self { - Self::Virtio(b) => &b.irq_trigger.irq_evt, - Self::VhostUser(b) => &b.irq_trigger.irq_evt, - } - } - - fn interrupt_status(&self) -> Arc { + fn interrupt_trigger(&self) -> &IrqTrigger { match self { - Self::Virtio(b) => b.irq_trigger.irq_status.clone(), - Self::VhostUser(b) => b.irq_trigger.irq_status.clone(), + Self::Virtio(b) => &b.irq_trigger, + Self::VhostUser(b) => &b.irq_trigger, } } 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 2304262c26e..99b27915598 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -4,7 +4,6 @@ // Portions Copyright 2019 Intel Corporation. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::sync::atomic::AtomicU32; use std::sync::Arc; use log::error; @@ -311,13 +310,8 @@ impl VirtioDevice for VhostUserBlock &self.queue_evts } - fn interrupt_evt(&self) -> &EventFd { - &self.irq_trigger.irq_evt - } - - /// Returns the current device interrupt status. - fn interrupt_status(&self) -> Arc { - self.irq_trigger.irq_status.clone() + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.irq_trigger } fn read_config(&self, offset: u64, data: &mut [u8]) { @@ -342,13 +336,13 @@ impl VirtioDevice for VhostUserBlock // with guest driver as well. self.vu_handle .set_features(self.acked_features) - .map_err(ActivateError::VhostUser)?; - self.vu_handle - .setup_backend( - &mem, - &[(0, &self.queues[0], &self.queue_evts[0])], - &self.irq_trigger, - ) + .and_then(|()| { + self.vu_handle.setup_backend( + &mem, + &[(0, &self.queues[0], &self.queue_evts[0])], + &self.irq_trigger, + ) + }) .map_err(|err| { self.metrics.activate_fails.inc(); ActivateError::VhostUser(err) diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs index 994508ff8ce..7313cbb8b5c 100644 --- a/src/vmm/src/devices/virtio/block/virtio/device.rs +++ b/src/vmm/src/devices/virtio/block/virtio/device.rs @@ -11,7 +11,6 @@ use std::fs::{File, OpenOptions}; use std::io::{Seek, SeekFrom, Write}; use std::os::linux::fs::MetadataExt; use std::path::PathBuf; -use std::sync::atomic::AtomicU32; use std::sync::Arc; use block_io::FileEngine; @@ -609,13 +608,8 @@ impl VirtioDevice for VirtioBlock { &self.queue_evts } - fn interrupt_evt(&self) -> &EventFd { - &self.irq_trigger.irq_evt - } - - /// Returns the current device interrupt status. - fn interrupt_status(&self) -> Arc { - self.irq_trigger.irq_status.clone() + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.irq_trigger } fn read_config(&self, offset: u64, mut data: &mut [u8]) { @@ -658,8 +652,8 @@ impl VirtioDevice for VirtioBlock { } if self.activate_evt.write(1).is_err() { - error!("Block: Cannot write to activate_evt"); - return Err(ActivateError::BadActivate); + self.metrics.activate_fails.inc(); + return Err(ActivateError::EventFd); } self.device_state = DeviceState::Activated(mem); Ok(()) diff --git a/src/vmm/src/devices/virtio/device.rs b/src/vmm/src/devices/virtio/device.rs index 80135576e13..86c8327d8c8 100644 --- a/src/vmm/src/devices/virtio/device.rs +++ b/src/vmm/src/devices/virtio/device.rs @@ -119,11 +119,12 @@ pub trait VirtioDevice: AsAny + Send { /// Returns the device queues event fds. fn queue_events(&self) -> &[EventFd]; - /// Returns the device interrupt eventfd. - fn interrupt_evt(&self) -> &EventFd; - /// Returns the current device interrupt status. - fn interrupt_status(&self) -> Arc; + fn interrupt_status(&self) -> Arc { + Arc::clone(&self.interrupt_trigger().irq_status) + } + + fn interrupt_trigger(&self) -> &IrqTrigger; /// The set of feature bits shifted by `page * 32`. fn avail_features_by_page(&self, page: u32) -> u32 { @@ -266,11 +267,7 @@ pub(crate) mod tests { todo!() } - fn interrupt_evt(&self) -> &EventFd { - todo!() - } - - fn interrupt_status(&self) -> Arc { + fn interrupt_trigger(&self) -> &IrqTrigger { todo!() } diff --git a/src/vmm/src/devices/virtio/mmio.rs b/src/vmm/src/devices/virtio/mmio.rs index 96bd914d984..7cf09f723a5 100644 --- a/src/vmm/src/devices/virtio/mmio.rs +++ b/src/vmm/src/devices/virtio/mmio.rs @@ -11,10 +11,10 @@ use std::sync::{Arc, Mutex, MutexGuard}; use utils::byte_order; -use crate::devices::virtio::device::VirtioDevice; +use crate::devices::virtio::device::{IrqType, VirtioDevice}; use crate::devices::virtio::device_status; use crate::devices::virtio::queue::Queue; -use crate::logger::warn; +use crate::logger::{error, warn}; use crate::vstate::memory::{GuestAddress, GuestMemoryMmap}; // TODO crosvm uses 0 here, but IIRC virtio specified some other vendor id that should be used @@ -187,9 +187,20 @@ impl MmioTransport { self.device_status = status; let device_activated = self.locked_device().is_activated(); if !device_activated && self.are_queues_valid() { - self.locked_device() - .activate(self.mem.clone()) - .expect("Failed to activate device"); + // temporary variable needed for borrow checker + let activate_result = self.locked_device().activate(self.mem.clone()); + if let Err(err) = activate_result { + self.device_status |= DEVICE_NEEDS_RESET; + + // Section 2.1.2 of the specification states that we need to send a device + // configuration change interrupt + let _ = self + .locked_device() + .interrupt_trigger() + .trigger_irq(IrqType::Config); + + error!("Failed to activate virtio device: {}", err) + } } } _ if (status & FAILED) != 0 => { @@ -306,7 +317,9 @@ impl MmioTransport { 0x20 => { if self.check_device_status( device_status::DRIVER, - device_status::FEATURES_OK | device_status::FAILED, + device_status::FEATURES_OK + | device_status::FAILED + | device_status::DEVICE_NEEDS_RESET, ) { self.locked_device() .ack_features_by_page(self.acked_features_select, v); @@ -339,7 +352,10 @@ impl MmioTransport { } } 0x100..=0xfff => { - if self.check_device_status(device_status::DRIVER, device_status::FAILED) { + if self.check_device_status( + device_status::DRIVER, + device_status::FAILED | device_status::DEVICE_NEEDS_RESET, + ) { self.locked_device().write_config(offset - 0x100, data) } else { warn!("can not write to device config data area before driver is ready"); @@ -363,6 +379,8 @@ pub(crate) mod tests { use utils::u64_to_usize; use super::*; + use crate::devices::virtio::device::IrqTrigger; + use crate::devices::virtio::device_status::DEVICE_NEEDS_RESET; use crate::devices::virtio::ActivateError; use crate::utilities::test_utils::single_region_mem; use crate::vstate::memory::GuestMemoryMmap; @@ -371,12 +389,12 @@ pub(crate) mod tests { pub(crate) struct DummyDevice { acked_features: u64, avail_features: u64, - interrupt_evt: EventFd, - interrupt_status: Arc, + interrupt_trigger: IrqTrigger, queue_evts: Vec, queues: Vec, device_activated: bool, config_bytes: [u8; 0xeff], + activate_should_error: bool, } impl DummyDevice { @@ -384,8 +402,7 @@ pub(crate) mod tests { DummyDevice { acked_features: 0, avail_features: 0, - interrupt_evt: EventFd::new(libc::EFD_NONBLOCK).unwrap(), - interrupt_status: Arc::new(AtomicU32::new(0)), + interrupt_trigger: IrqTrigger::new().unwrap(), queue_evts: vec![ EventFd::new(libc::EFD_NONBLOCK).unwrap(), EventFd::new(libc::EFD_NONBLOCK).unwrap(), @@ -393,6 +410,7 @@ pub(crate) mod tests { queues: vec![Queue::new(16), Queue::new(32)], device_activated: false, config_bytes: [0; 0xeff], + activate_should_error: false, } } @@ -430,12 +448,8 @@ pub(crate) mod tests { &self.queue_evts } - fn interrupt_evt(&self) -> &EventFd { - &self.interrupt_evt - } - - fn interrupt_status(&self) -> Arc { - self.interrupt_status.clone() + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.interrupt_trigger } fn read_config(&self, offset: u64, data: &mut [u8]) { @@ -450,7 +464,11 @@ pub(crate) mod tests { fn activate(&mut self, _: GuestMemoryMmap) -> Result<(), ActivateError> { self.device_activated = true; - Ok(()) + if self.activate_should_error { + Err(ActivateError::EventFd) + } else { + Ok(()) + } } fn is_activated(&self) -> bool { @@ -823,6 +841,63 @@ pub(crate) mod tests { assert_eq!(read_le_u32(&buf[..]), 1); } + #[test] + fn test_bus_device_activate_failure() { + let m = single_region_mem(0x1000); + let device = DummyDevice { + activate_should_error: true, + ..DummyDevice::new() + }; + let mut d = MmioTransport::new(m, Arc::new(Mutex::new(device)), false); + + set_device_status(&mut d, device_status::ACKNOWLEDGE); + set_device_status(&mut d, device_status::ACKNOWLEDGE | device_status::DRIVER); + set_device_status( + &mut d, + device_status::ACKNOWLEDGE | device_status::DRIVER | device_status::FEATURES_OK, + ); + + let mut buf = [0; 4]; + let queue_len = d.locked_device().queues().len(); + for q in 0..queue_len { + d.queue_select = q.try_into().unwrap(); + write_le_u32(&mut buf[..], 16); + d.bus_write(0x38, &buf[..]); + write_le_u32(&mut buf[..], 1); + d.bus_write(0x44, &buf[..]); + } + assert!(d.are_queues_valid()); + assert_eq!( + d.locked_device().interrupt_status().load(Ordering::SeqCst), + 0 + ); + + set_device_status( + &mut d, + device_status::ACKNOWLEDGE + | device_status::DRIVER + | device_status::FEATURES_OK + | device_status::DRIVER_OK, + ); + + // Failure in activate results in `DEVICE_NEEDS_RESET` status being set + assert_ne!(d.device_status & DEVICE_NEEDS_RESET, 0); + // We injected an interrupt of type "configuration change" + assert_eq!( + d.locked_device().interrupt_status().load(Ordering::SeqCst), + VIRTIO_MMIO_INT_CONFIG + ); + // We actually wrote to the eventfd + assert_eq!( + d.locked_device() + .interrupt_trigger() + .irq_evt + .read() + .unwrap(), + 1 + ); + } + fn activate_device(d: &mut MmioTransport) { set_device_status(d, device_status::ACKNOWLEDGE); set_device_status(d, device_status::ACKNOWLEDGE | device_status::DRIVER); diff --git a/src/vmm/src/devices/virtio/mod.rs b/src/vmm/src/devices/virtio/mod.rs index 23ab2914401..3575c53f1e2 100644 --- a/src/vmm/src/devices/virtio/mod.rs +++ b/src/vmm/src/devices/virtio/mod.rs @@ -8,7 +8,6 @@ //! Implements virtio devices, queues, and transport mechanisms. use std::any::Any; -use std::io::Error as IOError; pub mod balloon; pub mod block; @@ -40,6 +39,7 @@ mod device_status { pub const FAILED: u32 = 128; pub const FEATURES_OK: u32 = 8; pub const DRIVER_OK: u32 = 4; + pub const DEVICE_NEEDS_RESET: u32 = 64; } /// Types taken from linux/virtio_ids.h. @@ -60,10 +60,10 @@ pub const NOTIFY_REG_OFFSET: u32 = 0x50; /// Errors triggered when activating a VirtioDevice. #[derive(Debug, thiserror::Error, displaydoc::Display)] pub enum ActivateError { - /// Epoll error. - EpollCtl(IOError), - /// General error at activation. - BadActivate, + /// Wrong number of queue for virtio device: expected {expected}, got {got} + QueueMismatch { expected: usize, got: usize }, + /// Failed to write to activate eventfd + EventFd, /// Vhost user: {0} VhostUser(vhost_user::VhostUserError), } diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index f79692012e5..1e0d2153296 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -9,7 +9,6 @@ use std::io::Read; use std::mem; use std::net::Ipv4Addr; -use std::sync::atomic::AtomicU32; use std::sync::{Arc, Mutex}; use libc::EAGAIN; @@ -823,14 +822,9 @@ impl VirtioDevice for Net { &self.queue_evts } - fn interrupt_evt(&self) -> &EventFd { - &self.irq_trigger.irq_evt + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.irq_trigger } - - fn interrupt_status(&self) -> Arc { - self.irq_trigger.irq_status.clone() - } - fn read_config(&self, offset: u64, data: &mut [u8]) { if let Some(config_space_bytes) = self.config_space.as_slice().get(u64_to_usize(offset)..) { let len = config_space_bytes.len().min(data.len()); @@ -868,8 +862,8 @@ impl VirtioDevice for Net { } if self.activate_evt.write(1).is_err() { - error!("Net: Cannot write to activate_evt"); - return Err(super::super::ActivateError::BadActivate); + self.metrics.activate_fails.inc(); + return Err(ActivateError::EventFd); } self.device_state = DeviceState::Activated(mem); Ok(()) diff --git a/src/vmm/src/devices/virtio/rng/device.rs b/src/vmm/src/devices/virtio/rng/device.rs index 512bf66c1d3..0186ce4d7fd 100644 --- a/src/vmm/src/devices/virtio/rng/device.rs +++ b/src/vmm/src/devices/virtio/rng/device.rs @@ -259,12 +259,8 @@ impl VirtioDevice for Entropy { &self.queue_events } - fn interrupt_evt(&self) -> &EventFd { - &self.irq_trigger.irq_evt - } - - fn interrupt_status(&self) -> Arc { - self.irq_trigger.irq_status.clone() + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.irq_trigger } fn avail_features(&self) -> u64 { @@ -288,10 +284,9 @@ impl VirtioDevice for Entropy { } fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> { - self.activate_event.write(1).map_err(|err| { - error!("entropy: Cannot write to activate_evt: {err}"); + self.activate_event.write(1).map_err(|_| { METRICS.activate_fails.inc(); - super::super::ActivateError::BadActivate + ActivateError::EventFd })?; self.device_state = DeviceState::Activated(mem); Ok(()) diff --git a/src/vmm/src/devices/virtio/vsock/device.rs b/src/vmm/src/devices/virtio/vsock/device.rs index aa9668779f4..d48782901ec 100644 --- a/src/vmm/src/devices/virtio/vsock/device.rs +++ b/src/vmm/src/devices/virtio/vsock/device.rs @@ -5,23 +5,22 @@ // Use of this source code is governed by a BSD-style license that can be // found in the THIRD-PARTY file. +//! This is the `VirtioDevice` implementation for our vsock device. It handles the virtio-level +//! device logic: feature negociation, device configuration, and device activation. +//! +//! We aim to conform to the VirtIO v1.1 spec: +//! https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.html +//! +//! The vsock device has two input parameters: a CID to identify the device, and a +//! `VsockBackend` to use for offloading vsock traffic. +//! +//! Upon its activation, the vsock device registers handlers for the following events/FDs: +//! - an RX queue FD; +//! - a TX queue FD; +//! - an event queue FD; and +//! - a backend FD. + use std::fmt::Debug; -/// This is the `VirtioDevice` implementation for our vsock device. It handles the virtio-level -/// device logic: feature negociation, device configuration, and device activation. -/// -/// We aim to conform to the VirtIO v1.1 spec: -/// https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.html -/// -/// The vsock device has two input parameters: a CID to identify the device, and a -/// `VsockBackend` to use for offloading vsock traffic. -/// -/// Upon its activation, the vsock device registers handlers for the following events/FDs: -/// - an RX queue FD; -/// - a TX queue FD; -/// - an event queue FD; and -/// - a backend FD. -use std::sync::atomic::AtomicU32; -use std::sync::Arc; use log::{error, warn}; use utils::byte_order; @@ -290,12 +289,8 @@ where &self.queue_events } - fn interrupt_evt(&self) -> &EventFd { - &self.irq_trigger.irq_evt - } - - fn interrupt_status(&self) -> Arc { - self.irq_trigger.irq_status.clone() + fn interrupt_trigger(&self) -> &IrqTrigger { + &self.irq_trigger } fn read_config(&self, offset: u64, data: &mut [u8]) { @@ -330,18 +325,15 @@ where fn activate(&mut self, mem: GuestMemoryMmap) -> Result<(), ActivateError> { if self.queues.len() != defs::VSOCK_NUM_QUEUES { METRICS.activate_fails.inc(); - error!( - "Cannot perform activate. Expected {} queue(s), got {}", - defs::VSOCK_NUM_QUEUES, - self.queues.len() - ); - return Err(ActivateError::BadActivate); + return Err(ActivateError::QueueMismatch { + expected: defs::VSOCK_NUM_QUEUES, + got: self.queues.len(), + }); } if self.activate_evt.write(1).is_err() { METRICS.activate_fails.inc(); - error!("Cannot write to activate_evt",); - return Err(ActivateError::BadActivate); + return Err(ActivateError::EventFd); } self.device_state = DeviceState::Activated(mem);