Guest error region refactor

src/hyperlight_common/src/mem/mod.rs

@@ -84,7 +84,6 @@ pub struct GuestPanicContextData {
 pub struct HyperlightPEB {
     pub security_cookie_seed: u64,
     pub guest_function_dispatch_ptr: u64,
-    pub guestErrorData: GuestErrorData,
     pub pCode: *mut c_char,
     pub pOutb: *mut c_void,
     pub pOutbContext: *mut c_void,

src/hyperlight_guest/src/entrypoint.rs

@@ -23,7 +23,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::host_function_call::{outb, OutBAction};
@@ -146,8 +145,6 @@ pub extern "win64" fn entrypoint(peb_address: u64, seed: u64, ops: u64, max_log_
 
             (*peb_ptr).guest_function_dispatch_ptr = dispatch_function as usize as u64;
 
-            reset_error();
-
             hyperlight_main();
         }
     });

src/hyperlight_guest/src/guest_error.rs

@@ -14,73 +14,27 @@ 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 log::error;
 
 use crate::entrypoint::halt;
 use crate::host_function_call::{outb, OutBAction};
-use crate::P_PEB;
+use crate::shared_output_data::push_shared_output_data;
 
 pub(crate) fn write_error(error_code: ErrorCode, message: Option<&str>) {
     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!(!(*P_PEB.unwrap()).guestErrorData.guestErrorBuffer.is_null());
-        let len = guest_error_buffer.len();
-        if guest_error_buffer.len() > (*P_PEB.unwrap()).guestErrorData.guestErrorSize 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(),
-                (*P_PEB.unwrap()).guestErrorData.guestErrorSize 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()
-                    - (*P_PEB.unwrap()).guestErrorData.guestErrorSize 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 = (*P_PEB.unwrap()).guestErrorData.guestErrorBuffer as *mut u8;
-        core::ptr::copy_nonoverlapping(guest_error_buffer.as_ptr(), dest_ptr, len);
-    }
-}
-
-pub(crate) fn reset_error() {
-    unsafe {
-        let peb_ptr = P_PEB.unwrap();
-        core::ptr::write_bytes(
-            (*peb_ptr).guestErrorData.guestErrorBuffer,
-            0,
-            (*peb_ptr).guestErrorData.guestErrorSize as usize,
-        );
-    }
+    push_shared_output_data(guest_error_buffer)
+        .expect("Unable to push guest error to shared output data");
 }
 
 pub(crate) fn set_error(error_code: ErrorCode, message: &str) {

src/hyperlight_guest/src/guest_function_call.rs

@@ -23,7 +23,7 @@ use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode;
 
 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::shared_input_data::try_pop_shared_input_data_into;
 use crate::shared_output_data::push_shared_output_data;
 use crate::REGISTERED_GUEST_FUNCTIONS;
@@ -81,8 +81,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_host/src/func/guest_err.rs

@@ -21,18 +21,27 @@ use crate::mem::shared_mem::HostSharedMemory;
 use crate::metrics::{METRIC_GUEST_ERROR, METRIC_GUEST_ERROR_LABEL_CODE};
 use crate::sandbox::mem_mgr::MemMgrWrapper;
 use crate::{log_then_return, Result};
+
 /// Check for a guest error and return an `Err` if one was found,
 /// and `Ok` if one was not found.
-pub(crate) fn check_for_guest_error(mgr: &MemMgrWrapper<HostSharedMemory>) -> Result<()> {
-    let guest_err = mgr.as_ref().get_guest_error()?;
+pub(crate) fn check_for_guest_error(mgr: &mut MemMgrWrapper<HostSharedMemory>) -> Result<()> {
+    let guest_err = mgr.as_mut().get_guest_error().ok();
+    let Some(guest_err) = guest_err else {
+        return Ok(());
+    };
+
+    metrics::counter!(
+        METRIC_GUEST_ERROR,
+        METRIC_GUEST_ERROR_LABEL_CODE => (guest_err.code as u64).to_string()
+    )
+    .increment(1);
+
     match guest_err.code {
         ErrorCode::NoError => Ok(()),
         ErrorCode::StackOverflow => {
-            metrics::counter!(METRIC_GUEST_ERROR, METRIC_GUEST_ERROR_LABEL_CODE => (guest_err.code as u64).to_string()).increment(1);
             log_then_return!(StackOverflow());
         }
         _ => {
-            metrics::counter!(METRIC_GUEST_ERROR, METRIC_GUEST_ERROR_LABEL_CODE => (guest_err.code as u64).to_string()).increment(1);
             log_then_return!(GuestError(guest_err.code, guest_err.message.clone()));
         }
     }

src/hyperlight_host/src/mem/layout.rs

@@ -22,8 +22,7 @@ use rand::{rng, RngCore};
 use tracing::{instrument, Span};
 
 use super::memory_region::MemoryRegionType::{
-    Code, GuardPage, GuestErrorData, Heap, InputData, OutputData, PageTables, PanicContext, Peb,
-    Stack,
+    Code, GuardPage, Heap, InputData, OutputData, PageTables, PanicContext, Peb, Stack,
 };
 use super::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionVecBuilder};
 use super::mgr::AMOUNT_OF_MEMORY_PER_PT;
@@ -45,8 +44,6 @@ use crate::{log_then_return, new_error, Result};
 // +-------------------------------------------+
 // |              Input Data                   |
 // +-------------------------------------------+
-// |           Guest Error Log                 |
-// +-------------------------------------------+
 // |                PEB Struct                 | (HyperlightPEB size)
 // +-------------------------------------------+
 // |               Guest Code                  |
@@ -60,9 +57,6 @@ use crate::{log_then_return, new_error, Result};
 // |                   PML4                    |
 // +-------------------------------------------+ 0x0_000
 
-/// - `GuestError` - contains a buffer for any guest error that occurred.
-///   the length of this field is `GuestErrorBufferSize` from `SandboxConfiguration`
-///
 /// - `InputData` -  this is a buffer that is used for input data to the host program.
 ///   the length of this field is `InputDataSize` from `SandboxConfiguration`
 ///
@@ -95,7 +89,6 @@ pub(crate) struct SandboxMemoryLayout {
     peb_offset: usize,
     peb_security_cookie_seed_offset: usize,
     peb_guest_dispatch_function_ptr_offset: usize, // set by guest in guest entrypoint
-    peb_guest_error_offset: usize,
     peb_code_and_outb_pointer_offset: usize,
     peb_runmode_offset: usize,
     peb_input_data_offset: usize,
@@ -106,7 +99,6 @@ pub(crate) struct SandboxMemoryLayout {
 
     // The following are the actual values
     // that are written to the PEB struct
-    pub(super) guest_error_buffer_offset: usize,
     pub(super) input_data_buffer_offset: usize,
     pub(super) output_data_buffer_offset: usize,
     guest_panic_context_buffer_offset: usize,
@@ -143,10 +135,6 @@ impl Debug for SandboxMemoryLayout {
                 "Guest Dispatch Function Pointer Offset",
                 &format_args!("{:#x}", self.peb_guest_dispatch_function_ptr_offset),
             )
-            .field(
-                "Guest Error Offset",
-                &format_args!("{:#x}", self.peb_guest_error_offset),
-            )
             .field(
                 "Code and OutB Pointer Offset",
                 &format_args!("{:#x}", self.peb_code_and_outb_pointer_offset),
@@ -171,10 +159,6 @@ impl Debug for SandboxMemoryLayout {
                 "Guest Stack Offset",
                 &format_args!("{:#x}", self.peb_guest_stack_data_offset),
             )
-            .field(
-                "Guest Error Buffer Offset",
-                &format_args!("{:#x}", self.guest_error_buffer_offset),
-            )
             .field(
                 "Input Data Buffer Offset",
                 &format_args!("{:#x}", self.input_data_buffer_offset),
@@ -259,7 +243,6 @@ impl SandboxMemoryLayout {
             peb_offset + offset_of!(HyperlightPEB, security_cookie_seed);
         let peb_guest_dispatch_function_ptr_offset =
             peb_offset + offset_of!(HyperlightPEB, guest_function_dispatch_ptr);
-        let peb_guest_error_offset = peb_offset + offset_of!(HyperlightPEB, guestErrorData);
         let peb_code_and_outb_pointer_offset = peb_offset + offset_of!(HyperlightPEB, pCode);
         let peb_runmode_offset = peb_offset + offset_of!(HyperlightPEB, runMode);
         let peb_input_data_offset = peb_offset + offset_of!(HyperlightPEB, inputdata);
@@ -272,12 +255,8 @@ impl SandboxMemoryLayout {
         // The following offsets are the actual values that relate to memory layout,
         // which are written to PEB struct
         let peb_address = Self::BASE_ADDRESS + peb_offset;
-        let guest_error_buffer_offset = round_up_to(
-            peb_guest_stack_data_offset + size_of::<GuestStackData>(),
-            PAGE_SIZE_USIZE,
-        );
         let input_data_buffer_offset = round_up_to(
-            guest_error_buffer_offset + cfg.get_guest_error_buffer_size(),
+            peb_guest_stack_data_offset + size_of::<GuestStackData>(),
             PAGE_SIZE_USIZE,
         );
         let output_data_buffer_offset = round_up_to(
@@ -305,15 +284,13 @@ impl SandboxMemoryLayout {
             heap_size,
             peb_security_cookie_seed_offset,
             peb_guest_dispatch_function_ptr_offset,
-            peb_guest_error_offset,
             peb_code_and_outb_pointer_offset,
             peb_runmode_offset,
             peb_input_data_offset,
             peb_output_data_offset,
             peb_guest_panic_context_offset,
             peb_heap_data_offset,
             peb_guest_stack_data_offset,
-            guest_error_buffer_offset,
             sandbox_memory_config: cfg,
             code_size,
             input_data_buffer_offset,
@@ -333,18 +310,6 @@ impl SandboxMemoryLayout {
         self.peb_runmode_offset
     }
 
-    /// Get the offset in guest memory to the max size of the guest error buffer
-    #[instrument(skip_all, parent = Span::current(), level= "Trace")]
-    pub(super) fn get_guest_error_buffer_size_offset(&self) -> usize {
-        self.peb_guest_error_offset
-    }
-
-    /// Get the offset in guest memory to the error message buffer pointer
-    #[instrument(skip_all, parent = Span::current(), level= "Trace")]
-    fn get_guest_error_buffer_pointer_offset(&self) -> usize {
-        self.peb_guest_error_offset + size_of::<u64>()
-    }
-
     /// Get the offset in guest memory to the output data size
     #[instrument(skip_all, parent = Span::current(), level= "Trace")]
     pub(super) fn get_output_data_size_offset(&self) -> usize {
@@ -542,7 +507,6 @@ impl SandboxMemoryLayout {
         let mut total_mapped_memory_size: usize = round_up_to(code_size, PAGE_SIZE_USIZE);
         total_mapped_memory_size += round_up_to(stack_size, PAGE_SIZE_USIZE);
         total_mapped_memory_size += round_up_to(heap_size, PAGE_SIZE_USIZE);
-        total_mapped_memory_size += round_up_to(cfg.get_guest_error_buffer_size(), PAGE_SIZE_USIZE);
         total_mapped_memory_size += round_up_to(cfg.get_input_data_size(), PAGE_SIZE_USIZE);
         total_mapped_memory_size += round_up_to(cfg.get_output_data_size(), PAGE_SIZE_USIZE);
         total_mapped_memory_size +=
@@ -626,30 +590,12 @@ impl SandboxMemoryLayout {
         }
 
         // PEB
-        let guest_error_offset = builder.push_page_aligned(
+        let input_data_offset = builder.push_page_aligned(
             size_of::<HyperlightPEB>(),
             MemoryRegionFlags::READ | MemoryRegionFlags::WRITE,
             Peb,
         );
 
-        let expected_guest_error_offset =
-            TryInto::<usize>::try_into(self.guest_error_buffer_offset)?;
-
-        if guest_error_offset != expected_guest_error_offset {
-            return Err(new_error!(
-                "Guest error offset does not match expected Guest error offset expected:  {}, actual:  {}",
-                expected_guest_error_offset,
-                guest_error_offset
-            ));
-        }
-
-        // guest error
-        let input_data_offset = builder.push_page_aligned(
-            self.sandbox_memory_config.get_guest_error_buffer_size(),
-            MemoryRegionFlags::READ | MemoryRegionFlags::WRITE,
-            GuestErrorData,
-        );
-
         let expected_input_data_offset = TryInto::<usize>::try_into(self.input_data_buffer_offset)?;
 
         if input_data_offset != expected_input_data_offset {
@@ -818,14 +764,6 @@ impl SandboxMemoryLayout {
 
         // Skip guest_dispatch_function_ptr_offset because it is set by the guest
 
-        // Set up Guest Error Fields
-        let addr = get_address!(guest_error_buffer);
-        shared_mem.write_u64(self.get_guest_error_buffer_pointer_offset(), addr)?;
-        shared_mem.write_u64(
-            self.get_guest_error_buffer_size_offset(),
-            u64::try_from(self.sandbox_memory_config.get_guest_error_buffer_size())?,
-        )?;
-
         // Skip code, is set when loading binary
         // skip outb and outb context, is set when running in_proc
 
@@ -958,8 +896,6 @@ mod tests {
 
         expected_size += round_up_to(size_of::<HyperlightPEB>(), PAGE_SIZE_USIZE);
 
-        expected_size += round_up_to(cfg.get_guest_error_buffer_size(), PAGE_SIZE_USIZE);
-
         expected_size += round_up_to(cfg.get_input_data_size(), PAGE_SIZE_USIZE);
 
         expected_size += round_up_to(cfg.get_output_data_size(), PAGE_SIZE_USIZE);

src/hyperlight_host/src/mem/memory_region.rs

@@ -131,8 +131,6 @@ pub enum MemoryRegionType {
     Code,
     /// The region contains the PEB
     Peb,
-    /// The region contains the Guest Error Data
-    GuestErrorData,
     /// The region contains the Input Data
     InputData,
     /// The region contains the Output Data

src/hyperlight_host/src/mem/mgr.rs

@@ -199,7 +199,6 @@ where
                             MemoryRegionType::OutputData => PAGE_PRESENT | PAGE_RW | PAGE_NX,
                             MemoryRegionType::Peb => PAGE_PRESENT | PAGE_RW | PAGE_NX,
                             MemoryRegionType::PanicContext => PAGE_PRESENT | PAGE_RW | PAGE_NX,
-                            MemoryRegionType::GuestErrorData => PAGE_PRESENT | PAGE_RW | PAGE_NX,
                             MemoryRegionType::PageTables => PAGE_PRESENT | PAGE_RW | PAGE_NX,
                         },
                         // If there is an error then the address isn't mapped so mark it as not present
@@ -591,22 +590,11 @@ impl SandboxMemoryManager<HostSharedMemory> {
 
     /// Get the guest error data
     #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")]
-    pub(crate) fn get_guest_error(&self) -> Result<GuestError> {
-        // get memory buffer max size
-        let err_buffer_size_offset = self.layout.get_guest_error_buffer_size_offset();
-        let max_err_buffer_size = self.shared_mem.read::<u64>(err_buffer_size_offset)?;
-
-        // get guest error from layout and shared mem
-        let mut guest_error_buffer = vec![b'0'; usize::try_from(max_err_buffer_size)?];
-        let err_msg_offset = self.layout.guest_error_buffer_offset;
-        self.shared_mem
-            .copy_to_slice(guest_error_buffer.as_mut_slice(), err_msg_offset)?;
-        GuestError::try_from(guest_error_buffer.as_slice()).map_err(|e| {
-            new_error!(
-                "get_guest_error: failed to convert buffer to GuestError: {}",
-                e
-            )
-        })
+    pub(crate) fn get_guest_error(&mut self) -> Result<GuestError> {
+        self.shared_mem.try_pop_buffer_into::<GuestError>(
+            self.layout.output_data_buffer_offset,
+            self.layout.sandbox_memory_config.get_output_data_size(),
+        )
     }
 
     /// Read guest panic data from the `SharedMemory` contained within `self`