Skip to content

Commit

Permalink
PciPath to Bdf conversion is infallible; prove it and refactor (#774)
Browse files Browse the repository at this point in the history
  • Loading branch information
@iximeow
iximeow authored Oct 1, 2024
1 parent f255595 commit 5a71bed
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 84 deletions.
81 changes: 9 additions & 72 deletions bin/propolis-server/src/lib/initializer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,18 +200,10 @@ impl<'a> MachineInitializer<'a> {
event_handler: &Arc<dyn super::vm::guest_event::ChipsetEventHandler>,
) -> Result<RegisteredChipset, Error> {
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)?;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand All @@ -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(())
}
Expand All @@ -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(),
Expand All @@ -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(())
}

Expand Down Expand Up @@ -1017,30 +970,14 @@ 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::Bdf, Error> =
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
// would happen is ill-understood. So, only check disks here.
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,
Expand All @@ -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,
Expand Down
53 changes: 41 additions & 12 deletions lib/propolis/src/hw/pci/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -194,18 +193,48 @@ impl FromStr for Bdf {
}
}

impl TryFrom<propolis_types::PciPath> for Bdf {
type Error = std::io::Error;
// The corresponding `propolis_types_pcipath_is_always_valid_bdf` validates this
// `From` impl exhaustively.
impl From<propolis_types::PciPath> 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<Self, Self::Error> {
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
);
}
}
}
}

Expand Down

0 comments on commit 5a71bed

Please sign in to comment.