From 15e0c73e0266cb70d7b7169ff9eb4fa4f371fdf1 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 3 Jul 2024 17:17:34 +0100 Subject: [PATCH 1/6] refactor: Simplify VirtioDevice trait by combining methods Combine the `interrupt_evt` and `interrupt_status` methods into a single method `interrupt_trigger` that returns a `IrqTrigger` reference (which essentially combines the two objects originally returned by the status and evt methods). The advantage to this is that `IrqTrigger` exposes a `trigger_irq` method, which I'd like to use in the next commit. Signed-off-by: Patrick Roy --- src/vmm/src/device_manager/mmio.rs | 23 +++++++++---------- src/vmm/src/devices/virtio/balloon/device.rs | 10 ++------ src/vmm/src/devices/virtio/block/device.rs | 18 ++++----------- .../devices/virtio/block/vhost_user/device.rs | 10 ++------ .../src/devices/virtio/block/virtio/device.rs | 10 ++------ src/vmm/src/devices/virtio/device.rs | 15 +++++------- src/vmm/src/devices/virtio/mmio.rs | 15 ++++-------- src/vmm/src/devices/virtio/net/device.rs | 10 ++------ src/vmm/src/devices/virtio/rng/device.rs | 8 ++----- src/vmm/src/devices/virtio/vsock/device.rs | 12 +++------- 10 files changed, 39 insertions(+), 92 deletions(-) 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..763285949f8 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]) { 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..d5b096da885 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]) { diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs index 994508ff8ce..a6043cb438d 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]) { 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..c217008a7e9 100644 --- a/src/vmm/src/devices/virtio/mmio.rs +++ b/src/vmm/src/devices/virtio/mmio.rs @@ -363,6 +363,7 @@ pub(crate) mod tests { use utils::u64_to_usize; use super::*; + use crate::devices::virtio::device::IrqTrigger; use crate::devices::virtio::ActivateError; use crate::utilities::test_utils::single_region_mem; use crate::vstate::memory::GuestMemoryMmap; @@ -371,8 +372,7 @@ 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, @@ -384,8 +384,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(), @@ -430,12 +429,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]) { diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index f79692012e5..f6b6d805bf5 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()); diff --git a/src/vmm/src/devices/virtio/rng/device.rs b/src/vmm/src/devices/virtio/rng/device.rs index 512bf66c1d3..3381a5635c6 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 { diff --git a/src/vmm/src/devices/virtio/vsock/device.rs b/src/vmm/src/devices/virtio/vsock/device.rs index aa9668779f4..5a99f7d7ac1 100644 --- a/src/vmm/src/devices/virtio/vsock/device.rs +++ b/src/vmm/src/devices/virtio/vsock/device.rs @@ -6,6 +6,7 @@ // found in the THIRD-PARTY file. 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. /// @@ -20,9 +21,6 @@ use std::fmt::Debug; /// - 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; use utils::eventfd::EventFd; @@ -290,12 +288,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]) { From 7c03f826ebbe2340296e9b2c3acc2b2bf7a8c946 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 3 Jul 2024 17:41:53 +0100 Subject: [PATCH 2/6] doc: fix doc comment in vsock/device.rs Correctly mark the doc comment as an inner doc comment for the enclosing macro, instead of an outer doc comment for the second import. Signed-off-by: Patrick Roy --- src/vmm/src/devices/virtio/vsock/device.rs | 29 +++++++++++----------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/vmm/src/devices/virtio/vsock/device.rs b/src/vmm/src/devices/virtio/vsock/device.rs index 5a99f7d7ac1..c69393c1bab 100644 --- a/src/vmm/src/devices/virtio/vsock/device.rs +++ b/src/vmm/src/devices/virtio/vsock/device.rs @@ -5,22 +5,23 @@ // 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 log::{error, warn}; use utils::byte_order; use utils::eventfd::EventFd; From 297db951fce59de63cd875b75b5447135347eef9 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 3 Jul 2024 16:01:50 +0100 Subject: [PATCH 3/6] fix: do not panic if virtio device activation return `Err(...)` When the guest driver sets a virtio devices status to `DRIVER_OK`, we proceed with calling `VirtioDevice::activate`. However, our MMIO transport layer assumes that this activation cannot go wrong, and calls `.expect(...)` on the result. For most devices, this is fine, as the activate method doesn't do much besides writing to an event_fd (and I can't think of a scenario in which this could go wrong). However, our vhost-user-blk device has some non-trivial logic inside of its `activate` method, which includes communication with the vhost-user-backend via a unix socket. If this unix socket gets closed early, this causes `activate` to return an error, and thus consequently a panic in the MMIO code. The virtio spec, in Section 2.2, has the following to say [1]: > The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state that a reset is needed. If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device MUST send a device configuration change notification to the driver. So the spec-conform way of handling an activation error is setting the `DEVICE_NEEDS_RESET` flag in the device_status field (which is what this commit does). This will fix the panic, however it will most certainly still not result in correct device operations (e.g. a vhost-user-backend dying will still be unrecoverable). This is because Firecracker does not actually implement device reset, see also #3074. Thus, the device will simply be "dead" to the guest: All operations where we currently simply abort and do nothing if the device is in the FAILED state will do the same in the DEVICE_NEEDS_RESET state. [1]: https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.pdf Signed-off-by: Patrick Roy --- src/vmm/src/devices/virtio/mmio.rs | 27 ++++++++++++++++++++------- src/vmm/src/devices/virtio/mod.rs | 1 + 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/vmm/src/devices/virtio/mmio.rs b/src/vmm/src/devices/virtio/mmio.rs index c217008a7e9..efe7e31d228 100644 --- a/src/vmm/src/devices/virtio/mmio.rs +++ b/src/vmm/src/devices/virtio/mmio.rs @@ -11,7 +11,7 @@ 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; @@ -186,10 +186,18 @@ impl MmioTransport { DRIVER_OK if self.device_status == (ACKNOWLEDGE | DRIVER | FEATURES_OK) => { 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"); + if !device_activated + && self.are_queues_valid() + && self.locked_device().activate(self.mem.clone()).is_err() + { + 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); } } _ if (status & FAILED) != 0 => { @@ -306,7 +314,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 +349,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"); diff --git a/src/vmm/src/devices/virtio/mod.rs b/src/vmm/src/devices/virtio/mod.rs index 23ab2914401..93b07c44760 100644 --- a/src/vmm/src/devices/virtio/mod.rs +++ b/src/vmm/src/devices/virtio/mod.rs @@ -40,6 +40,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. From 53338f7ee719794a0d016f02e96dbfc6d2e8adef Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 3 Jul 2024 18:02:06 +0100 Subject: [PATCH 4/6] test: ensure virtio device activation failure is handled correctly Add a unittest to deal with the case where virtio device activation fails. In this case, the device state needs to be put to DEVICE_NEEDS_RESET, and an interrupt should have been generated. Signed-off-by: Patrick Roy --- src/vmm/src/devices/virtio/mmio.rs | 66 +++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/src/vmm/src/devices/virtio/mmio.rs b/src/vmm/src/devices/virtio/mmio.rs index efe7e31d228..67907661b6b 100644 --- a/src/vmm/src/devices/virtio/mmio.rs +++ b/src/vmm/src/devices/virtio/mmio.rs @@ -377,6 +377,7 @@ pub(crate) mod tests { 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; @@ -390,6 +391,7 @@ pub(crate) mod tests { queues: Vec, device_activated: bool, config_bytes: [u8; 0xeff], + activate_should_error: bool, } impl DummyDevice { @@ -405,6 +407,7 @@ pub(crate) mod tests { queues: vec![Queue::new(16), Queue::new(32)], device_activated: false, config_bytes: [0; 0xeff], + activate_should_error: false, } } @@ -458,7 +461,11 @@ pub(crate) mod tests { fn activate(&mut self, _: GuestMemoryMmap) -> Result<(), ActivateError> { self.device_activated = true; - Ok(()) + if self.activate_should_error { + Err(ActivateError::BadActivate) + } else { + Ok(()) + } } fn is_activated(&self) -> bool { @@ -831,6 +838,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); From 057d0d8d781b1866c1aa80e715eb8f57ae364e0b Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 4 Jul 2024 11:12:16 +0100 Subject: [PATCH 5/6] refactor: centralize activation failure logging Log activation failures at the only call-site of `activate`, instead of inside each individual `activate` function. For this, untangle some of the `ActivationError` variants - `BadActivate` was almost exclusively used in the case where writing to the activation eventfd failed, except in the vsock device, where it was also used to indicate that the number of queues the guest gave us was wrong (which this commit factors out into its own error variant). Signed-off-by: Patrick Roy --- src/vmm/src/devices/virtio/balloon/device.rs | 3 +- .../src/devices/virtio/block/virtio/device.rs | 3 +- src/vmm/src/devices/virtio/mmio.rs | 31 ++++++++++--------- src/vmm/src/devices/virtio/mod.rs | 9 +++--- src/vmm/src/devices/virtio/net/device.rs | 3 +- src/vmm/src/devices/virtio/rng/device.rs | 5 ++- src/vmm/src/devices/virtio/vsock/device.rs | 13 +++----- 7 files changed, 31 insertions(+), 36 deletions(-) diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/src/vmm/src/devices/virtio/balloon/device.rs index 763285949f8..065d9a1fb2f 100644 --- a/src/vmm/src/devices/virtio/balloon/device.rs +++ b/src/vmm/src/devices/virtio/balloon/device.rs @@ -613,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/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs index a6043cb438d..b9a952e3daf 100644 --- a/src/vmm/src/devices/virtio/block/virtio/device.rs +++ b/src/vmm/src/devices/virtio/block/virtio/device.rs @@ -652,8 +652,7 @@ impl VirtioDevice for VirtioBlock { } if self.activate_evt.write(1).is_err() { - error!("Block: Cannot write to activate_evt"); - return Err(ActivateError::BadActivate); + return Err(ActivateError::EventFd); } self.device_state = DeviceState::Activated(mem); Ok(()) diff --git a/src/vmm/src/devices/virtio/mmio.rs b/src/vmm/src/devices/virtio/mmio.rs index 67907661b6b..7cf09f723a5 100644 --- a/src/vmm/src/devices/virtio/mmio.rs +++ b/src/vmm/src/devices/virtio/mmio.rs @@ -14,7 +14,7 @@ use utils::byte_order; 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 @@ -186,18 +186,21 @@ impl MmioTransport { DRIVER_OK if self.device_status == (ACKNOWLEDGE | DRIVER | FEATURES_OK) => { 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()).is_err() - { - 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); + if !device_activated && self.are_queues_valid() { + // 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 => { @@ -462,7 +465,7 @@ pub(crate) mod tests { fn activate(&mut self, _: GuestMemoryMmap) -> Result<(), ActivateError> { self.device_activated = true; if self.activate_should_error { - Err(ActivateError::BadActivate) + Err(ActivateError::EventFd) } else { Ok(()) } diff --git a/src/vmm/src/devices/virtio/mod.rs b/src/vmm/src/devices/virtio/mod.rs index 93b07c44760..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; @@ -61,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 f6b6d805bf5..3ab8b0ad66c 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -862,8 +862,7 @@ 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); + 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 3381a5635c6..0186ce4d7fd 100644 --- a/src/vmm/src/devices/virtio/rng/device.rs +++ b/src/vmm/src/devices/virtio/rng/device.rs @@ -284,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 c69393c1bab..d48782901ec 100644 --- a/src/vmm/src/devices/virtio/vsock/device.rs +++ b/src/vmm/src/devices/virtio/vsock/device.rs @@ -325,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); From 8eb22295223d2edf0fd662f4c15a63cac38d115a Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 4 Jul 2024 11:16:56 +0100 Subject: [PATCH 6/6] fix: insert missing `activate_fails` metrics increases Make sure that whenever the activation of a virtio device fails, we set the `activate_fails` metric. Signed-off-by: Patrick Roy --- .../src/devices/virtio/block/vhost_user/device.rs | 14 +++++++------- src/vmm/src/devices/virtio/block/virtio/device.rs | 1 + src/vmm/src/devices/virtio/net/device.rs | 1 + 3 files changed, 9 insertions(+), 7 deletions(-) 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 d5b096da885..99b27915598 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -336,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 b9a952e3daf..7313cbb8b5c 100644 --- a/src/vmm/src/devices/virtio/block/virtio/device.rs +++ b/src/vmm/src/devices/virtio/block/virtio/device.rs @@ -652,6 +652,7 @@ impl VirtioDevice for VirtioBlock { } if self.activate_evt.write(1).is_err() { + self.metrics.activate_fails.inc(); return Err(ActivateError::EventFd); } self.device_state = DeviceState::Activated(mem); diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 3ab8b0ad66c..1e0d2153296 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -862,6 +862,7 @@ impl VirtioDevice for Net { } if self.activate_evt.write(1).is_err() { + self.metrics.activate_fails.inc(); return Err(ActivateError::EventFd); } self.device_state = DeviceState::Activated(mem);