rearrange explanatory comments

Author: gjcolombo

lib/propolis-client/src/support.rs

@@ -30,6 +30,19 @@ 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`.
+///
+/// NOTE: Version 1.2.1 of the NVMe specification (June 5, 2016) specifies in
+/// section 1.5 that ASCII data fields, including serial numbers, must be
+/// left-justified and must use 0x20 bytes (spaces) as the padding value. This
+/// function allows callers to choose a non-0x20 padding value to preserve the
+/// serial numbers for existing disks, which serial numbers may have been used
+/// previously and persisted into a VM's nonvolatile EFI variables (such as its
+/// boot order variables).
+//
+// TODO(#790): Ideally, this routine would have no `pad` parameter at all and
+// would always pad with spaces, but whether this is ultimately possible depends
+// on whether Omicron can start space-padding serial numbers for disks that were
+// attached to a Propolis VM that zero-padded their serial numbers.
 pub fn nvme_serial_from_str(s: &str, pad: u8) -> [u8; 20] {
     let mut sn = [0u8; 20];
 
@@ -600,5 +613,8 @@ mod test {
             nvme_serial_from_str("very_long_disk_name_goes_here", b'?'),
             *expected
         );
+
+        let expected = b"nonvolatile EFI\0\0\0\0\0";
+        assert_eq!(nvme_serial_from_str("nonvolatile EFI", 0), *expected);
     }
 }

phd-tests/framework/src/test_vm/config.rs

@@ -316,26 +316,11 @@ impl<'dr> VmConfig<'dr> {
                     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.
+                        // Omicron supplies (or will supply, as of this writing)
+                        // 0 as the padding byte to maintain compatibility for
+                        // existing disks. Match that behavior here so that PHD
+                        // and Omicron VM configurations are as similar as
+                        // possible.
                         0,
                     ),
                 }),