Support guest memory sizes >=1028 GiB (#912)

Fixes #907.

This change both produces an E820 table for OVMF to consult for highmem
layout, and clamps the RTC CMOS bytes through which we communicate
highmem size to OVMF. The E820 table supplants the RTC CMOS for OVMF's
purposes, so in theory nothing will check those bytes anymore. Even so,
clamping to them to `ffffff` seems more legible.

Author: iximeow

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

@@ -1112,6 +1112,28 @@ impl MachineInitializer<'_> {
         smb_tables.commit()
     }
 
+    fn generate_e820(&self) -> Result<Entry, MachineInitError> {
+        info!(self.log, "Generating E820 map for guest address space");
+
+        let mut e820_table = fwcfg::formats::E820Table::new();
+
+        for (addr, len, kind) in self.machine.map_physmem.mappings().into_iter()
+        {
+            let addr = addr.try_into().expect("usize should fit into u64");
+            let len = len.try_into().expect("usize should fit into u64");
+            match kind {
+                propolis::vmm::MapType::Dram => {
+                    e820_table.add_mem(addr, len);
+                }
+                _ => {
+                    e820_table.add_reserved(addr, len);
+                }
+            }
+        }
+
+        Ok(e820_table.finish())
+    }
+
     fn generate_bootorder(&self) -> Result<Option<Entry>, MachineInitError> {
         info!(
             self.log,
@@ -1212,6 +1234,10 @@ impl MachineInitializer<'_> {
                 MachineInitError::FwcfgInsertFailed("bootorder", e)
             })?;
         }
+        let e820_entry = self.generate_e820()?;
+        fwcfg
+            .insert_named("etc/e820", e820_entry)
+            .map_err(|e| MachineInitError::FwcfgInsertFailed("e820", e))?;
 
         let ramfb = ramfb::RamFb::create(
             self.log.new(slog::o!("component" => "ramfb")),

bin/propolis-standalone/src/main.rs

@@ -964,6 +964,32 @@ fn generate_smbios(params: SmbiosParams) -> anyhow::Result<smbios::TableBytes> {
     Ok(smb_tables.commit())
 }
 
+fn generate_e820(
+    machine: &Machine,
+    log: &slog::Logger,
+) -> anyhow::Result<fwcfg::Entry> {
+    slog::info!(log, "Generating E820 map for guest address space",);
+
+    let mut e820_table = fwcfg::formats::E820Table::new();
+
+    use propolis::vmm::MapType;
+
+    for (addr, len, kind) in machine.map_physmem.mappings().into_iter() {
+        let addr = addr.try_into().context("usize should fit into u64")?;
+        let len = len.try_into().context("usize should fit into u64")?;
+        match kind {
+            MapType::Dram => {
+                e820_table.add_mem(addr, len);
+            }
+            _ => {
+                e820_table.add_reserved(addr, len);
+            }
+        }
+    }
+
+    Ok(e820_table.finish())
+}
+
 fn generate_bootorder(
     config: &config::Config,
     log: &slog::Logger,
@@ -1350,6 +1376,8 @@ fn setup_instance(
     {
         fwcfg.insert_named("bootorder", boot_config).unwrap();
     }
+    let e820_entry = generate_e820(machine, log).expect("can build E820 table");
+    fwcfg.insert_named("etc/e820", e820_entry).unwrap();
 
     fwcfg.attach(pio, &machine.acc_mem);
 

lib/propolis/src/hw/bhyve/rtc.rs

@@ -94,7 +94,15 @@ impl BhyveRtc {
 
         // High-mem
         if high_mem > 0 {
-            let high = ((high_mem / CHUNK) as u32).to_le_bytes();
+            // If high_mem is 1TiB or larger, the division below produces a
+            // number that overflows the 24 bits available at
+            // `CMOS_OFF_MEM_HIGH`. Clamp the value so guests aren't subjected
+            // to arbitrary wrapping. OVMF is told about the highmem layout via
+            // E820 table anyway, so the only thing that might care about these
+            // bytes are guest OSes that check the RTC CMOS bytes directly.
+            let chunks =
+                std::cmp::min(high_mem / CHUNK, u32::MAX as usize) as u32;
+            let high = chunks.to_le_bytes();
 
             hdl.rtc_write(CMOS_OFF_MEM_HIGH, high[0])?;
             hdl.rtc_write(CMOS_OFF_MEM_HIGH + 1, high[1])?;

lib/propolis/src/hw/qemu/fwcfg.rs

@@ -1057,6 +1057,158 @@ mod test {
 pub mod formats {
     use super::Entry;
     use crate::hw::pci;
+    use zerocopy::{Immutable, IntoBytes};
+
+    /// A type for a range described in an E820 map entry.
+    ///
+    /// This is canonically defined as the ACPI "Address Range Types", though we
+    /// only define the types we use, which are a subset of the types that EDK2
+    /// is known to care about, which itself is a subset of types that ACPI and
+    /// OEMs define or guest OSes may care about.
+    #[derive(IntoBytes, Immutable)]
+    #[repr(u32)]
+    enum EfiAcpiMemoryType {
+        Memory = 1,
+        Reserved = 2,
+        // For reference, though these types are unused.
+        // Acpi = 3,
+        // Nvs = 4,
+    }
+
+    /// One address/length/type entry in the E820 map.
+    ///
+    /// This is... almost defined by ACPI's "Address Range Descriptor Structure"
+    /// table, under "INT 15H, E820". Critically, ACPI defines this structure
+    /// with an additional "Extended Attributes" field which EDK2 does not know
+    /// about and so we do not provide. Consequently the size of this struct is
+    /// 20 bytes as defined in `OvmfPkg/Include/IndustryStandard/E820.h` rather
+    /// than the ACPI definition's 24 bytes.
+    #[derive(IntoBytes, Immutable)]
+    #[repr(C, packed)]
+    struct E820Entry64 {
+        base_addr: u64,
+        length: u64,
+        ty: EfiAcpiMemoryType,
+    }
+
+    /// A list of E820 memory map entries.
+    ///
+    /// This is not defined by ACPI, but is an EDK2 implementation of a QMEU
+    /// construct to communicate an E820 map to the firmware. It is parsed by
+    /// EDK2 and added to its EFI memory map; it is not, itself, the memory map
+    /// that OVMF presents via UEFI services. It is not required to be sorted,
+    /// and EDK2 ignores entries starting below 4 GiB. Adding additional
+    /// low-memory entries is not harmful, but not valuable to EDK2 either.
+    pub struct E820Table(Vec<E820Entry64>);
+    impl E820Table {
+        pub fn new() -> Self {
+            Self(Vec::new())
+        }
+
+        /// Add an address range corresponding to usable memory.
+        pub fn add_mem(&mut self, base_addr: u64, length: u64) {
+            self.0.push(E820Entry64 {
+                base_addr,
+                length,
+                ty: EfiAcpiMemoryType::Memory,
+            });
+        }
+
+        /// Add a reserved address, not to be used by the guest OS.
+        pub fn add_reserved(&mut self, base_addr: u64, length: u64) {
+            self.0.push(E820Entry64 {
+                base_addr,
+                length,
+                ty: EfiAcpiMemoryType::Reserved,
+            });
+        }
+
+        pub fn finish(self) -> Entry {
+            Entry::Bytes(self.0.as_bytes().to_vec())
+        }
+    }
+
+    #[cfg(test)]
+    mod test_e820 {
+        use super::{E820Entry64, E820Table};
+        use crate::hw::qemu::fwcfg::Entry;
+
+        #[test]
+        fn entry_size_is_correct() {
+            // Compare the size of our definition of an E820 to EDK2's
+            // definition. EDK2 interprets our provided bytes by its definition,
+            // so they must match.
+            assert_eq!(std::mem::size_of::<E820Entry64>(), 20);
+        }
+
+        #[test]
+        fn basic() {
+            let mut e820_table = E820Table::new();
+
+            // Arbitrary bit patterns here, just to make eyeballing the layout
+            // more straightforward.
+            //
+            // Also note the E820 table itself does not check if ranges overlap.
+            // In practice it is directly constructed from an ASpace, which does
+            // perform those checks.
+            e820_table.add_mem(0x0102_0304_0506_0010, 0x1122_3344_5566_7788);
+            e820_table
+                .add_reserved(0x0102_0304_0506_fff0, 0xffee_ddcc_bbaa_9988);
+
+            // We also don't require the E820 map to be ordered. ACPI does not
+            // imply that it should be, nor do EDK2 or guest OSes, even though
+            // entries are often enumerated in address order.
+            e820_table.add_mem(0x0102_0304_0506_0000, 0x1122_3344_5566_7799);
+
+            // rustfmt::skip here and below because eight bytes per line helps
+            // eyeball with the entries as written above. rustfmt would try to
+            // fit ten bytes per row to pack the 80-column width and that's just
+            // annoying here.
+            #[rustfmt::skip]
+            const FIRST_ENTRY: [u8; 20] = [
+                0x10, 0x00, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01,
+                0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11,
+                0x01, 0x00, 0x00, 0x00,
+            ];
+
+            #[rustfmt::skip]
+            const SECOND_ENTRY: [u8; 20] = [
+                0xf0, 0xff, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01,
+                0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff,
+                0x02, 0x00, 0x00, 0x00,
+            ];
+
+            #[rustfmt::skip]
+            const THIRD_ENTRY: [u8; 20] = [
+                0x00, 0x00, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01,
+                0x99, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11,
+                0x01, 0x00, 0x00, 0x00,
+            ];
+
+            let entry = e820_table.finish();
+            let Entry::Bytes(bytes) = entry else {
+                panic!("entry did not produce bytes, but instead {:?}", entry);
+            };
+
+            let expected_size =
+                FIRST_ENTRY.len() + SECOND_ENTRY.len() + THIRD_ENTRY.len();
+            assert_eq!(bytes.len(), expected_size);
+
+            let tests = [
+                (&bytes[0..20], &FIRST_ENTRY, "First E820 entry"),
+                (&bytes[20..40], &SECOND_ENTRY, "Second E820 entry"),
+                (&bytes[40..60], &THIRD_ENTRY, "Third E820 entry"),
+            ];
+
+            for (actual, expected, entry_name) in tests.iter() {
+                assert_eq!(
+                    actual, expected,
+                    "{} contents are incorrect",
+                    entry_name
+                );
+            }
+        }
+    }
 
     /// Collect one or more device elections for use in generating a boot order
     /// `fw_cfg` entry, suitable for consumption by OVMF bootrom.
@@ -1117,7 +1269,7 @@ pub mod formats {
     }
 
     #[cfg(test)]
-    mod test {
+    mod test_bootorder {
         use super::BootOrder;
         use crate::hw::pci::BusLocation;
         use crate::hw::qemu::fwcfg;

lib/propolis/src/vmm/mem.rs

@@ -61,6 +61,22 @@ pub(crate) struct MapEnt {
     kind: MapKind,
 }
 
+impl MapEnt {
+    fn map_type(&self) -> MapType {
+        match &self.kind {
+            MapKind::Dram(_) => MapType::Dram,
+            MapKind::Rom(_) => MapType::Rom,
+            MapKind::MmioReserve => MapType::Mmio,
+        }
+    }
+}
+
+pub enum MapType {
+    Dram,
+    Rom,
+    Mmio,
+}
+
 pub struct PhysMap {
     map: Arc<Mutex<ASpace<MapEnt>>>,
     hdl: Arc<VmmHdl>,
@@ -162,6 +178,17 @@ impl PhysMap {
         Ok(())
     }
 
+    pub fn mappings(&self) -> Vec<(usize, usize, MapType)> {
+        let guard = self.map.lock().unwrap();
+        let mut mappings = Vec::new();
+
+        for (addr, len, ent) in guard.iter() {
+            mappings.push((addr, len, ent.map_type()));
+        }
+
+        mappings
+    }
+
     pub(crate) fn memctx(&mut self) -> Arc<MemCtx> {
         self.memctx.clone()
     }