From 8528157b9bce59f5c3b2260221fb3fb338c70e3b Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 28 Sep 2021 17:00:49 +1000 Subject: [PATCH 1/3] agent/pci: Extend Slot type to represent PCI function as well pci::Slot represents a PCI slot. However, in all cases where we use it, we actually care about addressing a specific PCI function. So, at the moment we can only refer to function 0 in each slot. Replace pci::Slot with pci::SlotFn to represent both the slot and function. Signed-off-by: David Gibson --- src/agent/src/device.rs | 2 +- src/agent/src/pci.rs | 217 ++++++++++++++++++++++++++++------------ src/agent/src/rpc.rs | 2 +- 3 files changed, 157 insertions(+), 64 deletions(-) diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index 067c6d03d..93cf50cd5 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -71,7 +71,7 @@ pub fn pcipath_to_sysfs(root_bus_sysfs: &str, pcipath: &pci::Path) -> Result + fmt::Display + Copy>(v: T) -> anyhow::Result { - if let Ok(v8) = v.try_into() { - if v8 <= SLOT_MAX { - return Ok(Slot(v8)); +// Represents a PCI function's slot (a.k.a. device) and function +// numbers, giving its location on a single logical bus +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub struct SlotFn(u8); + +impl SlotFn { + pub fn new(ss: T, f: U) -> anyhow::Result + where + T: TryInto + fmt::Display + Copy, + U: TryInto + fmt::Display + Copy, + { + let ss8 = match ss.try_into() { + Ok(ss8) if ss8 <= SLOT_MAX => ss8, + _ => { + return Err(anyhow!( + "PCI slot {} should be in range [0..{:#x}]", + ss, + SLOT_MAX + )); } - } - Err(anyhow!( - "PCI slot {} should be in range [0..{:#x}]", - v, - SLOT_MAX - )) + }; + + let f8 = match f.try_into() { + Ok(f8) if f8 <= FUNCTION_MAX => f8, + _ => { + return Err(anyhow!( + "PCI function {} should be in range [0..{:#x}]", + f, + FUNCTION_MAX + )); + } + }; + + Ok(SlotFn(ss8 << FUNCTION_BITS | f8)) + } + + pub fn slot(self) -> u8 { + self.0 >> FUNCTION_BITS + } + + pub fn function(self) -> u8 { + self.0 & FUNCTION_MAX } } -impl FromStr for Slot { +impl FromStr for SlotFn { type Err = anyhow::Error; fn from_str(s: &str) -> anyhow::Result { - let v = isize::from_str_radix(s, 16)?; - Slot::new(v) + let mut tokens = s.split('.').fuse(); + let slot = tokens.next(); + let func = tokens.next(); + + if slot.is_none() || tokens.next().is_some() { + return Err(anyhow!( + "PCI slot/function {} should have the format SS.F", + s + )); + } + + let slot = isize::from_str_radix(slot.unwrap(), 16)?; + let func = match func { + Some(func) => isize::from_str_radix(func, 16)?, + None => 0, + }; + + SlotFn::new(slot, func) } } -impl fmt::Display for Slot { +impl fmt::Display for SlotFn { fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { - write!(f, "{:02x}", self.0) + write!(f, "{:02x}.{:01x}", self.slot(), self.function()) } } #[derive(Clone, Debug, PartialEq, Eq)] -pub struct Path(Vec); +pub struct Path(Vec); impl Path { - pub fn new(slots: Vec) -> anyhow::Result { + pub fn new(slots: Vec) -> anyhow::Result { if slots.is_empty() { return Err(anyhow!("PCI path must have at least one element")); } @@ -63,7 +108,7 @@ impl Path { // Let Path be treated as a slice of Slots impl Deref for Path { - type Target = [Slot]; + type Target = [SlotFn]; fn deref(&self) -> &Self::Target { &self.0 @@ -85,83 +130,131 @@ impl FromStr for Path { type Err = anyhow::Error; fn from_str(s: &str) -> anyhow::Result { - let rslots: anyhow::Result> = s.split('/').map(Slot::from_str).collect(); + let rslots: anyhow::Result> = s.split('/').map(SlotFn::from_str).collect(); Path::new(rslots?) } } #[cfg(test)] mod tests { - use crate::pci::{Path, Slot}; + use super::*; use std::str::FromStr; #[test] - fn test_slot() { + fn test_slotfn() { // Valid slots - let slot = Slot::new(0x00).unwrap(); - assert_eq!(format!("{}", slot), "00"); + let sf = SlotFn::new(0x00, 0x0).unwrap(); + assert_eq!(format!("{}", sf), "00.0"); - let slot = Slot::from_str("00").unwrap(); - assert_eq!(format!("{}", slot), "00"); + let sf = SlotFn::from_str("00.0").unwrap(); + assert_eq!(format!("{}", sf), "00.0"); - let slot = Slot::new(31).unwrap(); - let slot2 = Slot::from_str("1f").unwrap(); - assert_eq!(slot, slot2); + let sf = SlotFn::from_str("00").unwrap(); + assert_eq!(format!("{}", sf), "00.0"); + + let sf = SlotFn::new(31, 7).unwrap(); + let sf2 = SlotFn::from_str("1f.7").unwrap(); + assert_eq!(sf, sf2); // Bad slots - let slot = Slot::new(-1); - assert!(slot.is_err()); + let sf = SlotFn::new(-1, 0); + assert!(sf.is_err()); - let slot = Slot::new(32); - assert!(slot.is_err()); + let sf = SlotFn::new(32, 0); + assert!(sf.is_err()); - let slot = Slot::from_str("20"); - assert!(slot.is_err()); + let sf = SlotFn::from_str("20.0"); + assert!(sf.is_err()); - let slot = Slot::from_str("xy"); - assert!(slot.is_err()); + let sf = SlotFn::from_str("20"); + assert!(sf.is_err()); - let slot = Slot::from_str("00/"); - assert!(slot.is_err()); + let sf = SlotFn::from_str("xy.0"); + assert!(sf.is_err()); - let slot = Slot::from_str(""); - assert!(slot.is_err()); + let sf = SlotFn::from_str("xy"); + assert!(sf.is_err()); + + // Bad functions + let sf = SlotFn::new(0, -1); + assert!(sf.is_err()); + + let sf = SlotFn::new(0, 8); + assert!(sf.is_err()); + + let sf = SlotFn::from_str("00.8"); + assert!(sf.is_err()); + + let sf = SlotFn::from_str("00.x"); + assert!(sf.is_err()); + + // Bad formats + let sf = SlotFn::from_str(""); + assert!(sf.is_err()); + + let sf = SlotFn::from_str("00.0.0"); + assert!(sf.is_err()); + + let sf = SlotFn::from_str("00.0/"); + assert!(sf.is_err()); + + let sf = SlotFn::from_str("00/"); + assert!(sf.is_err()); } #[test] fn test_path() { - let slot3 = Slot::new(0x03).unwrap(); - let slot4 = Slot::new(0x04).unwrap(); - let slot5 = Slot::new(0x05).unwrap(); + let sf3_0 = SlotFn::new(0x03, 0).unwrap(); + let sf4_0 = SlotFn::new(0x04, 0).unwrap(); + let sf5_0 = SlotFn::new(0x05, 0).unwrap(); + let sfa_5 = SlotFn::new(0x0a, 5).unwrap(); + let sfb_6 = SlotFn::new(0x0b, 6).unwrap(); + let sfc_7 = SlotFn::new(0x0c, 7).unwrap(); // Valid paths - let pcipath = Path::new(vec![slot3]).unwrap(); - assert_eq!(format!("{}", pcipath), "03"); + let pcipath = Path::new(vec![sf3_0]).unwrap(); + assert_eq!(format!("{}", pcipath), "03.0"); + let pcipath2 = Path::from_str("03.0").unwrap(); + assert_eq!(pcipath, pcipath2); let pcipath2 = Path::from_str("03").unwrap(); assert_eq!(pcipath, pcipath2); assert_eq!(pcipath.len(), 1); - assert_eq!(pcipath[0], slot3); + assert_eq!(pcipath[0], sf3_0); - let pcipath = Path::new(vec![slot3, slot4]).unwrap(); - assert_eq!(format!("{}", pcipath), "03/04"); + let pcipath = Path::new(vec![sf3_0, sf4_0]).unwrap(); + assert_eq!(format!("{}", pcipath), "03.0/04.0"); + let pcipath2 = Path::from_str("03.0/04.0").unwrap(); + assert_eq!(pcipath, pcipath2); let pcipath2 = Path::from_str("03/04").unwrap(); assert_eq!(pcipath, pcipath2); assert_eq!(pcipath.len(), 2); - assert_eq!(pcipath[0], slot3); - assert_eq!(pcipath[1], slot4); + assert_eq!(pcipath[0], sf3_0); + assert_eq!(pcipath[1], sf4_0); - let pcipath = Path::new(vec![slot3, slot4, slot5]).unwrap(); - assert_eq!(format!("{}", pcipath), "03/04/05"); + let pcipath = Path::new(vec![sf3_0, sf4_0, sf5_0]).unwrap(); + assert_eq!(format!("{}", pcipath), "03.0/04.0/05.0"); + let pcipath2 = Path::from_str("03.0/04.0/05.0").unwrap(); + assert_eq!(pcipath, pcipath2); let pcipath2 = Path::from_str("03/04/05").unwrap(); assert_eq!(pcipath, pcipath2); assert_eq!(pcipath.len(), 3); - assert_eq!(pcipath[0], slot3); - assert_eq!(pcipath[1], slot4); - assert_eq!(pcipath[2], slot5); + assert_eq!(pcipath[0], sf3_0); + assert_eq!(pcipath[1], sf4_0); + assert_eq!(pcipath[2], sf5_0); + + let pcipath = Path::new(vec![sfa_5, sfb_6, sfc_7]).unwrap(); + assert_eq!(format!("{}", pcipath), "0a.5/0b.6/0c.7"); + let pcipath2 = Path::from_str("0a.5/0b.6/0c.7").unwrap(); + assert_eq!(pcipath, pcipath2); + assert_eq!(pcipath.len(), 3); + assert_eq!(pcipath[0], sfa_5); + assert_eq!(pcipath[1], sfb_6); + assert_eq!(pcipath[2], sfc_7); // Bad paths assert!(Path::new(vec!()).is_err()); assert!(Path::from_str("20").is_err()); + assert!(Path::from_str("00.8").is_err()); assert!(Path::from_str("//").is_err()); assert!(Path::from_str("xyz").is_err()); } diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index d48815db6..e4665ac36 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -1606,7 +1606,7 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { async fn do_add_swap(sandbox: &Arc>, req: &AddSwapRequest) -> Result<()> { let mut slots = Vec::new(); for slot in &req.PCIPath { - slots.push(pci::Slot::new(*slot)?); + slots.push(pci::SlotFn::new(*slot, 0)?); } let pcipath = pci::Path::new(slots)?; let dev_name = get_virtio_blk_pci_device_name(sandbox, &pcipath).await?; From e50b05d93cb97ee982e93eab273c0a5ba458d712 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 1 Oct 2021 17:21:05 +1000 Subject: [PATCH 2/3] agent/pci: Add type to represent PCI addresses Add a new pci::Address type which represents a guest PCI address in DDDD:BB:SS.F form. fixes #2745 Signed-off-by: David Gibson --- src/agent/src/pci.rs | 86 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/src/agent/src/pci.rs b/src/agent/src/pci.rs index 3b2b7ef83..25ad9a6a0 100644 --- a/src/agent/src/pci.rs +++ b/src/agent/src/pci.rs @@ -94,6 +94,53 @@ impl fmt::Display for SlotFn { } } +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub struct Address { + domain: u16, + bus: u8, + slotfn: SlotFn, +} + +impl Address { + pub fn new(domain: u16, bus: u8, slotfn: SlotFn) -> Self { + Address { + domain, + bus, + slotfn, + } + } +} + +impl FromStr for Address { + type Err = anyhow::Error; + + fn from_str(s: &str) -> anyhow::Result { + let mut tokens = s.split(':').fuse(); + let domain = tokens.next(); + let bus = tokens.next(); + let slotfn = tokens.next(); + + if domain.is_none() || bus.is_none() || slotfn.is_none() || tokens.next().is_some() { + return Err(anyhow!( + "PCI address {} should have the format DDDD:BB:SS.F", + s + )); + } + + let domain = u16::from_str_radix(domain.unwrap(), 16)?; + let bus = u8::from_str_radix(bus.unwrap(), 16)?; + let slotfn = SlotFn::from_str(slotfn.unwrap())?; + + Ok(Address::new(domain, bus, slotfn)) + } +} + +impl fmt::Display for Address { + fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { + write!(f, "{:04x}:{:02x}:{}", self.domain, self.bus, self.slotfn) + } +} + #[derive(Clone, Debug, PartialEq, Eq)] pub struct Path(Vec); @@ -202,6 +249,45 @@ mod tests { assert!(sf.is_err()); } + #[test] + fn test_address() { + // Valid addresses + let sf0_0 = SlotFn::new(0, 0).unwrap(); + let sf1f_7 = SlotFn::new(0x1f, 7).unwrap(); + + let addr = Address::new(0, 0, sf0_0); + assert_eq!(format!("{}", addr), "0000:00:00.0"); + let addr2 = Address::from_str("0000:00:00.0").unwrap(); + assert_eq!(addr, addr2); + + let addr = Address::new(0xffff, 0xff, sf1f_7); + assert_eq!(format!("{}", addr), "ffff:ff:1f.7"); + let addr2 = Address::from_str("ffff:ff:1f.7").unwrap(); + assert_eq!(addr, addr2); + + // Bad addresses + let addr = Address::from_str("10000:00:00.0"); + assert!(addr.is_err()); + + let addr = Address::from_str("0000:100:00.0"); + assert!(addr.is_err()); + + let addr = Address::from_str("0000:00:20.0"); + assert!(addr.is_err()); + + let addr = Address::from_str("0000:00:00.8"); + assert!(addr.is_err()); + + let addr = Address::from_str("xyz"); + assert!(addr.is_err()); + + let addr = Address::from_str("xyxy:xy:xy.z"); + assert!(addr.is_err()); + + let addr = Address::from_str("0000:00:00.0:00"); + assert!(addr.is_err()); + } + #[test] fn test_path() { let sf3_0 = SlotFn::new(0x03, 0).unwrap(); From 72044180e48eb11f0c5d53f8243fd0cb0e03ae84 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 5 Oct 2021 14:33:03 +1100 Subject: [PATCH 3/3] agent/device: Return PCI address from wait_for_pci_device() wait_for_pci_device() waits for the PCI device at the given path to become ready, but it doesn't currently give you any meaningful handle on that device. Change the signature, so that it returns the PCI address of the device. Signed-off-by: David Gibson --- src/agent/src/device.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index 93cf50cd5..3258be116 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -277,13 +277,23 @@ impl UeventMatcher for PciMatcher { } } -pub async fn wait_for_pci_device(sandbox: &Arc>, pcipath: &pci::Path) -> Result<()> { +pub async fn wait_for_pci_device( + sandbox: &Arc>, + pcipath: &pci::Path, +) -> Result { let root_bus_sysfs = format!("{}{}", SYSFS_DIR, create_pci_root_bus_path()); let sysfs_rel_path = pcipath_to_sysfs(&root_bus_sysfs, pcipath)?; let matcher = PciMatcher::new(&sysfs_rel_path)?; - let _ = wait_for_uevent(sandbox, matcher).await?; - Ok(()) + let uev = wait_for_uevent(sandbox, matcher).await?; + + let addr = uev + .devpath + .rsplit('/') + .next() + .ok_or_else(|| anyhow!("Bad device path {:?} in uevent", &uev.devpath))?; + let addr = pci::Address::from_str(addr)?; + Ok(addr) } /// Scan SCSI bus for the given SCSI address(SCSI-Id and LUN)