From 2cae019a3b53c56c1c53c7a76d5077644a60cdc7 Mon Sep 17 00:00:00 2001 From: Isaac Woods Date: Sat, 14 Mar 2026 17:52:14 +0000 Subject: [PATCH 1/2] Remove panics in GAS handling code --- src/address.rs | 71 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/src/address.rs b/src/address.rs index fee34398..0bb3eeb3 100644 --- a/src/address.rs +++ b/src/address.rs @@ -2,6 +2,7 @@ //! in a wide range of address spaces. use crate::{AcpiError, Handler, PhysicalMapping}; +use core::ptr; use log::warn; /// This is the raw form of a Generic Address Structure, and follows the layout found in the ACPI tables. @@ -143,7 +144,7 @@ where } AddressSpace::SystemIo => Ok(MappedGas { gas, handler: handler.clone(), mapping: None }), other => { - warn!("Tried to map GAS of unsupported type {:?}", other); + warn!("Mapping a GAS in address space {:?} is not supported!", other); Err(AcpiError::LibUnimplemented) } } @@ -158,25 +159,24 @@ where match self.gas.address_space { AddressSpace::SystemMemory => { let mapping = self.mapping.as_ref().unwrap(); - let value = match access_size_bits { - 8 => unsafe { core::ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u8) as u64 }, - 16 => unsafe { core::ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u16) as u64 }, - 32 => unsafe { core::ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u32) as u64 }, - 64 => unsafe { core::ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u64) }, - _ => panic!(), - }; - Ok(value) + match access_size_bits { + 8 => Ok(unsafe { ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u8) as u64 }), + 16 => Ok(unsafe { ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u16) as u64 }), + 32 => Ok(unsafe { ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u32) as u64 }), + 64 => Ok(unsafe { ptr::read_volatile(mapping.virtual_start.as_ptr() as *const u64) }), + _ => Err(AcpiError::InvalidGenericAddress), + } } - AddressSpace::SystemIo => { - let value = match access_size_bits { - 8 => self.handler.read_io_u8(self.gas.address as u16) as u64, - 16 => self.handler.read_io_u16(self.gas.address as u16) as u64, - 32 => self.handler.read_io_u32(self.gas.address as u16) as u64, - _ => panic!(), - }; - Ok(value) + AddressSpace::SystemIo => match access_size_bits { + 8 => Ok(self.handler.read_io_u8(self.gas.address as u16) as u64), + 16 => Ok(self.handler.read_io_u16(self.gas.address as u16) as u64), + 32 => Ok(self.handler.read_io_u32(self.gas.address as u16) as u64), + _ => Err(AcpiError::InvalidGenericAddress), + }, + _ => { + warn!("Read from GAS with address space {:?} is not supported. Ignored.", self.gas.address_space); + Err(AcpiError::LibUnimplemented) } - _ => unimplemented!(), } } @@ -188,16 +188,16 @@ where let mapping = self.mapping.as_ref().unwrap(); match access_size_bits { 8 => unsafe { - core::ptr::write_volatile(mapping.virtual_start.as_ptr(), value as u8); + ptr::write_volatile(mapping.virtual_start.as_ptr(), value as u8); }, 16 => unsafe { - core::ptr::write_volatile(mapping.virtual_start.as_ptr() as *mut u16, value as u16); + ptr::write_volatile(mapping.virtual_start.as_ptr() as *mut u16, value as u16); }, 32 => unsafe { - core::ptr::write_volatile(mapping.virtual_start.as_ptr() as *mut u32, value as u32); + ptr::write_volatile(mapping.virtual_start.as_ptr() as *mut u32, value as u32); }, - 64 => unsafe { core::ptr::write_volatile(mapping.virtual_start.as_ptr() as *mut u64, value) }, - _ => panic!(), + 64 => unsafe { ptr::write_volatile(mapping.virtual_start.as_ptr() as *mut u64, value) }, + _ => return Err(AcpiError::InvalidGenericAddress), } Ok(()) } @@ -206,11 +206,14 @@ where 8 => self.handler.write_io_u8(self.gas.address as u16, value as u8), 16 => self.handler.write_io_u16(self.gas.address as u16, value as u16), 32 => self.handler.write_io_u32(self.gas.address as u16, value as u32), - _ => panic!(), + _ => return Err(AcpiError::InvalidGenericAddress), } Ok(()) } - _ => unimplemented!(), + _ => { + warn!("Write to GAS with address space {:?} is not supported. Ignored.", self.gas.address_space); + Err(AcpiError::LibUnimplemented) + } } } } @@ -228,7 +231,7 @@ fn gas_decode_access_bit_width(gas: GenericAddress) -> Result { * * We use a third method, based on the alignment of the address, for registers that have * non-zero bit offsets. These are not typically encountered in normal registers - they very - * often mean the GAS has come from APEI (ACPI Platform Error Interface), and so needs speical + * often mean the GAS has come from APEI (ACPI Platform Error Interface), and so needs special * handling. */ if gas.bit_offset == 0 && [8, 16, 32, 64].contains(&gas.bit_width) { @@ -242,7 +245,19 @@ fn gas_decode_access_bit_width(gas: GenericAddress) -> Result { _ => Err(AcpiError::InvalidGenericAddress), } } else { - // TODO: work out access size based on alignment of the address - todo!() + /* + * Work out the access size based on the alignment of the address. We round up the total + * width (`bit_offset + bit_width`) to the next power-of-two, up to 64. + */ + let total_width = gas.bit_offset + gas.bit_width; + Ok(if total_width <= 8 { + 8 + } else if total_width <= 16 { + 16 + } else if total_width <= 32 { + 32 + } else { + 64 + }) } } From b82dcef5b8627da8ebbdaafc2e45b6c6cc4d07c3 Mon Sep 17 00:00:00 2001 From: Isaac Woods Date: Sat, 14 Mar 2026 20:15:52 +0000 Subject: [PATCH 2/2] Remove some panics in AML interpreter - Do not panic by default on fatal errors - Do not panic on encountering `DefLoad` or `DefLoadTable` - Do not panic on `DefNotify` - Add correct object type for `RawDataBuffer`s - Correctly handle `AccessField` and `ExtendedAccessField` - Partially handle stores to names --- src/aml/mod.rs | 96 ++++++++++++++++++++++++++++++++++++++++++-------- src/lib.rs | 4 +-- 2 files changed, 84 insertions(+), 16 deletions(-) diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 92aab751..1a858ad6 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -561,6 +561,17 @@ where }); } } + Opcode::Notify => { + // TODO: may need special handling on the node to get path? + let [Argument::Namestring(name), Argument::Object(value)] = &op.arguments[..] else { + panic!(); + }; + let value = value.as_integer()?; + + info!("Notify {:?} with value {}", name, value); + // TODO: support + return Err(AmlError::LibUnimplemented); + } Opcode::FromBCD => self.do_from_bcd(&mut context, op)?, Opcode::ToBCD => self.do_to_bcd(&mut context, op)?, Opcode::Name => { @@ -581,6 +592,7 @@ where let arg = arg.as_integer()?; self.handler.handle_fatal_error(*typ, *code, arg); context.retire_op(op); + return Err(AmlError::FatalErrorEncountered); } Opcode::OpRegion => { let [ @@ -822,6 +834,35 @@ where context.contribute_arg(Argument::Object(result)); context.retire_op(op); } + Opcode::Load => { + let [Argument::Namestring(object), Argument::Object(result)] = &op.arguments[..] else { + panic!(); + }; + // TODO: read the AML from the object and load it + warn!("Ignoring unsupported DefLoad operation (object={}, result = {})", object, result); + context.retire_op(op); + return Err(AmlError::LibUnimplemented); + } + Opcode::LoadTable => { + let [ + Argument::Object(signature), + Argument::Object(oem_id), + Argument::Object(oem_table_id), + Argument::Object(root_path), + Argument::Object(parameter_path), + Argument::Object(parameter_data), + ] = &op.arguments[..] + else { + panic!(); + }; + // TODO: search for the table in the RSDT/XSDT and load the contained AML + warn!( + "Ignoring unsupported DefLoadTable operation (signature = {}, oem_id = {}, oem_table_id = {}, root_path = {}, parameter_path = {}, parameter_data = {})", + signature, oem_id, oem_table_id, root_path, parameter_path, parameter_data + ); + context.retire_op(op); + return Err(AmlError::LibUnimplemented); + } Opcode::Sleep => { let [Argument::Object(msec)] = &op.arguments[..] else { panic!() }; self.handler.sleep(msec.as_integer()?); @@ -936,7 +977,7 @@ where // XXX: 15 is reserved ObjectType::Debug => 16, ObjectType::Reference => panic!(), - ObjectType::RawDataBuffer => todo!(), + ObjectType::RawDataBuffer => 17, }; context.contribute_arg(Argument::Object(Object::Integer(typ).wrap())); @@ -1266,8 +1307,17 @@ where let name = name.resolve(&context.current_scope)?; self.namespace.lock().insert(name, Object::Event(Arc::new(AtomicU64::new(0))).wrap())?; } - Opcode::LoadTable => todo!(), - Opcode::Load => todo!(), + Opcode::LoadTable => { + context.start(OpInFlight::new(Opcode::LoadTable, &[ResolveBehaviour::TermArg; 6])); + } + Opcode::Load => { + let name = context.namestring()?; + context.start(OpInFlight::new_with( + Opcode::Load, + vec![Argument::Namestring(name)], + &[ResolveBehaviour::Target], + )); + } Opcode::Stall => context.start(OpInFlight::new(Opcode::Stall, &[ResolveBehaviour::TermArg])), Opcode::Sleep => context.start(OpInFlight::new(Opcode::Sleep, &[ResolveBehaviour::TermArg])), Opcode::Acquire => context.start(OpInFlight::new(opcode, &[ResolveBehaviour::SuperName])), @@ -1710,7 +1760,7 @@ where kind: FieldUnitKind, start_pc: usize, pkg_length: usize, - flags: u8, + mut flags: u8, ) -> Result<(), AmlError> { const RESERVED_FIELD: u8 = 0x00; const ACCESS_FIELD: u8 = 0x01; @@ -1731,18 +1781,22 @@ where * elements. They change the access type and attributes for remaining fields in * the list. */ - let _access_type = context.next()?; + let access_type = context.next()?; let _access_attrib = context.next()?; - todo!() + flags.set_bits(0..4, access_type); + } + EXTENDED_ACCESS_FIELD => { + let access_type = context.next()?; + let _extended_access_attrib = context.next()?; + let _access_length = context.next()?; + flags.set_bits(0..4, access_type); + warn!("Ignoring extended attributes and length in ExtendedAccessField"); } CONNECT_FIELD => { // TODO: either consume a namestring or `BufferData` (it's not // clear what a buffer data acc is lmao) todo!("Connect field :("); } - EXTENDED_ACCESS_FIELD => { - todo!("Extended access field :("); - } _ => { context.current_block.pc -= 1; // TODO: this should only ever be a nameseg really... @@ -2123,8 +2177,7 @@ where Object::Package(_) => "[Package]".to_string(), Object::PowerResource { .. } => "[Power Resource]".to_string(), Object::Processor { .. } => "[Processor]".to_string(), - // TODO: what even is one of these?? - Object::RawDataBuffer => todo!(), + Object::RawDataBuffer => "[Raw Data Buffer]".to_string(), Object::String(value) => value.clone(), Object::ThermalZone => "[Thermal Zone]".to_string(), Object::Debug => "[Debug Object]".to_string(), @@ -2339,10 +2392,21 @@ where _ => panic!("Stores to objects like {:?} are not yet supported", target), }, - Argument::Namestring(_) => todo!(), - Argument::ByteData(_) | Argument::DWordData(_) | Argument::TrackedPc(_) | Argument::PkgLength(_) => { - panic!() + Argument::Namestring(name) => { + let existing = self.namespace.lock().get(name.clone()); + match existing { + Ok(existing) => { + unsafe { + // TODO: this should likely be doing an implicit cast depending on object type? + *existing.gain_mut(&token) = (*object).clone(); + } + } + Err(_) => { + self.namespace.lock().insert(name.clone(), object)?; + } + } } + _ => panic!("Invalid argument type as DefStore target"), } Ok(to_return) @@ -3330,6 +3394,10 @@ pub enum AmlError { PrtInvalidSource, PrtNoEntry, + /// An OEM-defined fatal error has occured. The specification states a host should log this + /// fatal error and then shutdown in a timely fashion. + FatalErrorEncountered, + /// This is emitted to signal that the library does not support the requested behaviour. This /// should eventually never be emitted. LibUnimplemented, diff --git a/src/lib.rs b/src/lib.rs index 03e4c290..91b53697 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -72,7 +72,7 @@ use core::{ pin::Pin, ptr::NonNull, }; -use log::warn; +use log::{error, warn}; use rsdp::Rsdp; /// `AcpiTables` should be constructed after finding the RSDP or RSDT/XSDT and allows enumeration @@ -480,7 +480,7 @@ pub trait Handler: Clone { #[cfg(feature = "aml")] fn handle_fatal_error(&self, fatal_type: u8, fatal_code: u32, fatal_arg: u64) { - panic!( + error!( "Fatal error while executing AML (encountered DefFatalOp). fatal_type = {}, fatal_code = {}, fatal_arg = {}", fatal_type, fatal_code, fatal_arg );