diff --git a/Cargo.lock b/Cargo.lock index c7c40c1..d496b0c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -269,6 +269,7 @@ dependencies = [ "percore", "smccc", "spin 0.12.0", + "thiserror", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index beff62e..9d3e6c7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ memory_access = { path = "memory_access" } percore = "0.2" smccc = "0.2" spin = { version = "0.12", features = ["lazy", "once", "spin_mutex"], default-features = false } +thiserror = { version = "2.0", default-features = false } [lints.rust] deprecated-safe = "warn" diff --git a/memory_access/src/lib.rs b/memory_access/src/lib.rs index 091d0ed..0465f0b 100644 --- a/memory_access/src/lib.rs +++ b/memory_access/src/lib.rs @@ -45,14 +45,17 @@ impl MemoryAccessWidth { } } -/// Decoded representation of a trapped guest memory access. -pub struct DecodedMemoryAccess { +/// Guest memory access decoded from the instruction syndrome that triggered an exception. +/// +/// Exception handlers use this to emulate the faulting load or store instruction without decoding +/// the instruction bytes themselves. +pub struct MemoryAccess { /// The faulting intermediate physical address. pub ipa: u64, /// The width of the guest access. pub width: MemoryAccessWidth, /// Whether the access was a read or write. - pub kind: DecodedMemoryAccessKind, + pub kind: MemoryAccessKind, /// The general-purpose register encoded in the syndrome. pub register_index: usize, /// Whether the read result should be sign-extended. @@ -61,9 +64,47 @@ pub struct DecodedMemoryAccess { pub register_width_64: bool, } +impl MemoryAccess { + /// Decodes a Data Abort instruction syndrome into a guest memory access. + /// + /// Returns `None` when the syndrome does not include enough information to emulate the access + /// or when a write access needs a saved register value that `read_register` cannot provide. + pub fn decode( + iss: u32, + hpfar_fipa: u64, + far_va: u64, + mut read_register: impl FnMut(usize) -> Option, + ) -> Option { + // Keep emulation syndrome-only: without ISV, ISS does not describe a GPR transfer well + // enough to handle the abort without decoding the trapped instruction. + if !decode_valid_instruction_syndrome(iss) { + return None; + } + + let width = decode_memory_access_width(iss); + let register_index = decode_memory_access_register_index(iss); + let kind = decode_memory_access_kind(iss, register_index, width, &mut read_register)?; + + Some(Self { + ipa: decode_fault_ipa(hpfar_fipa, far_va), + width, + kind, + register_index, + sign_extend: decode_memory_access_sign_extend(iss), + register_width_64: decode_memory_access_register_width_64(iss), + }) + } + + /// Extends an emulated read value according to the decoded access and target register width. + #[must_use] + pub fn extend_read_result(&self, value: u64) -> u64 { + extend_read_result(value, self.width, self.sign_extend, self.register_width_64) + } +} + /// Decoded read or write direction for a trapped memory access. #[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub enum DecodedMemoryAccessKind { +pub enum MemoryAccessKind { /// Guest read from memory into a register. Read, /// Guest write from a register to memory. @@ -95,39 +136,8 @@ const DATA_ABORT_ISS_WNR: u32 = 1 << 6; const FAULT_IPA_PAGE_SHIFT: u64 = 12; const FAULT_IPA_PAGE_OFFSET_MASK: u64 = (1 << FAULT_IPA_PAGE_SHIFT) - 1; -/// Decodes a Data Abort instruction syndrome into a guest memory access. -/// -/// Returns `None` when the syndrome does not include enough information to emulate the access or -/// when a write access needs a saved register value that `read_register` cannot provide. -pub fn decode_memory_access( - iss: u32, - hpfar_fipa: u64, - far_va: u64, - mut read_register: impl FnMut(usize) -> Option, -) -> Option { - // Keep emulation syndrome-only: without ISV, ISS does not describe a GPR transfer well - // enough to handle the abort without decoding the trapped instruction. - if !decode_valid_instruction_syndrome(iss) { - return None; - } - - let width = decode_memory_access_width(iss); - let register_index = decode_memory_access_register_index(iss); - let kind = decode_memory_access_kind(iss, register_index, width, &mut read_register)?; - - Some(DecodedMemoryAccess { - ipa: decode_fault_ipa(hpfar_fipa, far_va), - width, - kind, - register_index, - sign_extend: decode_memory_access_sign_extend(iss), - register_width_64: decode_memory_access_register_width_64(iss), - }) -} - -/// Extends an emulated read value according to the decoded access and target register width. #[must_use] -pub fn extend_read_result( +fn extend_read_result( value: u64, width: MemoryAccessWidth, sign_extend: bool, @@ -171,12 +181,12 @@ fn decode_memory_access_kind( register_index: usize, width: MemoryAccessWidth, read_register: &mut impl FnMut(usize) -> Option, -) -> Option { +) -> Option { if decode_memory_access_is_write(iss) { let value = read_register(register_index)? & width.mask(); - Some(DecodedMemoryAccessKind::Write { value }) + Some(MemoryAccessKind::Write { value }) } else { - Some(DecodedMemoryAccessKind::Read) + Some(MemoryAccessKind::Read) } } @@ -213,21 +223,36 @@ mod tests { | flags } - fn decode_with_register(iss: u32, register_value: u64) -> Option { - decode_memory_access(iss, 0x12345, 0xffff_0000_0000_0abc, |index| { + fn decode_with_register(iss: u32, register_value: u64) -> Option { + MemoryAccess::decode(iss, 0x12345, 0xffff_0000_0000_0abc, |index| { assert_eq!(index, 7); Some(register_value) }) } + fn read_access( + width: MemoryAccessWidth, + sign_extend: bool, + register_width_64: bool, + ) -> MemoryAccess { + MemoryAccess { + ipa: 0, + width, + kind: MemoryAccessKind::Read, + register_index: 0, + sign_extend, + register_width_64, + } + } + #[test] fn decode_rejects_missing_instruction_syndrome() { - assert!(decode_memory_access(0, 0x12345, 0xabc, |_| Some(0)).is_none()); + assert!(MemoryAccess::decode(0, 0x12345, 0xabc, |_| Some(0)).is_none()); } #[test] fn decode_read_access_from_syndrome_fields() { - let access = decode_memory_access( + let access = MemoryAccess::decode( iss( MemoryAccessWidth::U32, 9, @@ -241,7 +266,7 @@ mod tests { assert_eq!(access.ipa, 0x1234_5abc); assert_eq!(access.width, MemoryAccessWidth::U32); - assert_eq!(access.kind, DecodedMemoryAccessKind::Read); + assert_eq!(access.kind, MemoryAccessKind::Read); assert_eq!(access.register_index, 9); assert!(access.sign_extend); assert!(access.register_width_64); @@ -255,10 +280,7 @@ mod tests { ) .expect("access should decode"); - assert_eq!( - access.kind, - DecodedMemoryAccessKind::Write { value: 0x1234 } - ); + assert_eq!(access.kind, MemoryAccessKind::Write { value: 0x1234 }); assert_eq!(access.width, MemoryAccessWidth::U16); assert_eq!(access.register_index, 7); } @@ -266,7 +288,7 @@ mod tests { #[test] fn decode_write_access_rejects_unavailable_register_value() { assert!( - decode_memory_access( + MemoryAccess::decode( iss(MemoryAccessWidth::U64, 4, DATA_ABORT_ISS_WNR), 0x12345, 0xabc, @@ -279,11 +301,11 @@ mod tests { #[test] fn extend_read_result_zero_extends_to_32_bit_registers() { assert_eq!( - extend_read_result(0x1_ffff_ffff, MemoryAccessWidth::U32, false, false), + read_access(MemoryAccessWidth::U32, false, false).extend_read_result(0x1_ffff_ffff), 0xffff_ffff ); assert_eq!( - extend_read_result(0x1234, MemoryAccessWidth::U16, false, false), + read_access(MemoryAccessWidth::U16, false, false).extend_read_result(0x1234), 0x1234 ); } @@ -291,11 +313,11 @@ mod tests { #[test] fn extend_read_result_sign_extends_to_requested_register_width() { assert_eq!( - extend_read_result(0x80, MemoryAccessWidth::U8, true, true), + read_access(MemoryAccessWidth::U8, true, true).extend_read_result(0x80), 0xffff_ffff_ffff_ff80 ); assert_eq!( - extend_read_result(0x80, MemoryAccessWidth::U8, true, false), + read_access(MemoryAccessWidth::U8, true, false).extend_read_result(0x80), 0xffff_ff80 ); } diff --git a/src/exceptions.rs b/src/exceptions.rs index f12d26d..ed03867 100644 --- a/src/exceptions.rs +++ b/src/exceptions.rs @@ -8,6 +8,7 @@ use aarch64_rt::{ExceptionHandlers, RegisterStateRef as VolatileRegisterStateRef}; use core::arch::naked_asm; +use thiserror::Error; /// Non-volatile registers saved by RITM's synchronous lower-EL handler wrapper. /// @@ -22,6 +23,10 @@ pub struct NonVolatileRegisters { const _: () = assert!(size_of::() == 8 * 10); +#[derive(Clone, Copy, Debug, Eq, Error, PartialEq)] +#[error("invalid guest register index")] +pub struct GuestRegisterWriteError; + /// Guest register view for synchronous lower-EL exceptions. /// /// This combines the volatile registers saved by `aarch64-rt` with the x19-x28 frame saved by @@ -61,7 +66,20 @@ impl<'a> GuestRegisterStateRef<'a> { } /// Updates guest GPR `index`. - pub fn write_gpr(&mut self, index: usize, value: u64) -> bool { + /// + /// # Errors + /// + /// Returns [`GuestRegisterWriteError`] if `index` is not a guest GPR. + /// + /// # Safety + /// + /// The caller must ensure that `value` is safe to write into guest GPR `index` for the trapped + /// instruction or exception being handled. + pub unsafe fn write_gpr( + &mut self, + index: usize, + value: u64, + ) -> Result<(), GuestRegisterWriteError> { match index { 0..=18 => { // SAFETY: We only update the saved guest register targeted by the handler. @@ -83,14 +101,19 @@ impl<'a> GuestRegisterStateRef<'a> { } } 31 => {} - _ => return false, + _ => return Err(GuestRegisterWriteError), } - true + Ok(()) + } + + /// Returns the saved exception return address. + pub fn exception_return_address(&self) -> usize { + self.volatile.elr } - /// Returns the saved exception return address and status. - pub fn exception_return(&self) -> (usize, u64) { - (self.volatile.elr, self.volatile.spsr) + /// Returns the saved exception return status. + pub fn exception_return_status(&self) -> u64 { + self.volatile.spsr } /// Advances the saved exception return address by `byte_count`. @@ -149,13 +172,7 @@ impl ExceptionHandlers for Exceptions { extern "C" fn sync_lower_with_nonvolatile( volatile: VolatileRegisterStateRef, - nonvolatile: *mut NonVolatileRegisters, + nonvolatile: &mut NonVolatileRegisters, ) { - // SAFETY: The naked wrapper passes a valid pointer to the x19-x28 frame it saved on the stack. - let nonvolatile = unsafe { - nonvolatile - .as_mut() - .expect("non-volatile register frame should not be null") - }; crate::hypervisor::handle_sync_lower(GuestRegisterStateRef::new(volatile, nonvolatile)); } diff --git a/src/hvc_response.rs b/src/hvc_response.rs index 4398e0a..539394b 100644 --- a/src/hvc_response.rs +++ b/src/hvc_response.rs @@ -64,7 +64,8 @@ impl From<[u64; 18]> for HvcResponse { } impl HvcResult { - pub(crate) fn modify_register_state(self, register_state: &mut GuestRegisterStateRef) { + /// Applies the HVC result to the saved guest register state following the SMCCC convention. + pub fn modify_register_state(self, register_state: &mut GuestRegisterStateRef) { match self { HvcResult::Handled(Ok(HvcResponse::Success(results))) => { write_response_registers(register_state, &results); @@ -73,19 +74,37 @@ impl HvcResult { write_response_registers(register_state, &results); } HvcResult::Handled(Err(error)) => { - register_state.write_gpr(0, error_to_u64(error)); + // SAFETY: x0 is the SMCCC return value register. + unsafe { + register_state + .write_gpr(0, error_to_u64(error)) + .expect("x0 is a valid guest register"); + } } HvcResult::Unhandled => { debug!("HVC call not handled, returning NOT_SUPPORTED"); - register_state.write_gpr(0, error_to_u64(NotSupported)); + // SAFETY: x0 is the SMCCC return value register. + unsafe { + register_state + .write_gpr(0, error_to_u64(NotSupported)) + .expect("x0 is a valid guest register"); + } } } } } fn write_response_registers(register_state: &mut GuestRegisterStateRef, results: &[u64]) { + assert!(results.len() <= 18); + for (index, value) in results.iter().copied().enumerate() { - register_state.write_gpr(index, value); + // SAFETY: SMCCC responses return values in x0-x17, and callers only pass slices from + // fixed-size x0-x3 or x0-x17 response arrays. + unsafe { + register_state + .write_gpr(index, value) + .expect("SMCCC response register index should be valid"); + } } } diff --git a/src/hypervisor.rs b/src/hypervisor.rs index 58c9290..5377d37 100644 --- a/src/hypervisor.rs +++ b/src/hypervisor.rs @@ -29,11 +29,16 @@ use arm_sysregs::{ }; use core::arch::naked_asm; use log::debug; -use memory_access::{DecodedMemoryAccessKind, decode_memory_access, extend_read_result}; -use spin::Once; +use memory_access::{MemoryAccess, MemoryAccessKind}; +use spin::LazyLock; use spin::mutex::SpinMutex; -static STAGE2_CONFIG: Once = Once::new(); +static STAGE2_CONFIG: LazyLock = LazyLock::new(|| { + let mut builder = Stage2Builder::new(); + PlatformImpl::configure_memory_access(&mut builder) + .expect("failed to configure stage-2 memory access"); + builder.build() +}); const AARCH64_INSTRUCTION_LENGTH: usize = 4; const EC_DATA_ABORT_LOWER_EL: u8 = 0x24; @@ -164,12 +169,7 @@ fn setup_stage2() { } fn stage2_config() -> &'static Stage2Config { - STAGE2_CONFIG.call_once(|| { - let mut builder = Stage2Builder::new(); - PlatformImpl::configure_memory_access(&mut builder) - .expect("failed to configure stage-2 memory access"); - builder.build() - }) + &STAGE2_CONFIG } /// Returns to EL1. @@ -314,7 +314,7 @@ pub fn handle_sync_lower(mut register_state: GuestRegisterStateRef) { } fn try_memory_access_handler(register_state: &mut GuestRegisterStateRef) -> Result<(), ()> { - let decoded = decode_memory_access( + let decoded = MemoryAccess::decode( read_esr_el2().iss(), read_hpfar_el2().fipa(), read_far_el2().va(), @@ -328,7 +328,7 @@ fn try_memory_access_handler(register_state: &mut GuestRegisterStateRef) -> Resu .ok_or(())?; match decoded.kind { - DecodedMemoryAccessKind::Read => { + MemoryAccessKind::Read => { let read_handler = handler_match.region.handler.read.ok_or(())?; let access = MemoryReadAccess { ipa: decoded.ipa, @@ -338,22 +338,18 @@ fn try_memory_access_handler(register_state: &mut GuestRegisterStateRef) -> Resu match read_handler(access) { MemoryReadResult::Value(value) => { - let value = extend_read_result( - value, - decoded.width, - decoded.sign_extend, - decoded.register_width_64, - ); - write_saved_guest_register(register_state, decoded.register_index, value) - .then_some(()) - .ok_or(())?; + let value = decoded.extend_read_result(value); + // SAFETY: The decoded register is the trapped load instruction's Rt, and + // `value` is the emulated load result for that instruction. + unsafe { register_state.write_gpr(decoded.register_index, value) } + .map_err(|_| ())?; advance_guest_pc(register_state); Ok(()) } MemoryReadResult::Fault => Err(()), } } - DecodedMemoryAccessKind::Write { value } => { + MemoryAccessKind::Write { value } => { let write_handler = handler_match.region.handler.write.ok_or(())?; let access = MemoryWriteAccess { ipa: decoded.ipa, @@ -373,14 +369,6 @@ fn try_memory_access_handler(register_state: &mut GuestRegisterStateRef) -> Resu } } -fn write_saved_guest_register( - register_state: &mut GuestRegisterStateRef, - index: usize, - value: u64, -) -> bool { - register_state.write_gpr(index, value) -} - fn advance_guest_pc(register_state: &mut GuestRegisterStateRef) { // SAFETY: The memory access handler has emulated the trapped instruction, so guest execution can // resume at the following instruction. @@ -404,7 +392,8 @@ fn inject_data_abort(register_state: &mut GuestRegisterStateRef) { let handler = vbar + 0x200; // Current EL with SPx Sync // Save current context to guest EL1 regs - let (elr, spsr) = register_state.exception_return(); + let elr = register_state.exception_return_address(); + let spsr = register_state.exception_return_status(); // SAFETY: We are accessing EL1 system registers to inject exception. unsafe { write_elr_el1(ElrEl1::from_bits_retain(elr as u64)); diff --git a/src/stage2.rs b/src/stage2.rs index 80af92a..c30e800 100644 --- a/src/stage2.rs +++ b/src/stage2.rs @@ -173,7 +173,7 @@ impl MemoryAccessHandlerRegistry { } fn register(&mut self, region: MemoryAccessRegion) -> Result<(), Stage2ConfigError> { - if self.regions.len() >= MAX_MEMORY_ACCESS_HANDLERS { + if self.regions.is_full() { return Err(Stage2ConfigError::TooManyMemoryAccessHandlerRegions); } @@ -317,7 +317,7 @@ impl Stage2Builder { } fn check_allowed_range_capacity(&self) -> Result<(), Stage2ConfigError> { - if self.allowed_ranges.len() >= MAX_ALLOWED_RANGES { + if self.allowed_ranges.is_full() { Err(Stage2ConfigError::TooManyAllowedRanges) } else { Ok(()) @@ -348,17 +348,17 @@ impl Stage2Builder { } /// Rounds `address` down to the nearest stage-2 page boundary. -pub(crate) fn align_down_to_page(address: usize) -> usize { +pub fn align_down_to_page(address: usize) -> usize { address / PAGE_SIZE * PAGE_SIZE } /// Rounds `address` up to the nearest stage-2 page boundary. -pub(crate) fn align_up_to_page(address: usize) -> usize { +pub fn align_up_to_page(address: usize) -> usize { address.next_multiple_of(PAGE_SIZE) } /// Converts a `usize` address to an IPA. -pub(crate) fn to_ipa(address: usize) -> u64 { +pub fn to_ipa(address: usize) -> u64 { u64::try_from(address).expect("IPA should fit in u64") }