Skip to content

Commit

Permalink
fix(net): set tap offload features on restore
Browse files Browse the repository at this point in the history
Tap offload features configuration was moved from the device creation
time to the device activation time by the following commit:

commit 1e5d3db
Author: Nikita Zakirov <zakironi@amazon.com>
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 <kalyazin@amazon.com>
  • Loading branch information
kalyazin committed Oct 2, 2024
1 parent d3b02e0 commit 279cf5c
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 2 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/vmm/src/devices/virtio/net/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 8 additions & 1 deletion src/vmm/src/devices/virtio/net/persist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)?;

Check warning on line 137 in src/vmm/src/devices/virtio/net/persist.rs

View check run for this annotation

Codecov / codecov/patch

src/vmm/src/devices/virtio/net/persist.rs#L134-L137

Added lines #L134 - L137 were not covered by tests

net.device_state = DeviceState::Activated(constructor_args.mem);
}

Expand Down

0 comments on commit 279cf5c

Please sign in to comment.