From 93694a455069072751b8904a90d9228d96f95799 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 25 Jul 2024 19:19:50 +0000 Subject: [PATCH 01/11] refactor: use InstanceSpecV0 in fewer places Implement conversions between InstanceSpecV0 and the internal spec type and start converting uses of the former to the latter. The notable exception is machine initialization, which still relies on an InstanceSpecV0. --- .../src/lib/migrate/destination.rs | 4 +- bin/propolis-server/src/lib/server.rs | 46 +++- .../src/lib/spec/api_request.rs | 33 +-- .../src/lib/spec/api_spec_v0.rs | 256 ++++++++++++++++++ bin/propolis-server/src/lib/spec/builder.rs | 218 +++++++-------- .../src/lib/spec/config_toml.rs | 69 ++--- bin/propolis-server/src/lib/spec/mod.rs | 109 ++++++-- bin/propolis-server/src/lib/vm/ensure.rs | 32 ++- bin/propolis-server/src/lib/vm/mod.rs | 12 +- .../src/lib/vm/state_driver.rs | 8 +- 10 files changed, 540 insertions(+), 247 deletions(-) create mode 100644 bin/propolis-server/src/lib/spec/api_spec_v0.rs diff --git a/bin/propolis-server/src/lib/migrate/destination.rs b/bin/propolis-server/src/lib/migrate/destination.rs index 7f23beb15..21b12f09c 100644 --- a/bin/propolis-server/src/lib/migrate/destination.rs +++ b/bin/propolis-server/src/lib/migrate/destination.rs @@ -346,8 +346,8 @@ impl RonV0 { }?; info!(self.log(), "Destination read Preamble: {:?}", preamble); - if let Err(e) = - preamble.is_migration_compatible(ensure_ctx.instance_spec()) + if let Err(e) = preamble + .is_migration_compatible(&ensure_ctx.instance_spec().clone().into()) { error!( self.log(), diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index 6dd0c9066..084230f9d 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -17,6 +17,9 @@ use std::net::SocketAddrV6; use std::sync::Arc; use crate::serial::history_buffer::SerialHistoryOffset; +use crate::spec; +use crate::spec::api_spec_v0::ApiSpecParseError; +use crate::vm::ensure::VmEnsureRequest; use crate::vm::VmError; use dropshot::{ channel, endpoint, ApiDescription, HttpError, HttpResponseCreated, @@ -29,8 +32,10 @@ use internal_dns::ServiceName; pub use nexus_client::Client as NexusClient; use oximeter::types::ProducerRegistry; use propolis_api_types as api; -use propolis_api_types::instance_spec::components::devices::QemuPvpanic; -use propolis_api_types::instance_spec::{self, VersionedInstanceSpec}; +use propolis_api_types::instance_spec::VersionedInstanceSpec; +use propolis_api_types::instance_spec::{ + self, components::devices::QemuPvpanic, +}; pub use propolis_server_config::Config as VmTomlConfig; use rfb::tungstenite::BinaryWs; @@ -107,7 +112,7 @@ impl DropshotEndpointContext { fn instance_spec_from_request( request: &api::InstanceEnsureRequest, toml_config: &VmTomlConfig, -) -> Result { +) -> Result { let mut spec_builder = SpecBuilder::new(&request.properties); spec_builder.add_devices_from_config(toml_config)?; @@ -135,8 +140,14 @@ fn instance_spec_from_request( spec_builder.add_serial_port(port)?; } - spec_builder.add_pvpanic_device(QemuPvpanic { enable_isa: true })?; - Ok(VersionedInstanceSpec::V0(spec_builder.finish())) + #[cfg(feature = "falcon")] + spec_builder.set_softnpu_com4()?; + + spec_builder.add_pvpanic_device(spec::QemuPvpanic(QemuPvpanic { + enable_isa: true, + }))?; + + Ok(spec_builder.finish()) } /// Wrapper around a [`NexusClient`] object, which allows deferring @@ -217,14 +228,16 @@ async fn find_local_nexus_client( async fn instance_ensure_common( rqctx: RequestContext>, - request: api::InstanceSpecEnsureRequest, + properties: api::InstanceProperties, + migrate: Option, + instance_spec: spec::Spec, ) -> Result, HttpError> { let server_context = rqctx.context(); let oximeter_registry = server_context .static_config .metrics .as_ref() - .map(|_| ProducerRegistry::with_id(request.properties.id)); + .map(|_| ProducerRegistry::with_id(properties.id)); let nexus_client = find_local_nexus_client(rqctx.server.local_addr, rqctx.log.clone()) @@ -240,6 +253,7 @@ async fn instance_ensure_common( local_server_addr: rqctx.server.local_addr, }; + let request = VmEnsureRequest { properties, migrate, instance_spec }; server_context .vm .ensure(&server_context.log, request, ensure_options) @@ -289,11 +303,9 @@ async fn instance_ensure( instance_ensure_common( rqctx, - api::InstanceSpecEnsureRequest { - properties: request.properties, - instance_spec, - migrate: request.migrate, - }, + request.properties, + request.migrate, + instance_spec, ) .await } @@ -306,7 +318,15 @@ async fn instance_spec_ensure( rqctx: RequestContext>, request: TypedBody, ) -> Result, HttpError> { - instance_ensure_common(rqctx, request.into_inner()).await + let request = request.into_inner(); + let VersionedInstanceSpec::V0(v0_spec) = request.instance_spec; + let spec = + spec::Spec::try_from(v0_spec).map_err(|e: ApiSpecParseError| { + HttpError::for_bad_request(None, e.to_string()) + })?; + + instance_ensure_common(rqctx, request.properties, request.migrate, spec) + .await } async fn instance_get_common( diff --git a/bin/propolis-server/src/lib/spec/api_request.rs b/bin/propolis-server/src/lib/spec/api_request.rs index 7c84be5a5..b83cb0337 100644 --- a/bin/propolis-server/src/lib/spec/api_request.rs +++ b/bin/propolis-server/src/lib/spec/api_request.rs @@ -25,7 +25,7 @@ use propolis_api_types::{ }; use thiserror::Error; -use super::{ParsedNetworkDevice, ParsedStorageDevice}; +use super::{Disk, Nic}; #[derive(Debug, Error)] pub(crate) enum DeviceRequestError { @@ -71,7 +71,7 @@ fn slot_to_pci_path( pub(super) fn parse_disk_from_request( disk: &DiskRequest, -) -> Result { +) -> Result { let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?; let backend_name = format!("{}-backend", disk.name); let device_spec = match disk.device.as_ref() { @@ -100,17 +100,12 @@ pub(super) fn parse_disk_from_request( readonly: disk.read_only, }); - Ok(ParsedStorageDevice { - device_name, - device_spec, - backend_name, - backend_spec, - }) + Ok(Disk { device_name, device_spec, backend_name, backend_spec }) } pub(super) fn parse_cloud_init_from_request( base64: String, -) -> Result { +) -> Result { let name = "cloud-init"; let pci_path = slot_to_pci_path(Slot(0), SlotType::CloudInit)?; let backend_name = name.to_string(); @@ -123,17 +118,12 @@ pub(super) fn parse_cloud_init_from_request( pci_path, }); - Ok(ParsedStorageDevice { - device_name, - device_spec, - backend_name, - backend_spec, - }) + Ok(Disk { device_name, device_spec, backend_name, backend_spec }) } pub(super) fn parse_nic_from_request( nic: &NetworkInterfaceRequest, -) -> Result { +) -> Result { let pci_path = slot_to_pci_path(nic.slot, SlotType::Nic)?; let (device_name, backend_name) = super::pci_path_to_nic_names(pci_path); let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic { @@ -146,12 +136,7 @@ pub(super) fn parse_nic_from_request( vnic_name: nic.name.to_string(), }); - Ok(ParsedNetworkDevice { - device_name, - device_spec, - backend_name, - backend_spec, - }) + Ok(Nic { device_name, device_spec, backend_name, backend_spec }) } #[cfg(test)] @@ -161,9 +146,7 @@ mod test { use super::*; - fn check_parsed_storage_device_backend_pointer( - parsed: &ParsedStorageDevice, - ) { + fn check_parsed_storage_device_backend_pointer(parsed: &Disk) { let device_to_backend = match &parsed.device_spec { StorageDeviceV0::VirtioDisk(d) => d.backend_name.clone(), StorageDeviceV0::NvmeDisk(d) => d.backend_name.clone(), diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs new file mode 100644 index 000000000..acc93c1a9 --- /dev/null +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -0,0 +1,256 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +//! Conversions from version-0 instance specs in the [`propolis_api_types`] +//! crate to the internal [`super::Spec`] representation. + +use propolis_api_types::instance_spec::{ + components::devices::{SerialPort as SerialPortSpec, SerialPortNumber}, + v0::{InstanceSpecV0, NetworkDeviceV0, StorageDeviceV0}, +}; +use thiserror::Error; + +#[cfg(feature = "falcon")] +use propolis_api_types::instance_spec::{ + components::devices::SoftNpuPort as SoftNpuPortSpec, v0::NetworkBackendV0, +}; + +#[cfg(feature = "falcon")] +use crate::spec::SoftNpuPort; + +use super::{ + builder::{SpecBuilder, SpecBuilderError}, + Disk, Nic, PciPciBridge, QemuPvpanic, SerialPort, Spec, +}; + +#[derive(Debug, Error)] +pub(crate) enum ApiSpecParseError { + #[error(transparent)] + BuilderError(#[from] SpecBuilderError), + + #[error("backend {0} not found for device {1}")] + BackendNotFound(String, String), + + #[error("backend {0} not used by any device")] + BackendNotUsed(String), + + #[cfg(feature = "falcon")] + #[error("network backend for device {0} is not a DLPI backend")] + NotDlpiBackend(String), +} + +impl From for InstanceSpecV0 { + fn from(val: Spec) -> Self { + let mut spec = InstanceSpecV0::default(); + + spec.devices.board = val.board; + for disk in val.disks { + let _old = spec + .devices + .storage_devices + .insert(disk.device_name, disk.device_spec); + + assert!(_old.is_none()); + + let _old = spec + .backends + .storage_backends + .insert(disk.backend_name, disk.backend_spec); + + assert!(_old.is_none()); + } + + for nic in val.nics { + let _old = spec + .devices + .network_devices + .insert(nic.device_name, nic.device_spec); + + assert!(_old.is_none()); + + let _old = spec + .backends + .network_backends + .insert(nic.backend_name, nic.backend_spec); + + assert!(_old.is_none()); + } + + for (index, serial) in val.serial.iter().enumerate() { + if *serial == SerialPort::Enabled { + let _old = spec.devices.serial_ports.insert( + format!("com{}", index + 1), + SerialPortSpec { + num: match index { + 0 => SerialPortNumber::Com1, + 1 => SerialPortNumber::Com2, + 2 => SerialPortNumber::Com3, + 3 => SerialPortNumber::Com4, + _ => unreachable!(), + }, + }, + ); + + assert!(_old.is_none()); + } + } + + for bridge in val.pci_pci_bridges { + let _old = + spec.devices.pci_pci_bridges.insert(bridge.name(), bridge.0); + + assert!(_old.is_none()); + } + + spec.devices.qemu_pvpanic = val.pvpanic.map(|pvpanic| pvpanic.0); + + #[cfg(feature = "falcon")] + { + spec.devices.softnpu_pci_port = val.softnpu.pci_port; + spec.devices.softnpu_p9 = val.softnpu.p9_device; + spec.devices.p9fs = val.softnpu.p9fs; + for port in val.softnpu.ports { + let _old = spec.devices.softnpu_ports.insert( + port.name.clone(), + SoftNpuPortSpec { + name: port.name, + backend_name: port.backend_name.clone(), + }, + ); + + assert!(_old.is_none()); + + let _old = spec.backends.network_backends.insert( + port.backend_name, + NetworkBackendV0::Dlpi(port.backend_spec), + ); + + assert!(_old.is_none()); + } + } + + spec + } +} + +impl TryFrom for Spec { + type Error = ApiSpecParseError; + + fn try_from(mut value: InstanceSpecV0) -> Result { + let mut builder = SpecBuilder::with_board(value.devices.board); + for (device_name, device_spec) in value.devices.storage_devices { + let backend_name = match &device_spec { + StorageDeviceV0::VirtioDisk(disk) => &disk.backend_name, + StorageDeviceV0::NvmeDisk(disk) => &disk.backend_name, + }; + + let (backend_name, backend_spec) = value + .backends + .storage_backends + .remove_entry(backend_name) + .ok_or_else(|| { + ApiSpecParseError::BackendNotFound( + backend_name.to_owned(), + device_name.clone(), + ) + })?; + + builder.add_storage_device(Disk { + device_name, + device_spec, + backend_name, + backend_spec, + })?; + } + + if let Some(backend) = value.backends.storage_backends.keys().next() { + return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned())); + } + + for (device_name, device_spec) in value.devices.network_devices { + let backend_name = match &device_spec { + NetworkDeviceV0::VirtioNic(nic) => &nic.backend_name, + }; + + let (backend_name, backend_spec) = value + .backends + .network_backends + .remove_entry(backend_name) + .ok_or_else(|| { + ApiSpecParseError::BackendNotFound( + backend_name.to_owned(), + device_name.clone(), + ) + })?; + + builder.add_network_device(Nic { + device_name, + device_spec, + backend_name, + backend_spec, + })?; + } + + // SoftNPU ports can have network backends, so consume the SoftNPU + // device fields before checking to see if the network backend list is + // empty. + #[cfg(feature = "falcon")] + { + if let Some(softnpu_pci) = value.devices.softnpu_pci_port { + builder.set_softnpu_pci_port(softnpu_pci)?; + } + + if let Some(softnpu_p9) = value.devices.softnpu_p9 { + builder.set_softnpu_p9(softnpu_p9)?; + } + + if let Some(p9fs) = value.devices.p9fs { + builder.set_p9fs(p9fs)?; + } + + for (port_name, port) in value.devices.softnpu_ports { + let (backend_name, backend_spec) = value + .backends + .network_backends + .remove_entry(&port.backend_name) + .ok_or_else(|| { + ApiSpecParseError::BackendNotFound( + port.backend_name, + port_name.clone(), + ) + })?; + + let NetworkBackendV0::Dlpi(backend_spec) = backend_spec else { + return Err(ApiSpecParseError::NotDlpiBackend(port_name)); + }; + + builder.add_softnpu_port(SoftNpuPort { + name: port_name, + backend_name, + backend_spec, + })?; + } + } + + if let Some(backend) = value.backends.network_backends.keys().next() { + return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned())); + } + + // TODO(gjc) do more to preserve names + for serial_port in value.devices.serial_ports.values() { + builder.add_serial_port(serial_port.num)?; + } + + // TODO(gjc) do more to preserve names + for bridge in value.devices.pci_pci_bridges.into_values() { + builder.add_pci_bridge(PciPciBridge(bridge))?; + } + + if let Some(pvpanic) = value.devices.qemu_pvpanic { + builder.add_pvpanic_device(QemuPvpanic(pvpanic))?; + } + + Ok(builder.finish()) + } +} diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index adb5658d4..b5c3a72f2 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -10,11 +10,9 @@ use propolis_api_types::{ instance_spec::{ components::{ board::{Board, Chipset, I440Fx}, - devices::{ - PciPciBridge, QemuPvpanic, SerialPort, SerialPortNumber, - }, + devices::SerialPortNumber, }, - v0::{DeviceSpecV0, InstanceSpecV0, NetworkDeviceV0, StorageDeviceV0}, + v0::{NetworkDeviceV0, StorageDeviceV0}, PciPath, }, DiskRequest, InstanceProperties, NetworkInterfaceRequest, @@ -22,12 +20,8 @@ use propolis_api_types::{ use thiserror::Error; #[cfg(feature = "falcon")] -use propolis_api_types::instance_spec::{ - components::{ - backends::DlpiNetworkBackend, - devices::{P9fs, SoftNpuP9, SoftNpuPciPort, SoftNpuPort}, - }, - v0::NetworkBackendV0, +use propolis_api_types::instance_spec::components::devices::{ + P9fs, SoftNpuP9, SoftNpuPciPort, }; use crate::config; @@ -35,14 +29,13 @@ use crate::config; use super::{ api_request::{self, DeviceRequestError}, config_toml::{ConfigTomlError, ParsedConfig}, - ParsedNetworkDevice, ParsedStorageDevice, + Disk, Nic, PciPciBridge, QemuPvpanic, }; #[cfg(feature = "falcon")] -use super::ParsedSoftNpu; +use super::{SoftNpu, SoftNpuPort}; /// Errors that can arise while building an instance spec from component parts. -#[allow(clippy::enum_variant_names)] #[derive(Debug, Error)] pub(crate) enum SpecBuilderError { #[error("error parsing config TOML")] @@ -51,11 +44,8 @@ pub(crate) enum SpecBuilderError { #[error("error parsing device in ensure request")] DeviceRequest(#[from] DeviceRequestError), - #[error("A device with name {0} already exists")] - DeviceNameInUse(String), - - #[error("A backend with name {0} already exists")] - BackendNameInUse(String), + #[error("A component with name {0} already exists")] + ComponentNameInUse(String), #[error("A PCI device is already attached at {0:?}")] PciPathInUse(PciPath), @@ -63,14 +53,15 @@ pub(crate) enum SpecBuilderError { #[error("Serial port {0:?} is already specified")] SerialPortInUse(SerialPortNumber), - #[cfg(feature = "falcon")] - #[error("SoftNpu port {0:?} is already specified")] - SoftNpuPortInUse(String), + #[error("pvpanic device already specified")] + PvpanicInUse, } +#[derive(Debug, Default)] pub(crate) struct SpecBuilder { - spec: InstanceSpecV0, + spec: super::Spec, pci_paths: BTreeSet, + component_names: BTreeSet, } trait PciComponent { @@ -103,11 +94,15 @@ impl SpecBuilder { }; Self { - spec: InstanceSpecV0 { - devices: DeviceSpecV0 { board, ..Default::default() }, - backends: Default::default(), - }, - pci_paths: Default::default(), + spec: super::Spec { board, ..Default::default() }, + ..Default::default() + } + } + + pub(super) fn with_board(board: Board) -> Self { + Self { + spec: super::Spec { board, ..Default::default() }, + ..Default::default() } } @@ -152,7 +147,7 @@ impl SpecBuilder { ) -> Result<(), SpecBuilderError> { let parsed = ParsedConfig::try_from(config)?; - let Chipset::I440Fx(ref mut i440fx) = self.spec.devices.board.chipset; + let Chipset::I440Fx(ref mut i440fx) = self.spec.board.chipset; i440fx.enable_pcie = parsed.enable_pcie; for disk in parsed.disks { @@ -164,7 +159,7 @@ impl SpecBuilder { } for bridge in parsed.pci_bridges { - self.add_pci_bridge(bridge.name, bridge.bridge)?; + self.add_pci_bridge(bridge)?; } #[cfg(feature = "falcon")] @@ -176,21 +171,21 @@ impl SpecBuilder { #[cfg(feature = "falcon")] fn add_parsed_softnpu_devices( &mut self, - devices: ParsedSoftNpu, + devices: SoftNpu, ) -> Result<(), SpecBuilderError> { - for pci_port in devices.pci_ports { + if let Some(pci_port) = devices.pci_port { self.set_softnpu_pci_port(pci_port)?; } for port in devices.ports { - self.add_softnpu_port(port.name.clone(), port)?; + self.add_softnpu_port(port)?; } - for p9 in devices.p9_devices { + if let Some(p9) = devices.p9_device { self.set_softnpu_p9(p9)?; } - for p9fs in devices.p9fs { + if let Some(p9fs) = devices.p9fs { self.set_p9fs(p9fs)?; } @@ -214,83 +209,57 @@ impl SpecBuilder { /// Adds a storage device with an associated backend. pub(super) fn add_storage_device( &mut self, - ParsedStorageDevice { - device_name, - device_spec, - backend_name, - backend_spec, - }: ParsedStorageDevice, + disk: Disk, ) -> Result<&Self, SpecBuilderError> { - if self.spec.devices.storage_devices.contains_key(&device_name) { - return Err(SpecBuilderError::DeviceNameInUse(device_name)); + if self.component_names.contains(&disk.device_name) { + return Err(SpecBuilderError::ComponentNameInUse(disk.device_name)); } - if self.spec.backends.storage_backends.contains_key(&backend_name) { - return Err(SpecBuilderError::BackendNameInUse(backend_name)); + if self.component_names.contains(&disk.backend_name) { + return Err(SpecBuilderError::ComponentNameInUse( + disk.backend_name, + )); } - self.register_pci_device(device_spec.pci_path())?; - let _old = - self.spec.devices.storage_devices.insert(device_name, device_spec); - - assert!(_old.is_none()); - let _old = self - .spec - .backends - .storage_backends - .insert(backend_name, backend_spec); - - assert!(_old.is_none()); + + self.register_pci_device(disk.device_spec.pci_path())?; + self.component_names.insert(disk.device_name.clone()); + self.component_names.insert(disk.backend_name.clone()); + self.spec.disks.push(disk); Ok(self) } /// Adds a network device with an associated backend. pub(super) fn add_network_device( &mut self, - ParsedNetworkDevice { - device_name, - device_spec, - backend_name, - backend_spec, - }: ParsedNetworkDevice, + nic: Nic, ) -> Result<&Self, SpecBuilderError> { - if self.spec.devices.network_devices.contains_key(&device_name) { - return Err(SpecBuilderError::DeviceNameInUse(device_name)); + if self.component_names.contains(&nic.device_name) { + return Err(SpecBuilderError::ComponentNameInUse(nic.device_name)); } - if self.spec.backends.network_backends.contains_key(&backend_name) { - return Err(SpecBuilderError::BackendNameInUse(backend_name)); + if self.component_names.contains(&nic.backend_name) { + return Err(SpecBuilderError::ComponentNameInUse(nic.backend_name)); } - self.register_pci_device(device_spec.pci_path())?; - let _old = - self.spec.devices.network_devices.insert(device_name, device_spec); - - assert!(_old.is_none()); - let _old = self - .spec - .backends - .network_backends - .insert(backend_name, backend_spec); - - assert!(_old.is_none()); + self.register_pci_device(nic.device_spec.pci_path())?; + self.component_names.insert(nic.device_name.clone()); + self.component_names.insert(nic.backend_name.clone()); + self.spec.nics.push(nic); Ok(self) } /// Adds a PCI-PCI bridge. pub fn add_pci_bridge( &mut self, - bridge_name: String, - bridge_spec: PciPciBridge, + bridge: PciPciBridge, ) -> Result<&Self, SpecBuilderError> { - if self.spec.devices.pci_pci_bridges.contains_key(&bridge_name) { - return Err(SpecBuilderError::DeviceNameInUse(bridge_name)); + let name = bridge.name(); + if self.component_names.contains(&name) { + return Err(SpecBuilderError::ComponentNameInUse(name)); } - self.register_pci_device(bridge_spec.pci_path)?; - let _old = - self.spec.devices.pci_pci_bridges.insert(bridge_name, bridge_spec); - - assert!(_old.is_none()); + self.register_pci_device(bridge.0.pci_path)?; + self.spec.pci_pci_bridges.push(bridge); Ok(self) } @@ -299,24 +268,17 @@ impl SpecBuilder { &mut self, port: SerialPortNumber, ) -> Result<&Self, SpecBuilderError> { - if self - .spec - .devices - .serial_ports - .insert( - match port { - SerialPortNumber::Com1 => "com1", - SerialPortNumber::Com2 => "com2", - SerialPortNumber::Com3 => "com3", - SerialPortNumber::Com4 => "com4", - } - .to_string(), - SerialPort { num: port }, - ) - .is_some() - { + let idx = match port { + SerialPortNumber::Com1 => 0, + SerialPortNumber::Com2 => 1, + SerialPortNumber::Com3 => 2, + SerialPortNumber::Com4 => 3, + }; + + if self.spec.serial[idx] != super::SerialPort::Disabled { Err(SpecBuilderError::SerialPortInUse(port)) } else { + self.spec.serial[idx] = super::SerialPort::Enabled; Ok(self) } } @@ -325,24 +287,31 @@ impl SpecBuilder { &mut self, pvpanic: QemuPvpanic, ) -> Result<&Self, SpecBuilderError> { - if self.spec.devices.qemu_pvpanic.is_some() { - return Err(SpecBuilderError::DeviceNameInUse( - "pvpanic".to_string(), - )); + if self.spec.pvpanic.is_some() { + return Err(SpecBuilderError::PvpanicInUse); } - self.spec.devices.qemu_pvpanic = Some(pvpanic); - + self.spec.pvpanic = Some(pvpanic); Ok(self) } + #[cfg(feature = "falcon")] + pub fn set_softnpu_com4(&mut self) -> Result<&Self, SpecBuilderError> { + if self.spec.serial[3] != super::SerialPort::Disabled { + Err(SpecBuilderError::SerialPortInUse(SerialPortNumber::Com4)) + } else { + self.spec.serial[3] = super::SerialPort::SoftNpu; + Ok(self) + } + } + #[cfg(feature = "falcon")] pub fn set_softnpu_pci_port( &mut self, pci_port: SoftNpuPciPort, ) -> Result<&Self, SpecBuilderError> { self.register_pci_device(pci_port.pci_path)?; - self.spec.devices.softnpu_pci_port = Some(pci_port); + self.spec.softnpu.pci_port = Some(pci_port); Ok(self) } @@ -352,39 +321,38 @@ impl SpecBuilder { p9: SoftNpuP9, ) -> Result<&Self, SpecBuilderError> { self.register_pci_device(p9.pci_path)?; - self.spec.devices.softnpu_p9 = Some(p9); + self.spec.softnpu.p9_device = Some(p9); Ok(self) } #[cfg(feature = "falcon")] pub fn set_p9fs(&mut self, p9fs: P9fs) -> Result<&Self, SpecBuilderError> { self.register_pci_device(p9fs.pci_path)?; - self.spec.devices.p9fs = Some(p9fs); + self.spec.softnpu.p9fs = Some(p9fs); Ok(self) } #[cfg(feature = "falcon")] pub fn add_softnpu_port( &mut self, - key: String, port: SoftNpuPort, ) -> Result<&Self, SpecBuilderError> { - let _old = self.spec.backends.network_backends.insert( - port.backend_name.clone(), - NetworkBackendV0::Dlpi(DlpiNetworkBackend { - vnic_name: port.backend_name.clone(), - }), - ); - assert!(_old.is_none()); - if self.spec.devices.softnpu_ports.insert(key, port.clone()).is_some() { - Err(SpecBuilderError::SoftNpuPortInUse(port.name)) - } else { - Ok(self) + if self.component_names.contains(&port.name) { + return Err(SpecBuilderError::ComponentNameInUse(port.name)); } + + if self.component_names.contains(&port.backend_name) { + return Err(SpecBuilderError::ComponentNameInUse( + port.backend_name, + )); + } + + self.spec.softnpu.ports.push(port); + Ok(self) } /// Yields the completed spec, consuming the builder. - pub fn finish(self) -> InstanceSpecV0 { + pub fn finish(self) -> super::Spec { self.spec } } diff --git a/bin/propolis-server/src/lib/spec/config_toml.rs b/bin/propolis-server/src/lib/spec/config_toml.rs index 337c24bb3..6eb69fbf6 100644 --- a/bin/propolis-server/src/lib/spec/config_toml.rs +++ b/bin/propolis-server/src/lib/spec/config_toml.rs @@ -9,7 +9,9 @@ use std::str::{FromStr, ParseBoolError}; use propolis_api_types::instance_spec::{ components::{ backends::{FileStorageBackend, VirtioNetworkBackend}, - devices::{NvmeDisk, PciPciBridge, VirtioDisk, VirtioNic}, + devices::{ + NvmeDisk, PciPciBridge as BridgeSpec, VirtioDisk, VirtioNic, + }, }, v0::{ NetworkBackendV0, NetworkDeviceV0, StorageBackendV0, StorageDeviceV0, @@ -20,15 +22,18 @@ use thiserror::Error; #[cfg(feature = "falcon")] use propolis_api_types::instance_spec::components::devices::{ - P9fs, SoftNpuP9, SoftNpuPciPort, SoftNpuPort, + P9fs, SoftNpuP9, SoftNpuPciPort, }; +#[cfg(feature = "falcon")] +use super::SoftNpuPort; + use crate::config; -use super::{ParsedNetworkDevice, ParsedStorageDevice}; +use super::{Disk, Nic, PciPciBridge}; #[cfg(feature = "falcon")] -use super::ParsedSoftNpu; +use super::SoftNpu; #[derive(Debug, Error)] pub(crate) enum ConfigTomlError { @@ -77,12 +82,12 @@ pub(crate) enum ConfigTomlError { #[derive(Default)] pub(super) struct ParsedConfig { pub(super) enable_pcie: bool, - pub(super) disks: Vec, - pub(super) nics: Vec, - pub(super) pci_bridges: Vec, + pub(super) disks: Vec, + pub(super) nics: Vec, + pub(super) pci_bridges: Vec, #[cfg(feature = "falcon")] - pub(super) softnpu: ParsedSoftNpu, + pub(super) softnpu: SoftNpu, } impl TryFrom<&config::Config> for ParsedConfig { @@ -135,7 +140,7 @@ impl TryFrom<&config::Config> for ParsedConfig { backend_config, )?; - parsed.disks.push(ParsedStorageDevice { + parsed.disks.push(Disk { device_name: device_name.to_owned(), device_spec, backend_name, @@ -150,12 +155,11 @@ impl TryFrom<&config::Config> for ParsedConfig { } #[cfg(feature = "falcon")] "softnpu-pci-port" => { - parsed.softnpu.pci_ports.push( - parse_softnpu_pci_port_from_config( + parsed.softnpu.pci_port = + Some(parse_softnpu_pci_port_from_config( device_name, device, - )?, - ); + )?); } #[cfg(feature = "falcon")] "softnpu-port" => { @@ -166,16 +170,14 @@ impl TryFrom<&config::Config> for ParsedConfig { } #[cfg(feature = "falcon")] "softnpu-p9" => { - parsed.softnpu.p9_devices.push( + parsed.softnpu.p9_device = Some( parse_softnpu_p9_from_config(device_name, device)?, ); } #[cfg(feature = "falcon")] "pci-virtio-9p" => { - parsed - .softnpu - .p9fs - .push(parse_p9fs_from_config(device_name, device)?); + parsed.softnpu.p9fs = + Some(parse_p9fs_from_config(device_name, device)?); } _ => { return Err(ConfigTomlError::UnrecognizedDeviceType( @@ -284,7 +286,7 @@ pub(super) fn parse_storage_device_from_config( pub(super) fn parse_network_device_from_config( name: &str, device: &config::Device, -) -> Result { +) -> Result { let vnic_name = device .get_string("vnic") .ok_or_else(|| ConfigTomlError::NoVnicName(name.to_owned()))?; @@ -306,34 +308,20 @@ pub(super) fn parse_network_device_from_config( pci_path, }); - Ok(ParsedNetworkDevice { - device_name, - device_spec, - backend_name, - backend_spec, - }) -} - -pub(super) struct ParsedPciPciBridge { - pub(super) name: String, - pub(super) bridge: PciPciBridge, + Ok(Nic { device_name, device_spec, backend_name, backend_spec }) } pub(super) fn parse_pci_bridge_from_config( bridge: &config::PciBridge, -) -> Result { - let name = format!("pci-bridge-{}", bridge.downstream_bus); +) -> Result { let pci_path = PciPath::from_str(&bridge.pci_path).map_err(|e| { ConfigTomlError::PciPathParseFailed(bridge.pci_path.to_string(), e) })?; - Ok(ParsedPciPciBridge { - name, - bridge: PciPciBridge { - downstream_bus: bridge.downstream_bus, - pci_path, - }, - }) + Ok(PciPciBridge(BridgeSpec { + downstream_bus: bridge.downstream_bus, + pci_path, + })) } #[cfg(feature = "falcon")] @@ -365,6 +353,8 @@ pub(super) fn parse_softnpu_port_from_config( name: &str, device: &config::Device, ) -> Result { + use propolis_api_types::instance_spec::components::backends::DlpiNetworkBackend; + let vnic_name = device .get_string("vnic") .ok_or_else(|| ConfigTomlError::NoVnicName(name.to_owned()))?; @@ -372,6 +362,7 @@ pub(super) fn parse_softnpu_port_from_config( Ok(SoftNpuPort { name: name.to_owned(), backend_name: vnic_name.to_owned(), + backend_spec: DlpiNetworkBackend { vnic_name: vnic_name.to_owned() }, }) } diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 3826d119f..e056f7f32 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -2,44 +2,113 @@ // 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/. -//! Helper functions for building instance specs from server parameters. +//! Instance specs describe how to configure a VM and what components it has. +//! +//! This module defines an "internal" spec type for the server's instance +//! initialization code to use. This spec type is not `Serialize` and is not +//! meant to be sent over the wire in API requests or the migration protocol. +//! This allows the internal representation to change freely between versions +//! (so long as it can consistently be converted to and from the wire-format +//! spec in the [`propolis_api_types`] crate); this, in turn, allows this +//! representation to take forms that might otherwise be hard to change in a +//! backward-compatible way. -use propolis_api_types::instance_spec::{v0::*, PciPath}; +use propolis_api_types::instance_spec::{ + components::{ + board::Board, + devices::{ + PciPciBridge as PciPciBridgeSpec, QemuPvpanic as QemuPvpanicSpec, + }, + }, + v0::*, + PciPath, +}; #[cfg(feature = "falcon")] -use propolis_api_types::instance_spec::components::devices::{ - P9fs, SoftNpuP9, SoftNpuPciPort, SoftNpuPort, +use propolis_api_types::instance_spec::components::{ + backends::DlpiNetworkBackend, + devices::{P9fs, SoftNpuP9, SoftNpuPciPort}, }; mod api_request; +pub(crate) mod api_spec_v0; pub(crate) mod builder; mod config_toml; +#[derive(Clone, Debug, Default)] +pub(crate) struct Spec { + board: Board, + disks: Vec, + nics: Vec, + + // TODO(#735): Preserve device names for identification purposes. + serial: [SerialPort; 4], + + // TODO(#735): Preserve device names for identification purposes. + pci_pci_bridges: Vec, + pvpanic: Option, + + #[cfg(feature = "falcon")] + softnpu: SoftNpu, +} + /// Describes a storage device/backend pair parsed from an input source like an /// API request or a config TOML entry. -struct ParsedStorageDevice { - device_name: String, - device_spec: StorageDeviceV0, - backend_name: String, - backend_spec: StorageBackendV0, +#[derive(Clone, Debug)] +pub struct Disk { + pub device_name: String, + pub device_spec: StorageDeviceV0, + pub backend_name: String, + pub backend_spec: StorageBackendV0, } /// Describes a network device/backend pair parsed from an input source like an /// API request or a config TOML entry. -struct ParsedNetworkDevice { - device_name: String, - device_spec: NetworkDeviceV0, - backend_name: String, - backend_spec: NetworkBackendV0, +#[derive(Clone, Debug)] +pub struct Nic { + pub device_name: String, + pub device_spec: NetworkDeviceV0, + pub backend_name: String, + pub backend_spec: NetworkBackendV0, +} + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum SerialPort { + #[default] + Disabled, + Enabled, + + #[cfg(feature = "falcon")] + SoftNpu, +} + +#[derive(Clone, Debug)] +pub struct QemuPvpanic(pub QemuPvpanicSpec); + +#[derive(Clone, Debug)] +pub struct PciPciBridge(pub PciPciBridgeSpec); + +impl PciPciBridge { + pub fn name(&self) -> String { + format!("pci-bridge-{}", self.0.downstream_bus) + } +} + +#[cfg(feature = "falcon")] +#[derive(Clone, Debug)] +pub struct SoftNpuPort { + pub name: String, + pub backend_name: String, + pub backend_spec: DlpiNetworkBackend, } #[cfg(feature = "falcon")] -#[derive(Default)] -struct ParsedSoftNpu { - pci_ports: Vec, - ports: Vec, - p9_devices: Vec, - p9fs: Vec, +#[derive(Clone, Debug, Default)] +pub struct SoftNpu { + pub pci_port: Option, + pub ports: Vec, + pub p9_device: Option, + pub p9fs: Option, } /// Generates NIC device and backend names from the NIC's PCI path. This is diff --git a/bin/propolis-server/src/lib/vm/ensure.rs b/bin/propolis-server/src/lib/vm/ensure.rs index 65c97fd14..081a9b3dd 100644 --- a/bin/propolis-server/src/lib/vm/ensure.rs +++ b/bin/propolis-server/src/lib/vm/ensure.rs @@ -30,9 +30,9 @@ use std::sync::Arc; use oximeter::types::ProducerRegistry; use oximeter_instruments::kstat::KstatSampler; use propolis_api_types::{ - instance_spec::{v0::InstanceSpecV0, VersionedInstanceSpec}, - InstanceEnsureResponse, InstanceMigrateInitiateResponse, - InstanceProperties, InstanceSpecEnsureRequest, InstanceState, + instance_spec::v0::InstanceSpecV0, InstanceEnsureResponse, + InstanceMigrateInitiateRequest, InstanceMigrateInitiateResponse, + InstanceProperties, InstanceState, }; use slog::{debug, info}; @@ -40,6 +40,7 @@ use crate::{ initializer::{ build_instance, MachineInitializer, MachineInitializerState, }, + spec, stats::create_kstat_sampler, vm::request_queue::InstanceAutoStart, }; @@ -52,12 +53,18 @@ use super::{ EnsureOptions, InstanceEnsureResponseTx, VmError, }; +pub(crate) struct VmEnsureRequest { + pub(crate) properties: InstanceProperties, + pub(crate) migrate: Option, + pub(crate) instance_spec: spec::Spec, +} + /// Holds state about an instance ensure request that has not yet produced any /// VM objects or driven the VM state machine to the `ActiveVm` state. pub(crate) struct VmEnsureNotStarted<'a> { log: &'a slog::Logger, vm: &'a Arc, - ensure_request: &'a InstanceSpecEnsureRequest, + ensure_request: &'a VmEnsureRequest, ensure_options: &'a EnsureOptions, ensure_response_tx: InstanceEnsureResponseTx, state_publisher: &'a mut StatePublisher, @@ -67,7 +74,7 @@ impl<'a> VmEnsureNotStarted<'a> { pub(super) fn new( log: &'a slog::Logger, vm: &'a Arc, - ensure_request: &'a InstanceSpecEnsureRequest, + ensure_request: &'a VmEnsureRequest, ensure_options: &'a EnsureOptions, ensure_response_tx: InstanceEnsureResponseTx, state_publisher: &'a mut StatePublisher, @@ -82,9 +89,8 @@ impl<'a> VmEnsureNotStarted<'a> { } } - pub(crate) fn instance_spec(&self) -> &InstanceSpecV0 { - let VersionedInstanceSpec::V0(v0) = &self.ensure_request.instance_spec; - v0 + pub(crate) fn instance_spec(&self) -> &spec::Spec { + &self.ensure_request.instance_spec } pub(crate) fn state_publisher(&mut self) -> &mut StatePublisher { @@ -162,10 +168,10 @@ impl<'a> VmEnsureNotStarted<'a> { // Set up the 'shell' instance into which the rest of this routine will // add components. - let VersionedInstanceSpec::V0(v0_spec) = &spec; + let v0_spec: InstanceSpecV0 = self.instance_spec().clone().into(); let machine = build_instance( &properties.vm_name(), - v0_spec, + &v0_spec, options.use_reservoir, vmm_log, )?; @@ -176,7 +182,7 @@ impl<'a> VmEnsureNotStarted<'a> { devices: Default::default(), block_backends: Default::default(), crucible_backends: Default::default(), - spec: v0_spec, + spec: &v0_spec, properties, toml_config: &options.toml_config, producer_registry: options.oximeter_registry.clone(), @@ -184,7 +190,7 @@ impl<'a> VmEnsureNotStarted<'a> { kstat_sampler: initialize_kstat_sampler( self.log, properties, - v0_spec, + &v0_spec, options.oximeter_registry.clone(), ), }; @@ -259,7 +265,7 @@ impl<'a> VmEnsureNotStarted<'a> { pub(crate) struct VmEnsureObjectsCreated<'a> { log: &'a slog::Logger, vm: &'a Arc, - ensure_request: &'a InstanceSpecEnsureRequest, + ensure_request: &'a VmEnsureRequest, ensure_options: &'a EnsureOptions, ensure_response_tx: InstanceEnsureResponseTx, state_publisher: &'a mut StatePublisher, diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index 1608ef069..c9fe469f5 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -82,13 +82,13 @@ use std::{collections::BTreeMap, net::SocketAddr, sync::Arc}; use active::ActiveVm; +use ensure::VmEnsureRequest; use oximeter::types::ProducerRegistry; use propolis_api_types::{ instance_spec::{v0::InstanceSpecV0, VersionedInstanceSpec}, InstanceEnsureResponse, InstanceMigrateStatusResponse, - InstanceMigrationStatus, InstanceProperties, InstanceSpecEnsureRequest, - InstanceSpecGetResponse, InstanceState, InstanceStateMonitorResponse, - MigrationState, + InstanceMigrationStatus, InstanceProperties, InstanceSpecGetResponse, + InstanceState, InstanceStateMonitorResponse, MigrationState, }; use slog::info; use state_driver::StateDriverOutput; @@ -503,7 +503,7 @@ impl Vm { pub(crate) async fn ensure( self: &Arc, log: &slog::Logger, - ensure_request: InstanceSpecEnsureRequest, + ensure_request: VmEnsureRequest, options: EnsureOptions, ) -> Result { let log_for_driver = @@ -554,8 +554,8 @@ impl Vm { _ => {} }; - let VersionedInstanceSpec::V0(v0_spec) = - ensure_request.instance_spec.clone(); + let v0_spec: InstanceSpecV0 = + ensure_request.instance_spec.clone().into(); let thread_count = usize::max( VMM_MIN_RT_THREADS, diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index a5fa9cdae..90647debb 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -14,7 +14,7 @@ use propolis_api_types::{ instance_spec::{ components::backends::CrucibleStorageBackend, v0::StorageBackendV0, }, - InstanceSpecEnsureRequest, InstanceState, MigrationState, + InstanceState, MigrationState, }; use slog::{error, info}; use tokio::sync::Notify; @@ -28,7 +28,7 @@ use crate::{ }; use super::{ - ensure::{VmEnsureActive, VmEnsureNotStarted}, + ensure::{VmEnsureActive, VmEnsureNotStarted, VmEnsureRequest}, guest_event::{self, GuestEvent}, objects::VmObjects, request_queue::{self, ExternalRequest, InstanceAutoStart}, @@ -261,7 +261,7 @@ pub(super) async fn run_state_driver( log: slog::Logger, vm: Arc, mut state_publisher: StatePublisher, - ensure_request: InstanceSpecEnsureRequest, + ensure_request: VmEnsureRequest, ensure_result_tx: InstanceEnsureResponseTx, ensure_options: super::EnsureOptions, ) -> StateDriverOutput { @@ -308,7 +308,7 @@ async fn create_and_activate_vm<'a>( log: &'a slog::Logger, vm: &'a Arc, state_publisher: &'a mut StatePublisher, - ensure_request: &'a InstanceSpecEnsureRequest, + ensure_request: &'a VmEnsureRequest, ensure_result_tx: InstanceEnsureResponseTx, ensure_options: &'a super::EnsureOptions, ) -> anyhow::Result> { From 261acf6541c48afef4d5b299ee59afe5c9ce228d Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 26 Jul 2024 00:50:32 +0000 Subject: [PATCH 02/11] refactor: use internal spec type everywhere except the preamble --- bin/propolis-server/src/lib/initializer.rs | 198 ++++++------------ bin/propolis-server/src/lib/migrate/source.rs | 2 +- .../src/lib/spec/api_request.rs | 10 +- .../src/lib/spec/api_spec_v0.rs | 23 +- .../src/lib/spec/config_toml.rs | 8 +- bin/propolis-server/src/lib/spec/mod.rs | 19 +- bin/propolis-server/src/lib/stats/mod.rs | 9 +- bin/propolis-server/src/lib/vm/ensure.rs | 20 +- bin/propolis-server/src/lib/vm/mod.rs | 25 ++- bin/propolis-server/src/lib/vm/objects.rs | 11 +- .../src/lib/vm/state_driver.rs | 79 ++++--- 11 files changed, 164 insertions(+), 240 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 6f5ac4209..e86d8af90 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -11,6 +11,7 @@ use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use crate::serial::Serial; +use crate::spec; use crate::stats::{ track_network_interface_kstats, track_vcpu_kstats, VirtualDiskProducer, VirtualMachine, @@ -38,15 +39,15 @@ use propolis::hw::uart::LpcUart; use propolis::hw::{nvme, virtio}; use propolis::intr_pins; use propolis::vmm::{self, Builder, Machine}; -use propolis_api_types::instance_spec::{self, v0::InstanceSpecV0}; +use propolis_api_types::instance_spec; use propolis_api_types::InstanceProperties; use slog::info; /// Arbitrary ROM limit for now const MAX_ROM_SIZE: usize = 0x20_0000; -fn get_spec_guest_ram_limits(spec: &InstanceSpecV0) -> (usize, usize) { - let memsize = spec.devices.board.memory_mb as usize * MB; +fn get_spec_guest_ram_limits(spec: &spec::Spec) -> (usize, usize) { + let memsize = spec.board.memory_mb as usize * MB; let lowmem = memsize.min(3 * GB); let highmem = memsize.saturating_sub(3 * GB); (lowmem, highmem) @@ -54,7 +55,7 @@ fn get_spec_guest_ram_limits(spec: &InstanceSpecV0) -> (usize, usize) { pub fn build_instance( name: &str, - spec: &InstanceSpecV0, + spec: &spec::Spec, use_reservoir: bool, _log: slog::Logger, ) -> Result { @@ -65,7 +66,7 @@ pub fn build_instance( track_dirty: true, }; let mut builder = Builder::new(name, create_opts)? - .max_cpus(spec.devices.board.cpus)? + .max_cpus(spec.board.cpus)? .add_mem_region(0, lowmem, "lowmem")? .add_rom_region(0x1_0000_0000 - MAX_ROM_SIZE, MAX_ROM_SIZE, "bootrom")? .add_mmio_region(0xc000_0000_usize, 0x2000_0000_usize, "dev32")? @@ -118,7 +119,7 @@ pub struct MachineInitializer<'a> { pub(crate) devices: DeviceMap, pub(crate) block_backends: BlockBackendMap, pub(crate) crucible_backends: CrucibleBackendMap, - pub(crate) spec: &'a InstanceSpecV0, + pub(crate) spec: &'a spec::Spec, pub(crate) properties: &'a InstanceProperties, pub(crate) toml_config: &'a crate::server::VmTomlConfig, pub(crate) producer_registry: Option, @@ -194,15 +195,16 @@ impl<'a> MachineInitializer<'a> { event_handler: &Arc, ) -> Result { let mut pci_builder = pci::topology::Builder::new(); - for (name, bridge) in &self.spec.devices.pci_pci_bridges { + for bridge in &self.spec.pci_pci_bridges { let desc = pci::topology::BridgeDescription::new( - pci::topology::LogicalBusId(bridge.downstream_bus), - bridge.pci_path.try_into().map_err(|e| { + pci::topology::LogicalBusId(bridge.0.downstream_bus), + bridge.0.pci_path.try_into().map_err(|e| { Error::new( ErrorKind::InvalidInput, format!( "Couldn't get PCI BDF for bridge {}: {}", - name, e + bridge.name(), + e ), ) })?, @@ -212,7 +214,7 @@ impl<'a> MachineInitializer<'a> { let pci::topology::FinishedTopology { topology: pci_topology, bridges } = pci_builder.finish(self.machine)?; - match self.spec.devices.board.chipset { + match self.spec.board.chipset { instance_spec::components::board::Chipset::I440Fx(i440fx) => { let power_ref = Arc::downgrade(event_handler); let reset_ref = Arc::downgrade(event_handler); @@ -295,22 +297,25 @@ impl<'a> MachineInitializer<'a> { &mut self, chipset: &RegisteredChipset, ) -> Result, Error> { - use instance_spec::components::devices::SerialPortNumber; - let mut com1 = None; - for (name, serial_spec) in &self.spec.devices.serial_ports { - let (irq, port) = match serial_spec.num { - SerialPortNumber::Com1 => (ibmpc::IRQ_COM1, ibmpc::PORT_COM1), - SerialPortNumber::Com2 => (ibmpc::IRQ_COM2, ibmpc::PORT_COM2), - SerialPortNumber::Com3 => (ibmpc::IRQ_COM3, ibmpc::PORT_COM3), - SerialPortNumber::Com4 => (ibmpc::IRQ_COM4, ibmpc::PORT_COM4), + for (index, serial_spec) in self.spec.serial.iter().enumerate() { + if *serial_spec != spec::SerialPort::Enabled { + continue; + } + + let (name, irq, port) = match index { + 0 => ("com1", ibmpc::IRQ_COM1, ibmpc::PORT_COM1), + 1 => ("com2", ibmpc::IRQ_COM2, ibmpc::PORT_COM2), + 2 => ("com3", ibmpc::IRQ_COM3, ibmpc::PORT_COM3), + 3 => ("com4", ibmpc::IRQ_COM4, ibmpc::PORT_COM4), + _ => unreachable!(), }; let dev = LpcUart::new(chipset.irq_pin(irq).unwrap()); dev.set_autodiscard(true); LpcUart::attach(&dev, &self.machine.bus_pio, port); - self.devices.insert(name.clone(), dev.clone()); - if matches!(serial_spec.num, SerialPortNumber::Com1) { + self.devices.insert(name.to_owned(), dev.clone()); + if index == 0 { assert!(com1.is_none()); com1 = Some(dev); } @@ -353,8 +358,8 @@ impl<'a> MachineInitializer<'a> { &mut self, virtual_machine: VirtualMachine, ) -> Result<(), anyhow::Error> { - if let Some(ref spec) = self.spec.devices.qemu_pvpanic { - if spec.enable_isa { + if let Some(spec) = &self.spec.pvpanic { + if spec.0.enable_isa { let pvpanic = QemuPvpanic::create( self.log.new(slog::o!("dev" => "qemu-pvpanic")), ); @@ -497,16 +502,17 @@ impl<'a> MachineInitializer<'a> { Nvme, } - 'devloop: for (name, device_spec) in &self.spec.devices.storage_devices - { + 'devloop: for disk in &self.spec.disks { info!( self.log, "Creating storage device {} with properties {:?}", - name, - device_spec + disk.device_name, + disk.device_spec ); - let (device_interface, backend_name, pci_path) = match device_spec { + let (device_interface, backend_name, pci_path) = match &disk + .device_spec + { instance_spec::v0::StorageDeviceV0::VirtioDisk(disk) => { (DeviceInterface::Virtio, &disk.backend_name, disk.pci_path) } @@ -515,34 +521,19 @@ impl<'a> MachineInitializer<'a> { } }; - let backend_spec = self - .spec - .backends - .storage_backends - .get(backend_name) - .ok_or_else(|| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Backend {} not found for storage device {}", - backend_name, name - ), - ) - })?; - let bdf: pci::Bdf = pci_path.try_into().map_err(|e| { Error::new( ErrorKind::InvalidInput, format!( "Couldn't get PCI BDF for storage device {}: {}", - name, e + disk.device_name, e ), ) })?; let StorageBackendInstance { be: backend, crucible } = self .create_storage_backend_from_spec( - backend_spec, + &disk.backend_spec, backend_name, &nexus_client, ) @@ -562,12 +553,11 @@ impl<'a> MachineInitializer<'a> { DeviceInterface::Nvme => { // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); + let component = format!("nvme-{}", disk.device_name); let nvme = nvme::PciNvme::create( - name.to_string(), + disk.device_name.to_string(), mdts, - self.log.new( - slog::o!("component" => format!("nvme-{}", name)), - ), + self.log.new(slog::o!("component" => component)), ); self.devices .insert(format!("pci-nvme-{bdf}"), nvme.clone()); @@ -643,49 +633,24 @@ impl<'a> MachineInitializer<'a> { let mut interface_ids: Option = self.kstat_sampler.as_ref().map(|_| Vec::new()); - for (name, vnic_spec) in &self.spec.devices.network_devices { - info!(self.log, "Creating vNIC {}", name); + for nic in &self.spec.nics { + info!(self.log, "Creating vNIC {}", nic.device_name); + let instance_spec::v0::NetworkDeviceV0::VirtioNic(vnic_spec) = - vnic_spec; - - let backend_spec = self - .spec - .backends - .network_backends - .get(&vnic_spec.backend_name) - .ok_or_else(|| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Backend {} not found for vNIC {}", - vnic_spec.backend_name, name - ), - ) - })?; + &nic.device_spec; + let bdf: pci::Bdf = vnic_spec.pci_path.try_into().map_err(|e| { Error::new( ErrorKind::InvalidInput, - format!("Couldn't get PCI BDF for vNIC {}: {}", name, e), + format!( + "Couldn't get PCI BDF for vNIC {}: {}", + nic.device_name, e + ), ) })?; - let vnic_name = match backend_spec { - instance_spec::v0::NetworkBackendV0::Virtio(spec) => { - &spec.vnic_name - } - instance_spec::v0::NetworkBackendV0::Dlpi(_) => { - return Err(Error::new( - ErrorKind::InvalidInput, - format!( - "Network backend must be virtio for vNIC {}", - name, - ), - )); - } - }; - let viona = virtio::PciVirtioViona::new( - vnic_name, + &nic.backend_spec.vnic_name, 0x100, &self.machine.hdl, )?; @@ -767,56 +732,29 @@ impl<'a> MachineInitializer<'a> { &mut self, chipset: &RegisteredChipset, ) -> Result<(), Error> { + let softnpu = &self.spec.softnpu; + // Check to make sure we actually have both a pci port and at least one // regular SoftNpu port, otherwise just return. - let pci_port = match &self.spec.devices.softnpu_pci_port { + let pci_port = match &softnpu.pci_port { Some(tfp) => tfp, None => return Ok(()), }; - if self.spec.devices.softnpu_ports.is_empty() { + if softnpu.ports.is_empty() { return Ok(()); } - let mut ports: Vec<&instance_spec::components::devices::SoftNpuPort> = - self.spec.devices.softnpu_ports.values().collect(); + // Get a Vec of references to the ports which will then be sorted by + // port name. + let mut ports: Vec<_> = softnpu.ports.iter().collect(); // SoftNpu ports are named __vnic by falcon, where // indicates the intended order. - ports.sort_by_key(|p| &p.name); - - let mut data_links: Vec = Vec::new(); - for x in &ports { - let backend = self - .spec - .backends - .network_backends - .get(&x.backend_name) - .ok_or_else(|| { - Error::new( - ErrorKind::InvalidInput, - format!( - "Backend {} not found for softnpu port", - x.backend_name - ), - ) - })?; - - let vnic = match &backend { - instance_spec::v0::NetworkBackendV0::Dlpi(dlpi) => { - &dlpi.vnic_name - } - _ => { - return Err(Error::new( - ErrorKind::InvalidInput, - format!( - "Softnpu port must have DLPI backend: {}", - x.backend_name - ), - )); - } - }; - data_links.push(vnic.clone()); - } + ports.sort_by_key(|p| p.name.as_str()); + let data_links = ports + .iter() + .map(|port| port.backend_spec.vnic_name.clone()) + .collect(); // Set up an LPC uart for ASIC management comms from the guest. // @@ -834,17 +772,15 @@ impl<'a> MachineInitializer<'a> { let p9_handler = virtio::softnpu::SoftNpuP9Handler::new( "/dev/softnpufs".to_owned(), "/dev/softnpufs".to_owned(), - self.spec.devices.softnpu_ports.len() as u16, + ports.len() as u16, pipeline.clone(), self.log.clone(), ); let vio9p = virtio::p9fs::PciVirtio9pfs::new(0x40, Arc::new(p9_handler)); self.devices.insert("softnpu-p9fs".to_string(), vio9p.clone()); - let bdf: pci::Bdf = self - .spec - .devices - .softnpu_p9 + let bdf = softnpu + .p9_device .as_ref() .ok_or_else(|| { Error::new( @@ -903,11 +839,11 @@ impl<'a> MachineInitializer<'a> { &mut self, chipset: &RegisteredChipset, ) -> Result<(), Error> { + let softnpu = &self.spec.softnpu; // Check that there is actually a p9fs device to register, if not bail // early. - let p9fs = match &self.spec.devices.p9fs { - Some(p9fs) => p9fs, - None => return Ok(()), + let Some(p9fs) = &softnpu.p9fs else { + return Ok(()); }; let bdf: pci::Bdf = p9fs.pci_path.try_into().map_err(|e| { diff --git a/bin/propolis-server/src/lib/migrate/source.rs b/bin/propolis-server/src/lib/migrate/source.rs index a8c3c5d9a..5abc41aa1 100644 --- a/bin/propolis-server/src/lib/migrate/source.rs +++ b/bin/propolis-server/src/lib/migrate/source.rs @@ -468,7 +468,7 @@ impl<'vm, T: MigrateConn> RonV0Runner<'vm, T> { async fn sync(&mut self) -> Result<(), MigrateError> { self.update_state(MigrationState::Sync); let preamble = Preamble::new(VersionedInstanceSpec::V0( - self.vm.lock_shared().await.instance_spec().clone(), + self.vm.lock_shared().await.instance_spec().clone().into(), )); let s = ron::ser::to_string(&preamble) .map_err(codec::ProtocolError::from)?; diff --git a/bin/propolis-server/src/lib/spec/api_request.rs b/bin/propolis-server/src/lib/spec/api_request.rs index b83cb0337..7369ec6fe 100644 --- a/bin/propolis-server/src/lib/spec/api_request.rs +++ b/bin/propolis-server/src/lib/spec/api_request.rs @@ -15,10 +15,7 @@ use propolis_api_types::{ }, devices::{NvmeDisk, VirtioDisk, VirtioNic}, }, - v0::{ - NetworkBackendV0, NetworkDeviceV0, StorageBackendV0, - StorageDeviceV0, - }, + v0::{NetworkDeviceV0, StorageBackendV0, StorageDeviceV0}, PciPath, }, DiskRequest, NetworkInterfaceRequest, Slot, @@ -132,10 +129,7 @@ pub(super) fn parse_nic_from_request( pci_path, }); - let backend_spec = NetworkBackendV0::Virtio(VirtioNetworkBackend { - vnic_name: nic.name.to_string(), - }); - + let backend_spec = VirtioNetworkBackend { vnic_name: nic.name.to_string() }; Ok(Nic { device_name, device_spec, backend_name, backend_spec }) } diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index acc93c1a9..fb851ee75 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -7,14 +7,12 @@ use propolis_api_types::instance_spec::{ components::devices::{SerialPort as SerialPortSpec, SerialPortNumber}, - v0::{InstanceSpecV0, NetworkDeviceV0, StorageDeviceV0}, + v0::{InstanceSpecV0, NetworkBackendV0, NetworkDeviceV0, StorageDeviceV0}, }; use thiserror::Error; #[cfg(feature = "falcon")] -use propolis_api_types::instance_spec::{ - components::devices::SoftNpuPort as SoftNpuPortSpec, v0::NetworkBackendV0, -}; +use propolis_api_types::instance_spec::components::devices::SoftNpuPort as SoftNpuPortSpec; #[cfg(feature = "falcon")] use crate::spec::SoftNpuPort; @@ -35,6 +33,9 @@ pub(crate) enum ApiSpecParseError { #[error("backend {0} not used by any device")] BackendNotUsed(String), + #[error("network backend for guest NIC {0} is not a viona backend")] + GuestNicInvalidBackend(String), + #[cfg(feature = "falcon")] #[error("network backend for device {0} is not a DLPI backend")] NotDlpiBackend(String), @@ -69,10 +70,10 @@ impl From for InstanceSpecV0 { assert!(_old.is_none()); - let _old = spec - .backends - .network_backends - .insert(nic.backend_name, nic.backend_spec); + let _old = spec.backends.network_backends.insert( + nic.backend_name, + NetworkBackendV0::Virtio(nic.backend_spec), + ); assert!(_old.is_none()); } @@ -184,6 +185,12 @@ impl TryFrom for Spec { ) })?; + let NetworkBackendV0::Virtio(backend_spec) = backend_spec else { + return Err(ApiSpecParseError::GuestNicInvalidBackend( + device_name, + )); + }; + builder.add_network_device(Nic { device_name, device_spec, diff --git a/bin/propolis-server/src/lib/spec/config_toml.rs b/bin/propolis-server/src/lib/spec/config_toml.rs index 6eb69fbf6..f28ca76dd 100644 --- a/bin/propolis-server/src/lib/spec/config_toml.rs +++ b/bin/propolis-server/src/lib/spec/config_toml.rs @@ -13,9 +13,7 @@ use propolis_api_types::instance_spec::{ NvmeDisk, PciPciBridge as BridgeSpec, VirtioDisk, VirtioNic, }, }, - v0::{ - NetworkBackendV0, NetworkDeviceV0, StorageBackendV0, StorageDeviceV0, - }, + v0::{NetworkDeviceV0, StorageBackendV0, StorageDeviceV0}, PciPath, }; use thiserror::Error; @@ -296,9 +294,7 @@ pub(super) fn parse_network_device_from_config( .ok_or_else(|| ConfigTomlError::InvalidPciPath(name.to_owned()))?; let (device_name, backend_name) = super::pci_path_to_nic_names(pci_path); - let backend_spec = NetworkBackendV0::Virtio(VirtioNetworkBackend { - vnic_name: vnic_name.to_owned(), - }); + let backend_spec = VirtioNetworkBackend { vnic_name: vnic_name.to_owned() }; let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic { backend_name: backend_name.clone(), diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index e056f7f32..513d794c7 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -15,6 +15,7 @@ use propolis_api_types::instance_spec::{ components::{ + backends::VirtioNetworkBackend, board::Board, devices::{ PciPciBridge as PciPciBridgeSpec, QemuPvpanic as QemuPvpanicSpec, @@ -37,19 +38,19 @@ mod config_toml; #[derive(Clone, Debug, Default)] pub(crate) struct Spec { - board: Board, - disks: Vec, - nics: Vec, + pub board: Board, + pub disks: Vec, + pub nics: Vec, // TODO(#735): Preserve device names for identification purposes. - serial: [SerialPort; 4], + pub serial: [SerialPort; 4], // TODO(#735): Preserve device names for identification purposes. - pci_pci_bridges: Vec, - pvpanic: Option, + pub pci_pci_bridges: Vec, + pub pvpanic: Option, #[cfg(feature = "falcon")] - softnpu: SoftNpu, + pub softnpu: SoftNpu, } /// Describes a storage device/backend pair parsed from an input source like an @@ -62,14 +63,12 @@ pub struct Disk { pub backend_spec: StorageBackendV0, } -/// Describes a network device/backend pair parsed from an input source like an -/// API request or a config TOML entry. #[derive(Clone, Debug)] pub struct Nic { pub device_name: String, pub device_spec: NetworkDeviceV0, pub backend_name: String, - pub backend_spec: NetworkBackendV0, + pub backend_spec: VirtioNetworkBackend, } #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] diff --git a/bin/propolis-server/src/lib/stats/mod.rs b/bin/propolis-server/src/lib/stats/mod.rs index 683626ea3..4fd55ac12 100644 --- a/bin/propolis-server/src/lib/stats/mod.rs +++ b/bin/propolis-server/src/lib/stats/mod.rs @@ -14,12 +14,11 @@ use oximeter::{ }; use oximeter_instruments::kstat::KstatSampler; use oximeter_producer::{Config, Error, Server}; -use propolis_api_types::{ - instance_spec::v0::InstanceSpecV0, InstanceProperties, -}; +use propolis_api_types::InstanceProperties; use slog::Logger; use uuid::Uuid; +use crate::spec; use crate::{server::MetricsEndpointConfig, vm::NetworkInterfaceIds}; mod network_interface; @@ -190,11 +189,11 @@ pub fn start_oximeter_server( pub(crate) fn create_kstat_sampler( log: &Logger, properties: &InstanceProperties, - spec: &InstanceSpecV0, + spec: &spec::Spec, ) -> Option { let kstat_limit = usize::try_from( (u32::from(properties.vcpus) * KSTAT_LIMIT_PER_VCPU) - + (spec.devices.network_devices.len() as u32 * SAMPLE_BUFFER), + + (spec.nics.len() as u32 * SAMPLE_BUFFER), ) .unwrap(); diff --git a/bin/propolis-server/src/lib/vm/ensure.rs b/bin/propolis-server/src/lib/vm/ensure.rs index 081a9b3dd..84b1fd26d 100644 --- a/bin/propolis-server/src/lib/vm/ensure.rs +++ b/bin/propolis-server/src/lib/vm/ensure.rs @@ -30,9 +30,8 @@ use std::sync::Arc; use oximeter::types::ProducerRegistry; use oximeter_instruments::kstat::KstatSampler; use propolis_api_types::{ - instance_spec::v0::InstanceSpecV0, InstanceEnsureResponse, - InstanceMigrateInitiateRequest, InstanceMigrateInitiateResponse, - InstanceProperties, InstanceState, + InstanceEnsureResponse, InstanceMigrateInitiateRequest, + InstanceMigrateInitiateResponse, InstanceProperties, InstanceState, }; use slog::{debug, info}; @@ -168,10 +167,9 @@ impl<'a> VmEnsureNotStarted<'a> { // Set up the 'shell' instance into which the rest of this routine will // add components. - let v0_spec: InstanceSpecV0 = self.instance_spec().clone().into(); let machine = build_instance( &properties.vm_name(), - &v0_spec, + spec, options.use_reservoir, vmm_log, )?; @@ -182,7 +180,7 @@ impl<'a> VmEnsureNotStarted<'a> { devices: Default::default(), block_backends: Default::default(), crucible_backends: Default::default(), - spec: &v0_spec, + spec, properties, toml_config: &options.toml_config, producer_registry: options.oximeter_registry.clone(), @@ -190,7 +188,7 @@ impl<'a> VmEnsureNotStarted<'a> { kstat_sampler: initialize_kstat_sampler( self.log, properties, - &v0_spec, + self.instance_spec(), options.oximeter_registry.clone(), ), }; @@ -227,10 +225,8 @@ impl<'a> VmEnsureNotStarted<'a> { init.initialize_storage_devices(&chipset, options.nexus_client.clone()) .await?; - let ramfb = init.initialize_fwcfg(v0_spec.devices.board.cpus)?; - + let ramfb = init.initialize_fwcfg(self.instance_spec().board.cpus)?; init.initialize_cpus().await?; - let vcpu_tasks = Box::new(crate::vcpu_tasks::VcpuTasks::new( &machine, event_queue.clone() @@ -246,7 +242,7 @@ impl<'a> VmEnsureNotStarted<'a> { } = init; Ok(InputVmObjects { - instance_spec: v0_spec.clone(), + instance_spec: spec.clone(), vcpu_tasks, machine, devices, @@ -383,7 +379,7 @@ impl<'a> VmEnsureActive<'a> { fn initialize_kstat_sampler( log: &slog::Logger, properties: &InstanceProperties, - spec: &InstanceSpecV0, + spec: &spec::Spec, producer_registry: Option, ) -> Option { let registry = producer_registry?; diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index c9fe469f5..ae1f2212c 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -85,17 +85,17 @@ use active::ActiveVm; use ensure::VmEnsureRequest; use oximeter::types::ProducerRegistry; use propolis_api_types::{ - instance_spec::{v0::InstanceSpecV0, VersionedInstanceSpec}, - InstanceEnsureResponse, InstanceMigrateStatusResponse, - InstanceMigrationStatus, InstanceProperties, InstanceSpecGetResponse, - InstanceState, InstanceStateMonitorResponse, MigrationState, + instance_spec::VersionedInstanceSpec, InstanceEnsureResponse, + InstanceMigrateStatusResponse, InstanceMigrationStatus, InstanceProperties, + InstanceSpecGetResponse, InstanceState, InstanceStateMonitorResponse, + MigrationState, }; use slog::info; use state_driver::StateDriverOutput; use state_publisher::StatePublisher; use tokio::sync::{oneshot, watch, RwLock, RwLockReadGuard}; -use crate::{server::MetricsEndpointConfig, vnc::VncServer}; +use crate::{server::MetricsEndpointConfig, spec, vnc::VncServer}; mod active; pub(crate) mod ensure; @@ -219,7 +219,7 @@ struct VmDescription { properties: InstanceProperties, /// The VM's last-known instance specification. - spec: InstanceSpecV0, + spec: spec::Spec, /// The runtime on which the VM's state driver is running (or on which it /// ran). @@ -330,7 +330,7 @@ impl Vm { let state = vm.external_state_rx.borrow().clone(); Ok(InstanceSpecGetResponse { properties: vm.properties.clone(), - spec: VersionedInstanceSpec::V0(spec), + spec: VersionedInstanceSpec::V0(spec.into()), state: state.state, }) } @@ -343,7 +343,7 @@ impl Vm { | VmState::RundownComplete(vm) => Ok(InstanceSpecGetResponse { properties: vm.properties.clone(), state: vm.external_state_rx.borrow().state, - spec: VersionedInstanceSpec::V0(vm.spec.clone()), + spec: VersionedInstanceSpec::V0(vm.spec.clone().into()), }), } } @@ -554,12 +554,10 @@ impl Vm { _ => {} }; - let v0_spec: InstanceSpecV0 = - ensure_request.instance_spec.clone().into(); - let thread_count = usize::max( VMM_MIN_RT_THREADS, - VMM_BASE_RT_THREADS + v0_spec.devices.board.cpus as usize, + VMM_BASE_RT_THREADS + + ensure_request.instance_spec.board.cpus as usize, ); let tokio_rt = tokio::runtime::Builder::new_multi_thread() @@ -570,6 +568,7 @@ impl Vm { .map_err(VmError::TokioRuntimeInitializationFailed)?; let properties = ensure_request.properties.clone(); + let spec = ensure_request.instance_spec.clone(); let vm_for_driver = self.clone(); guard.driver = Some(tokio_rt.spawn(async move { state_driver::run_state_driver( @@ -586,7 +585,7 @@ impl Vm { guard.state = VmState::WaitingForInit(VmDescription { external_state_rx: external_rx.clone(), properties, - spec: v0_spec, + spec, tokio_rt: Some(tokio_rt), }); } diff --git a/bin/propolis-server/src/lib/vm/objects.rs b/bin/propolis-server/src/lib/vm/objects.rs index 79e9897ee..45f450de7 100644 --- a/bin/propolis-server/src/lib/vm/objects.rs +++ b/bin/propolis-server/src/lib/vm/objects.rs @@ -17,11 +17,10 @@ use propolis::{ vmm::VmmHdl, Machine, }; -use propolis_api_types::instance_spec::v0::InstanceSpecV0; use slog::{error, info}; use tokio::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; -use crate::{serial::Serial, vcpu_tasks::VcpuTaskController}; +use crate::{serial::Serial, spec, vcpu_tasks::VcpuTaskController}; use super::{ state_driver::VmStartReason, BlockBackendMap, CrucibleBackendMap, DeviceMap, @@ -44,7 +43,7 @@ pub(crate) struct VmObjects { /// A collection of objects that should eventually be wrapped in a lock and /// stored in a `VmObjects` structure. See [`VmObjectsLocked`]. pub(super) struct InputVmObjects { - pub instance_spec: InstanceSpecV0, + pub instance_spec: spec::Spec, pub vcpu_tasks: Box, pub machine: Machine, pub devices: DeviceMap, @@ -61,7 +60,7 @@ pub(crate) struct VmObjectsLocked { log: slog::Logger, /// The instance spec that describes this collection of objects. - instance_spec: InstanceSpecV0, + instance_spec: spec::Spec, /// The set of tasks that run this VM's vCPUs. vcpu_tasks: Box, @@ -132,12 +131,12 @@ impl VmObjectsLocked { } /// Yields the VM's current instance spec. - pub(crate) fn instance_spec(&self) -> &InstanceSpecV0 { + pub(crate) fn instance_spec(&self) -> &spec::Spec { &self.instance_spec } /// Yields a mutable reference to the VM's current instance spec. - pub(crate) fn instance_spec_mut(&mut self) -> &mut InstanceSpecV0 { + pub(crate) fn instance_spec_mut(&mut self) -> &mut spec::Spec { &mut self.instance_spec } diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index 90647debb..be364b62c 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -661,50 +661,49 @@ impl StateDriver { } let mut objects = self.objects.lock_exclusive().await; - let (readonly, old_vcr_json) = { - let StorageBackendV0::Crucible(bes) = objects - .instance_spec() - .backends - .storage_backends - .get(&disk_name) - .ok_or_else(|| spec_element_not_found(&disk_name))? - else { - return Err(spec_element_not_found(&disk_name)); - }; - - (bes.readonly, &bes.request_json) + let backend = objects + .crucible_backends() + .get(backend_id) + .ok_or_else(|| { + let msg = format!("No crucible backend for id {backend_id}"); + dropshot::HttpError::for_not_found(Some(msg.clone()), msg) + })? + .clone(); + + // TODO(gjc) would be better off with a map in the spec type, but this + // will do for now + let Some(disk) = objects + .instance_spec_mut() + .disks + .iter_mut() + .find(|disk| disk.device_name == disk_name) + else { + return Err(spec_element_not_found(&disk_name)); }; - let replace_result = { - let backend = objects - .crucible_backends() - .get(backend_id) - .ok_or_else(|| { - let msg = - format!("No crucible backend for id {backend_id}"); - dropshot::HttpError::for_not_found(Some(msg.clone()), msg) - })?; - - backend.vcr_replace(old_vcr_json, &new_vcr_json).await.map_err( - |e| { - dropshot::HttpError::for_bad_request( - Some(e.to_string()), - e.to_string(), - ) - }, - ) - }?; - - let new_bes = StorageBackendV0::Crucible(CrucibleStorageBackend { + let StorageBackendV0::Crucible(CrucibleStorageBackend { + request_json: old_vcr_json, readonly, - request_json: new_vcr_json, - }); + }) = &disk.backend_spec + else { + return Err(spec_element_not_found(&disk_name)); + }; - objects - .instance_spec_mut() - .backends - .storage_backends - .insert(disk_name, new_bes); + let replace_result = backend + .vcr_replace(old_vcr_json, &new_vcr_json) + .await + .map_err(|e| { + dropshot::HttpError::for_bad_request( + Some(e.to_string()), + e.to_string(), + ) + })?; + + disk.backend_spec = + StorageBackendV0::Crucible(CrucibleStorageBackend { + readonly: *readonly, + request_json: new_vcr_json, + }); info!(self.log, "replaced Crucible VCR"; "backend_id" => %backend_id); From 65232de3d032f06818aed50ff66e7d8a80adec9c Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 26 Jul 2024 18:31:23 +0000 Subject: [PATCH 03/11] refactor: be more careful about preserving component names --- bin/propolis-server/src/lib/initializer.rs | 60 ++++++------ bin/propolis-server/src/lib/server.rs | 7 +- .../src/lib/spec/api_request.rs | 24 +++-- .../src/lib/spec/api_spec_v0.rs | 92 +++++++++---------- bin/propolis-server/src/lib/spec/builder.rs | 81 ++++++++-------- .../src/lib/spec/config_toml.rs | 62 +++++++------ bin/propolis-server/src/lib/spec/mod.rs | 70 +++++++++----- .../src/lib/vm/state_driver.rs | 8 +- .../src/instance_spec/components/devices.rs | 2 +- 9 files changed, 216 insertions(+), 190 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index e86d8af90..46b36835a 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -40,6 +40,7 @@ use propolis::hw::{nvme, virtio}; use propolis::intr_pins; use propolis::vmm::{self, Builder, Machine}; use propolis_api_types::instance_spec; +use propolis_api_types::instance_spec::components::devices::SerialPortNumber; use propolis_api_types::InstanceProperties; use slog::info; @@ -195,16 +196,15 @@ impl<'a> MachineInitializer<'a> { event_handler: &Arc, ) -> Result { let mut pci_builder = pci::topology::Builder::new(); - for bridge in &self.spec.pci_pci_bridges { + for (name, bridge) in &self.spec.pci_pci_bridges { let desc = pci::topology::BridgeDescription::new( - pci::topology::LogicalBusId(bridge.0.downstream_bus), - bridge.0.pci_path.try_into().map_err(|e| { + 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 {}: {}", - bridge.name(), - e + name, e ), ) })?, @@ -298,24 +298,31 @@ impl<'a> MachineInitializer<'a> { chipset: &RegisteredChipset, ) -> Result, Error> { let mut com1 = None; - for (index, serial_spec) in self.spec.serial.iter().enumerate() { - if *serial_spec != spec::SerialPort::Enabled { + for (num, user) in self.spec.serial.iter() { + if *user != spec::SerialPortUser::Standard { continue; } - let (name, irq, port) = match index { - 0 => ("com1", ibmpc::IRQ_COM1, ibmpc::PORT_COM1), - 1 => ("com2", ibmpc::IRQ_COM2, ibmpc::PORT_COM2), - 2 => ("com3", ibmpc::IRQ_COM3, ibmpc::PORT_COM3), - 3 => ("com4", ibmpc::IRQ_COM4, ibmpc::PORT_COM4), - _ => unreachable!(), + let (name, irq, port) = match num { + SerialPortNumber::Com1 => { + ("com1", ibmpc::IRQ_COM1, ibmpc::PORT_COM1) + } + SerialPortNumber::Com2 => { + ("com2", ibmpc::IRQ_COM2, ibmpc::PORT_COM2) + } + SerialPortNumber::Com3 => { + ("com3", ibmpc::IRQ_COM3, ibmpc::PORT_COM3) + } + SerialPortNumber::Com4 => { + ("com4", ibmpc::IRQ_COM4, ibmpc::PORT_COM4) + } }; let dev = LpcUart::new(chipset.irq_pin(irq).unwrap()); dev.set_autodiscard(true); LpcUart::attach(&dev, &self.machine.bus_pio, port); self.devices.insert(name.to_owned(), dev.clone()); - if index == 0 { + if *num == SerialPortNumber::Com1 { assert!(com1.is_none()); com1 = Some(dev); } @@ -358,8 +365,8 @@ impl<'a> MachineInitializer<'a> { &mut self, virtual_machine: VirtualMachine, ) -> Result<(), anyhow::Error> { - if let Some(spec) = &self.spec.pvpanic { - if spec.0.enable_isa { + if let Some(pvpanic) = &self.spec.pvpanic { + if pvpanic.spec.enable_isa { let pvpanic = QemuPvpanic::create( self.log.new(slog::o!("dev" => "qemu-pvpanic")), ); @@ -502,11 +509,11 @@ impl<'a> MachineInitializer<'a> { Nvme, } - 'devloop: for disk in &self.spec.disks { + 'devloop: for (disk_name, disk) in &self.spec.disks { info!( self.log, "Creating storage device {} with properties {:?}", - disk.device_name, + disk_name, disk.device_spec ); @@ -526,7 +533,7 @@ impl<'a> MachineInitializer<'a> { ErrorKind::InvalidInput, format!( "Couldn't get PCI BDF for storage device {}: {}", - disk.device_name, e + disk_name, e ), ) })?; @@ -553,9 +560,9 @@ impl<'a> MachineInitializer<'a> { DeviceInterface::Nvme => { // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); - let component = format!("nvme-{}", disk.device_name); + let component = format!("nvme-{}", disk_name); let nvme = nvme::PciNvme::create( - disk.device_name.to_string(), + disk_name.to_owned(), mdts, self.log.new(slog::o!("component" => component)), ); @@ -633,9 +640,8 @@ impl<'a> MachineInitializer<'a> { let mut interface_ids: Option = self.kstat_sampler.as_ref().map(|_| Vec::new()); - for nic in &self.spec.nics { - info!(self.log, "Creating vNIC {}", nic.device_name); - + for (device_name, nic) in &self.spec.nics { + info!(self.log, "Creating vNIC {}", device_name); let instance_spec::v0::NetworkDeviceV0::VirtioNic(vnic_spec) = &nic.device_spec; @@ -644,7 +650,7 @@ impl<'a> MachineInitializer<'a> { ErrorKind::InvalidInput, format!( "Couldn't get PCI BDF for vNIC {}: {}", - nic.device_name, e + device_name, e ), ) })?; @@ -750,10 +756,10 @@ impl<'a> MachineInitializer<'a> { // SoftNpu ports are named __vnic by falcon, where // indicates the intended order. - ports.sort_by_key(|p| p.name.as_str()); + ports.sort_by_key(|p| p.0.as_str()); let data_links = ports .iter() - .map(|port| port.backend_spec.vnic_name.clone()) + .map(|port| port.1.backend_spec.vnic_name.clone()) .collect(); // Set up an LPC uart for ASIC management comms from the guest. diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index 084230f9d..e2022b266 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -143,9 +143,10 @@ fn instance_spec_from_request( #[cfg(feature = "falcon")] spec_builder.set_softnpu_com4()?; - spec_builder.add_pvpanic_device(spec::QemuPvpanic(QemuPvpanic { - enable_isa: true, - }))?; + spec_builder.add_pvpanic_device(spec::QemuPvpanic { + name: "pvpanic".to_string(), + spec: QemuPvpanic { enable_isa: true }, + })?; Ok(spec_builder.finish()) } diff --git a/bin/propolis-server/src/lib/spec/api_request.rs b/bin/propolis-server/src/lib/spec/api_request.rs index 7369ec6fe..393f0e654 100644 --- a/bin/propolis-server/src/lib/spec/api_request.rs +++ b/bin/propolis-server/src/lib/spec/api_request.rs @@ -22,7 +22,7 @@ use propolis_api_types::{ }; use thiserror::Error; -use super::{Disk, Nic}; +use super::{Disk, Nic, ParsedDiskRequest, ParsedNicRequest}; #[derive(Debug, Error)] pub(crate) enum DeviceRequestError { @@ -68,7 +68,7 @@ fn slot_to_pci_path( pub(super) fn parse_disk_from_request( disk: &DiskRequest, -) -> Result { +) -> Result { let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?; let backend_name = format!("{}-backend", disk.name); let device_spec = match disk.device.as_ref() { @@ -97,30 +97,35 @@ pub(super) fn parse_disk_from_request( readonly: disk.read_only, }); - Ok(Disk { device_name, device_spec, backend_name, backend_spec }) + Ok(ParsedDiskRequest { + name: device_name, + disk: Disk { device_spec, backend_name, backend_spec }, + }) } pub(super) fn parse_cloud_init_from_request( base64: String, -) -> Result { +) -> Result { let name = "cloud-init"; let pci_path = slot_to_pci_path(Slot(0), SlotType::CloudInit)?; let backend_name = name.to_string(); let backend_spec = StorageBackendV0::Blob(BlobStorageBackend { base64, readonly: true }); - let device_name = name.to_string(); let device_spec = StorageDeviceV0::VirtioDisk(VirtioDisk { backend_name: name.to_string(), pci_path, }); - Ok(Disk { device_name, device_spec, backend_name, backend_spec }) + Ok(ParsedDiskRequest { + name: name.to_owned(), + disk: Disk { device_spec, backend_name, backend_spec }, + }) } pub(super) fn parse_nic_from_request( nic: &NetworkInterfaceRequest, -) -> Result { +) -> Result { let pci_path = slot_to_pci_path(nic.slot, SlotType::Nic)?; let (device_name, backend_name) = super::pci_path_to_nic_names(pci_path); let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic { @@ -130,7 +135,10 @@ pub(super) fn parse_nic_from_request( }); let backend_spec = VirtioNetworkBackend { vnic_name: nic.name.to_string() }; - Ok(Nic { device_name, device_spec, backend_name, backend_spec }) + Ok(ParsedNicRequest { + name: device_name, + nic: Nic { device_spec, backend_name, backend_spec }, + }) } #[cfg(test)] diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index fb851ee75..d6a6aa2dc 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -6,7 +6,7 @@ //! crate to the internal [`super::Spec`] representation. use propolis_api_types::instance_spec::{ - components::devices::{SerialPort as SerialPortSpec, SerialPortNumber}, + components::devices::{SerialPort as SerialPortDesc, SerialPortNumber}, v0::{InstanceSpecV0, NetworkBackendV0, NetworkDeviceV0, StorageDeviceV0}, }; use thiserror::Error; @@ -19,7 +19,7 @@ use crate::spec::SoftNpuPort; use super::{ builder::{SpecBuilder, SpecBuilderError}, - Disk, Nic, PciPciBridge, QemuPvpanic, SerialPort, Spec, + Disk, Nic, QemuPvpanic, SerialPortUser, Spec, }; #[derive(Debug, Error)] @@ -46,11 +46,11 @@ impl From for InstanceSpecV0 { let mut spec = InstanceSpecV0::default(); spec.devices.board = val.board; - for disk in val.disks { + for (disk_name, disk) in val.disks { let _old = spec .devices .storage_devices - .insert(disk.device_name, disk.device_spec); + .insert(disk_name, disk.device_spec); assert!(_old.is_none()); @@ -62,11 +62,9 @@ impl From for InstanceSpecV0 { assert!(_old.is_none()); } - for nic in val.nics { - let _old = spec - .devices - .network_devices - .insert(nic.device_name, nic.device_spec); + for (nic_name, nic) in val.nics { + let _old = + spec.devices.network_devices.insert(nic_name, nic.device_spec); assert!(_old.is_none()); @@ -78,44 +76,41 @@ impl From for InstanceSpecV0 { assert!(_old.is_none()); } - for (index, serial) in val.serial.iter().enumerate() { - if *serial == SerialPort::Enabled { - let _old = spec.devices.serial_ports.insert( - format!("com{}", index + 1), - SerialPortSpec { - num: match index { - 0 => SerialPortNumber::Com1, - 1 => SerialPortNumber::Com2, - 2 => SerialPortNumber::Com3, - 3 => SerialPortNumber::Com4, - _ => unreachable!(), - }, - }, - ); + for (num, user) in val.serial.iter() { + if *user == SerialPortUser::Standard { + let name = match num { + SerialPortNumber::Com1 => "com1", + SerialPortNumber::Com2 => "com2", + SerialPortNumber::Com3 => "com3", + SerialPortNumber::Com4 => "com4", + }; + + let _old = spec + .devices + .serial_ports + .insert(name.to_owned(), SerialPortDesc { num: *num }); assert!(_old.is_none()); } } - for bridge in val.pci_pci_bridges { - let _old = - spec.devices.pci_pci_bridges.insert(bridge.name(), bridge.0); - + for (bridge_name, bridge) in val.pci_pci_bridges { + let _old = spec.devices.pci_pci_bridges.insert(bridge_name, bridge); assert!(_old.is_none()); } - spec.devices.qemu_pvpanic = val.pvpanic.map(|pvpanic| pvpanic.0); + spec.devices.qemu_pvpanic = val.pvpanic.map(|pvpanic| pvpanic.spec); #[cfg(feature = "falcon")] { spec.devices.softnpu_pci_port = val.softnpu.pci_port; spec.devices.softnpu_p9 = val.softnpu.p9_device; spec.devices.p9fs = val.softnpu.p9fs; - for port in val.softnpu.ports { + for (port_name, port) in val.softnpu.ports { let _old = spec.devices.softnpu_ports.insert( - port.name.clone(), + port_name.clone(), SoftNpuPortSpec { - name: port.name, + name: port_name, backend_name: port.backend_name.clone(), }, ); @@ -157,12 +152,10 @@ impl TryFrom for Spec { ) })?; - builder.add_storage_device(Disk { + builder.add_storage_device( device_name, - device_spec, - backend_name, - backend_spec, - })?; + Disk { device_spec, backend_name, backend_spec }, + )?; } if let Some(backend) = value.backends.storage_backends.keys().next() { @@ -191,12 +184,10 @@ impl TryFrom for Spec { )); }; - builder.add_network_device(Nic { + builder.add_network_device( device_name, - device_spec, - backend_name, - backend_spec, - })?; + Nic { device_spec, backend_name, backend_spec }, + )?; } // SoftNPU ports can have network backends, so consume the SoftNPU @@ -232,11 +223,10 @@ impl TryFrom for Spec { return Err(ApiSpecParseError::NotDlpiBackend(port_name)); }; - builder.add_softnpu_port(SoftNpuPort { - name: port_name, - backend_name, - backend_spec, - })?; + builder.add_softnpu_port( + port_name, + SoftNpuPort { backend_name, backend_spec }, + )?; } } @@ -249,13 +239,15 @@ impl TryFrom for Spec { builder.add_serial_port(serial_port.num)?; } - // TODO(gjc) do more to preserve names - for bridge in value.devices.pci_pci_bridges.into_values() { - builder.add_pci_bridge(PciPciBridge(bridge))?; + for (name, bridge) in value.devices.pci_pci_bridges { + builder.add_pci_bridge(name, bridge)?; } if let Some(pvpanic) = value.devices.qemu_pvpanic { - builder.add_pvpanic_device(QemuPvpanic(pvpanic))?; + builder.add_pvpanic_device(QemuPvpanic { + name: "pvpanic".to_string(), + spec: pvpanic, + })?; } Ok(builder.finish()) diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index b5c3a72f2..7af844f24 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -10,7 +10,7 @@ use propolis_api_types::{ instance_spec::{ components::{ board::{Board, Chipset, I440Fx}, - devices::SerialPortNumber, + devices::{PciPciBridge, SerialPortNumber}, }, v0::{NetworkDeviceV0, StorageDeviceV0}, PciPath, @@ -24,16 +24,16 @@ use propolis_api_types::instance_spec::components::devices::{ P9fs, SoftNpuP9, SoftNpuPciPort, }; -use crate::config; +use crate::{config, spec::SerialPortUser}; use super::{ api_request::{self, DeviceRequestError}, config_toml::{ConfigTomlError, ParsedConfig}, - Disk, Nic, PciPciBridge, QemuPvpanic, + Disk, Nic, QemuPvpanic, }; #[cfg(feature = "falcon")] -use super::{SoftNpu, SoftNpuPort}; +use super::{ParsedSoftNpu, SoftNpuPort}; /// Errors that can arise while building an instance spec from component parts. #[derive(Debug, Error)] @@ -112,7 +112,8 @@ impl SpecBuilder { &mut self, nic: &NetworkInterfaceRequest, ) -> Result<(), SpecBuilderError> { - self.add_network_device(api_request::parse_nic_from_request(nic)?)?; + let parsed = api_request::parse_nic_from_request(nic)?; + self.add_network_device(parsed.name, parsed.nic)?; Ok(()) } @@ -122,7 +123,8 @@ impl SpecBuilder { &mut self, disk: &DiskRequest, ) -> Result<(), SpecBuilderError> { - self.add_storage_device(api_request::parse_disk_from_request(disk)?)?; + let parsed = api_request::parse_disk_from_request(disk)?; + self.add_storage_device(parsed.name, parsed.disk)?; Ok(()) } @@ -132,10 +134,8 @@ impl SpecBuilder { &mut self, base64: String, ) -> Result<(), SpecBuilderError> { - self.add_storage_device(api_request::parse_cloud_init_from_request( - base64, - )?)?; - + let parsed = api_request::parse_cloud_init_from_request(base64)?; + self.add_storage_device(parsed.name, parsed.disk)?; Ok(()) } @@ -151,15 +151,15 @@ impl SpecBuilder { i440fx.enable_pcie = parsed.enable_pcie; for disk in parsed.disks { - self.add_storage_device(disk)?; + self.add_storage_device(disk.name, disk.disk)?; } for nic in parsed.nics { - self.add_network_device(nic)?; + self.add_network_device(nic.name, nic.nic)?; } for bridge in parsed.pci_bridges { - self.add_pci_bridge(bridge)?; + self.add_pci_bridge(bridge.name, bridge.bridge)?; } #[cfg(feature = "falcon")] @@ -171,14 +171,14 @@ impl SpecBuilder { #[cfg(feature = "falcon")] fn add_parsed_softnpu_devices( &mut self, - devices: SoftNpu, + devices: ParsedSoftNpu, ) -> Result<(), SpecBuilderError> { if let Some(pci_port) = devices.pci_port { self.set_softnpu_pci_port(pci_port)?; } for port in devices.ports { - self.add_softnpu_port(port)?; + self.add_softnpu_port(port.name, port.port)?; } if let Some(p9) = devices.p9_device { @@ -209,10 +209,11 @@ impl SpecBuilder { /// Adds a storage device with an associated backend. pub(super) fn add_storage_device( &mut self, + disk_name: String, disk: Disk, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&disk.device_name) { - return Err(SpecBuilderError::ComponentNameInUse(disk.device_name)); + if self.component_names.contains(&disk_name) { + return Err(SpecBuilderError::ComponentNameInUse(disk_name)); } if self.component_names.contains(&disk.backend_name) { @@ -222,19 +223,21 @@ impl SpecBuilder { } self.register_pci_device(disk.device_spec.pci_path())?; - self.component_names.insert(disk.device_name.clone()); + self.component_names.insert(disk_name.clone()); self.component_names.insert(disk.backend_name.clone()); - self.spec.disks.push(disk); + let _old = self.spec.disks.insert(disk_name, disk); + assert!(_old.is_none()); Ok(self) } /// Adds a network device with an associated backend. pub(super) fn add_network_device( &mut self, + nic_name: String, nic: Nic, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&nic.device_name) { - return Err(SpecBuilderError::ComponentNameInUse(nic.device_name)); + if self.component_names.contains(&nic_name) { + return Err(SpecBuilderError::ComponentNameInUse(nic_name)); } if self.component_names.contains(&nic.backend_name) { @@ -242,24 +245,26 @@ impl SpecBuilder { } self.register_pci_device(nic.device_spec.pci_path())?; - self.component_names.insert(nic.device_name.clone()); + self.component_names.insert(nic_name.clone()); self.component_names.insert(nic.backend_name.clone()); - self.spec.nics.push(nic); + let _old = self.spec.nics.insert(nic_name, nic); + assert!(_old.is_none()); Ok(self) } /// Adds a PCI-PCI bridge. pub fn add_pci_bridge( &mut self, + name: String, bridge: PciPciBridge, ) -> Result<&Self, SpecBuilderError> { - let name = bridge.name(); if self.component_names.contains(&name) { return Err(SpecBuilderError::ComponentNameInUse(name)); } - self.register_pci_device(bridge.0.pci_path)?; - self.spec.pci_pci_bridges.push(bridge); + self.register_pci_device(bridge.pci_path)?; + let _old = self.spec.pci_pci_bridges.insert(name, bridge); + assert!(_old.is_none()); Ok(self) } @@ -268,17 +273,9 @@ impl SpecBuilder { &mut self, port: SerialPortNumber, ) -> Result<&Self, SpecBuilderError> { - let idx = match port { - SerialPortNumber::Com1 => 0, - SerialPortNumber::Com2 => 1, - SerialPortNumber::Com3 => 2, - SerialPortNumber::Com4 => 3, - }; - - if self.spec.serial[idx] != super::SerialPort::Disabled { + if self.spec.serial.insert(port, SerialPortUser::Standard).is_some() { Err(SpecBuilderError::SerialPortInUse(port)) } else { - self.spec.serial[idx] = super::SerialPort::Enabled; Ok(self) } } @@ -297,10 +294,10 @@ impl SpecBuilder { #[cfg(feature = "falcon")] pub fn set_softnpu_com4(&mut self) -> Result<&Self, SpecBuilderError> { - if self.spec.serial[3] != super::SerialPort::Disabled { - Err(SpecBuilderError::SerialPortInUse(SerialPortNumber::Com4)) + let port = SerialPortNumber::Com4; + if self.spec.serial.insert(port, SerialPortUser::SoftNpu).is_some() { + Err(SpecBuilderError::SerialPortInUse(port)) } else { - self.spec.serial[3] = super::SerialPort::SoftNpu; Ok(self) } } @@ -335,10 +332,11 @@ impl SpecBuilder { #[cfg(feature = "falcon")] pub fn add_softnpu_port( &mut self, + port_name: String, port: SoftNpuPort, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&port.name) { - return Err(SpecBuilderError::ComponentNameInUse(port.name)); + if self.component_names.contains(&port_name) { + return Err(SpecBuilderError::ComponentNameInUse(port_name)); } if self.component_names.contains(&port.backend_name) { @@ -347,7 +345,8 @@ impl SpecBuilder { )); } - self.spec.softnpu.ports.push(port); + let _old = self.spec.softnpu.ports.insert(port_name, port); + assert!(_old.is_none()); Ok(self) } diff --git a/bin/propolis-server/src/lib/spec/config_toml.rs b/bin/propolis-server/src/lib/spec/config_toml.rs index f28ca76dd..2f059a8dd 100644 --- a/bin/propolis-server/src/lib/spec/config_toml.rs +++ b/bin/propolis-server/src/lib/spec/config_toml.rs @@ -9,9 +9,7 @@ use std::str::{FromStr, ParseBoolError}; use propolis_api_types::instance_spec::{ components::{ backends::{FileStorageBackend, VirtioNetworkBackend}, - devices::{ - NvmeDisk, PciPciBridge as BridgeSpec, VirtioDisk, VirtioNic, - }, + devices::{NvmeDisk, PciPciBridge, VirtioDisk, VirtioNic}, }, v0::{NetworkDeviceV0, StorageBackendV0, StorageDeviceV0}, PciPath, @@ -23,15 +21,14 @@ use propolis_api_types::instance_spec::components::devices::{ P9fs, SoftNpuP9, SoftNpuPciPort, }; -#[cfg(feature = "falcon")] -use super::SoftNpuPort; - use crate::config; -use super::{Disk, Nic, PciPciBridge}; +use super::{ + Disk, Nic, ParsedDiskRequest, ParsedNicRequest, ParsedPciBridgeRequest, +}; #[cfg(feature = "falcon")] -use super::SoftNpu; +use super::{ParsedSoftNpu, ParsedSoftNpuPort, SoftNpuPort}; #[derive(Debug, Error)] pub(crate) enum ConfigTomlError { @@ -80,12 +77,12 @@ pub(crate) enum ConfigTomlError { #[derive(Default)] pub(super) struct ParsedConfig { pub(super) enable_pcie: bool, - pub(super) disks: Vec, - pub(super) nics: Vec, - pub(super) pci_bridges: Vec, + pub(super) disks: Vec, + pub(super) nics: Vec, + pub(super) pci_bridges: Vec, #[cfg(feature = "falcon")] - pub(super) softnpu: SoftNpu, + pub(super) softnpu: ParsedSoftNpu, } impl TryFrom<&config::Config> for ParsedConfig { @@ -138,11 +135,9 @@ impl TryFrom<&config::Config> for ParsedConfig { backend_config, )?; - parsed.disks.push(Disk { - device_name: device_name.to_owned(), - device_spec, - backend_name, - backend_spec, + parsed.disks.push(ParsedDiskRequest { + name: device_name.to_owned(), + disk: Disk { device_spec, backend_name, backend_spec }, }); } "pci-virtio-viona" => { @@ -284,7 +279,7 @@ pub(super) fn parse_storage_device_from_config( pub(super) fn parse_network_device_from_config( name: &str, device: &config::Device, -) -> Result { +) -> Result { let vnic_name = device .get_string("vnic") .ok_or_else(|| ConfigTomlError::NoVnicName(name.to_owned()))?; @@ -304,20 +299,27 @@ pub(super) fn parse_network_device_from_config( pci_path, }); - Ok(Nic { device_name, device_spec, backend_name, backend_spec }) + Ok(ParsedNicRequest { + name: device_name, + nic: Nic { device_spec, backend_name, backend_spec }, + }) } pub(super) fn parse_pci_bridge_from_config( bridge: &config::PciBridge, -) -> Result { +) -> Result { let pci_path = PciPath::from_str(&bridge.pci_path).map_err(|e| { ConfigTomlError::PciPathParseFailed(bridge.pci_path.to_string(), e) })?; - Ok(PciPciBridge(BridgeSpec { - downstream_bus: bridge.downstream_bus, - pci_path, - })) + let name = format!("pci-bridge-{}", bridge.pci_path); + Ok(ParsedPciBridgeRequest { + name, + bridge: PciPciBridge { + downstream_bus: bridge.downstream_bus, + pci_path, + }, + }) } #[cfg(feature = "falcon")] @@ -348,17 +350,21 @@ pub(super) fn parse_softnpu_pci_port_from_config( pub(super) fn parse_softnpu_port_from_config( name: &str, device: &config::Device, -) -> Result { +) -> Result { use propolis_api_types::instance_spec::components::backends::DlpiNetworkBackend; let vnic_name = device .get_string("vnic") .ok_or_else(|| ConfigTomlError::NoVnicName(name.to_owned()))?; - Ok(SoftNpuPort { + Ok(ParsedSoftNpuPort { name: name.to_owned(), - backend_name: vnic_name.to_owned(), - backend_spec: DlpiNetworkBackend { vnic_name: vnic_name.to_owned() }, + port: SoftNpuPort { + backend_name: vnic_name.to_owned(), + backend_spec: DlpiNetworkBackend { + vnic_name: vnic_name.to_owned(), + }, + }, }) } diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 513d794c7..82669a9be 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -13,12 +13,14 @@ //! representation to take forms that might otherwise be hard to change in a //! backward-compatible way. +use std::collections::HashMap; + use propolis_api_types::instance_spec::{ components::{ backends::VirtioNetworkBackend, board::Board, devices::{ - PciPciBridge as PciPciBridgeSpec, QemuPvpanic as QemuPvpanicSpec, + PciPciBridge, QemuPvpanic as QemuPvpanicDesc, SerialPortNumber, }, }, v0::*, @@ -39,14 +41,12 @@ mod config_toml; #[derive(Clone, Debug, Default)] pub(crate) struct Spec { pub board: Board, - pub disks: Vec, - pub nics: Vec, + pub disks: HashMap, + pub nics: HashMap, - // TODO(#735): Preserve device names for identification purposes. - pub serial: [SerialPort; 4], + pub serial: HashMap, - // TODO(#735): Preserve device names for identification purposes. - pub pci_pci_bridges: Vec, + pub pci_pci_bridges: HashMap, pub pvpanic: Option, #[cfg(feature = "falcon")] @@ -57,7 +57,6 @@ pub(crate) struct Spec { /// API request or a config TOML entry. #[derive(Clone, Debug)] pub struct Disk { - pub device_name: String, pub device_spec: StorageDeviceV0, pub backend_name: String, pub backend_spec: StorageBackendV0, @@ -65,38 +64,29 @@ pub struct Disk { #[derive(Clone, Debug)] pub struct Nic { - pub device_name: String, pub device_spec: NetworkDeviceV0, pub backend_name: String, pub backend_spec: VirtioNetworkBackend, } -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] -pub enum SerialPort { - #[default] - Disabled, - Enabled, +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum SerialPortUser { + Standard, #[cfg(feature = "falcon")] SoftNpu, } #[derive(Clone, Debug)] -pub struct QemuPvpanic(pub QemuPvpanicSpec); - -#[derive(Clone, Debug)] -pub struct PciPciBridge(pub PciPciBridgeSpec); - -impl PciPciBridge { - pub fn name(&self) -> String { - format!("pci-bridge-{}", self.0.downstream_bus) - } +pub struct QemuPvpanic { + #[allow(dead_code)] + pub name: String, + pub spec: QemuPvpanicDesc, } #[cfg(feature = "falcon")] #[derive(Clone, Debug)] pub struct SoftNpuPort { - pub name: String, pub backend_name: String, pub backend_spec: DlpiNetworkBackend, } @@ -105,7 +95,37 @@ pub struct SoftNpuPort { #[derive(Clone, Debug, Default)] pub struct SoftNpu { pub pci_port: Option, - pub ports: Vec, + pub ports: HashMap, + pub p9_device: Option, + pub p9fs: Option, +} + +struct ParsedDiskRequest { + name: String, + disk: Disk, +} + +struct ParsedNicRequest { + name: String, + nic: Nic, +} + +struct ParsedPciBridgeRequest { + name: String, + bridge: PciPciBridge, +} + +#[cfg(feature = "falcon")] +struct ParsedSoftNpuPort { + name: String, + port: SoftNpuPort, +} + +#[cfg(feature = "falcon")] +#[derive(Default)] +struct ParsedSoftNpu { + pub pci_port: Option, + pub ports: Vec, pub p9_device: Option, pub p9fs: Option, } diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index be364b62c..70bae5a1a 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -670,13 +670,7 @@ impl StateDriver { })? .clone(); - // TODO(gjc) would be better off with a map in the spec type, but this - // will do for now - let Some(disk) = objects - .instance_spec_mut() - .disks - .iter_mut() - .find(|disk| disk.device_name == disk_name) + let Some(disk) = objects.instance_spec_mut().disks.get_mut(&disk_name) else { return Err(spec_element_not_found(&disk_name)); }; diff --git a/crates/propolis-api-types/src/instance_spec/components/devices.rs b/crates/propolis-api-types/src/instance_spec/components/devices.rs index 9f4e2e6c7..b99e5c3b9 100644 --- a/crates/propolis-api-types/src/instance_spec/components/devices.rs +++ b/crates/propolis-api-types/src/instance_spec/components/devices.rs @@ -125,7 +125,7 @@ impl MigrationElement for VirtioNic { /// A serial port identifier, which determines what I/O ports a guest can use to /// access a port. #[derive( - Clone, Copy, Deserialize, Serialize, Debug, PartialEq, Eq, JsonSchema, + Clone, Copy, Deserialize, Serialize, Debug, PartialEq, Eq, JsonSchema, Hash, )] #[serde(deny_unknown_fields, rename_all = "snake_case")] pub enum SerialPortNumber { From e2a31171051a94c870607e3d4271d26fc7bc45b3 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Sat, 27 Jul 2024 00:07:20 +0000 Subject: [PATCH 04/11] refactor: remove refs to V0 enums from internal spec type --- bin/propolis-server/src/lib/initializer.rs | 38 +++++----- .../src/lib/spec/api_request.rs | 34 +++++---- .../src/lib/spec/api_spec_v0.rs | 22 +++--- bin/propolis-server/src/lib/spec/builder.rs | 24 +----- .../src/lib/spec/config_toml.rs | 27 +++---- bin/propolis-server/src/lib/spec/mod.rs | 76 +++++++++++++++++-- .../src/lib/vm/state_driver.rs | 18 ++--- 7 files changed, 140 insertions(+), 99 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 46b36835a..2088dce20 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use crate::serial::Serial; -use crate::spec; +use crate::spec::{self, StorageBackend}; use crate::stats::{ track_network_interface_kstats, track_vcpu_kstats, VirtualDiskProducer, VirtualMachine, @@ -391,12 +391,12 @@ impl<'a> MachineInitializer<'a> { async fn create_storage_backend_from_spec( &self, - backend_spec: &instance_spec::v0::StorageBackendV0, + backend_spec: &StorageBackend, backend_name: &str, nexus_client: &Option, ) -> Result { match backend_spec { - instance_spec::v0::StorageBackendV0::Crucible(spec) => { + StorageBackend::Crucible(spec) => { info!(self.log, "Creating Crucible disk"; "backend_name" => backend_name); @@ -433,7 +433,7 @@ impl<'a> MachineInitializer<'a> { let crucible = Some((be.get_uuid().await?, be.clone())); Ok(StorageBackendInstance { be, crucible }) } - instance_spec::v0::StorageBackendV0::File(spec) => { + StorageBackend::File(spec) => { info!(self.log, "Creating file disk backend"; "path" => &spec.path); @@ -459,7 +459,7 @@ impl<'a> MachineInitializer<'a> { Ok(StorageBackendInstance { be, crucible: None }) } - instance_spec::v0::StorageBackendV0::Blob(spec) => { + StorageBackend::Blob(spec) => { let bytes = base64::Engine::decode( &base64::engine::general_purpose::STANDARD, &spec.base64, @@ -520,10 +520,10 @@ impl<'a> MachineInitializer<'a> { let (device_interface, backend_name, pci_path) = match &disk .device_spec { - instance_spec::v0::StorageDeviceV0::VirtioDisk(disk) => { + spec::StorageDevice::Virtio(disk) => { (DeviceInterface::Virtio, &disk.backend_name, disk.pci_path) } - instance_spec::v0::StorageDeviceV0::NvmeDisk(disk) => { + spec::StorageDevice::Nvme(disk) => { (DeviceInterface::Nvme, &disk.backend_name, disk.pci_path) } }; @@ -642,18 +642,16 @@ impl<'a> MachineInitializer<'a> { for (device_name, nic) in &self.spec.nics { info!(self.log, "Creating vNIC {}", device_name); - let instance_spec::v0::NetworkDeviceV0::VirtioNic(vnic_spec) = - &nic.device_spec; - - let bdf: pci::Bdf = vnic_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.try_into().map_err(|e| { + Error::new( + ErrorKind::InvalidInput, + format!( + "Couldn't get PCI BDF for vNIC {}: {}", + device_name, e + ), + ) + })?; let viona = virtio::PciVirtioViona::new( &nic.backend_spec.vnic_name, @@ -665,7 +663,7 @@ impl<'a> MachineInitializer<'a> { // Only push to interface_ids if kstat_sampler exists if let Some(ref mut ids) = interface_ids { - ids.push((vnic_spec.interface_id, viona.instance_id()?)); + ids.push((nic.device_spec.interface_id, viona.instance_id()?)); } chipset.pci_attach(bdf, viona); diff --git a/bin/propolis-server/src/lib/spec/api_request.rs b/bin/propolis-server/src/lib/spec/api_request.rs index 393f0e654..b778a485f 100644 --- a/bin/propolis-server/src/lib/spec/api_request.rs +++ b/bin/propolis-server/src/lib/spec/api_request.rs @@ -15,14 +15,16 @@ use propolis_api_types::{ }, devices::{NvmeDisk, VirtioDisk, VirtioNic}, }, - v0::{NetworkDeviceV0, StorageBackendV0, StorageDeviceV0}, PciPath, }, DiskRequest, NetworkInterfaceRequest, Slot, }; use thiserror::Error; -use super::{Disk, Nic, ParsedDiskRequest, ParsedNicRequest}; +use super::{ + Disk, Nic, ParsedDiskRequest, ParsedNicRequest, StorageBackend, + StorageDevice, +}; #[derive(Debug, Error)] pub(crate) enum DeviceRequestError { @@ -72,11 +74,11 @@ pub(super) fn parse_disk_from_request( let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?; let backend_name = format!("{}-backend", disk.name); let device_spec = match disk.device.as_ref() { - "virtio" => StorageDeviceV0::VirtioDisk(VirtioDisk { + "virtio" => StorageDevice::Virtio(VirtioDisk { backend_name: backend_name.clone(), pci_path, }), - "nvme" => StorageDeviceV0::NvmeDisk(NvmeDisk { + "nvme" => StorageDevice::Nvme(NvmeDisk { backend_name: backend_name.clone(), pci_path, }), @@ -89,7 +91,7 @@ pub(super) fn parse_disk_from_request( }; let device_name = disk.name.clone(); - let backend_spec = StorageBackendV0::Crucible(CrucibleStorageBackend { + let backend_spec = StorageBackend::Crucible(CrucibleStorageBackend { request_json: serde_json::to_string(&disk.volume_construction_request) .map_err(|e| { DeviceRequestError::SerializationError(disk.name.clone(), e) @@ -110,9 +112,9 @@ pub(super) fn parse_cloud_init_from_request( let pci_path = slot_to_pci_path(Slot(0), SlotType::CloudInit)?; let backend_name = name.to_string(); let backend_spec = - StorageBackendV0::Blob(BlobStorageBackend { base64, readonly: true }); + StorageBackend::Blob(BlobStorageBackend { base64, readonly: true }); - let device_spec = StorageDeviceV0::VirtioDisk(VirtioDisk { + let device_spec = StorageDevice::Virtio(VirtioDisk { backend_name: name.to_string(), pci_path, }); @@ -128,11 +130,11 @@ pub(super) fn parse_nic_from_request( ) -> Result { let pci_path = slot_to_pci_path(nic.slot, SlotType::Nic)?; let (device_name, backend_name) = super::pci_path_to_nic_names(pci_path); - let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic { + let device_spec = VirtioNic { backend_name: backend_name.clone(), interface_id: nic.interface_id, pci_path, - }); + }; let backend_spec = VirtioNetworkBackend { vnic_name: nic.name.to_string() }; Ok(ParsedNicRequest { @@ -148,13 +150,13 @@ mod test { use super::*; - fn check_parsed_storage_device_backend_pointer(parsed: &Disk) { - let device_to_backend = match &parsed.device_spec { - StorageDeviceV0::VirtioDisk(d) => d.backend_name.clone(), - StorageDeviceV0::NvmeDisk(d) => d.backend_name.clone(), + fn check_parsed_storage_device_backend_pointer(parsed: &ParsedDiskRequest) { + let device_to_backend = match &parsed.disk.device_spec { + StorageDevice::Virtio(d) => d.backend_name.clone(), + StorageDevice::Nvme(d) => d.backend_name.clone(), }; - assert_eq!(device_to_backend, parsed.backend_name); + assert_eq!(device_to_backend, parsed.disk.backend_name); } #[test] @@ -186,8 +188,8 @@ mod test { }; let parsed = parse_nic_from_request(&req).unwrap(); - let NetworkDeviceV0::VirtioNic(nic) = &parsed.device_spec; - assert_eq!(nic.backend_name, parsed.backend_name); + let VirtioNic { backend_name, .. } = &parsed.nic.device_spec; + assert_eq!(*backend_name, parsed.nic.backend_name); } #[test] diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index d6a6aa2dc..41a9be32c 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -50,21 +50,23 @@ impl From for InstanceSpecV0 { let _old = spec .devices .storage_devices - .insert(disk_name, disk.device_spec); + .insert(disk_name, disk.device_spec.into()); assert!(_old.is_none()); let _old = spec .backends .storage_backends - .insert(disk.backend_name, disk.backend_spec); + .insert(disk.backend_name, disk.backend_spec.into()); assert!(_old.is_none()); } for (nic_name, nic) in val.nics { - let _old = - spec.devices.network_devices.insert(nic_name, nic.device_spec); + let _old = spec + .devices + .network_devices + .insert(nic_name, NetworkDeviceV0::VirtioNic(nic.device_spec)); assert!(_old.is_none()); @@ -154,7 +156,11 @@ impl TryFrom for Spec { builder.add_storage_device( device_name, - Disk { device_spec, backend_name, backend_spec }, + Disk { + device_spec: device_spec.into(), + backend_name, + backend_spec: backend_spec.into(), + }, )?; } @@ -163,10 +169,8 @@ impl TryFrom for Spec { } for (device_name, device_spec) in value.devices.network_devices { - let backend_name = match &device_spec { - NetworkDeviceV0::VirtioNic(nic) => &nic.backend_name, - }; - + let NetworkDeviceV0::VirtioNic(device_spec) = device_spec; + let backend_name = &device_spec.backend_name; let (backend_name, backend_spec) = value .backends .network_backends diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index 7af844f24..4b80989ae 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -12,7 +12,6 @@ use propolis_api_types::{ board::{Board, Chipset, I440Fx}, devices::{PciPciBridge, SerialPortNumber}, }, - v0::{NetworkDeviceV0, StorageDeviceV0}, PciPath, }, DiskRequest, InstanceProperties, NetworkInterfaceRequest, @@ -64,27 +63,6 @@ pub(crate) struct SpecBuilder { component_names: BTreeSet, } -trait PciComponent { - fn pci_path(&self) -> PciPath; -} - -impl PciComponent for StorageDeviceV0 { - fn pci_path(&self) -> PciPath { - match self { - StorageDeviceV0::VirtioDisk(disk) => disk.pci_path, - StorageDeviceV0::NvmeDisk(disk) => disk.pci_path, - } - } -} - -impl PciComponent for NetworkDeviceV0 { - fn pci_path(&self) -> PciPath { - match self { - NetworkDeviceV0::VirtioNic(nic) => nic.pci_path, - } - } -} - impl SpecBuilder { pub fn new(properties: &InstanceProperties) -> Self { let board = Board { @@ -244,7 +222,7 @@ impl SpecBuilder { return Err(SpecBuilderError::ComponentNameInUse(nic.backend_name)); } - self.register_pci_device(nic.device_spec.pci_path())?; + self.register_pci_device(nic.device_spec.pci_path)?; self.component_names.insert(nic_name.clone()); self.component_names.insert(nic.backend_name.clone()); let _old = self.spec.nics.insert(nic_name, nic); diff --git a/bin/propolis-server/src/lib/spec/config_toml.rs b/bin/propolis-server/src/lib/spec/config_toml.rs index 2f059a8dd..2de57ad8f 100644 --- a/bin/propolis-server/src/lib/spec/config_toml.rs +++ b/bin/propolis-server/src/lib/spec/config_toml.rs @@ -11,7 +11,6 @@ use propolis_api_types::instance_spec::{ backends::{FileStorageBackend, VirtioNetworkBackend}, devices::{NvmeDisk, PciPciBridge, VirtioDisk, VirtioNic}, }, - v0::{NetworkDeviceV0, StorageBackendV0, StorageDeviceV0}, PciPath, }; use thiserror::Error; @@ -25,6 +24,7 @@ use crate::config; use super::{ Disk, Nic, ParsedDiskRequest, ParsedNicRequest, ParsedPciBridgeRequest, + StorageBackend, StorageDevice, }; #[cfg(feature = "falcon")] @@ -114,12 +114,10 @@ impl TryFrom<&config::Config> for ParsedConfig { parse_storage_device_from_config(device_name, device)?; let backend_name = match &device_spec { - StorageDeviceV0::VirtioDisk(disk) => { - disk.backend_name.clone() - } - StorageDeviceV0::NvmeDisk(disk) => { + StorageDevice::Virtio(disk) => { disk.backend_name.clone() } + StorageDevice::Nvme(disk) => disk.backend_name.clone(), }; let backend_config = @@ -191,9 +189,9 @@ impl TryFrom<&config::Config> for ParsedConfig { pub(super) fn parse_storage_backend_from_config( name: &str, backend: &config::BlockDevice, -) -> Result { +) -> Result { let backend_spec = match backend.bdtype.as_str() { - "file" => StorageBackendV0::File(FileStorageBackend { + "file" => StorageBackend::File(FileStorageBackend { path: backend .options .get("path") @@ -233,7 +231,7 @@ pub(super) fn parse_storage_backend_from_config( pub(super) fn parse_storage_device_from_config( name: &str, device: &config::Device, -) -> Result { +) -> Result { enum Interface { Virtio, Nvme, @@ -268,10 +266,10 @@ pub(super) fn parse_storage_device_from_config( Ok(match interface { Interface::Virtio => { - StorageDeviceV0::VirtioDisk(VirtioDisk { backend_name, pci_path }) + StorageDevice::Virtio(VirtioDisk { backend_name, pci_path }) } Interface::Nvme => { - StorageDeviceV0::NvmeDisk(NvmeDisk { backend_name, pci_path }) + StorageDevice::Nvme(NvmeDisk { backend_name, pci_path }) } }) } @@ -290,14 +288,13 @@ pub(super) fn parse_network_device_from_config( let (device_name, backend_name) = super::pci_path_to_nic_names(pci_path); let backend_spec = VirtioNetworkBackend { vnic_name: vnic_name.to_owned() }; - - let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic { + let device_spec = VirtioNic { backend_name: backend_name.clone(), - // We don't allow for configuration to specify the interface_id, so we - // generate a new one. + // NICs added by the configuration TOML have no control plane- + // supplied correlation IDs. interface_id: uuid::Uuid::nil(), pci_path, - }); + }; Ok(ParsedNicRequest { name: device_name, diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 82669a9be..3181f2708 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -17,13 +17,17 @@ use std::collections::HashMap; use propolis_api_types::instance_spec::{ components::{ - backends::VirtioNetworkBackend, + backends::{ + BlobStorageBackend, CrucibleStorageBackend, FileStorageBackend, + VirtioNetworkBackend, + }, board::Board, devices::{ - PciPciBridge, QemuPvpanic as QemuPvpanicDesc, SerialPortNumber, + NvmeDisk, PciPciBridge, QemuPvpanic as QemuPvpanicDesc, + SerialPortNumber, VirtioDisk, VirtioNic, }, }, - v0::*, + v0::{StorageBackendV0, StorageDeviceV0}, PciPath, }; @@ -55,16 +59,76 @@ pub(crate) struct Spec { /// Describes a storage device/backend pair parsed from an input source like an /// API request or a config TOML entry. +#[derive(Clone, Debug)] +pub enum StorageDevice { + Virtio(VirtioDisk), + Nvme(NvmeDisk), +} + +impl StorageDevice { + pub fn pci_path(&self) -> PciPath { + match self { + StorageDevice::Virtio(disk) => disk.pci_path, + StorageDevice::Nvme(disk) => disk.pci_path, + } + } +} + +impl From for StorageDeviceV0 { + fn from(value: StorageDevice) -> Self { + match value { + StorageDevice::Virtio(d) => Self::VirtioDisk(d), + StorageDevice::Nvme(d) => Self::NvmeDisk(d), + } + } +} + +impl From for StorageDevice { + fn from(value: StorageDeviceV0) -> Self { + match value { + StorageDeviceV0::VirtioDisk(d) => Self::Virtio(d), + StorageDeviceV0::NvmeDisk(d) => Self::Nvme(d), + } + } +} + +#[derive(Clone, Debug)] +pub enum StorageBackend { + Crucible(CrucibleStorageBackend), + File(FileStorageBackend), + Blob(BlobStorageBackend), +} + +impl From for StorageBackendV0 { + fn from(value: StorageBackend) -> Self { + match value { + StorageBackend::Crucible(be) => Self::Crucible(be), + StorageBackend::File(be) => Self::File(be), + StorageBackend::Blob(be) => Self::Blob(be), + } + } +} + +impl From for StorageBackend { + fn from(value: StorageBackendV0) -> Self { + match value { + StorageBackendV0::Crucible(be) => Self::Crucible(be), + StorageBackendV0::File(be) => Self::File(be), + StorageBackendV0::Blob(be) => Self::Blob(be), + } + } +} + #[derive(Clone, Debug)] pub struct Disk { - pub device_spec: StorageDeviceV0, + pub device_spec: StorageDevice, pub backend_name: String, - pub backend_spec: StorageBackendV0, + pub backend_spec: StorageBackend, } #[derive(Clone, Debug)] pub struct Nic { - pub device_spec: NetworkDeviceV0, + pub device_spec: VirtioNic, pub backend_name: String, pub backend_spec: VirtioNetworkBackend, } diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index 70bae5a1a..a0298b4e4 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -11,10 +11,8 @@ use std::{ use anyhow::Context; use propolis_api_types::{ - instance_spec::{ - components::backends::CrucibleStorageBackend, v0::StorageBackendV0, - }, - InstanceState, MigrationState, + instance_spec::components::backends::CrucibleStorageBackend, InstanceState, + MigrationState, }; use slog::{error, info}; use tokio::sync::Notify; @@ -24,6 +22,7 @@ use crate::{ migrate::{ destination::DestinationProtocol, source::SourceProtocol, MigrateRole, }, + spec::StorageBackend, vm::state_publisher::ExternalStateUpdate, }; @@ -675,7 +674,7 @@ impl StateDriver { return Err(spec_element_not_found(&disk_name)); }; - let StorageBackendV0::Crucible(CrucibleStorageBackend { + let StorageBackend::Crucible(CrucibleStorageBackend { request_json: old_vcr_json, readonly, }) = &disk.backend_spec @@ -693,11 +692,10 @@ impl StateDriver { ) })?; - disk.backend_spec = - StorageBackendV0::Crucible(CrucibleStorageBackend { - readonly: *readonly, - request_json: new_vcr_json, - }); + disk.backend_spec = StorageBackend::Crucible(CrucibleStorageBackend { + readonly: *readonly, + request_json: new_vcr_json, + }); info!(self.log, "replaced Crucible VCR"; "backend_id" => %backend_id); From 7a5b955507a59ba865e6f54e86d066f037f01041 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 3 Sep 2024 20:53:15 +0000 Subject: [PATCH 05/11] cleanup --- .../src/lib/spec/api_spec_v0.rs | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 41a9be32c..9733ee048 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -27,8 +27,8 @@ pub(crate) enum ApiSpecParseError { #[error(transparent)] BuilderError(#[from] SpecBuilderError), - #[error("backend {0} not found for device {1}")] - BackendNotFound(String, String), + #[error("backend {backend} not found for device {device}")] + BackendNotFound { backend: String, device: String }, #[error("backend {0} not used by any device")] BackendNotUsed(String), @@ -137,6 +137,9 @@ impl TryFrom for Spec { fn try_from(mut value: InstanceSpecV0) -> Result { let mut builder = SpecBuilder::with_board(value.devices.board); + + // Examine each storage device and peel its backend off of the input + // spec. for (device_name, device_spec) in value.devices.storage_devices { let backend_name = match &device_spec { StorageDeviceV0::VirtioDisk(disk) => &disk.backend_name, @@ -147,11 +150,9 @@ impl TryFrom for Spec { .backends .storage_backends .remove_entry(backend_name) - .ok_or_else(|| { - ApiSpecParseError::BackendNotFound( - backend_name.to_owned(), - device_name.clone(), - ) + .ok_or_else(|| ApiSpecParseError::BackendNotFound { + backend: backend_name.to_owned(), + device: device_name.clone(), })?; builder.add_storage_device( @@ -164,10 +165,13 @@ impl TryFrom for Spec { )?; } + // Once all the devices have been checked, there should be no unpaired + // backends remaining. if let Some(backend) = value.backends.storage_backends.keys().next() { return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned())); } + // Repeat this process for network devices. for (device_name, device_spec) in value.devices.network_devices { let NetworkDeviceV0::VirtioNic(device_spec) = device_spec; let backend_name = &device_spec.backend_name; @@ -175,11 +179,9 @@ impl TryFrom for Spec { .backends .network_backends .remove_entry(backend_name) - .ok_or_else(|| { - ApiSpecParseError::BackendNotFound( - backend_name.to_owned(), - device_name.clone(), - ) + .ok_or_else(|| ApiSpecParseError::BackendNotFound { + backend: backend_name.to_owned(), + device: device_name.clone(), })?; let NetworkBackendV0::Virtio(backend_spec) = backend_spec else { @@ -216,11 +218,9 @@ impl TryFrom for Spec { .backends .network_backends .remove_entry(&port.backend_name) - .ok_or_else(|| { - ApiSpecParseError::BackendNotFound( - port.backend_name, - port_name.clone(), - ) + .ok_or_else(|| ApiSpecParseError::BackendNotFound { + backend: port.backend_name, + device: port_name.clone(), })?; let NetworkBackendV0::Dlpi(backend_spec) = backend_spec else { @@ -238,7 +238,7 @@ impl TryFrom for Spec { return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned())); } - // TODO(gjc) do more to preserve names + // TODO(#735): Serial ports need to have names like other devices. for serial_port in value.devices.serial_ports.values() { builder.add_serial_port(serial_port.num)?; } From 87324c201e286e4dafcee1e49504f37bc10ef195 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 3 Sep 2024 22:57:41 +0000 Subject: [PATCH 06/11] improve doc comments --- bin/propolis-server/src/lib/spec/mod.rs | 30 ++++++++++++++++--------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 3181f2708..8147b7ed0 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -4,14 +4,15 @@ //! Instance specs describe how to configure a VM and what components it has. //! -//! This module defines an "internal" spec type for the server's instance -//! initialization code to use. This spec type is not `Serialize` and is not -//! meant to be sent over the wire in API requests or the migration protocol. -//! This allows the internal representation to change freely between versions -//! (so long as it can consistently be converted to and from the wire-format -//! spec in the [`propolis_api_types`] crate); this, in turn, allows this -//! representation to take forms that might otherwise be hard to change in a -//! backward-compatible way. +//! This module defines a crate-internal instance spec type, [`Spec`], and its +//! constituent types, like [`Disk`] and [`Nic`]. Unlike the types in +//! [`propolis_api_types::instance_spec`], these internal types are not +//! `Serialize` and are never meant to be used over the wire in API requests or +//! the migration protocol. This allows them to change freely between Propolis +//! versions, so long as they can consistently be converted to and from the +//! wire-format types in the [`propolis_api_types`] crate. This, in turn, allows +//! [`Spec`] and its component types to take forms that might otherwise be hard +//! to change in a backward-compatible way. use std::collections::HashMap; @@ -42,6 +43,15 @@ pub(crate) mod api_spec_v0; pub(crate) mod builder; mod config_toml; +/// An instance specification that describes a VM's configuration and +/// components. +/// +/// NOTE: This struct's fields are `pub` to make it convenient to access the +/// individual parts of a fully-constructed spec. Modules that consume specs may +/// assert that they are valid (no duplicate component names, no duplicate PCI +/// device paths, etc.). When constructing a new spec, use the +/// [`builder::SpecBuilder`] struct to catch requests that violate these +/// invariants. #[derive(Clone, Debug, Default)] pub(crate) struct Spec { pub board: Board, @@ -57,8 +67,7 @@ pub(crate) struct Spec { pub softnpu: SoftNpu, } -/// Describes a storage device/backend pair parsed from an input source like an -/// API request or a config TOML entry. +/// Describes the device half of a [`Disk`]. #[derive(Clone, Debug)] pub enum StorageDevice { Virtio(VirtioDisk), @@ -92,6 +101,7 @@ impl From for StorageDevice { } } +/// Describes the backend half of a [`Disk`]. #[derive(Clone, Debug)] pub enum StorageBackend { Crucible(CrucibleStorageBackend), From d72cada9d83fcc1c8a07faf0d5d3c3cd5620933e Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 4 Sep 2024 22:32:05 +0000 Subject: [PATCH 07/11] include cleanup --- bin/propolis-server/src/lib/initializer.rs | 8 ++--- bin/propolis-server/src/lib/server.rs | 41 ++++++++++++---------- bin/propolis-server/src/lib/stats/mod.rs | 4 +-- bin/propolis-server/src/lib/vm/ensure.rs | 8 ++--- bin/propolis-server/src/lib/vm/mod.rs | 4 +-- bin/propolis-server/src/lib/vm/objects.rs | 10 +++--- 6 files changed, 39 insertions(+), 36 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 2088dce20..9d76e0a1f 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -11,7 +11,7 @@ use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use crate::serial::Serial; -use crate::spec::{self, StorageBackend}; +use crate::spec::{self, Spec, StorageBackend}; use crate::stats::{ track_network_interface_kstats, track_vcpu_kstats, VirtualDiskProducer, VirtualMachine, @@ -47,7 +47,7 @@ use slog::info; /// Arbitrary ROM limit for now const MAX_ROM_SIZE: usize = 0x20_0000; -fn get_spec_guest_ram_limits(spec: &spec::Spec) -> (usize, usize) { +fn get_spec_guest_ram_limits(spec: &Spec) -> (usize, usize) { let memsize = spec.board.memory_mb as usize * MB; let lowmem = memsize.min(3 * GB); let highmem = memsize.saturating_sub(3 * GB); @@ -56,7 +56,7 @@ fn get_spec_guest_ram_limits(spec: &spec::Spec) -> (usize, usize) { pub fn build_instance( name: &str, - spec: &spec::Spec, + spec: &Spec, use_reservoir: bool, _log: slog::Logger, ) -> Result { @@ -120,7 +120,7 @@ pub struct MachineInitializer<'a> { pub(crate) devices: DeviceMap, pub(crate) block_backends: BlockBackendMap, pub(crate) crucible_backends: CrucibleBackendMap, - pub(crate) spec: &'a spec::Spec, + pub(crate) spec: &'a Spec, pub(crate) properties: &'a InstanceProperties, pub(crate) toml_config: &'a crate::server::VmTomlConfig, pub(crate) producer_registry: Option, diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index e2022b266..c98fb3aa9 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -16,11 +16,18 @@ use std::net::SocketAddr; use std::net::SocketAddrV6; use std::sync::Arc; -use crate::serial::history_buffer::SerialHistoryOffset; -use crate::spec; -use crate::spec::api_spec_v0::ApiSpecParseError; -use crate::vm::ensure::VmEnsureRequest; -use crate::vm::VmError; +use crate::{ + serial::history_buffer::SerialHistoryOffset, + spec::{ + self, + api_spec_v0::ApiSpecParseError, + builder::{SpecBuilder, SpecBuilderError}, + Spec, + }, + vm::{ensure::VmEnsureRequest, VmError}, + vnc::{self, VncServer}, +}; + use dropshot::{ channel, endpoint, ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk, HttpResponseUpdatedNoContent, Path, Query, RequestContext, @@ -32,20 +39,17 @@ use internal_dns::ServiceName; pub use nexus_client::Client as NexusClient; use oximeter::types::ProducerRegistry; use propolis_api_types as api; -use propolis_api_types::instance_spec::VersionedInstanceSpec; use propolis_api_types::instance_spec::{ - self, components::devices::QemuPvpanic, + self, components::devices::QemuPvpanic, VersionedInstanceSpec, }; - pub use propolis_server_config::Config as VmTomlConfig; use rfb::tungstenite::BinaryWs; use slog::{error, warn, Logger}; use tokio::sync::MutexGuard; -use tokio_tungstenite::tungstenite::protocol::{Role, WebSocketConfig}; -use tokio_tungstenite::WebSocketStream; - -use crate::spec::builder::{SpecBuilder, SpecBuilderError}; -use crate::vnc::{self, VncServer}; +use tokio_tungstenite::{ + tungstenite::protocol::{Role, WebSocketConfig}, + WebSocketStream, +}; /// Configuration used to set this server up to provide Oximeter metrics. #[derive(Debug, Clone)] @@ -112,7 +116,7 @@ impl DropshotEndpointContext { fn instance_spec_from_request( request: &api::InstanceEnsureRequest, toml_config: &VmTomlConfig, -) -> Result { +) -> Result { let mut spec_builder = SpecBuilder::new(&request.properties); spec_builder.add_devices_from_config(toml_config)?; @@ -231,7 +235,7 @@ async fn instance_ensure_common( rqctx: RequestContext>, properties: api::InstanceProperties, migrate: Option, - instance_spec: spec::Spec, + instance_spec: Spec, ) -> Result, HttpError> { let server_context = rqctx.context(); let oximeter_registry = server_context @@ -321,10 +325,9 @@ async fn instance_spec_ensure( ) -> Result, HttpError> { let request = request.into_inner(); let VersionedInstanceSpec::V0(v0_spec) = request.instance_spec; - let spec = - spec::Spec::try_from(v0_spec).map_err(|e: ApiSpecParseError| { - HttpError::for_bad_request(None, e.to_string()) - })?; + let spec = Spec::try_from(v0_spec).map_err(|e: ApiSpecParseError| { + HttpError::for_bad_request(None, e.to_string()) + })?; instance_ensure_common(rqctx, request.properties, request.migrate, spec) .await diff --git a/bin/propolis-server/src/lib/stats/mod.rs b/bin/propolis-server/src/lib/stats/mod.rs index 4fd55ac12..0a23d3e84 100644 --- a/bin/propolis-server/src/lib/stats/mod.rs +++ b/bin/propolis-server/src/lib/stats/mod.rs @@ -18,7 +18,7 @@ use propolis_api_types::InstanceProperties; use slog::Logger; use uuid::Uuid; -use crate::spec; +use crate::spec::Spec; use crate::{server::MetricsEndpointConfig, vm::NetworkInterfaceIds}; mod network_interface; @@ -189,7 +189,7 @@ pub fn start_oximeter_server( pub(crate) fn create_kstat_sampler( log: &Logger, properties: &InstanceProperties, - spec: &spec::Spec, + spec: &Spec, ) -> Option { let kstat_limit = usize::try_from( (u32::from(properties.vcpus) * KSTAT_LIMIT_PER_VCPU) diff --git a/bin/propolis-server/src/lib/vm/ensure.rs b/bin/propolis-server/src/lib/vm/ensure.rs index 84b1fd26d..3aff7d3bf 100644 --- a/bin/propolis-server/src/lib/vm/ensure.rs +++ b/bin/propolis-server/src/lib/vm/ensure.rs @@ -39,7 +39,7 @@ use crate::{ initializer::{ build_instance, MachineInitializer, MachineInitializerState, }, - spec, + spec::Spec, stats::create_kstat_sampler, vm::request_queue::InstanceAutoStart, }; @@ -55,7 +55,7 @@ use super::{ pub(crate) struct VmEnsureRequest { pub(crate) properties: InstanceProperties, pub(crate) migrate: Option, - pub(crate) instance_spec: spec::Spec, + pub(crate) instance_spec: Spec, } /// Holds state about an instance ensure request that has not yet produced any @@ -88,7 +88,7 @@ impl<'a> VmEnsureNotStarted<'a> { } } - pub(crate) fn instance_spec(&self) -> &spec::Spec { + pub(crate) fn instance_spec(&self) -> &Spec { &self.ensure_request.instance_spec } @@ -379,7 +379,7 @@ impl<'a> VmEnsureActive<'a> { fn initialize_kstat_sampler( log: &slog::Logger, properties: &InstanceProperties, - spec: &spec::Spec, + spec: &Spec, producer_registry: Option, ) -> Option { let registry = producer_registry?; diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index ae1f2212c..87364fa36 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -95,7 +95,7 @@ use state_driver::StateDriverOutput; use state_publisher::StatePublisher; use tokio::sync::{oneshot, watch, RwLock, RwLockReadGuard}; -use crate::{server::MetricsEndpointConfig, spec, vnc::VncServer}; +use crate::{server::MetricsEndpointConfig, spec::Spec, vnc::VncServer}; mod active; pub(crate) mod ensure; @@ -219,7 +219,7 @@ struct VmDescription { properties: InstanceProperties, /// The VM's last-known instance specification. - spec: spec::Spec, + spec: Spec, /// The runtime on which the VM's state driver is running (or on which it /// ran). diff --git a/bin/propolis-server/src/lib/vm/objects.rs b/bin/propolis-server/src/lib/vm/objects.rs index 45f450de7..a79cee1d7 100644 --- a/bin/propolis-server/src/lib/vm/objects.rs +++ b/bin/propolis-server/src/lib/vm/objects.rs @@ -20,7 +20,7 @@ use propolis::{ use slog::{error, info}; use tokio::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; -use crate::{serial::Serial, spec, vcpu_tasks::VcpuTaskController}; +use crate::{serial::Serial, spec::Spec, vcpu_tasks::VcpuTaskController}; use super::{ state_driver::VmStartReason, BlockBackendMap, CrucibleBackendMap, DeviceMap, @@ -43,7 +43,7 @@ pub(crate) struct VmObjects { /// A collection of objects that should eventually be wrapped in a lock and /// stored in a `VmObjects` structure. See [`VmObjectsLocked`]. pub(super) struct InputVmObjects { - pub instance_spec: spec::Spec, + pub instance_spec: Spec, pub vcpu_tasks: Box, pub machine: Machine, pub devices: DeviceMap, @@ -60,7 +60,7 @@ pub(crate) struct VmObjectsLocked { log: slog::Logger, /// The instance spec that describes this collection of objects. - instance_spec: spec::Spec, + instance_spec: Spec, /// The set of tasks that run this VM's vCPUs. vcpu_tasks: Box, @@ -131,12 +131,12 @@ impl VmObjectsLocked { } /// Yields the VM's current instance spec. - pub(crate) fn instance_spec(&self) -> &spec::Spec { + pub(crate) fn instance_spec(&self) -> &Spec { &self.instance_spec } /// Yields a mutable reference to the VM's current instance spec. - pub(crate) fn instance_spec_mut(&mut self) -> &mut spec::Spec { + pub(crate) fn instance_spec_mut(&mut self) -> &mut Spec { &mut self.instance_spec } From 45af51e7ace63f0c160af546a5789da4f7dbea85 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 11 Sep 2024 16:09:08 +0000 Subject: [PATCH 08/11] clean up some type/variant names --- bin/propolis-server/src/lib/initializer.rs | 17 +++++++----- bin/propolis-server/src/lib/server.rs | 4 +-- .../src/lib/spec/api_spec_v0.rs | 26 +++++++++---------- bin/propolis-server/src/lib/spec/builder.rs | 6 ++--- bin/propolis-server/src/lib/spec/mod.rs | 7 ++--- 5 files changed, 31 insertions(+), 29 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 9d76e0a1f..941c8cd7d 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -298,8 +298,11 @@ impl<'a> MachineInitializer<'a> { chipset: &RegisteredChipset, ) -> Result, Error> { let mut com1 = None; + + // Create UART devices for all of the serial ports in the spec that + // requested one. for (num, user) in self.spec.serial.iter() { - if *user != spec::SerialPortUser::Standard { + if *user != spec::SerialPortDevice::Uart { continue; } @@ -509,12 +512,12 @@ impl<'a> MachineInitializer<'a> { Nvme, } - 'devloop: for (disk_name, disk) in &self.spec.disks { + for (disk_name, disk) in &self.spec.disks { info!( self.log, - "Creating storage device {} with properties {:?}", - disk_name, - disk.device_spec + "Creating storage device"; + "name" => disk_name, + "spec" => ?disk.device_spec ); let (device_interface, backend_name, pci_path) = match &disk @@ -591,7 +594,7 @@ impl<'a> MachineInitializer<'a> { virtual disk metrics can't be reported for it"; "disk_id" => %disk_id, ); - continue 'devloop; + continue; }; // Register the block device as a metric producer, if we've been @@ -612,7 +615,7 @@ impl<'a> MachineInitializer<'a> { "disk_id" => %disk_id, "error" => ?e, ); - continue 'devloop; + continue; }; // Set the on-completion callback for the block device, to diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index c98fb3aa9..2440e18a3 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -20,7 +20,7 @@ use crate::{ serial::history_buffer::SerialHistoryOffset, spec::{ self, - api_spec_v0::ApiSpecParseError, + api_spec_v0::ApiSpecError, builder::{SpecBuilder, SpecBuilderError}, Spec, }, @@ -325,7 +325,7 @@ async fn instance_spec_ensure( ) -> Result, HttpError> { let request = request.into_inner(); let VersionedInstanceSpec::V0(v0_spec) = request.instance_spec; - let spec = Spec::try_from(v0_spec).map_err(|e: ApiSpecParseError| { + let spec = Spec::try_from(v0_spec).map_err(|e: ApiSpecError| { HttpError::for_bad_request(None, e.to_string()) })?; diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 9733ee048..60d849092 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -19,13 +19,13 @@ use crate::spec::SoftNpuPort; use super::{ builder::{SpecBuilder, SpecBuilderError}, - Disk, Nic, QemuPvpanic, SerialPortUser, Spec, + Disk, Nic, QemuPvpanic, SerialPortDevice, Spec, }; #[derive(Debug, Error)] -pub(crate) enum ApiSpecParseError { +pub(crate) enum ApiSpecError { #[error(transparent)] - BuilderError(#[from] SpecBuilderError), + Builder(#[from] SpecBuilderError), #[error("backend {backend} not found for device {device}")] BackendNotFound { backend: String, device: String }, @@ -79,7 +79,7 @@ impl From for InstanceSpecV0 { } for (num, user) in val.serial.iter() { - if *user == SerialPortUser::Standard { + if *user == SerialPortDevice::Uart { let name = match num { SerialPortNumber::Com1 => "com1", SerialPortNumber::Com2 => "com2", @@ -133,7 +133,7 @@ impl From for InstanceSpecV0 { } impl TryFrom for Spec { - type Error = ApiSpecParseError; + type Error = ApiSpecError; fn try_from(mut value: InstanceSpecV0) -> Result { let mut builder = SpecBuilder::with_board(value.devices.board); @@ -150,7 +150,7 @@ impl TryFrom for Spec { .backends .storage_backends .remove_entry(backend_name) - .ok_or_else(|| ApiSpecParseError::BackendNotFound { + .ok_or_else(|| ApiSpecError::BackendNotFound { backend: backend_name.to_owned(), device: device_name.clone(), })?; @@ -168,7 +168,7 @@ impl TryFrom for Spec { // Once all the devices have been checked, there should be no unpaired // backends remaining. if let Some(backend) = value.backends.storage_backends.keys().next() { - return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned())); + return Err(ApiSpecError::BackendNotUsed(backend.to_owned())); } // Repeat this process for network devices. @@ -179,15 +179,13 @@ impl TryFrom for Spec { .backends .network_backends .remove_entry(backend_name) - .ok_or_else(|| ApiSpecParseError::BackendNotFound { + .ok_or_else(|| ApiSpecError::BackendNotFound { backend: backend_name.to_owned(), device: device_name.clone(), })?; let NetworkBackendV0::Virtio(backend_spec) = backend_spec else { - return Err(ApiSpecParseError::GuestNicInvalidBackend( - device_name, - )); + return Err(ApiSpecError::GuestNicInvalidBackend(device_name)); }; builder.add_network_device( @@ -218,13 +216,13 @@ impl TryFrom for Spec { .backends .network_backends .remove_entry(&port.backend_name) - .ok_or_else(|| ApiSpecParseError::BackendNotFound { + .ok_or_else(|| ApiSpecError::BackendNotFound { backend: port.backend_name, device: port_name.clone(), })?; let NetworkBackendV0::Dlpi(backend_spec) = backend_spec else { - return Err(ApiSpecParseError::NotDlpiBackend(port_name)); + return Err(ApiSpecError::NotDlpiBackend(port_name)); }; builder.add_softnpu_port( @@ -235,7 +233,7 @@ impl TryFrom for Spec { } if let Some(backend) = value.backends.network_backends.keys().next() { - return Err(ApiSpecParseError::BackendNotUsed(backend.to_owned())); + return Err(ApiSpecError::BackendNotUsed(backend.to_owned())); } // TODO(#735): Serial ports need to have names like other devices. diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index 4b80989ae..d13f25adc 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -23,7 +23,7 @@ use propolis_api_types::instance_spec::components::devices::{ P9fs, SoftNpuP9, SoftNpuPciPort, }; -use crate::{config, spec::SerialPortUser}; +use crate::{config, spec::SerialPortDevice}; use super::{ api_request::{self, DeviceRequestError}, @@ -251,7 +251,7 @@ impl SpecBuilder { &mut self, port: SerialPortNumber, ) -> Result<&Self, SpecBuilderError> { - if self.spec.serial.insert(port, SerialPortUser::Standard).is_some() { + if self.spec.serial.insert(port, SerialPortDevice::Uart).is_some() { Err(SpecBuilderError::SerialPortInUse(port)) } else { Ok(self) @@ -273,7 +273,7 @@ impl SpecBuilder { #[cfg(feature = "falcon")] pub fn set_softnpu_com4(&mut self) -> Result<&Self, SpecBuilderError> { let port = SerialPortNumber::Com4; - if self.spec.serial.insert(port, SerialPortUser::SoftNpu).is_some() { + if self.spec.serial.insert(port, SerialPortDevice::SoftNpu).is_some() { Err(SpecBuilderError::SerialPortInUse(port)) } else { Ok(self) diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 8147b7ed0..706bef589 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -58,7 +58,7 @@ pub(crate) struct Spec { pub disks: HashMap, pub nics: HashMap, - pub serial: HashMap, + pub serial: HashMap, pub pci_pci_bridges: HashMap, pub pvpanic: Option, @@ -143,9 +143,10 @@ pub struct Nic { pub backend_spec: VirtioNetworkBackend, } +/// A kind of device to install as the listener on a COM port. #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum SerialPortUser { - Standard, +pub enum SerialPortDevice { + Uart, #[cfg(feature = "falcon")] SoftNpu, From da7a83fdeb05277731e34c3c6dcb5709dc7537b9 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 11 Sep 2024 16:21:08 +0000 Subject: [PATCH 09/11] clean up asserts in spec conversion --- .../src/lib/spec/api_spec_v0.rs | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 60d849092..7c9431b40 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -44,38 +44,43 @@ pub(crate) enum ApiSpecError { impl From for InstanceSpecV0 { fn from(val: Spec) -> Self { let mut spec = InstanceSpecV0::default(); - spec.devices.board = val.board; + + // Internal instance specs (the inputs to this function) should assign + // a unique name to each component they describe. This invariant is + // enforced by the spec builder. Since component names are globally + // unique, they should never collide when inserted into an API spec's + // device and backend maps. for (disk_name, disk) in val.disks { - let _old = spec + let old = spec .devices .storage_devices .insert(disk_name, disk.device_spec.into()); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); - let _old = spec + let old = spec .backends .storage_backends .insert(disk.backend_name, disk.backend_spec.into()); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); } for (nic_name, nic) in val.nics { - let _old = spec + let old = spec .devices .network_devices .insert(nic_name, NetworkDeviceV0::VirtioNic(nic.device_spec)); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); - let _old = spec.backends.network_backends.insert( + let old = spec.backends.network_backends.insert( nic.backend_name, NetworkBackendV0::Virtio(nic.backend_spec), ); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); } for (num, user) in val.serial.iter() { @@ -87,18 +92,18 @@ impl From for InstanceSpecV0 { SerialPortNumber::Com4 => "com4", }; - let _old = spec + let old = spec .devices .serial_ports .insert(name.to_owned(), SerialPortDesc { num: *num }); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); } } for (bridge_name, bridge) in val.pci_pci_bridges { - let _old = spec.devices.pci_pci_bridges.insert(bridge_name, bridge); - assert!(_old.is_none()); + let old = spec.devices.pci_pci_bridges.insert(bridge_name, bridge); + assert!(old.is_none(), "{old:?}"); } spec.devices.qemu_pvpanic = val.pvpanic.map(|pvpanic| pvpanic.spec); @@ -109,7 +114,7 @@ impl From for InstanceSpecV0 { spec.devices.softnpu_p9 = val.softnpu.p9_device; spec.devices.p9fs = val.softnpu.p9fs; for (port_name, port) in val.softnpu.ports { - let _old = spec.devices.softnpu_ports.insert( + let old = spec.devices.softnpu_ports.insert( port_name.clone(), SoftNpuPortSpec { name: port_name, @@ -117,14 +122,14 @@ impl From for InstanceSpecV0 { }, ); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); - let _old = spec.backends.network_backends.insert( + let old = spec.backends.network_backends.insert( port.backend_name, NetworkBackendV0::Dlpi(port.backend_spec), ); - assert!(_old.is_none()); + assert!(old.is_none(), "{old:?}"); } } From 02be2a254b2171eda1498f4c3aa403a49fca25dd Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 11 Sep 2024 17:02:54 +0000 Subject: [PATCH 10/11] make spec conversion assertions more informative --- .../src/lib/spec/api_spec_v0.rs | 93 +++++++++++-------- 1 file changed, 52 insertions(+), 41 deletions(-) diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 7c9431b40..276d11239 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -5,6 +5,8 @@ //! Conversions from version-0 instance specs in the [`propolis_api_types`] //! crate to the internal [`super::Spec`] representation. +use std::collections::HashMap; + use propolis_api_types::instance_spec::{ components::devices::{SerialPort as SerialPortDesc, SerialPortNumber}, v0::{InstanceSpecV0, NetworkBackendV0, NetworkDeviceV0, StorageDeviceV0}, @@ -43,44 +45,53 @@ pub(crate) enum ApiSpecError { impl From for InstanceSpecV0 { fn from(val: Spec) -> Self { + // Inserts a component entry into the supplied map, asserting first that + // the supplied key is not present in that map. + // + // This assertion is valid because internal instance specs should assign + // a unique name to each component they describe. The spec builder + // upholds this invariant at spec creation time. + fn insert_component( + map: &mut HashMap, + key: String, + val: T, + ) { + assert!( + !map.contains_key(&key), + "component name {} already exists in output spec", + &key + ); + map.insert(key, val); + } + let mut spec = InstanceSpecV0::default(); spec.devices.board = val.board; - - // Internal instance specs (the inputs to this function) should assign - // a unique name to each component they describe. This invariant is - // enforced by the spec builder. Since component names are globally - // unique, they should never collide when inserted into an API spec's - // device and backend maps. for (disk_name, disk) in val.disks { - let old = spec - .devices - .storage_devices - .insert(disk_name, disk.device_spec.into()); - - assert!(old.is_none(), "{old:?}"); - - let old = spec - .backends - .storage_backends - .insert(disk.backend_name, disk.backend_spec.into()); + insert_component( + &mut spec.devices.storage_devices, + disk_name, + disk.device_spec.into(), + ); - assert!(old.is_none(), "{old:?}"); + insert_component( + &mut spec.backends.storage_backends, + disk.backend_name, + disk.backend_spec.into(), + ); } for (nic_name, nic) in val.nics { - let old = spec - .devices - .network_devices - .insert(nic_name, NetworkDeviceV0::VirtioNic(nic.device_spec)); - - assert!(old.is_none(), "{old:?}"); + insert_component( + &mut spec.devices.network_devices, + nic_name, + NetworkDeviceV0::VirtioNic(nic.device_spec), + ); - let old = spec.backends.network_backends.insert( + insert_component( + &mut spec.backends.network_backends, nic.backend_name, NetworkBackendV0::Virtio(nic.backend_spec), ); - - assert!(old.is_none(), "{old:?}"); } for (num, user) in val.serial.iter() { @@ -92,18 +103,20 @@ impl From for InstanceSpecV0 { SerialPortNumber::Com4 => "com4", }; - let old = spec - .devices - .serial_ports - .insert(name.to_owned(), SerialPortDesc { num: *num }); - - assert!(old.is_none(), "{old:?}"); + insert_component( + &mut spec.devices.serial_ports, + name.to_owned(), + SerialPortDesc { num: *num }, + ); } } for (bridge_name, bridge) in val.pci_pci_bridges { - let old = spec.devices.pci_pci_bridges.insert(bridge_name, bridge); - assert!(old.is_none(), "{old:?}"); + insert_component( + &mut spec.devices.pci_pci_bridges, + bridge_name, + bridge, + ); } spec.devices.qemu_pvpanic = val.pvpanic.map(|pvpanic| pvpanic.spec); @@ -114,7 +127,8 @@ impl From for InstanceSpecV0 { spec.devices.softnpu_p9 = val.softnpu.p9_device; spec.devices.p9fs = val.softnpu.p9fs; for (port_name, port) in val.softnpu.ports { - let old = spec.devices.softnpu_ports.insert( + insert_component( + &mut spec.devices.softnpu_ports, port_name.clone(), SoftNpuPortSpec { name: port_name, @@ -122,14 +136,11 @@ impl From for InstanceSpecV0 { }, ); - assert!(old.is_none(), "{old:?}"); - - let old = spec.backends.network_backends.insert( + insert_component( + &mut spec.backends.network_backends, port.backend_name, NetworkBackendV0::Dlpi(port.backend_spec), ); - - assert!(old.is_none(), "{old:?}"); } } From 6fcdd1f6f8b168c82206a95e6e0b95efbc49ada0 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 11 Sep 2024 13:36:04 -0700 Subject: [PATCH 11/11] add track_caller attribute (thanks eliza!) Co-authored-by: Eliza Weisman --- bin/propolis-server/src/lib/spec/api_spec_v0.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 276d11239..a41b5185e 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -51,6 +51,7 @@ impl From for InstanceSpecV0 { // This assertion is valid because internal instance specs should assign // a unique name to each component they describe. The spec builder // upholds this invariant at spec creation time. + #[track_caller] fn insert_component( map: &mut HashMap, key: String,