From a1a6a3c5c68adc09641c490a6771ed8703ce2faf Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 20 Mar 2025 14:00:41 +0000 Subject: [PATCH 1/9] fix: make cargo test pass with gdb feature In CI we always run unittests without `--features gdb`, but actually with the gdb feature they dont compile. Fix this with some cfgs. Signed-off-by: Patrick Roy --- .../src/api_server/request/machine_configuration.rs | 10 ++++++++++ src/vmm/src/resources.rs | 2 ++ 2 files changed, 12 insertions(+) diff --git a/src/firecracker/src/api_server/request/machine_configuration.rs b/src/firecracker/src/api_server/request/machine_configuration.rs index 4457a9b6b75..2e8addffb74 100644 --- a/src/firecracker/src/api_server/request/machine_configuration.rs +++ b/src/firecracker/src/api_server/request/machine_configuration.rs @@ -123,6 +123,8 @@ mod tests { cpu_template: None, track_dirty_pages: Some(false), huge_pages: Some(expected), + #[cfg(feature = "gdb")] + gdb_socket_path: None, }; assert_eq!( vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()), @@ -142,6 +144,8 @@ mod tests { cpu_template: Some(StaticCpuTemplate::None), track_dirty_pages: Some(false), huge_pages: Some(HugePageConfig::None), + #[cfg(feature = "gdb")] + gdb_socket_path: None, }; assert_eq!( vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()), @@ -161,6 +165,8 @@ mod tests { cpu_template: None, track_dirty_pages: Some(true), huge_pages: Some(HugePageConfig::None), + #[cfg(feature = "gdb")] + gdb_socket_path: None, }; assert_eq!( vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()), @@ -184,6 +190,8 @@ mod tests { cpu_template: Some(StaticCpuTemplate::T2), track_dirty_pages: Some(true), huge_pages: Some(HugePageConfig::None), + #[cfg(feature = "gdb")] + gdb_socket_path: None, }; assert_eq!( vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()), @@ -209,6 +217,8 @@ mod tests { cpu_template: None, track_dirty_pages: Some(true), huge_pages: Some(HugePageConfig::None), + #[cfg(feature = "gdb")] + gdb_socket_path: None, }; assert_eq!( vmm_action_from_request(parse_put_machine_config(&Body::new(body)).unwrap()), diff --git a/src/vmm/src/resources.rs b/src/vmm/src/resources.rs index 113c24cad51..e65cc7a0121 100644 --- a/src/vmm/src/resources.rs +++ b/src/vmm/src/resources.rs @@ -1328,6 +1328,8 @@ mod tests { cpu_template: Some(StaticCpuTemplate::V1N1), track_dirty_pages: Some(false), huge_pages: Some(HugePageConfig::None), + #[cfg(feature = "gdb")] + gdb_socket_path: None, }; assert_ne!( From c2353b1d6c2bbe1e12523f46b5cb66b8aa7469af Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 21 Mar 2025 15:24:11 +0000 Subject: [PATCH 2/9] refactor: de-generify fdt.rs There is absolutely no need for generics anywhere in this module, as the trait we were being generic over had only a single implementor. Signed-off-by: Patrick Roy --- src/vmm/src/arch/aarch64/fdt.rs | 95 ++++++++++++------------------ src/vmm/src/arch/aarch64/mod.rs | 6 +- src/vmm/src/device_manager/mmio.rs | 15 ----- 3 files changed, 42 insertions(+), 74 deletions(-) diff --git a/src/vmm/src/arch/aarch64/fdt.rs b/src/vmm/src/arch/aarch64/fdt.rs index 2eb154a33b3..c708add44bf 100644 --- a/src/vmm/src/arch/aarch64/fdt.rs +++ b/src/vmm/src/arch/aarch64/fdt.rs @@ -15,6 +15,7 @@ use vm_memory::GuestMemoryError; use super::super::{DeviceType, InitrdConfig}; use super::cache_info::{CacheEntry, read_cache_config}; use super::gic::GICDevice; +use crate::device_manager::mmio::MMIODeviceInfo; use crate::devices::acpi::vmgenid::{VMGENID_MEM_SIZE, VmGenId}; use crate::vstate::memory::{Address, GuestMemory, GuestMemoryMmap}; @@ -42,16 +43,6 @@ const GIC_FDT_IRQ_TYPE_PPI: u32 = 1; const IRQ_TYPE_EDGE_RISING: u32 = 1; const IRQ_TYPE_LEVEL_HI: u32 = 4; -/// Trait for devices to be added to the Flattened Device Tree. -pub trait DeviceInfoForFDT { - /// Returns the address where this device will be loaded. - fn addr(&self) -> u64; - /// Returns the associated interrupt for this device. - fn irq(&self) -> u32; - /// Returns the amount of memory that needs to be reserved for this device. - fn length(&self) -> u64; -} - /// Errors thrown while configuring the Flattened Device Tree for aarch64. #[derive(Debug, thiserror::Error, displaydoc::Display)] pub enum FdtError { @@ -64,11 +55,11 @@ pub enum FdtError { } /// Creates the flattened device tree for this aarch64 microVM. -pub fn create_fdt( +pub fn create_fdt( guest_mem: &GuestMemoryMmap, vcpu_mpidr: Vec, cmdline: CString, - device_info: &HashMap<(DeviceType, String), T>, + device_info: &HashMap<(DeviceType, String), MMIODeviceInfo>, gic_device: &GICDevice, vmgenid: &Option, initrd: &Option, @@ -361,17 +352,18 @@ fn create_psci_node(fdt: &mut FdtWriter) -> Result<(), FdtError> { Ok(()) } -fn create_virtio_node( - fdt: &mut FdtWriter, - dev_info: &T, -) -> Result<(), FdtError> { - let virtio_mmio = fdt.begin_node(&format!("virtio_mmio@{:x}", dev_info.addr()))?; +fn create_virtio_node(fdt: &mut FdtWriter, dev_info: &MMIODeviceInfo) -> Result<(), FdtError> { + let virtio_mmio = fdt.begin_node(&format!("virtio_mmio@{:x}", dev_info.addr))?; fdt.property_string("compatible", "virtio,mmio")?; - fdt.property_array_u64("reg", &[dev_info.addr(), dev_info.length()])?; + fdt.property_array_u64("reg", &[dev_info.addr, dev_info.len])?; fdt.property_array_u32( "interrupts", - &[GIC_FDT_IRQ_TYPE_SPI, dev_info.irq(), IRQ_TYPE_EDGE_RISING], + &[ + GIC_FDT_IRQ_TYPE_SPI, + dev_info.irq.unwrap().into(), + IRQ_TYPE_EDGE_RISING, + ], )?; fdt.property_u32("interrupt-parent", GIC_PHANDLE)?; fdt.end_node(virtio_mmio)?; @@ -379,38 +371,36 @@ fn create_virtio_node( Ok(()) } -fn create_serial_node( - fdt: &mut FdtWriter, - dev_info: &T, -) -> Result<(), FdtError> { - let serial = fdt.begin_node(&format!("uart@{:x}", dev_info.addr()))?; +fn create_serial_node(fdt: &mut FdtWriter, dev_info: &MMIODeviceInfo) -> Result<(), FdtError> { + let serial = fdt.begin_node(&format!("uart@{:x}", dev_info.addr))?; fdt.property_string("compatible", "ns16550a")?; - fdt.property_array_u64("reg", &[dev_info.addr(), dev_info.length()])?; + fdt.property_array_u64("reg", &[dev_info.addr, dev_info.len])?; fdt.property_u32("clocks", CLOCK_PHANDLE)?; fdt.property_string("clock-names", "apb_pclk")?; fdt.property_array_u32( "interrupts", - &[GIC_FDT_IRQ_TYPE_SPI, dev_info.irq(), IRQ_TYPE_EDGE_RISING], + &[ + GIC_FDT_IRQ_TYPE_SPI, + dev_info.irq.unwrap().into(), + IRQ_TYPE_EDGE_RISING, + ], )?; fdt.end_node(serial)?; Ok(()) } -fn create_rtc_node( - fdt: &mut FdtWriter, - dev_info: &T, -) -> Result<(), FdtError> { +fn create_rtc_node(fdt: &mut FdtWriter, dev_info: &MMIODeviceInfo) -> Result<(), FdtError> { // Driver requirements: // https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/rtc/arm,pl031.yaml // We do not offer the `interrupt` property because the device // does not implement interrupt support. let compatible = b"arm,pl031\0arm,primecell\0"; - let rtc = fdt.begin_node(&format!("rtc@{:x}", dev_info.addr()))?; + let rtc = fdt.begin_node(&format!("rtc@{:x}", dev_info.addr))?; fdt.property("compatible", compatible)?; - fdt.property_array_u64("reg", &[dev_info.addr(), dev_info.length()])?; + fdt.property_array_u64("reg", &[dev_info.addr, dev_info.len])?; fdt.property_u32("clocks", CLOCK_PHANDLE)?; fdt.property_string("clock-names", "apb_pclk")?; fdt.end_node(rtc)?; @@ -418,12 +408,12 @@ fn create_rtc_node( Ok(()) } -fn create_devices_node( +fn create_devices_node( fdt: &mut FdtWriter, - dev_info: &HashMap<(DeviceType, String), T, S>, + dev_info: &HashMap<(DeviceType, String), MMIODeviceInfo>, ) -> Result<(), FdtError> { // Create one temp Vec to store all virtio devices - let mut ordered_virtio_device: Vec<&T> = Vec::new(); + let mut ordered_virtio_device: Vec<&MMIODeviceInfo> = Vec::new(); for ((device_type, _device_id), info) in dev_info { match device_type { @@ -437,7 +427,7 @@ fn create_devices_node u64 { - self.addr - } - fn irq(&self) -> u32 { - self.irq - } - fn length(&self) -> u64 { - LEN - } - } // The `load` function from the `device_tree` will mistakenly check the actual size // of the buffer with the allocated size. This works around that. fn set_size(buf: &mut [u8], pos: usize, val: u32) { @@ -493,17 +467,26 @@ mod tests { let dev_info: HashMap<(DeviceType, std::string::String), MMIODeviceInfo> = [ ( (DeviceType::Serial, DeviceType::Serial.to_string()), - MMIODeviceInfo { addr: 0x00, irq: 1 }, + MMIODeviceInfo { + addr: 0x00, + irq: NonZeroU32::new(1), + len: LEN, + }, ), ( (DeviceType::Virtio(1), "virtio".to_string()), - MMIODeviceInfo { addr: LEN, irq: 2 }, + MMIODeviceInfo { + addr: LEN, + irq: NonZeroU32::new(2), + len: LEN, + }, ), ( (DeviceType::Rtc, "rtc".to_string()), MMIODeviceInfo { addr: 2 * LEN, - irq: 3, + irq: NonZeroU32::new(3), + len: LEN, }, ), ] diff --git a/src/vmm/src/arch/aarch64/mod.rs b/src/vmm/src/arch/aarch64/mod.rs index 7ab1685cf69..a09721e1775 100644 --- a/src/vmm/src/arch/aarch64/mod.rs +++ b/src/vmm/src/arch/aarch64/mod.rs @@ -23,9 +23,9 @@ use std::fmt::Debug; use vm_memory::GuestMemoryError; -pub use self::fdt::DeviceInfoForFDT; use self::gic::GICDevice; use crate::arch::DeviceType; +use crate::device_manager::mmio::MMIODeviceInfo; use crate::devices::acpi::vmgenid::VmGenId; use crate::vstate::memory::{Address, Bytes, GuestAddress, GuestMemory, GuestMemoryMmap}; @@ -63,11 +63,11 @@ pub fn arch_memory_regions(size: usize) -> Vec<(GuestAddress, usize)> { /// * `device_info` - A hashmap containing the attached devices for building FDT device nodes. /// * `gic_device` - The GIC device. /// * `initrd` - Information about an optional initrd. -pub fn configure_system( +pub fn configure_system( guest_mem: &GuestMemoryMmap, cmdline_cstring: CString, vcpu_mpidr: Vec, - device_info: &HashMap<(DeviceType, String), T>, + device_info: &HashMap<(DeviceType, String), MMIODeviceInfo>, gic_device: &GICDevice, vmgenid: &Option, initrd: &Option, diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index b4961623a15..20d9b381e1c 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -23,8 +23,6 @@ use vm_allocator::AllocPolicy; use super::resources::ResourceAllocator; use crate::arch::DeviceType; use crate::arch::DeviceType::Virtio; -#[cfg(target_arch = "aarch64")] -use crate::arch::aarch64::DeviceInfoForFDT; use crate::devices::BusDevice; #[cfg(target_arch = "aarch64")] use crate::devices::legacy::RTCDevice; @@ -522,19 +520,6 @@ impl MMIODeviceManager { } } -#[cfg(target_arch = "aarch64")] -impl DeviceInfoForFDT for MMIODeviceInfo { - fn addr(&self) -> u64 { - self.addr - } - fn irq(&self) -> u32 { - self.irq.unwrap().into() - } - fn length(&self) -> u64 { - self.len - } -} - #[cfg(test)] mod tests { From 2aae2bfa3cb2e0946ea9b11927c458d27992830c Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 24 Mar 2025 16:49:17 +0000 Subject: [PATCH 3/9] refactor: do not redefine x86 constant in test_vsock_bof This test is all about testing what happens when a vsock driver places virtio buffers close to / around the MMIO gap on x86_64 systems. Since this test thus doesn't really serve a purpose on aarch64, we can just cfg(target_arch = "x86_64") it, and reuse the constants from arch::x86_64 instead of redefining them again. fwiw, I have no idea what "bof" means. I'm also confused by this test passing on ARM in the first place, given that some of the comments indicate to me that it should fail if the buffers overlap the mmio gap (which doesn't exist on arm, so nothing should fail?). Signed-off-by: Patrick Roy --- src/vmm/src/arch/x86_64/mod.rs | 3 +- .../src/devices/virtio/vsock/event_handler.rs | 40 +++++++++++-------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/vmm/src/arch/x86_64/mod.rs b/src/vmm/src/arch/x86_64/mod.rs index 24a7de7ec8a..9c3c2b11ce3 100644 --- a/src/vmm/src/arch/x86_64/mod.rs +++ b/src/vmm/src/arch/x86_64/mod.rs @@ -73,7 +73,8 @@ pub enum ConfigurationError { StartInfoSetup, } -const FIRST_ADDR_PAST_32BITS: u64 = 1 << 32; +/// First address that cannot be addressed using 32 bit anymore. +pub const FIRST_ADDR_PAST_32BITS: u64 = 1 << 32; /// Size of MMIO gap at top of 32-bit address space. pub const MEM_32BIT_GAP_SIZE: u64 = 768 << 20; diff --git a/src/vmm/src/devices/virtio/vsock/event_handler.rs b/src/vmm/src/devices/virtio/vsock/event_handler.rs index a16cff7dbfc..2dab8490119 100755 --- a/src/vmm/src/devices/virtio/vsock/event_handler.rs +++ b/src/vmm/src/devices/virtio/vsock/event_handler.rs @@ -223,10 +223,7 @@ mod tests { use super::super::*; use super::*; - use crate::devices::virtio::vsock::packet::VSOCK_PKT_HDR_SIZE; use crate::devices::virtio::vsock::test_utils::{EventHandlerContext, TestContext}; - use crate::test_utils::multi_region_mem; - use crate::vstate::memory::Bytes; #[test] fn test_txq_event() { @@ -427,8 +424,9 @@ mod tests { // function for testing error cases, so the asserts always expect is_err() to be true. When // desc_idx = 0 we are altering the header (first descriptor in the chain), and when // desc_idx = 1 we are altering the packet buffer. + #[cfg(target_arch = "x86_64")] fn vsock_bof_helper(test_ctx: &mut TestContext, desc_idx: usize, addr: u64, len: u32) { - use crate::vstate::memory::GuestAddress; + use crate::vstate::memory::{Bytes, GuestAddress}; assert!(desc_idx <= 1); @@ -472,19 +470,22 @@ mod tests { } #[test] + #[cfg(target_arch = "x86_64")] + #[allow(clippy::cast_possible_truncation)] /* casting of constants we know fit into u32 */ fn test_vsock_bof() { + use crate::arch::MMIO_MEM_START; + use crate::arch::x86_64::{FIRST_ADDR_PAST_32BITS, MEM_32BIT_GAP_SIZE}; + use crate::devices::virtio::vsock::packet::VSOCK_PKT_HDR_SIZE; + use crate::test_utils::multi_region_mem; use crate::vstate::memory::GuestAddress; - const GAP_SIZE: u32 = 768 << 20; - const FIRST_AFTER_GAP: usize = 1 << 32; - const GAP_START_ADDR: usize = FIRST_AFTER_GAP - GAP_SIZE as usize; const MIB: usize = 1 << 20; let mut test_ctx = TestContext::new(); test_ctx.mem = multi_region_mem(&[ (GuestAddress(0), 8 * MIB), - (GuestAddress((GAP_START_ADDR - MIB) as u64), MIB), - (GuestAddress(FIRST_AFTER_GAP as u64), MIB), + (GuestAddress(MMIO_MEM_START - MIB as u64), MIB), + (GuestAddress(FIRST_ADDR_PAST_32BITS), MIB), ]); // The default configured descriptor chains are valid. @@ -506,20 +507,25 @@ mod tests { } // Let's check what happens when the header descriptor is right before the gap. - vsock_bof_helper( - &mut test_ctx, - 0, - GAP_START_ADDR as u64 - 1, - VSOCK_PKT_HDR_SIZE, - ); + vsock_bof_helper(&mut test_ctx, 0, MMIO_MEM_START - 1, VSOCK_PKT_HDR_SIZE); // Let's check what happens when the buffer descriptor crosses into the gap, but does // not go past its right edge. - vsock_bof_helper(&mut test_ctx, 1, GAP_START_ADDR as u64 - 4, GAP_SIZE + 4); + vsock_bof_helper( + &mut test_ctx, + 1, + MMIO_MEM_START - 4, + MEM_32BIT_GAP_SIZE as u32 + 4, + ); // Let's modify the buffer descriptor addr and len such that it crosses over the MMIO gap, // and check we cannot assemble the VsockPkts. - vsock_bof_helper(&mut test_ctx, 1, GAP_START_ADDR as u64 - 4, GAP_SIZE + 100); + vsock_bof_helper( + &mut test_ctx, + 1, + MMIO_MEM_START - 4, + MEM_32BIT_GAP_SIZE as u32 + 100, + ); } #[test] From 6b7061d14e6ff0df912f4147a3d9708b1523f9d9 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 21 Mar 2025 17:24:43 +0000 Subject: [PATCH 4/9] refactor: introduce mib_to_bytes helper Left-shifting by 20 to convert MiB to bytes is ubiquitous in our codebase, but probably somewhat arcane to people not familiar with low level stuff. Let's define a helper for the conversion. Signed-off-by: Patrick Roy --- src/vmm/src/arch/x86_64/mod.rs | 10 +++++----- src/vmm/src/builder.rs | 3 ++- src/vmm/src/devices/virtio/vsock/event_handler.rs | 3 ++- src/vmm/src/resources.rs | 4 +++- src/vmm/src/utils/mod.rs | 8 ++++++++ src/vmm/src/vstate/memory.rs | 8 ++++---- src/vmm/src/vstate/vcpu.rs | 3 ++- src/vmm/src/vstate/vm.rs | 3 ++- 8 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/vmm/src/arch/x86_64/mod.rs b/src/vmm/src/arch/x86_64/mod.rs index 9c3c2b11ce3..ad680d89cff 100644 --- a/src/vmm/src/arch/x86_64/mod.rs +++ b/src/vmm/src/arch/x86_64/mod.rs @@ -41,7 +41,7 @@ use linux_loader::loader::elf::start_info::{ use crate::arch::{BootProtocol, InitrdConfig, SYSTEM_MEM_SIZE, SYSTEM_MEM_START}; use crate::device_manager::resources::ResourceAllocator; -use crate::utils::u64_to_usize; +use crate::utils::{mib_to_bytes, u64_to_usize}; use crate::vstate::memory::{ Address, GuestAddress, GuestMemory, GuestMemoryMmap, GuestMemoryRegion, }; @@ -77,7 +77,7 @@ pub enum ConfigurationError { pub const FIRST_ADDR_PAST_32BITS: u64 = 1 << 32; /// Size of MMIO gap at top of 32-bit address space. -pub const MEM_32BIT_GAP_SIZE: u64 = 768 << 20; +pub const MEM_32BIT_GAP_SIZE: u64 = mib_to_bytes(768) as u64; /// The start of the memory area reserved for MMIO devices. pub const MMIO_MEM_START: u64 = FIRST_ADDR_PAST_32BITS - MEM_32BIT_GAP_SIZE; /// The size of the memory area reserved for MMIO devices. @@ -403,7 +403,7 @@ mod tests { ); // Now assigning some memory that falls before the 32bit memory hole. - let mem_size = 128 << 20; + let mem_size = mib_to_bytes(128); let gm = arch_mem(mem_size); let mut resource_allocator = ResourceAllocator::new().unwrap(); configure_system( @@ -428,7 +428,7 @@ mod tests { .unwrap(); // Now assigning some memory that is equal to the start of the 32bit memory hole. - let mem_size = 3328 << 20; + let mem_size = mib_to_bytes(3328); let gm = arch_mem(mem_size); let mut resource_allocator = ResourceAllocator::new().unwrap(); configure_system( @@ -453,7 +453,7 @@ mod tests { .unwrap(); // Now assigning some memory that falls after the 32bit memory hole. - let mem_size = 3330 << 20; + let mem_size = mib_to_bytes(3330); let gm = arch_mem(mem_size); let mut resource_allocator = ResourceAllocator::new().unwrap(); configure_system( diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index ae9cab9e51c..d21ed0ca40e 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -1050,6 +1050,7 @@ pub(crate) mod tests { use crate::mmds::data_store::{Mmds, MmdsVersion}; use crate::mmds::ns::MmdsNetworkStack; use crate::test_utils::{single_region_mem, single_region_mem_at}; + use crate::utils::mib_to_bytes; use crate::vmm_config::balloon::{BALLOON_DEV_ID, BalloonBuilder, BalloonDeviceConfig}; use crate::vmm_config::boot_source::DEFAULT_KERNEL_CMDLINE; use crate::vmm_config::drive::{BlockBuilder, BlockDeviceConfig}; @@ -1113,7 +1114,7 @@ pub(crate) mod tests { } pub(crate) fn default_vmm() -> Vmm { - let (kvm, mut vm, guest_memory) = setup_vm_with_memory(128 << 20); + let (kvm, mut vm, guest_memory) = setup_vm_with_memory(mib_to_bytes(128)); let mmio_device_manager = MMIODeviceManager::new(); let acpi_device_manager = ACPIDeviceManager::new(); diff --git a/src/vmm/src/devices/virtio/vsock/event_handler.rs b/src/vmm/src/devices/virtio/vsock/event_handler.rs index 2dab8490119..632148546e5 100755 --- a/src/vmm/src/devices/virtio/vsock/event_handler.rs +++ b/src/vmm/src/devices/virtio/vsock/event_handler.rs @@ -477,9 +477,10 @@ mod tests { use crate::arch::x86_64::{FIRST_ADDR_PAST_32BITS, MEM_32BIT_GAP_SIZE}; use crate::devices::virtio::vsock::packet::VSOCK_PKT_HDR_SIZE; use crate::test_utils::multi_region_mem; + use crate::utils::mib_to_bytes; use crate::vstate::memory::GuestAddress; - const MIB: usize = 1 << 20; + const MIB: usize = mib_to_bytes(1); let mut test_ctx = TestContext::new(); test_ctx.mem = multi_region_mem(&[ diff --git a/src/vmm/src/resources.rs b/src/vmm/src/resources.rs index e65cc7a0121..2d472a3f3e2 100644 --- a/src/vmm/src/resources.rs +++ b/src/vmm/src/resources.rs @@ -13,6 +13,7 @@ use crate::logger::info; use crate::mmds; use crate::mmds::data_store::{Mmds, MmdsVersion}; use crate::mmds::ns::MmdsNetworkStack; +use crate::utils::mib_to_bytes; use crate::utils::net::ipv4addr::is_link_local_valid; use crate::vmm_config::balloon::*; use crate::vmm_config::boot_source::{ @@ -462,7 +463,8 @@ impl VmResources { self.machine_config.huge_pages, ) } else { - let regions = crate::arch::arch_memory_regions(self.machine_config.mem_size_mib << 20); + let regions = + crate::arch::arch_memory_regions(mib_to_bytes(self.machine_config.mem_size_mib)); GuestMemoryMmap::anonymous( regions.into_iter(), self.machine_config.track_dirty_pages, diff --git a/src/vmm/src/utils/mod.rs b/src/vmm/src/utils/mod.rs index a0ee2e90b6b..9c81c7a3016 100644 --- a/src/vmm/src/utils/mod.rs +++ b/src/vmm/src/utils/mod.rs @@ -13,6 +13,9 @@ pub mod sm; use std::num::Wrapping; use std::result::Result; +/// How many bits to left-shift by to convert MiB to bytes +const MIB_TO_BYTES_SHIFT: usize = 20; + /// Return the default page size of the platform, in bytes. pub fn get_page_size() -> Result { // SAFETY: Safe because the parameters are valid. @@ -45,3 +48,8 @@ pub const fn usize_to_u64(num: usize) -> u64 { pub const fn wrap_usize_to_u32(num: usize) -> Wrapping { Wrapping(((num as u64) & 0xFFFFFFFF) as u32) } + +/// Converts MiB to Bytes +pub const fn mib_to_bytes(mib: usize) -> usize { + mib << MIB_TO_BYTES_SHIFT +} diff --git a/src/vmm/src/vstate/memory.rs b/src/vmm/src/vstate/memory.rs index e0f47848684..8875791f3a6 100644 --- a/src/vmm/src/vstate/memory.rs +++ b/src/vmm/src/vstate/memory.rs @@ -23,7 +23,7 @@ use vmm_sys_util::errno; use crate::DirtyBitmap; use crate::arch::arch_memory_regions; -use crate::utils::{get_page_size, u64_to_usize}; +use crate::utils::{get_page_size, mib_to_bytes, u64_to_usize}; use crate::vmm_config::machine_config::HugePageConfig; /// Type of GuestMemoryMmap. @@ -72,7 +72,7 @@ where huge_pages: HugePageConfig, ) -> Result { let memfd_file = create_memfd(mem_size_mib, huge_pages.into())?.into_file(); - let regions = arch_memory_regions(mem_size_mib << 20).into_iter(); + let regions = arch_memory_regions(mib_to_bytes(mem_size_mib)).into_iter(); Self::create( regions, @@ -330,7 +330,7 @@ fn create_memfd( size: usize, hugetlb_size: Option, ) -> Result { - let mem_size = size << 20; + let mem_size = mib_to_bytes(size); // Create a memfd. let opts = memfd::MemfdOptions::default() .hugetlb(hugetlb_size) @@ -713,7 +713,7 @@ mod tests { #[test] fn test_create_memfd() { let size = 1; - let size_mb = 1 << 20; + let size_mb = mib_to_bytes(1) as u64; let memfd = create_memfd(size, None).unwrap(); diff --git a/src/vmm/src/vstate/vcpu.rs b/src/vmm/src/vstate/vcpu.rs index d54d7de759b..b7de0017e46 100644 --- a/src/vmm/src/vstate/vcpu.rs +++ b/src/vmm/src/vstate/vcpu.rs @@ -775,6 +775,7 @@ pub(crate) mod tests { use crate::devices::BusDevice; use crate::devices::bus::DummyDevice; use crate::seccomp::get_empty_filters; + use crate::utils::mib_to_bytes; use crate::utils::signal::validate_signal_num; use crate::vstate::kvm::Kvm; use crate::vstate::memory::{GuestAddress, GuestMemoryMmap}; @@ -962,7 +963,7 @@ pub(crate) mod tests { fn vcpu_configured_for_boot() -> (VcpuHandle, EventFd, GuestMemoryMmap) { Vcpu::register_kick_signal_handler(); // Need enough mem to boot linux. - let mem_size = 64 << 20; + let mem_size = mib_to_bytes(64); let (kvm, _, mut vcpu, vm_mem) = setup_vcpu(mem_size); let vcpu_exit_evt = vcpu.exit_evt.try_clone().unwrap(); diff --git a/src/vmm/src/vstate/vm.rs b/src/vmm/src/vstate/vm.rs index 4292c2f2f49..bb13b17ee99 100644 --- a/src/vmm/src/vstate/vm.rs +++ b/src/vmm/src/vstate/vm.rs @@ -135,6 +135,7 @@ impl Vm { pub(crate) mod tests { use super::*; use crate::test_utils::single_region_mem; + use crate::utils::mib_to_bytes; use crate::vstate::kvm::Kvm; use crate::vstate::memory::GuestMemoryMmap; @@ -189,7 +190,7 @@ pub(crate) mod tests { #[test] fn test_create_vcpus() { let vcpu_count = 2; - let (_, mut vm, _) = setup_vm_with_memory(128 << 20); + let (_, mut vm, _) = setup_vm_with_memory(mib_to_bytes(128)); let (vcpu_vec, _) = vm.create_vcpus(vcpu_count).unwrap(); From 3dc7319175846e4380c8d6e2653845ddde758323 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 24 Mar 2025 10:30:05 +0000 Subject: [PATCH 5/9] refactor: drop pointless .map() calls The return value here was ignored anyway, so no need to map the unit () to VmmData::Empty. Signed-off-by: Patrick Roy --- src/vmm/src/rpc_interface.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/vmm/src/rpc_interface.rs b/src/vmm/src/rpc_interface.rs index 8eb7227347a..127b75e594e 100644 --- a/src/vmm/src/rpc_interface.rs +++ b/src/vmm/src/rpc_interface.rs @@ -808,14 +808,12 @@ impl RuntimeApiController { // vhost-user-block updates if new_cfg.path_on_host.is_none() && new_cfg.rate_limiter.is_none() { vmm.update_vhost_user_block_config(&new_cfg.drive_id) - .map(|()| VmmData::Empty) .map_err(DriveError::DeviceUpdate)?; } // virtio-block updates if let Some(new_path) = new_cfg.path_on_host { vmm.update_block_device_path(&new_cfg.drive_id, new_path) - .map(|()| VmmData::Empty) .map_err(DriveError::DeviceUpdate)?; } if new_cfg.rate_limiter.is_some() { @@ -824,7 +822,6 @@ impl RuntimeApiController { RateLimiterUpdate::from(new_cfg.rate_limiter).bandwidth, RateLimiterUpdate::from(new_cfg.rate_limiter).ops, ) - .map(|()| VmmData::Empty) .map_err(DriveError::DeviceUpdate)?; } Ok(VmmData::Empty) From 062d491c36eda3ec7c399eee0f3f9bf0000b375c Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 24 Mar 2025 10:40:12 +0000 Subject: [PATCH 6/9] refactor: move StartMicrovmError::AllocateResources into VmmError This means that `create_vmm_and_vcpus` now does not use any variants of `StartMicrovmError` directly, and as a next step can be converted to return `VmmError` directly, which will eliminate all the `map_err` inside. Signed-off-by: Patrick Roy --- src/vmm/src/builder.rs | 6 +++--- src/vmm/src/lib.rs | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index d21ed0ca40e..fc9da4b996b 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -130,8 +130,6 @@ pub enum StartMicrovmError { SetVmResources(MachineConfigError), /// Cannot create the entropy device: {0} CreateEntropyDevice(crate::devices::virtio::rng::EntropyError), - /// Failed to allocate guest resource: {0} - AllocateResources(#[from] vm_allocator::Error), /// Error configuring ACPI: {0} #[cfg(target_arch = "x86_64")] Acpi(#[from] crate::acpi::AcpiError), @@ -177,7 +175,9 @@ fn create_vmm_and_vcpus( .map_err(VmmError::Vm) .map_err(StartMicrovmError::Internal)?; - let resource_allocator = ResourceAllocator::new()?; + let resource_allocator = ResourceAllocator::new() + .map_err(VmmError::AllocateResources) + .map_err(StartMicrovmError::Internal)?; // Instantiate the MMIO device manager. let mmio_device_manager = MMIODeviceManager::new(); diff --git a/src/vmm/src/lib.rs b/src/vmm/src/lib.rs index 492077df068..4908731c4f7 100644 --- a/src/vmm/src/lib.rs +++ b/src/vmm/src/lib.rs @@ -205,6 +205,8 @@ pub const HTTP_MAX_PAYLOAD_SIZE: usize = 51200; /// have permissions to open the KVM fd). #[derive(Debug, thiserror::Error, displaydoc::Display)] pub enum VmmError { + /// Failed to allocate guest resource: {0} + AllocateResources(#[from] vm_allocator::Error), #[cfg(target_arch = "aarch64")] /// Invalid command line error. Cmdline, From 85eca1bc72de4d2e994f449a130fec4a8a064957 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 24 Mar 2025 10:43:38 +0000 Subject: [PATCH 7/9] refactor: Have create_vmm_and_vcpus return VmmError Eliminate a lot of .map_err(Internal) to make the code more readable. Signed-off-by: Patrick Roy --- src/vmm/src/builder.rs | 44 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index fc9da4b996b..e7472801b77 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -32,6 +32,7 @@ use vmm_sys_util::eventfd::EventFd; #[cfg(target_arch = "x86_64")] use crate::acpi; use crate::arch::{BootProtocol, EntryPoint, InitrdConfig}; +use crate::builder::StartMicrovmError::Internal; #[cfg(target_arch = "aarch64")] use crate::construct_kvm_mpidrs; use crate::cpu_config::templates::{ @@ -157,27 +158,15 @@ fn create_vmm_and_vcpus( uffd: Option, vcpu_count: u8, kvm_capabilities: Vec, -) -> Result<(Vmm, Vec), StartMicrovmError> { - use self::StartMicrovmError::*; - - let kvm = Kvm::new(kvm_capabilities) - .map_err(VmmError::Kvm) - .map_err(StartMicrovmError::Internal)?; +) -> Result<(Vmm, Vec), VmmError> { + let kvm = Kvm::new(kvm_capabilities).map_err(VmmError::Kvm)?; // Set up Kvm Vm and register memory regions. // Build custom CPU config if a custom template is provided. - let mut vm = Vm::new(&kvm) - .map_err(VmmError::Vm) - .map_err(StartMicrovmError::Internal)?; - kvm.check_memory(&guest_memory) - .map_err(VmmError::Kvm) - .map_err(StartMicrovmError::Internal)?; - vm.memory_init(&guest_memory) - .map_err(VmmError::Vm) - .map_err(StartMicrovmError::Internal)?; + let mut vm = Vm::new(&kvm).map_err(VmmError::Vm)?; + kvm.check_memory(&guest_memory).map_err(VmmError::Kvm)?; + vm.memory_init(&guest_memory).map_err(VmmError::Vm)?; - let resource_allocator = ResourceAllocator::new() - .map_err(VmmError::AllocateResources) - .map_err(StartMicrovmError::Internal)?; + let resource_allocator = ResourceAllocator::new().map_err(VmmError::AllocateResources)?; // Instantiate the MMIO device manager. let mmio_device_manager = MMIODeviceManager::new(); @@ -185,10 +174,7 @@ fn create_vmm_and_vcpus( // Instantiate ACPI device manager. let acpi_device_manager = ACPIDeviceManager::new(); - let (vcpus, vcpus_exit_evt) = vm - .create_vcpus(vcpu_count) - .map_err(VmmError::Vm) - .map_err(Internal)?; + let (vcpus, vcpus_exit_evt) = vm.create_vcpus(vcpu_count).map_err(VmmError::Vm)?; #[cfg(target_arch = "x86_64")] let pio_device_manager = { @@ -196,14 +182,10 @@ fn create_vmm_and_vcpus( set_stdout_nonblocking(); // Serial device setup. - let serial_device = - setup_serial_device(event_manager, std::io::stdin(), io::stdout()).map_err(Internal)?; + let serial_device = setup_serial_device(event_manager, std::io::stdin(), io::stdout())?; // x86_64 uses the i8042 reset event as the Vmm exit event. - let reset_evt = vcpus_exit_evt - .try_clone() - .map_err(VmmError::EventFd) - .map_err(Internal)?; + let reset_evt = vcpus_exit_evt.try_clone().map_err(VmmError::EventFd)?; // create pio dev manager with legacy devices // TODO Remove these unwraps. @@ -276,7 +258,8 @@ pub fn build_microvm_for_boot( None, vm_resources.machine_config.vcpu_count, cpu_template.kvm_capabilities.clone(), - )?; + ) + .map_err(Internal)?; #[cfg(feature = "gdb")] let (gdb_tx, gdb_rx) = mpsc::channel(); @@ -471,7 +454,8 @@ pub fn build_microvm_from_snapshot( uffd, vm_resources.machine_config.vcpu_count, microvm_state.kvm_state.kvm_cap_modifiers.clone(), - )?; + ) + .map_err(Internal)?; #[cfg(target_arch = "x86_64")] { From 8b26e0c99375f614f63128e446a5ae5dd0a8d3b2 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 24 Mar 2025 10:46:17 +0000 Subject: [PATCH 8/9] refactor: use #[from] in VmmError When a variant wraps another error type, and this error type does not appear in any other variant, use thiserrors #[from] directive to implement a `From` impl. This allows eliminating almost all remaining `map_err`s from `create_vmm_and_vcpus`, as the `?` operator will automatically call .from() once (although it cannot do it for multiple layers of wrapping, which is why it was important to change the function to return VmmError). Signed-off-by: Patrick Roy --- src/vmm/src/builder.rs | 12 ++++++------ src/vmm/src/lib.rs | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index e7472801b77..f8d51b69573 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -159,14 +159,14 @@ fn create_vmm_and_vcpus( vcpu_count: u8, kvm_capabilities: Vec, ) -> Result<(Vmm, Vec), VmmError> { - let kvm = Kvm::new(kvm_capabilities).map_err(VmmError::Kvm)?; + let kvm = Kvm::new(kvm_capabilities)?; // Set up Kvm Vm and register memory regions. // Build custom CPU config if a custom template is provided. - let mut vm = Vm::new(&kvm).map_err(VmmError::Vm)?; - kvm.check_memory(&guest_memory).map_err(VmmError::Kvm)?; - vm.memory_init(&guest_memory).map_err(VmmError::Vm)?; + let mut vm = Vm::new(&kvm)?; + kvm.check_memory(&guest_memory)?; + vm.memory_init(&guest_memory)?; - let resource_allocator = ResourceAllocator::new().map_err(VmmError::AllocateResources)?; + let resource_allocator = ResourceAllocator::new()?; // Instantiate the MMIO device manager. let mmio_device_manager = MMIODeviceManager::new(); @@ -174,7 +174,7 @@ fn create_vmm_and_vcpus( // Instantiate ACPI device manager. let acpi_device_manager = ACPIDeviceManager::new(); - let (vcpus, vcpus_exit_evt) = vm.create_vcpus(vcpu_count).map_err(VmmError::Vm)?; + let (vcpus, vcpus_exit_evt) = vm.create_vcpus(vcpu_count)?; #[cfg(target_arch = "x86_64")] let pio_device_manager = { diff --git a/src/vmm/src/lib.rs b/src/vmm/src/lib.rs index 4908731c4f7..993409dc819 100644 --- a/src/vmm/src/lib.rs +++ b/src/vmm/src/lib.rs @@ -257,9 +257,9 @@ pub enum VmmError { /// Cannot spawn Vcpu thread: {0} VcpuSpawn(io::Error), /// Vm error: {0} - Vm(vstate::vm::VmError), + Vm(#[from] vstate::vm::VmError), /// Kvm error: {0} - Kvm(vstate::kvm::KvmError), + Kvm(#[from] vstate::kvm::KvmError), /// Error thrown by observer object on Vmm initialization: {0} VmmObserverInit(vmm_sys_util::errno::Error), /// Error thrown by observer object on Vmm teardown: {0} From 52104ba3cdbabbeadd63125724e589273b1e83ff Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 24 Mar 2025 10:53:09 +0000 Subject: [PATCH 9/9] refactor: impl From for StartMicrovmError This allows us to eliminate some more .map_err(Internal). Signed-off-by: Patrick Roy --- src/vmm/src/builder.rs | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/vmm/src/builder.rs b/src/vmm/src/builder.rs index f8d51b69573..cd1e063fb40 100644 --- a/src/vmm/src/builder.rs +++ b/src/vmm/src/builder.rs @@ -104,7 +104,7 @@ pub enum StartMicrovmError { /// Cannot load initrd due to an invalid image: {0} InitrdRead(io::Error), /// Internal error while starting microVM: {0} - Internal(VmmError), + Internal(#[from] VmmError), /// Failed to get CPU template: {0} GetCpuTemplate(#[from] GetCpuTemplateError), /// Invalid kernel command line: {0} @@ -258,8 +258,7 @@ pub fn build_microvm_for_boot( None, vm_resources.machine_config.vcpu_count, cpu_template.kvm_capabilities.clone(), - ) - .map_err(Internal)?; + )?; #[cfg(feature = "gdb")] let (gdb_tx, gdb_rx) = mpsc::channel(); @@ -306,7 +305,7 @@ pub fn build_microvm_for_boot( } #[cfg(target_arch = "aarch64")] - attach_legacy_devices_aarch64(event_manager, &mut vmm, &mut boot_cmdline).map_err(Internal)?; + attach_legacy_devices_aarch64(event_manager, &mut vmm, &mut boot_cmdline)?; attach_vmgenid_device(&mut vmm)?; @@ -346,8 +345,7 @@ pub fn build_microvm_for_boot( .ok_or_else(|| MissingSeccompFilters("vcpu".to_string()))? .clone(), ) - .map_err(VmmError::VcpuStart) - .map_err(Internal)?; + .map_err(VmmError::VcpuStart)?; // Load seccomp filters for the VMM thread. // Execution panics if filters cannot be loaded, use --no-seccomp if skipping filters @@ -358,8 +356,7 @@ pub fn build_microvm_for_boot( .get("vmm") .ok_or_else(|| MissingSeccompFilters("vmm".to_string()))?, ) - .map_err(VmmError::SeccompFilters) - .map_err(Internal)?; + .map_err(VmmError::SeccompFilters)?; event_manager.add_subscriber(vmm.clone()); @@ -384,10 +381,7 @@ pub fn build_and_boot_microvm( debug!("event_end: build microvm for boot"); // The vcpus start off in the `Paused` state, let them run. debug!("event_start: boot microvm"); - vmm.lock() - .unwrap() - .resume_vm() - .map_err(StartMicrovmError::Internal)?; + vmm.lock().unwrap().resume_vm()?; debug!("event_end: boot microvm"); Ok(vmm) } @@ -559,7 +553,7 @@ fn load_kernel( let mut kernel_file = boot_config .kernel_file .try_clone() - .map_err(|err| StartMicrovmError::Internal(VmmError::KernelFile(err)))?; + .map_err(VmmError::KernelFile)?; let entry_addr = Loader::load::( guest_memory, @@ -593,7 +587,7 @@ fn load_kernel( let mut kernel_file = boot_config .kernel_file .try_clone() - .map_err(|err| StartMicrovmError::Internal(VmmError::KernelFile(err)))?; + .map_err(VmmError::KernelFile)?; let entry_addr = Loader::load::( guest_memory, @@ -762,8 +756,7 @@ pub fn configure_system_for_boot( for vcpu in vcpus.iter_mut() { vcpu.kvm_vcpu .init(&cpu_template.vcpu_features) - .map_err(VmmError::VcpuInit) - .map_err(Internal)?; + .map_err(VmmError::VcpuInit)?; } let mut regs = Aarch64RegisterVec::default(); @@ -787,8 +780,7 @@ pub fn configure_system_for_boot( for vcpu in vcpus.iter_mut() { vcpu.kvm_vcpu .configure(vmm.guest_memory(), entry_point, &vcpu_config) - .map_err(VmmError::VcpuConfigure) - .map_err(Internal)?; + .map_err(VmmError::VcpuConfigure)?; } // Write the kernel command line to guest memory. This is x86_64 specific, since on @@ -836,8 +828,7 @@ pub fn configure_system_for_boot( &vcpu_config, &optional_capabilities, ) - .map_err(VmmError::VcpuConfigure) - .map_err(Internal)?; + .map_err(VmmError::VcpuConfigure)?; } let vcpu_mpidr = vcpus .iter_mut()