[common,guest,host] refactored guest and host error data regions

- 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 #297.

Signed-off-by: danbugs <[email protected]>

Author: danbugs

src/hyperlight_common/src/input_output.rs

@@ -18,16 +18,20 @@ use core::slice::from_raw_parts_mut;
 
 use anyhow::{bail, Result};
 
+/// The host will use the `InputDataSection` to pass data to the guest. This can be, for example,
+/// the issuing of a function call or the result of calling a host function.
 pub struct InputDataSection {
     ptr: *mut u8,
     len: u64,
 }
 
 impl InputDataSection {
+    /// Creates a new `InputDataSection` with the given pointer and length.
     pub fn new(ptr: *mut u8, len: u64) -> Self {
         InputDataSection { ptr, len }
     }
 
+    /// Tries to pop shared input data into a type `T`. The type `T` must implement the `TryFrom` trait
     pub fn try_pop_shared_input_data_into<T>(&self) -> Result<T>
     where
         T: for<'a> TryFrom<&'a [u8]>,
@@ -79,6 +83,8 @@ impl InputDataSection {
     }
 }
 
+/// The guest will use the `OutputDataSection` to pass data back to the host. This can be, for example,
+/// issuing a host function call or the result of a guest function call.
 pub struct OutputDataSection {
     pub ptr: *mut u8,
     pub len: u64,
@@ -87,10 +93,12 @@ pub struct OutputDataSection {
 impl OutputDataSection {
     const STACK_PTR_SIZE: usize = size_of::<u64>();
 
+    /// Creates a new `OutputDataSection` with the given pointer and length.
     pub fn new(ptr: *mut u8, len: u64) -> Self {
         OutputDataSection { ptr, len }
     }
 
+    /// Pushes shared output data to the output buffer.
     pub fn push_shared_output_data(&self, data: Vec<u8>) -> Result<()> {
         let shared_buffer_size = self.len as usize;
         let odb: &mut [u8] = unsafe { from_raw_parts_mut(self.ptr, shared_buffer_size) };

src/hyperlight_common/src/lib.rs

@@ -25,8 +25,6 @@ pub const PAGE_SIZE: usize = 0x1_000; // 4KB
 extern crate alloc;
 extern crate core;
 
-use crate::peb::{HyperlightPEB, RunMode};
-
 pub mod flatbuffer_wrappers;
 /// cbindgen:ignore
 /// FlatBuffers-related utilities and (mostly) generated code
@@ -48,11 +46,11 @@ pub mod peb;
 
 /// We keep track of the PEB address in a global variable that references a region of
 /// shared memory.
-pub static mut PEB: *mut HyperlightPEB = core::ptr::null_mut();
+pub static mut PEB: *mut peb::HyperlightPEB = core::ptr::null_mut();
 
 /// Hyperlight supports running in both hypervisor mode and in process mode. We keep track of that
 /// state in this global variable.
-pub static mut RUNNING_MODE: RunMode = RunMode::None;
+pub static mut RUNNING_MODE: peb::RunMode = peb::RunMode::None;
 
 /// For in-process mode, we can't call the `outb` instruction directly because it is a privileged
 /// instruction. Instead, we use a function pointer to call an `outb_handler` function.

src/hyperlight_common/src/peb.rs

@@ -61,14 +61,6 @@ pub struct HyperlightPEB {
     /// before re-entering the guest.
     guest_function_dispatch_ptr: u64,
 
-    /// Guest error data can be used to pass guest error information
-    /// between host and the guest.
-    guest_error_data: Option<MemoryRegion>,
-
-    /// Host error data can be used to pass host error information
-    /// between the host and the guest.
-    host_error_data: Option<MemoryRegion>,
-
     /// The input data pointer is used to pass data from
     /// the host to the guest.
     input_data: Option<MemoryRegion>,
@@ -109,8 +101,6 @@ impl HyperlightPEB {
             guest_memory_base_address,
             guest_memory_size,
             guest_function_dispatch_ptr: 0,
-            guest_error_data: None,
-            host_error_data: None,
             input_data: None,
             output_data: None,
             guest_panic_context: None,
@@ -134,10 +124,6 @@ impl HyperlightPEB {
     /// - +--------------------------+
     /// - | Input data               | 16KB
     /// - +--------------------------+
-    /// - | Host error data          | 4KB
-    /// - +--------------------------+
-    /// - | Guest error data         | 4KB
-    /// - +--------------------------+
     /// - | Guest heap data          | (configurable size)
     /// - +--------------------------+
     /// - | Guest stack data         | (configurable size)
@@ -159,29 +145,19 @@ impl HyperlightPEB {
 
         let guest_heap_size = self.get_guest_heap_data_size();
 
-        self.set_guest_error_data_region(
-            guest_stack_size + guest_heap_size, // start at the end of the host function details
-            PAGE_SIZE as u64,                   // 4KB
-        );
-
-        self.set_host_error_data_region(
-            guest_stack_size + guest_heap_size + PAGE_SIZE as u64, // start at the end of the guest error data
-            PAGE_SIZE as u64,                                      // 4KB
-        );
-
         self.set_input_data_region(
-            guest_stack_size + guest_heap_size + PAGE_SIZE as u64 * 2, // start at the end of the host error data
-            PAGE_SIZE as u64 * 4,                                      // 16KB
+            guest_stack_size + guest_heap_size, // start at the end of the heap
+            PAGE_SIZE as u64 * 4,               // 16KB
         );
 
         self.set_output_data_region(
-            guest_stack_size + guest_heap_size + PAGE_SIZE as u64 * 6, // start at the end of the input data
+            guest_stack_size + guest_heap_size + PAGE_SIZE as u64 * 4, // start at the end of the input data
             PAGE_SIZE as u64 * 4,                                      // 16KB
         );
 
         self.set_guest_panic_context_region(
-            guest_stack_size + guest_heap_size + PAGE_SIZE as u64 * 10, // start at the end of the output data
-            PAGE_SIZE as u64,                                           // 4KB
+            guest_stack_size + guest_heap_size + PAGE_SIZE as u64 * 8, // start at the end of the output data
+            PAGE_SIZE as u64,                                          // 4KB
         );
     }
 
@@ -291,57 +267,6 @@ impl HyperlightPEB {
             .size
     }
 
-    /// Sets the guest error data region.
-    pub fn set_guest_error_data_region(&mut self, offset: u64, size: u64) {
-        self.guest_error_data = Some(MemoryRegion {
-            offset: Some(offset),
-            size,
-        });
-    }
-
-    /// Gets the guest error data region depending on the running mode (i.e., if `RunMode::Hypervisor` this
-    /// returns the same as `get_guest_error_guest_address`. If `RunMode::InProcessWindows` or `RunMode::InProcessLinux`, it
-    /// returns the error data host address).
-    pub fn get_guest_error_data_address(&self) -> u64 {
-        let region = self
-            .guest_error_data
-            .as_ref()
-            .expect("Guest error data region not set");
-        match self.run_mode {
-            RunMode::Hypervisor => self.get_guest_error_guest_address(),
-            RunMode::InProcessWindows | RunMode::InProcessLinux => {
-                region.offset.unwrap() + self.guest_memory_host_base_address
-            }
-            _ => panic!("Invalid running mode"),
-        }
-    }
-
-    /// Gets the guest error data region with guest addresses.
-    pub fn get_guest_error_guest_address(&self) -> u64 {
-        self.guest_error_data
-            .as_ref()
-            .expect("Guest error data region not set")
-            .offset
-            .unwrap()
-            + self.guest_memory_base_address
-    }
-
-    /// Gets the guest error data size.
-    pub fn get_guest_error_data_size(&self) -> u64 {
-        self.guest_error_data
-            .as_ref()
-            .expect("Guest error data region not set")
-            .size
-    }
-
-    /// Sets the host error data region.
-    pub fn set_host_error_data_region(&mut self, offset: u64, size: u64) {
-        self.host_error_data = Some(MemoryRegion {
-            offset: Some(offset),
-            size,
-        });
-    }
-
     /// Sets the input data region.
     pub fn set_input_data_region(&mut self, offset: u64, size: u64) {
         self.input_data = Some(MemoryRegion {
@@ -350,24 +275,6 @@ impl HyperlightPEB {
         });
     }
 
-    /// Gets the host error data region with guest addresses.
-    pub fn get_host_error_guest_address(&self) -> u64 {
-        self.host_error_data
-            .as_ref()
-            .expect("Host error data region not set")
-            .offset
-            .unwrap()
-            + self.guest_memory_base_address
-    }
-
-    /// Gets the host error data size.
-    pub fn get_host_error_data_size(&self) -> u64 {
-        self.host_error_data
-            .as_ref()
-            .expect("Host error data region not set")
-            .size
-    }
-
     /// Gets the input data region with guest addresses.
     pub fn get_input_data_guest_region(&self) -> (u64, u64) {
         let region = self.input_data.as_ref().expect("Input data region not set");

src/hyperlight_guest/src/entrypoint.rs

@@ -24,7 +24,6 @@ use log::LevelFilter;
 use spin::Once;
 
 use crate::gdt::load_gdt;
-use crate::guest_error::reset_error;
 use crate::guest_function_call::dispatch_function;
 use crate::guest_logger::init_logger;
 use crate::idtr::load_idt;
@@ -142,8 +141,6 @@ pub extern "win64" fn entrypoint(peb_address: u64, seed: u64, max_log_level: u64
             _ => panic!("Invalid runmode in PEB"),
         }
 
-        reset_error();
-
         hyperlight_main();
     });
 

src/hyperlight_guest/src/guest_error.rs

@@ -14,72 +14,33 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-use alloc::string::{String, ToString};
+use alloc::string::ToString;
 use alloc::vec::Vec;
 use core::ffi::{c_char, CStr};
 
 use hyperlight_common::flatbuffer_wrappers::guest_error::{ErrorCode, GuestError};
+use hyperlight_common::input_output::OutputDataSection;
 use hyperlight_common::outb::{outb, OutBAction};
 use hyperlight_common::PEB;
-use log::error;
 
 use crate::entrypoint::halt;
 
 pub(crate) fn write_error(error_code: ErrorCode, message: Option<&str>) {
     let peb = unsafe { (*PEB).clone() };
+    let output_data: OutputDataSection = peb.get_output_data_region().into();
+
     let guest_error = GuestError::new(
         error_code.clone(),
         message.map_or("".to_string(), |m| m.to_string()),
     );
-    let mut guest_error_buffer: Vec<u8> = (&guest_error)
+
+    let guest_error_buffer: Vec<u8> = (&guest_error)
         .try_into()
         .expect("Invalid guest_error_buffer, could not be converted to a Vec<u8>");
 
-    unsafe {
-        assert_ne!(!peb.get_guest_error_data_address(), 0);
-        let len = guest_error_buffer.len();
-        if guest_error_buffer.len() > peb.get_guest_error_data_size() as usize {
-            error!(
-                "Guest error buffer is too small to hold the error message: size {} buffer size {} message may be truncated",
-                guest_error_buffer.len(),
-                peb.get_guest_error_data_size() as usize
-            );
-            // get the length of the message
-            let message_len = message.map_or("".to_string(), |m| m.to_string()).len();
-            // message is too long, truncate it
-            let truncate_len =
-                message_len - (guest_error_buffer.len() - peb.get_guest_error_data_size() as usize);
-            let truncated_message = message
-                .map_or("".to_string(), |m| m.to_string())
-                .chars()
-                .take(truncate_len)
-                .collect::<String>();
-            let guest_error = GuestError::new(error_code, truncated_message);
-            guest_error_buffer = (&guest_error)
-                .try_into()
-                .expect("Invalid guest_error_buffer, could not be converted to a Vec<u8>");
-        }
-
-        // Optimally, we'd use copy_from_slice here, but, because
-        // p_guest_error_buffer is a *mut c_void, we can't do that.
-        // Instead, we do the copying manually using pointer arithmetic.
-        // Plus; before, we'd do an assert w/ the result from copy_from_slice,
-        // but, because copy_nonoverlapping doesn't return anything, we can't do that.
-        // Instead, we do the prior asserts/checks to check the destination pointer isn't null
-        // and that there is enough space in the destination buffer for the copy.
-        let dest_ptr = peb.get_guest_error_data_address() as *mut u8;
-        core::ptr::copy_nonoverlapping(guest_error_buffer.as_ptr(), dest_ptr, len);
-    }
-}
-
-pub(crate) fn reset_error() {
-    unsafe {
-        core::ptr::write_bytes(
-            (*PEB).get_guest_error_data_address() as *mut u8,
-            0,
-            (*PEB).get_guest_error_data_size() as usize,
-        );
-    }
+    output_data
+        .push_shared_output_data(guest_error_buffer)
+        .expect("Failed to push shared output data");
 }
 
 pub(crate) fn set_error(error_code: ErrorCode, message: &str) {

src/hyperlight_guest/src/guest_function_call.rs

@@ -24,7 +24,7 @@ use hyperlight_common::input_output::{InputDataSection, OutputDataSection};
 
 use crate::entrypoint::halt;
 use crate::error::{HyperlightGuestError, Result};
-use crate::guest_error::{reset_error, set_error};
+use crate::guest_error::set_error;
 use crate::{PEB, REGISTERED_GUEST_FUNCTIONS};
 
 type GuestFunc = fn(&FunctionCall) -> Result<Vec<u8>>;
@@ -80,8 +80,6 @@ pub(crate) fn call_guest_function(function_call: FunctionCall) -> Result<Vec<u8>
 #[no_mangle]
 #[inline(never)]
 fn internal_dispatch_function() -> Result<()> {
-    reset_error();
-
     #[cfg(debug_assertions)]
     log::trace!("internal_dispatch_function");
 

src/hyperlight_guest/src/host_error.rs

@@ -34,8 +34,6 @@ pub mod guest_error;
 pub mod guest_function_call;
 pub mod guest_function_definition;
 pub mod guest_function_register;
-// TODO(danbugs:297): bring back
-// pub mod host_error;
 
 pub(crate) mod guest_logger;
 pub mod memory;