Remove Snapshot's empty regions field (#1509)

Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>

Author: ludfjig

src/hyperlight_host/src/mem/mgr.rs

@@ -864,24 +864,20 @@ impl SandboxMemoryManager<HostSharedMemory> {
 #[cfg(test)]
 #[cfg(all(not(feature = "i686-guest"), target_arch = "x86_64"))]
 mod tests {
-    use hyperlight_common::vmem::{MappingKind, PAGE_TABLE_SIZE};
     use hyperlight_testing::sandbox_sizes::{LARGE_HEAP_SIZE, MEDIUM_HEAP_SIZE, SMALL_HEAP_SIZE};
     use hyperlight_testing::simple_guest_as_string;
 
     use crate::GuestBinary;
-    use crate::mem::memory_region::MemoryRegionFlags;
     use crate::sandbox::SandboxConfiguration;
     use crate::sandbox::snapshot::Snapshot;
 
-    /// Verify page tables for a given configuration.
-    /// Creates a Snapshot and verifies every page in every region has correct PTEs.
+    /// Build a Snapshot for the given configuration and verify the
+    /// NULL page is not mapped in its page tables.
     fn verify_page_tables(name: &str, config: SandboxConfiguration) {
         let path = simple_guest_as_string().expect("failed to get simple guest path");
         let snapshot = Snapshot::from_env(GuestBinary::FilePath(path), config)
             .unwrap_or_else(|e| panic!("{}: failed to create snapshot: {}", name, e));
 
-        let regions = snapshot.regions();
-
         // Verify NULL page (0x0) is NOT mapped
         assert!(
             unsafe { hyperlight_common::vmem::virt_to_phys(&snapshot, 0, 1) }
@@ -890,57 +886,6 @@ mod tests {
             "{}: NULL page (0x0) should NOT be mapped",
             name
         );
-
-        // Verify every page in every region
-        for region in regions {
-            let mut addr = region.guest_region.start as u64;
-
-            while addr < region.guest_region.end as u64 {
-                let mapping = unsafe { hyperlight_common::vmem::virt_to_phys(&snapshot, addr, 1) }
-                    .next()
-                    .unwrap_or_else(|| {
-                        panic!(
-                            "{}: {:?} region: address 0x{:x} is not mapped",
-                            name, region.region_type, addr
-                        )
-                    });
-
-                // Verify identity mapping (phys == virt for low memory)
-                assert_eq!(
-                    mapping.phys_base, addr,
-                    "{}: {:?} region: address 0x{:x} should identity map, got phys 0x{:x}",
-                    name, region.region_type, addr, mapping.phys_base
-                );
-
-                // Verify kind is Basic
-                let MappingKind::Basic(bm) = mapping.kind else {
-                    panic!(
-                        "{}: {:?} region: address 0x{:x} should be kind basic, got {:?}",
-                        name, region.region_type, addr, mapping.kind
-                    );
-                };
-
-                // Verify writable
-                let actual = bm.writable;
-                let expected = region.flags.contains(MemoryRegionFlags::WRITE);
-                assert_eq!(
-                    actual, expected,
-                    "{}: {:?} region: address 0x{:x} has writable {}, expected {} (region flags: {:?})",
-                    name, region.region_type, addr, actual, expected, region.flags
-                );
-
-                // Verify executable
-                let actual = bm.executable;
-                let expected = region.flags.contains(MemoryRegionFlags::EXECUTE);
-                assert_eq!(
-                    actual, expected,
-                    "{}: {:?} region: address 0x{:x} has executable {}, expected {} (region flags: {:?})",
-                    name, region.region_type, addr, actual, expected, region.flags
-                );
-
-                addr += PAGE_TABLE_SIZE as u64;
-            }
-        }
     }
 
     #[test]

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-use std::collections::HashSet;
 use std::path::Path;
 use std::sync::{Arc, Mutex};
 
@@ -534,24 +533,13 @@ impl MultiUseSandbox {
         self.vm.set_stack_top(snapshot.stack_top_gva());
         self.vm.set_entrypoint(snapshot.entrypoint());
 
-        let current_regions: HashSet<_> = self.vm.get_mapped_regions().cloned().collect();
-        let snapshot_regions: HashSet<_> = snapshot.regions().iter().cloned().collect();
-
-        let regions_to_unmap = current_regions.difference(&snapshot_regions);
-        let regions_to_map = snapshot_regions.difference(&current_regions);
-
-        for region in regions_to_unmap {
+        let current_regions: Vec<MemoryRegion> = self.vm.get_mapped_regions().cloned().collect();
+        for region in &current_regions {
             self.vm
                 .unmap_region(region)
                 .map_err(HyperlightVmError::UnmapRegion)?;
         }
 
-        for region in regions_to_map {
-            // Safety: The region has been mapped before, and at that point the caller promised that the memory region is valid
-            // in their call to `MultiUseSandbox::map_region`
-            unsafe { self.vm.map_region(region) }.map_err(HyperlightVmError::MapRegion)?;
-        }
-
         // The restored snapshot is now our most current snapshot
         self.snapshot = Some(snapshot.clone());
 

src/hyperlight_host/src/sandbox/snapshot/mod.rs

@@ -68,9 +68,6 @@ pub struct Snapshot {
     layout: crate::mem::layout::SandboxMemoryLayout,
     /// Memory of the sandbox at the time this snapshot was taken
     memory: ReadonlySharedMemory,
-    /// The memory regions that were mapped when this snapshot was
-    /// taken (excluding initial sandbox regions)
-    regions: Vec<MemoryRegion>,
     /// Extra debug information about the binary in this snapshot,
     /// from when the binary was first loaded into the snapshot.
     ///
@@ -364,12 +361,9 @@ impl Snapshot {
             - hyperlight_common::layout::SCRATCH_TOP_EXN_STACK_OFFSET
             + 1;
 
-        let extra_regions = Vec::new();
-
         Ok(Self {
             memory: ReadonlySharedMemory::from_bytes(&memory, layout.snapshot_size)?,
             layout,
-            regions: extra_regions,
             load_info,
             stack_top_gva: exn_stack_top_gva,
             sregs: None,
@@ -547,19 +541,9 @@ impl Snapshot {
         debug_assert!(guest_visible_size.is_multiple_of(PAGE_SIZE));
         layout.set_snapshot_size(guest_visible_size);
 
-        // Drop the embedder-provided regions: post-compaction every
-        // VA that used to map into a `map_file_cow` region has been
-        // rewritten to point at the new copy inside the snapshot blob
-        // (see the `guest_page` walk above). Re-mapping the originals
-        // on restore is unnecessary for the translation to work and
-        // actively risks corrupting the snapshot if the new snapshot
-        // PAs overlap the old region PAs.
-        let regions: Vec<MemoryRegion> = Vec::new();
-
         Ok(Self {
             layout,
             memory: ReadonlySharedMemory::from_bytes(&memory, guest_visible_size)?,
-            regions,
             load_info,
             stack_top_gva,
             sregs: Some(sregs),
@@ -574,11 +558,6 @@ impl Snapshot {
         self.snapshot_generation
     }
 
-    /// Get the mapped regions from this snapshot
-    pub(crate) fn regions(&self) -> &[MemoryRegion] {
-        &self.regions
-    }
-
     /// Return the main memory contents of the snapshot
     #[instrument(skip_all, parent = Span::current(), level= "Trace")]
     pub(crate) fn memory(&self) -> &ReadonlySharedMemory {