diff --git a/Cargo.toml b/Cargo.toml index 1d14a8e4..3620d85a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,10 @@ byteorder = { version = "1.5.0", default-features = false } [dev-dependencies] aml_test_tools = { path = "tools/aml_test_tools" } +lock_api = "0.4.14" +parking_lot = "0.12.5" pretty_env_logger = "0.5.0" +serial_test = "4.0.1" [features] default = ["alloc", "aml"] diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 37ec1e60..6de45287 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -109,6 +109,11 @@ where region_handlers: Spinlock>>, global_lock_mutex: Handle, + + /// How many times has the thread that owns the global lock acquired it? Zero should correspond + /// to [`global_lock_mutex`] being unlocked. + global_lock_acquisition_count: AtomicU64, + registers: Arc>, facs: Option>, } @@ -148,6 +153,7 @@ where integer_size: IntegerSize::from_revision(dsdt_revision), region_handlers: Spinlock::new(BTreeMap::new()), global_lock_mutex, + global_lock_acquisition_count: AtomicU64::new(0), registers, facs, } @@ -327,25 +333,34 @@ where } pub fn acquire_global_lock(&self, timeout: u16) -> Result<(), AmlError> { + // This lock is released by `release_global_lock`. Acquire unconditionally since AML + // mutexes are reentrant, and the [`Handler`] might want to take action on each acquisition. self.handler.acquire(self.global_lock_mutex, timeout)?; - // Now we've acquired the AML-side mutex, acquire the hardware side - // TODO: count the number of times we have to go round this loop / enforce a timeout? - loop { - if self.try_do_acquire_firmware_lock() { - break Ok(()); - } else { - /* - * The lock is owned by the firmware. We have set the pending bit - we now need to - * wait for the firmware to signal it has released the lock. - * - * TODO: this should wait for an interrupt from the firmware. That needs more infra - * so for now let's just spin round and try and acquire it again... - */ - self.handler.release(self.global_lock_mutex); - continue; + let last = self.global_lock_acquisition_count.fetch_add(1, Ordering::Relaxed); + + // The firmware lock does not have an acquisition counter, so don't try and acquire a + // firmware lock we already own. + if last == 0 { + // Now we've acquired the AML-side mutex, acquire the hardware side + // TODO: count the number of times we have to go round this loop / enforce a timeout? + loop { + if self.try_do_acquire_firmware_lock() { + break; + } else { + /* + * The lock is owned by the firmware. We have set the pending bit - we now need + * to wait for the firmware to signal it has released the lock. + * + * TODO: this should wait for an interrupt from the firmware. That needs more + * infra so for now let's just spin round and try and acquire it again... + */ + continue; + } } } + + Ok(()) } /// Attempt to acquire the firmware lock, setting the owned bit if the lock is free. If the @@ -379,10 +394,22 @@ where } pub fn release_global_lock(&self) -> Result<(), AmlError> { - let is_pending = self.do_release_firmware_lock(); - if is_pending { - self.registers.pm1_control_registers.set_bit(Pm1ControlBit::GlobalLockRelease, true).unwrap(); + let c = self.global_lock_acquisition_count.fetch_sub(1, Ordering::Relaxed); + + // Only release the firmware lock if that was the last global lock acquisition that this + // thread was holding. + // + // There is no risk of `c` changing - this thread still holds the global lock mutex, so only + // this thread can change `self.global_lock_acquisition_count`. + if c == 1 { + let is_pending = self.do_release_firmware_lock(); + if is_pending { + self.registers.pm1_control_registers.set_bit(Pm1ControlBit::GlobalLockRelease, true).unwrap(); + } } + + // This mutex was locked in `acquire_global_mutex`. + self.handler.release(self.global_lock_mutex); Ok(()) } @@ -884,7 +911,8 @@ where } Opcode::Acquire => { extract_args!(op => [Argument::Object(mutex)]); - let Object::Mutex { mutex, sync_level: _ } = **mutex else { + let mutex = mutex.clone().unwrap_reference(); + let Object::Mutex { mutex, sync_level: _ } = *mutex else { Err(AmlError::InvalidOperationOnObject { op: Operation::Acquire, typ: mutex.typ() })? }; let timeout = context.next_u16()?; @@ -900,7 +928,8 @@ where } Opcode::Release => { extract_args!(op => [Argument::Object(mutex)]); - let Object::Mutex { mutex, sync_level: _ } = **mutex else { + let mutex = mutex.clone().unwrap_reference(); + let Object::Mutex { mutex, sync_level: _ } = *mutex else { Err(AmlError::InvalidOperationOnObject { op: Operation::Release, typ: mutex.typ() })? }; diff --git a/src/lib.rs b/src/lib.rs index 057b2a5c..7e897f68 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -507,8 +507,23 @@ pub trait Handler: Clone { /// /// AML mutexes are **reentrant** - that is, a thread may acquire the same mutex more than once /// without causing a deadlock. + /// + /// Note: The ACPI specification says: "A Mutex must be totally released before an invocation + /// completes." - this crate does not enforce that, and it also does not forcibly release + /// mutexes when the top-level called method exits. #[cfg(feature = "aml")] fn acquire(&self, mutex: Handle, timeout: u16) -> Result<(), aml::AmlError>; + + /// Release the mutex referred to by the given handle. + /// + /// AML mutexes are reentrant - this function should release only one acquisition of the mutex. + /// + /// In correctly written AML code, the number of Acquire statements (which cause the interpreter + /// to call [`acquire`](Self::acquire) must equal the number of Release statements (which causes + /// this function to be called). + /// + /// According to the ACPI spec: "It is fatal to release ownership on a Mutex unless it is + /// currently owned" - this crate does not specify what a Handler should do in this case. #[cfg(feature = "aml")] fn release(&self, mutex: Handle); diff --git a/tests/global_lock.rs b/tests/global_lock.rs new file mode 100644 index 00000000..4c7d37fa --- /dev/null +++ b/tests/global_lock.rs @@ -0,0 +1,281 @@ +//! Test the ACPI Global Lock mechanism. + +// These tests make use of `serial_test` as the test infra creates one firmware lock and then uses +// it for all tests - which causes these tests of the locking mechanism to conflict with each other +// when run in parallel. + +use acpi::{Handle, Handler, PhysicalMapping, aml::AmlError}; +use aml_test_tools::new_interpreter; +use lock_api::RawReentrantMutex; +use parking_lot::{RawMutex, RawThreadId}; +use pci_types::PciAddress; +use serial_test::serial; +use std::{ + sync::{ + Arc, + Barrier, + atomic::{AtomicBool, Ordering}, + }, + thread, + time::Duration, +}; + +mod test_infra; + +#[test] +#[serial] +fn uncontended_acquire_release() { + let interpreter = new_interpreter(LockHandler::new()); + + interpreter.acquire_global_lock(0).expect("Failed to acquire lock"); + interpreter.release_global_lock().expect("Failed to release lock"); +} + +#[test] +#[serial] +fn single_thread_acquire_release() { + let interpreter = new_interpreter(LockHandler::new()); + + interpreter.acquire_global_lock(0).expect("Failed to acquire lock (1)"); + interpreter.acquire_global_lock(0).expect("Failed to acquire lock (2)"); + interpreter.release_global_lock().expect("Failed to release lock (2)"); + interpreter.release_global_lock().expect("Failed to release lock (1)"); +} + +#[test] +#[serial] +fn multi_thread_acquire_release() { + // Steps in order (threads are labeled A & B): + // 1: A acquires lock + // 2: B attempts to acquire lock, fails + // 3: A waits for B to complete #2. + // 4: A releases lock + // 5: B waits for A to release lock and acquires lock successfully. + + let handler = LockHandler::new(); + let interpreter = new_interpreter(handler.clone()); + + let barrier = Barrier::new(2); + + let success = AtomicBool::new(false); + + // 1: A acquires the lock + interpreter.acquire_global_lock(0).expect("1: A failed to acquire lock"); + thread::scope(|s| { + s.spawn(|| { + let interpreter = new_interpreter(handler); + + // 2: B attempts to acquire lock, fails. + let e = interpreter.acquire_global_lock(0).expect_err("2: B managed to acquire lock!"); + assert!(matches!(e, AmlError::MutexAcquireTimeout)); + + // 3: A waits for B to complete #2 + barrier.wait(); + // 4: Occurs in thread A (the main thread) + + // 5: B attempts to acquire lock, waiting as needed. + interpreter.acquire_global_lock(10000).expect("5: B failed to acquire lock"); + + success.store(true, Ordering::Relaxed); + + interpreter.release_global_lock().expect("B: Failed to release global lock"); + }); + + // 3: A waits for B to complete #2 + barrier.wait(); + + // 4: A releases the global lock + interpreter.release_global_lock().expect("Failed to release global lock"); + }); + + // Check that B acquired lock successfully + assert!(success.load(Ordering::Relaxed)); +} + +#[test] +#[serial] +fn uacpi_global_lock_test() { + // This test is adapted from the uACPI test file `tests/test-cases/global-lock.asl` + const AML: &str = r#" +DefinitionBlock ("", "DSDT", 2, "uTEST", "TESTTABL", 0xF0F0F0F0) +{ + Method (CHEK, 1, Serialized, 15) + { + If (Arg0 != 0) { + Debug = "Failed to acquire the global lock!" + Return (1) + } + + Return (0) + } + + Method (MAIN, 0, Serialized) + { + Local0 = 0 + + Local0 += CHEK(Acquire (_GL, 0xFFFF)) + Local0 += CHEK(Acquire (_GL, 0xFFFF)) + Local0 += CHEK(Acquire (_GL, 0xFFFF)) + Local0 += CHEK(Acquire (_GL, 0xFFFF)) + + Release(_GL) + Release(_GL) + Release(_GL) + Release(_GL) + + Return (Local0) + } +} +"#; + let handler = LockHandler::new(); + test_infra::run_aml_test(AML, handler); +} + +#[derive(Clone)] +struct LockHandler { + // Use RawReentrantMutex for two reasons: + // 1. We need a raw mutex because we don't want to hold onto the RAII MutexGuard - we'll control + // locking and unlocking manually + // 2. We need to handle reentrancy, so RawMutex by itself is insufficient. + mutex: Arc>, +} + +impl LockHandler { + const MUTEX_HANDLE: Handle = Handle(1); + + pub fn new() -> Self { + Self { mutex: Arc::new(RawReentrantMutex::INIT) } + } +} + +impl Handler for LockHandler { + fn create_mutex(&self) -> Handle { + // Don't add complexity for a simple test handler - we only need one mutex. + Self::MUTEX_HANDLE + } + + fn acquire(&self, mutex: Handle, timeout: u16) -> Result<(), AmlError> { + assert_eq!(mutex, Self::MUTEX_HANDLE); + + match timeout { + 0 => self.mutex.try_lock(), + 0xffff => { + self.mutex.lock(); + true + } + _ => self.mutex.try_lock_for(Duration::from_millis(timeout as u64)), + } + .ok_or(AmlError::MutexAcquireTimeout) + } + + fn release(&self, mutex: Handle) { + assert_eq!(mutex, Self::MUTEX_HANDLE); + assert!(self.mutex.is_owned_by_current_thread()); + + // Safety: We've just checked that it's this thread that owns the mutex, so it's safe to + // unlock. + unsafe { + self.mutex.unlock(); + } + } + + unsafe fn map_physical_region(&self, _physical_address: usize, _size: usize) -> PhysicalMapping { + unimplemented!() + } + + fn unmap_physical_region(_region: &PhysicalMapping) { + // Do nothing + } + + fn read_u8(&self, _address: usize) -> u8 { + unimplemented!() + } + + fn read_u16(&self, _address: usize) -> u16 { + unimplemented!() + } + + fn read_u32(&self, _address: usize) -> u32 { + unimplemented!() + } + + fn read_u64(&self, _address: usize) -> u64 { + unimplemented!() + } + + fn write_u8(&self, _address: usize, _value: u8) { + unimplemented!() + } + + fn write_u16(&self, _address: usize, _value: u16) { + unimplemented!() + } + + fn write_u32(&self, _address: usize, _value: u32) { + unimplemented!() + } + + fn write_u64(&self, _address: usize, _value: u64) { + unimplemented!() + } + + fn read_io_u8(&self, _port: u16) -> u8 { + unimplemented!() + } + + fn read_io_u16(&self, _port: u16) -> u16 { + unimplemented!() + } + + fn read_io_u32(&self, _port: u16) -> u32 { + unimplemented!() + } + + fn write_io_u8(&self, _port: u16, _value: u8) { + unimplemented!() + } + + fn write_io_u16(&self, _port: u16, _value: u16) { + unimplemented!() + } + + fn write_io_u32(&self, _port: u16, _value: u32) { + unimplemented!() + } + + fn read_pci_u8(&self, _address: PciAddress, _offset: u16) -> u8 { + unimplemented!() + } + + fn read_pci_u16(&self, _address: PciAddress, _offset: u16) -> u16 { + unimplemented!() + } + + fn read_pci_u32(&self, _address: PciAddress, _offset: u16) -> u32 { + unimplemented!() + } + + fn write_pci_u8(&self, _address: PciAddress, _offset: u16, _value: u8) { + unimplemented!() + } + + fn write_pci_u16(&self, _address: PciAddress, _offset: u16, _value: u16) { + unimplemented!() + } + + fn write_pci_u32(&self, _address: PciAddress, _offset: u16, _value: u32) { + unimplemented!() + } + + fn nanos_since_boot(&self) -> u64 { + unimplemented!() + } + + fn stall(&self, _microseconds: u64) { + unimplemented!() + } + + fn sleep(&self, _milliseconds: u64) { + unimplemented!() + } +} diff --git a/tests/test_infra/mod.rs b/tests/test_infra/mod.rs index eda991bd..45f4fc7b 100644 --- a/tests/test_infra/mod.rs +++ b/tests/test_infra/mod.rs @@ -12,8 +12,8 @@ use aml_test_tools::{ }; use std::str::FromStr; -// The following two functions are very similar in structure, but whilst there are only two of them -// it's not worth adding complexity to make them DRY. +// `run_aml_test` and `run_opcodes_test` are very similar in structure, but whilst there are only +// two of them it's not worth adding complexity to make them DRY. /// Run a test against an ASL string. ///