Skip to content

Remove Snapshot's empty regions field #1509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ludfjig wants to merge 1 commit into
base: main
Choose a base branch
from
+4 −92
Open

Remove Snapshot's empty regions field #1509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 2 additions & 57 deletions src/hyperlight_host/src/mem/mgr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,24 +871,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) }
Expand All @@ -897,57 +893,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]
Expand Down
16 changes: 2 additions & 14 deletions src/hyperlight_host/src/sandbox/initialized_multi_use.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::atomic::Ordering;
use std::sync::{Arc, Mutex};
Expand Down Expand Up @@ -533,24 +532,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());

Expand Down
21 changes: 0 additions & 21 deletions src/hyperlight_host/src/sandbox/snapshot/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,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.
///
Expand Down Expand Up @@ -373,13 +370,10 @@ impl Snapshot {
- hyperlight_common::layout::SCRATCH_TOP_EXN_STACK_OFFSET
+ 1;

let extra_regions = Vec::new();

Ok(Self {
sandbox_id: SANDBOX_CONFIGURATION_COUNTER.fetch_add(1, Ordering::Relaxed),
memory: ReadonlySharedMemory::from_bytes(&memory, layout.snapshot_size)?,
layout,
regions: extra_regions,
load_info,
stack_top_gva: exn_stack_top_gva,
sregs: None,
Expand Down Expand Up @@ -558,20 +552,10 @@ 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 {
sandbox_id,
layout,
memory: ReadonlySharedMemory::from_bytes(&memory, guest_visible_size)?,
regions,
load_info,
stack_top_gva,
sregs: Some(sregs),
Expand All @@ -591,11 +575,6 @@ impl Snapshot {
self.sandbox_id
}

/// 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 {
Expand Down
Loading