diff --git a/lib/propolis-client/src/support.rs b/lib/propolis-client/src/support.rs index 071982d6..1f47da8c 100644 --- a/lib/propolis-client/src/support.rs +++ b/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); } } diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 61d64a5b..752c2517 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/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, ), }),