Skip to content

Change host's memory setup to be minimally configured #297

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.

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
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

danbugs
Copy link
Contributor

@danbugs danbugs commented Feb 27, 2025

We want to allow embedding custom guests in Hyperlight, but, currently, the static memory layout we configure in the host is too strict. This PR is modifying our memory layout to be minimally configured.

Here's an example of how a guest could address its memory:
image

Changes:

  • BASE_ADDRESS modified to be 0x0 instead of 0x200_000 (2MB), removing the arbitrary unmapped section of memory.
  • Moved guest code section to be at 0x0, shifting paging sections up.
  • Removes host/mem/layout.rs.
  • Removes MemMgrWrapper abstraction.
  • Added undifferentiated custom guest memory section that the guest can address in any way it wants.
  • Added API in common library to allow the guest to address the undifferentiated memory region.
  • Introduces SandboxBuilder API.
  • Added DebugPrint OutBAction, which replaces our HostPrint (inspired by this).
  • Puts us closer to running Nanvix on vanilla Hyperlight.
  • Makes input/output stacks portable (i.e., no longer relying on usize, which might be different between the guest and host).
  • Removed host function details memory region from shared memory (was only used for validation, which had questionable usefulness).
  • Refactored guest and host error data regions. Host error was never properly used and is removed, guest errors are now passed around via input/output stacks.

@danbugs danbugs added kind/refactor For PRs that restructure or remove code without adding new functionality. kind/enhancement For PRs adding features, improving functionality, docs, tests, etc. labels Feb 27, 2025
danbugs added a commit to danbugs/hyperlight that referenced this pull request Feb 27, 2025
…st code to start at 0x0

instead of after paging sections

This is the first step in moving memory sections around to provide a more generic
layout. After this, I'll move continue moving sections to fulfill the layout shown
in the description for PR hyperlight-dev#297.

Changes:
- removed no longer applicable checks in hypervisor_handler
- changed the way we build memory in get_memory_regions
- added functions to get the offsets/addresses to paging sections
as they are no longer static from 0x0 and are now relative to code size
- updated code docs
- changed set_up_shared_memory to properly place paging sections
- updated paging docs

Signed-off-by: danbugs <[email protected]>
danbugs added a commit to danbugs/hyperlight that referenced this pull request Feb 27, 2025
…st code to start at 0x0

instead of after paging sections

This is the first step in moving memory sections around to provide a more generic
layout. After this, I'll move continue moving sections to fulfill the layout shown
in the description for PR hyperlight-dev#297.

Changes:
- removed no longer applicable checks in hypervisor_handler
- changed the way we build memory in get_memory_regions
- added functions to get the offsets/addresses to paging sections
as they are no longer static from 0x0 and are now relative to code size
- updated code docs
- changed set_up_shared_memory to properly place paging sections
- updated paging docs

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from 56852a6 to 40a8091 Compare February 27, 2025 16:59
danbugs added a commit to danbugs/hyperlight that referenced this pull request Feb 28, 2025
…st code to start at 0x0

instead of after paging sections

This is the first step in moving memory sections around to provide a more generic
layout. After this, I'll move continue moving sections to fulfill the layout shown
in the description for PR hyperlight-dev#297.

Changes:
- removed no longer applicable checks in hypervisor_handler
- changed the way we build memory in get_memory_regions
- added functions to get the offsets/addresses to paging sections
as they are no longer static from 0x0 and are now relative to code size
- updated code docs
- changed set_up_shared_memory to properly place paging sections
- updated paging docs

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch 3 times, most recently from b7435ae to ce255f8 Compare February 28, 2025 02:06
danbugs added a commit to danbugs/hyperlight that referenced this pull request Mar 17, 2025
…st code to start at 0x0

instead of after paging sections

This is the first step in moving memory sections around to provide a more generic
layout. After this, I'll move continue moving sections to fulfill the layout shown
in the description for PR hyperlight-dev#297.

Changes:
- removed no longer applicable checks in hypervisor_handler
- changed the way we build memory in get_memory_regions
- added functions to get the offsets/addresses to paging sections
as they are no longer static from 0x0 and are now relative to code size
- updated code docs
- changed set_up_shared_memory to properly place paging sections
- updated paging docs

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from 8aa7f89 to f21aabc Compare March 17, 2025 00:46
danbugs added a commit to danbugs/hyperlight that referenced this pull request Mar 17, 2025
…st code to start at 0x0

instead of after paging sections

This is the first step in moving memory sections around to provide a more generic
layout. After this, I'll move continue moving sections to fulfill the layout shown
in the description for PR hyperlight-dev#297.

Changes:
- removed no longer applicable checks in hypervisor_handler
- changed the way we build memory in get_memory_regions
- added functions to get the offsets/addresses to paging sections
as they are no longer static from 0x0 and are now relative to code size
- updated code docs
- changed set_up_shared_memory to properly place paging sections
- updated paging docs

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from f21aabc to 53069f2 Compare March 17, 2025 15:50
danbugs added a commit to danbugs/hyperlight that referenced this pull request Mar 19, 2025
In hyperlight-dev#297, we make the base address of guest memory 0x0 instead of 0x200_000 and
remove statically defined memory regions. This commit updates documentation in
accordance to these changes.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch 2 times, most recently from 4980140 to 3212be7 Compare March 19, 2025 05:31
danbugs added a commit to danbugs/hyperlight that referenced this pull request Mar 19, 2025
In hyperlight-dev#297, we make the base address of guest memory 0x0 instead of 0x200_000 and
remove statically defined memory regions. This commit updates documentation in
accordance to these changes.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from 3212be7 to be061f0 Compare March 19, 2025 05:31
danbugs added a commit to danbugs/hyperlight that referenced this pull request Mar 19, 2025
In hyperlight-dev#297, we make the base address of guest memory 0x0 instead of 0x200_000 and
remove statically defined memory regions. This commit updates documentation in
accordance to these changes.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from be061f0 to 76d67a8 Compare March 19, 2025 15:23
@danbugs danbugs removed the kind/refactor For PRs that restructure or remove code without adding new functionality. label Mar 19, 2025
danbugs added a commit to danbugs/hyperlight that referenced this pull request Apr 11, 2025
In hyperlight-dev#297, we make the base address of guest memory 0x0 instead of 0x200_000 and
remove statically defined memory regions. This commit updates documentation in
accordance to these changes.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch 3 times, most recently from 06dd5e3 to 6e82f64 Compare April 14, 2025 22:49
danbugs added a commit to danbugs/hyperlight that referenced this pull request Apr 14, 2025
In hyperlight-dev#297, we make the base address of guest memory 0x0 instead of 0x200_000 and
remove statically defined memory regions. This commit updates documentation in
accordance to these changes.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch 2 times, most recently from cbf1071 to 63e3c69 Compare April 14, 2025 23:03
danbugs added 4 commits April 21, 2025 16:56
- cleaned up the HyperlightPEB API in the common library.
-- added MemoryRegion struct to better group related offsets and sizes.
-- removed pub fields from HyperlightPEB struct making fields accessible only via getters/setters.
-- cleaned up, commented, and re-organized existing fxns for HyperlightPEB struct.
- now we modify the rsp in the host if the guest sets up a new stack region (i.e., essentially dropping
the tmp stack).

Signed-off-by: danbugs <[email protected]>
- the guest error data region was removed and now, instead, we leverage the output data region to communicate guest errors back to the host.
- previously, the host error data region was incorrectly accessed making it essentially unused. This commit removes the region altogether. There is
some merit to the region existing in some way, but this is outside of the scope of hyperlight-dev#297.

Signed-off-by: danbugs <[email protected]>
- updated our hello-world example,
- updated the README,
- changed last remaining use of HostPrint to use print (wrapper for using the DebugPrint OutbAction), and
- added an extra test in sandbox_builder.

Signed-off-by: danbugs <[email protected]>
…dler + re-added seccomp test

Previously set mem_access_handler_fn to be NOOP. Brought back original functionality;
albeit a bit questionable.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch 4 times, most recently from 3c6255b to 815cb5b Compare April 21, 2025 23:16
@danbugs danbugs force-pushed the danbugs/memory-layout branch 3 times, most recently from 8b04b73 to 3570f4f Compare April 25, 2025 17:58
danbugs added 4 commits April 25, 2025 18:07
Tests are already covered in the sandbox_builder. In the next commit, I also added
a test for Windows in-process mode using the load library.

Signed-off-by: danbugs <[email protected]>
- Changed the ABI for the inprocess outb handler from `extern "C"` to
`extern sysv64`. This was needed because `extern "C"` caused issues on Windows.
Previously, we used `extern "win64"`, which was problematic for guests
intending to use `hyperlight_common` and not supporting the Windows ABI.
- Added a test for running inprocess on Windows w/ load library.

Signed-off-by: danbugs <[email protected]>
…simpleguest] fixed stack setup

(1) PE guests must have the RSP be 16-byte aligned, added stack alignment on init_rsp and re-setup to account for it.
(2) We're now properly updating the min stack address needed for chkstk via the HyperlightPEB.
(3) Added tests for PE guests.

Signed-off-by: danbugs <[email protected]>
- added convenience justfile rule to test like CI (w/ different feature combinations),
- removed executable_heap feature (heap is always executable now).
- fixed check_stack_guard functionality w/ stack cookie (currently broken in main).
- fixed logging to get proper output_data_region address (host vs. guest address space when
in process vs. in hypervisor).
- modified print_output to return the message length.

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from 3570f4f to caa40e3 Compare April 25, 2025 20:23
@danbugs danbugs marked this pull request as ready for review April 25, 2025 20:23
@danbugs danbugs force-pushed the danbugs/memory-layout branch from caa40e3 to 20ba609 Compare April 25, 2025 20:53
Satisfying clippy in release by removing panic/unwrap/expect usages from the new
APIs

Signed-off-by: danbugs <[email protected]>
@danbugs danbugs force-pushed the danbugs/memory-layout branch from 20ba609 to c76215f Compare April 25, 2025 21:05
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good. I haven't looked through everything in great detail, but I have an initial round of feedback

  • Moving things out of hyperlight_guest into hyperlight_common doesn't seem right for features that will never be used from the host side. I think creating another guest crate is a better idea (although I am still curious why this is needed. Maybe a defaut feature flag on hyperlight-guest that enables our panic-handler and other incompatible tings could be another idea), to let consumers turn off default-features, and still have access to the structs and things.
  • SandboxBuilder is a good idea, but it currently replaces both layout.rs and memory_region.rs. I don't think we want a SandboxBuilder to have to interact with memory like this. In my opinion, the sandbox builder should only contain things the user can change, like guestbinary, host funcs, etc, and not memory-regions, rsp, and whatnot. In addition, my thoughts are that users should be able to call UninitializedSandbox::new(guest_binary), but if you require more features, then you can use a builder (or a config!) for that. With these changes, users are forced to use a builder, and don't have access to new anymore, which I'm not sure is quite right.
  • The stack changes look reasonable on a first glance, but haven't thought about it too much
  • Maybe it would be possible to split this PR into a couple smaller ones? Just to make it easier to review

entries. Each entry is set to the value `p << 21|i << 12` where `p` is the page
table number and `i` is the index of the entry in the page table. Thus, the first
entry of the first page table is `0x000_000`, the second entry is `0x000_000 +
0x1000`, and so on. The first entry of the second page table is `0x200_000 +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these mentions of 0x200_000 correct?

Comment on lines +52 to 53

Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you missing something here?


/// Get a return value from a host function call.
/// This usually requires a host function to be called first using `call_host_function`.
pub fn get_host_return_value<T: TryFrom<ReturnValue>>() -> Result<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think hyperlight-common is the right crate for these things, because hyperlight-host would never need this. I think a better alternative is put these in a new crate, that hyperlight-guest can re-export

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

}

/// Pushes shared output data to the output buffer.
pub fn push_shared_output_data(&self, data: Vec<u8>) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly replace Vec with &[u8]?

/// # Safety
/// This function is not thread safe and assumes `outb` is safe to call directly.
#[no_mangle]
pub unsafe extern "C" fn _putchar(c: c_char) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it seems like this is only a guest function so this is here so that when hyperlight_host includes it clippy does not complain

@@ -438,6 +439,21 @@ impl Hypervisor for KVMDriver {
dbg_mem_access_fn,
)?;

// The guest may have chosen a different stack region. If so, we drop usage of our tmp stack.
let mut hyperlight_peb = self.mem_sections.read_hyperlight_peb()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This circumvents our abtraction for shared memory (applies to all drivers). We should re-think this

.shared_mem
.try_pop_buffer_into::<FbGuestError>(odr_buffer as usize, odr_size as usize);

let guest_err = if let Ok(err) = maybe_guest_err {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we circumvent our shared-memory abstraction again by reading the raw PDB from memory, which we should probably not do. In addition, I'm not a big fan of the way we check for errors here by trying to deserializng an error and see if it works or not

use crate::mem::ptr::RawPtr;

#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
pub(crate) struct HyperlightHostError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to get rid of this

/// Initializes the IDTR with the given base address and size.
///
/// # Safety
/// TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 TODO comments here that might want addressing

(*PEB)
.get_guest_panic_context_address()
.expect("Failed to get panic context") as *mut u8,
(*PEB)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think count should be the length of the string, not the length of the buffer. (Previous bug I know!)

/// It's a mode primarily used for debugging and testing.
pub run_mode: RunMode,

/// On Windows, Hyperlight supports in-process execution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a preexisting issue, but perhaps take the opportunity to clarify that this isn't windows-only anymore?

impl HyperlightPEB {
pub fn set_default_memory_layout(&mut self) {
// we set the guest stack at the start of the guest memory region to leverage
// the stack guard page before it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't the guest setting up this guard page by itself?


#[repr(C)]
#[derive(Clone, Default)]
pub struct HyperlightPEB {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that all of these fields should be updated rarely, what's the benefit of placing this structure in shared memory (where it consumes an entire page & host accesses might race with guest accesses) over just having dedicated exits (outb actions) to set the ones that need to be setable?

where
T: for<'a> TryFrom<&'a [u8]>,
{
let input_data_buffer = unsafe { from_raw_parts_mut(self.ptr, self.len) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this constructing its own slice & not using the HostSharedMemory apis to access the shared memory?

This is also missing a safety justification, both for data race UB which I think is a problem with it at present (look at the comments on HostSharedMemory for how we are being careful about that) & just for spatial safety (which would require e.g. marking new as unsafe and documenting the safety invariants on the pointer it takes for example). Using the safe HostSharedMemory apis would fix this.


impl SandboxMemorySections {
pub(crate) fn new() -> Self {
Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of having this dynamic structure?

For the sections that are needed & have accessors below, surely it would be better to have a static structure with fields for each one. For any other sections, why would the host need to concern itself with them at all?

}
}

#[cfg(mshv2)]
Copy link
Contributor

@syntactically syntactically Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there mshv code in this common file & not locked away in the mshv-specific module?

/// let uninitialized_sandbox = sandbox_builder.build().unwrap();
/// ```
pub struct SandboxBuilder {
guest_binary: GuestBinary,
Copy link
Contributor

@syntactically syntactically Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the host should be able to set the guest memory size, but other than that agreed.

/// Default uninitialized sandboxes have the following memory layout:
/// - +--------------------------+
/// - |Custom guest memory (CGM) | (see note 1)
/// - +--------------------------+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of the next 4 sections, as opposed to just initializing rsp at the top of the cgm section and initializing the bottom of the section withe the guest code? Is the guest able to reclaim them after it starts running?

self.load_addr != RawPtr::from(BASE_ADDRESS as u64)
}

fn get_page_flags(p: usize, i: usize, sandbox_memory_sections: &SandboxMemorySections) -> u64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we still have multiple different page flags options as opposed to the pages being truly undifferentiated?

// TODO(see #430) consider how the three functions below fit into the bigger
// picture w/ implementors of this trait having similar fxns.
/// Return data as mut slice
fn as_mut_slice(&mut self) -> &mut [u8] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with ludvig; these don't seem to uphold the safety concerns that prompted splitting up the sharedmemory implementations in the first place.

Copy link
Contributor

@simongdavies simongdavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't quite get to the end of this but quite a few comments

@@ -1,80 +1,103 @@
# Paging in Hyperlight

Hyperlight uses paging, which means the all addresses inside a Hyperlight VM are treated as virtual addresses by the processor. Specifically, Hyperlight uses (ordinary) 4-level paging. 4-level paging is used because we set the following control registers on logical cores inside a VM: `CR0.PG = 1, CR4.PAE = 1, IA32_EFER.LME = 1, and CR4.LA57 = 0`. A Hyperlight VM is limited to 1GB of addressable memory, see below for more details. These control register settings have the following effects:
Hyperlight uses paging, which means the all addresses inside a Hyperlight VM are
treated as virtual addresses by the processor. Specifically, Hyperlight uses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why these line lengths have changed, makes it hard to see what really changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a tool option to format everything to not be longer than x number of chars per line.
I think it is usually useful, but on this particularly example it is harder to see what changed.


/// The guest panic context pointer can be used to pass
/// panic context data from the guest to the host.
pub guest_panic_context_ptr: u64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This question might be answered later but how would an early panic be handled (i.e. before this memory was located/accessible - maybe this cannot happen) or if the pointer or length is zero, any reason why this is not statically defined?

/// The size of the guest memory region.
pub guest_memory_size: u64,

// - Guest configured fields
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we refer to guest configured fields does this mean for hyperlight guests rather than something like Nanvix? If so we should probably make this clear here? Also not clear why this is in hypleright common if this is the case?

@@ -20,6 +20,8 @@ limitations under the License.
// We use Arbitrary during fuzzing, which requires std
#![cfg_attr(not(feature = "fuzzing"), no_std)]

pub const PAGE_SIZE: usize = 0x1_000; // 4KB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we passed page size on inialisation?

// /// in-process mode turned on, or linux), the same container is just
// /// a no-op
// #[cfg(inprocess)]
// pub(crate) mod leaked_outb;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this addressed in a later commit?

@@ -490,6 +488,21 @@ impl Hypervisor for HypervLinuxDriver {
dbg_mem_access_fn,
)?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this code different for each hypervisor or should it be a common function somewhere

@@ -340,6 +338,21 @@ impl Hypervisor for HypervWindowsDriver {
dbg_mem_access_hdl,
)?;

// The guest may have chosen a different stack region. If so, we drop usage of our tmp stack.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks the same as the code in the hyperv linux case

}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did the test go?

limitations under the License.
*/

// /*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more line and block comments


#[allow(clippy::unwrap_used)]
// this is safe because create the custom guest memory section w/ the SandboxBuilder usage
offset.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we using unwrap in here? Applies to all uses. Is there are reason why we would want to crash the host in these cases?

Copy link
Contributor

@dblnz dblnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, @danbugs!
I do have some comments about the changes:

  • I do agree with @ludfjig on the SandboxBuilder, it brings simplicity to the user, but it shouldn't handle the memory.
  • some tests (where feasible) on the added structs would bring a lot of value in the long run
  • Also, I don't know if I missed it, but I think a small guide or example on how one might use this (both host and guest) would be very helpful.

@@ -62,6 +62,22 @@ clean-rust:

# Note: most testing recipes take an optional "features" comma separated list argument. If provided, these will be passed to cargo as **THE ONLY FEATURES**, i.e. default features will be disabled.

# run full CI test matrix
test-like-ci config=default-target hypervisor="kvm":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be somehow achieved with act. Might be worth taking a look when you have some time. I've been using it on other repos and it's quite useful.

@@ -1,80 +1,103 @@
# Paging in Hyperlight

Hyperlight uses paging, which means the all addresses inside a Hyperlight VM are treated as virtual addresses by the processor. Specifically, Hyperlight uses (ordinary) 4-level paging. 4-level paging is used because we set the following control registers on logical cores inside a VM: `CR0.PG = 1, CR4.PAE = 1, IA32_EFER.LME = 1, and CR4.LA57 = 0`. A Hyperlight VM is limited to 1GB of addressable memory, see below for more details. These control register settings have the following effects:
Hyperlight uses paging, which means the all addresses inside a Hyperlight VM are
treated as virtual addresses by the processor. Specifically, Hyperlight uses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a tool option to format everything to not be longer than x number of chars per line.
I think it is usually useful, but on this particularly example it is harder to see what changed.

2. Bits 47:39 of X are used to index into PML4, giving us the address of the PDPT.
3. Bits 38:30 of X are used to index into PDPT, giving us the address of the PD.
4. Bits 29:21 of X are used to index into PD, giving us the address of the PT.
5. Bits 20:12 of X are used to index into PT, giving us a base address of a 4K page.
6. Bits 11:0 of X are treated as an offset.
7. The final physical address is the base address + the offset.

However, because we have only one PDPT4E and only one PDPT4E, bits 47:30 must always be zero. Each PDE points to a PT, and because each PTE with index `p,i` (where p is the page table number of i is the entry within that page) has value `p << 21|i << 12`, the base address received in step 5 above is always just bits 29:12 of X itself. **As bits 11:0 are an offset this means that translating a virtual address to a physical address is essentially a NO-OP**.
However, because we have only one PDPT4E and only one PDPT4E, bits 47:30 must always
be zero. Each PDE points to a PT, and because each PTE with index `p,i` (where p i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text in the parentheses has been wrongly formatted, some words are missing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests to the operations on InputDataSection and OutputDataSection would be nice to avoid regression when changes are added.

@@ -0,0 +1,472 @@
use anyhow::{bail, Result};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a copyright notice is needed here also

@@ -113,7 +98,7 @@ impl HostFuncsWrapper {

fn register_host_function_helper(
self_: &mut HostFuncsWrapper,
mgr: &mut SandboxMemoryManager<ExclusiveSharedMemory>,
_mgr: &mut SandboxMemoryManager<ExclusiveSharedMemory>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is not used can we remove this parameter?

}
fn get_hv_handler(&self) -> &HypervisorHandler {
#[cfg(test)]
pub(crate) fn get_hv_handler(&self) -> &HypervisorHandler {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If this is only used in the test module, it can be moved there.

@@ -29,8 +29,11 @@ use crate::mem::shared_mem::GuestSharedMemory;
///
/// NOTE: This is not part of the C Hyperlight API , it is intended only to be
/// called in proc through a pointer passed to the guest.
extern "win64" fn call_outb(ptr: *mut Arc<Mutex<dyn OutBHandlerCaller>>, port: u16, data: u64) {
let outb_handlercaller = unsafe { Box::from_raw(ptr) };
extern "sysv64" fn call_outb(ptr: *mut core::ffi::c_void, port: u16, data: u64) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there's a change from win64 to sysv64.

@@ -0,0 +1,1308 @@
use std::collections::BTreeMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also have a license notice

.memory_sections
.write_hyperlight_peb(hyperlight_peb)?;

//TODO(danbugs:297): bring back host functions and adding host_printer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover TODO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement For PRs adding features, improving functionality, docs, tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants