arch: move GuestMemory creation to common of linux module

This requires exporting the memory layout from the arch crates, but it
does simplify the bloated build_vm interface a bit. It also will allow
for more fine-grained control the backing memory of GuestMemory.

TEST=test_all
BUG=b:183988204

Change-Id: Ie76755198d2fdc2a41bd538650939d6550686b88
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2809434
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Zach Reizner <zachr@chromium.org>

aarch64/src/lib.rs

@@ -4,10 +4,8 @@
 
 use std::collections::BTreeMap;
 use std::error::Error as StdError;
-use std::ffi::{CStr, CString};
 use std::fmt::{self, Display};
-use std::fs::File;
-use std::io::{self, Seek};
+use std::io::{self};
 use std::sync::Arc;
 
 use arch::{
@@ -15,13 +13,8 @@ use arch::{
     VmComponents, VmImage,
 };
 use base::Event;
-use devices::{
-    Bus, BusError, IrqChip, IrqChipAArch64, PciAddress, PciConfigMmio, PciDevice, PciInterruptPin,
-    ProtectionType,
-};
-use hypervisor::{
-    DeviceKind, Hypervisor, HypervisorCap, PsciVersion, VcpuAArch64, VcpuFeature, VmAArch64,
-};
+use devices::{Bus, BusError, IrqChip, IrqChipAArch64, PciConfigMmio, PciDevice, ProtectionType};
+use hypervisor::{DeviceKind, Hypervisor, HypervisorCap, VcpuAArch64, VcpuFeature, VmAArch64};
 use minijail::Minijail;
 use remain::sorted;
 use resources::SystemAllocator;
@@ -222,13 +215,19 @@ pub struct AArch64;
 impl arch::LinuxArch for AArch64 {
     type Error = Error;
 
-    fn build_vm<V, Vcpu, I, FD, FV, FI, E1, E2, E3>(
+    fn guest_memory_layout(
+        components: &VmComponents,
+    ) -> std::result::Result<Vec<(GuestAddress, u64)>, Self::Error> {
+        Ok(arch_memory_regions(components.memory_size))
+    }
+
+    fn build_vm<V, Vcpu, I, FD, FI, E1, E2>(
         mut components: VmComponents,
         serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>,
         serial_jail: Option<Minijail>,
         _battery: (&Option<BatteryType>, Option<Minijail>),
+        mut vm: V,
         create_devices: FD,
-        create_vm: FV,
         create_irq_chip: FI,
     ) -> std::result::Result<RunnableLinuxVm<V, Vcpu, I>, Self::Error>
     where
@@ -241,20 +240,17 @@ impl arch::LinuxArch for AArch64 {
             &mut SystemAllocator,
             &Event,
         ) -> std::result::Result<Vec<(Box<dyn PciDevice>, Option<Minijail>)>, E1>,
-        FV: FnOnce(GuestMemory) -> std::result::Result<V, E2>,
-        FI: FnOnce(&V, /* vcpu_count: */ usize) -> std::result::Result<I, E3>,
+        FI: FnOnce(&V, /* vcpu_count: */ usize) -> std::result::Result<I, E2>,
         E1: StdError + 'static,
         E2: StdError + 'static,
-        E3: StdError + 'static,
     {
         let has_bios = match components.vm_image {
             VmImage::Bios(_) => true,
             _ => false,
         };
 
+        let mem = vm.get_memory().clone();
         let mut resources = Self::get_resource_allocator(components.memory_size);
-        let mem = Self::setup_memory(components.memory_size)?;
-        let mut vm = create_vm(mem.clone()).map_err(|e| Error::CreateVm(Box::new(e)))?;
 
         if components.protected_vm == ProtectionType::Protected {
             vm.enable_protected_vm(
@@ -270,7 +266,7 @@ impl arch::LinuxArch for AArch64 {
         let vcpu_count = components.vcpu_count;
         let mut vcpus = Vec::with_capacity(vcpu_count);
         for vcpu_id in 0..vcpu_count {
-            let vcpu = *vm
+            let vcpu: Vcpu = *vm
                 .create_vcpu(vcpu_id)
                 .map_err(Error::CreateVcpu)?
                 .downcast::<Vcpu>()
@@ -429,12 +425,6 @@ impl arch::LinuxArch for AArch64 {
 }
 
 impl AArch64 {
-    fn setup_memory(mem_size: u64) -> Result<GuestMemory> {
-        let arch_mem_regions = arch_memory_regions(mem_size);
-        let mem = GuestMemory::new(&arch_mem_regions).map_err(Error::SetupGuestMemory)?;
-        Ok(mem)
-    }
-
     fn get_high_mmio_base_size(mem_size: u64) -> (u64, u64) {
         let base = AARCH64_PHYS_MEM_START + mem_size;
         let size = u64::max_value() - base;

arch/src/lib.rs

@@ -127,6 +127,16 @@ pub struct VirtioDeviceStub {
 pub trait LinuxArch {
     type Error: StdError;
 
+    /// Returns a Vec of the valid memory addresses as pairs of address and length. These should be
+    /// used to configure the `GuestMemory` structure for the platform.
+    ///
+    /// # Arguments
+    ///
+    /// * `components` - Parts used to determine the memory layout.
+    fn guest_memory_layout(
+        components: &VmComponents,
+    ) -> std::result::Result<Vec<(GuestAddress, u64)>, Self::Error>;
+
     /// Takes `VmComponents` and generates a `RunnableLinuxVm`.
     ///
     /// # Arguments
@@ -135,15 +145,14 @@ pub trait LinuxArch {
     /// * `serial_parameters` - definitions for how the serial devices should be configured.
     /// * `battery` - defines what battery device will be created.
     /// * `create_devices` - Function to generate a list of devices.
-    /// * `create_vm` - Function to generate a VM.
     /// * `create_irq_chip` - Function to generate an IRQ chip.
-    fn build_vm<V, Vcpu, I, FD, FV, FI, E1, E2, E3>(
+    fn build_vm<V, Vcpu, I, FD, FI, E1, E2>(
         components: VmComponents,
         serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>,
         serial_jail: Option<Minijail>,
         battery: (&Option<BatteryType>, Option<Minijail>),
+        vm: V,
         create_devices: FD,
-        create_vm: FV,
         create_irq_chip: FI,
     ) -> std::result::Result<RunnableLinuxVm<V, Vcpu, I>, Self::Error>
     where
@@ -156,11 +165,9 @@ pub trait LinuxArch {
             &mut SystemAllocator,
             &Event,
         ) -> std::result::Result<Vec<(Box<dyn PciDevice>, Option<Minijail>)>, E1>,
-        FV: FnOnce(GuestMemory) -> std::result::Result<V, E2>,
-        FI: FnOnce(&V, /* vcpu_count: */ usize) -> std::result::Result<I, E3>,
+        FI: FnOnce(&V, /* vcpu_count: */ usize) -> std::result::Result<I, E2>,
         E1: StdError + 'static,
-        E2: StdError + 'static,
-        E3: StdError + 'static;
+        E2: StdError + 'static;
 
     /// Configures the vcpu and should be called once per vcpu from the vcpu's thread.
     ///

src/crosvm.rs

@@ -201,6 +201,7 @@ pub struct Config {
     pub vcpu_affinity: Option<VcpuAffinity>,
     pub no_smt: bool,
     pub memory: Option<u64>,
+    pub memory_file: Option<PathBuf>,
     pub executable_path: Option<Executable>,
     pub android_fstab: Option<PathBuf>,
     pub initrd_path: Option<PathBuf>,
@@ -275,6 +276,7 @@ impl Default for Config {
             vcpu_affinity: None,
             no_smt: false,
             memory: None,
+            memory_file: None,
             executable_path: None,
             android_fstab: None,
             initrd_path: None,

src/linux.rs

@@ -105,6 +105,7 @@ pub enum Error {
     CreateDiskError(disk::Error),
     CreateEvent(base::Error),
     CreateGrallocError(rutabaga_gfx::RutabagaError),
+    CreateKvm(base::Error),
     CreateSignalFd(base::SignalFdError),
     CreateSocket(io::Error),
     CreateTapDevice(NetError),
@@ -114,6 +115,7 @@ pub enum Error {
     CreateUsbProvider(devices::usb::host_backend::error::Error),
     CreateVcpu(base::Error),
     CreateVfioDevice(devices::vfio::VfioError),
+    CreateVm(base::Error),
     CreateWaitContext(base::Error),
     DeviceJail(minijail::Error),
     DevicePivotRoot(minijail::Error),
@@ -131,6 +133,7 @@ pub enum Error {
     GuestCachedTooLarge(std::num::TryFromIntError),
     GuestFreeMissing(),
     GuestFreeTooLarge(std::num::TryFromIntError),
+    GuestMemoryLayout(<Arch as LinuxArch>::Error),
     #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
     HandleDebugCommand(<Arch as LinuxArch>::Error),
     InputDeviceNew(virtio::InputError),
@@ -222,6 +225,7 @@ impl Display for Error {
             CreateDiskError(e) => write!(f, "failed to create virtual disk: {}", e),
             CreateEvent(e) => write!(f, "failed to create event: {}", e),
             CreateGrallocError(e) => write!(f, "failed to create gralloc: {}", e),
+            CreateKvm(e) => write!(f, "failed to create kvm: {}", e),
             CreateSignalFd(e) => write!(f, "failed to create signalfd: {}", e),
             CreateSocket(e) => write!(f, "failed to create socket: {}", e),
             CreateTapDevice(e) => write!(f, "failed to create tap device: {}", e),
@@ -233,6 +237,7 @@ impl Display for Error {
             CreateUsbProvider(e) => write!(f, "failed to create usb provider: {}", e),
             CreateVcpu(e) => write!(f, "failed to create vcpu: {}", e),
             CreateVfioDevice(e) => write!(f, "Failed to create vfio device {}", e),
+            CreateVm(e) => write!(f, "failed to create vm: {}", e),
             CreateWaitContext(e) => write!(f, "failed to create wait context: {}", e),
             DeviceJail(e) => write!(f, "failed to jail device: {}", e),
             DevicePivotRoot(e) => write!(f, "failed to pivot root device: {}", e),
@@ -250,6 +255,7 @@ impl Display for Error {
             GuestCachedTooLarge(e) => write!(f, "guest cached is too large: {}", e),
             GuestFreeMissing() => write!(f, "guest free is missing from balloon stats"),
             GuestFreeTooLarge(e) => write!(f, "guest free is too large: {}", e),
+            GuestMemoryLayout(e) => write!(f, "failed to create guest memory layout: {}", e),
             #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
             HandleDebugCommand(e) => write!(f, "failed to handle a gdb command: {}", e),
             InputDeviceNew(e) => write!(f, "failed to set up input device: {}", e),
@@ -2259,12 +2265,6 @@ fn file_to_i64<P: AsRef<Path>>(path: P, nth: usize) -> io::Result<i64> {
         .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "empty file"))
 }
 
-fn create_kvm(device_path: &Path, mem: GuestMemory) -> base::Result<KvmVm> {
-    let kvm = Kvm::new_with_path(device_path)?;
-    let vm = KvmVm::new(&kvm, mem)?;
-    Ok(vm)
-}
-
 fn create_kvm_kernel_irq_chip(
     vm: &KvmVm,
     vcpu_count: usize,
@@ -2286,8 +2286,14 @@ fn create_kvm_split_irq_chip(
 }
 
 pub fn run_config(cfg: Config) -> Result<()> {
-    let kvm_device_path = cfg.kvm_device_path.clone();
-    let create_kvm_with_path = |mem| create_kvm(&kvm_device_path, mem);
+    let components = setup_vm_components(&cfg)?;
+
+    let guest_mem_layout =
+        Arch::guest_memory_layout(&components).map_err(Error::GuestMemoryLayout)?;
+    let guest_mem = GuestMemory::new(&guest_mem_layout).unwrap();
+    let kvm = Kvm::new_with_path(&cfg.kvm_device_path).map_err(Error::CreateKvm)?;
+    let vm = KvmVm::new(&kvm, guest_mem).map_err(Error::CreateVm)?;
+
     if cfg.split_irqchip {
         #[cfg(any(target_arch = "arm", target_arch = "aarch64"))]
         {
@@ -2296,39 +2302,14 @@ pub fn run_config(cfg: Config) -> Result<()> {
 
         #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
         {
-            run_vm::<_, KvmVcpu, _, _, _>(cfg, create_kvm_with_path, create_kvm_split_irq_chip)
+            run_vm::<KvmVcpu, _, _, _>(cfg, components, vm, create_kvm_split_irq_chip)
         }
     } else {
-        run_vm::<_, KvmVcpu, _, _, _>(cfg, create_kvm_with_path, create_kvm_kernel_irq_chip)
+        run_vm::<KvmVcpu, _, _, _>(cfg, components, vm, create_kvm_kernel_irq_chip)
     }
 }
 
-fn run_vm<V, Vcpu, I, FV, FI>(cfg: Config, create_vm: FV, create_irq_chip: FI) -> Result<()>
-where
-    V: VmArch + 'static,
-    Vcpu: VcpuArch + 'static,
-    I: IrqChipArch + 'static,
-    FV: FnOnce(GuestMemory) -> base::Result<V>,
-    FI: FnOnce(
-        &V,
-        usize, // vcpu_count
-        Tube,  // ioapic_device_tube
-    ) -> base::Result<I>,
-{
-    if cfg.sandbox {
-        // Printing something to the syslog before entering minijail so that libc's syslogger has a
-        // chance to open files necessary for its operation, like `/etc/localtime`. After jailing,
-        // access to those files will not be possible.
-        info!("crosvm entering multiprocess mode");
-    }
-
-    let (usb_control_tube, usb_provider) =
-        HostBackendDeviceProvider::new().map_err(Error::CreateUsbProvider)?;
-    // Masking signals is inherently dangerous, since this can persist across clones/execs. Do this
-    // before any jailed devices have been spawned, so that we can catch any of them that fail very
-    // quickly.
-    let sigchld_fd = SignalFd::new(libc::SIGCHLD).map_err(Error::CreateSignalFd)?;
-
+fn setup_vm_components(cfg: &Config) -> Result<VmComponents> {
     let initrd_image = if let Some(initrd_path) = &cfg.initrd_path {
         Some(File::open(initrd_path).map_err(|e| Error::OpenInitrd(initrd_path.clone(), e))?)
     } else {
@@ -2345,18 +2326,7 @@ where
         _ => panic!("Did not receive a bios or kernel, should be impossible."),
     };
 
-    let mut control_tubes = Vec::new();
-    #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
-    let gdb_socket = if let Some(port) = cfg.gdb {
-        // GDB needs a control socket to interrupt vcpus.
-        let (gdb_host_tube, gdb_control_tube) = Tube::pair().map_err(Error::CreateTube)?;
-        control_tubes.push(TaggedControlTube::Vm(gdb_host_tube));
-        Some((port, gdb_control_tube))
-    } else {
-        None
-    };
-
-    let components = VmComponents {
+    Ok(VmComponents {
         memory_size: cfg
             .memory
             .unwrap_or(256)
@@ -2383,9 +2353,40 @@ where
         rt_cpus: cfg.rt_cpus.clone(),
         protected_vm: cfg.protected_vm,
         #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
-        gdb: gdb_socket,
+        gdb: None,
         dmi_path: cfg.dmi_path.clone(),
-    };
+    })
+}
+
+fn run_vm<Vcpu, V, I, FI>(
+    cfg: Config,
+    #[allow(unused_mut)] mut components: VmComponents,
+    vm: V,
+    create_irq_chip: FI,
+) -> Result<()>
+where
+    Vcpu: VcpuArch + 'static,
+    V: VmArch + 'static,
+    I: IrqChipArch + 'static,
+    FI: FnOnce(
+        &V,
+        usize, // vcpu_count
+        Tube,  // ioapic_device_tube
+    ) -> base::Result<I>,
+{
+    if cfg.sandbox {
+        // Printing something to the syslog before entering minijail so that libc's syslogger has a
+        // chance to open files necessary for its operation, like `/etc/localtime`. After jailing,
+        // access to those files will not be possible.
+        info!("crosvm entering multiprocess mode");
+    }
+
+    let (usb_control_tube, usb_provider) =
+        HostBackendDeviceProvider::new().map_err(Error::CreateUsbProvider)?;
+    // Masking signals is inherently dangerous, since this can persist across clones/execs. Do this
+    // before any jailed devices have been spawned, so that we can catch any of them that fail very
+    // quickly.
+    let sigchld_fd = SignalFd::new(libc::SIGCHLD).map_err(Error::CreateSignalFd)?;
 
     let control_server_socket = match &cfg.socket_path {
         Some(path) => Some(UnlinkUnixSeqpacketListener(
@@ -2394,6 +2395,16 @@ where
         None => None,
     };
 
+    let mut control_tubes = Vec::new();
+
+    #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
+    if let Some(port) = cfg.gdb {
+        // GDB needs a control socket to interrupt vcpus.
+        let (gdb_host_tube, gdb_control_tube) = Tube::pair().map_err(Error::CreateTube)?;
+        control_tubes.push(TaggedControlTube::Vm(gdb_host_tube));
+        components.gdb = Some((port, gdb_control_tube));
+    }
+
     let (wayland_host_tube, wayland_device_tube) = Tube::pair().map_err(Error::CreateTube)?;
     control_tubes.push(TaggedControlTube::VmMemory(wayland_host_tube));
     // Balloon gets a special socket so balloon requests can be forwarded from the main process.
@@ -2473,6 +2484,7 @@ where
         &cfg.serial_parameters,
         simple_jail(&cfg, "serial")?,
         battery,
+        vm,
         |mem, vm, sys_allocator, exit_evt| {
             create_devices(
                 &cfg,
@@ -2491,7 +2503,6 @@ where
                 Arc::clone(&map_request),
             )
         },
-        create_vm,
         |vm, vcpu_count| create_irq_chip(vm, vcpu_count, ioapic_device_tube),
     )
     .map_err(Error::BuildVm)?;

x86_64/src/lib.rs

@@ -349,13 +349,23 @@ fn arch_memory_regions(size: u64, bios_size: Option<u64>) -> Vec<(GuestAddress,
 impl arch::LinuxArch for X8664arch {
     type Error = Error;
 
-    fn build_vm<V, Vcpu, I, FD, FV, FI, E1, E2, E3>(
+    fn guest_memory_layout(
+        components: &VmComponents,
+    ) -> std::result::Result<Vec<(GuestAddress, u64)>, Self::Error> {
+        let bios_size = match &components.vm_image {
+            VmImage::Bios(bios_file) => Some(bios_file.metadata().map_err(Error::LoadBios)?.len()),
+            VmImage::Kernel(_) => None,
+        };
+        Ok(arch_memory_regions(components.memory_size, bios_size))
+    }
+
+    fn build_vm<V, Vcpu, I, FD, FI, E1, E2>(
         mut components: VmComponents,
         serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>,
         serial_jail: Option<Minijail>,
         battery: (&Option<BatteryType>, Option<Minijail>),
+        mut vm: V,
         create_devices: FD,
-        create_vm: FV,
         create_irq_chip: FI,
     ) -> std::result::Result<RunnableLinuxVm<V, Vcpu, I>, Self::Error>
     where
@@ -368,28 +378,18 @@ impl arch::LinuxArch for X8664arch {
             &mut SystemAllocator,
             &Event,
         ) -> std::result::Result<Vec<(Box<dyn PciDevice>, Option<Minijail>)>, E1>,
-        FV: FnOnce(GuestMemory) -> std::result::Result<V, E2>,
-        FI: FnOnce(&V, /* vcpu_count: */ usize) -> std::result::Result<I, E3>,
+        FI: FnOnce(&V, /* vcpu_count: */ usize) -> std::result::Result<I, E2>,
         E1: StdError + 'static,
         E2: StdError + 'static,
-        E3: StdError + 'static,
     {
         if components.protected_vm != ProtectionType::Unprotected {
             return Err(Error::UnsupportedProtectionType);
         }
 
-        let bios_size = match components.vm_image {
-            VmImage::Bios(ref mut bios_file) => {
-                Some(bios_file.metadata().map_err(Error::LoadBios)?.len())
-            }
-            VmImage::Kernel(_) => None,
-        };
-        let has_bios = bios_size.is_some();
-        let mem = Self::setup_memory(components.memory_size, bios_size)?;
+        let mem = vm.get_memory().clone();
         let mut resources = Self::get_resource_allocator(&mem);
 
         let vcpu_count = components.vcpu_count;
-        let mut vm = create_vm(mem.clone()).map_err(|e| Error::CreateVm(Box::new(e)))?;
         let mut irq_chip =
             create_irq_chip(&vm, vcpu_count).map_err(|e| Error::CreateIrqChip(Box::new(e)))?;
 
@@ -524,7 +524,7 @@ impl arch::LinuxArch for X8664arch {
             vcpu_affinity: components.vcpu_affinity,
             no_smt: components.no_smt,
             irq_chip,
-            has_bios,
+            has_bios: matches!(components.vm_image, VmImage::Bios(_)),
             io_bus,
             mmio_bus,
             pid_debug_label_map,
@@ -930,15 +930,6 @@ impl X8664arch {
         Ok(())
     }
 
-    /// This creates a GuestMemory object for this VM
-    ///
-    /// * `mem_size` - Desired physical memory size in bytes for this VM
-    fn setup_memory(mem_size: u64, bios_size: Option<u64>) -> Result<GuestMemory> {
-        let arch_mem_regions = arch_memory_regions(mem_size, bios_size);
-        let mem = GuestMemory::new(&arch_mem_regions).map_err(Error::SetupGuestMemory)?;
-        Ok(mem)
-    }
-
     /// This returns the start address of high mmio
     ///
     /// # Arguments

x86_64/src/test_integration.rs

@@ -12,7 +12,7 @@ use super::cpuid::setup_cpuid;
 use super::interrupts::set_lint;
 use super::regs::{setup_fpu, setup_msrs, setup_regs, setup_sregs};
 use super::X8664arch;
-use super::{acpi, bootparam, mptable, smbios};
+use super::{acpi, arch_memory_regions, bootparam, mptable, smbios};
 use super::{
     BOOT_STACK_POINTER, END_ADDR_BEFORE_32BITS, KERNEL_64BIT_ENTRY_OFFSET, KERNEL_START_OFFSET,
     X86_64_SCI_IRQ, ZERO_PAGE_OFFSET,
@@ -95,8 +95,9 @@ where
     let write_addr = GuestAddress(0x4000);
 
     // guest mem is 400 pages
-    let guest_mem = X8664arch::setup_memory(memory_size, None).unwrap();
-    // let guest_mem = GuestMemory::new(&[(GuestAddress(0), memory_size)]).unwrap();
+    let arch_mem_regions = arch_memory_regions(memory_size, None);
+    let guest_mem = GuestMemory::new(&arch_mem_regions).unwrap();
+
     let mut resources = X8664arch::get_resource_allocator(&guest_mem);
 
     let (hyp, mut vm) = create_vm(guest_mem.clone());