From 58e9709c1f701a8e7d376581e397fc069dc44c5e Mon Sep 17 00:00:00 2001 From: Xuewei Niu Date: Fri, 27 Oct 2023 16:43:58 +0800 Subject: [PATCH] dragonball: Changes for ZizhengBian's comments - Dragonball's vhost-net feature not depends on virtio-net feature. - Remove `TapError` from dbs-virtio-devices's Error, and add `VirtioNet` and `VhostNet` two fields. - Downgrade visiblity of two fields of `VhostNetDeviceMgr` from `pub(crate)`. - File an issue to record a todo for network rate limiter. - Print internal errors with `{0:?}. Signed-off-by: Xuewei Niu --- src/dragonball/Cargo.lock | 13 ++++ src/dragonball/Cargo.toml | 2 +- src/dragonball/src/api/v1/mod.rs | 2 + src/dragonball/src/api/v1/vmm_action.rs | 4 +- .../src/dbs_virtio_devices/src/lib.rs | 17 +++-- .../src/dbs_virtio_devices/src/net.rs | 17 +++-- .../src/vhost/vhost_kern/net.rs | 75 ++++++++++++------- src/dragonball/src/device_manager/mod.rs | 2 +- .../src/device_manager/vhost_net_dev_mgr.rs | 23 +++--- .../src/device_manager/virtio_net_dev_mgr.rs | 1 + src/dragonball/src/error.rs | 2 +- .../crates/hypervisor/src/dragonball/mod.rs | 6 +- 12 files changed, 106 insertions(+), 58 deletions(-) diff --git a/src/dragonball/Cargo.lock b/src/dragonball/Cargo.lock index c21991b339..064b11d504 100644 --- a/src/dragonball/Cargo.lock +++ b/src/dragonball/Cargo.lock @@ -396,6 +396,7 @@ dependencies = [ "serde_json", "thiserror", "threadpool", + "vhost", "virtio-bindings", "virtio-queue", "vm-memory", @@ -2071,6 +2072,18 @@ version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" +[[package]] +name = "vhost" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a6769e8dbf5276b4376439fbf36bb880d203bf614bf7ef444198edc24b5a9f35" +dependencies = [ + "bitflags 1.3.2", + "libc", + "vm-memory", + "vmm-sys-util", +] + [[package]] name = "virtio-bindings" version = "0.1.0" diff --git a/src/dragonball/Cargo.toml b/src/dragonball/Cargo.toml index 29f79a6ca5..95680d8dee 100644 --- a/src/dragonball/Cargo.toml +++ b/src/dragonball/Cargo.toml @@ -63,4 +63,4 @@ virtio-net = ["dbs-virtio-devices/virtio-net", "virtio-queue"] virtio-fs = ["dbs-virtio-devices/virtio-fs-pro", "virtio-queue", "atomic-guest-memory"] virtio-mem = ["dbs-virtio-devices/virtio-mem", "virtio-queue", "atomic-guest-memory"] virtio-balloon = ["dbs-virtio-devices/virtio-balloon", "virtio-queue"] -vhost-net = ["virtio-net", "dbs-virtio-devices/vhost-net"] +vhost-net = ["dbs-virtio-devices/vhost-net"] diff --git a/src/dragonball/src/api/v1/mod.rs b/src/dragonball/src/api/v1/mod.rs index 6b6ed0645f..470b6703c4 100644 --- a/src/dragonball/src/api/v1/mod.rs +++ b/src/dragonball/src/api/v1/mod.rs @@ -19,5 +19,7 @@ mod machine_config; pub use self::machine_config::{VmConfigError, MAX_SUPPORTED_VCPUS}; /// Wrapper for configuring the virtio networking +#[cfg(any(feature = "virtio-net", feature = "vhost-net"))] mod virtio_net; +#[cfg(any(feature = "virtio-net", feature = "vhost-net"))] pub use virtio_net::{Backend, NetworkInterfaceConfig, NetworkInterfaceUpdateConfig, VirtioConfig}; diff --git a/src/dragonball/src/api/v1/vmm_action.rs b/src/dragonball/src/api/v1/vmm_action.rs index 63db56cb26..dc2f2f0651 100644 --- a/src/dragonball/src/api/v1/vmm_action.rs +++ b/src/dragonball/src/api/v1/vmm_action.rs @@ -106,7 +106,7 @@ pub enum VmmActionError { VirtioNet(#[source] VirtioNetDeviceError), #[cfg(feature = "vhost-net")] - #[error("vhost-net device error: {0}")] + #[error("vhost-net device error: {0:?}")] /// Vhost-net device relared errors. VhostNet(#[source] VhostNetDeviceError), @@ -318,7 +318,7 @@ impl VmmService { VmmAction::RemoveBlockDevice(drive_id) => { self.remove_block_device(vmm, event_mgr, &drive_id) } - #[cfg(feature = "virtio-net")] + #[cfg(any(feature = "virtio-net", feature = "vhost-net"))] VmmAction::InsertNetworkDevice(config) => match config.backend { Backend::Virtio(_) => { #[cfg(not(feature = "virtio-net"))] diff --git a/src/dragonball/src/dbs_virtio_devices/src/lib.rs b/src/dragonball/src/dbs_virtio_devices/src/lib.rs index 548a934b15..9262550f62 100644 --- a/src/dragonball/src/dbs_virtio_devices/src/lib.rs +++ b/src/dragonball/src/dbs_virtio_devices/src/lib.rs @@ -46,6 +46,7 @@ pub mod vhost; use std::io::Error as IOError; +use net::NetError; use virtio_queue::Error as VqError; use vm_memory::{GuestAddress, GuestAddressSpace, GuestMemoryError}; @@ -209,14 +210,14 @@ pub enum Error { #[error("virtio-fs error: {0}")] VirtioFs(fs::Error), - #[cfg(feature = "vhost")] - #[error("vhost error: {0}")] - /// Error from vhost-net. - VhostNet(#[from] vhost_rs::Error), - #[cfg(feature = "virtio-net")] - #[error("tap device operation error: {0}")] - TapDeviceError(#[source] TapError), + #[error("virtio-net error: {0:?}")] + VirtioNet(NetError), + + #[cfg(feature = "vhost-net")] + #[error("vhost-net error: {0:?}")] + /// Error from vhost-net. + VhostNet(vhost::vhost_kern::net::Error), #[cfg(feature = "virtio-mem")] #[error("Virtio-mem error: {0}")] @@ -234,7 +235,7 @@ pub enum TapError { #[error("missing {0} flags")] MissingFlags(String), - #[error("failed to set offload: {0}")] + #[error("failed to set offload: {0:?}")] SetOffload(#[source] dbs_utils::net::TapError), #[error("failed to set vnet_hdr_size: {0}")] diff --git a/src/dragonball/src/dbs_virtio_devices/src/net.rs b/src/dragonball/src/dbs_virtio_devices/src/net.rs index 7d0afd5431..671873ade5 100644 --- a/src/dragonball/src/dbs_virtio_devices/src/net.rs +++ b/src/dragonball/src/dbs_virtio_devices/src/net.rs @@ -31,8 +31,8 @@ use vmm_sys_util::eventfd::EventFd; use crate::device::{VirtioDeviceConfig, VirtioDeviceInfo}; use crate::{ - ActivateError, ActivateResult, ConfigResult, DbsGuestAddressSpace, Error, Result, VirtioDevice, - VirtioQueueConfig, TYPE_NET, + ActivateError, ActivateResult, ConfigResult, DbsGuestAddressSpace, Error, Result, TapError, + VirtioDevice, VirtioQueueConfig, TYPE_NET, }; const NET_DRIVER_NAME: &str = "virtio-net"; @@ -57,6 +57,13 @@ const PATCH_RATE_LIMITER_EVENT: u32 = 5; // Number of DeviceEventT events supported by this implementation. pub const NET_EVENTS_COUNT: u32 = 6; +/// Error for virtio-net devices to handle requests from guests. +#[derive(Debug, thiserror::Error)] +pub enum NetError { + #[error("tap device operation error: {0:?}")] + TapError(#[source] TapError), +} + /// Metrics specific to the net device. #[derive(Default, Serialize)] pub struct NetDeviceMetrics { @@ -651,11 +658,11 @@ impl Net { tap.set_offload( net_gen::TUN_F_CSUM | net_gen::TUN_F_UFO | net_gen::TUN_F_TSO4 | net_gen::TUN_F_TSO6, ) - .map_err(|err| Error::TapDeviceError(crate::TapError::SetOffload(err)))?; + .map_err(|err| Error::VirtioNet(NetError::TapError(TapError::SetOffload(err))))?; let vnet_hdr_size = vnet_hdr_len() as i32; tap.set_vnet_hdr_size(vnet_hdr_size) - .map_err(|err| Error::TapDeviceError(crate::TapError::SetVnetHdrSize(err)))?; + .map_err(|err| Error::VirtioNet(NetError::TapError(TapError::SetVnetHdrSize(err))))?; info!("net tap set finished"); let mut avail_features = 1u64 << VIRTIO_NET_F_GUEST_CSUM @@ -709,7 +716,7 @@ impl Net { ) -> Result { info!("open net tap {}", host_dev_name); let tap = Tap::open_named(host_dev_name.as_str(), false) - .map_err(|err| Error::TapDeviceError(crate::TapError::Open(err)))?; + .map_err(|err| Error::VirtioNet(NetError::TapError(TapError::Open(err))))?; info!("net tap opened"); Self::new_with_tap( diff --git a/src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_kern/net.rs b/src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_kern/net.rs index 71e3c0a8f3..dfa5bec83a 100644 --- a/src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_kern/net.rs +++ b/src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_kern/net.rs @@ -31,7 +31,7 @@ use virtio_bindings::bindings::virtio_ring::*; use virtio_queue::{Descriptor, DescriptorChain, QueueT}; use vm_memory::{Address, Bytes, GuestMemory, GuestMemoryRegion, MemoryRegionAddress}; -use crate::net::{NetDeviceMetrics, vnet_hdr_len}; +use crate::net::{vnet_hdr_len, NetDeviceMetrics}; #[cfg(test)] use crate::vhost::vhost_kern::test_utils::{ MockVhostBackend as VhostBackend, MockVhostNet as VhostNet, @@ -47,6 +47,15 @@ const CTRL_SLOT: u32 = 0; // Control queue size const CTRL_QUEUE_SIZE: u16 = 64; +/// Error for vhost-net devices to handle requests from guests. +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("tap device operation error: {0:?}")] + TapError(#[source] TapError), + #[error("vhost error: {0}")] + VhostError(#[source] vhost_rs::Error), +} + /// Vhost-net device implementation pub struct Net where @@ -97,12 +106,14 @@ fn validate_and_configure_tap(tap: &Tap, vq_pairs: usize) -> VirtioResult<()> { ) .collect::>(); if !missing_flags.is_empty() { - return Err(VirtioError::TapDeviceError(TapError::MissingFlags( - missing_flags - .into_iter() - .map(|flag| *flag) - .collect::>() - .join(", "), + return Err(VirtioError::VhostNet(Error::TapError( + TapError::MissingFlags( + missing_flags + .into_iter() + .map(|flag| *flag) + .collect::>() + .join(", "), + ), ))); } @@ -110,11 +121,11 @@ fn validate_and_configure_tap(tap: &Tap, vq_pairs: usize) -> VirtioResult<()> { tap.set_offload( net_gen::TUN_F_CSUM | net_gen::TUN_F_UFO | net_gen::TUN_F_TSO4 | net_gen::TUN_F_TSO6, ) - .map_err(|err| VirtioError::TapDeviceError(TapError::SetOffload(err)))?; + .map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::SetOffload(err))))?; let vnet_hdr_size = vnet_hdr_len() as i32; tap.set_vnet_hdr_size(vnet_hdr_size) - .map_err(|err| VirtioError::TapDeviceError(TapError::SetVnetHdrSize(err)))?; + .map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::SetVnetHdrSize(err))))?; Ok(()) } @@ -137,7 +148,7 @@ where let taps = tap .into_mq_taps(vq_pairs) - .map_err(|err| VirtioError::TapDeviceError(TapError::Open(err)))?; + .map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::Open(err))))?; for tap in taps.iter() { validate_and_configure_tap(tap, vq_pairs)?; } @@ -234,9 +245,9 @@ where ) -> VirtioResult { // Open a TAP interface let tap = Tap::open_named(&host_dev_name, vq_pairs > 1) - .map_err(|err| VirtioError::TapDeviceError(TapError::Open(err)))?; + .map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::Open(err))))?; tap.enable() - .map_err(|err| VirtioError::TapDeviceError(TapError::Enable(err)))?; + .map_err(|err| VirtioError::VhostNet(Error::TapError(TapError::Enable(err))))?; Self::new_with_tap(tap, vq_pairs, guest_mac, queue_sizes, event_mgr) } @@ -255,8 +266,10 @@ where if self.handles.is_empty() { for _ in 0..vq_pairs { - self.handles - .push(VhostNet::::new(config.vm_as.clone()).map_err(VirtioError::VhostNet)?); + self.handles.push( + VhostNet::::new(config.vm_as.clone()) + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?, + ); } } @@ -317,11 +330,17 @@ where trace!(target: "vhost-net", "{}: Net::init_vhost_dev(pair_index: {})", NET_DRIVER_NAME, pair_index); let handle = &mut self.handles[pair_index]; - handle.set_owner().map_err(VirtioError::VhostNet)?; + handle + .set_owner() + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; - let avail_features = handle.get_features().map_err(VirtioError::VhostNet)?; + let avail_features = handle + .get_features() + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; let features = self.device_info.acked_features() & avail_features; - handle.set_features(features).map_err(VirtioError::VhostNet)?; + handle + .set_features(features) + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; let mut regions = Vec::new(); for region in mem.iter() { @@ -335,12 +354,14 @@ where guest_phys_addr: guest_phys_addr.raw_value(), memory_size: region.len(), userspace_addr: userspace_addr as *const u8 as u64, - mmap_offset: 0u64, - mmap_handle: -1i32, + mmap_offset: 0, + mmap_handle: -1, }); } - handle.set_mem_table(®ions).map_err(VirtioError::VhostNet)?; + handle + .set_mem_table(®ions) + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; self.init_vhost_queues(pair_index, config)?; @@ -376,7 +397,7 @@ where handle .set_vring_num(vq_index, queue_cfg.queue.size()) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; if let Some(vring_base) = &self.kernel_vring_bases { let base = if vq_index == 0 { @@ -386,11 +407,11 @@ where }; handle .set_vring_base(vq_index, base as u16) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; } else { handle .set_vring_base(vq_index, 0) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; } let config_data = &VringConfigData { @@ -405,19 +426,19 @@ where handle .set_vring_addr(vq_index, config_data) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; handle .set_vring_call(vq_index, intr_evts[queue_index]) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; handle .set_vring_kick(vq_index, &queue_cfg.eventfd) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; handle .set_backend(vq_index, Some(&tap.tap_file)) - .map_err(VirtioError::VhostNet)?; + .map_err(|err| VirtioError::VhostNet(Error::VhostError(err)))?; } Ok(()) diff --git a/src/dragonball/src/device_manager/mod.rs b/src/dragonball/src/device_manager/mod.rs index 63b6dbe4e4..6ec463dd1d 100644 --- a/src/dragonball/src/device_manager/mod.rs +++ b/src/dragonball/src/device_manager/mod.rs @@ -545,7 +545,7 @@ pub struct DeviceManager { pub(crate) balloon_manager: BalloonDeviceMgr, #[cfg(feature = "vhost-net")] - pub(crate) vhost_net_manager: VhostNetDeviceMgr, + vhost_net_manager: VhostNetDeviceMgr, } impl DeviceManager { diff --git a/src/dragonball/src/device_manager/vhost_net_dev_mgr.rs b/src/dragonball/src/device_manager/vhost_net_dev_mgr.rs index e9d4cf3955..a7666b47a8 100644 --- a/src/dragonball/src/device_manager/vhost_net_dev_mgr.rs +++ b/src/dragonball/src/device_manager/vhost_net_dev_mgr.rs @@ -3,18 +3,18 @@ // // SPDX-License-Identifier: Apache-2.0 +use std::result::Result; +use std::sync::Arc; + use dbs_utils::net::MacAddr; -use dbs_virtio_devices::{vhost::vhost_kern::net::Net, Error as VirtioError}; +use dbs_virtio_devices::vhost::vhost_kern::net::Net; +use dbs_virtio_devices::Error as VirtioError; use serde::{Deserialize, Serialize}; -use std::{result::Result, sync::Arc}; use virtio_queue::QueueSync; -use crate::{ - address_space_manager::{GuestAddressSpaceImpl, GuestRegionImpl}, - config_manager::{ConfigItem, DeviceConfigInfos}, -}; - use super::{DeviceManager, DeviceMgrError, DeviceOpContext}; +use crate::address_space_manager::{GuestAddressSpaceImpl, GuestRegionImpl}; +use crate::config_manager::{ConfigItem, DeviceConfigInfos}; /// Default number of virtio queues, one rx/tx pair. pub const NUM_QUEUES: usize = 2; @@ -44,10 +44,10 @@ pub enum VhostNetDeviceError { #[error("invalid queue number {0} for vhost-net device")] InvalidQueueNum(usize), /// Failure from device manager. - #[error("failure in device manager operations: {0}")] + #[error("failure in device manager operations: {0:?}")] DeviceManager(#[source] DeviceMgrError), /// Failure from virtio subsystem. - #[error(transparent)] + #[error("virtio error: {0:?}")] Virtio(VirtioError), /// Split this at some point. /// Internal errors are due to resource exhaustion. @@ -60,6 +60,7 @@ pub enum VhostNetDeviceError { } /// Configuration information for vhost net devices. +/// TODO: https://github.com/kata-containers/kata-containers/issues/8382. #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct VhostNetDeviceConfigInfo { /// Id of the guest network interface. @@ -138,8 +139,8 @@ impl ConfigItem for VhostNetDeviceConfigInfo { /// Device manager to manage all vhost net devices. pub struct VhostNetDeviceMgr { - pub(crate) info_list: DeviceConfigInfos, - pub(crate) use_shared_irq: bool, + info_list: DeviceConfigInfos, + use_shared_irq: bool, } impl VhostNetDeviceMgr { diff --git a/src/dragonball/src/device_manager/virtio_net_dev_mgr.rs b/src/dragonball/src/device_manager/virtio_net_dev_mgr.rs index f57c4ed67b..ddbce30057 100644 --- a/src/dragonball/src/device_manager/virtio_net_dev_mgr.rs +++ b/src/dragonball/src/device_manager/virtio_net_dev_mgr.rs @@ -123,6 +123,7 @@ impl VirtioNetDeviceConfigUpdateInfo { } /// Configuration information for virtio net devices. +/// TODO: https://github.com/kata-containers/kata-containers/issues/8382. #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize, Default)] pub struct VirtioNetDeviceConfigInfo { /// ID of the guest network interface. diff --git a/src/dragonball/src/error.rs b/src/dragonball/src/error.rs index 44c3b04dd9..35b92244f8 100644 --- a/src/dragonball/src/error.rs +++ b/src/dragonball/src/error.rs @@ -196,7 +196,7 @@ pub enum StartMicroVmError { /// Vhost-net device errors. #[cfg(feature = "vhost-net")] - #[error("vhost-net errors: {0}")] + #[error("vhost-net errors: {0:?}")] VhostNetDeviceError(#[source] device_manager::vhost_net_dev_mgr::VhostNetDeviceError), } diff --git a/src/runtime-rs/crates/hypervisor/src/dragonball/mod.rs b/src/runtime-rs/crates/hypervisor/src/dragonball/mod.rs index 5376d8343f..f4cb798bc4 100644 --- a/src/runtime-rs/crates/hypervisor/src/dragonball/mod.rs +++ b/src/runtime-rs/crates/hypervisor/src/dragonball/mod.rs @@ -208,9 +208,11 @@ impl From<&NetworkConfig> for DragonballNetworkConfig { let virtio_config = DragonballVirtioConfig { iface_id: value.virt_iface_name.clone(), host_dev_name: value.host_dev_name.clone(), - // TODO(justxuewei): rx_rate_limiter is not supported. + // TODO(justxuewei): rx_rate_limiter is not supported, see: + // https://github.com/kata-containers/kata-containers/issues/8327. rx_rate_limiter: None, - // TODO(justxuewei): tx_rate_limiter is not supported. + // TODO(justxuewei): tx_rate_limiter is not supported, see: + // https://github.com/kata-containers/kata-containers/issues/8327. tx_rate_limiter: None, allow_duplicate_mac: value.allow_duplicate_mac, };