Merge branch 'firecracker-microvm:main' into main

Author: xxgj

.buildkite/pipeline_perf.py

@@ -108,6 +108,7 @@
 REVISION_B = os.environ.get("REVISION_B")
 REVISION_A_ARTIFACTS = os.environ.get("REVISION_A_ARTIFACTS")
 REVISION_B_ARTIFACTS = os.environ.get("REVISION_B_ARTIFACTS")
+A_B_TEST_MAX_ITERATIONS = 4
 
 # Either both are specified or neither. Only doing either is a bug. If you want to
 # run performance tests _on_ a specific commit, specify neither and put your commit
@@ -125,21 +126,11 @@
     action="append",
 )
 
-retry = {}
-if REVISION_A:
-    # Enable automatic retry and disable manual retries to suppress spurious issues.
-    retry["automatic"] = [
-        {"exit_status": -1, "limit": 1},
-        {"exit_status": 1, "limit": 1},
-    ]
-    retry["manual"] = False
-
 pipeline = BKPipeline(
     # Boost priority from 1 to 2 so these jobs are preferred by ag=1 agents
     priority=2,
     # use ag=1 instances to make sure no two performance tests are scheduled on the same instance
     agents={"ag": 1},
-    retry=retry,
     # Heavy post-failure dumps (full snapshot + chroot copy) are useful
     # for triaging performance flakes that are hard to reproduce locally.
     # Cheap dumps are always on; this flag turns the heavy block on.
@@ -163,7 +154,7 @@
     artifacts = []
     if REVISION_A:
         devtool_opts += " --ab"
-        test_script_opts = f'{ab_opts} run --binaries-a build/{REVISION_A}/ --binaries-b build/{REVISION_B} --pytest-opts "{test_selector}"'
+        test_script_opts = f'{ab_opts} run --binaries-a build/{REVISION_A}/ --binaries-b build/{REVISION_B} --max-iterations={A_B_TEST_MAX_ITERATIONS} --pytest-opts "{test_selector}"'
         if REVISION_A_ARTIFACTS:
             artifacts.append(REVISION_A_ARTIFACTS)
             artifacts.append(REVISION_B_ARTIFACTS)

src/vmm/src/devices/virtio/mem/device.rs

@@ -539,6 +539,10 @@ impl VirtioMem {
         self.config.plugged_size -= usize_to_u64(self.nb_blocks_to_len(plugged_before));
         self.config.plugged_size += usize_to_u64(self.nb_blocks_to_len(plugged_after));
 
+        // Update kvm slots before doing any discards to prevent guest from re-faulting just
+        // discarded memory.
+        self.update_kvm_slots(range)?;
+
         // If unplugging, discard the range
         if !plug {
             self.guest_memory()
@@ -550,8 +554,7 @@ impl VirtioMem {
                     error!("virtio-mem: Failed to discard memory range: {}", err);
                 });
         }
-
-        self.update_kvm_slots(range)
+        Ok(())
     }
 
     /// Updates the requested size of the virtio-mem device.

src/vmm/src/devices/virtio/net/device.rs

@@ -39,7 +39,7 @@ use crate::devices::{DeviceError, report_net_event_fail};
 use crate::dumbo::pdu::arp::ETH_IPV4_FRAME_LEN;
 use crate::dumbo::pdu::ethernet::{EthernetFrame, PAYLOAD_OFFSET};
 use crate::impl_device_type;
-use crate::logger::{IncMetric, METRICS, error};
+use crate::logger::{IncMetric, METRICS, error, warn};
 use crate::mmds::data_store::Mmds;
 use crate::mmds::ns::MmdsNetworkStack;
 use crate::rate_limiter::{BucketUpdate, RateLimiter, TokenType};
@@ -1032,24 +1032,12 @@ impl VirtioDevice for Net {
     }
 
     fn write_config(&mut self, offset: u64, data: &[u8]) {
-        let config_space_bytes = self.config_space.as_mut_slice();
-        let start = usize::try_from(offset).ok();
-        let end = start.and_then(|s| s.checked_add(data.len()));
-        // Only the guest_mac field (bytes 0..size_of::<MacAddr>()) is writable by the guest.
-        // All other fields (status, max_virtqueue_pairs, mtu) are read-only device fields.
-        let Some(dst) = start
-            .zip(end)
-            .filter(|&(_, end)| end <= std::mem::size_of::<MacAddr>())
-            .and_then(|(start, end)| config_space_bytes.get_mut(start..end))
-        else {
-            error!("Failed to write config space");
-            self.metrics.cfg_fails.inc();
-            return;
-        };
-
-        dst.copy_from_slice(data);
-        self.guest_mac = Some(self.config_space.guest_mac);
-        self.metrics.mac_address_updates.inc();
+        self.metrics.cfg_fails.inc();
+        warn!(
+            "virtio-net: guest driver attempted to write device config (offset={:#x}, len={:#x})",
+            offset,
+            data.len()
+        );
     }
 
     fn activate(
@@ -1295,42 +1283,52 @@ pub mod tests {
     }
 
     #[test]
-    fn test_virtio_device_rewrite_config() {
+    fn test_virtio_device_config_space_is_read_only() {
         let mut net = default_net();
-        set_mac(&mut net, MacAddr::from_str("11:22:33:44:55:66").unwrap());
-
-        let new_config: [u8; MAC_ADDR_LEN as usize] = [0x66, 0x55, 0x44, 0x33, 0x22, 0x11];
-        net.write_config(0, &new_config);
-        let mut new_config_read = [0u8; MAC_ADDR_LEN as usize];
-        net.read_config(0, &mut new_config_read);
-        assert_eq!(new_config, new_config_read);
-
-        // Check that the guest MAC was updated.
-        let expected_guest_mac = MacAddr::from_bytes_unchecked(&new_config);
-        assert_eq!(expected_guest_mac, net.guest_mac.unwrap());
-        assert_eq!(net.metrics.mac_address_updates.count(), 1);
-
-        // Partial write (this is how the kernel sets a new mac address) - byte by byte.
-        let new_config = [0x11, 0x22, 0x33, 0x44, 0x55, 0x66];
-        for i in 0..new_config.len() {
-            net.write_config(i as u64, &new_config[i..=i]);
+        let initial_mac = MacAddr::from_str("11:22:33:44:55:66").unwrap();
+        set_mac(&mut net, initial_mac);
+
+        let initial_bytes: [u8; MAC_ADDR_LEN as usize] = [0x11, 0x22, 0x33, 0x44, 0x55, 0x66];
+
+        // Sanity check: the configured MAC is what the guest reads back.
+        let mut config_read = [0u8; MAC_ADDR_LEN as usize];
+        net.read_config(0, &mut config_read);
+        assert_eq!(config_read, initial_bytes);
+        assert_eq!(net.guest_mac.unwrap(), initial_mac);
+
+        // A 6-byte write to offset 0 must be rejected and leave both the
+        // config space bytes and the device's stored guest_mac unchanged.
+        let attempted_bytes: [u8; MAC_ADDR_LEN as usize] = [0x66, 0x55, 0x44, 0x33, 0x22, 0x11];
+        let cfg_fails_before = net.metrics.cfg_fails.count();
+        net.write_config(0, &attempted_bytes);
+        net.read_config(0, &mut config_read);
+        assert_eq!(config_read, initial_bytes);
+        assert_eq!(net.guest_mac.unwrap(), initial_mac);
+        assert_eq!(net.metrics.cfg_fails.count(), cfg_fails_before + 1);
+
+        // Single-byte writes covering the MAC region must also be rejected
+        // without changing state.
+        let cfg_fails_before = net.metrics.cfg_fails.count();
+        for i in 0..attempted_bytes.len() {
+            net.write_config(i as u64, &attempted_bytes[i..=i]);
         }
-        net.read_config(0, &mut new_config_read);
-        assert_eq!(new_config, new_config_read);
-
-        // Invalid write.
-        net.write_config(5, &new_config);
-        // Verify old config was untouched.
-        new_config_read = [0u8; MAC_ADDR_LEN as usize];
-        net.read_config(0, &mut new_config_read);
-        assert_eq!(new_config, new_config_read);
-
-        // Large offset that may cause an overflow.
-        net.write_config(u64::MAX, &new_config);
-        // Verify old config was untouched.
-        new_config_read = [0u8; MAC_ADDR_LEN as usize];
-        net.read_config(0, &mut new_config_read);
-        assert_eq!(new_config, new_config_read);
+        net.read_config(0, &mut config_read);
+        assert_eq!(config_read, initial_bytes);
+        assert_eq!(net.guest_mac.unwrap(), initial_mac);
+        assert_eq!(
+            net.metrics.cfg_fails.count(),
+            cfg_fails_before + attempted_bytes.len() as u64
+        );
+
+        // Out-of-range and overflowing offsets must be rejected without
+        // changing state.
+        let cfg_fails_before = net.metrics.cfg_fails.count();
+        net.write_config(5, &attempted_bytes);
+        net.write_config(u64::MAX, &attempted_bytes);
+        net.read_config(0, &mut config_read);
+        assert_eq!(config_read, initial_bytes);
+        assert_eq!(net.guest_mac.unwrap(), initial_mac);
+        assert_eq!(net.metrics.cfg_fails.count(), cfg_fails_before + 2);
     }
 
     #[test]

src/vmm/src/devices/virtio/net/metrics.rs

@@ -12,15 +12,13 @@
 //!  "net_eth0": {
 //!     "activate_fails": "SharedIncMetric",
 //!     "cfg_fails": "SharedIncMetric",
-//!     "mac_address_updates": "SharedIncMetric",
 //!     "no_rx_avail_buffer": "SharedIncMetric",
 //!     "no_tx_avail_buffer": "SharedIncMetric",
 //!     ...
 //!  }
 //!  "net_eth1": {
 //!     "activate_fails": "SharedIncMetric",
 //!     "cfg_fails": "SharedIncMetric",
-//!     "mac_address_updates": "SharedIncMetric",
 //!     "no_rx_avail_buffer": "SharedIncMetric",
 //!     "no_tx_avail_buffer": "SharedIncMetric",
 //!     ...
@@ -29,15 +27,13 @@
 //!  "net_iface_id": {
 //!     "activate_fails": "SharedIncMetric",
 //!     "cfg_fails": "SharedIncMetric",
-//!     "mac_address_updates": "SharedIncMetric",
 //!     "no_rx_avail_buffer": "SharedIncMetric",
 //!     "no_tx_avail_buffer": "SharedIncMetric",
 //!     ...
 //!  }
 //!  "net": {
 //!     "activate_fails": "SharedIncMetric",
 //!     "cfg_fails": "SharedIncMetric",
-//!     "mac_address_updates": "SharedIncMetric",
 //!     "no_rx_avail_buffer": "SharedIncMetric",
 //!     "no_tx_avail_buffer": "SharedIncMetric",
 //!     ...
@@ -149,8 +145,6 @@ pub struct NetDeviceMetrics {
     pub activate_fails: SharedIncMetric,
     /// Number of times when interacting with the space config of a network device failed.
     pub cfg_fails: SharedIncMetric,
-    /// Number of times the mac address was updated through the config space.
-    pub mac_address_updates: SharedIncMetric,
     /// No available buffer for the net device rx queue.
     pub no_rx_avail_buffer: SharedIncMetric,
     /// No available buffer for the net device tx queue.
@@ -218,8 +212,6 @@ impl NetDeviceMetrics {
     pub fn aggregate(&mut self, other: &Self) {
         self.activate_fails.add(other.activate_fails.fetch_diff());
         self.cfg_fails.add(other.cfg_fails.fetch_diff());
-        self.mac_address_updates
-            .add(other.mac_address_updates.fetch_diff());
         self.no_rx_avail_buffer
             .add(other.no_rx_avail_buffer.fetch_diff());
         self.no_tx_avail_buffer

src/vmm/src/gdb/target.rs

@@ -1,6 +1,7 @@
 // Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+use std::cfg_select;
 use std::collections::HashMap;
 use std::sync::mpsc::{Receiver, RecvError};
 use std::sync::{Arc, Mutex, PoisonError};
@@ -19,14 +20,16 @@ use gdbstub::target::ext::breakpoints::{
 };
 use gdbstub::target::ext::thread_extra_info::{ThreadExtraInfo, ThreadExtraInfoOps};
 use gdbstub::target::{Target, TargetError, TargetResult};
-#[cfg(target_arch = "aarch64")]
-use gdbstub_arch::aarch64::AArch64 as GdbArch;
-#[cfg(target_arch = "aarch64")]
-use gdbstub_arch::aarch64::reg::AArch64CoreRegs as CoreRegs;
-#[cfg(target_arch = "x86_64")]
-use gdbstub_arch::x86::X86_64_SSE as GdbArch;
-#[cfg(target_arch = "x86_64")]
-use gdbstub_arch::x86::reg::X86_64CoreRegs as CoreRegs;
+cfg_select! {
+    target_arch = "aarch64" => {
+        use gdbstub_arch::aarch64::AArch64 as GdbArch;
+        use gdbstub_arch::aarch64::reg::AArch64CoreRegs as CoreRegs;
+    }
+    target_arch = "x86_64" => {
+        use gdbstub_arch::x86::X86_64_SSE as GdbArch;
+        use gdbstub_arch::x86::reg::X86_64CoreRegs as CoreRegs;
+    }
+}
 use vm_memory::{Bytes, GuestAddress, GuestMemoryError};
 
 use super::arch;

tests/README.md

@@ -242,7 +242,7 @@ compare boottime of microVMs between Firecracker binaries compiled from the
 ```sh
 tools/devtool -y build --rev main --release
 tools/devtool -y build --rev HEAD --release
-tools/devtool -y test --no-build --ab -- run build/main build/HEAD --pytest-opts integration_tests/performance/test_boottime.py::test_boottime
+tools/devtool -y test --no-build --ab -- run --binaries-a build/main --binaries-b build/HEAD --pytest-opts integration_tests/performance/test_boottime.py::test_boottime
 ```
 
 To download custom artifacts use

tests/conftest.py

@@ -287,20 +287,6 @@ def bin_vmclock_path(test_fc_session_root_path):
     yield vmclock_helper_bin_path
 
 
-@pytest.fixture(scope="session")
-def change_net_config_space_bin(test_fc_session_root_path):
-    """Build a binary that changes the MMIO config space."""
-    change_net_config_space_bin = os.path.join(
-        test_fc_session_root_path, "change_net_config_space"
-    )
-    build_tools.gcc_compile(
-        "host_tools/change_net_config_space.c",
-        change_net_config_space_bin,
-        extra_flags="-static",
-    )
-    yield change_net_config_space_bin
-
-
 @pytest.fixture(scope="session")
 def devmem_bin(test_fc_session_root_path):
     """Build a minimal /dev/mem read/write tool."""

tests/host_tools/change_net_config_space.c

@@ -102,7 +102,6 @@ def validate_fc_metrics(metrics):
     net_metrics = [
         "activate_fails",
         "cfg_fails",
-        "mac_address_updates",
         "no_rx_avail_buffer",
         "no_tx_avail_buffer",
         "event_fails",