For discussion: A way of improving regression/integration testing for ASL snippets

Cargo.toml

@@ -1,5 +1,5 @@
 [workspace]
-members = ["tools/aml_tester", "tools/acpi_dumper"]
+members = ["tools/aml_tester", "tools/acpi_dumper", "tools/aml_test_tools"]
 resolver = "2"
 
 [package]
@@ -20,6 +20,10 @@ spinning_top = "0.3.0"
 pci_types = { version = "0.10.0", public = true }
 byteorder = { version = "1.5.0", default-features = false }
 
+[dev-dependencies]
+aml_test_tools = { path = "tools/aml_test_tools" }
+pretty_env_logger = "0.5.0"
+
 [features]
 default = ["alloc", "aml"]
 alloc = []

src/aml/mod.rs

@@ -95,8 +95,9 @@ impl<H> Interpreter<H>
 where
     H: Handler,
 {
-    /// Construct a new `Interpreter`. This does not load any tables - if you have an `AcpiTables`
-    /// already, use [`Interpreter::new_from_tables`] instead.
+    /// Construct a new [`Interpreter`]. This does not load any tables - if you have an
+    /// [`crate::AcpiTables`] already, construct an [`AcpiPlatform`] first and then use
+    /// [`Interpreter::new_from_platform`]
     pub fn new(
         handler: H,
         dsdt_revision: u8,
@@ -120,8 +121,7 @@ where
         }
     }
 
-    /// Construct a new `Interpreter` with the given set of ACPI tables. This will automatically
-    /// load the DSDT and any SSDTs in the supplied [`AcpiTables`].
+    /// Construct a new [`Interpreter`] with the given [`AcpiPlatform`].
     pub fn new_from_platform(platform: &AcpiPlatform<H>) -> Result<Interpreter<H>, AcpiError> {
         fn load_table(interpreter: &Interpreter<impl Handler>, table: AmlTable) -> Result<(), AcpiError> {
             let mapping = unsafe {
@@ -158,7 +158,7 @@ where
     }
 
     /// Load the supplied byte stream as an AML table. This should be only the encoded AML stream -
-    /// not the header at the start of a table. If you've used [`Interpreter::new_from_tables`],
+    /// not the header at the start of a table. If you've used [`Interpreter::new_from_platform`],
     /// you'll likely not need to load any tables manually.
     pub fn load_table(&self, stream: &[u8]) -> Result<(), AmlError> {
         let context = unsafe { MethodContext::new_from_table(stream) };

src/lib.rs

@@ -20,7 +20,7 @@
 //!
 //! Next, you'll need to get the physical address of either the RSDP, or the RSDT/XSDT. The method
 //! for doing this depends on the platform you're running on and how you were booted. If you know
-//! the system was booted via the BIOS, you can use [`rsdp::Rsdp::search_for_on_bios`]. UEFI provides a
+//! the system was booted via the BIOS, you can use [`Rsdp::search_for_on_bios`]. UEFI provides a
 //! separate mechanism for getting the address of the RSDP.
 //!
 //! You then need to construct an instance of [`AcpiTables`], which can be done in a few ways
@@ -29,7 +29,18 @@
 //! * Use [`AcpiTables::from_rsdt`] if you have the physical address of the RSDT/XSDT
 //!
 //! Once you have an [`AcpiTables`], you can search for relevant tables, or use the higher-level
-//! interfaces, such as [`PlatformInfo`], [`PciConfigRegions`], or [`HpetInfo`].
+//! interfaces, such as [`PowerProfile`], or [`HpetInfo`].
+//!
+//! If you have the `aml` feature enabled then you can construct an
+//! [AML interpreter](`crate::aml::Interpreter`) by first constructing an
+//! [`AcpiPlatform`](platform::AcpiPlatform) and then the interpreter:
+//!
+//! ```ignore,rust
+//! let mut acpi_handler = YourHandler::new(/*args*/);
+//! let acpi_tables = unsafe { AcpiTables::from_rsdp(acpi_handler.clone(), rsdp_addr) }.unwrap();
+//! let platform = AcpiPlatform::new(acpi_tables, acpi_handler).unwrap();
+//! let interpreter = Interpreter::new_from_platform(&platform).unwrap();
+//! ```
 
 #![no_std]
 #![feature(allocator_api)]
@@ -398,9 +409,9 @@ pub trait Handler: Clone {
     /// - `size` must be at least `size_of::<T>()`.
     unsafe fn map_physical_region<T>(&self, physical_address: usize, size: usize) -> PhysicalMapping<Self, T>;
 
-    /// Unmap the given physical mapping. This is called when a `PhysicalMapping` is dropped, you should **not** manually call this.
+    /// Unmap the given physical mapping. This is called when a [`PhysicalMapping`] is dropped, you should **not** manually call this.
     ///
-    /// Note: A reference to the `Handler` used to construct `region` can be acquired by calling [`PhysicalMapping::mapper`].
+    /// Note: A reference to the [`Handler`] used to construct `region` can be acquired from [`PhysicalMapping::handler`].
     fn unmap_physical_region<T>(region: &PhysicalMapping<Self, T>);
 
     // TODO: maybe we should map stuff ourselves in the AML interpreter and do this internally?

tests/normal_fields.rs

@@ -0,0 +1,109 @@
+// Test operations on "normal" fields - those that are not Index or Bank fields.
+
+use aml_test_tools::handlers::std_test_handler::{
+    Command,
+    construct_std_handler,
+    create_mutex,
+    read_io_u8,
+    read_io_u16,
+    read_u16,
+    write_io_u8,
+    write_io_u16,
+    write_u16,
+};
+
+mod test_infra;
+
+#[test]
+fn test_basic_store_and_load() {
+    const AML: &str = r#"DefinitionBlock("%FN%", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
+    OperationRegion(MEM, SystemMemory, 0x40000, 0x1000)
+    Field(MEM, WordAcc, NoLock, Preserve) {
+        A, 16,
+        B, 16
+    }
+
+    Method(MAIN, 0, NotSerialized) {
+        A = 0xA5A5
+        B = A
+        Return (0)
+    }
+}
+"#;
+
+    const EXPECTED_COMMANDS: &[Command] = &[
+        create_mutex(),
+        // A = 0xA5A5
+        write_u16(0x40000, 0xA5A5),
+        // B = A
+        read_u16(0x40000, 0xA5A5),
+        write_u16(0x40002, 0xA5A5),
+    ];
+
+    let handler = construct_std_handler(EXPECTED_COMMANDS.to_vec());
+    test_infra::run_aml_test(AML, handler);
+}
+
+#[test]
+fn test_narrow_access_store_and_load() {
+    const AML: &str = r#"DefinitionBlock("%FN%", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
+    OperationRegion(MEM, SystemIO, 0x40, 0x10)
+    Field(MEM, ByteAcc, NoLock, Preserve) {
+        A, 16,
+        B, 16
+    }
+
+    Method(MAIN, 0, NotSerialized) {
+        A = 0xA55A
+        B = A
+        Return (0)
+    }
+}
+"#;
+
+    const EXPECTED_COMMANDS: &[Command] = &[
+        create_mutex(),
+        // A = 0xA55A
+        write_io_u8(0x40, 0x5A),
+        write_io_u8(0x41, 0xA5),
+        // B = A
+        read_io_u8(0x40, 0x5A),
+        read_io_u8(0x41, 0xA5),
+        write_io_u8(0x42, 0x5A),
+        write_io_u8(0x43, 0xA5),
+    ];
+
+    let handler = construct_std_handler(EXPECTED_COMMANDS.to_vec());
+    test_infra::run_aml_test(AML, handler);
+}
+
+#[test]
+fn test_unaligned_field_store() {
+    const AML: &str = r#"DefinitionBlock("%FN%", "DSDT", 1, "RSACPI", "BUFFLD", 1) {
+    OperationRegion(MEM, SystemIO, 0x40, 0x10)
+    Field(MEM, WordAcc, NoLock, Preserve) {
+        A, 7,
+        B, 8
+    }
+
+    Method(MAIN, 0, NotSerialized) {
+        A = 4
+        B = A
+
+        Return (0)
+    }
+}
+"#;
+
+    const EXPECTED_COMMANDS: &[Command] = &[
+        create_mutex(),
+        read_io_u16(0x40, 0),
+        write_io_u16(0x40, 0x04),
+        read_io_u16(0x40, 4),
+        read_io_u16(0x40, 4),
+        write_io_u16(0x40, 0x204),
+    ];
+
+    let handler = construct_std_handler(EXPECTED_COMMANDS.to_vec());
+    test_infra::run_aml_test(AML, handler);
+}

tests/test_infra/mod.rs

@@ -0,0 +1,13 @@
+use acpi::Handler;
+use aml_test_tools::{new_interpreter, run_test_for_string, TestResult};
+use aml_test_tools::handlers::logging_handler::LoggingHandler;
+
+pub fn run_aml_test(asl: &'static str, handler: impl Handler) {
+    // Tests calling `run_aml_test` don't do much else, and we usually want logging, so initialize it here.
+    let _ = pretty_env_logger::try_init();
+
+    let logged_handler = LoggingHandler::new(handler);
+    let mut interpreter = new_interpreter(logged_handler);
+
+    assert_eq!(run_test_for_string(asl, &mut interpreter), TestResult::Pass);
+}

tools/aml_test_tools/Cargo.toml

@@ -0,0 +1,12 @@
+[package]
+name = "aml_test_tools"
+version = "0.1.0"
+authors = ["Isaac Woods", "Martin Hughes"]
+edition = "2024"
+publish = false
+
+[dependencies]
+acpi = { path = "../.." }
+log = "0.4"
+pci_types = "0.10.0"
+tempfile = "3.26.0"