take serial numbers in NVMe specs

Author: gjcolombo

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: {:?}",

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)),
                     );

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);

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.

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,
     ))

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
+        );
+    }
 }

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<u8>,
         log: slog::Logger,
     ) -> Arc<Self> {
@@ -527,16 +527,12 @@ impl PciNvme {
         let cqes = size_of::<CompletionQueueEntry>().trailing_zeros() as u8;
         let sqes = size_of::<SubmissionQueueEntry>().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

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
       },

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

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,
+                    ),
                 }),
             };