diff --git a/src/vmm/src/arch/x86_64/mptable.rs b/src/vmm/src/arch/x86_64/mptable.rs index 6b6bde062df..dfd2a8b6a35 100644 --- a/src/vmm/src/arch/x86_64/mptable.rs +++ b/src/vmm/src/arch/x86_64/mptable.rs @@ -82,7 +82,7 @@ const MPC_SIGNATURE: [c_char; 4] = char_array!(c_char; 'P', 'C', 'M', 'P'); const MPC_SPEC: i8 = 4; const MPC_OEM: [c_char; 8] = char_array!(c_char; 'F', 'C', ' ', ' ', ' ', ' ', ' ', ' '); const MPC_PRODUCT_ID: [c_char; 12] = ['0' as c_char; 12]; -const BUS_TYPE_ISA: [u8; 6] = [b'I', b'S', b'A', b' ', b' ', b' ']; +const BUS_TYPE_ISA: [u8; 6] = *b"ISA "; const IO_APIC_DEFAULT_PHYS_BASE: u32 = 0xfec0_0000; // source: linux/arch/x86/include/asm/apicdef.h const APIC_DEFAULT_PHYS_BASE: u32 = 0xfee0_0000; // source: linux/arch/x86/include/asm/apicdef.h const APIC_VERSION: u8 = 0x14; diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index fbdf24d80f1..d01ed7c863d 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -80,8 +80,6 @@ pub enum StartMicrovmError { CreateNetDevice(crate::devices::virtio::net::NetError), /// Cannot create pmem device: {0} CreatePmemDevice(#[from] crate::devices::virtio::pmem::device::PmemError), - /// Cannot create RateLimiter: {0} - CreateRateLimiter(io::Error), /// Error creating legacy device: {0} #[cfg(target_arch = "x86_64")] CreateLegacyDevice(device_manager::legacy::LegacyDeviceError), diff --git a/src/vmm/src/device_manager/legacy.rs b/src/vmm/src/device_manager/legacy.rs index 799ac1631f9..dd64ff91882 100644 --- a/src/vmm/src/device_manager/legacy.rs +++ b/src/vmm/src/device_manager/legacy.rs @@ -12,14 +12,11 @@ use acpi_tables::aml::AmlError; use acpi_tables::{Aml, aml}; use crate::devices::legacy::{I8042Device, SerialDevice}; -use crate::vstate::bus::BusError; use crate::vstate::vm::KvmVm; /// Errors corresponding to the `PortIODeviceManager`. #[derive(Debug, derive_more::From, thiserror::Error, displaydoc::Display)] pub enum LegacyDeviceError { - /// Failed to add legacy device to Bus: {0} - BusError(BusError), /// Failed to create EventFd: {0} EventFd(std::io::Error), } @@ -58,12 +55,12 @@ impl PortIODeviceManager { self.stdio_serial.clone(), Self::SERIAL_PORT_ADDRESS, Self::SERIAL_PORT_SIZE, - )?; + ); io_bus.insert( self.i8042.clone(), Self::I8042_KDB_DATA_REGISTER_ADDRESS, Self::I8042_KDB_DATA_REGISTER_SIZE, - )?; + ); vm.register_irq( self.stdio_serial diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index 78b7bbd8c44..1c9d39807c2 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -28,7 +28,7 @@ use crate::devices::virtio::device::{VirtioDevice, VirtioDeviceId, VirtioDeviceT use crate::devices::virtio::transport::mmio::MmioTransport; #[cfg(target_arch = "x86_64")] use crate::logger::debug; -use crate::vstate::bus::{Bus, BusError}; +use crate::vstate::bus::Bus; #[cfg(target_arch = "x86_64")] use crate::vstate::memory::GuestAddress; use crate::vstate::resources::ResourceAllocator; @@ -39,8 +39,6 @@ use crate::vstate::vm::KvmVm; pub enum MmioError { /// Failed to allocate requested resource: {0} Allocator(#[from] vm_allocator::Error), - /// Failed to insert device on the bus: {0} - BusInsert(#[from] BusError), /// Failed to allocate requested resourc: {0} Cmdline(#[from] linux_loader::cmdline::Error), /// Could not create IRQ for MMIO device: {0} @@ -206,7 +204,7 @@ impl MMIODeviceManager { device.inner.clone(), device.resources.addr, device.resources.len, - )?; + ); let sub_id = event_manager.add_subscriber(device.inner.lock().expect("Poisoned lock").device()); @@ -311,7 +309,7 @@ impl MMIODeviceManager { device.inner.clone(), device.resources.addr, device.resources.len, - )?; + ); self.serial = Some(device); Ok(()) @@ -368,7 +366,7 @@ impl MMIODeviceManager { device.inner.clone(), device.resources.addr, device.resources.len, - )?; + ); self.rtc = Some(device); Ok(()) } @@ -396,7 +394,7 @@ impl MMIODeviceManager { device.inner.clone(), device.resources.addr, device.resources.len, - )?; + ); self.boot_timer = Some(device); Ok(()) diff --git a/src/vmm/src/device_manager/mod.rs b/src/vmm/src/device_manager/mod.rs index 95c8877eead..11b1636538b 100644 --- a/src/vmm/src/device_manager/mod.rs +++ b/src/vmm/src/device_manager/mod.rs @@ -57,7 +57,6 @@ use crate::vmm_config::drive::{BlockDeviceConfig, DriveError}; use crate::vmm_config::mmds::MmdsConfigError; use crate::vmm_config::net::{NetBuilder, NetworkInterfaceConfig, NetworkInterfaceError}; use crate::vmm_config::pmem::{PmemConfig, PmemConfigError}; -use crate::vstate::bus::BusError; use crate::vstate::memory::GuestMemoryMmap; use crate::vstate::vm::{KvmVm, Vm}; @@ -89,8 +88,6 @@ pub enum DeviceManagerCreateError { pub enum AttachDeviceError { /// MMIO transport error: {0} MmioTransport(#[from] MmioError), - /// Error inserting device in bus: {0} - Bus(#[from] BusError), /// Error while registering ACPI with KVM: {0} AttachAcpiDevice(#[from] ACPIDeviceError), #[cfg(target_arch = "aarch64")] @@ -605,8 +602,7 @@ impl DeviceManager { vm.common .mmio_bus - .remove(pci_device.config_bar_addr(), CAPABILITY_BAR_SIZE) - .map_err(PciManagerError::Bus)?; + .remove(pci_device.config_bar_addr(), CAPABILITY_BAR_SIZE); self.pci_devices .pci_segment @@ -657,8 +653,6 @@ pub enum DevicePersistError { MmioTransport, /// PCI Device manager: {0} PciDeviceManager(#[from] PciManagerError), - /// Bus error: {0} - Bus(#[from] BusError), #[cfg(target_arch = "aarch64")] /// Legacy: {0} Legacy(#[from] std::io::Error), @@ -691,8 +685,6 @@ pub enum DeviceManagerPersistError { AcpiRestore(#[from] ACPIDeviceError), /// Error restoring PCI devices: {0} PciRestore(DevicePersistError), - /// Error inserting device in bus: {0} - Bus(#[from] BusError), /// Error creating DeviceManager: {0} DeviceManager(#[from] DeviceManagerCreateError), } @@ -787,9 +779,9 @@ impl<'a> Persist<'a> for DeviceManager { #[cfg(test)] pub(crate) mod tests { - use super::*; use vmm_sys_util::tempfile::TempFile; + use super::*; use crate::builder::tests::{ CustomBlockConfig, default_kernel_cmdline, default_vmm, insert_block_devices, }; diff --git a/src/vmm/src/device_manager/pci_mngr.rs b/src/vmm/src/device_manager/pci_mngr.rs index 95e9290cdc9..a3ee4962c88 100644 --- a/src/vmm/src/device_manager/pci_mngr.rs +++ b/src/vmm/src/device_manager/pci_mngr.rs @@ -39,7 +39,6 @@ use crate::pci::bus::PciRootError; use crate::resources::VmResources; use crate::snapshot::Persist; use crate::vmm_config::memory_hotplug::MemoryHotplugConfig; -use crate::vstate::bus::BusError; use crate::vstate::interrupts::InterruptError; use crate::vstate::memory::GuestMemoryMmap; use crate::vstate::vm::KvmVm; @@ -56,8 +55,6 @@ pub struct PciDevices { pub enum PciManagerError { /// Resource allocation error: {0} ResourceAllocation(#[from] vm_allocator::Error), - /// Bus error: {0} - Bus(#[from] BusError), /// PCI root error: {0} PciRoot(#[from] PciRootError), /// MSI error: {0} @@ -80,16 +77,13 @@ impl PciDevices { // Currently we don't assign any IRQs to PCI devices. We will be using MSI-X interrupts // only. - let pci_segment = PciSegment::new(0, vm, &[0u8; 32])?; + let pci_segment = PciSegment::new(0, vm, &[0u8; 32]); self.pci_segment = Some(pci_segment); Ok(()) } - fn register_bars_with_bus( - vm: &KvmVm, - virtio_device: &Arc>, - ) -> Result<(), PciManagerError> { + fn register_bars_with_bus(vm: &KvmVm, virtio_device: &Arc>) { let virtio_device_locked = virtio_device.lock().expect("Poisoned lock"); debug!( @@ -101,9 +95,7 @@ impl PciDevices { virtio_device.clone(), virtio_device_locked.config_bar_addr(), CAPABILITY_BAR_SIZE, - )?; - - Ok(()) + ); } fn attach_common( @@ -127,7 +119,7 @@ impl PciDevices { self.virtio_devices .insert((device_type, id), virtio_device.clone()); - Self::register_bars_with_bus(vm, &virtio_device)?; + Self::register_bars_with_bus(vm, &virtio_device); let mut device = virtio_device.lock().expect("Poisoned lock"); device.register_notification_ioevents(vm)?; diff --git a/src/vmm/src/devices/pci/pci_segment.rs b/src/vmm/src/devices/pci/pci_segment.rs index 1761b8c2261..c3e651b1daa 100644 --- a/src/vmm/src/devices/pci/pci_segment.rs +++ b/src/vmm/src/devices/pci/pci_segment.rs @@ -23,7 +23,7 @@ use crate::pci::PciSBDF; #[cfg(target_arch = "x86_64")] use crate::pci::bus::{PCI_CONFIG_IO_PORT, PCI_CONFIG_IO_PORT_SIZE}; use crate::pci::bus::{PciBus, PciConfigIo, PciConfigMmio, PciRoot, PciRootError}; -use crate::vstate::bus::{BusDeviceSync, BusError}; +use crate::vstate::bus::BusDeviceSync; use crate::vstate::resources::ResourceAllocator; use crate::vstate::vm::KvmVm; @@ -70,7 +70,7 @@ impl std::fmt::Debug for PciSegment { } impl PciSegment { - fn build(id: u16, vm: &Arc, pci_irq_slots: &[u8; 32]) -> Result { + fn build(id: u16, vm: &Arc, pci_irq_slots: &[u8; 32]) -> PciSegment { let pci_root = PciRoot::new(None); let pci_bus = Arc::new(Mutex::new(PciBus::new(pci_root))); @@ -81,7 +81,7 @@ impl PciSegment { Arc::clone(&pci_config_mmio) as Arc, mmio_config_address, PCI_MMIO_CONFIG_SIZE_PER_SEGMENT, - )?; + ); let resource_allocator = vm.resource_allocator(); @@ -91,7 +91,7 @@ impl PciSegment { let start_of_mem64_area = resource_allocator.mmio64_memory.base(); let end_of_mem64_area = resource_allocator.mmio64_memory.end(); - let segment = PciSegment { + PciSegment { id, pci_bus, pci_config_mmio, @@ -106,25 +106,19 @@ impl PciSegment { start_of_mem64_area, end_of_mem64_area, pci_irq_slots: *pci_irq_slots, - }; - - Ok(segment) + } } #[cfg(target_arch = "x86_64")] - pub(crate) fn new( - id: u16, - vm: &Arc, - pci_irq_slots: &[u8; 32], - ) -> Result { - let mut segment = Self::build(id, vm, pci_irq_slots)?; + pub(crate) fn new(id: u16, vm: &Arc, pci_irq_slots: &[u8; 32]) -> PciSegment { + let mut segment = Self::build(id, vm, pci_irq_slots); let pci_config_io = Arc::new(Mutex::new(PciConfigIo::new(Arc::clone(&segment.pci_bus)))); vm.pio_bus.insert( pci_config_io.clone(), PCI_CONFIG_IO_PORT, PCI_CONFIG_IO_PORT_SIZE, - )?; + ); segment.pci_config_io = Some(pci_config_io); @@ -140,16 +134,12 @@ impl PciSegment { PCI_CONFIG_IO_PORT + PCI_CONFIG_IO_PORT_SIZE - 1 ); - Ok(segment) + segment } #[cfg(target_arch = "aarch64")] - pub(crate) fn new( - id: u16, - vm: &Arc, - pci_irq_slots: &[u8; 32], - ) -> Result { - let segment = Self::build(id, vm, pci_irq_slots)?; + pub(crate) fn new(id: u16, vm: &Arc, pci_irq_slots: &[u8; 32]) -> PciSegment { + let segment = Self::build(id, vm, pci_irq_slots); info!( "pci: adding PCI segment: id={:#x}, PCI MMIO config address: {:#x}, mem32 area: \ [{:#x}-{:#x}], mem64 area: [{:#x}-{:#x}]", @@ -161,7 +151,7 @@ impl PciSegment { segment.end_of_mem64_area, ); - Ok(segment) + segment } pub(crate) fn next_device_sbdf(&self) -> Result { @@ -470,7 +460,7 @@ mod tests { let vmm = default_vmm(); let kvm_vm = vmm.vm.as_kvm().unwrap().clone(); let pci_irq_slots = &[0u8; 32]; - let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots).unwrap(); + let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots); assert_eq!(pci_segment.id, 0); assert_eq!( @@ -502,7 +492,7 @@ mod tests { let vmm = default_vmm(); let kvm_vm = vmm.vm.as_kvm().unwrap().clone(); let pci_irq_slots = &[0u8; 32]; - let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots).unwrap(); + let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots); let mut data = [0u8; u64_to_usize(PCI_CONFIG_IO_PORT_SIZE)]; kvm_vm.pio_bus.read(PCI_CONFIG_IO_PORT, &mut data).unwrap(); @@ -518,7 +508,7 @@ mod tests { let vmm = default_vmm(); let kvm_vm = vmm.vm.as_kvm().unwrap().clone(); let pci_irq_slots = &[0u8; 32]; - let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots).unwrap(); + let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots); let mut data = [0u8; u64_to_usize(PCI_MMIO_CONFIG_SIZE_PER_SEGMENT)]; @@ -539,18 +529,52 @@ mod tests { #[test] fn test_next_device_bdf() { + use crate::pci::configuration::PciConfiguration; + use crate::pci::{PciClassCode, PciDevice, PciMassStorageSubclass}; + + struct PciDevMock(PciConfiguration); + impl PciDevice for PciDevMock { + fn write_config_register( + &mut self, + reg_idx: u16, + offset: u8, + data: &[u8], + ) -> Option> { + self.0.write_config_register(reg_idx, offset, data); + None + } + fn read_config_register(&mut self, reg_idx: u16) -> u32 { + self.0.read_reg(reg_idx) + } + } + fn mock_dev() -> Arc> { + Arc::new(Mutex::new(PciDevMock(PciConfiguration::new_type0( + 0x42, + 0x0, + 0x0, + PciClassCode::MassStorageController, + PciMassStorageSubclass::SerialScsiController as u8, + 0x13, + 0x12, + )))) + } + let vmm = default_vmm(); let kvm_vm = vmm.vm.as_kvm().unwrap().clone(); let pci_irq_slots = &[0u8; 32]; - let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots).unwrap(); + let pci_segment = PciSegment::new(0, &kvm_vm, pci_irq_slots); // Start checking from device id 1, since 0 is allocated to the Root port. + // `next_device_sbdf` only inspects the bus, so the caller must `add_device` + // before requesting the next id. for dev_id in 1..32 { let sbdf = pci_segment.next_device_sbdf().unwrap(); // In our case we have a single Segment with id 0, which has // a single bus with id 0. Also, each device of ours has a // single function. assert_eq!(sbdf, PciSBDF::new(0, 0, dev_id, 0)); + let mut segment = pci_segment.pci_bus.lock().unwrap(); + segment.add_device(dev_id, mock_dev()).unwrap(); } // We can only have 32 devices on a segment diff --git a/src/vmm/src/devices/virtio/balloon/device.rs b/src/vmm/src/devices/virtio/balloon/device.rs index 7c68f2bd40e..4295082b868 100644 --- a/src/vmm/src/devices/virtio/balloon/device.rs +++ b/src/vmm/src/devices/virtio/balloon/device.rs @@ -939,7 +939,12 @@ impl VirtioDevice for Balloon { .zip(end) .and_then(|(start, end)| config_space_bytes.get_mut(start..end)) else { - error!("Failed to write config space"); + warn!( + "virtio-balloon: guest driver attempted to write device config out of bounds \ + (offset={:#x}, len={:#x})", + offset, + data.len() + ); return; }; 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 5f16d965954..92638bc0c1b 100644 --- a/src/vmm/src/devices/virtio/block/vhost_user/device.rs +++ b/src/vmm/src/devices/virtio/block/vhost_user/device.rs @@ -25,7 +25,7 @@ use crate::devices::virtio::vhost_user::{VhostUserHandleBackend, VhostUserHandle use crate::devices::virtio::vhost_user_metrics::{ VhostUserDeviceMetrics, VhostUserMetricsPerDevice, }; -use crate::logger::{IncMetric, StoreMetric, error, log_dev_preview_warning}; +use crate::logger::{IncMetric, StoreMetric, log_dev_preview_warning, warn}; use crate::utils::u64_to_usize; use crate::vmm_config::drive::BlockDeviceConfig; use crate::vstate::memory::GuestMemoryMmap; diff --git a/src/vmm/src/devices/virtio/block/virtio/device.rs b/src/vmm/src/devices/virtio/block/virtio/device.rs index afdfe6ef812..6ee9c3ac4d8 100644 --- a/src/vmm/src/devices/virtio/block/virtio/device.rs +++ b/src/vmm/src/devices/virtio/block/virtio/device.rs @@ -292,9 +292,7 @@ impl VirtioBlock { let rate_limiter = config .rate_limiter - .map(RateLimiterConfig::try_into) - .transpose() - .map_err(VirtioBlockError::RateLimiter)? + .map(RateLimiter::from) .unwrap_or_default(); let mut avail_features = (1u64 << VIRTIO_F_VERSION_1) | (1u64 << VIRTIO_RING_F_EVENT_IDX); @@ -632,8 +630,13 @@ impl VirtioDevice for VirtioBlock { .zip(end) .and_then(|(start, end)| config_space_bytes.get_mut(start..end)) else { - error!("Failed to write config space"); self.metrics.cfg_fails.inc(); + warn!( + "virtio-block: guest driver attempted to write device config out of bounds \ + (offset={:#x}, len={:#x})", + offset, + data.len() + ); return; }; @@ -1684,7 +1687,7 @@ mod tests { // Create bandwidth rate limiter that allows only 5120 bytes/s with bucket size of 8 // bytes. - let mut rl = RateLimiter::new(512, 0, 100, 0, 0, 0).unwrap(); + let mut rl = RateLimiter::new(512, 0, 100, 0, 0, 0); // Use up the budget. assert!(rl.consume(512, TokenType::Bytes)); @@ -1753,7 +1756,7 @@ mod tests { let status_addr = GuestAddress(vq.dtable[2].addr.get()); // Create ops rate limiter that allows only 10 ops/s with bucket size of 1 ops. - let mut rl = RateLimiter::new(0, 0, 0, 1, 0, 100).unwrap(); + let mut rl = RateLimiter::new(0, 0, 0, 1, 0, 100); // Use up the budget. assert!(rl.consume(1, TokenType::Ops)); diff --git a/src/vmm/src/devices/virtio/mem/device.rs b/src/vmm/src/devices/virtio/mem/device.rs index fe02479adef..113ba8fd2d3 100644 --- a/src/vmm/src/devices/virtio/mem/device.rs +++ b/src/vmm/src/devices/virtio/mem/device.rs @@ -29,7 +29,7 @@ use crate::devices::virtio::queue::{ }; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; use crate::impl_device_type; -use crate::logger::{IncMetric, debug, error, info}; +use crate::logger::{IncMetric, debug, error, info, warn}; use crate::utils::{bytes_to_mib, mib_to_bytes, u64_to_usize, usize_to_u64}; use crate::vstate::interrupts::InterruptError; use crate::vstate::memory::{ @@ -641,8 +641,12 @@ impl VirtioDevice for VirtioMem { self.config.as_slice() } - fn write_config(&mut self, offset: u64, _data: &[u8]) { - error!("virtio-mem: Attempted write to read-only config space at offset {offset}"); + fn write_config(&mut self, offset: u64, data: &[u8]) { + warn!( + "virtio-mem: guest driver attempted to write device config (offset={:#x}, len={:#x})", + offset, + data.len() + ); } fn is_activated(&self) -> bool { diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index 2fb6bfaf3ea..68e0bf29d2b 100644 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -2198,7 +2198,7 @@ pub mod tests { let mut th = TestHelper::get_default(&mem); th.activate_net(); - th.net().rx_rate_limiter = RateLimiter::new(0, 0, 0, 0, 0, 0).unwrap(); + th.net().rx_rate_limiter = RateLimiter::new(0, 0, 0, 0, 0, 0); // There is no actual event on the rate limiter's timerfd. check_metric_after_block!( th.net().metrics.event_fails, @@ -2213,7 +2213,7 @@ pub mod tests { let mut th = TestHelper::get_default(&mem); th.activate_net(); - th.net().tx_rate_limiter = RateLimiter::new(0, 0, 0, 0, 0, 0).unwrap(); + th.net().tx_rate_limiter = RateLimiter::new(0, 0, 0, 0, 0, 0); th.simulate_event(NetEvent::TxRateLimiter); // There is no actual event on the rate limiter's timerfd. check_metric_after_block!( @@ -2232,7 +2232,7 @@ pub mod tests { // Test TX bandwidth rate limiting { // create bandwidth rate limiter that allows 40960 bytes/s with bucket size 4096 bytes - let mut rl = RateLimiter::new(0x1000, 0, 100, 0, 0, 0).unwrap(); + let mut rl = RateLimiter::new(0x1000, 0, 100, 0, 0, 0); // use up the budget assert!(rl.consume(0x1000, TokenType::Bytes)); @@ -2301,7 +2301,7 @@ pub mod tests { // Test RX bandwidth rate limiting { // create bandwidth rate limiter that allows 2000 bytes/s with bucket size 1000 bytes - let mut rl = RateLimiter::new(1000, 0, 1000, 0, 0, 0).unwrap(); + let mut rl = RateLimiter::new(1000, 0, 1000, 0, 0, 0); // set up RX assert!(th.net().rx_buffer.used_descriptors == 0); @@ -2386,7 +2386,7 @@ pub mod tests { // Test TX ops rate limiting { // create ops rate limiter that allows 10 ops/s with bucket size 1 ops - let mut rl = RateLimiter::new(0, 0, 0, 1, 0, 100).unwrap(); + let mut rl = RateLimiter::new(0, 0, 0, 1, 0, 100); // use up the budget assert!(rl.consume(1, TokenType::Ops)); @@ -2432,7 +2432,7 @@ pub mod tests { // Test RX ops rate limiting { // create ops rate limiter that allows 2 ops/s with bucket size 1 ops - let mut rl = RateLimiter::new(0, 0, 0, 1, 0, 1000).unwrap(); + let mut rl = RateLimiter::new(0, 0, 0, 1, 0, 1000); // set up RX assert!(th.net().rx_buffer.used_descriptors == 0); @@ -2512,8 +2512,8 @@ pub mod tests { let mut th = TestHelper::get_default(&mem); th.activate_net(); - th.net().rx_rate_limiter = RateLimiter::new(10, 0, 10, 2, 0, 2).unwrap(); - th.net().tx_rate_limiter = RateLimiter::new(10, 0, 10, 2, 0, 2).unwrap(); + th.net().rx_rate_limiter = RateLimiter::new(10, 0, 10, 2, 0, 2); + th.net().tx_rate_limiter = RateLimiter::new(10, 0, 10, 2, 0, 2); let rx_bytes = TokenBucket::new(1000, 1001, 1002).unwrap(); let rx_ops = TokenBucket::new(1003, 1004, 1005).unwrap(); diff --git a/src/vmm/src/devices/virtio/net/test_utils.rs b/src/vmm/src/devices/virtio/net/test_utils.rs index 2d3462c4345..c2fd5d73ddd 100644 --- a/src/vmm/src/devices/virtio/net/test_utils.rs +++ b/src/vmm/src/devices/virtio/net/test_utils.rs @@ -401,14 +401,13 @@ pub mod test { }; // Create the descriptor chain. - let mut iter = desc_list.iter().peekable(); let mut addr = self.data_addr() + addr_offset; - while let Some(&(index, len, flags)) = iter.next() { + for (i, &(index, len, flags)) in desc_list.iter().enumerate() { let desc = &queue.dtable[index as usize]; - desc.set(addr, len, flags, 0); - if let Some(&&(next_index, _, _)) = iter.peek() { - desc.flags.set(flags | VIRTQ_DESC_F_NEXT); - desc.next.set(next_index); + if let Some(&(next_index, _, _)) = desc_list.get(i + 1) { + desc.set(addr, len, flags | VIRTQ_DESC_F_NEXT, next_index); + } else { + desc.set(addr, len, flags, 0); } addr += u64::from(len); diff --git a/src/vmm/src/devices/virtio/pmem/device.rs b/src/vmm/src/devices/virtio/pmem/device.rs index c81bb75d19f..d603adbc12b 100644 --- a/src/vmm/src/devices/virtio/pmem/device.rs +++ b/src/vmm/src/devices/virtio/pmem/device.rs @@ -21,7 +21,7 @@ use crate::devices::virtio::pmem::metrics::{PmemMetrics, PmemMetricsPerDevice}; use crate::devices::virtio::queue::{DescriptorChain, InvalidAvailIdx, Queue, QueueError}; use crate::devices::virtio::transport::{VirtioInterrupt, VirtioInterruptType}; use crate::impl_device_type; -use crate::logger::{IncMetric, error, info}; +use crate::logger::{IncMetric, error, info, warn}; use crate::rate_limiter::{BucketUpdate, RateLimiter, TokenType}; use crate::utils::{align_up, u64_to_usize}; use crate::vmm_config::RateLimiterConfig; @@ -61,8 +61,6 @@ pub enum PmemError { Queue(#[from] QueueError), /// Error during obtaining the descriptor from the queue: {0} QueuePop(#[from] InvalidAvailIdx), - /// Error creating rate limiter: {0} - RateLimiter(std::io::Error), } const VIRTIO_PMEM_REQ_TYPE_FLUSH: u32 = 0; @@ -330,9 +328,7 @@ impl Pmem { let rate_limiter = config .rate_limiter - .map(RateLimiterConfig::try_into) - .transpose() - .map_err(PmemError::RateLimiter)? + .map(RateLimiter::from) .unwrap_or_default(); Ok(Self { @@ -553,7 +549,14 @@ impl VirtioDevice for Pmem { self.guest_region.config_space.as_slice() } - fn write_config(&mut self, _offset: u64, _data: &[u8]) {} + fn write_config(&mut self, offset: u64, data: &[u8]) { + self.metrics.cfg_fails.inc(); + warn!( + "virtio-pmem: guest driver attempted to write device config (offset={:#x}, len={:#x})", + offset, + data.len() + ); + } fn activate( &mut self, diff --git a/src/vmm/src/devices/virtio/rng/device.rs b/src/vmm/src/devices/virtio/rng/device.rs index cc98fb8b592..74bd24f2edf 100644 --- a/src/vmm/src/devices/virtio/rng/device.rs +++ b/src/vmm/src/devices/virtio/rng/device.rs @@ -514,7 +514,7 @@ mod tests { fn test_bandwidth_rate_limiter() { let mem = create_virtio_mem(); // Rate Limiter with 4000 bytes / sec allowance and no initial burst allowance - let device = Entropy::new(RateLimiter::new(4000, 0, 1000, 0, 0, 0).unwrap()).unwrap(); + let device = Entropy::new(RateLimiter::new(4000, 0, 1000, 0, 0, 0)).unwrap(); let mut th = VirtioTestHelper::::new(&mem, device); th.activate_device(&mem); @@ -562,7 +562,7 @@ mod tests { let mem = create_virtio_mem(); // Rate Limiter with unlimited bandwidth and allowance for 1 operation every 100 msec, // (10 ops/sec), without initial burst. - let device = Entropy::new(RateLimiter::new(0, 0, 0, 1, 0, 100).unwrap()).unwrap(); + let device = Entropy::new(RateLimiter::new(0, 0, 0, 1, 0, 100)).unwrap(); let mut th = VirtioTestHelper::::new(&mem, device); th.activate_device(&mem); diff --git a/src/vmm/src/devices/virtio/test_utils.rs b/src/vmm/src/devices/virtio/test_utils.rs index 94580c44c72..32a9162ed09 100644 --- a/src/vmm/src/devices/virtio/test_utils.rs +++ b/src/vmm/src/devices/virtio/test_utils.rs @@ -469,14 +469,15 @@ pub(crate) mod test { let vq = &self.virtqueues[queue]; // Create the descriptor chain - let mut iter = desc_list.iter().peekable(); - while let Some(&(index, addr, len, flags)) = iter.next() { + for window in desc_list.windows(2) { + let (index, addr, len, flags) = window[0]; + let (next_index, _, _, _) = window[1]; let desc = &vq.dtable[index as usize]; - desc.set(addr, len, flags, 0); - if let Some(&&(next_index, _, _, _)) = iter.peek() { - desc.flags.set(flags | VIRTQ_DESC_F_NEXT); - desc.next.set(next_index); - } + desc.set(addr, len, flags | VIRTQ_DESC_F_NEXT, next_index); + } + // Set the last descriptor without NEXT flag. + if let Some(&(index, addr, len, flags)) = desc_list.last() { + vq.dtable[index as usize].set(addr, len, flags, 0); } // Mark the chain as available. @@ -520,14 +521,13 @@ pub(crate) mod test { let vq = &self.virtqueues[queue]; // Create the descriptor chain - let mut iter = desc_list.iter().peekable(); let mut addr = self.data_address() + addr_offset; - while let Some(&(index, len, flags)) = iter.next() { + for (i, &(index, len, flags)) in desc_list.iter().enumerate() { let desc = &vq.dtable[index as usize]; - desc.set(addr, len, flags, 0); - if let Some(&&(next_index, _, _)) = iter.peek() { - desc.flags.set(flags | VIRTQ_DESC_F_NEXT); - desc.next.set(next_index); + if let Some(&(next_index, _, _)) = desc_list.get(i + 1) { + desc.set(addr, len, flags | VIRTQ_DESC_F_NEXT, next_index); + } else { + desc.set(addr, len, flags, 0); } addr += u64::from(len); diff --git a/src/vmm/src/dumbo/tcp/endpoint.rs b/src/vmm/src/dumbo/tcp/endpoint.rs index c8f536aaf02..fb2a4c63273 100644 --- a/src/vmm/src/dumbo/tcp/endpoint.rs +++ b/src/vmm/src/dumbo/tcp/endpoint.rs @@ -39,6 +39,8 @@ const CONNECTION_RTO_COUNT_MAX: u16 = 15; // since it effectively limits the size of the keys (URIs) we're willing to use. const RCV_BUF_MAX_SIZE: u32 = 2500; +const _: () = assert!(RCV_BUF_MAX_SIZE <= MAX_WINDOW_SIZE); + // Represents the local endpoint of a HTTP over TCP connection which carries GET requests // to the MMDS. #[derive(Debug)] @@ -90,20 +92,12 @@ impl Endpoint { /// unit. /// - `connection_rto_count_max`: How many consecutive timeout-based retransmission may occur /// before the connection resets itself. - /// ## Panics: - /// - `assert!(RCV_BUF_MAX_SIZE <= MAX_WINDOW_SIZE as usize);` pub fn new( segment: &TcpSegment, eviction_threshold: NonZeroU64, connection_rto_period: NonZeroU64, connection_rto_count_max: NonZeroU16, ) -> Result { - // This simplifies things, and is a very reasonable assumption. - #[allow(clippy::assertions_on_constants)] - { - assert!(RCV_BUF_MAX_SIZE <= MAX_WINDOW_SIZE); - } - let connection = Connection::passive_open( segment, RCV_BUF_MAX_SIZE, diff --git a/src/vmm/src/pci/bus.rs b/src/vmm/src/pci/bus.rs index b31180c77a1..e8890c905cc 100644 --- a/src/vmm/src/pci/bus.rs +++ b/src/vmm/src/pci/bus.rs @@ -5,7 +5,6 @@ // // SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause -use std::collections::HashMap; use std::fmt::Debug; use std::sync::{Arc, Barrier, Mutex}; @@ -76,17 +75,17 @@ impl PciDevice for PciRoot { } /// A PCI bus definition +#[derive(Default)] pub struct PciBus { - /// Devices attached to this bus. - /// Device 0 is host bridge. - pub devices: HashMap>>, - device_ids: Vec, + /// Devices attached to this bus. Slot 0 is reserved for host bridge. + pub devices: [Option>>; NUM_DEVICE_IDS], } impl Debug for PciBus { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let occupied: [bool; NUM_DEVICE_IDS] = std::array::from_fn(|i| self.devices[i].is_some()); f.debug_struct("Root Firecracker PCI Bus") - .field("device_ids", &self.device_ids) + .field("device_ids", &occupied) .finish() } } @@ -94,50 +93,41 @@ impl Debug for PciBus { impl PciBus { /// Create a new PCI bus pub fn new(pci_root: PciRoot) -> Self { - let mut devices: HashMap>> = HashMap::new(); - let mut device_ids: Vec = vec![false; NUM_DEVICE_IDS]; - - devices.insert(0, Arc::new(Mutex::new(pci_root))); - device_ids[0] = true; - - PciBus { - devices, - device_ids, - } + let mut bus: Self = Default::default(); + bus.devices[0] = Some(Arc::new(Mutex::new(pci_root))); + bus } - /// Insert a device in the bus + /// Insert a device in the bus at the specified slot (`device_id`). pub fn add_device( &mut self, device_id: u8, device: Arc>, ) -> Result<(), PciRootError> { - if self.devices.contains_key(&device_id) { + let slot = &mut self.devices[device_id as usize]; + if slot.is_some() { return Err(PciRootError::DuplicateDeviceId(device_id)); } - self.devices.insert(device_id, device); - self.device_ids[device_id as usize] = true; + *slot = Some(device); Ok(()) } - /// Remove a device from the bus and free its device ID slot + /// Remove a device from the bus at the specified slot (`device_id`). pub fn remove_device(&mut self, device_id: u8) { - self.devices.remove(&device_id); - self.device_ids[device_id as usize] = false; + self.devices[device_id as usize] = None; } - /// Get a new device ID - // idx is bounded by NUM_DEVICE_IDS (32), so it always fits in u8. + /// Get the next unused device ID. + /// + /// Note: this is non-reserving — repeated calls without an intervening `add_device` will + /// return the same ID. #[allow(clippy::cast_possible_truncation)] - pub fn next_device_id(&mut self) -> Result { - for (idx, device_id) in self.device_ids.iter_mut().enumerate() { - if !(*device_id) { - *device_id = true; - return Ok(idx as u8); - } + pub fn next_device_id(&self) -> Result { + if let Some(position) = self.devices.iter().position(Option::is_none) { + Ok(position as u8) + } else { + Err(PciRootError::NoPciDeviceSlotAvailable) } - - Err(PciRootError::NoPciDeviceSlotAvailable) } } @@ -189,12 +179,8 @@ impl PciConfigIo { // NOTE: Potential contention among vCPU threads on this lock. This should not // be a problem currently, since we mainly access this when we are setting up devices. // We might want to do some profiling to ensure this does not become a bottleneck. - self.pci_bus + self.pci_bus.as_ref().lock().unwrap().devices[usize::from(device)] .as_ref() - .lock() - .unwrap() - .devices - .get(&device) .map_or(0xffff_ffff, |d| { d.lock().unwrap().read_config_register(register) }) @@ -230,7 +216,7 @@ impl PciConfigIo { // be a problem currently, since we mainly access this when we are setting up devices. // We might want to do some profiling to ensure this does not become a bottleneck. let pci_bus = self.pci_bus.as_ref().lock().unwrap(); - if let Some(d) = pci_bus.devices.get(&device) { + if let Some(d) = pci_bus.devices[usize::from(device)].as_ref() { let mut device = d.lock().unwrap(); // offset is validated to be < 4 at the top of this function. @@ -326,11 +312,8 @@ impl PciConfigMmio { return 0xffff_ffff; } - self.pci_bus - .lock() - .unwrap() - .devices - .get(&device) + self.pci_bus.lock().unwrap().devices[usize::from(device)] + .as_ref() .map_or(0xffff_ffff, |d| { d.lock().unwrap().read_config_register(register) }) @@ -354,7 +337,7 @@ impl PciConfigMmio { } let pci_bus = self.pci_bus.lock().unwrap(); - if let Some(d) = pci_bus.devices.get(&device) { + if let Some(d) = pci_bus.devices[usize::from(device)].as_ref() { let mut device = d.lock().unwrap(); // offset is validated to be < 4 at the top of this function. diff --git a/src/vmm/src/rate_limiter/mod.rs b/src/vmm/src/rate_limiter/mod.rs index 58a63194a1c..4656c3da221 100644 --- a/src/vmm/src/rate_limiter/mod.rs +++ b/src/vmm/src/rate_limiter/mod.rs @@ -1,9 +1,9 @@ // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +use std::fmt; use std::os::unix::io::{AsRawFd, RawFd}; use std::time::{Duration, Instant}; -use std::{fmt, io}; use utils::time::TimerFd; @@ -338,10 +338,6 @@ impl RateLimiter { /// /// If either bytes/ops *size* or *refill_time* are **zero**, the limiter /// is **disabled** for that respective token type. - /// - /// # Errors - /// - /// If the timerfd creation fails, an error is returned. pub fn new( bytes_total_capacity: u64, bytes_one_time_burst: u64, @@ -349,7 +345,7 @@ impl RateLimiter { ops_total_capacity: u64, ops_one_time_burst: u64, ops_complete_refill_time_ms: u64, - ) -> io::Result { + ) -> Self { let bytes_token_bucket = TokenBucket::new( bytes_total_capacity, bytes_one_time_burst, @@ -367,12 +363,12 @@ impl RateLimiter { // seccomp-blocked from creating the timer_fd at that time. let timer_fd = TimerFd::new(); - Ok(RateLimiter { + RateLimiter { bandwidth: bytes_token_bucket, ops: ops_token_bucket, timer_fd, timer_active: false, - }) + } } // Arm the timer of the rate limiter with the provided `TimerState`. @@ -515,8 +511,7 @@ impl AsRawFd for RateLimiter { impl Default for RateLimiter { /// Default RateLimiter is a no-op limiter with infinite budget. fn default() -> Self { - // Safe to unwrap since this will not attempt to create timer_fd. - RateLimiter::new(0, 0, 0, 0, 0, 0).expect("Failed to build default RateLimiter") + RateLimiter::new(0, 0, 0, 0, 0, 0) } } @@ -954,7 +949,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_new() { - let l = RateLimiter::new(1000, 1001, 1002, 1003, 1004, 1005).unwrap(); + let l = RateLimiter::new(1000, 1001, 1002, 1003, 1004, 1005); let bw = l.bandwidth.unwrap(); assert_eq!(bw.capacity(), 1000); @@ -972,7 +967,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_manual_replenish() { // rate limiter with limit of 1000 bytes/s and 1000 ops/s - let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000).unwrap(); + let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000); // consume 123 bytes assert!(l.consume(123, TokenType::Bytes)); @@ -993,7 +988,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_bandwidth() { // rate limiter with limit of 1000 bytes/s - let mut l = RateLimiter::new(1000, 0, 1000, 0, 0, 0).unwrap(); + let mut l = RateLimiter::new(1000, 0, 1000, 0, 0, 0); // limiter should not be blocked assert!(!l.is_blocked()); @@ -1026,7 +1021,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_ops() { // rate limiter with limit of 1000 ops/s - let mut l = RateLimiter::new(0, 0, 0, 1000, 0, 1000).unwrap(); + let mut l = RateLimiter::new(0, 0, 0, 1000, 0, 1000); // limiter should not be blocked assert!(!l.is_blocked()); @@ -1059,7 +1054,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_full() { // rate limiter with limit of 1000 bytes/s and 1000 ops/s - let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000).unwrap(); + let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000); // limiter should not be blocked assert!(!l.is_blocked()); @@ -1095,7 +1090,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_overconsumption() { // initialize the rate limiter - let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000).unwrap(); + let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000); // try to consume 2.5x the bucket size // we are "borrowing" 1.5x the bucket size in tokens since // the bucket is full @@ -1114,7 +1109,7 @@ pub(crate) mod tests { assert!(!l.is_blocked()); // reset the rate limiter - let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000).unwrap(); + let mut l = RateLimiter::new(1000, 0, 1000, 1000, 0, 1000); // try to consume 1.5x the bucket size // we are "borrowing" 1.5x the bucket size in tokens since // the bucket is full, should arm the timer to 0.5x replenish @@ -1151,7 +1146,7 @@ pub(crate) mod tests { #[test] fn test_update_buckets() { - let mut x = RateLimiter::new(1000, 2000, 1000, 10, 20, 1000).unwrap(); + let mut x = RateLimiter::new(1000, 2000, 1000, 10, 20, 1000); let initial_bw = x.bandwidth.clone(); let initial_ops = x.ops.clone(); @@ -1183,7 +1178,7 @@ pub(crate) mod tests { #[test] fn test_rate_limiter_debug() { - let l = RateLimiter::new(1, 2, 3, 4, 5, 6).unwrap(); + let l = RateLimiter::new(1, 2, 3, 4, 5, 6); assert_eq!( format!("{:?}", l), format!( diff --git a/src/vmm/src/rate_limiter/persist.rs b/src/vmm/src/rate_limiter/persist.rs index 042d577ec53..e4d6c8ae713 100644 --- a/src/vmm/src/rate_limiter/persist.rs +++ b/src/vmm/src/rate_limiter/persist.rs @@ -3,6 +3,8 @@ //! Defines the structures needed for saving/restoring a RateLimiter. +use std::io; + use serde::{Deserialize, Serialize}; use utils::time::TimerFd; @@ -126,7 +128,7 @@ mod tests { #[test] fn test_rate_limiter_persistence() { let refill_time = 100_000; - let mut rate_limiter = RateLimiter::new(100, 0, refill_time, 10, 0, refill_time).unwrap(); + let mut rate_limiter = RateLimiter::new(100, 0, refill_time, 10, 0, refill_time); // Check that RateLimiter restores correctly if untouched. let restored_rate_limiter = diff --git a/src/vmm/src/vmm_config/entropy.rs b/src/vmm/src/vmm_config/entropy.rs index 439657c90f6..ee67d65f75e 100644 --- a/src/vmm/src/vmm_config/entropy.rs +++ b/src/vmm/src/vmm_config/entropy.rs @@ -8,6 +8,7 @@ use serde::{Deserialize, Serialize}; use super::RateLimiterConfig; use crate::devices::virtio::rng::{Entropy, EntropyError}; +use crate::rate_limiter::RateLimiter; /// This struct represents the strongly typed equivalent of the json body from entropy device /// related requests. @@ -33,8 +34,6 @@ impl From<&Entropy> for EntropyDeviceConfig { pub enum EntropyDeviceError { /// Could not create Entropy device: {0} CreateDevice(#[from] EntropyError), - /// Could not create RateLimiter from configuration: {0} - CreateRateLimiter(#[from] std::io::Error), } /// A builder type used to construct an Entropy device @@ -54,9 +53,9 @@ impl EntropyDeviceBuilder { ) -> Result>, EntropyDeviceError> { let rate_limiter = config .rate_limiter - .map(RateLimiterConfig::try_into) - .transpose()?; - let dev = Arc::new(Mutex::new(Entropy::new(rate_limiter.unwrap_or_default())?)); + .map(RateLimiter::from) + .unwrap_or_default(); + let dev = Arc::new(Mutex::new(Entropy::new(rate_limiter)?)); self.0 = Some(dev.clone()); Ok(dev) diff --git a/src/vmm/src/vmm_config/mod.rs b/src/vmm/src/vmm_config/mod.rs index b67a486c2f9..6a861375b17 100644 --- a/src/vmm/src/vmm_config/mod.rs +++ b/src/vmm/src/vmm_config/mod.rs @@ -1,9 +1,6 @@ // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::convert::{From, TryInto}; -use std::io; - use serde::{Deserialize, Serialize}; use crate::devices::virtio::device::VirtioDeviceType; @@ -148,12 +145,10 @@ impl From> for RateLimiterUpdate { } } -impl TryInto for RateLimiterConfig { - type Error = io::Error; - - fn try_into(self) -> Result { - let bw = self.bandwidth.unwrap_or_default(); - let ops = self.ops.unwrap_or_default(); +impl From for RateLimiter { + fn from(cfg: RateLimiterConfig) -> Self { + let bw = cfg.bandwidth.unwrap_or_default(); + let ops = cfg.ops.unwrap_or_default(); RateLimiter::new( bw.size, bw.one_time_burst.unwrap_or(0), @@ -208,7 +203,7 @@ mod tests { refill_time: REFILL_TIME * 2, }), }; - let rl: RateLimiter = rlconf.try_into().unwrap(); + let rl: RateLimiter = rlconf.into(); assert_eq!(rl.bandwidth().unwrap().capacity(), SIZE); assert_eq!(rl.bandwidth().unwrap().one_time_burst(), ONE_TIME_BURST); assert_eq!(rl.bandwidth().unwrap().refill_time_ms(), REFILL_TIME); @@ -232,7 +227,7 @@ mod tests { bandwidth: Some(bw_tb_cfg), ops: None, }; - let rl: RateLimiter = rl_conf.try_into().unwrap(); + let rl: RateLimiter = rl_conf.into(); let generated_rl_conf = RateLimiterConfig::from(&rl); assert_eq!(generated_rl_conf, rl_conf); assert_eq!(generated_rl_conf.into_option(), Some(rl_conf)); diff --git a/src/vmm/src/vmm_config/net.rs b/src/vmm/src/vmm_config/net.rs index 653836dade0..f1e3f8ea736 100644 --- a/src/vmm/src/vmm_config/net.rs +++ b/src/vmm/src/vmm_config/net.rs @@ -1,7 +1,6 @@ // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::convert::TryInto; use std::ops::Deref; use std::sync::{Arc, Mutex}; @@ -11,6 +10,8 @@ use super::RateLimiterConfig; use crate::VmmError; use crate::devices::virtio::device::VirtioDevice; use crate::devices::virtio::net::{Net, TapError}; +use crate::logger::warn; +use crate::rate_limiter::RateLimiter; use crate::utils::net::mac::MacAddr; /// This struct represents the strongly typed equivalent of the json body from net iface @@ -67,8 +68,6 @@ pub struct NetworkInterfaceUpdateConfig { pub enum NetworkInterfaceError { /// Could not create the network device: {0} CreateNetworkDevice(#[from] crate::devices::virtio::net::NetError), - /// Cannot create the rate limiter: {0} - CreateRateLimiter(#[from] std::io::Error), /// Unable to update the net device: {0} DeviceUpdate(#[from] VmmError), /// The MAC address is already in use: {0} @@ -130,6 +129,11 @@ impl NetBuilder { .iter() .position(|net| net.lock().expect("Poisoned lock").id() == netif_config.iface_id) { + warn!( + "Replacing existing network device '{}'. The previous device configuration will \ + be destroyed.", + netif_config.iface_id + ); self.net_devices.swap_remove(index); } @@ -144,22 +148,20 @@ impl NetBuilder { pub fn create_net(cfg: NetworkInterfaceConfig) -> Result { let rx_rate_limiter = cfg .rx_rate_limiter - .map(super::RateLimiterConfig::try_into) - .transpose() - .map_err(NetworkInterfaceError::CreateRateLimiter)?; + .map(RateLimiter::from) + .unwrap_or_default(); let tx_rate_limiter = cfg .tx_rate_limiter - .map(super::RateLimiterConfig::try_into) - .transpose() - .map_err(NetworkInterfaceError::CreateRateLimiter)?; + .map(RateLimiter::from) + .unwrap_or_default(); // Create and return the Net device crate::devices::virtio::net::Net::new( cfg.iface_id, &cfg.host_dev_name, cfg.guest_mac, - rx_rate_limiter.unwrap_or_default(), - tx_rate_limiter.unwrap_or_default(), + rx_rate_limiter, + tx_rate_limiter, cfg.mtu, ) .map_err(NetworkInterfaceError::CreateNetworkDevice) diff --git a/src/vmm/src/vstate/bus.rs b/src/vmm/src/vstate/bus.rs index 44158928365..5a008605cba 100644 --- a/src/vmm/src/vstate/bus.rs +++ b/src/vmm/src/vstate/bus.rs @@ -10,7 +10,6 @@ use std::cmp::Ordering; use std::collections::btree_map::BTreeMap; use std::sync::{Arc, Barrier, Mutex, RwLock, Weak}; -use std::{error, fmt, result}; /// Trait for devices that respond to reads or writes in an arbitrary address space. /// @@ -53,29 +52,12 @@ impl BusDeviceSync for Mutex { } /// Error type for [`Bus`]-related operations. -#[derive(Debug)] +#[derive(Debug, thiserror::Error, displaydoc::Display)] pub enum BusError { - /// The insertion failed because the new device overlapped with an old device. - Overlap, - /// Failed to operate on zero sized range. - ZeroSizedRange, /// Failed to find address range. MissingAddressRange, - /// The supplied range is invalid. - InvalidRange, } -/// Result type for [`Bus`]-related operations. -pub type Result = result::Result; - -impl fmt::Display for BusError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "bus_error: {self:?}") - } -} - -impl error::Error for BusError {} - /// Holds a base and end representing the address space occupied by a `BusDevice`. /// /// * base - The address at which the range start. @@ -90,12 +72,10 @@ pub struct BusRange { #[allow(missing_docs)] impl BusRange { - pub fn new(base: u64, len: u64) -> Result { - if len == 0 { - return Err(BusError::ZeroSizedRange); - } - let end = base.checked_add(len - 1).ok_or(BusError::InvalidRange)?; - Ok(BusRange { base, end }) + pub fn new(base: u64, len: u64) -> Self { + assert!(0 < len, "BusRange: zero-sized range"); + let end = base.checked_add(len - 1).expect("BusRange: range overflow"); + BusRange { base, end } } pub fn base(&self) -> u64 { @@ -151,7 +131,7 @@ impl Bus { fn first_before(&self, addr: u64) -> Option<(BusRange, Arc)> { let devices = self.devices.read().unwrap(); - let (range, dev) = devices.range(..=BusRange::new(addr, 1).ok()?).next_back()?; + let (range, dev) = devices.range(..=BusRange::new(addr, 1)).next_back()?; dev.upgrade().map(|d| (*range, d.clone())) } @@ -168,48 +148,45 @@ impl Bus { } /// Insert a device into the [`Bus`] in the range [`addr`, `addr` + `len`]. - pub fn insert(&self, device: Arc, base: u64, len: u64) -> Result<()> { - let new_range = BusRange::new(base, len)?; - - // Reject all cases where the new device's range overlaps with an existing device. - if self - .devices - .read() - .unwrap() - .iter() - .any(|(range, _dev)| range.overlaps(&new_range)) - { - return Err(BusError::Overlap); - } + /// + /// # Panics + /// + /// Panics if `len` is zero, if the range overflows, or if the new range overlaps with an + /// existing device. These conditions indicate a bug in the VMM address layout. + pub fn insert(&self, device: Arc, base: u64, len: u64) { + let new_range = BusRange::new(base, len); - if self - .devices - .write() - .unwrap() - .insert(new_range, Arc::downgrade(&device)) - .is_some() - { - return Err(BusError::Overlap); - } + let mut devices = self.devices.write().unwrap(); + + assert!( + !devices + .iter() + .any(|(range, _dev)| range.overlaps(&new_range)), + "Bus::insert: overlapping range at base {base:#x} len {len:#x}" + ); - Ok(()) + devices.insert(new_range, Arc::downgrade(&device)); } /// Removes the device at the given address space range. - pub fn remove(&self, base: u64, len: u64) -> Result<()> { - let bus_range = BusRange::new(base, len)?; - - if self.devices.write().unwrap().remove(&bus_range).is_none() { - return Err(BusError::MissingAddressRange); - } - - Ok(()) + /// + /// # Panics + /// + /// Panics if `len` is zero, if the range overflows, or if no device is registered at the + /// given range. The caller is expected to remove only ranges that were previously inserted. + pub fn remove(&self, base: u64, len: u64) { + let bus_range = BusRange::new(base, len); + let result = self.devices.write().unwrap().remove(&bus_range); + assert!( + result.is_some(), + "Bus::remove: no device at base {base:#x} len {len:#x}" + ); } /// Reads data from the device that owns the range containing `addr` and puts it into `data`. /// /// Returns true on success, otherwise `data` is untouched. - pub fn read(&self, addr: u64, data: &mut [u8]) -> Result<()> { + pub fn read(&self, addr: u64, data: &mut [u8]) -> Result<(), BusError> { if let Some((base, offset, dev)) = self.resolve(addr) { // OK to unwrap as lock() failing is a serious error condition and should panic. dev.read(base, offset, data); @@ -222,7 +199,7 @@ impl Bus { /// Writes `data` to the device that owns the range containing `addr`. /// /// Returns true on success, otherwise `data` is untouched. - pub fn write(&self, addr: u64, data: &[u8]) -> Result>> { + pub fn write(&self, addr: u64, data: &[u8]) -> Result>, BusError> { if let Some((base, offset, dev)) = self.resolve(addr) { // OK to unwrap as lock() failing is a serious error condition and should panic. Ok(dev.write(base, offset, data)) @@ -260,79 +237,81 @@ mod tests { #[test] fn bus_range_new() { - // Zero length is invalid. - assert!(matches!(BusRange::new(0, 0), Err(BusError::ZeroSizedRange))); - assert!(matches!( - BusRange::new(u64::MAX, 0), - Err(BusError::ZeroSizedRange) - )); - - // Overflow is invalid. - assert!(matches!( - BusRange::new(u64::MAX, 2), - Err(BusError::InvalidRange) - )); - assert!(matches!( - BusRange::new(2, u64::MAX), - Err(BusError::InvalidRange) - )); - // Ranges that exactly reach u64::MAX are valid. - let r = BusRange::new(u64::MAX, 1).unwrap(); + let r = BusRange::new(u64::MAX, 1); assert_eq!(r.base(), u64::MAX); assert_eq!(r.end(), u64::MAX); - let r = BusRange::new(1, u64::MAX).unwrap(); + let r = BusRange::new(1, u64::MAX); assert_eq!(r.base(), 1); assert_eq!(r.end(), u64::MAX); - let r = BusRange::new(u64::MAX - 4095, 4096).unwrap(); + let r = BusRange::new(u64::MAX - 4095, 4096); assert_eq!(r.base(), u64::MAX - 4095); assert_eq!(r.end(), u64::MAX); // One sized valid range. - let r = BusRange::new(0, 1).unwrap(); + let r = BusRange::new(0, 1); assert_eq!(r.base(), 0); assert_eq!(r.end(), 0); // Normal valid range. - let r = BusRange::new(0x1000, 0x400).unwrap(); + let r = BusRange::new(0x1000, 0x400); assert_eq!(r.base(), 0x1000); assert_eq!(r.end(), 0x13ff); } + #[test] + #[should_panic(expected = "zero-sized range")] + fn bus_range_new_zero_len() { + BusRange::new(0, 0); + } + + #[test] + #[should_panic(expected = "range overflow")] + fn bus_range_new_overflow() { + BusRange::new(u64::MAX, 2); + } + #[test] fn bus_insert() { let bus = Bus::new(); let dummy = Arc::new(DummyDevice); - bus.insert(dummy.clone(), 0x10, 0).unwrap_err(); - bus.insert(dummy.clone(), 0x10, 0x10).unwrap(); - - let result = bus.insert(dummy.clone(), 0x0f, 0x10); - assert_eq!(format!("{result:?}"), "Err(Overlap)"); - - bus.insert(dummy.clone(), 0x10, 0x10).unwrap_err(); - bus.insert(dummy.clone(), 0x10, 0x15).unwrap_err(); - bus.insert(dummy.clone(), 0x12, 0x15).unwrap_err(); - bus.insert(dummy.clone(), 0x12, 0x01).unwrap_err(); - bus.insert(dummy.clone(), 0x0, 0x20).unwrap_err(); - bus.insert(dummy.clone(), 0x20, 0x05).unwrap(); - bus.insert(dummy.clone(), 0x25, 0x05).unwrap(); - bus.insert(dummy, 0x0, 0x10).unwrap(); + bus.insert(dummy.clone(), 0x10, 0x10); + bus.insert(dummy.clone(), 0x20, 0x05); + bus.insert(dummy.clone(), 0x25, 0x05); + bus.insert(dummy, 0x0, 0x10); } #[test] - fn bus_remove() { + #[should_panic(expected = "zero-sized range")] + fn bus_insert_zero_len() { let bus = Bus::new(); - let dummy: Arc = Arc::new(DummyDevice); + bus.insert(Arc::new(DummyDevice), 0x10, 0); + } - bus.remove(0x42, 0x0).unwrap_err(); + #[test] + #[should_panic(expected = "overlapping range")] + fn bus_insert_overlap() { + let bus = Bus::new(); + let dummy = Arc::new(DummyDevice); + bus.insert(dummy.clone(), 0x10, 0x10); + bus.insert(dummy, 0x0f, 0x10); + } - bus.remove(0x13, 0x12).unwrap_err(); + #[test] + #[should_panic(expected = "no device at base")] + fn bus_remove_non_existing() { + let bus = Bus::new(); + bus.remove(0x13, 0x12) + } - bus.insert(dummy.clone(), 0x13, 0x12).unwrap(); - bus.remove(0x42, 0x42).unwrap_err(); - bus.remove(0x13, 0x12).unwrap(); + #[test] + fn bus_remove() { + let bus = Bus::new(); + let dummy: Arc = Arc::new(DummyDevice); + bus.insert(dummy.clone(), 0x13, 0x12); + bus.remove(0x13, 0x12); } #[test] @@ -340,7 +319,7 @@ mod tests { fn bus_read_write() { let bus = Bus::new(); let dummy = Arc::new(DummyDevice); - bus.insert(dummy.clone(), 0x10, 0x10).unwrap(); + bus.insert(dummy.clone(), 0x10, 0x10); bus.read(0x10, &mut [0, 0, 0, 0]).unwrap(); bus.write(0x10, &[0, 0, 0, 0]).unwrap(); bus.read(0x11, &mut [0, 0, 0, 0]).unwrap(); @@ -358,7 +337,7 @@ mod tests { fn bus_read_write_values() { let bus = Bus::new(); let dummy = Arc::new(ConstantDevice); - bus.insert(dummy.clone(), 0x10, 0x10).unwrap(); + bus.insert(dummy.clone(), 0x10, 0x10); let mut values = [0, 1, 2, 3]; bus.read(0x10, &mut values).unwrap(); @@ -372,19 +351,19 @@ mod tests { #[test] #[allow(clippy::redundant_clone)] fn busrange_cmp() { - let range = BusRange::new(0x10, 2).unwrap(); - assert_eq!(range, BusRange::new(0x10, 3).unwrap()); - assert_eq!(range, BusRange::new(0x10, 2).unwrap()); + let range = BusRange::new(0x10, 2); + assert_eq!(range, BusRange::new(0x10, 3)); + assert_eq!(range, BusRange::new(0x10, 2)); - assert!(range < BusRange::new(0x12, 1).unwrap()); - assert!(range < BusRange::new(0x12, 3).unwrap()); + assert!(range < BusRange::new(0x12, 1)); + assert!(range < BusRange::new(0x12, 3)); assert_eq!(range, range.clone()); let bus = Bus::new(); let mut data = [1, 2, 3, 4]; let device = Arc::new(DummyDevice); - bus.insert(device.clone(), 0x10, 0x10).unwrap(); + bus.insert(device.clone(), 0x10, 0x10); bus.write(0x10, &data).unwrap(); bus.read(0x10, &mut data).unwrap(); assert_eq!(data, [1, 2, 3, 4]); @@ -392,14 +371,14 @@ mod tests { #[test] fn bus_range_overlap() { - let a = BusRange::new(0x1000, 0x400).unwrap(); - assert!(a.overlaps(&BusRange::new(0x1000, 0x400).unwrap())); - assert!(a.overlaps(&BusRange::new(0xf00, 0x400).unwrap())); - assert!(a.overlaps(&BusRange::new(0x1000, 0x01).unwrap())); - assert!(a.overlaps(&BusRange::new(0xfff, 0x02).unwrap())); - assert!(a.overlaps(&BusRange::new(0x1100, 0x100).unwrap())); - assert!(a.overlaps(&BusRange::new(0x13ff, 0x100).unwrap())); - assert!(!a.overlaps(&BusRange::new(0x1400, 0x100).unwrap())); - assert!(!a.overlaps(&BusRange::new(0xf00, 0x100).unwrap())); + let a = BusRange::new(0x1000, 0x400); + assert!(a.overlaps(&BusRange::new(0x1000, 0x400))); + assert!(a.overlaps(&BusRange::new(0xf00, 0x400))); + assert!(a.overlaps(&BusRange::new(0x1000, 0x01))); + assert!(a.overlaps(&BusRange::new(0xfff, 0x02))); + assert!(a.overlaps(&BusRange::new(0x1100, 0x100))); + assert!(a.overlaps(&BusRange::new(0x13ff, 0x100))); + assert!(!a.overlaps(&BusRange::new(0x1400, 0x100))); + assert!(!a.overlaps(&BusRange::new(0xf00, 0x100))); } } diff --git a/src/vmm/src/vstate/vcpu.rs b/src/vmm/src/vstate/vcpu.rs index ffc473a70ac..8f41cfb89c6 100644 --- a/src/vmm/src/vstate/vcpu.rs +++ b/src/vmm/src/vstate/vcpu.rs @@ -802,7 +802,7 @@ pub(crate) mod tests { let bus = Arc::new(Bus::new()); let dummy = Arc::new(Mutex::new(DummyDevice)); - bus.insert(dummy, 0x10, 0x10).unwrap(); + bus.insert(dummy, 0x10, 0x10); vcpu.set_mmio_bus(bus); let addr = 0x10; diff --git a/src/vmm/src/vstate/vm.rs b/src/vmm/src/vstate/vm.rs index ce37231e038..92484dffe7c 100644 --- a/src/vmm/src/vstate/vm.rs +++ b/src/vmm/src/vstate/vm.rs @@ -714,13 +714,10 @@ impl KvmVm { /// Set GSI routes to KVM pub fn set_gsi_routes(&self) -> Result<(), InterruptError> { let entries = self.common.interrupts.lock().expect("Poisoned lock"); - let mut routes = KvmIrqRouting::new(0)?; - - for entry in entries.values() { - if entry.masked { - continue; - } - routes.push(entry.entry)?; + let unmasked = entries.values().filter(|e| !e.masked).count(); + let mut routes = KvmIrqRouting::new(unmasked)?; + for (slot, entry) in entries.values().filter(|e| !e.masked).enumerate() { + routes.as_mut_slice()[slot] = entry.entry; } self.common.fd.set_gsi_routing(&routes)?;