diff --git a/CHANGELOG.md b/CHANGELOG.md index c101dd00481..ba96defe78e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ replaced by the `--` separator for extra arguments. - Changed the output of the `--version` command line parameter to include a list of supported snapshot data format versions for the firecracker binary. +- Increased the maximum number of virtio devices from 11 to 19. +- Added a new check that prevents creating v0.23 snapshots when more than 11 + devices are attached. ### Fixed diff --git a/src/arch/src/x86_64/layout.rs b/src/arch/src/x86_64/layout.rs index b4c3f149428..ecfa073facb 100644 --- a/src/arch/src/x86_64/layout.rs +++ b/src/arch/src/x86_64/layout.rs @@ -18,11 +18,11 @@ pub const CMDLINE_MAX_SIZE: usize = 0x10000; /// Start of the high memory. pub const HIMEM_START: u64 = 0x0010_0000; //1 MB. -// Typically, on x86 systems 16 IRQs are used (0-15). +// Typically, on x86 systems 24 IRQs are used (0-23). /// First usable IRQ ID for virtio device interrupts on x86_64. pub const IRQ_BASE: u32 = 5; /// Last usable IRQ ID for virtio device interrupts on x86_64. -pub const IRQ_MAX: u32 = 15; +pub const IRQ_MAX: u32 = 23; /// Address for the TSS setup. pub const KVM_TSS_ADDRESS: u64 = 0xfffb_d000; diff --git a/src/arch/src/x86_64/mptable.rs b/src/arch/src/x86_64/mptable.rs index d8cf300a327..a603bfb6c02 100644 --- a/src/arch/src/x86_64/mptable.rs +++ b/src/arch/src/x86_64/mptable.rs @@ -5,6 +5,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the THIRD-PARTY file. +use std::convert::TryFrom; use std::io; use std::mem; use std::result; @@ -15,6 +16,8 @@ use libc::c_char; use arch_gen::x86::mpspec; use vm_memory::{Address, ByteValued, Bytes, GuestAddress, GuestMemory, GuestMemoryMmap}; +use crate::IRQ_MAX; + // This is a workaround to the Rust enforcement specifying that any implementation of a foreign // trait (in this case `ByteValued`) where: // * the type that is implementing the trait is foreign or @@ -57,6 +60,8 @@ pub enum Error { Clear, /// Number of CPUs exceeds the maximum supported CPUs TooManyCpus, + /// Number of IRQs exceeds the maximum supported IRQs + TooManyIrqs, /// Failure to write the MP floating pointer. WriteMpfIntel, /// Failure to write MP CPU entry. @@ -120,7 +125,7 @@ fn compute_mp_size(num_cpus: u8) -> usize { + mem::size_of::() * (num_cpus as usize) + mem::size_of::() + mem::size_of::() - + mem::size_of::() * 16 + + mem::size_of::() * (IRQ_MAX as usize + 1) + mem::size_of::() * 2 } @@ -215,7 +220,7 @@ pub fn setup_mptable(mem: &GuestMemoryMmap, num_cpus: u8) -> Result<()> { checksum = checksum.wrapping_add(compute_checksum(&mpc_ioapic.0)); } // Per kvm_setup_default_irq_routing() in kernel - for i in 0..16 { + for i in 0..=u8::try_from(IRQ_MAX).map_err(|_| Error::TooManyIrqs)? { let size = mem::size_of::() as u64; let mut mpc_intsrc = MpcIntsrcWrapper(mpspec::mpc_intsrc::default()); mpc_intsrc.0.type_ = mpspec::MP_INTSRC as u8; diff --git a/src/vmm/src/device_manager/mmio.rs b/src/vmm/src/device_manager/mmio.rs index eedbc01117f..5e6358e4544 100644 --- a/src/vmm/src/device_manager/mmio.rs +++ b/src/vmm/src/device_manager/mmio.rs @@ -314,6 +314,17 @@ impl MMIODeviceManager { &self.id_to_dev_info } + #[cfg(target_arch = "x86_64")] + /// Gets the number of interrupts used by the devices registered. + pub fn used_irqs_count(&self) -> usize { + let mut irq_number = 0; + let _: Result<()> = self.for_each_device(|_, _, device_info, _| { + irq_number += device_info.irqs.len(); + Ok(()) + }); + irq_number + } + /// Gets the the specified device. pub fn get_device( &self, @@ -723,6 +734,7 @@ mod tests { Ok(()) }); assert_eq!(count, 3); + assert_eq!(device_manager.used_irqs_count(), 2); } } diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs index c3d1e3e4fea..d6d91fdeb4b 100644 --- a/src/vmm/src/persist.rs +++ b/src/vmm/src/persist.rs @@ -18,11 +18,13 @@ use crate::mem_size_mib; use crate::vmm_config::snapshot::{CreateSnapshotParams, LoadSnapshotParams, SnapshotType}; use crate::vstate::{self, vcpu::VcpuState, vm::VmState}; +use crate::device_manager::mmio::MMIODeviceManager; use crate::device_manager::persist::DeviceStates; use crate::memory_snapshot; use crate::memory_snapshot::{GuestMemoryState, SnapshotMemory}; use crate::version_map::FC_VERSION_TO_SNAP_VERSION; use crate::Vmm; +use arch::IRQ_BASE; use cpuid::common::{get_vendor_id_from_cpuid, get_vendor_id_from_host}; use logger::{error, info}; use polly::event_manager::EventManager; @@ -32,6 +34,10 @@ use versionize::{VersionMap, Versionize, VersionizeResult}; use versionize_derive::Versionize; use vm_memory::GuestMemoryMmap; +const FC_V0_23_SNAP_VERSION: u16 = 1; +const FC_V0_23_IRQ_NUMBER: u32 = 16; +const FC_V0_23_MAX_DEVICES: u32 = FC_V0_23_IRQ_NUMBER - IRQ_BASE; + /// Holds information related to the VM that is not part of VmState. #[derive(Debug, PartialEq, Versionize)] // NOTICE: Any changes to this structure require a snapshot version bump. @@ -115,6 +121,8 @@ pub enum CreateSnapshotError { SerializeMicrovmState(snapshot::Error), /// Failed to open the snapshot backing file. SnapshotBackingFile(io::Error), + /// Number of devices exceeds the maximum supported devices for the snapshot data version. + TooManyDevices(usize), } impl Display for CreateSnapshotError { @@ -132,6 +140,12 @@ impl Display for CreateSnapshotError { MicrovmState(err) => write!(f, "Cannot save microvm state: {}", err), SerializeMicrovmState(err) => write!(f, "Cannot serialize MicrovmState: {:?}", err), SnapshotBackingFile(err) => write!(f, "Cannot open snapshot file: {:?}", err), + TooManyDevices(val) => write!( + f, + "Too many devices attached: {}. The maximum number allowed \ + for the snapshot data version requested is {}.", + val, FC_V0_23_MAX_DEVICES + ), } } } @@ -187,6 +201,7 @@ pub fn create_snapshot( ¶ms.snapshot_path, ¶ms.version, version_map, + &vmm.mmio_device_manager, )?; Ok(()) @@ -197,6 +212,7 @@ fn snapshot_state_to_file( snapshot_path: &PathBuf, version: &Option, version_map: VersionMap, + device_manager: &MMIODeviceManager, ) -> std::result::Result<(), CreateSnapshotError> { use self::CreateSnapshotError::*; let mut snapshot_file = OpenOptions::new() @@ -208,6 +224,10 @@ fn snapshot_state_to_file( // Translate the microVM version to its corresponding snapshot data format. let snapshot_data_version = match version { Some(version) => match FC_VERSION_TO_SNAP_VERSION.get(version) { + Some(&FC_V0_23_SNAP_VERSION) => { + validate_devices_number(device_manager.used_irqs_count())?; + Ok(FC_V0_23_SNAP_VERSION) + } Some(data_version) => Ok(*data_version), _ => Err(InvalidVersion), }, @@ -330,6 +350,14 @@ fn guest_memory_from_file( GuestMemoryMmap::restore(&mem_file, mem_state, track_dirty_pages).map_err(DeserializeMemory) } +fn validate_devices_number(device_number: usize) -> std::result::Result<(), CreateSnapshotError> { + use self::CreateSnapshotError::TooManyDevices; + if device_number > FC_V0_23_MAX_DEVICES as usize { + return Err(TooManyDevices(device_number)); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -462,6 +490,9 @@ mod tests { let err = SnapshotBackingFile(io::Error::from_raw_os_error(0)); let _ = format!("{}{:?}", err, err); + + let err = TooManyDevices(0); + let _ = format!("{}{:?}", err, err); } #[test] diff --git a/tests/framework/builder.py b/tests/framework/builder.py index c34d41905b5..bb13a515d65 100644 --- a/tests/framework/builder.py +++ b/tests/framework/builder.py @@ -179,6 +179,17 @@ def __init__(self, microvm): """Initialize the snapshot builder.""" self._microvm = microvm + def create_snapshot_dir(self): + """Create dir and files for saving snapshot state and memory.""" + chroot_path = self._microvm.jailer.chroot_path() + snapshot_dir = os.path.join(chroot_path, "snapshot") + Path(snapshot_dir).mkdir(parents=True, exist_ok=True) + cmd = 'chown {}:{} {}'.format(self._microvm.jailer.uid, + self._microvm.jailer.gid, + snapshot_dir) + utils.run_cmd(cmd) + return snapshot_dir + def create(self, disks, ssh_key: Artifact, @@ -190,13 +201,7 @@ def create(self, # Disable API timeout as the APIs for snapshot related procedures # take longer. self._microvm.api_session.untime() - chroot_path = self._microvm.jailer.chroot_path() - snapshot_dir = os.path.join(chroot_path, "snapshot") - Path(snapshot_dir).mkdir(parents=True, exist_ok=True) - cmd = 'chown {}:{} {}'.format(self._microvm.jailer.uid, - self._microvm.jailer.gid, - snapshot_dir) - utils.run_cmd(cmd) + snapshot_dir = self.create_snapshot_dir() self._microvm.pause_to_snapshot( mem_file_path="/snapshot/"+mem_file_name, snapshot_path="/snapshot/"+snapshot_name, diff --git a/tests/integration_tests/functional/test_max_devices.py b/tests/integration_tests/functional/test_max_devices.py new file mode 100644 index 00000000000..94465cbacbf --- /dev/null +++ b/tests/integration_tests/functional/test_max_devices.py @@ -0,0 +1,73 @@ +# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Tests scenario for adding the maximum number of devices to a microVM.""" + +import platform +import pytest +import host_tools.network as net_tools + +# IRQs are available from 5 to 23, so the maximum number of devices +# supported at the same time is 19. +MAX_DEVICES_ATTACHED = 19 + + +@pytest.mark.skipif( + platform.machine() != "x86_64", + reason="Firecracker supports 24 IRQs on x86_64." +) +def test_attach_maximum_devices(test_microvm_with_ssh, network_config): + """Test attaching maximum number of devices to the microVM.""" + test_microvm = test_microvm_with_ssh + test_microvm.spawn() + + # Set up a basic microVM. + test_microvm.basic_config() + + # Add (`MAX_DEVICES_ATTACHED` - 1) devices because the rootfs + # has already been configured in the `basic_config()`function. + guest_ips = [] + for i in range(MAX_DEVICES_ATTACHED - 1): + # Create tap before configuring interface. + _tap, _host_ip, guest_ip = test_microvm.ssh_network_config( + network_config, + str(i) + ) + guest_ips.append(guest_ip) + + test_microvm.start() + + # Test that network devices attached are operational. + for i in range(MAX_DEVICES_ATTACHED - 1): + test_microvm.ssh_config['hostname'] = guest_ips[i] + ssh_connection = net_tools.SSHConnection(test_microvm.ssh_config) + # Verify if guest can run commands. + exit_code, _, _ = ssh_connection.execute_command("sync") + assert exit_code == 0 + + +@pytest.mark.skipif( + platform.machine() != "x86_64", + reason="Firecracker supports 24 IRQs on x86_64." +) +def test_attach_too_many_devices(test_microvm_with_ssh, network_config): + """Test attaching to a microVM more devices than available IRQs.""" + test_microvm = test_microvm_with_ssh + test_microvm.spawn() + + # Set up a basic microVM. + test_microvm.basic_config() + + # Add `MAX_DEVICES_ATTACHED` network devices on top of the + # already configured rootfs. + for i in range(MAX_DEVICES_ATTACHED): + # Create tap before configuring interface. + _tap, _host_ip, _guest_ip = test_microvm.ssh_network_config( + network_config, + str(i) + ) + + # Attempting to start a microVM with more than + # `MAX_DEVICES_ATTACHED` devices should fail. + response = test_microvm.actions.put(action_type='InstanceStart') + assert test_microvm.api_session.is_status_bad_request(response.status_code) + assert "no more IRQs are available" in response.text diff --git a/tests/integration_tests/functional/test_snapshot_version.py b/tests/integration_tests/functional/test_snapshot_version.py index 4a8e73f1202..675c4f5d0a3 100644 --- a/tests/integration_tests/functional/test_snapshot_version.py +++ b/tests/integration_tests/functional/test_snapshot_version.py @@ -11,6 +11,35 @@ from framework.microvms import VMMicro import host_tools.network as net_tools # pylint: disable=import-error +# Firecracker v0.23 used 16 IRQ lines. For virtio devices, +# IRQs are available from 5 to 23, so the maximum number +# of devices allowed at the same time was 11. +FC_V0_23_MAX_DEVICES_ATTACHED = 11 + + +def _create_and_start_microvm_with_net_devices(test_microvm, + network_config, + devices_no): + test_microvm.spawn() + # Set up a basic microVM: configure the boot source and + # add a root device. + test_microvm.basic_config(track_dirty_pages=True) + + # Add network devices on top of the already configured rootfs for a + # total of (`devices_no` + 1) devices. + for i in range(devices_no): + # Create tap before configuring interface. + _tap, _host_ip, _guest_ip = test_microvm.ssh_network_config( + network_config, + str(i) + ) + test_microvm.start() + + ssh_connection = net_tools.SSHConnection(test_microvm.ssh_config) + # Verify if guest can run commands. + exit_code, _, _ = ssh_connection.execute_command("sync") + assert exit_code == 0 + @pytest.mark.skipif( platform.machine() != "x86_64", @@ -50,7 +79,7 @@ def test_restore_from_past_versions(bin_cloner_path): def create_512mb_full_snapshot(bin_cloner_path, target_version: str = None, fc_binary=None, jailer_binary=None): - """Create a snapshoft from a 2vcpu 512MB microvm.""" + """Test scenario: create a snapshot from a 2vcpu 512MB microvm.""" vm_instance = VMMicro.spawn(bin_cloner_path, True, fc_binary, jailer_binary) # Attempt to connect to the fresh microvm. @@ -123,3 +152,69 @@ def test_restore_in_past_versions(bin_cloner_path): exit_code, _, _ = ssh_connection.execute_command("sleep 1 && sync") assert exit_code == 0 + + +@pytest.mark.skipif( + platform.machine() != "x86_64", + reason="Not supported yet." +) +def test_create_with_past_version(test_microvm_with_ssh, network_config): + """Test scenario: restore in previous versions with too many devices.""" + test_microvm = test_microvm_with_ssh + + # Create and start a microVM with (`FC_V0_23_MAX_DEVICES_ATTACHED` - 1) + # network devices. + devices_no = FC_V0_23_MAX_DEVICES_ATTACHED - 1 + _create_and_start_microvm_with_net_devices(test_microvm, + network_config, + devices_no) + + snapshot_builder = SnapshotBuilder(test_microvm) + # Create directory and files for saving snapshot state and memory. + _snapshot_dir = snapshot_builder.create_snapshot_dir() + # Pause and create a snapshot of the microVM. Firecracker v0.23 allowed a + # maximum of `FC_V0_23_MAX_DEVICES_ATTACHED` virtio devices at a time. + # This microVM has `FC_V0_23_MAX_DEVICES_ATTACHED` devices, including the + # rootfs, so snapshotting should succeed. + test_microvm.pause_to_snapshot( + mem_file_path="/snapshot/vm.mem", + snapshot_path="/snapshot/vm.vmstate", + diff=True, + version="0.23.0") + + +@pytest.mark.skipif( + platform.machine() != "x86_64", + reason="Not supported yet." +) +def test_create_with_too_many_devices(test_microvm_with_ssh, network_config): + """Test scenario: restore in previous versions with too many devices.""" + test_microvm = test_microvm_with_ssh + + # Create and start a microVM with `FC_V0_23_MAX_DEVICES_ATTACHED` + # network devices. + devices_no = FC_V0_23_MAX_DEVICES_ATTACHED + _create_and_start_microvm_with_net_devices(test_microvm, + network_config, + devices_no) + + snapshot_builder = SnapshotBuilder(test_microvm) + # Create directory and files for saving snapshot state and memory. + _snapshot_dir = snapshot_builder.create_snapshot_dir() + + # Pause microVM for snapshot. + response = test_microvm.vm.patch(state='Paused') + assert test_microvm.api_session.is_status_no_content(response.status_code) + + # Attempt to create a snapshot with version: `0.23.0`. Firecracker + # v0.23 allowed a maximum of `FC_V0_23_MAX_DEVICES_ATTACHED` virtio + # devices at a time. This microVM has `FC_V0_23_MAX_DEVICES_ATTACHED` + # network devices on top of the rootfs, so the limit is exceeded. + response = test_microvm.snapshot_create.put( + mem_file_path="/snapshot/vm.vmstate", + snapshot_path="/snapshot/vm.mem", + diff=True, + version="0.23.0" + ) + assert test_microvm.api_session.is_status_bad_request(response.status_code) + assert "Too many devices attached" in response.text