VMs using default `nvme list` from nvme-cli segfault

[InternalIncident]

[root@rocky-test]# nvme list
Segmentation fault (core dumped)

generic nvme list likely expects nvme devices to have a model number that isn't null, using -o json produces output.

the most recent (2.3) nvme spec specifies that model number is set by the vendor as a ASCII string in section 4.5.2 and this doesn't appear to be considered an optional field.

Would it make sense to populate that field with the disk type as distributed or local?

[hawkw]

I believe the model number we return is determined when the IdentifyController struct is initialized here:

propolis/lib/propolis/src/hw/nvme/mod.rs

Lines 834 to 856 in 979b728

	// 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: *serial_number,
	ieee: OXIDE_OUI,
	mdts: mdts.unwrap_or(0),
	// We use standard Completion/Submission Queue Entry structures with no extra
	// data, so required (minimum) == maximum
	sqes: NvmQueueEntrySize(0).with_maximum(sqes).with_required(sqes),
	cqes: NvmQueueEntrySize(0).with_maximum(cqes).with_required(cqes),
	// Supporting multiple namespaces complicates I/O dispatching,
	// so for now we limit the device to a single namespace.
	nn: 1,
	// bit 2 indicates support for the Dataset Management command
	oncs: (1 << 2),
	// bit 0 indicates volatile write cache is present
	vwc: 1,
	// bit 8 indicates Doorbell Buffer support
	oacs: (1 << 8),
	..Default::default()
	};

...and since this code doesn't explicitly set a value for mn (the model number), it gets 40 bytes of zero from the Default impl here:

propolis/lib/propolis/src/hw/nvme/bits.rs

Line 1060 in 979b728

mn: [0; 40],

Pedantically, I might argue that "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" is a valid ASCII string (it's the empty string), but clearly nvme-cli doesn't agree, which is not unreasonable of it.

In propolis-server, the value of the serial number used in IdentifyController is passed through from the instance spec, over here:

propolis/bin/propolis-server/src/lib/initializer.rs

Lines 899 to 918 in 979b728

	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(
	&nvme_spec.serial_number,
	mdts,
	self.log.new(slog::o!("component" => component)),
	);
	self.devices.insert(device_id.clone(), nvme.clone());
	block::attach(&nvme.block_attach, backend.attachment())
	.unwrap();
	chipset.pci_attach(bdf, nvme.clone());
	nvme

We probably want to add a model number field to the NvmeDisk instance spec type, here, in addition to the serial number:

propolis/crates/propolis-api-types-versions/src/initial/components/devices.rs

Lines 23 to 36 in 979b728

	/// A disk that presents an NVMe interface to the guest.
	#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
	#[serde(deny_unknown_fields)]
	pub struct NvmeDisk {
	/// The name of the disk's backend component.
	pub backend_id: SpecKey,
	/// 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],
	}

That way, the control plane can provide propolis with whatever it thinks is an appropriate model number to present to the guest.

[iximeow]

james had a PR about this a bit ago: #979 (and @hawkw you can see why it's still Like This For Now)

[iximeow]

relatedly on what we should do with the model number: #1004

[hawkw]

existing disks have NVMe-violating null strings for their model number and changing that can break guest-determined boot options.

i'm going to kill myself

[iximeow]

Pedantically, I might argue that [..]

the NVMe spec has also out-pedanticted you there too by saying "ASCII strings shall contain only code values (i.e., byte values or octet values) 20h through 7Eh.", so, propolis is just wrong :)

[hawkw]

i guess we should probably make a category of propolis issues that are just like "All The Things We Can Fix After We Have Some Meaningful Concept of Virtual Platform Versioning", huh? unless we already have that and i've forgotten about it