From 279cf5c74eeafc313c3783604d1d1966544da9bf Mon Sep 17 00:00:00 2001 From: Nikita Kalyazin Date: Wed, 2 Oct 2024 11:20:36 +0000 Subject: [PATCH 1/3] fix(net): set tap offload features on restore Tap offload features configuration was moved from the device creation time to the device activation time by the following commit: commit 1e5d3dba6a92358e3bee3ccb429ef383c479c588 Author: Nikita Zakirov Date: Fri Jan 19 15:48:21 2024 +0000 fix(net): Apply only supported TAP offloading features Since device activation code is only called on the boot path, the features were not automatically configured on the restore path. This change configures them on the restore path as well. The change does not include a unit test as we do not have a mockable interface for the tap device. The change does not include an integration test as we have not yet found a way to reproduce the issue using the existing test framework. Signed-off-by: Nikita Kalyazin --- CHANGELOG.md | 12 ++++++++++++ src/vmm/src/devices/virtio/net/device.rs | 2 +- src/vmm/src/devices/virtio/net/persist.rs | 9 ++++++++- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 143412c50f5..e0ad7ca78fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## \[Unreleased\] + +### Fixed + +- [#4824](https://github.com/firecracker-microvm/firecracker/pull/4824): Add + missing configuration of tap offload features when restoring from a snapshot. + Setting the features was previously + [moved](https://github.com/firecracker-microvm/firecracker/pull/4680/commits/49ed5ea4b48ccd98903da037368fa3108f58ac1f) + from net device creation to device activation time, but it was not reflected + in the restore path. This was leading to inability to connect to the restored + VM if the offload features were used. + ## \[1.9.0\] ### Added diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index e34676b2c31..d6543794e02 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/src/vmm/src/devices/virtio/net/device.rs @@ -669,7 +669,7 @@ impl Net { /// Builds the offload features we will setup on the TAP device based on the features that the /// guest supports. - fn build_tap_offload_features(guest_supported_features: u64) -> u32 { + pub fn build_tap_offload_features(guest_supported_features: u64) -> u32 { let add_if_supported = |tap_features: &mut u32, supported_features: u64, tap_flag: u32, virtio_flag: u32| { if supported_features & (1 << virtio_flag) != 0 { diff --git a/src/vmm/src/devices/virtio/net/persist.rs b/src/vmm/src/devices/virtio/net/persist.rs index 271977a4792..eb482e29af8 100644 --- a/src/vmm/src/devices/virtio/net/persist.rs +++ b/src/vmm/src/devices/virtio/net/persist.rs @@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize}; use utils::net::mac::MacAddr; use super::device::Net; -use super::NET_NUM_QUEUES; +use super::{TapError, NET_NUM_QUEUES}; use crate::devices::virtio::device::DeviceState; use crate::devices::virtio::persist::{PersistError as VirtioStateError, VirtioDeviceState}; use crate::devices::virtio::queue::FIRECRACKER_MAX_QUEUE_SIZE; @@ -65,6 +65,8 @@ pub enum NetPersistError { VirtioState(#[from] VirtioStateError), /// Indicator that no MMDS is associated with this device. NoMmdsDataStore, + /// Setting tap interface offload flags failed: {0} + TapSetOffload(TapError), } impl Persist<'_> for Net { @@ -129,6 +131,11 @@ impl Persist<'_> for Net { net.acked_features = state.virtio_state.acked_features; if state.virtio_state.activated { + let supported_flags: u32 = Net::build_tap_offload_features(net.acked_features); + net.tap + .set_offload(supported_flags) + .map_err(NetPersistError::TapSetOffload)?; + net.device_state = DeviceState::Activated(constructor_args.mem); } From 860a82b69dee54590b77783c7f779f5af1ed7a5e Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 13 Sep 2024 16:48:09 +0100 Subject: [PATCH 2/3] test: move `test_binary_{size,static_linking}`.py to functional tests The idea here is that these tests depend on the compilation step, and thus test a production binary. This smells "integration" to me. The actual reason for moving this however is so that we can have the `build` group no longer depend on the shared compilation step. Signed-off-by: Patrick Roy --- tests/integration_tests/{build => functional}/test_binary_size.py | 0 .../{build => functional}/test_binary_static_linking.py | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/integration_tests/{build => functional}/test_binary_size.py (100%) rename tests/integration_tests/{build => functional}/test_binary_static_linking.py (100%) diff --git a/tests/integration_tests/build/test_binary_size.py b/tests/integration_tests/functional/test_binary_size.py similarity index 100% rename from tests/integration_tests/build/test_binary_size.py rename to tests/integration_tests/functional/test_binary_size.py diff --git a/tests/integration_tests/build/test_binary_static_linking.py b/tests/integration_tests/functional/test_binary_static_linking.py similarity index 100% rename from tests/integration_tests/build/test_binary_static_linking.py rename to tests/integration_tests/functional/test_binary_static_linking.py From f1a09cf5fba3331fd8ced49222fb9040d002f657 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Barb=C3=A1chano?= Date: Mon, 9 Sep 2024 15:05:57 +0200 Subject: [PATCH 3/3] fix(release): ensure debuginfo file contains debugging info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seems recently Cargo defaulted to use `strip=debuginfo`. This inadvertently made our debuginfo files much smaller Fix the issue by using `strip=none` and add a test so that it breaks if this somehow changes again. Signed-off-by: Pablo Barbáchano --- CHANGELOG.md | 3 ++ Cargo.toml | 4 ++ .../functional/test_binary.py | 48 +++++++++++++++++++ .../functional/test_binary_static_linking.py | 22 --------- 4 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 tests/integration_tests/functional/test_binary.py delete mode 100644 tests/integration_tests/functional/test_binary_static_linking.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e0ad7ca78fd..d5232f0db1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ and this project adheres to from net device creation to device activation time, but it was not reflected in the restore path. This was leading to inability to connect to the restored VM if the offload features were used. +- [#4829](https://github.com/firecracker-microvm/firecracker/pull/4829): v1.9.0 + was missing most of the debugging information in the debuginfo file, due to a + change in the Cargo defaults. This has been corrected. ## \[1.9.0\] diff --git a/Cargo.toml b/Cargo.toml index 1a5187f4f2f..914c4cdc87c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,3 +27,7 @@ panic = "abort" [profile.release] panic = "abort" lto = true +strip = "none" + +[profile.bench] +strip = "debuginfo" diff --git a/tests/integration_tests/functional/test_binary.py b/tests/integration_tests/functional/test_binary.py new file mode 100644 index 00000000000..67181bd3edb --- /dev/null +++ b/tests/integration_tests/functional/test_binary.py @@ -0,0 +1,48 @@ +# Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Tests to check several aspects of the binaries""" + +import re +import subprocess + +import pytest + +from framework import utils + + +@pytest.mark.timeout(500) +def test_firecracker_binary_static_linking(microvm_factory): + """ + Test to make sure the firecracker binary is statically linked. + """ + fc_binary_path = microvm_factory.fc_binary_path + _, stdout, stderr = utils.check_output(f"file {fc_binary_path}") + assert "" in stderr + # expected "statically linked" for aarch64 and + # "static-pie linked" for x86_64 + assert "statically linked" in stdout or "static-pie linked" in stdout + + +def test_release_debuginfo(microvm_factory): + """Ensure the debuginfo file has the right ELF sections""" + fc_binary = microvm_factory.fc_binary_path + debuginfo = fc_binary.with_suffix(".debug") + stdout = subprocess.check_output( + ["readelf", "-S", str(debuginfo)], + encoding="ascii", + ) + matches = { + match[0] for match in re.findall(r"\[..] (\.(\w|\.)+)", stdout, re.MULTILINE) + } + needed_sections = { + ".debug_aranges", + ".debug_info", + ".debug_abbrev", + ".debug_line", + ".debug_frame", + ".debug_str", + ".debug_ranges", + } + missing_sections = needed_sections - matches + assert missing_sections == set() diff --git a/tests/integration_tests/functional/test_binary_static_linking.py b/tests/integration_tests/functional/test_binary_static_linking.py deleted file mode 100644 index 1c277a4d00b..00000000000 --- a/tests/integration_tests/functional/test_binary_static_linking.py +++ /dev/null @@ -1,22 +0,0 @@ -# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 -"""Tests to check if the release binary is statically linked. - -""" - -import pytest - -from framework import utils - - -@pytest.mark.timeout(500) -def test_firecracker_binary_static_linking(microvm_factory): - """ - Test to make sure the firecracker binary is statically linked. - """ - fc_binary_path = microvm_factory.fc_binary_path - _, stdout, stderr = utils.check_output(f"file {fc_binary_path}") - assert "" in stderr - # expected "statically linked" for aarch64 and - # "static-pie linked" for x86_64 - assert "statically linked" in stdout or "static-pie linked" in stdout