From 5a71bede2f3497e4afef9df5103db119cf7f21ad Mon Sep 17 00:00:00 2001 From: iximeow Date: Tue, 1 Oct 2024 20:13:47 +0000 Subject: [PATCH] PciPath to Bdf conversion is infallible; prove it and refactor (#774) --- bin/propolis-server/src/lib/initializer.rs | 81 +++------------------- lib/propolis/src/hw/pci/mod.rs | 53 ++++++++++---- 2 files changed, 50 insertions(+), 84 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index bd8eb4f2a..4ba74f1a8 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -200,18 +200,10 @@ impl<'a> MachineInitializer<'a> { event_handler: &Arc, ) -> Result { let mut pci_builder = pci::topology::Builder::new(); - for (name, bridge) in &self.spec.pci_pci_bridges { + for bridge in self.spec.pci_pci_bridges.values() { let desc = pci::topology::BridgeDescription::new( pci::topology::LogicalBusId(bridge.downstream_bus), - bridge.pci_path.try_into().map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Couldn't get PCI BDF for bridge {}: {}", - name, e - ), - ) - })?, + bridge.pci_path.into(), ); pci_builder.add_bridge(desc)?; } @@ -524,15 +516,7 @@ impl<'a> MachineInitializer<'a> { } }; - let bdf: pci::Bdf = pci_path.try_into().map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Couldn't get PCI BDF for storage device {}: {}", - disk_name, e - ), - ) - })?; + let bdf: pci::Bdf = pci_path.into(); let StorageBackendInstance { be: backend, crucible } = self .create_storage_backend_from_spec( @@ -638,16 +622,7 @@ impl<'a> MachineInitializer<'a> { for (device_name, nic) in &self.spec.nics { info!(self.log, "Creating vNIC {}", device_name); - let bdf: pci::Bdf = - nic.device_spec.pci_path.try_into().map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Couldn't get PCI BDF for vNIC {}: {}", - device_name, e - ), - ) - })?; + let bdf: pci::Bdf = nic.device_spec.pci_path.into(); let viona = virtio::PciVirtioViona::new( &nic.backend_spec.vnic_name, @@ -789,16 +764,7 @@ impl<'a> MachineInitializer<'a> { ) })? .pci_path - .try_into() - .map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Couldn't get PCI BDF for SoftNpu p9 device: {}", - e - ), - ) - })?; + .into(); chipset.pci_attach(bdf, vio9p.clone()); // Create the SoftNpu device. @@ -821,15 +787,9 @@ impl<'a> MachineInitializer<'a> { self.devices.insert("softnpu-main".to_string(), softnpu.clone()); // Create the SoftNpu PCI port. - let bdf: pci::Bdf = pci_port.pci_path.try_into().map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!("Couldn't get PCI BDF for SoftNpu pci port: {}", e), - ) - })?; self.devices .insert("softnpu-pciport".to_string(), softnpu.pci_port.clone()); - chipset.pci_attach(bdf, softnpu.pci_port.clone()); + chipset.pci_attach(pci_port.pci_path.into(), softnpu.pci_port.clone()); Ok(()) } @@ -846,13 +806,6 @@ impl<'a> MachineInitializer<'a> { return Ok(()); }; - let bdf: pci::Bdf = p9fs.pci_path.try_into().map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!("Couldn't get PCI BDF for p9fs device: {}", e), - ) - })?; - let handler = virtio::p9fs::HostFSHandler::new( p9fs.source.to_owned(), p9fs.target.to_owned(), @@ -861,7 +814,7 @@ impl<'a> MachineInitializer<'a> { ); let vio9p = virtio::p9fs::PciVirtio9pfs::new(0x40, Arc::new(handler)); self.devices.insert("falcon-p9fs".to_string(), vio9p.clone()); - chipset.pci_attach(bdf, vio9p); + chipset.pci_attach(p9fs.pci_path.into(), vio9p); Ok(()) } @@ -1017,22 +970,6 @@ impl<'a> MachineInitializer<'a> { let mut order = fwcfg::formats::BootOrder::new(); - let parse_bdf = - |pci_path: &propolis_api_types::instance_spec::PciPath| { - let bdf: Result = - pci_path.to_owned().try_into().map_err(|e| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Couldn't get PCI BDF for {}: {}", - pci_path, e - ), - ) - }); - - bdf - }; - for boot_entry in boot_names.order.iter() { // Theoretically we could support booting from network devices by // matching them here and adding their PCI paths, but exactly what @@ -1040,7 +977,7 @@ impl<'a> MachineInitializer<'a> { if let Some(spec) = self.spec.disks.get(boot_entry.name.as_str()) { match &spec.device_spec { StorageDevice::Virtio(disk) => { - let bdf = parse_bdf(&disk.pci_path)?; + let bdf: pci::Bdf = disk.pci_path.into(); if bdf.bus.get() != 0 { return Err(Error::new( ErrorKind::InvalidInput, @@ -1051,7 +988,7 @@ impl<'a> MachineInitializer<'a> { order.add_disk(bdf.location); } StorageDevice::Nvme(disk) => { - let bdf = parse_bdf(&disk.pci_path)?; + let bdf: pci::Bdf = disk.pci_path.into(); if bdf.bus.get() != 0 { return Err(Error::new( ErrorKind::InvalidInput, diff --git a/lib/propolis/src/hw/pci/mod.rs b/lib/propolis/src/hw/pci/mod.rs index 161d018c0..95e5febc3 100644 --- a/lib/propolis/src/hw/pci/mod.rs +++ b/lib/propolis/src/hw/pci/mod.rs @@ -2,7 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use std::convert::TryFrom; use std::fmt::Result as FmtResult; use std::fmt::{Display, Formatter}; use std::io::{Error, ErrorKind}; @@ -194,18 +193,48 @@ impl FromStr for Bdf { } } -impl TryFrom for Bdf { - type Error = std::io::Error; +// The corresponding `propolis_types_pcipath_is_always_valid_bdf` validates this +// `From` impl exhaustively. +impl From for Bdf { + fn from(value: propolis_types::PciPath) -> Self { + // PciPath is exactly as well-formed as Bdf + Bdf::new_unchecked(value.bus(), value.device(), value.function()) + } +} - fn try_from(value: propolis_types::PciPath) -> Result { - Bdf::new(value.bus(), value.device(), value.function()).ok_or_else( - || { - Error::new( - ErrorKind::InvalidInput, - "Failed to convert raw PCI path to BDF".to_string(), - ) - }, - ) +#[test] +fn propolis_types_pcipath_is_always_valid_bdf() { + fn check_bdf(bus: u8, device: u8, function: u8) -> bool { + let bdf = Bdf::new(bus, device, function); + let pci_path = propolis_types::PciPath::new(bus, device, function); + + match (bdf, pci_path) { + (None, Err(_)) => { + return true; + } + (Some(_), Err(_)) | (None, Ok(_)) => { + return false; + } + (Some(bdf), Ok(pci_path)) => { + let converted_bdf: Bdf = pci_path.into(); + converted_bdf == bdf + } + } + } + + for bus in 0..=255 { + for device in 0..=255 { + for function in 0..=255 { + assert!( + check_bdf(bus, device, function), + "Bdf and PciPath did not match for \ + bus/device/function {}/{}/{}", + bus, + device, + function + ); + } + } } }