From 98d7955e3411e40f1efa13dcac3809f70689b223 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 26 Jul 2019 11:48:07 -0700 Subject: [PATCH] vm-virtio: Add support for notifying about virtio config update As per the VIRTIO specification, every virtio device configuration can be updated while the guest is running. The guest needs to be notified when this happens, and it can be done in two different ways, depending on the type of interrupt being used for those devices. In case the device uses INTx, the allocated IRQ pin is shared between queues and configuration updates. The way for the guest to differentiate between an interrupt meant for a virtqueue or meant for a configuration update is tied to the value of the ISR status field. This field is a simple 32 bits bitmask where only bit 0 and 1 can be changed, the rest is reserved. In case the device uses MSI/MSI-X, the driver should allocate a dedicated vector for configuration updates. This case is much simpler as it only requires the device to send the appropriate MSI vector. The cloud-hypervisor codebase was not supporting the update of a virtio device configuration. This patch extends the existing VirtioInterrupt closure to accept a type that can be Config or Queue, so that based on this type, the closure implementation can make the right choice about which interrupt pin or vector to trigger. Signed-off-by: Sebastien Boeuf --- vm-virtio/src/block.rs | 18 ++--- vm-virtio/src/console.rs | 10 +-- vm-virtio/src/device.rs | 13 +++- vm-virtio/src/fs.rs | 30 +++----- vm-virtio/src/net.rs | 10 +-- vm-virtio/src/pmem.rs | 12 +--- vm-virtio/src/rng.rs | 12 +--- vm-virtio/src/transport/pci_common_config.rs | 11 +-- vm-virtio/src/transport/pci_device.rs | 72 +++++++++++++------- 9 files changed, 93 insertions(+), 95 deletions(-) diff --git a/vm-virtio/src/block.rs b/vm-virtio/src/block.rs index 084889fd6..04aad2a21 100755 --- a/vm-virtio/src/block.rs +++ b/vm-virtio/src/block.rs @@ -17,14 +17,13 @@ use std::os::linux::fs::MetadataExt; use std::os::unix::io::AsRawFd; use std::path::PathBuf; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use std::thread; use super::Error as DeviceError; use super::{ ActivateError, ActivateResult, DescriptorChain, DeviceEventT, Queue, VirtioDevice, - VirtioDeviceType, INTERRUPT_STATUS_USED_RING, + VirtioDeviceType, VirtioInterruptType, }; use crate::VirtioInterrupt; use virtio_bindings::virtio_blk::*; @@ -325,7 +324,6 @@ struct BlockEpollHandler { mem: GuestMemoryMmap, disk_image: T, disk_nsectors: u64, - interrupt_status: Arc, interrupt_cb: Arc, disk_image_id: Vec, } @@ -376,12 +374,12 @@ impl BlockEpollHandler { } fn signal_used_queue(&self, queue_index: usize) -> result::Result<(), DeviceError> { - self.interrupt_status - .fetch_or(INTERRUPT_STATUS_USED_RING as usize, Ordering::SeqCst); - (self.interrupt_cb)(&self.queues[queue_index]).map_err(|e| { - error!("Failed to signal used queue: {:?}", e); - DeviceError::FailedSignalingUsedQueue(e) - }) + (self.interrupt_cb)(&VirtioInterruptType::Queue, Some(&self.queues[queue_index])).map_err( + |e| { + error!("Failed to signal used queue: {:?}", e); + DeviceError::FailedSignalingUsedQueue(e) + }, + ) } #[allow(dead_code)] @@ -600,7 +598,6 @@ impl VirtioDevice for Block { &mut self, mem: GuestMemoryMmap, interrupt_cb: Arc, - status: Arc, queues: Vec, mut queue_evts: Vec, ) -> ActivateResult { @@ -644,7 +641,6 @@ impl VirtioDevice for Block { mem, disk_image, disk_nsectors: self.disk_nsectors, - interrupt_status: status, interrupt_cb, disk_image_id, }; diff --git a/vm-virtio/src/console.rs b/vm-virtio/src/console.rs index 2b2a23883..0827223e0 100755 --- a/vm-virtio/src/console.rs +++ b/vm-virtio/src/console.rs @@ -10,14 +10,13 @@ use std::io; use std::io::Write; use std::os::unix::io::AsRawFd; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::thread; use super::Error as DeviceError; use super::{ ActivateError, ActivateResult, DeviceEventT, Queue, VirtioDevice, VirtioDeviceType, - INTERRUPT_STATUS_USED_RING, VIRTIO_F_VERSION_1, + VirtioInterruptType, VIRTIO_F_VERSION_1, }; use crate::VirtioInterrupt; use vm_memory::{Bytes, GuestMemoryMmap}; @@ -38,7 +37,6 @@ const KILL_EVENT: DeviceEventT = 3; struct ConsoleEpollHandler { queues: Vec, mem: GuestMemoryMmap, - interrupt_status: Arc, interrupt_cb: Arc, in_buffer: Arc>>, out: Box, @@ -128,9 +126,7 @@ impl ConsoleEpollHandler { } fn signal_used_queue(&self) -> result::Result<(), DeviceError> { - self.interrupt_status - .fetch_or(INTERRUPT_STATUS_USED_RING as usize, Ordering::SeqCst); - (self.interrupt_cb)(&self.queues[0]).map_err(|e| { + (self.interrupt_cb)(&VirtioInterruptType::Queue, Some(&self.queues[0])).map_err(|e| { error!("Failed to signal used queue: {:?}", e); DeviceError::FailedSignalingUsedQueue(e) }) @@ -333,7 +329,6 @@ impl VirtioDevice for Console { &mut self, mem: GuestMemoryMmap, interrupt_cb: Arc, - status: Arc, queues: Vec, mut queue_evts: Vec, ) -> ActivateResult { @@ -360,7 +355,6 @@ impl VirtioDevice for Console { let mut handler = ConsoleEpollHandler { queues, mem, - interrupt_status: status, interrupt_cb, in_buffer: self.input.in_buffer.clone(), out, diff --git a/vm-virtio/src/device.rs b/vm-virtio/src/device.rs index 5fcb4d318..6be4638f6 100644 --- a/vm-virtio/src/device.rs +++ b/vm-virtio/src/device.rs @@ -8,12 +8,20 @@ use super::*; use pci::{PciBarConfiguration, PciCapability}; -use std::sync::atomic::AtomicUsize; use std::sync::Arc; use vm_memory::GuestMemoryMmap; use vmm_sys_util::EventFd; -pub type VirtioInterrupt = Box std::result::Result<(), std::io::Error> + Send + Sync>; +pub enum VirtioInterruptType { + Config, + Queue, +} + +pub type VirtioInterrupt = Box< + Fn(&VirtioInterruptType, Option<&Queue>) -> std::result::Result<(), std::io::Error> + + Send + + Sync, +>; /// Trait for virtio devices to be driven by a virtio transport. /// @@ -49,7 +57,6 @@ pub trait VirtioDevice: Send { &mut self, mem: GuestMemoryMmap, interrupt_evt: Arc, - status: Arc, queues: Vec, queue_evts: Vec, ) -> ActivateResult; diff --git a/vm-virtio/src/fs.rs b/vm-virtio/src/fs.rs index f777872fb..9f21b6a72 100644 --- a/vm-virtio/src/fs.rs +++ b/vm-virtio/src/fs.rs @@ -2,11 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 use super::Error as DeviceError; -use super::{ - ActivateError, ActivateResult, Queue, VirtioDevice, VirtioDeviceType, - INTERRUPT_STATUS_USED_RING, -}; -use crate::{VirtioInterrupt, VIRTIO_F_VERSION_1_BITMASK}; +use super::{ActivateError, ActivateResult, Queue, VirtioDevice, VirtioDeviceType}; +use crate::{VirtioInterrupt, VirtioInterruptType, VIRTIO_F_VERSION_1_BITMASK}; use epoll; use libc::EFD_NONBLOCK; use std::cmp; @@ -14,7 +11,6 @@ use std::io; use std::io::Write; use std::os::unix::io::AsRawFd; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use std::thread; use vhost_rs::vhost_user::message::{VhostUserProtocolFeatures, VhostUserVirtioFeatures}; @@ -92,7 +88,6 @@ type Result = result::Result; struct FsEpollHandler { vu_call_evt_queue_list: Vec<(EventFd, Queue)>, - interrupt_status: Arc, interrupt_cb: Arc, kill_evt: EventFd, } @@ -137,16 +132,15 @@ impl FsEpollHandler { if let Err(e) = self.vu_call_evt_queue_list[x].0.read() { error!("Failed to get queue event: {:?}", e); break 'epoll; - } else { - self.interrupt_status - .fetch_or(INTERRUPT_STATUS_USED_RING as usize, Ordering::SeqCst); - if let Err(e) = (self.interrupt_cb)(&self.vu_call_evt_queue_list[x].1) { - error!( - "Failed to signal used queue: {:?}", - DeviceError::FailedSignalingUsedQueue(e) - ); - break 'epoll; - } + } else if let Err(e) = (self.interrupt_cb)( + &VirtioInterruptType::Queue, + Some(&self.vu_call_evt_queue_list[x].1), + ) { + error!( + "Failed to signal used queue: {:?}", + DeviceError::FailedSignalingUsedQueue(e) + ); + break 'epoll; } } x if (x == kill_evt_index) => { @@ -390,7 +384,6 @@ impl VirtioDevice for Fs { &mut self, mem: GuestMemoryMmap, interrupt_cb: Arc, - status: Arc, queues: Vec, queue_evts: Vec, ) -> ActivateResult { @@ -419,7 +412,6 @@ impl VirtioDevice for Fs { let mut handler = FsEpollHandler { vu_call_evt_queue_list, - interrupt_status: status, interrupt_cb, kill_evt, }; diff --git a/vm-virtio/src/net.rs b/vm-virtio/src/net.rs index 2426f46f1..0050fb426 100644 --- a/vm-virtio/src/net.rs +++ b/vm-virtio/src/net.rs @@ -15,7 +15,6 @@ use std::mem; use std::net::Ipv4Addr; use std::os::unix::io::AsRawFd; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use std::thread; use std::vec::Vec; @@ -25,7 +24,7 @@ use net_gen; use super::Error as DeviceError; use super::{ ActivateError, ActivateResult, DeviceEventT, Queue, VirtioDevice, VirtioDeviceType, - INTERRUPT_STATUS_USED_RING, + VirtioInterruptType, }; use crate::VirtioInterrupt; use net_util::{MacAddr, Tap, TapError, MAC_ADDR_LEN}; @@ -120,16 +119,13 @@ struct NetEpollHandler { tap: Tap, rx: RxVirtio, tx: TxVirtio, - interrupt_status: Arc, interrupt_cb: Arc, kill_evt: EventFd, } impl NetEpollHandler { fn signal_used_queue(&self, queue: &Queue) -> result::Result<(), DeviceError> { - self.interrupt_status - .fetch_or(INTERRUPT_STATUS_USED_RING as usize, Ordering::SeqCst); - (self.interrupt_cb)(queue).map_err(|e| { + (self.interrupt_cb)(&VirtioInterruptType::Queue, Some(queue)).map_err(|e| { error!("Failed to signal used queue: {:?}", e); DeviceError::FailedSignalingUsedQueue(e) }) @@ -538,7 +534,6 @@ impl VirtioDevice for Net { &mut self, mem: GuestMemoryMmap, interrupt_cb: Arc, - status: Arc, mut queues: Vec, mut queue_evts: Vec, ) -> ActivateResult { @@ -571,7 +566,6 @@ impl VirtioDevice for Net { tap, rx: RxVirtio::new(rx_queue, rx_queue_evt), tx: TxVirtio::new(tx_queue, tx_queue_evt), - interrupt_status: status, interrupt_cb, kill_evt, }; diff --git a/vm-virtio/src/pmem.rs b/vm-virtio/src/pmem.rs index 152724734..cc3de2122 100644 --- a/vm-virtio/src/pmem.rs +++ b/vm-virtio/src/pmem.rs @@ -15,16 +15,15 @@ use std::io::{self, Write}; use std::mem::size_of; use std::os::unix::io::AsRawFd; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use std::thread; use super::Error as DeviceError; use super::{ ActivateError, ActivateResult, DescriptorChain, DeviceEventT, Queue, VirtioDevice, - VirtioDeviceType, INTERRUPT_STATUS_USED_RING, VIRTIO_F_VERSION_1, + VirtioDeviceType, VIRTIO_F_VERSION_1, }; -use crate::VirtioInterrupt; +use crate::{VirtioInterrupt, VirtioInterruptType}; use vm_memory::{ Address, ByteValued, Bytes, GuestAddress, GuestMemoryError, GuestMemoryMmap, GuestUsize, }; @@ -157,7 +156,6 @@ struct PmemEpollHandler { queue: Queue, mem: GuestMemoryMmap, disk: File, - interrupt_status: Arc, interrupt_cb: Arc, queue_evt: EventFd, kill_evt: EventFd, @@ -209,9 +207,7 @@ impl PmemEpollHandler { } fn signal_used_queue(&self) -> result::Result<(), DeviceError> { - self.interrupt_status - .fetch_or(INTERRUPT_STATUS_USED_RING as usize, Ordering::SeqCst); - (self.interrupt_cb)(&self.queue).map_err(|e| { + (self.interrupt_cb)(&VirtioInterruptType::Queue, Some(&self.queue)).map_err(|e| { error!("Failed to signal used queue: {:?}", e); DeviceError::FailedSignalingUsedQueue(e) }) @@ -374,7 +370,6 @@ impl VirtioDevice for Pmem { &mut self, mem: GuestMemoryMmap, interrupt_cb: Arc, - status: Arc, mut queues: Vec, mut queue_evts: Vec, ) -> ActivateResult { @@ -402,7 +397,6 @@ impl VirtioDevice for Pmem { queue: queues.remove(0), mem, disk, - interrupt_status: status, interrupt_cb, queue_evt: queue_evts.remove(0), kill_evt, diff --git a/vm-virtio/src/rng.rs b/vm-virtio/src/rng.rs index 78a9f451c..b86692b7a 100755 --- a/vm-virtio/src/rng.rs +++ b/vm-virtio/src/rng.rs @@ -9,16 +9,15 @@ use std::fs::File; use std::io; use std::os::unix::io::AsRawFd; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; use std::thread; use super::Error as DeviceError; use super::{ ActivateError, ActivateResult, DeviceEventT, Queue, VirtioDevice, VirtioDeviceType, - INTERRUPT_STATUS_USED_RING, VIRTIO_F_VERSION_1, + VIRTIO_F_VERSION_1, }; -use crate::VirtioInterrupt; +use crate::{VirtioInterrupt, VirtioInterruptType}; use vm_memory::{Bytes, GuestMemoryMmap}; use vmm_sys_util::EventFd; @@ -35,7 +34,6 @@ struct RngEpollHandler { queues: Vec, mem: GuestMemoryMmap, random_file: File, - interrupt_status: Arc, interrupt_cb: Arc, queue_evt: EventFd, kill_evt: EventFd, @@ -77,9 +75,7 @@ impl RngEpollHandler { } fn signal_used_queue(&self) -> result::Result<(), DeviceError> { - self.interrupt_status - .fetch_or(INTERRUPT_STATUS_USED_RING as usize, Ordering::SeqCst); - (self.interrupt_cb)(&self.queues[0]).map_err(|e| { + (self.interrupt_cb)(&VirtioInterruptType::Queue, Some(&self.queues[0])).map_err(|e| { error!("Failed to signal used queue: {:?}", e); DeviceError::FailedSignalingUsedQueue(e) }) @@ -229,7 +225,6 @@ impl VirtioDevice for Rng { &mut self, mem: GuestMemoryMmap, interrupt_cb: Arc, - status: Arc, queues: Vec, mut queue_evts: Vec, ) -> ActivateResult { @@ -257,7 +252,6 @@ impl VirtioDevice for Rng { queues, mem, random_file, - interrupt_status: status, interrupt_cb, queue_evt: queue_evts.remove(0), kill_evt, diff --git a/vm-virtio/src/transport/pci_common_config.rs b/vm-virtio/src/transport/pci_common_config.rs index 8daf31039..edbf84296 100644 --- a/vm-virtio/src/transport/pci_common_config.rs +++ b/vm-virtio/src/transport/pci_common_config.rs @@ -8,6 +8,8 @@ extern crate byteorder; use byteorder::{ByteOrder, LittleEndian}; +use std::sync::atomic::{AtomicU16, Ordering}; +use std::sync::Arc; use vm_memory::GuestAddress; use crate::{Queue, VirtioDevice}; @@ -40,7 +42,7 @@ pub struct VirtioPciCommonConfig { pub device_feature_select: u32, pub driver_feature_select: u32, pub queue_select: u16, - pub msix_config: u16, + pub msix_config: Arc, } impl VirtioPciCommonConfig { @@ -120,7 +122,7 @@ impl VirtioPciCommonConfig { fn read_common_config_word(&self, offset: u64, queues: &[Queue]) -> u16 { debug!("read_common_config_word: offset 0x{:x}", offset); match offset { - 0x10 => self.msix_config, + 0x10 => self.msix_config.load(Ordering::SeqCst), 0x12 => queues.len() as u16, // num_queues 0x16 => self.queue_select, 0x18 => self.with_queue(queues, |q| q.size).unwrap_or(0), @@ -143,7 +145,7 @@ impl VirtioPciCommonConfig { fn write_common_config_word(&mut self, offset: u64, value: u16, queues: &mut Vec) { debug!("write_common_config_word: offset 0x{:x}", offset); match offset { - 0x10 => self.msix_config = value, + 0x10 => self.msix_config.store(value, Ordering::SeqCst), 0x16 => self.queue_select = value, 0x18 => self.with_queue_mut(queues, |q| q.size = value), 0x1a => self.with_queue_mut(queues, |q| q.vector = value), @@ -272,7 +274,6 @@ mod tests { &mut self, _mem: GuestMemoryMmap, _interrupt_evt: Arc, - _status: Arc, _queues: Vec, _queue_evts: Vec, ) -> ActivateResult { @@ -298,7 +299,7 @@ mod tests { device_feature_select: 0x0, driver_feature_select: 0x0, queue_select: 0xff, - msix_config: 0, + msix_config: Arc::new(AtomicU16::new(0)), }; let dev = &mut DummyDevice(0) as &mut dyn VirtioDevice; diff --git a/vm-virtio/src/transport/pci_device.rs b/vm-virtio/src/transport/pci_device.rs index a4916d281..39cadeb6a 100755 --- a/vm-virtio/src/transport/pci_device.rs +++ b/vm-virtio/src/transport/pci_device.rs @@ -13,7 +13,7 @@ extern crate vm_memory; extern crate vmm_sys_util; use libc::EFD_NONBLOCK; -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicU16, AtomicUsize, Ordering}; use std::sync::Arc; use std::sync::Mutex; @@ -30,8 +30,9 @@ use vmm_sys_util::{EventFd, Result}; use super::VirtioPciCommonConfig; use crate::{ - Queue, VirtioDevice, VirtioDeviceType, VirtioInterrupt, DEVICE_ACKNOWLEDGE, DEVICE_DRIVER, - DEVICE_DRIVER_OK, DEVICE_FAILED, DEVICE_FEATURES_OK, DEVICE_INIT, + Queue, VirtioDevice, VirtioDeviceType, VirtioInterrupt, VirtioInterruptType, + DEVICE_ACKNOWLEDGE, DEVICE_DRIVER, DEVICE_DRIVER_OK, DEVICE_FAILED, DEVICE_FEATURES_OK, + DEVICE_INIT, INTERRUPT_STATUS_CONFIG_CHANGED, INTERRUPT_STATUS_USED_RING, }; #[allow(clippy::enum_variant_names)] @@ -254,7 +255,7 @@ impl VirtioPciDevice { device_feature_select: 0, driver_feature_select: 0, queue_select: 0, - msix_config: 0, + msix_config: Arc::new(AtomicU16::new(0)), }, msix_config, msix_num, @@ -376,10 +377,20 @@ impl PciDevice for VirtioPciDevice { ) { self.configuration.set_irq(irq_num as u8, irq_pin); - let cb = Arc::new(Box::new(move |_queue: &Queue| { - let param = InterruptParameters { msix: None }; - (irq_cb)(param) - }) as VirtioInterrupt); + let interrupt_status = self.interrupt_status.clone(); + let cb = Arc::new(Box::new( + move |int_type: &VirtioInterruptType, _queue: Option<&Queue>| { + let param = InterruptParameters { msix: None }; + + let status = match int_type { + VirtioInterruptType::Config => INTERRUPT_STATUS_CONFIG_CHANGED, + VirtioInterruptType::Queue => INTERRUPT_STATUS_USED_RING, + }; + interrupt_status.fetch_or(status as usize, Ordering::SeqCst); + + (irq_cb)(param) + }, + ) as VirtioInterrupt); self.interrupt_cb = Some(cb); } @@ -393,22 +404,38 @@ impl PciDevice for VirtioPciDevice { let msix_config_clone = msix_config.clone(); - let cb = Arc::new(Box::new(move |queue: &Queue| { - let config = &mut msix_config_clone.lock().unwrap(); - let entry = &config.table_entries[queue.vector as usize]; + let common_config_msi_vector = self.common_config.msix_config.clone(); + let cb = Arc::new(Box::new( + move |int_type: &VirtioInterruptType, queue: Option<&Queue>| { + let vector = match int_type { + VirtioInterruptType::Config => { + common_config_msi_vector.load(Ordering::SeqCst) + } + VirtioInterruptType::Queue => { + if let Some(q) = queue { + q.vector + } else { + 0 + } + } + }; - // In case the vector control register associated with the entry - // has its first bit set, this means the vector is masked and the - // device should not inject the interrupt. - // Instead, the Pending Bit Array table is updated to reflect there - // is a pending interrupt for this specific vector. - if config.masked() || entry.masked() { - config.set_pba_bit(queue.vector, false); - return Ok(()); - } + let config = &mut msix_config_clone.lock().unwrap(); + let entry = &config.table_entries[vector as usize]; - (msi_cb)(InterruptParameters { msix: Some(entry) }) - }) as VirtioInterrupt); + // In case the vector control register associated with the entry + // has its first bit set, this means the vector is masked and the + // device should not inject the interrupt. + // Instead, the Pending Bit Array table is updated to reflect there + // is a pending interrupt for this specific vector. + if config.masked() || entry.masked() { + config.set_pba_bit(vector, false); + return Ok(()); + } + + (msi_cb)(InterruptParameters { msix: Some(entry) }) + }, + ) as VirtioInterrupt); self.interrupt_cb = Some(cb); } @@ -585,7 +612,6 @@ impl PciDevice for VirtioPciDevice { .activate( mem, interrupt_cb, - self.interrupt_status.clone(), self.queues.clone(), self.queue_evts.split_off(0), )