From 275de453d5eb355d18f113368989f6dced848cc4 Mon Sep 17 00:00:00 2001 From: "alex.lyn" Date: Thu, 21 Dec 2023 11:07:56 +0800 Subject: [PATCH 1/5] runtime-rs: remove useless get_host_guest_map and its test case Fixes: #8665 Signed-off-by: alex.lyn --- .../crates/hypervisor/src/device/driver/mod.rs | 11 ++--------- .../crates/hypervisor/src/device/driver/vfio.rs | 6 ------ 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs index 2aff85b8c4..8ccc3dfc68 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs @@ -12,8 +12,8 @@ mod virtio_net; mod virtio_vsock; pub use vfio::{ - bind_device_to_host, bind_device_to_vfio, get_host_guest_map, get_vfio_device, HostDevice, - VfioBusMode, VfioConfig, VfioDevice, + bind_device_to_host, bind_device_to_vfio, get_vfio_device, HostDevice, VfioBusMode, VfioConfig, + VfioDevice, }; pub use virtio_blk::{ BlockConfig, BlockDevice, KATA_BLK_DEV_TYPE, KATA_MMIO_BLK_DEV_TYPE, KATA_NVDIMM_DEV_TYPE, @@ -207,11 +207,4 @@ mod tests { assert!(root_slot.is_some()); assert_eq!(root_slot.unwrap().0, 1); } - - #[test] - fn test_get_host_guest_map() { - // test unwrap is fine, no panic occurs. - let hg_map = get_host_guest_map("".to_owned()); - assert!(hg_map.is_none()); - } } diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs index 07890de659..be1eb3cb52 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs @@ -70,12 +70,6 @@ pub fn generate_guest_pci_path(bdf: String) -> Result { }) } -// get host/guest mapping for info -pub fn get_host_guest_map(host_bdf: String) -> Option { - // safe, just do unwrap as `HOST_GUEST_MAP` is always valid. - HOST_GUEST_MAP.read().unwrap().get(&host_bdf).cloned() -} - pub fn do_check_iommu_on() -> Result { let element = std::fs::read_dir(SYS_CLASS_IOMMU)? .filter_map(|e| e.ok()) From 1b5758c1f29491f9e39eafd7ed48acde7dbc4660 Mon Sep 17 00:00:00 2001 From: "alex.lyn" Date: Thu, 21 Dec 2023 11:35:18 +0800 Subject: [PATCH 2/5] runtime-rs: Move the PciPath-related code to a dedicated file Move the pciPath code to a new file pci_path.rs and update the references. Fixes: #8665 Signed-off-by: alex.lyn --- .../crates/hypervisor/src/ch/inner_device.rs | 2 +- .../hypervisor/src/device/driver/mod.rs | 179 ----------------- .../hypervisor/src/device/driver/vfio.rs | 10 +- .../src/device/driver/virtio_blk.rs | 2 +- .../crates/hypervisor/src/device/mod.rs | 1 + .../crates/hypervisor/src/device/pci_path.rs | 184 ++++++++++++++++++ 6 files changed, 193 insertions(+), 185 deletions(-) create mode 100644 src/runtime-rs/crates/hypervisor/src/device/pci_path.rs diff --git a/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs b/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs index c4aa39e818..6b9cbff22b 100644 --- a/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs +++ b/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs @@ -5,11 +5,11 @@ // SPDX-License-Identifier: Apache-2.0 use super::inner::CloudHypervisorInner; +use crate::device::pci_path::PciPath; use crate::device::DeviceType; use crate::HybridVsockDevice; use crate::NetworkConfig; use crate::NetworkDevice; -use crate::PciPath; use crate::ShareFsConfig; use crate::ShareFsDevice; use crate::VfioDevice; diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs index 8ccc3dfc68..3e51d48a33 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/mod.rs @@ -29,182 +29,3 @@ pub use virtio_vsock::{ pub mod vhost_user_blk; pub use vhost_user::{VhostUserConfig, VhostUserDevice, VhostUserType}; - -use anyhow::{anyhow, Context, Result}; - -// Tips: -// The Re-write `PciSlot` and `PciPath` with rust that it origins from `pcipath.go`: -// - -// The PCI spec reserves 5 bits for slot number (a.k.a. device -// number), giving slots 0..31 -const PCI_SLOT_BITS: u32 = 5; -const MAX_PCI_SLOTS: u32 = (1 << PCI_SLOT_BITS) - 1; - -// A PciSlot describes where a PCI device sits on a single bus -// -// This encapsulates the PCI slot number a.k.a device number, which is -// limited to a 5 bit value [0x00..0x1f] by the PCI specification -// -// To support multifunction device's, It's needed to extend -// this to include the PCI 3-bit function number as well. -#[derive(Clone, Debug, Default, PartialEq)] -pub struct PciSlot(pub u8); - -impl PciSlot { - pub fn convert_from_string(s: &str) -> Result { - if s.is_empty() || s.len() > 2 { - return Err(anyhow!("string given is invalid.")); - } - - let base = 16; - let n = u64::from_str_radix(s, base).context("convert string to number failed")?; - if n >> PCI_SLOT_BITS > 0 { - return Err(anyhow!( - "number {:?} exceeds MAX:{:?}, failed.", - n, - MAX_PCI_SLOTS - )); - } - - Ok(PciSlot(n as u8)) - } - - pub fn convert_from_u32(v: u32) -> Result { - if v > MAX_PCI_SLOTS { - return Err(anyhow!("value {:?} exceeds MAX: {:?}", v, MAX_PCI_SLOTS)); - } - - Ok(PciSlot(v as u8)) - } - - pub fn convert_to_string(&self) -> String { - format!("{:02x}", self.0) - } -} - -// A PciPath describes where a PCI sits in a PCI hierarchy. -// -// Consists of a list of PCI slots, giving the slot of each bridge -// that must be traversed from the PCI root to reach the device, -// followed by the slot of the device itself. -// -// When formatted into a string is written as "xx/.../yy/zz". Here, -// zz is the slot of the device on its PCI bridge, yy is the slot of -// the bridge on its parent bridge and so forth until xx is the slot -// of the "most upstream" bridge on the root bus. -// -// If a device is directly connected to the root bus, which used in -// lightweight hypervisors, such as dragonball/firecracker/clh, and -// its PciPath.slots will contains only one PciSlot. -#[derive(Clone, Debug, Default, PartialEq)] -pub struct PciPath { - // list of PCI slots - slots: Vec, -} - -impl PciPath { - // method to format the PciPath into a string - pub fn convert_to_string(&self) -> String { - self.slots - .iter() - .map(|pci_slot| format!("{:02x}", pci_slot.0)) - .collect::>() - .join("/") - } - - // method to parse a PciPath from a string - pub fn convert_from_string(path: &str) -> Result { - if path.is_empty() { - return Err(anyhow!("path given is empty.")); - } - - let mut pci_slots: Vec = Vec::new(); - let slots: Vec<&str> = path.split('/').collect(); - for slot in slots { - match PciSlot::convert_from_string(slot) { - Ok(s) => pci_slots.push(s), - Err(e) => return Err(anyhow!("slot is invalid with: {:?}", e)), - } - } - - Ok(PciPath { slots: pci_slots }) - } - - pub fn from_pci_slots(slots: Vec) -> Option { - if slots.is_empty() { - return None; - } - - Some(PciPath { slots }) - } - - // device_slot to get the slot of the device on its PCI bridge - pub fn get_device_slot(&self) -> Option { - self.slots.last().cloned() - } - - // root_slot to get the slot of the "most upstream" bridge on the root bus - pub fn get_root_slot(&self) -> Option { - self.slots.first().cloned() - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_pci_slot() { - // min - let pci_slot_01 = PciSlot::convert_from_string("00"); - assert!(pci_slot_01.is_ok()); - // max - let pci_slot_02 = PciSlot::convert_from_string("1f"); - assert!(pci_slot_02.is_ok()); - - // exceed - let pci_slot_03 = PciSlot::convert_from_string("20"); - assert!(pci_slot_03.is_err()); - - // valid number - let pci_slot_04 = PciSlot::convert_from_u32(1_u32); - assert!(pci_slot_04.is_ok()); - assert_eq!(pci_slot_04.as_ref().unwrap().0, 1_u8); - let pci_slot_str = pci_slot_04.as_ref().unwrap().convert_to_string(); - assert_eq!(pci_slot_str, format!("{:02x}", pci_slot_04.unwrap().0)); - - // max number - let pci_slot_05 = PciSlot::convert_from_u32(31_u32); - assert!(pci_slot_05.is_ok()); - assert_eq!(pci_slot_05.unwrap().0, 31_u8); - - // exceed and error - let pci_slot_06 = PciSlot::convert_from_u32(32_u32); - assert!(pci_slot_06.is_err()); - } - - #[test] - fn test_pci_patch() { - let pci_path_0 = PciPath::convert_from_string("01/0a/05"); - assert!(pci_path_0.is_ok()); - let pci_path_unwrap = pci_path_0.unwrap(); - assert_eq!(pci_path_unwrap.slots[0].0, 1); - assert_eq!(pci_path_unwrap.slots[1].0, 10); - assert_eq!(pci_path_unwrap.slots[2].0, 5); - - let pci_path_01 = PciPath::from_pci_slots(vec![PciSlot(1), PciSlot(10), PciSlot(5)]); - assert!(pci_path_01.is_some()); - let pci_path = pci_path_01.unwrap(); - let pci_path_02 = pci_path.convert_to_string(); - assert_eq!(pci_path_02, "01/0a/05".to_string()); - - let dev_slot = pci_path.get_device_slot(); - assert!(dev_slot.is_some()); - assert_eq!(dev_slot.unwrap().0, 5); - - let root_slot = pci_path.get_root_slot(); - assert!(root_slot.is_some()); - assert_eq!(root_slot.unwrap().0, 1); - } -} diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs index be1eb3cb52..e488281841 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs @@ -20,12 +20,14 @@ use async_trait::async_trait; use lazy_static::lazy_static; use path_clean::PathClean; -use crate::{ - device::{hypervisor, Device, DeviceType}, - PciPath, PciSlot, -}; use kata_sys_util::fs::get_base_name; +use crate::device::{ + hypervisor, + pci_path::{PciPath, PciSlot}, + Device, DeviceType, +}; + pub const SYS_BUS_PCI_DRIVER_PROBE: &str = "/sys/bus/pci/drivers_probe"; pub const SYS_BUS_PCI_DEVICES: &str = "/sys/bus/pci/devices"; pub const SYS_KERN_IOMMU_GROUPS: &str = "/sys/kernel/iommu_groups"; diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/virtio_blk.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/virtio_blk.rs index a93f8553d9..f16fe8eb04 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/virtio_blk.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/virtio_blk.rs @@ -4,10 +4,10 @@ // SPDX-License-Identifier: Apache-2.0 // +use crate::device::pci_path::PciPath; use crate::device::Device; use crate::device::DeviceType; use crate::Hypervisor as hypervisor; -use crate::PciPath; use anyhow::{anyhow, Context, Result}; use async_trait::async_trait; diff --git a/src/runtime-rs/crates/hypervisor/src/device/mod.rs b/src/runtime-rs/crates/hypervisor/src/device/mod.rs index 71b9575dbe..7ad69b2e7a 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/mod.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/mod.rs @@ -17,6 +17,7 @@ use async_trait::async_trait; pub mod device_manager; pub mod driver; +pub mod pci_path; pub mod util; #[derive(Debug)] diff --git a/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs b/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs new file mode 100644 index 0000000000..dea8c0a438 --- /dev/null +++ b/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs @@ -0,0 +1,184 @@ +// Copyright (c) 2019-2023 Alibaba Cloud +// Copyright (c) 2019-2023 Ant Group +// +// SPDX-License-Identifier: Apache-2.0 +// + +use anyhow::{anyhow, Context, Result}; + +// Tips: +// The Re-write `PciSlot` and `PciPath` with rust that it origins from `pcipath.go`: +// + +// The PCI spec reserves 5 bits for slot number (a.k.a. device +// number), giving slots 0..31 +const PCI_SLOT_BITS: u32 = 5; +const MAX_PCI_SLOTS: u32 = (1 << PCI_SLOT_BITS) - 1; + +// A PciSlot describes where a PCI device sits on a single bus +// +// This encapsulates the PCI slot number a.k.a device number, which is +// limited to a 5 bit value [0x00..0x1f] by the PCI specification +// +// To support multifunction device's, It's needed to extend +// this to include the PCI 3-bit function number as well. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct PciSlot(pub u8); + +impl PciSlot { + pub fn convert_from_string(s: &str) -> Result { + if s.is_empty() || s.len() > 2 { + return Err(anyhow!("string given is invalid.")); + } + + let base = 16; + let n = u64::from_str_radix(s, base).context("convert string to number failed")?; + if n >> PCI_SLOT_BITS > 0 { + return Err(anyhow!( + "number {:?} exceeds MAX:{:?}, failed.", + n, + MAX_PCI_SLOTS + )); + } + + Ok(PciSlot(n as u8)) + } + + pub fn convert_from_u32(v: u32) -> Result { + if v > MAX_PCI_SLOTS { + return Err(anyhow!("value {:?} exceeds MAX: {:?}", v, MAX_PCI_SLOTS)); + } + + Ok(PciSlot(v as u8)) + } + + pub fn convert_to_string(&self) -> String { + format!("{:02x}", self.0) + } +} + +// A PciPath describes where a PCI sits in a PCI hierarchy. +// +// Consists of a list of PCI slots, giving the slot of each bridge +// that must be traversed from the PCI root to reach the device, +// followed by the slot of the device itself. +// +// When formatted into a string is written as "xx/.../yy/zz". Here, +// zz is the slot of the device on its PCI bridge, yy is the slot of +// the bridge on its parent bridge and so forth until xx is the slot +// of the "most upstream" bridge on the root bus. +// +// If a device is directly connected to the root bus, which used in +// lightweight hypervisors, such as dragonball/firecracker/clh, and +// its PciPath.slots will contains only one PciSlot. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct PciPath { + // list of PCI slots + pub slots: Vec, +} + +impl PciPath { + // method to format the PciPath into a string + pub fn convert_to_string(&self) -> String { + self.slots + .iter() + .map(|pci_slot| format!("{:02x}", pci_slot.0)) + .collect::>() + .join("/") + } + + // method to parse a PciPath from a string + pub fn convert_from_string(path: &str) -> Result { + if path.is_empty() { + return Err(anyhow!("path given is empty.")); + } + + let mut pci_slots: Vec = Vec::new(); + let slots: Vec<&str> = path.split('/').collect(); + for slot in slots { + match PciSlot::convert_from_string(slot) { + Ok(s) => pci_slots.push(s), + Err(e) => return Err(anyhow!("slot is invalid with: {:?}", e)), + } + } + + Ok(PciPath { slots: pci_slots }) + } + + pub fn from_pci_slots(slots: Vec) -> Option { + if slots.is_empty() { + return None; + } + + Some(PciPath { slots }) + } + + // device_slot to get the slot of the device on its PCI bridge + pub fn get_device_slot(&self) -> Option { + self.slots.last().cloned() + } + + // root_slot to get the slot of the "most upstream" bridge on the root bus + pub fn get_root_slot(&self) -> Option { + self.slots.first().cloned() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_pci_slot() { + // min + let pci_slot_01 = PciSlot::convert_from_string("00"); + assert!(pci_slot_01.is_ok()); + // max + let pci_slot_02 = PciSlot::convert_from_string("1f"); + assert!(pci_slot_02.is_ok()); + + // exceed + let pci_slot_03 = PciSlot::convert_from_string("20"); + assert!(pci_slot_03.is_err()); + + // valid number + let pci_slot_04 = PciSlot::convert_from_u32(1_u32); + assert!(pci_slot_04.is_ok()); + assert_eq!(pci_slot_04.as_ref().unwrap().0, 1_u8); + let pci_slot_str = pci_slot_04.as_ref().unwrap().convert_to_string(); + assert_eq!(pci_slot_str, format!("{:02x}", pci_slot_04.unwrap().0)); + + // max number + let pci_slot_05 = PciSlot::convert_from_u32(31_u32); + assert!(pci_slot_05.is_ok()); + assert_eq!(pci_slot_05.unwrap().0, 31_u8); + + // exceed and error + let pci_slot_06 = PciSlot::convert_from_u32(32_u32); + assert!(pci_slot_06.is_err()); + } + + #[test] + fn test_pci_patch() { + let pci_path_0 = PciPath::convert_from_string("01/0a/05"); + assert!(pci_path_0.is_ok()); + let pci_path_unwrap = pci_path_0.unwrap(); + assert_eq!(pci_path_unwrap.slots[0].0, 1); + assert_eq!(pci_path_unwrap.slots[1].0, 10); + assert_eq!(pci_path_unwrap.slots[2].0, 5); + + let pci_path_01 = PciPath::from_pci_slots(vec![PciSlot(1), PciSlot(10), PciSlot(5)]); + assert!(pci_path_01.is_some()); + let pci_path = pci_path_01.unwrap(); + let pci_path_02 = pci_path.convert_to_string(); + assert_eq!(pci_path_02, "01/0a/05".to_string()); + + let dev_slot = pci_path.get_device_slot(); + assert!(dev_slot.is_some()); + assert_eq!(dev_slot.unwrap().0, 5); + + let root_slot = pci_path.get_root_slot(); + assert!(root_slot.is_some()); + assert_eq!(root_slot.unwrap().0, 1); + } +} From 5cc2890a10bf231294e7c817523550c48e87ae5d Mon Sep 17 00:00:00 2001 From: "alex.lyn" Date: Fri, 22 Dec 2023 10:34:41 +0800 Subject: [PATCH 3/5] runtime-rs: refactor and re-implement pci path. Do refactor and re-implement to make the pci path more "rusty". Fixes: #8665 Signed-off-by: alex.lyn --- .../crates/hypervisor/src/ch/inner_device.rs | 2 +- .../hypervisor/src/device/driver/vfio.rs | 4 +- .../crates/hypervisor/src/device/pci_path.rs | 126 +++++++++++------- .../crates/resource/src/manager_inner.rs | 2 +- 4 files changed, 85 insertions(+), 49 deletions(-) diff --git a/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs b/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs index 6b9cbff22b..e8ca2d3793 100644 --- a/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs +++ b/src/runtime-rs/crates/hypervisor/src/ch/inner_device.rs @@ -277,7 +277,7 @@ impl CloudHypervisorInner { )); } - PciPath::convert_from_string(toks[0]) + PciPath::try_from(toks[0]) } async fn handle_hvsock_device(&mut self, device: HybridVsockDevice) -> Result { diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs index e488281841..3e4534be3a 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs @@ -68,7 +68,7 @@ pub fn generate_guest_pci_path(bdf: String) -> Result { hg_map.write().unwrap().insert(host_bdf, guest_bdf); Ok(PciPath { - slots: vec![PciSlot::convert_from_u32(slot.into()).context("pci slot convert failed.")?], + slots: vec![PciSlot::new(slot)], }) } @@ -501,7 +501,7 @@ impl Device for VfioDevice { self.device_options.push(format!( "0000:{}={}", hostdev.bus_slot_func.clone(), - pci_path.convert_to_string() + pci_path.to_string() )); } } diff --git a/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs b/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs index dea8c0a438..2d0be7647f 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/pci_path.rs @@ -4,6 +4,8 @@ // SPDX-License-Identifier: Apache-2.0 // +use std::convert::TryFrom; + use anyhow::{anyhow, Context, Result}; // Tips: @@ -26,13 +28,30 @@ const MAX_PCI_SLOTS: u32 = (1 << PCI_SLOT_BITS) - 1; pub struct PciSlot(pub u8); impl PciSlot { - pub fn convert_from_string(s: &str) -> Result { + pub fn new(v: u8) -> PciSlot { + PciSlot(v) + } +} + +impl ToString for PciSlot { + fn to_string(&self) -> String { + format!("{:02x}", self.0) + } +} + +impl TryFrom<&str> for PciSlot { + type Error = anyhow::Error; + + fn try_from(s: &str) -> Result { if s.is_empty() || s.len() > 2 { return Err(anyhow!("string given is invalid.")); } let base = 16; - let n = u64::from_str_radix(s, base).context("convert string to number failed")?; + let n = u64::from_str_radix(s, base).context(format!( + "convert string to number with base {:?} failed.", + base + ))?; if n >> PCI_SLOT_BITS > 0 { return Err(anyhow!( "number {:?} exceeds MAX:{:?}, failed.", @@ -43,18 +62,18 @@ impl PciSlot { Ok(PciSlot(n as u8)) } +} - pub fn convert_from_u32(v: u32) -> Result { +impl TryFrom for PciSlot { + type Error = anyhow::Error; + + fn try_from(v: u32) -> Result { if v > MAX_PCI_SLOTS { return Err(anyhow!("value {:?} exceeds MAX: {:?}", v, MAX_PCI_SLOTS)); } Ok(PciSlot(v as u8)) } - - pub fn convert_to_string(&self) -> String { - format!("{:02x}", self.0) - } } // A PciPath describes where a PCI sits in a PCI hierarchy. @@ -78,34 +97,7 @@ pub struct PciPath { } impl PciPath { - // method to format the PciPath into a string - pub fn convert_to_string(&self) -> String { - self.slots - .iter() - .map(|pci_slot| format!("{:02x}", pci_slot.0)) - .collect::>() - .join("/") - } - - // method to parse a PciPath from a string - pub fn convert_from_string(path: &str) -> Result { - if path.is_empty() { - return Err(anyhow!("path given is empty.")); - } - - let mut pci_slots: Vec = Vec::new(); - let slots: Vec<&str> = path.split('/').collect(); - for slot in slots { - match PciSlot::convert_from_string(slot) { - Ok(s) => pci_slots.push(s), - Err(e) => return Err(anyhow!("slot is invalid with: {:?}", e)), - } - } - - Ok(PciPath { slots: pci_slots }) - } - - pub fn from_pci_slots(slots: Vec) -> Option { + pub fn new(slots: Vec) -> Option { if slots.is_empty() { return None; } @@ -124,6 +116,50 @@ impl PciPath { } } +impl ToString for PciPath { + // method to format the PciPath into a string + fn to_string(&self) -> String { + self.slots + .iter() + .map(|pci_slot| format!("{:02x}", pci_slot.0)) + .collect::>() + .join("/") + } +} + +// convert from u32 +impl TryFrom for PciPath { + type Error = anyhow::Error; + + fn try_from(slot: u32) -> Result { + Ok(PciPath { + slots: vec![PciSlot::try_from(slot).context("pci slot convert failed.")?], + }) + } +} + +impl TryFrom<&str> for PciPath { + type Error = anyhow::Error; + + // method to parse a PciPath from a string + fn try_from(path: &str) -> Result { + if path.is_empty() { + return Err(anyhow!("path given is empty.")); + } + + let mut pci_slots: Vec = Vec::new(); + let slots: Vec<&str> = path.split('/').collect(); + for slot in slots { + match PciSlot::try_from(slot) { + Ok(s) => pci_slots.push(s), + Err(e) => return Err(anyhow!("slot is invalid with: {:?}", e)), + } + } + + Ok(PciPath { slots: pci_slots }) + } +} + #[cfg(test)] mod tests { use super::*; @@ -131,46 +167,46 @@ mod tests { #[test] fn test_pci_slot() { // min - let pci_slot_01 = PciSlot::convert_from_string("00"); + let pci_slot_01 = PciSlot::try_from("00"); assert!(pci_slot_01.is_ok()); // max - let pci_slot_02 = PciSlot::convert_from_string("1f"); + let pci_slot_02 = PciSlot::try_from("1f"); assert!(pci_slot_02.is_ok()); // exceed - let pci_slot_03 = PciSlot::convert_from_string("20"); + let pci_slot_03 = PciSlot::try_from("20"); assert!(pci_slot_03.is_err()); // valid number - let pci_slot_04 = PciSlot::convert_from_u32(1_u32); + let pci_slot_04 = PciSlot::try_from(1_u32); assert!(pci_slot_04.is_ok()); assert_eq!(pci_slot_04.as_ref().unwrap().0, 1_u8); - let pci_slot_str = pci_slot_04.as_ref().unwrap().convert_to_string(); + let pci_slot_str = pci_slot_04.as_ref().unwrap().to_string(); assert_eq!(pci_slot_str, format!("{:02x}", pci_slot_04.unwrap().0)); // max number - let pci_slot_05 = PciSlot::convert_from_u32(31_u32); + let pci_slot_05 = PciSlot::try_from(31_u32); assert!(pci_slot_05.is_ok()); assert_eq!(pci_slot_05.unwrap().0, 31_u8); // exceed and error - let pci_slot_06 = PciSlot::convert_from_u32(32_u32); + let pci_slot_06 = PciSlot::try_from(32_u32); assert!(pci_slot_06.is_err()); } #[test] fn test_pci_patch() { - let pci_path_0 = PciPath::convert_from_string("01/0a/05"); + let pci_path_0 = PciPath::try_from("01/0a/05"); assert!(pci_path_0.is_ok()); let pci_path_unwrap = pci_path_0.unwrap(); assert_eq!(pci_path_unwrap.slots[0].0, 1); assert_eq!(pci_path_unwrap.slots[1].0, 10); assert_eq!(pci_path_unwrap.slots[2].0, 5); - let pci_path_01 = PciPath::from_pci_slots(vec![PciSlot(1), PciSlot(10), PciSlot(5)]); + let pci_path_01 = PciPath::new(vec![PciSlot(1), PciSlot(10), PciSlot(5)]); assert!(pci_path_01.is_some()); let pci_path = pci_path_01.unwrap(); - let pci_path_02 = pci_path.convert_to_string(); + let pci_path_02 = pci_path.to_string(); assert_eq!(pci_path_02, "01/0a/05".to_string()); let dev_slot = pci_path.get_device_slot(); diff --git a/src/runtime-rs/crates/resource/src/manager_inner.rs b/src/runtime-rs/crates/resource/src/manager_inner.rs index f266b7966a..2b70fe90c0 100644 --- a/src/runtime-rs/crates/resource/src/manager_inner.rs +++ b/src/runtime-rs/crates/resource/src/manager_inner.rs @@ -313,7 +313,7 @@ impl ResourceManagerInner { // The following would work for drivers virtio-blk-pci and mmio. // Once scsi support is added, need to handle scsi identifiers. let id = if let Some(pci_path) = device.config.pci_path { - pci_path.convert_to_string() + pci_path.to_string() } else { device.config.virt_path.clone() }; From 82d3cfdeda8ce36ebcf625307e76d1d94c4b15b2 Mon Sep 17 00:00:00 2001 From: "alex.lyn" Date: Fri, 22 Dec 2023 10:35:38 +0800 Subject: [PATCH 4/5] runtime-rs: Make VhostUserConfig's field pci_path type more specific Make VhostUserConfig pci_path's type more specific, change it from Option to Option. Fixes: #8665 Signed-off-by: alex.lyn --- .../crates/hypervisor/src/device/driver/vhost_user.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/vhost_user.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/vhost_user.rs index 53258821c5..43b21a1756 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/vhost_user.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/vhost_user.rs @@ -4,6 +4,8 @@ // SPDX-License-Identifier: Apache-2.0 // +use crate::device::pci_path::PciPath; + #[derive(Debug, Clone)] pub enum VhostUserType { /// Blk - represents a block vhostuser device type @@ -50,8 +52,8 @@ pub struct VhostUserConfig { pub device_type: VhostUserType, /// guest block driver pub driver_option: String, - /// pci_addr is the PCI address used to identify the slot at which the drive is attached. - pub pci_addr: Option, + /// pci_path is the PCI Path used to identify the slot at which the device is attached. + pub pci_path: Option, /// Block index of the device if assigned /// type u64 is not OK From 94c83cea84b7431622bf986141a018b39e16f853 Mon Sep 17 00:00:00 2001 From: "alex.lyn" Date: Fri, 22 Dec 2023 10:37:40 +0800 Subject: [PATCH 5/5] runtime-rs: Refactor vfio driver implementation It's important to ensure that these tasks which setup vfio devices are completed before add_device. So Moving vfio device setup code to a dedicated method at device building time which does not affect the behavior of other code. And this change makes it easier to understand the difference between create and attach, and also makes the boundaries clearer. Fixes: #8665 Signed-off-by: alex.lyn --- .../hypervisor/src/device/device_manager.rs | 2 +- .../hypervisor/src/device/driver/vfio.rs | 27 ++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/runtime-rs/crates/hypervisor/src/device/device_manager.rs b/src/runtime-rs/crates/hypervisor/src/device/device_manager.rs index 2b5ef6df54..04dddead35 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/device_manager.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/device_manager.rs @@ -291,7 +291,7 @@ impl DeviceManager { Arc::new(Mutex::new(VfioDevice::new( device_id.clone(), &vfio_dev_config, - ))) + )?)) } DeviceConfig::VhostUserBlkCfg(config) => { // try to find the device, found and just return id. diff --git a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs index 3e4534be3a..aac879a594 100644 --- a/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs +++ b/src/runtime-rs/crates/hypervisor/src/device/driver/vfio.rs @@ -252,7 +252,7 @@ pub struct VfioDevice { impl VfioDevice { // new with VfioConfig - pub fn new(device_id: String, dev_info: &VfioConfig) -> Self { + pub fn new(device_id: String, dev_info: &VfioConfig) -> Result { // devices and device_options are in a 1-1 mapping, used in // vfio-pci handler for kata-agent. let devices: Vec = Vec::with_capacity(MAX_DEV_ID_SIZE); @@ -262,7 +262,7 @@ impl VfioDevice { let dev_type = dev_info.dev_type.as_str(); let driver_type = VfioBusMode::driver_type(dev_type).to_owned(); - Self { + let mut vfio_device = Self { device_id, attach_count: 0, bus_mode: VfioBusMode::PCI, @@ -270,7 +270,13 @@ impl VfioDevice { config: dev_info.clone(), devices, device_options, - } + }; + + vfio_device + .initialize_vfio_device() + .context("initialize vfio device failed.")?; + + Ok(vfio_device) } fn get_host_path(&self) -> String { @@ -370,7 +376,7 @@ impl VfioDevice { Ok(DeviceVendor(device, vendor)) } - async fn set_vfio_config( + fn set_vfio_config( &mut self, iommu_devs_path: PathBuf, device_name: &str, @@ -422,11 +428,8 @@ impl VfioDevice { _ => None, } } -} -#[async_trait] -impl Device for VfioDevice { - async fn attach(&mut self, h: &dyn hypervisor) -> Result<()> { + fn initialize_vfio_device(&mut self) -> Result<()> { // host path: /dev/vfio/X let host_path = self.get_host_path(); // vfio group: X @@ -460,7 +463,6 @@ impl Device for VfioDevice { let mut hostdev: HostDevice = self .set_vfio_config(iommu_devs_path.clone(), device) - .await .context("set vfio config failed")?; let dev_prefix = self.get_vfio_prefix(); hostdev.hostdev_id = make_device_nameid(&dev_prefix, index, MAX_DEV_ID_SIZE); @@ -468,6 +470,13 @@ impl Device for VfioDevice { self.devices.push(hostdev); } + Ok(()) + } +} + +#[async_trait] +impl Device for VfioDevice { + async fn attach(&mut self, h: &dyn hypervisor) -> Result<()> { if self .increase_attach_count() .await