teach propolis-server to understand configurable boot order

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

@@ -33,14 +33,20 @@ use propolis::hw::ibmpc;
 use propolis::hw::pci;
 use propolis::hw::ps2::ctrl::PS2Ctrl;
 use propolis::hw::qemu::pvpanic::QemuPvpanic;
-use propolis::hw::qemu::{debug::QemuDebugPort, fwcfg, ramfb};
+use propolis::hw::qemu::{
+    debug::QemuDebugPort,
+    fwcfg::{self, Entry},
+    ramfb,
+};
 use propolis::hw::uart::LpcUart;
 use propolis::hw::{nvme, virtio};
 use propolis::intr_pins;
 use propolis::vmm::{self, Builder, Machine};
-use propolis_api_types::instance_spec::{self, v0::InstanceSpecV0};
+use propolis_api_types::instance_spec::{
+    self, v0::BootDeclaration, v0::InstanceSpecV0,
+};
 use propolis_api_types::InstanceProperties;
-use slog::info;
+use slog::{error, info};
 
 /// Arbitrary ROM limit for now
 const MAX_ROM_SIZE: usize = 0x20_0000;
@@ -122,6 +128,7 @@ pub struct MachineInitializer<'a> {
     pub(crate) properties: &'a InstanceProperties,
     pub(crate) toml_config: &'a crate::server::VmTomlConfig,
     pub(crate) producer_registry: Option<ProducerRegistry>,
+    pub(crate) boot_order: Option<Vec<BootDeclaration>>,
     pub(crate) state: MachineInitializerState,
     pub(crate) kstat_sampler: Option<KstatSampler>,
 }
@@ -1069,6 +1076,123 @@ impl<'a> MachineInitializer<'a> {
         smb_tables.commit()
     }
 
+    fn generate_bootorder(&self) -> Result<Option<Entry>, Error> {
+        info!(
+            self.log,
+            "Generating bootorder with order: {:?}",
+            self.boot_order.as_ref()
+        );
+        let Some(boot_names) = self.boot_order.as_ref() else {
+            return Ok(None);
+        };
+
+        let mut order = fwcfg::formats::BootOrder::new();
+
+        let parse_bdf =
+            |pci_path: &propolis_api_types::instance_spec::PciPath| {
+                let bdf: Result<pci::Bdf, Error> =
+                    pci_path.to_owned().try_into().map_err(|e| {
+                        Error::new(
+                            ErrorKind::InvalidInput,
+                            format!(
+                                "Couldn't get PCI BDF for {}: {}",
+                                pci_path, e
+                            ),
+                        )
+                    });
+
+                bdf
+            };
+
+        for boot_entry in boot_names.iter() {
+            // names may refer to a storage device or network device.
+            //
+            // realistically we won't be booting from network devices, but leave that as a question
+            // for plumbing on the next layer up.
+
+            let storage_backend = |spec| {
+                let spec: &instance_spec::v0::StorageDeviceV0 = spec;
+                match spec {
+                    instance_spec::v0::StorageDeviceV0::VirtioDisk(disk) => {
+                        &disk.backend_name
+                    }
+                    instance_spec::v0::StorageDeviceV0::NvmeDisk(disk) => {
+                        &disk.backend_name
+                    }
+                }
+            };
+
+            let network_backend = |spec| {
+                let spec: &instance_spec::v0::NetworkDeviceV0 = spec;
+                let instance_spec::v0::NetworkDeviceV0::VirtioNic(vnic_spec) =
+                    spec;
+
+                &vnic_spec.backend_name
+            };
+
+            if let Some(device_spec) = self
+                .spec
+                .devices
+                .storage_devices
+                .iter()
+                .find_map(|(_name, spec)| {
+                    eprintln!("checking name \"{}\", spec {:?}", _name, spec);
+                    if storage_backend(spec) == &boot_entry.name {
+                        Some(spec)
+                    } else {
+                        None
+                    }
+                })
+            {
+                match device_spec {
+                    instance_spec::v0::StorageDeviceV0::VirtioDisk(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.
+                        order.add_disk(bdf.location);
+                    }
+                    instance_spec::v0::StorageDeviceV0::NvmeDisk(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.
+                        //
+                        // TODO: separately, propolis-standalone passes an eui64
+                        // of 0, so do that here too. is that.. ok?
+                        order.add_nvme(bdf.location, 0);
+                    }
+                };
+            } else if let Some(vnic_spec) =
+                self.spec.devices.network_devices.iter().find_map(
+                    |(_name, spec)| {
+                        if network_backend(spec) == &boot_entry.name {
+                            Some(spec)
+                        } else {
+                            None
+                        }
+                    },
+                )
+            {
+                let instance_spec::v0::NetworkDeviceV0::VirtioNic(vnic_spec) =
+                    vnic_spec;
+
+                let bdf = parse_bdf(&vnic_spec.pci_path)?;
+
+                order.add_pci(bdf.location, "ethernet");
+            } else {
+                let message = format!(
+                    "Instance spec included boot entry which does not refer to an existing device: `{}`",
+                    boot_entry.name.as_str(),
+                );
+                // TODO(ixi): this is actually duplicative with the top-level `error!` that this
+                // unhandled `error` will bubble out to. Maybe just don't log here?
+                error!(self.log, "{}", message);
+                return Err(Error::new(ErrorKind::Other, message));
+            }
+        }
+
+        Ok(Some(order.finish()))
+    }
+
     /// Initialize qemu `fw_cfg` device, and populate it with data including CPU
     /// count, SMBIOS tables, and attached RAM-FB device.
     ///
@@ -1085,6 +1209,13 @@ impl<'a> MachineInitializer<'a> {
             )
             .unwrap();
 
+        // TODO(ixi): extract `generate_smbios` and expect `smbios::TableBytes` as an argument to
+        // initialize_fwcfg. make `generate_smbios` accept rom size as an explicit parameter. this
+        // avoids the implicit panic if these are called out of order (and makes it harder to call
+        // out of order).
+        //
+        // one step towards sharing smbios init code between propolis-standalone and
+        // propolis-server.
         let smbios::TableBytes { entry_point, structure_table } =
             self.generate_smbios();
         fwcfg
@@ -1100,6 +1231,10 @@ impl<'a> MachineInitializer<'a> {
             )
             .unwrap();
 
+        if let Some(boot_order) = self.generate_bootorder()? {
+            fwcfg.insert_named("bootorder", boot_order).unwrap();
+        }
+
         let ramfb = ramfb::RamFb::create(
             self.log.new(slog::o!("component" => "ramfb")),
         );

bin/propolis-server/src/lib/vm/ensure.rs

@@ -180,6 +180,7 @@ impl<'a> VmEnsureNotStarted<'a> {
             properties,
             toml_config: &options.toml_config,
             producer_registry: options.oximeter_registry.clone(),
+            boot_order: v0_spec.devices.boot_order.clone(),
             state: MachineInitializerState::default(),
             kstat_sampler: initialize_kstat_sampler(
                 self.log,

bin/propolis-standalone/src/main.rs

@@ -959,8 +959,19 @@ fn generate_smbios(params: SmbiosParams) -> anyhow::Result<smbios::TableBytes> {
     Ok(smb_tables.commit())
 }
 
-fn generate_bootorder(config: &config::Config) -> anyhow::Result<fwcfg::Entry> {
-    let names = config.main.boot_order.as_ref().unwrap();
+fn generate_bootorder(
+    config: &config::Config,
+    log: &slog::Logger,
+) -> anyhow::Result<Option<fwcfg::Entry>> {
+    let Some(names) = config.main.boot_order.as_ref() else {
+        return Ok(None);
+    };
+
+    slog::info!(
+        log,
+        "Bootorder declared as {:?}",
+        config.main.boot_order.as_ref()
+    );
 
     let mut order = fwcfg::formats::BootOrder::new();
     for name in names.iter() {
@@ -994,7 +1005,7 @@ fn generate_bootorder(config: &config::Config) -> anyhow::Result<fwcfg::Entry> {
             }
         }
     }
-    Ok(order.finish())
+    Ok(Some(order.finish()))
 }
 
 fn setup_instance(
@@ -1306,14 +1317,10 @@ fn setup_instance(
 
     // It is "safe" to generate bootorder (if requested) now, given that PCI
     // device configuration has been validated by preceding logic
-    if config.main.boot_order.is_some() {
-        fwcfg
-            .insert_named(
-                "bootorder",
-                generate_bootorder(&config)
-                    .context("Failed to generate boot order")?,
-            )
-            .unwrap();
+    if let Some(boot_config) = generate_bootorder(&config, log)
+        .context("Failed to generate boot order")?
+    {
+        fwcfg.insert_named("bootorder", boot_config).unwrap();
     }
 
     fwcfg.attach(pio, &machine.acc_mem);

crates/propolis-api-types/src/instance_spec/v0.rs

@@ -109,6 +109,11 @@ pub struct DeviceSpecV0 {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub qemu_pvpanic: Option<components::devices::QemuPvpanic>,
 
+    // same as above
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub boot_order: Option<Vec<BootDeclaration>>,
+
     #[cfg(feature = "falcon")]
     pub softnpu_pci_port: Option<components::devices::SoftNpuPciPort>,
     #[cfg(feature = "falcon")]
@@ -177,6 +182,11 @@ impl DeviceSpecV0 {
     }
 }
 
+#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
+pub struct BootDeclaration {
+    pub name: SpecKey,
+}
+
 #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
 #[serde(deny_unknown_fields, tag = "type", content = "component")]
 pub enum StorageBackendV0 {

lib/propolis-client/src/instance_spec.rs

@@ -184,6 +184,32 @@ 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.
+    ///
+    /// XXX: talk about what happens if names are included that do not name real devices..?
+    ///
+    /// 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<String>,
+    ) -> Result<&Self, SpecBuilderError> {
+        let boot_declarations = boot_order
+            .into_iter()
+            .map(|name| crate::types::BootDeclaration { name })
+            .collect();
+        eprintln!("setting boot order to {:?}", boot_declarations);
+        self.spec.devices.boot_order = Some(boot_declarations);
+
+        // TODO: would be nice to warn if any of the devices named here are not devices in the spec
+        // though.
+
+        Ok(self)
+    }
+
     /// Yields the completed spec, consuming the builder.
     pub fn finish(self) -> InstanceSpecV0 {
         self.spec

openapi/propolis-server.json

@@ -662,6 +662,13 @@
             "additionalProperties": {
               "$ref": "#/components/schemas/StorageDeviceV0"
             }
+          },
+          "boot_order": {
+            "nullable": true,
+            "type": "array",
+            "items": {
+              "$ref": "#/components/schemas/BootDeclaration"
+            }
           }
         },
         "required": [
@@ -1468,6 +1475,19 @@
           "vcr_matches"
         ]
       },
+      "BootDeclaration": {
+        "description": "A boot preference for some device.",
+        "type": "object",
+        "properties": {
+          "name": {
+            "description": "The name of the device to attempt booting from.",
+            "type": "string"
+          }
+        },
+        "required": [
+          "name"
+        ]
+      },
       "SerialPort": {
         "description": "A serial port device.",
         "type": "object",

phd-tests/framework/src/guest_os/mod.rs

@@ -13,6 +13,7 @@ mod alpine;
 mod debian11_nocloud;
 mod shell_commands;
 mod ubuntu22_04;
+mod ubuntu24_04;
 mod windows;
 mod windows_server_2016;
 mod windows_server_2019;
@@ -88,6 +89,7 @@ pub enum GuestOsKind {
     Alpine,
     Debian11NoCloud,
     Ubuntu2204,
+    Ubuntu2404,
     WindowsServer2016,
     WindowsServer2019,
     WindowsServer2022,
@@ -101,6 +103,7 @@ impl FromStr for GuestOsKind {
             "alpine" => Ok(Self::Alpine),
             "debian11nocloud" => Ok(Self::Debian11NoCloud),
             "ubuntu2204" => Ok(Self::Ubuntu2204),
+            "ubuntu2404" => Ok(Self::Ubuntu2404),
             "windowsserver2016" => Ok(Self::WindowsServer2016),
             "windowsserver2019" => Ok(Self::WindowsServer2019),
             "windowsserver2022" => Ok(Self::WindowsServer2022),
@@ -119,6 +122,7 @@ pub(super) fn get_guest_os_adapter(kind: GuestOsKind) -> Box<dyn GuestOs> {
             Box::new(debian11_nocloud::Debian11NoCloud)
         }
         GuestOsKind::Ubuntu2204 => Box::new(ubuntu22_04::Ubuntu2204),
+        GuestOsKind::Ubuntu2404 => Box::new(ubuntu24_04::Ubuntu2404),
         GuestOsKind::WindowsServer2016 => {
             Box::new(windows_server_2016::WindowsServer2016)
         }

phd-tests/framework/src/guest_os/ubuntu24_04.rs

@@ -0,0 +1,28 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//! Guest OS adaptations for Ubuntu 24.04 images. These must be prepped with
+//! a cloud-init disk that is configured with the appropriate user and password.
+
+use super::{CommandSequence, CommandSequenceEntry, GuestOs};
+
+pub(super) struct Ubuntu2404;
+
+impl GuestOs for Ubuntu2404 {
+    fn get_login_sequence(&self) -> CommandSequence {
+        CommandSequence(vec![
+            CommandSequenceEntry::wait_for("ubuntu login: "),
+            CommandSequenceEntry::write_str("root"),
+            CommandSequenceEntry::wait_for(self.get_shell_prompt()),
+        ])
+    }
+
+    fn get_shell_prompt(&self) -> &'static str {
+        "root@ubuntu:~#"
+    }
+
+    fn read_only_fs(&self) -> bool {
+        false
+    }
+}