diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 2b6107b18..c0fd10b49 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -1041,26 +1041,38 @@ impl<'a> MachineInitializer<'a> { match &spec.device_spec { StorageDevice::Virtio(disk) => { let bdf = parse_bdf(&disk.pci_path)?; - // TODO: check that bus is 0. only support boot devices - // directly attached to the root bus for now. + if bdf.bus.get() != 0 { + return Err(Error::new( + ErrorKind::InvalidInput, + "Boot device currently must be on PCI bus 0", + )); + } + order.add_disk(bdf.location); } StorageDevice::Nvme(disk) => { let bdf = parse_bdf(&disk.pci_path)?; - // TODO: check that bus is 0. only support boot devices - // directly attached to the root bus for now. - // + if bdf.bus.get() != 0 { + return Err(Error::new( + ErrorKind::InvalidInput, + "Boot device currently must be on PCI bus 0", + )); + } + // TODO: separately, propolis-standalone passes an eui64 // of 0, so do that here too. is that.. ok? order.add_nvme(bdf.location, 0); } }; } else { + // This should be unreachable - we check that the boot disk is + // valid when constructing the spec we're initializing from. let message = format!( - "Instance spec included boot entry which does not refer to an existing disk: `{}`", + "Instance spec included boot entry which does not refer \ + to an existing disk: `{}`", boot_entry.name.as_str(), ); - return Err(Error::new(ErrorKind::Other, message)); + return Err(Error::new(ErrorKind::InvalidInput, message)); } } diff --git a/lib/propolis-client/src/instance_spec.rs b/lib/propolis-client/src/instance_spec.rs index cb8bbc449..8c75bee98 100644 --- a/lib/propolis-client/src/instance_spec.rs +++ b/lib/propolis-client/src/instance_spec.rs @@ -186,14 +186,16 @@ impl SpecBuilderV0 { /// Sets a boot order. Names here refer to devices included in this spec. /// - /// Permissible to not this if the implicit boot order is desired, but the implicit boot order - /// may be unstable across device addition and removal. + /// Permissible to not set this if the implicit boot order is desired, but + /// the implicit boot order may be unstable across device addition and + /// removal. /// - /// If any devices named in this order are not actually present in the constructed spec, - /// Propolis will return an error when the spec is provided. + /// If any devices named in this order are not actually present in the + /// constructed spec, Propolis will return an error when the spec is + /// provided. /// - /// XXX: this should certainly return `&mut Self` - all the builders here should. check if any - /// of these are chained..? + /// XXX: this should certainly return `&mut Self` - all the builders here + /// should. check if any of these are chained..? pub fn set_boot_order( &mut self, boot_order: Vec, diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 2a4b42921..14d92fb32 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -132,10 +132,11 @@ impl<'dr> VmConfig<'dr> { self } - /// Add a new disk to the VM config, and add it to the front of the VM's boot order. + /// Add a new disk to the VM config, and add it to the front of the VM's + /// boot order. /// - /// The added disk will have the name `boot-disk`, and replace the previous existing - /// `boot-disk`. + /// The added disk will have the name `boot-disk`, and replace the previous + /// existing `boot-disk`. pub fn boot_disk( &mut self, artifact: &'dr str, @@ -207,28 +208,37 @@ impl<'dr> VmConfig<'dr> { }) .context("serializing Propolis server config")?; - // The first disk in the boot list might not be the disk a test *actually* expects to boot. + // The first disk in the boot list might not be the disk a test + // *actually* expects to boot. // - // If there are multiple bootable disks in the boot order, we'll assume they're all - // the same guest OS kind. So look for `boot-disk` - if there isn't a disk named - // `boot-disk` then fall back to hoping the first disk in the boot order is a bootable - // disk, and if *that* isn't a bootable disk, maybe the first disk is. + // If there are multiple bootable disks in the boot order, we'll assume + // they're all the same guest OS kind. So look for `boot-disk` - if + // there isn't a disk named `boot-disk` then fall back to hoping the + // first disk in the boot order is a bootable disk, and if *that* isn't + // a bootable disk, maybe the first disk is. // - // TODO: theoretically we might want to accept configuration of a specific guest OS adapter - // and avoid the guessing games. So far the above supports existing tests and makes them - // "Just Work", but a more complicated test may want more control here. - let boot_disk = - self.disks.iter().find(|d| d.name == "boot-disk") - .or_else(|| if let Some(boot_order) = self.boot_order.as_ref() { - boot_order.first().and_then(|name| self.disks.iter().find(|d| &d.name == name)) + // TODO: theoretically we might want to accept configuration of a + // specific guest OS adapter and avoid the guessing games. So far the + // above supports existing tests and makes them "Just Work", but a more + // complicated test may want more control here. + let boot_disk = self + .disks + .iter() + .find(|d| d.name == "boot-disk") + .or_else(|| { + if let Some(boot_order) = self.boot_order.as_ref() { + boot_order.first().and_then(|name| { + self.disks.iter().find(|d| &d.name == name) + }) } else { None - }) - .or_else(|| self.disks.first()) - .expect("VM config includes at least one disk (and maybe a boot order)?"); + } + }) + .or_else(|| self.disks.first()) + .expect("VM config includes at least one disk"); - // XXX: assuming all bootable images are equivalent to the first, or at least the same - // guest OS kind. + // XXX: assuming all bootable images are equivalent to the first, or at + // least the same guest OS kind. let DiskSource::Artifact(boot_artifact) = boot_disk.source else { unreachable!("boot disks always use artifacts as sources"); }; @@ -242,7 +252,6 @@ impl<'dr> VmConfig<'dr> { let mut disk_handles = Vec::new(); for disk in self.disks.iter() { disk_handles.push( - // format!("data-disk-{}", idx) make_disk(disk.name.to_owned(), framework, disk) .await .context("creating disk")?, diff --git a/phd-tests/tests/src/boot_order.rs b/phd-tests/tests/src/boot_order.rs index d906abd7d..569a72458 100644 --- a/phd-tests/tests/src/boot_order.rs +++ b/phd-tests/tests/src/boot_order.rs @@ -353,11 +353,7 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) { write_efivar(&vm, &bootvar(0xfff0), &boot_option_bytes).await?; let reread = read_efivar(&vm, &bootvar(0xfff0)).await?; if reread.is_empty() { - warn!( - "guest environment drops EFI variable writes! \ - exiting test WITHOUT VALIDATING ANYTHING" - ); - return Ok(()); + phd_skip!("Guest environment drops EFI variable writes"); } else { assert_eq!( boot_option_bytes,