WIP: add option to force guests to use swiotlb in dedicated memory region for all I/O

CHANGELOG.md

@@ -39,6 +39,9 @@ and this project adheres to
   the `SendCtrlAltDel` command not working for ACPI-enabled guest kernels, by
   dropping the i8042.nopnp argument from the default kernel command line
   Firecracker constructs.
+- #[???](???): Fix failure during diff snapshot creation corrupting internal
+  state about which pages were dirtied since the last snapshot, meaning no
+  further diff snapshots could ever be taken, even if the error is transient.
 
 ## [1.11.0]
 

Cargo.toml

@@ -32,3 +32,7 @@ strip = "none"
 
 [profile.bench]
 strip = "debuginfo"
+
+[patch.crates-io]
+linux-loader = { git = "https://github.com/roypat/linux-loader", branch = "vm-memory-update" }
+vm-memory = { git = "https://github.com/roypat/vm-memory", branch = "generic-region-container" }

resources/guest_configs/ci.config

@@ -5,6 +5,7 @@ CONFIG_SQUASHFS_ZSTD=y
 # aarch64 only TBD split into a separate file
 CONFIG_DEVMEM=y
 # CONFIG_ARM64_ERRATUM_3194386 is not set
+CONFIG_DMA_RESTRICTED_POOL=y
 # Needed for CTRL+ALT+DEL support
 CONFIG_SERIO=y
 CONFIG_SERIO_I8042=y

src/firecracker/src/api_server/request/machine_configuration.rs

@@ -119,6 +119,7 @@ mod tests {
             let expected_config = MachineConfigUpdate {
                 vcpu_count: Some(8),
                 mem_size_mib: Some(1024),
+                mem_config: Some(Default::default()),
                 smt: Some(false),
                 cpu_template: None,
                 track_dirty_pages: Some(false),
@@ -140,6 +141,7 @@ mod tests {
         let expected_config = MachineConfigUpdate {
             vcpu_count: Some(8),
             mem_size_mib: Some(1024),
+            mem_config: Some(Default::default()),
             smt: Some(false),
             cpu_template: Some(StaticCpuTemplate::None),
             track_dirty_pages: Some(false),
@@ -161,6 +163,7 @@ mod tests {
         let expected_config = MachineConfigUpdate {
             vcpu_count: Some(8),
             mem_size_mib: Some(1024),
+            mem_config: Some(Default::default()),
             smt: Some(false),
             cpu_template: None,
             track_dirty_pages: Some(true),
@@ -186,6 +189,7 @@ mod tests {
             let expected_config = MachineConfigUpdate {
                 vcpu_count: Some(8),
                 mem_size_mib: Some(1024),
+                mem_config: Some(Default::default()),
                 smt: Some(false),
                 cpu_template: Some(StaticCpuTemplate::T2),
                 track_dirty_pages: Some(true),
@@ -213,6 +217,7 @@ mod tests {
         let expected_config = MachineConfigUpdate {
             vcpu_count: Some(8),
             mem_size_mib: Some(1024),
+            mem_config: Some(Default::default()),
             smt: Some(true),
             cpu_template: None,
             track_dirty_pages: Some(true),

src/vmm/benches/memory_access.rs

@@ -2,7 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use criterion::{BatchSize, Criterion, criterion_group, criterion_main};
-use vm_memory::GuestMemory;
 use vmm::resources::VmResources;
 use vmm::vmm_config::machine_config::{HugePageConfig, MachineConfig};
 
@@ -14,7 +13,7 @@ fn bench_single_page_fault(c: &mut Criterion, configuration: VmResources) {
                 // Get a pointer to the first memory region (cannot do `.get_slice(GuestAddress(0),
                 // 1)`, because on ARM64 guest memory does not start at physical
                 // address 0).
-                let ptr = memory.iter().next().unwrap().as_ptr();
+                let ptr = memory.first().unwrap().as_ptr();
 
                 // fine to return both here, because ptr is not a reference into `memory` (e.g. no
                 // self-referential structs are happening here)

src/vmm/src/acpi/mod.rs

@@ -188,7 +188,9 @@ mod tests {
     use crate::acpi::{AcpiError, AcpiTableWriter};
     use crate::arch::x86_64::layout::{SYSTEM_MEM_SIZE, SYSTEM_MEM_START};
     use crate::builder::tests::default_vmm;
-    use crate::test_utils::arch_mem;
+    use crate::device_manager::resources::ResourceAllocator;
+    use crate::utils::u64_to_usize;
+    use crate::vstate::vm::tests::setup_vm_with_memory;
 
     struct MockSdt(Vec<u8>);
 
@@ -215,7 +217,7 @@ mod tests {
         // A mocke Vmm object with 128MBs of memory
         let mut vmm = default_vmm();
         let mut writer = AcpiTableWriter {
-            mem: &vmm.guest_memory,
+            mem: vmm.vm.guest_memory(),
             resource_allocator: &mut vmm.resource_allocator,
         };
 
@@ -263,15 +265,10 @@ mod tests {
     // change in the future.
     #[test]
     fn test_write_acpi_table_small_memory() {
-        let mut vmm = default_vmm();
-        vmm.guest_memory = arch_mem(
-            (SYSTEM_MEM_START + SYSTEM_MEM_SIZE - 4096)
-                .try_into()
-                .unwrap(),
-        );
+        let (_, vm) = setup_vm_with_memory(u64_to_usize(SYSTEM_MEM_START + SYSTEM_MEM_SIZE - 4096));
         let mut writer = AcpiTableWriter {
-            mem: &vmm.guest_memory,
-            resource_allocator: &mut vmm.resource_allocator,
+            mem: vm.guest_memory(),
+            resource_allocator: &mut ResourceAllocator::new().unwrap(),
         };
 
         let mut sdt = MockSdt(vec![0; usize::try_from(SYSTEM_MEM_SIZE).unwrap()]);

src/vmm/src/arch/aarch64/fdt.rs

@@ -10,20 +10,23 @@
 use std::fmt::Debug;
 
 use vm_fdt::{Error as VmFdtError, FdtWriter, FdtWriterNode};
-use vm_memory::GuestMemoryError;
+use vm_memory::{GuestMemoryError, GuestMemoryRegion};
 
 use super::super::DeviceType;
 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::initrd::InitrdConfig;
-use crate::vstate::memory::{Address, GuestMemory, GuestMemoryMmap};
+use crate::vstate::memory::{Address, GuestMemory, GuestMemoryMmap, KvmRegion};
 
 // This is a value for uniquely identifying the FDT node declaring the interrupt controller.
 const GIC_PHANDLE: u32 = 1;
 // This is a value for uniquely identifying the FDT node containing the clock definition.
 const CLOCK_PHANDLE: u32 = 2;
+/// Unique identifier for the /reserved-memory node describing the memory region the guest
+/// is allowed to use for swiotlb
+const IO_MEM_PHANDLE: u32 = 3;
 // You may be wondering why this big value?
 // This phandle is used to uniquely identify the FDT nodes containing cache information. Each cpu
 // can have a variable number of caches, some of these caches may be shared with other cpus.
@@ -56,8 +59,10 @@
 }
 
 /// Creates the flattened device tree for this aarch64 microVM.
+#[allow(clippy::too_many_arguments)]
 pub fn create_fdt(
     guest_mem: &GuestMemoryMmap,
+    swiotlb_region: Option<&KvmRegion>,
     vcpu_mpidr: Vec<u64>,
     cmdline: CString,
     device_info: &HashMap<(DeviceType, String), MMIODeviceInfo>,
@@ -83,7 +88,7 @@
     // containing description of the interrupt controller for this VM.
     fdt_writer.property_u32("interrupt-parent", GIC_PHANDLE)?;
     create_cpu_nodes(&mut fdt_writer, &vcpu_mpidr)?;
-    create_memory_node(&mut fdt_writer, guest_mem)?;
+    create_memory_node(&mut fdt_writer, guest_mem, swiotlb_region)?;
     create_chosen_node(&mut fdt_writer, cmdline, initrd)?;
     create_gic_node(&mut fdt_writer, gic_device)?;
     create_timer_node(&mut fdt_writer)?;
@@ -208,7 +213,11 @@
     Ok(())
 }
 
-fn create_memory_node(fdt: &mut FdtWriter, guest_mem: &GuestMemoryMmap) -> Result<(), FdtError> {
+fn create_memory_node(
+    fdt: &mut FdtWriter,
+    guest_mem: &GuestMemoryMmap,
+    swiotlb_region: Option<&KvmRegion>,
+) -> Result<(), FdtError> {
     // See https://github.com/torvalds/linux/blob/master/Documentation/devicetree/booting-without-of.txt#L960
     // for an explanation of this.
 
@@ -220,9 +229,13 @@
     // The reason we do this is that Linux does not allow remapping system memory. However, without
     // remap, kernel drivers cannot get virtual addresses to read data from device memory. Leaving
     // this memory region out allows Linux kernel modules to remap and thus read this region.
+    //
+    // The generic memory description must include the range that we later declare as a reserved
+    // carve out.
     let mem_size = guest_mem.last_addr().raw_value()
         - super::layout::DRAM_MEM_START
         - super::layout::SYSTEM_MEM_SIZE
+        + swiotlb_region.map(|r| r.len()).unwrap_or(0)
         + 1;
     let mem_reg_prop = &[
         super::layout::DRAM_MEM_START + super::layout::SYSTEM_MEM_SIZE,
@@ -233,6 +246,25 @@
     fdt.property_array_u64("reg", mem_reg_prop)?;
     fdt.end_node(mem)?;
 
+    if let Some(region) = swiotlb_region {
+        let rmem = fdt.begin_node("reserved-memory")?;
+        fdt.property_u32("#address-cells", ADDRESS_CELLS)?;
+        fdt.property_u32("#size-cells", SIZE_CELLS)?;
+        fdt.property_null("ranges")?;
+        let dma = fdt.begin_node("bouncy_boi")?;
+        fdt.property_string("compatible", "restricted-dma-pool")?;
+        fdt.property_phandle(IO_MEM_PHANDLE)?;
+        fdt.property_array_u64("reg", &[region.start_addr().0, region.len()])?;
+        fdt.end_node(dma)?;
+        fdt.end_node(rmem)?;
+
+        log::info!(
+            "Placed swiotlb region at [{}, {})",
+            region.start_addr().0,
+            region.start_addr().0 + region.len()
+        );
+    }
+
     Ok(())
 }
 
@@ -358,6 +390,9 @@
 
     fdt.property_string("compatible", "virtio,mmio")?;
     fdt.property_array_u64("reg", &[dev_info.addr, dev_info.len])?;
+    // Force all virtio device to place their vrings and buffer into the dedicated I/O memory region
+    // that is backed by traditional memory (e.g. not secret-free).
+    fdt.property_u32("memory-region", IO_MEM_PHANDLE)?;
     fdt.property_array_u32(
         "interrupts",
         &[
@@ -499,6 +534,7 @@
         let gic = create_gic(&vm, 1, None).unwrap();
         create_fdt(
             &mem,
+            None,
             vec![0],
             CString::new("console=tty0").unwrap(),
             &dev_info,
@@ -519,6 +555,7 @@
         let gic = create_gic(&vm, 1, None).unwrap();
         create_fdt(
             &mem,
+            None,
             vec![0],
             CString::new("console=tty0").unwrap(),
             &HashMap::<(DeviceType, std::string::String), MMIODeviceInfo>::new(),
@@ -544,6 +581,7 @@
 
         let current_dtb_bytes = create_fdt(
             &mem,
+            None,
             vec![0],
             CString::new("console=tty0").unwrap(),
             &HashMap::<(DeviceType, std::string::String), MMIODeviceInfo>::new(),
@@ -606,6 +644,7 @@
 
         let current_dtb_bytes = create_fdt(
             &mem,
+            None,
             vec![0],
             CString::new("console=tty0").unwrap(),
             &HashMap::<(DeviceType, std::string::String), MMIODeviceInfo>::new(),

src/vmm/src/arch/aarch64/kvm.rs

@@ -22,8 +22,6 @@ pub struct OptionalCapabilities {
 pub struct Kvm {
     /// KVM fd.
     pub fd: KvmFd,
-    /// Maximum number of memory slots allowed by KVM.
-    pub max_memslots: usize,
     /// Additional capabilities that were specified in cpu template.
     pub kvm_cap_modifiers: Vec<KvmCapability>,
 }
@@ -42,12 +40,10 @@ impl Kvm {
     /// Initialize [`Kvm`] type for Aarch64 architecture
     pub fn init_arch(
         fd: KvmFd,
-        max_memslots: usize,
         kvm_cap_modifiers: Vec<KvmCapability>,
     ) -> Result<Self, KvmArchError> {
         Ok(Self {
             fd,
-            max_memslots,
             kvm_cap_modifiers,
         })
     }

src/vmm/src/arch/aarch64/mod.rs

@@ -58,9 +58,20 @@
 
 /// Returns a Vec of the valid memory addresses for aarch64.
 /// See [`layout`](layout) module for a drawing of the specific memory model for this platform.
-pub fn arch_memory_regions(size: usize) -> Vec<(GuestAddress, usize)> {
-    let dram_size = min(size, layout::DRAM_MEM_MAX_SIZE);
-    vec![(GuestAddress(layout::DRAM_MEM_START), dram_size)]
+pub fn arch_memory_regions(offset: usize, size: usize) -> Vec<(GuestAddress, usize)> {
+    let dram_size = min(size, layout::DRAM_MEM_MAX_SIZE - offset);
+    vec![(
+        GuestAddress(layout::DRAM_MEM_START + offset as u64),
+        dram_size,
+    )]
+}
+
+/// How many bytes of physical guest memory are addressible before the final gap in
+/// the address space on this architecture.
+///
+/// There are no architectural gaps in the physical address space on aarch64, so this is 0
+pub fn bytes_before_last_gap() -> usize {
+    0
 }
 
 /// Configures the system for booting Linux.
@@ -89,7 +100,7 @@
     // Configure vCPUs with normalizing and setting the generated CPU configuration.
     for vcpu in vcpus.iter_mut() {
         vcpu.kvm_vcpu.configure(
-            &vmm.guest_memory,
+            vmm.vm.guest_memory(),
             entry_point,
             &vcpu_config,
             &optional_capabilities,
@@ -103,8 +114,16 @@
         .as_cstring()
         .expect("Cannot create cstring from cmdline string");
 
+    let swiotlb_region = match vmm.vm.swiotlb_regions().num_regions() {
+        0 | 1 => vmm.vm.swiotlb_regions().iter().next(),
+        _ => panic!(
+            "Firecracker tried to configure more than one swiotlb region. This is a logic bug."
+        ),
+    };
+
     let fdt = fdt::create_fdt(
-        &vmm.guest_memory,
+        vmm.vm.guest_memory(),
+        swiotlb_region,
         vcpu_mpidr,
         cmdline,
         vmm.mmio_device_manager.get_device_info(),
@@ -113,8 +132,10 @@
         initrd,
     )?;
 
-    let fdt_address = GuestAddress(get_fdt_addr(&vmm.guest_memory));
-    vmm.guest_memory.write_slice(fdt.as_slice(), fdt_address)?;
+    let fdt_address = GuestAddress(get_fdt_addr(vmm.vm.guest_memory()));
+    vmm.vm
+        .guest_memory()
+        .write_slice(fdt.as_slice(), fdt_address)?;
 
     Ok(())
 }
@@ -188,15 +209,15 @@
 
     #[test]
     fn test_regions_lt_1024gb() {
-        let regions = arch_memory_regions(1usize << 29);
+        let regions = arch_memory_regions(0, 1usize << 29);
         assert_eq!(1, regions.len());
         assert_eq!(GuestAddress(super::layout::DRAM_MEM_START), regions[0].0);
         assert_eq!(1usize << 29, regions[0].1);
     }
 
     #[test]
     fn test_regions_gt_1024gb() {
-        let regions = arch_memory_regions(1usize << 41);
+        let regions = arch_memory_regions(0, 1usize << 41);
         assert_eq!(1, regions.len());
         assert_eq!(GuestAddress(super::layout::DRAM_MEM_START), regions[0].0);
         assert_eq!(super::layout::DRAM_MEM_MAX_SIZE, regions[0].1);