Skip to content

Commit

Permalink
dragonball: Wrap config space into set_config_space
Browse files Browse the repository at this point in the history
Config space of network device is shared and accord with virtio 1.1 spec.
It is a good way to abstract the common part into one function.
`set_config_space()` implements this.

Plus, this patch removes `vq_pairs` from vhost-net devices, since there is
a possibility of data inconsistency. For example, some places read that
from `self.vq_pairs`, others read from `queue_sizes.len() / 2`.

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
  • Loading branch information
justxuewei committed Dec 22, 2023
1 parent beadce5 commit 88d797d
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 98 deletions.
1 change: 0 additions & 1 deletion src/dragonball/src/api/v1/virtio_net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ impl From<&NetworkInterfaceConfig> for VhostNetDeviceConfigInfo {
iface_id: config.iface_id.clone(),
host_dev_name: config.host_dev_name.clone(),
num_queues,
vq_pairs: num_queues / 2,
queue_size,
guest_mac: value.guest_mac,
allow_duplicate_mac: config.allow_duplicate_mac,
Expand Down
75 changes: 75 additions & 0 deletions src/dragonball/src/dbs_virtio_devices/src/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,73 @@ const PATCH_RATE_LIMITER_EVENT: u32 = 5;
// Number of DeviceEventT events supported by this implementation.
pub const NET_EVENTS_COUNT: u32 = 6;

// Config space of network config:
// https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-2000004
// MAC
const CONFIG_SPACE_MAC: usize = 0;
// Status
const CONFIG_SPACE_STATUS: usize = CONFIG_SPACE_MAC + MAC_ADDR_LEN;
const CONFIG_SPACE_STATUS_SIZE: usize = 2;
// Max virtqueue pairs
const CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS: usize = CONFIG_SPACE_STATUS + CONFIG_SPACE_STATUS_SIZE;
const CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS_SIZE: usize = 2;
// MTU
const CONFIG_SPACE_MTU: usize =
CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS + CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS_SIZE;
const CONFIG_SPACE_MTU_SIZE: usize = 2;
// Size of config space
const CONFIG_SPACE_SIZE: usize = MAC_ADDR_LEN
+ CONFIG_SPACE_STATUS_SIZE
+ CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS_SIZE
+ CONFIG_SPACE_MTU_SIZE;

// Default MTU for network device
pub const DEFAULT_MTU: u16 = 1500;

/// Setup config space for network device.
pub fn setup_config_space(
device_name: &str,
guest_mac: &Option<&MacAddr>,
avail_features: &mut u64,
vq_pairs: u16,
mtu: u16,
) -> Result<Vec<u8>> {
let mut config_space = Vec::with_capacity(CONFIG_SPACE_SIZE);
if let Some(mac) = guest_mac.as_ref() {
config_space[CONFIG_SPACE_MAC..CONFIG_SPACE_MAC + MAC_ADDR_LEN]
.copy_from_slice(mac.get_bytes());
// When this feature isn't available, the driver generates a random MAC address.
// Otherwise, it should attempt to read the device MAC address from the config space.
*avail_features |= 1u64 << VIRTIO_NET_F_MAC;
}

// Mark link as up: status only exists if VIRTIO_NET_F_STATUS is set.
if *avail_features & (1 << VIRTIO_NET_F_STATUS) != 0 {
config_space[CONFIG_SPACE_STATUS..CONFIG_SPACE_STATUS + CONFIG_SPACE_STATUS_SIZE]
.copy_from_slice(&(VIRTIO_NET_S_LINK_UP as u16).to_le_bytes());
}

// Set max virtqueue pairs, which only exists if VIRTIO_NET_F_MQ is set.
if *avail_features & (1 << VIRTIO_NET_F_MQ) != 0 {
if vq_pairs > 1 {
return Err(Error::InvalidInput);
}
config_space[CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS
..CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS + CONFIG_SPACE_MAX_VIRTQUEUE_PAIRS_SIZE]
.copy_from_slice(&vq_pairs.to_le_bytes());
}

config_space[CONFIG_SPACE_MTU..CONFIG_SPACE_MTU + CONFIG_SPACE_MTU_SIZE]
.copy_from_slice(&mtu.to_le_bytes());

debug!(
"{}: config space is set to {:X?}, guest_mac: {:?}, avail_feature: 0x{:X}, vq_pairs: {}, mtu: {}",
device_name, config_space, guest_mac, avail_features, vq_pairs, mtu
);

Ok(config_space)
}

/// Error for virtio-net devices to handle requests from guests.
#[derive(Debug, thiserror::Error)]
pub enum NetError {
Expand Down Expand Up @@ -641,6 +708,14 @@ impl<AS: GuestAddressSpace> Net<AS> {
avail_features |= 1u64 << VIRTIO_NET_F_MAC;
}

let config_space = setup_config_space(
NET_DRIVER_NAME,
&guest_mac,
&mut avail_features,
1,
DEFAULT_MTU,
)?;

let device_info = VirtioDeviceInfo::new(
NET_DRIVER_NAME.to_string(),
avail_features,
Expand Down
69 changes: 20 additions & 49 deletions src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_kern/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use virtio_bindings::bindings::virtio_ring::*;
use virtio_queue::{DescriptorChain, QueueT};
use vm_memory::{Address, GuestMemory, GuestMemoryRegion, MemoryRegionAddress};

use crate::net::{setup_config_space, DEFAULT_MTU};
use crate::vhost::net::{virtio_handle_ctrl_mq, virtio_handle_ctrl_status, FromNetCtrl};
#[cfg(test)]
use crate::vhost::vhost_kern::test_utils::{
Expand Down Expand Up @@ -65,7 +66,6 @@ where
R: GuestMemoryRegion + Sync + Send + 'static,
{
taps: Vec<Tap>,
vq_pairs: usize,
handles: Vec<VhostNet<AS>>,
device_info: VirtioDeviceInfo,
queue_sizes: Arc<Vec<u16>>,
Expand Down Expand Up @@ -134,13 +134,14 @@ where
/// Create a new vhost-net device with a given tap interface.
pub fn new_with_tap(
tap: Tap,
vq_pairs: usize,
guest_mac: Option<&MacAddr>,
queue_sizes: Arc<Vec<u16>>,
event_mgr: EpollManager,
) -> VirtioResult<Self> {
trace!(target: "vhost-net", "{}: Net::new_with_tap()", NET_DRIVER_NAME);

let vq_pairs = queue_sizes.len() / 2;

let taps = tap
.into_mq_taps(vq_pairs)
.map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::Open(err))))?;
Expand All @@ -163,41 +164,14 @@ where
if vq_pairs > 1 {
avail_features |= (1 << VIRTIO_NET_F_MQ | 1 << VIRTIO_NET_F_CTRL_VQ) as u64;
}
// Network device configuration layout:
// https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-2000004
// - [u8; 6]: mac address
// - u16: status
// - u16: max_virtqueue_pairs
// - u16: mtu
// - u32: speed
// - u8: duplex
let mut config_space = vec![0u8; 17];
if let Some(mac) = guest_mac {
// When this feature isn't available, the driver generates a random
// MAC address. Otherwise, it should attempt to read the device MAC
// address from the config space.
avail_features |= 1u64 << VIRTIO_NET_F_MAC;
config_space[0..6].copy_from_slice(mac.get_bytes());
} else {
avail_features &= !(1 << VIRTIO_NET_F_MAC) as u64;
}

// status: mark link as up
config_space[6] = VIRTIO_NET_S_LINK_UP as u8;
config_space[7] = 0;
// max_virtqueue_pairs: only support one rx/tx pair
config_space[8] = vq_pairs as u8;
config_space[9] = 0;
// mtu: 1500 = 1536 - vxlan header?
config_space[10] = 220;
config_space[11] = 5;
// speed: 1000Mb
config_space[12] = 232;
config_space[13] = 3;
config_space[14] = 0;
config_space[15] = 0;
// duplex: full duplex: 0x01
config_space[16] = 1;
let config_space = setup_config_space(
NET_DRIVER_NAME,
&guest_mac,
&mut avail_features,
(queue_sizes.len() / 2) as u16,
DEFAULT_MTU,
)?;

let device_info = VirtioDeviceInfo::new(
NET_DRIVER_NAME.to_owned(),
Expand All @@ -210,7 +184,6 @@ where

Ok(Net {
taps,
vq_pairs,
handles: Vec::new(),
device_info,
queue_sizes,
Expand All @@ -233,18 +206,19 @@ where
/// Create a vhost network with the Tap name
pub fn new(
host_dev_name: String,
vq_pairs: usize,
guest_mac: Option<&MacAddr>,
queue_sizes: Arc<Vec<u16>>,
event_mgr: EpollManager,
) -> VirtioResult<Self> {
let vq_pairs = queue_sizes.len() / 2;

// Open a TAP interface
let tap = Tap::open_named(&host_dev_name, vq_pairs > 1)
.map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::Open(err))))?;
tap.enable()
.map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::Enable(err))))?;

Self::new_with_tap(tap, vq_pairs, guest_mac, queue_sizes, event_mgr)
Self::new_with_tap(tap, guest_mac, queue_sizes, event_mgr)
}

fn do_device_activate(
Expand Down Expand Up @@ -284,30 +258,31 @@ where
Q: QueueT + Send + 'static,
R: GuestMemoryRegion + Sync + Send + 'static,
{
trace!(target: "vhost-net", "{}: Net::setup_vhost_backend(vq_pairs: {})", NET_DRIVER_NAME, self.vq_pairs);
let vq_pairs = self.queue_sizes.len() / 2;
trace!(target: "vhost-net", "{}: Net::setup_vhost_backend(vq_pairs: {})", NET_DRIVER_NAME, vq_pairs);

if self.vq_pairs < 1 {
if vq_pairs < 1 {
error!(
"{}: Invalid virtio queue pairs, expected a value greater than 0, but got {}",
NET_DRIVER_NAME, self.vq_pairs
NET_DRIVER_NAME, vq_pairs
);
return Err(VirtioError::ActivateError(Box::new(
ActivateError::InvalidParam,
)));
}

if self.handles.len() != self.vq_pairs || self.taps.len() != self.vq_pairs {
if self.handles.len() != vq_pairs || self.taps.len() != vq_pairs {
error!("{}: Invalid handlers or taps, handlers length {}, taps length {}, virtio queue pairs = {}",
NET_DRIVER_NAME,
self.handles.len(),
self.taps.len(),
self.vq_pairs);
vq_pairs);
return Err(VirtioError::ActivateError(Box::new(
ActivateError::InternalError,
)));
}

for idx in 0..self.vq_pairs {
for idx in 0..vq_pairs {
self.init_vhost_dev(idx, config, mem)?;
}

Expand Down Expand Up @@ -755,7 +730,6 @@ mod tests {
let epoll_mgr = EpollManager::default();
let mut dev: Net<Arc<GuestMemoryMmap>, QueueSync, GuestRegionMmap> = Net::new(
String::from("test_vhosttap"),
2,
Some(&guest_mac),
queue_sizes,
epoll_mgr,
Expand Down Expand Up @@ -797,7 +771,6 @@ mod tests {
let epoll_mgr = EpollManager::default();
let mut dev: Net<Arc<GuestMemoryMmap>, QueueSync, GuestRegionMmap> = Net::new(
String::from("test_vhosttap"),
2,
Some(&guest_mac),
queue_sizes,
epoll_mgr,
Expand Down Expand Up @@ -833,7 +806,6 @@ mod tests {
let epoll_mgr = EpollManager::default();
let mut dev: Net<Arc<GuestMemoryMmap>, QueueSync, GuestRegionMmap> = Net::new(
String::from("test_vhosttap"),
1,
Some(&guest_mac),
queue_sizes,
epoll_mgr,
Expand Down Expand Up @@ -878,7 +850,6 @@ mod tests {
let epoll_mgr = EpollManager::default();
let mut dev: Net<Arc<GuestMemoryMmap>, Queue, GuestRegionMmap> = Net::new(
String::from("test_vhosttap"),
1,
Some(&guest_mac),
queue_sizes,
epoll_mgr,
Expand Down
55 changes: 16 additions & 39 deletions src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@ use vhost_rs::vhost_user::{
use vhost_rs::Error as VhostError;
use virtio_bindings::bindings::virtio_net::{
virtio_net_ctrl_hdr, VIRTIO_NET_CTRL_MQ, VIRTIO_NET_F_CTRL_MAC_ADDR, VIRTIO_NET_F_CTRL_RX,
VIRTIO_NET_F_CTRL_VLAN, VIRTIO_NET_F_CTRL_VQ, VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MAC,
VIRTIO_NET_F_MQ, VIRTIO_NET_F_MTU, VIRTIO_NET_OK, VIRTIO_NET_S_LINK_UP,
VIRTIO_NET_F_CTRL_VLAN, VIRTIO_NET_F_CTRL_VQ, VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
VIRTIO_NET_F_MTU, VIRTIO_NET_OK,
};
use virtio_queue::{DescriptorChain, QueueT};
use vm_memory::GuestMemoryRegion;
use vmm_sys_util::epoll::EventSet;

use super::connection::{Endpoint, Listener};
use crate::net::{setup_config_space, DEFAULT_MTU};
use crate::vhost::net::{virtio_handle_ctrl_mq, virtio_handle_ctrl_status, FromNetCtrl};
use crate::vhost::vhost_user::connection::EndpointParam;
use crate::{
Expand Down Expand Up @@ -65,58 +66,34 @@ impl VhostUserNetDevice {
queue_sizes: Arc<Vec<u16>>,
epoll_mgr: EpollManager,
) -> VirtioResult<Self> {
// hard-coding MTU
info!(
"{}: slave support features 0x{:x}",
NET_DRIVER_NAME, avail_features
);

avail_features |= (1 << VIRTIO_NET_F_MTU) as u64;
// All these features depends on availability of control
// channel (VIRTIO_NET_F_CTRL_VQ).
// All these features depends on availability of control channel
// (VIRTIO_NET_F_CTRL_VQ).
avail_features &= !(1 << VIRTIO_NET_F_CTRL_VQ
| 1 << VIRTIO_NET_F_CTRL_RX
| 1 << VIRTIO_NET_F_CTRL_VLAN
| 1 << VIRTIO_NET_F_GUEST_ANNOUNCE
| 1 << VIRTIO_NET_F_MQ
| 1 << VIRTIO_NET_F_CTRL_MAC_ADDR) as u64;

// Multi-queue features
if queue_sizes.len() > 2 {
avail_features |= (1 << VIRTIO_NET_F_MQ | 1 << VIRTIO_NET_F_CTRL_VQ) as u64;
}
// Network device configuration layout:
// https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-2000004
// - [u8; 6]: mac address
// - u16: status
// - u16: max_virtqueue_pairs
// - u16: mtu
// - u32: speed
// - u8: duplex
let mut config_space = vec![0u8; 17];
if let Some(mac) = guest_mac {
// When this feature isn't available, the driver generates a random
// MAC address. Otherwise, it should attempt to read the device MAC
// address from the config space.
avail_features |= 1u64 << VIRTIO_NET_F_MAC;
config_space[0..6].copy_from_slice(mac.get_bytes());
} else {
avail_features &= !(1 << VIRTIO_NET_F_MAC) as u64;
}
// status: mark link as up
config_space[6] = VIRTIO_NET_S_LINK_UP as u8;
config_space[7] = 0;
// max_virtqueue_pairs: only support one rx/tx pair
config_space[8] = (queue_sizes.len() / 2) as u8;
config_space[9] = 0;
// mtu: 1500 = 1536 - vxlan header?
config_space[10] = 220;
config_space[11] = 5;
// speed: 1000Mb
config_space[12] = 232;
config_space[13] = 3;
config_space[14] = 0;
config_space[15] = 0;
// duplex: full duplex: 0x01
config_space[16] = 1;

let config_space = setup_config_space(
NET_DRIVER_NAME,
&guest_mac,
&mut avail_features,
(queue_sizes.len() / 2) as u16,
DEFAULT_MTU,
)?;

Ok(VhostUserNetDevice {
id: NET_DRIVER_NAME.to_owned(),
device_info: VirtioDeviceInfo::new(
Expand Down

0 comments on commit 88d797d

Please sign in to comment.