diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 5b9735658..940e21b6b 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -16,6 +16,7 @@ use anyhow::{anyhow, Context}; use clap::{Args, Parser, Subcommand}; use futures::{future, SinkExt}; use newtype_uuid::{GenericUuid, TypedUuid, TypedUuidKind, TypedUuidTag}; +use propolis_client::support::nvme_serial_from_str; use propolis_client::types::{ BlobStorageBackend, Board, Chipset, ComponentV0, CrucibleStorageBackend, I440Fx, InstanceEnsureRequest, InstanceInitializationMethod, @@ -241,6 +242,7 @@ impl DiskRequest { "nvme" => ComponentV0::NvmeDisk(NvmeDisk { backend_id: backend_id.clone(), pci_path, + serial_number: nvme_serial_from_str(&self.name, b' '), }), _ => anyhow::bail!( "invalid device type in disk request: {:?}", diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 583c441be..666e0f549 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -633,11 +633,17 @@ impl<'a> MachineInitializer<'a> { vioblk } DeviceInterface::Nvme => { + let spec::StorageDevice::Nvme(nvme_spec) = + &disk.device_spec + else { + unreachable!("disk is known to be an NVMe disk"); + }; + // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); let component = format!("nvme-{}", device_id); let nvme = nvme::PciNvme::create( - device_id.to_string(), + &nvme_spec.serial_number, mdts, self.log.new(slog::o!("component" => component)), ); diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs index 702fb409f..4010bb096 100644 --- a/bin/propolis-standalone/src/main.rs +++ b/bin/propolis-standalone/src/main.rs @@ -1230,7 +1230,14 @@ fn setup_instance( log.new(slog::o!("dev" => format!("nvme-{}", name))); // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); - let nvme = hw::nvme::PciNvme::create(dev_serial, mdts, log); + + let mut serial_number = [0u8; 20]; + let sz = dev_serial.len().min(20); + serial_number[..sz] + .clone_from_slice(&dev_serial.as_bytes()[..sz]); + + let nvme = + hw::nvme::PciNvme::create(&serial_number, mdts, log); guard.inventory.register_instance(&nvme, &bdf.to_string()); guard.inventory.register_block(&backend, 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 0c0c7e686..9dc86d031 100644 --- a/crates/propolis-api-types/src/instance_spec/components/devices.rs +++ b/crates/propolis-api-types/src/instance_spec/components/devices.rs @@ -29,6 +29,10 @@ pub struct NvmeDisk { /// The PCI bus/device/function at which this disk should be attached. pub pci_path: PciPath, + + /// The serial number to return in response to an NVMe Identify Controller + /// command. + pub serial_number: [u8; 20], } /// A network card that presents a virtio-net interface to the guest. diff --git a/crates/propolis-config-toml/src/spec.rs b/crates/propolis-config-toml/src/spec.rs index 07954c326..08baa7ee3 100644 --- a/crates/propolis-config-toml/src/spec.rs +++ b/crates/propolis-config-toml/src/spec.rs @@ -10,6 +10,7 @@ use std::{ }; use propolis_client::{ + support::nvme_serial_from_str, types::{ ComponentV0, DlpiNetworkBackend, FileStorageBackend, MigrationFailureInjector, NvmeDisk, P9fs, PciPciBridge, SoftNpuP9, @@ -289,9 +290,11 @@ fn parse_storage_device_from_config( Interface::Virtio => { ComponentV0::VirtioDisk(VirtioDisk { backend_id, pci_path }) } - Interface::Nvme => { - ComponentV0::NvmeDisk(NvmeDisk { backend_id, pci_path }) - } + Interface::Nvme => ComponentV0::NvmeDisk(NvmeDisk { + backend_id, + pci_path, + serial_number: nvme_serial_from_str(name, b' '), + }), }, id_to_return, )) diff --git a/lib/propolis-client/src/support.rs b/lib/propolis-client/src/support.rs index 228d309f3..071982d68 100644 --- a/lib/propolis-client/src/support.rs +++ b/lib/propolis-client/src/support.rs @@ -27,6 +27,18 @@ impl Default for Chipset { } } +/// Generates a 20-byte NVMe device serial number from the bytes in a string +/// slice. If the slice is too short to populate the entire serial number, the +/// remaining bytes are filled with `pad`. +pub fn nvme_serial_from_str(s: &str, pad: u8) -> [u8; 20] { + let mut sn = [0u8; 20]; + + let bytes_from_slice = sn.len().min(s.len()); + sn[..bytes_from_slice].copy_from_slice(&s.as_bytes()[..bytes_from_slice]); + sn[bytes_from_slice..].fill(pad); + sn +} + /// Clone of `InstanceSerialConsoleControlMessage` type defined in /// `propolis_api_types`, with which this must be kept in sync. /// @@ -572,4 +584,21 @@ mod test { { WebSocketStream::from_raw_socket(conn, Role::Server, None).await } + + #[test] + fn test_nvme_serial_from_str() { + use super::nvme_serial_from_str; + + let expected = b"hello world "; + assert_eq!(nvme_serial_from_str("hello world", b' '), *expected); + + let expected = b"enthusiasm!!!!!!!!!!"; + assert_eq!(nvme_serial_from_str("enthusiasm", b'!'), *expected); + + let expected = b"very_long_disk_name_"; + assert_eq!( + nvme_serial_from_str("very_long_disk_name_goes_here", b'?'), + *expected + ); + } } diff --git a/lib/propolis/src/hw/nvme/mod.rs b/lib/propolis/src/hw/nvme/mod.rs index 58450adc3..572291d05 100644 --- a/lib/propolis/src/hw/nvme/mod.rs +++ b/lib/propolis/src/hw/nvme/mod.rs @@ -505,7 +505,7 @@ pub struct PciNvme { impl PciNvme { /// Create a new pci-nvme device with the given values pub fn create( - serial_number: String, + serial_number: &[u8; 20], mdts: Option, log: slog::Logger, ) -> Arc { @@ -527,16 +527,12 @@ impl PciNvme { let cqes = size_of::().trailing_zeros() as u8; let sqes = size_of::().trailing_zeros() as u8; - let sz = std::cmp::min(20, serial_number.len()); - let mut sn: [u8; 20] = [0u8; 20]; - sn[..sz].clone_from_slice(&serial_number.as_bytes()[..sz]); - // Initialize the Identify structure returned when the host issues // an Identify Controller command. let ctrl_ident = bits::IdentifyController { vid: VENDOR_OXIDE, ssvid: VENDOR_OXIDE, - sn, + sn: *serial_number, ieee: OXIDE_OUI, mdts: mdts.unwrap_or(0), // We use standard Completion/Submission Queue Entry structures with no extra diff --git a/openapi/propolis-server.json b/openapi/propolis-server.json index 9e79d005f..4e23f600c 100644 --- a/openapi/propolis-server.json +++ b/openapi/propolis-server.json @@ -1513,11 +1513,23 @@ "$ref": "#/components/schemas/PciPath" } ] + }, + "serial_number": { + "description": "The serial number to return in response to an NVMe Identify Controller command.", + "type": "array", + "items": { + "type": "integer", + "format": "uint8", + "minimum": 0 + }, + "minItems": 20, + "maxItems": 20 } }, "required": [ "backend_id", - "pci_path" + "pci_path", + "serial_number" ], "additionalProperties": false }, diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index a0ca5c506..9458f4cd2 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/phd-tests/framework/src/disk/mod.rs @@ -95,6 +95,10 @@ impl DeviceName { pub fn into_string(self) -> String { self.0 } + + pub fn as_str(&self) -> &str { + self.0.as_str() + } } /// The name for a backend implementing storage for a disk. This is derived diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 1ea5ec14b..61d64a5b2 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use anyhow::Context; use cpuid_utils::CpuidIdent; use propolis_client::{ + support::nvme_serial_from_str, types::{ Board, BootOrderEntry, BootSettings, Chipset, ComponentV0, Cpuid, CpuidEntry, CpuidVendor, InstanceMetadata, InstanceSpecV0, @@ -313,6 +314,30 @@ impl<'dr> VmConfig<'dr> { backend_name.clone().into_string(), ), pci_path, + serial_number: nvme_serial_from_str( + device_name.as_str(), + // TODO(#790): Propolis's NVMe controller used to pad + // serial numbers with 0 bytes by default. This causes + // disk serial numbers to appear in the guest to be + // null-terminated strings. This, in turn, affects (or + // at least may affect) how guest firmware images + // identify disks when determining a VM's boot order: a + // disk whose serial number is padded to 20 bytes with + // spaces may not match a disk whose serial number was + // padded with \0 bytes. + // + // Unfortunately for us, guest firmware may persist disk + // identifiers to a VM's nonvolatile EFI variable store. + // This means that changing from zero-padded to + // space-padded serial numbers may break an existing + // VM's saved boot order. + // + // Until we decide whether and how to preserve + // compatibility for existing VMs that may have + // preserved a zero-padded disk ID, continue to zero-pad + // disk IDs in PHD to match previous behavior. + 0, + ), }), };