Rebase on fb69243 (#3)

* fix: increment `processor_count` if LocalX2Apic is found

Signed-off-by: Yuuki Takano <ytakanoster@gmail.com>

* fix: increment `local_nmi_line_count` if X1ApicNmi is found

Signed-off-by: Yuuki Takano <ytakanoster@gmail.com>

* Add `Debug` impl for `PhysicalMapping` even when `T` is not `Debug`

* Make `read_field` and friends take a mutable context

This is necessary because these methods will need to invoke methods in
the future.

Co-authored-by: Ron Williams <ron.williams.redox@gmail.com>

* Invoke `_SEG`, `_BBN`, and `_ADR` as methods for PCI region accesses

Co-authored-by: Ron Williams <ron.williams.redox@gmail.com>

* Move the `aml` crate to the 2021 Edition

Easy change while we're here

* Create `OpRegion` type to abstract away field manipulation

Lots of the field reading/writing behaviour isn't quite correct yet, but
this was getting unwieldly and spread about multiple places. Creating this
abstraction will hopefully help in future.

* Simplify `DefIncrement` and `DefDecrement` borrow checking

For some reason, this *definitely* required an extra scope before, but
doesn't seem to now...

* Add test for `DefIncrement` and `DefDecrement`

* Fix test utils

* Add `SdtHeaderIterator` to get all headers.

* fix comments

* aml: add extra debug info on parsing error

* AML: implement boolean field (#211)

* AML: Allow Field in ToInteger (rebased) (#213)

* acpi: add support for SPCR table (#216)

Add support for the Serial Port Console Redirection (SPCR). The table
provides information about the configuration and use of the serial
port or non-legacy UART interface.

* Add a function to find and maps the entire table. (#2)

---------

Signed-off-by: Yuuki Takano <ytakanoster@gmail.com>
Co-authored-by: Yuuki Takano <ytakanoster@gmail.com>
Co-authored-by: Caleb Robson <94545082+Spartan2909@users.noreply.github.com>
Co-authored-by: Isaac Woods <isaacwoods.home@gmail.com>
Co-authored-by: Ron Williams <ron.williams.redox@gmail.com>
Co-authored-by: rw-vanc <123669882+rw-vanc@users.noreply.github.com>
Co-authored-by: Carlos López <00xc@protonmail.com>

acpi/Cargo.toml

@@ -11,6 +11,7 @@ edition = "2021"
 
 [dependencies]
 bit_field = "0.10.2"
+bitflags = "2.5.0"
 log = "0.4.20"
 
 [features]

acpi/src/address.rs

@@ -16,6 +16,16 @@ pub(crate) struct RawGenericAddress {
     pub address: u64,
 }
 
+impl RawGenericAddress {
+    pub(crate) const fn is_empty(&self) -> bool {
+        self.address_space == 0
+            && self.bit_width == 0
+            && self.bit_offset == 0
+            && self.access_size == 0
+            && self.address == 0
+    }
+}
+
 #[derive(PartialEq, Eq, Clone, Copy, Debug)]
 pub enum AddressSpace {
     SystemMemory,

acpi/src/handler.rs

@@ -1,11 +1,10 @@
-use core::{ops::Deref, ptr::NonNull};
+use core::{fmt, ops::Deref, ptr::NonNull};
 
 /// Describes a physical mapping created by `AcpiHandler::map_physical_region` and unmapped by
 /// `AcpiHandler::unmap_physical_region`. The region mapped must be at least `size_of::<T>()`
 /// bytes, but may be bigger.
 ///
 /// See `PhysicalMapping::new` for the meaning of each field.
-#[derive(Debug)]
 pub struct PhysicalMapping<H, T>
 where
     H: AcpiHandler,
@@ -17,6 +16,21 @@ where
     handler: H,
 }
 
+impl<H, T> fmt::Debug for PhysicalMapping<H, T>
+where
+    H: AcpiHandler + fmt::Debug,
+{
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("PhysicalMapping")
+            .field("physical_start", &self.physical_start)
+            .field("virtual_start", &self.virtual_start)
+            .field("region_length", &self.region_length)
+            .field("mapped_length", &self.mapped_length)
+            .field("handler", &self.handler)
+            .finish()
+    }
+}
+
 impl<H, T> PhysicalMapping<H, T>
 where
     H: AcpiHandler,

acpi/src/lib.rs

@@ -75,6 +75,7 @@ pub mod madt;
 pub mod mcfg;
 pub mod rsdp;
 pub mod sdt;
+pub mod spcr;
 
 #[cfg(feature = "allocator_api")]
 mod managed_slice;
@@ -489,7 +490,7 @@ where
             };
             let r = header_mapping.validate(header_mapping.signature);
             if r.is_err() {
-                log::warn!("Found invalid SSDT at physical address {:p}: {:?}", table_phys_ptr, r);
+                log::warn!("Found invalid SDT at physical address {:p}: {:?}", table_phys_ptr, r);
                 continue;
             }
             let result = header_mapping.clone();

acpi/src/madt.rs

@@ -133,7 +133,9 @@ impl Madt {
                 MadtEntry::InterruptSourceOverride(_) => iso_count += 1,
                 MadtEntry::NmiSource(_) => nmi_source_count += 1,
                 MadtEntry::LocalApicNmi(_) => local_nmi_line_count += 1,
+                MadtEntry::X2ApicNmi(_) => local_nmi_line_count += 1,
                 MadtEntry::LocalApic(_) => processor_count += 1,
+                MadtEntry::LocalX2Apic(_) => processor_count += 1,
                 _ => (),
             }
         }

acpi/src/spcr.rs

@@ -0,0 +1,286 @@
+use crate::{
+    address::{GenericAddress, RawGenericAddress},
+    AcpiResult,
+    AcpiTable,
+    SdtHeader,
+    Signature,
+};
+use core::{
+    num::{NonZeroU32, NonZeroU8},
+    ptr,
+    slice,
+    str::{self, Utf8Error},
+};
+
+/// Serial Port Console Redirection (SPCR) Table.
+///
+/// The table provides information about the configuration and use of the
+/// serial port or non-legacy UART interface. On a system where the BIOS or
+/// system firmware uses the serial port for console input/output, this table
+/// should be used to convey information about the settings.
+///
+/// For more information, see [the official documentation](https://learn.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table).
+#[repr(C, packed)]
+#[derive(Debug)]
+pub struct Spcr {
+    pub header: SdtHeader,
+    interface_type: u8,
+    _reserved: [u8; 3],
+    base_address: RawGenericAddress,
+    interrupt_type: u8,
+    irq: u8,
+    global_system_interrupt: u32,
+    /// The baud rate the BIOS used for redirection.
+    configured_baud_rate: u8,
+    pub parity: u8,
+    pub stop_bits: u8,
+    flow_control: u8,
+    terminal_type: u8,
+    /// Language which the BIOS was redirecting. Must be 0.
+    pub language: u8,
+    pci_device_id: u16,
+    pci_vendor_id: u16,
+    pci_bus_number: u8,
+    pci_device_number: u8,
+    pci_function_number: u8,
+    pub pci_flags: u32,
+    /// PCI segment number. systems with fewer than 255 PCI buses, this number
+    /// will be 0.
+    pub pci_segment: u8,
+    uart_clock_freq: u32,
+    precise_baud_rate: u32,
+    namespace_string_length: u16,
+    namespace_string_offset: u16,
+}
+
+unsafe impl AcpiTable for Spcr {
+    const SIGNATURE: Signature = Signature::SPCR;
+
+    fn header(&self) -> &SdtHeader {
+        &self.header
+    }
+}
+
+impl Spcr {
+    /// Gets the type of the register interface.
+    pub fn interface_type(&self) -> SpcrInteraceType {
+        SpcrInteraceType::from(self.interface_type)
+    }
+
+    /// The base address of the Serial Port register set, if if console
+    /// redirection is enabled.
+    pub fn base_address(&self) -> Option<AcpiResult<GenericAddress>> {
+        (!self.base_address.is_empty()).then(|| GenericAddress::from_raw(self.base_address))
+    }
+
+    fn configured_baud_rate(&self) -> Option<NonZeroU32> {
+        match self.configured_baud_rate {
+            3 => unsafe { Some(NonZeroU32::new_unchecked(9600)) },
+            4 => unsafe { Some(NonZeroU32::new_unchecked(19200)) },
+            6 => unsafe { Some(NonZeroU32::new_unchecked(57600)) },
+            7 => unsafe { Some(NonZeroU32::new_unchecked(115200)) },
+            _ => None,
+        }
+    }
+
+    /// The baud rate the BIOS used for redirection, if configured.
+    pub fn baud_rate(&self) -> Option<NonZeroU32> {
+        NonZeroU32::new(self.precise_baud_rate).or_else(|| self.configured_baud_rate())
+    }
+
+    /// Flow control flags for the UART.
+    pub fn flow_control(&self) -> SpcrFlowControl {
+        SpcrFlowControl::from_bits_truncate(self.flow_control)
+    }
+
+    /// Interrupt type(s) used by the UART.
+    pub fn interrupt_type(&self) -> SpcrInterruptType {
+        SpcrInterruptType::from_bits_truncate(self.interrupt_type)
+    }
+
+    /// The PC-AT-compatible IRQ used by the UART, if the UART supports it.
+    /// Support is indicated by the [`interrupt_type`](Self::interrupt_type).
+    pub fn irq(&self) -> Option<u8> {
+        self.interrupt_type().contains(SpcrInterruptType::DUAL_8259).then_some(self.irq)
+    }
+
+    /// The Global System Interrupt (GSIV) used by the UART, if the UART
+    /// supports it. Support is indicated by the
+    /// [`interrupt_type`](Self::interrupt_type).
+    pub fn global_system_interrupt(&self) -> Option<u32> {
+        if self.interrupt_type().difference(SpcrInterruptType::DUAL_8259).is_empty() {
+            return None;
+        }
+        Some(self.global_system_interrupt)
+    }
+
+    /// The terminal protocol the BIOS was using for console redirection.
+    pub fn terminal_type(&self) -> SpcrTerminalType {
+        SpcrTerminalType::from_bits_truncate(self.terminal_type)
+    }
+
+    /// If the UART is a PCI device, returns its Device ID.
+    pub fn pci_device_id(&self) -> Option<u16> {
+        (self.pci_device_id != 0xffff).then_some(self.pci_device_id)
+    }
+
+    /// If the UART is a PCI device, returns its Vendor ID.
+    pub fn pci_vendor_id(&self) -> Option<u16> {
+        (self.pci_vendor_id != 0xffff).then_some(self.pci_vendor_id)
+    }
+
+    /// If the UART is a PCI device, returns its bus number.
+    pub fn pci_bus_number(&self) -> Option<NonZeroU8> {
+        NonZeroU8::new(self.pci_bus_number)
+    }
+
+    /// If the UART is a PCI device, returns its device number.
+    pub fn pci_device_number(&self) -> Option<NonZeroU8> {
+        NonZeroU8::new(self.pci_device_number)
+    }
+
+    /// If the UART is a PCI device, returns its function number.
+    pub fn pci_function_number(&self) -> Option<NonZeroU8> {
+        NonZeroU8::new(self.pci_function_number)
+    }
+
+    /// The UART clock frequency in Hz, if it can be determined.
+    pub const fn uart_clock_frequency(&self) -> Option<NonZeroU32> {
+        if self.header.revision <= 2 {
+            return None;
+        }
+        NonZeroU32::new(self.uart_clock_freq)
+    }
+
+    /// An ASCII string to uniquely identify this device. This string consists
+    /// of a fully qualified reference to the object that represents this
+    /// device in the ACPI namespace. If no namespace device exists,
+    /// the namespace string must only contain a single '.'.
+    pub fn namespace_string(&self) -> Result<&str, Utf8Error> {
+        let start = ptr::from_ref(self).cast::<u8>();
+        let bytes = unsafe {
+            let str_start = start.add(self.namespace_string_offset as usize);
+            slice::from_raw_parts(str_start, self.namespace_string_length as usize)
+        };
+        str::from_utf8(bytes)
+    }
+}
+
+bitflags::bitflags! {
+    /// Interrupt type(s) used by an UART.
+    #[derive(Clone, Copy, Debug)]
+    pub struct SpcrInterruptType: u8 {
+        /// PC-AT-compatible dual-8259 IRQ interrupt.
+        const DUAL_8259 = 1 << 0;
+        /// I/O APIC interrupt (Global System Interrupt).
+        const IO_APIC = 1 << 1;
+        /// I/O SAPIC interrupt (Global System Interrupt).
+        const IO_SAPIC = 1 << 2;
+        /// ARMH GIC interrupt (Global System Interrupt).
+        const ARMH_GIC = 1 << 3;
+        /// RISC-V PLIC/APLIC interrupt (Global System Interrupt).
+        const RISCV_PLIC = 1 << 4;
+    }
+}
+
+bitflags::bitflags! {
+    /// The terminal protocol the BIOS uses for console redirection.
+    #[derive(Clone, Copy, Debug)]
+    pub struct SpcrTerminalType: u8 {
+        const VT1000 = 1 << 0;
+        const EXTENDED_VT1000 = 1 << 1;
+        const VT_UTF8 = 1 << 2;
+        const ANSI = 1 << 3;
+    }
+}
+
+bitflags::bitflags! {
+    /// Flow control flags for the UART.
+    #[derive(Clone, Copy, Debug)]
+    pub struct SpcrFlowControl: u8 {
+        /// DCD required for transmit
+        const DCD = 1 << 0;
+        /// RTS/CTS hardware flow control
+        const RTS_CTS = 1 << 1;
+        /// XON/XOFF software control
+        const XON_XOFF = 1 << 2;
+    }
+}
+
+#[repr(u8)]
+#[derive(Clone, Copy, Debug)]
+pub enum SpcrInteraceType {
+    /// Full 16550 interface
+    Full16550,
+    /// Full 16450 interface (must also accept writing to the 16550 FCR register).
+    Full16450,
+    /// MAX311xE SPI UART
+    MAX311xE,
+    /// Arm PL011 UART
+    ArmPL011,
+    /// MSM8x60 (e.g. 8960)
+    MSM8x60,
+    /// Nvidia 16550
+    Nvidia16550,
+    /// TI OMAP
+    TiOmap,
+    /// APM88xxxx
+    APM88xxxx,
+    /// MSM8974
+    Msm8974,
+    /// SAM5250
+    Sam5250,
+    /// Intel USIF
+    IntelUSIF,
+    /// i.MX 6
+    Imx6,
+    /// (deprecated) Arm SBSA (2.x only) Generic UART supporting only 32-bit accesses
+    ArmSBSAGeneric32bit,
+    /// Arm SBSA Generic UART
+    ArmSBSAGeneric,
+    /// Arm DCC
+    ArmDCC,
+    /// VCM2835
+    Bcm2835,
+    /// SDM845 with clock rate of 1.8432 MHz
+    Sdm845_18432,
+    /// 16550-compatible with parameters defined in Generic Address Structure
+    Generic16550,
+    /// SDM845 with clock rate of 7.372 MHz
+    Sdm845_7372,
+    /// Intel LPSS
+    IntelLPSS,
+    /// RISC-V SBI console (any supported SBI mechanism)
+    RiscVSbi,
+    /// Unknown interface
+    Unknown(u8),
+}
+
+impl From<u8> for SpcrInteraceType {
+    fn from(val: u8) -> Self {
+        match val {
+            0x00 => Self::Full16550,
+            0x01 => Self::Full16450,
+            0x02 => Self::MAX311xE,
+            0x03 => Self::ArmPL011,
+            0x04 => Self::MSM8x60,
+            0x05 => Self::Nvidia16550,
+            0x06 => Self::TiOmap,
+            0x08 => Self::APM88xxxx,
+            0x09 => Self::Msm8974,
+            0x0A => Self::Sam5250,
+            0x0B => Self::IntelUSIF,
+            0x0C => Self::Imx6,
+            0x0D => Self::ArmSBSAGeneric32bit,
+            0x0E => Self::ArmSBSAGeneric,
+            0x0F => Self::ArmDCC,
+            0x10 => Self::Bcm2835,
+            0x11 => Self::Sdm845_18432,
+            0x12 => Self::Generic16550,
+            0x13 => Self::Sdm845_7372,
+            0x14 => Self::IntelLPSS,
+            0x15 => Self::RiscVSbi,
+            _ => Self::Unknown(val),
+        }
+    }
+}

aml/Cargo.toml

@@ -7,7 +7,7 @@ description = "Library for parsing AML"
 categories = ["hardware-support", "no-std"]
 readme = "../README.md"
 license = "MIT/Apache-2.0"
-edition = "2018"
+edition = "2021"
 
 [dependencies]
 log = "0.4"

aml/src/expression.rs

@@ -15,7 +15,7 @@ use alloc::{
     vec,
     vec::Vec,
 };
-use core::{cmp::Ordering, convert::TryInto, mem, ops::Deref};
+use core::{cmp::Ordering, mem, ops::Deref};
 
 pub fn expression_opcode<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
 where
@@ -331,7 +331,7 @@ where
             DebugVerbosity::AllScopes,
             "DefIncrement",
             super_name().map_with_context(|addend, context| {
-                let value = try_with_context!(context, context.read_target(&addend));
+                let value = try_with_context!(context, context.read_target(&addend)).clone();
                 let value = try_with_context!(context, value.as_integer(context));
                 let new_value = AmlValue::Integer(value + 1);
                 try_with_context!(context, context.store(addend, new_value.clone()));
@@ -353,7 +353,7 @@ where
             DebugVerbosity::AllScopes,
             "DefDecrement",
             super_name().map_with_context(|minuend, context| {
-                let value = try_with_context!(context, context.read_target(&minuend));
+                let value = try_with_context!(context, context.read_target(&minuend)).clone();
                 let value = try_with_context!(context, value.as_integer(context));
                 let new_value = AmlValue::Integer(value - 1);
                 try_with_context!(context, context.store(minuend, new_value.clone()));
@@ -376,8 +376,8 @@ where
             DebugVerbosity::AllScopes,
             "DefLOr",
             term_arg().then(term_arg()).map_with_context(|(left_arg, right_arg), context| {
-                let left = try_with_context!(context, left_arg.as_bool());
+                let left = try_with_context!(context, left_arg.as_bool(context));
-                let right = try_with_context!(context, right_arg.as_bool());
+                let right = try_with_context!(context, right_arg.as_bool(context));
                 (Ok(AmlValue::Boolean(left && right)), context)
             }),
         ))
@@ -397,8 +397,8 @@ where
             DebugVerbosity::AllScopes,
             "DefLOr",
             term_arg().then(term_arg()).map_with_context(|(left_arg, right_arg), context| {
-                let left = try_with_context!(context, left_arg.as_bool());
+                let left = try_with_context!(context, left_arg.as_bool(context));
-                let right = try_with_context!(context, right_arg.as_bool());
+                let right = try_with_context!(context, right_arg.as_bool(context));
                 (Ok(AmlValue::Boolean(left || right)), context)
             }),
         ))
@@ -418,7 +418,7 @@ where
             DebugVerbosity::AllScopes,
             "DefLNot",
             term_arg().map_with_context(|arg, context| {
-                let operand = try_with_context!(context, arg.as_bool());
+                let operand = try_with_context!(context, arg.as_bool(context));
                 (Ok(AmlValue::Boolean(!operand)), context)
             }),
         ))
@@ -812,6 +812,9 @@ where
                 AmlValue::Buffer(data) => {
                     AmlValue::Integer(try_with_context!(context, AmlValue::Buffer(data).as_integer(context)))
                 }
+                AmlValue::Field { .. } => {
+                    AmlValue::Integer(try_with_context!(context, operand.as_integer(context)))
+                }
                 AmlValue::String(string) => AmlValue::Integer(try_with_context!(
                     context,
                     if string.starts_with("0x") {

aml/src/lib.rs

@@ -50,6 +50,7 @@ pub(crate) mod misc;
 pub(crate) mod name_object;
 pub(crate) mod namespace;
 pub(crate) mod opcode;
+pub mod opregion;
 pub(crate) mod parser;
 pub mod pci_routing;
 pub(crate) mod pkg_length;
@@ -60,7 +61,11 @@ pub mod value;
 
 pub use crate::{namespace::*, value::AmlValue};
 
-use alloc::{boxed::Box, string::ToString};
+use alloc::{
+    boxed::Box,
+    format,
+    string::{String, ToString},
+};
 use core::mem;
 use log::{error, warn};
 use misc::{ArgNum, LocalNum};
@@ -143,6 +148,29 @@ impl AmlContext {
     }
 
     pub fn parse_table(&mut self, stream: &[u8]) -> Result<(), AmlError> {
+        fn stream_context(stream: &[u8], err_buf: &[u8]) -> String {
+            const BEFORE_LEN: usize = 4;
+            const ABBREV_LEN: usize = 4;
+            let abbreviated = if err_buf.len() >= ABBREV_LEN { &err_buf[..ABBREV_LEN] } else { err_buf };
+
+            if let Some(position) = (err_buf.as_ptr() as usize).checked_sub(stream.as_ptr() as usize) {
+                if position <= stream.len() {
+                    let before = if position > BEFORE_LEN {
+                        &stream[position - BEFORE_LEN..position]
+                    } else {
+                        &stream[..position]
+                    };
+                    return format!(
+                        "position {:#X}: preceding {:X?}, buf {:X?}",
+                        position + 36,
+                        before,
+                        abbreviated
+                    );
+                }
+            }
+            format!("buf {:X?}", abbreviated)
+        }
+
         if stream.len() == 0 {
             return Err(AmlError::UnexpectedEndOfStream);
         }
@@ -150,8 +178,8 @@ impl AmlContext {
         let table_length = PkgLength::from_raw_length(stream, stream.len() as u32).unwrap();
         match term_object::term_list(table_length).parse(stream, self) {
             Ok(_) => Ok(()),
-            Err((_, _, Propagate::Err(err))) => {
+            Err((err_buf, _, Propagate::Err(err))) => {
-                error!("Failed to parse AML stream. Err = {:?}", err);
+                error!("Failed to parse AML stream. Err = {:?}, {}", err, stream_context(stream, err_buf));
                 Err(err)
             }
             Err((_, _, other)) => {
@@ -387,186 +415,6 @@ impl AmlContext {
         }
     }
 
-    /// Read from an operation-region, performing only standard-sized reads (supported powers-of-2 only. If a field
-    /// is not one of these sizes, it may need to be masked, or multiple reads may need to be performed).
-    pub(crate) fn read_region(&self, region_handle: AmlHandle, offset: u64, length: u64) -> Result<u64, AmlError> {
-        use bit_field::BitField;
-        use core::convert::TryInto;
-        use value::RegionSpace;
-
-        let (region_space, region_base, _region_length, parent_device) = {
-            if let AmlValue::OpRegion { region, offset, length, parent_device } =
-                self.namespace.get(region_handle)?
-            {
-                (region, offset, length, parent_device)
-            } else {
-                return Err(AmlError::FieldRegionIsNotOpRegion);
-            }
-        };
-
-        match region_space {
-            RegionSpace::SystemMemory => {
-                let address = (region_base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
-                match length {
-                    8 => Ok(self.handler.read_u8(address) as u64),
-                    16 => Ok(self.handler.read_u16(address) as u64),
-                    32 => Ok(self.handler.read_u32(address) as u64),
-                    64 => Ok(self.handler.read_u64(address)),
-                    _ => Err(AmlError::FieldInvalidAccessSize),
-                }
-            }
-
-            RegionSpace::SystemIo => {
-                let port = (region_base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
-                match length {
-                    8 => Ok(self.handler.read_io_u8(port) as u64),
-                    16 => Ok(self.handler.read_io_u16(port) as u64),
-                    32 => Ok(self.handler.read_io_u32(port) as u64),
-                    _ => Err(AmlError::FieldInvalidAccessSize),
-                }
-            }
-
-            RegionSpace::PciConfig => {
-                /*
-                 * First, we need to get some extra information out of objects in the parent object. Both
-                 * `_SEG` and `_BBN` seem optional, with defaults that line up with legacy PCI implementations
-                 * (e.g. systems with a single segment group and a single root, respectively).
-                 */
-                let parent_device = parent_device.as_ref().unwrap();
-                let seg = match self.namespace.search(&AmlName::from_str("_SEG").unwrap(), parent_device) {
-                    Ok((_, handle)) => self
-                        .namespace
-                        .get(handle)?
-                        .as_integer(self)?
-                        .try_into()
-                        .map_err(|_| AmlError::FieldInvalidAddress)?,
-                    Err(AmlError::ValueDoesNotExist(_)) => 0,
-                    Err(err) => return Err(err),
-                };
-                let bbn = match self.namespace.search(&AmlName::from_str("_BBN").unwrap(), parent_device) {
-                    Ok((_, handle)) => self
-                        .namespace
-                        .get(handle)?
-                        .as_integer(self)?
-                        .try_into()
-                        .map_err(|_| AmlError::FieldInvalidAddress)?,
-                    Err(AmlError::ValueDoesNotExist(_)) => 0,
-                    Err(err) => return Err(err),
-                };
-                let adr = {
-                    let (_, handle) = self.namespace.search(&AmlName::from_str("_ADR").unwrap(), parent_device)?;
-                    self.namespace.get(handle)?.as_integer(self)?
-                };
-
-                let device = adr.get_bits(16..24) as u8;
-                let function = adr.get_bits(0..8) as u8;
-                let offset = (region_base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
-
-                match length {
-                    8 => Ok(self.handler.read_pci_u8(seg, bbn, device, function, offset) as u64),
-                    16 => Ok(self.handler.read_pci_u16(seg, bbn, device, function, offset) as u64),
-                    32 => Ok(self.handler.read_pci_u32(seg, bbn, device, function, offset) as u64),
-                    _ => Err(AmlError::FieldInvalidAccessSize),
-                }
-            }
-
-            // TODO
-            _ => unimplemented!(),
-        }
-    }
-
-    pub(crate) fn write_region(
-        &mut self,
-        region_handle: AmlHandle,
-        offset: u64,
-        length: u64,
-        value: u64,
-    ) -> Result<(), AmlError> {
-        use bit_field::BitField;
-        use core::convert::TryInto;
-        use value::RegionSpace;
-
-        let (region_space, region_base, _region_length, parent_device) = {
-            if let AmlValue::OpRegion { region, offset, length, parent_device } =
-                self.namespace.get(region_handle)?
-            {
-                (region, offset, length, parent_device)
-            } else {
-                return Err(AmlError::FieldRegionIsNotOpRegion);
-            }
-        };
-
-        match region_space {
-            RegionSpace::SystemMemory => {
-                let address = (region_base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
-                match length {
-                    8 => Ok(self.handler.write_u8(address, value as u8)),
-                    16 => Ok(self.handler.write_u16(address, value as u16)),
-                    32 => Ok(self.handler.write_u32(address, value as u32)),
-                    64 => Ok(self.handler.write_u64(address, value)),
-                    _ => Err(AmlError::FieldInvalidAccessSize),
-                }
-            }
-
-            RegionSpace::SystemIo => {
-                let port = (region_base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
-                match length {
-                    8 => Ok(self.handler.write_io_u8(port, value as u8)),
-                    16 => Ok(self.handler.write_io_u16(port, value as u16)),
-                    32 => Ok(self.handler.write_io_u32(port, value as u32)),
-                    _ => Err(AmlError::FieldInvalidAccessSize),
-                }
-            }
-
-            RegionSpace::PciConfig => {
-                /*
-                 * First, we need to get some extra information out of objects in the parent object. Both
-                 * `_SEG` and `_BBN` seem optional, with defaults that line up with legacy PCI implementations
-                 * (e.g. systems with a single segment group and a single root, respectively).
-                 */
-                let parent_device = parent_device.as_ref().unwrap();
-                let seg = match self.namespace.search(&AmlName::from_str("_SEG").unwrap(), parent_device) {
-                    Ok((_, handle)) => self
-                        .namespace
-                        .get(handle)?
-                        .as_integer(self)?
-                        .try_into()
-                        .map_err(|_| AmlError::FieldInvalidAddress)?,
-                    Err(AmlError::ValueDoesNotExist(_)) => 0,
-                    Err(err) => return Err(err),
-                };
-                let bbn = match self.namespace.search(&AmlName::from_str("_BBN").unwrap(), parent_device) {
-                    Ok((_, handle)) => self
-                        .namespace
-                        .get(handle)?
-                        .as_integer(self)?
-                        .try_into()
-                        .map_err(|_| AmlError::FieldInvalidAddress)?,
-                    Err(AmlError::ValueDoesNotExist(_)) => 0,
-                    Err(err) => return Err(err),
-                };
-                let adr = {
-                    let (_, handle) = self.namespace.search(&AmlName::from_str("_ADR").unwrap(), parent_device)?;
-                    self.namespace.get(handle)?.as_integer(self)?
-                };
-
-                let device = adr.get_bits(16..24) as u8;
-                let function = adr.get_bits(0..8) as u8;
-                let offset = (region_base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
-
-                match length {
-                    8 => Ok(self.handler.write_pci_u8(seg, bbn, device, function, offset, value as u8)),
-                    16 => Ok(self.handler.write_pci_u16(seg, bbn, device, function, offset, value as u16)),
-                    32 => Ok(self.handler.write_pci_u32(seg, bbn, device, function, offset, value as u32)),
-                    _ => Err(AmlError::FieldInvalidAccessSize),
-                }
-            }
-
-            // TODO
-            _ => unimplemented!(),
-        }
-    }
-
     fn add_predefined_objects(&mut self) {
         /*
          * These are the scopes predefined by the spec. Some tables will try to access them without defining them
@@ -605,7 +453,8 @@ impl AmlContext {
             .add_value(
                 AmlName::from_str("\\_OSI").unwrap(),
                 AmlValue::native_method(1, false, 0, |context| {
-                    Ok(match context.current_arg(0)?.as_string(context)?.as_str() {
+                    let value = context.current_arg(0)?.clone();
+                    Ok(match value.as_string(context)?.as_str() {
                         "Windows 2000" => true,       // 2000
                         "Windows 2001" => true,       // XP
                         "Windows 2001 SP1" => true,   // XP SP1

aml/src/opregion.rs

@@ -0,0 +1,270 @@
+use crate::{
+    value::{Args, FieldAccessType, FieldFlags, FieldUpdateRule},
+    AmlContext,
+    AmlError,
+    AmlName,
+    AmlValue,
+};
+use bit_field::BitField;
+
+#[derive(Clone, Debug)]
+pub struct OpRegion {
+    region: RegionSpace,
+    base: u64,
+    length: u64,
+    parent_device: Option<AmlName>,
+}
+
+impl OpRegion {
+    pub fn new(region: RegionSpace, base: u64, length: u64, parent_device: Option<AmlName>) -> OpRegion {
+        OpRegion { region, base, length, parent_device }
+    }
+
+    /// Get the length of this op-region, in **bits**.
+    pub fn length(&self) -> u64 {
+        self.length
+    }
+
+    /// Read a field from this op-region. This has looser requirements than `read`, and will
+    /// perform multiple standard-sized reads and mask the result as required.
+    pub fn read_field(
+        &self,
+        offset: u64,
+        length: u64,
+        flags: FieldFlags,
+        context: &mut AmlContext,
+    ) -> Result<AmlValue, AmlError> {
+        let _max_access_size = match self.region {
+            RegionSpace::SystemMemory => 64,
+            RegionSpace::SystemIo | RegionSpace::PciConfig => 32,
+            _ => unimplemented!(),
+        };
+        let minimum_access_size = match flags.access_type()? {
+            FieldAccessType::Any => 8,
+            FieldAccessType::Byte => 8,
+            FieldAccessType::Word => 16,
+            FieldAccessType::DWord => 32,
+            FieldAccessType::QWord => 64,
+            FieldAccessType::Buffer => 8, // TODO
+        };
+
+        /*
+         * Find the access size, as either the minimum access size allowed by the region, or the field length
+         * rounded up to the next power-of-2, whichever is larger.
+         */
+        let access_size = u64::max(minimum_access_size, length.next_power_of_two());
+
+        /*
+         * TODO: we need to decide properly how to read from the region itself. Complications:
+         *    - if the region has a minimum access size greater than the desired length, we need to read the
+         *      minimum and mask it (reading a byte from a WordAcc region)
+         *    - if the desired length is larger than we can read, we need to do multiple reads
+         */
+        let value = self.read(offset, access_size, context)?.get_bits(0..(length as usize));
+        Ok(AmlValue::Integer(value))
+    }
+
+    pub fn write_field(
+        &self,
+        offset: u64,
+        length: u64,
+        flags: FieldFlags,
+        value: AmlValue,
+        context: &mut AmlContext,
+    ) -> Result<(), AmlError> {
+        /*
+         * If the field's update rule is `Preserve`, we need to read the initial value of the field, so we can
+         * overwrite the correct bits. We destructure the field to do the actual write, so we read from it if
+         * needed here, otherwise the borrow-checker doesn't understand.
+         */
+        let mut field_value = match flags.field_update_rule()? {
+            FieldUpdateRule::Preserve => self.read_field(offset, length, flags, context)?.as_integer(context)?,
+            FieldUpdateRule::WriteAsOnes => 0xffffffff_ffffffff,
+            FieldUpdateRule::WriteAsZeros => 0x0,
+        };
+
+        let _maximum_access_size = match self.region {
+            RegionSpace::SystemMemory => 64,
+            RegionSpace::SystemIo | RegionSpace::PciConfig => 32,
+            _ => unimplemented!(),
+        };
+        let minimum_access_size = match flags.access_type()? {
+            FieldAccessType::Any => 8,
+            FieldAccessType::Byte => 8,
+            FieldAccessType::Word => 16,
+            FieldAccessType::DWord => 32,
+            FieldAccessType::QWord => 64,
+            FieldAccessType::Buffer => 8, // TODO
+        };
+
+        /*
+         * Find the access size, as either the minimum access size allowed by the region, or the field length
+         * rounded up to the next power-of-2, whichever is larger.
+         */
+        let access_size = u64::max(minimum_access_size, length.next_power_of_two());
+
+        field_value.set_bits(0..(length as usize), value.as_integer(context)?);
+        self.write(offset, access_size, field_value, context)
+    }
+
+    /// Perform a standard-size read from this op-region. `length` must be a supported power-of-2,
+    /// and `offset` correctly aligned for that `length`. `value` must be appropriately sized.
+    pub fn read(&self, offset: u64, length: u64, context: &mut AmlContext) -> Result<u64, AmlError> {
+        match self.region {
+            RegionSpace::SystemMemory => {
+                let address = (self.base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
+                match length {
+                    8 => Ok(context.handler.read_u8(address) as u64),
+                    16 => Ok(context.handler.read_u16(address) as u64),
+                    32 => Ok(context.handler.read_u32(address) as u64),
+                    64 => Ok(context.handler.read_u64(address)),
+                    _ => Err(AmlError::FieldInvalidAccessSize),
+                }
+            }
+
+            RegionSpace::SystemIo => {
+                let port = (self.base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
+                match length {
+                    8 => Ok(context.handler.read_io_u8(port) as u64),
+                    16 => Ok(context.handler.read_io_u16(port) as u64),
+                    32 => Ok(context.handler.read_io_u32(port) as u64),
+                    _ => Err(AmlError::FieldInvalidAccessSize),
+                }
+            }
+
+            RegionSpace::PciConfig => {
+                /*
+                 * First, we need to get some extra information out of objects in the parent object. Both
+                 * `_SEG` and `_BBN` seem optional, with defaults that line up with legacy PCI implementations
+                 * (e.g. systems with a single segment group and a single root, respectively).
+                 */
+                let parent_device = self.parent_device.as_ref().unwrap();
+                let seg = match context.invoke_method(
+                    &AmlName::from_str("_SEG").unwrap().resolve(parent_device).unwrap(),
+                    Args::EMPTY,
+                ) {
+                    Ok(seg) => seg.as_integer(context)?.try_into().map_err(|_| AmlError::FieldInvalidAddress)?,
+                    Err(AmlError::ValueDoesNotExist(_)) => 0,
+                    Err(err) => return Err(err),
+                };
+                let bbn = match context.invoke_method(
+                    &AmlName::from_str("_BBN").unwrap().resolve(parent_device).unwrap(),
+                    Args::EMPTY,
+                ) {
+                    Ok(bbn) => bbn.as_integer(context)?.try_into().map_err(|_| AmlError::FieldInvalidAddress)?,
+                    Err(AmlError::ValueDoesNotExist(_)) => 0,
+                    Err(err) => return Err(err),
+                };
+                let adr = {
+                    let adr = context.invoke_method(
+                        &AmlName::from_str("_ADR").unwrap().resolve(parent_device).unwrap(),
+                        Args::EMPTY,
+                    )?;
+                    adr.as_integer(context)?
+                };
+
+                let device = adr.get_bits(16..24) as u8;
+                let function = adr.get_bits(0..8) as u8;
+                let offset = (self.base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
+
+                match length {
+                    8 => Ok(context.handler.read_pci_u8(seg, bbn, device, function, offset) as u64),
+                    16 => Ok(context.handler.read_pci_u16(seg, bbn, device, function, offset) as u64),
+                    32 => Ok(context.handler.read_pci_u32(seg, bbn, device, function, offset) as u64),
+                    _ => Err(AmlError::FieldInvalidAccessSize),
+                }
+            }
+
+            // TODO
+            _ => unimplemented!(),
+        }
+    }
+
+    /// Perform a standard-size write to this op-region. `length` must be a supported power-of-2,
+    /// and `offset` correctly aligned for that `length`. `value` must be appropriately sized.
+    pub fn write(&self, offset: u64, length: u64, value: u64, context: &mut AmlContext) -> Result<(), AmlError> {
+        match self.region {
+            RegionSpace::SystemMemory => {
+                let address = (self.base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
+                match length {
+                    8 => Ok(context.handler.write_u8(address, value as u8)),
+                    16 => Ok(context.handler.write_u16(address, value as u16)),
+                    32 => Ok(context.handler.write_u32(address, value as u32)),
+                    64 => Ok(context.handler.write_u64(address, value)),
+                    _ => Err(AmlError::FieldInvalidAccessSize),
+                }
+            }
+
+            RegionSpace::SystemIo => {
+                let port = (self.base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
+                match length {
+                    8 => Ok(context.handler.write_io_u8(port, value as u8)),
+                    16 => Ok(context.handler.write_io_u16(port, value as u16)),
+                    32 => Ok(context.handler.write_io_u32(port, value as u32)),
+                    _ => Err(AmlError::FieldInvalidAccessSize),
+                }
+            }
+
+            RegionSpace::PciConfig => {
+                /*
+                 * First, we need to get some extra information out of objects in the parent object. Both
+                 * `_SEG` and `_BBN` seem optional, with defaults that line up with legacy PCI implementations
+                 * (e.g. systems with a single segment group and a single root, respectively).
+                 */
+                let parent_device = self.parent_device.as_ref().unwrap();
+                let seg = match context.invoke_method(
+                    &AmlName::from_str("_SEG").unwrap().resolve(parent_device).unwrap(),
+                    Args::EMPTY,
+                ) {
+                    Ok(seg) => seg.as_integer(context)?.try_into().map_err(|_| AmlError::FieldInvalidAddress)?,
+                    Err(AmlError::ValueDoesNotExist(_)) => 0,
+                    Err(err) => return Err(err),
+                };
+                let bbn = match context.invoke_method(
+                    &AmlName::from_str("_BBN").unwrap().resolve(parent_device).unwrap(),
+                    Args::EMPTY,
+                ) {
+                    Ok(bbn) => bbn.as_integer(context)?.try_into().map_err(|_| AmlError::FieldInvalidAddress)?,
+                    Err(AmlError::ValueDoesNotExist(_)) => 0,
+                    Err(err) => return Err(err),
+                };
+                let adr = {
+                    let adr = context.invoke_method(
+                        &AmlName::from_str("_ADR").unwrap().resolve(parent_device).unwrap(),
+                        Args::EMPTY,
+                    )?;
+                    adr.as_integer(context)?
+                };
+
+                let device = adr.get_bits(16..24) as u8;
+                let function = adr.get_bits(0..8) as u8;
+                let offset = (self.base + offset).try_into().map_err(|_| AmlError::FieldInvalidAddress)?;
+
+                match length {
+                    8 => Ok(context.handler.write_pci_u8(seg, bbn, device, function, offset, value as u8)),
+                    16 => Ok(context.handler.write_pci_u16(seg, bbn, device, function, offset, value as u16)),
+                    32 => Ok(context.handler.write_pci_u32(seg, bbn, device, function, offset, value as u32)),
+                    _ => Err(AmlError::FieldInvalidAccessSize),
+                }
+            }
+
+            // TODO
+            _ => unimplemented!(),
+        }
+    }
+}
+
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub enum RegionSpace {
+    SystemMemory,
+    SystemIo,
+    PciConfig,
+    EmbeddedControl,
+    SMBus,
+    SystemCmos,
+    PciBarTarget,
+    IPMI,
+    GeneralPurposeIo,
+    GenericSerialBus,
+    OemDefined(u8),
+}

aml/src/parser.rs

@@ -1,6 +1,6 @@
 use crate::{pkg_length::PkgLength, AmlContext, AmlError, AmlValue, DebugVerbosity};
 use alloc::vec::Vec;
-use core::{convert::TryInto, marker::PhantomData};
+use core::marker::PhantomData;
 use log::trace;
 
 /// This is the number of spaces added to indent a scope when printing parser debug messages.

aml/src/pci_routing.rs

@@ -9,7 +9,6 @@ use crate::{
 };
 use alloc::vec::Vec;
 use bit_field::BitField;
-use core::convert::TryInto;
 
 pub use crate::resource::IrqDescriptor;
 

aml/src/pkg_length.rs

@@ -74,7 +74,7 @@ where
          * OperationRegion length is in bytes, PkgLength is in bits, so conversion is needed
          */
         let region_bit_length = match region_value {
-            AmlValue::OpRegion { length, .. } => *length * 8,
+            AmlValue::OpRegion(region) => region.length() * 8,
             _ => return Err((input, context, Propagate::Err(AmlError::FieldRegionIsNotOpRegion))),
         };
 

aml/src/statement.rs

@@ -141,8 +141,12 @@ where
             pkg_length()
                 .then(term_arg())
                 .feed(|(length, predicate_arg)| {
-                    take_to_end_of_pkglength(length)
+                    take_to_end_of_pkglength(length).map_with_context(move |then_branch, context| {
-                        .map(move |then_branch| Ok((predicate_arg.as_bool()?, then_branch)))
+                        match predicate_arg.as_bool(context) {
+                            Ok(pred_val) => (Ok((pred_val, then_branch)), context),
+                            Err(e) => (Err(Propagate::Err(e)), context),
+                        }
+                    })
                 })
                 .then(choice!(
                     maybe_else_opcode
@@ -224,7 +228,7 @@ where
             DebugVerbosity::Scopes,
             "DefSleep",
             term_arg().map_with_context(|milliseconds, context| {
-                let milliseconds = try_with_context!(context, milliseconds.as_integer(&context));
+                let milliseconds = try_with_context!(context, milliseconds.as_integer(context));
                 context.handler.sleep(milliseconds);
                 (Ok(()), context)
             }),
@@ -245,7 +249,7 @@ where
             DebugVerbosity::Scopes,
             "DefStall",
             term_arg().map_with_context(|microseconds, context| {
-                let microseconds = try_with_context!(context, microseconds.as_integer(&context));
+                let microseconds = try_with_context!(context, microseconds.as_integer(context));
                 context.handler.stall(microseconds);
                 (Ok(()), context)
             }),
@@ -276,7 +280,7 @@ where
                         .map(move |body| Ok((first_predicate.clone(), predicate_stream, body)))
                 })
                 .map_with_context(|(first_predicate, predicate_stream, body), mut context| {
-                    if !try_with_context!(context, first_predicate.as_bool()) {
+                    if !try_with_context!(context, first_predicate.as_bool(context)) {
                         return (Ok(()), context);
                     }
 
@@ -307,7 +311,7 @@ where
                             {
                                 Ok((_, new_context, result)) => {
                                     context = new_context;
-                                    try_with_context!(context, result.as_bool())
+                                    try_with_context!(context, result.as_bool(context))
                                 }
                                 Err((_, context, err)) => return (Err(err), context),
                             };

aml/src/term_object.rs

@@ -4,6 +4,7 @@ use crate::{
     name_object::{name_seg, name_string, target, Target},
     namespace::{AmlName, LevelType},
     opcode::{self, ext_opcode, opcode},
+    opregion::{OpRegion, RegionSpace},
     parser::{
         choice,
         comment_scope,
@@ -19,7 +20,7 @@ use crate::{
     },
     pkg_length::{pkg_length, region_pkg_length, PkgLength},
     statement::statement_opcode,
-    value::{AmlValue, FieldFlags, MethodCode, MethodFlags, RegionSpace},
+    value::{AmlValue, FieldFlags, MethodCode, MethodFlags},
     AmlContext,
     AmlError,
     AmlHandle,
@@ -474,7 +475,7 @@ where
                         context.namespace.add_value_at_resolved_path(
                             name,
                             &context.current_scope,
-                            AmlValue::OpRegion { region, offset, length, parent_device }
+                            AmlValue::OpRegion(OpRegion::new(region, offset, length, parent_device))
                         )
                     );
                     (Ok(()), context)

aml/src/test_utils.rs

@@ -50,22 +50,22 @@ impl Handler for TestHandler {
         unimplemented!()
     }
 
-    fn read_pci_u8(&self, _segment: u16, _bus: u8, device: u8, _function: u8, _offset: u16) -> u8 {
+    fn read_pci_u8(&self, _segment: u16, _bus: u8, _device: u8, _function: u8, _offset: u16) -> u8 {
         unimplemented!()
     }
-    fn read_pci_u16(&self, _segment: u16, _bus: u8, device: u8, _function: u8, _offset: u16) -> u16 {
+    fn read_pci_u16(&self, _segment: u16, _bus: u8, _device: u8, _function: u8, _offset: u16) -> u16 {
         unimplemented!()
     }
-    fn read_pci_u32(&self, _segment: u16, _bus: u8, device: u8, _function: u8, _offset: u16) -> u32 {
+    fn read_pci_u32(&self, _segment: u16, _bus: u8, _device: u8, _function: u8, _offset: u16) -> u32 {
         unimplemented!()
     }
-    fn write_pci_u8(&self, _segment: u16, _bus: u8, device: u8, _function: u8, _offset: u16, _value: u8) {
+    fn write_pci_u8(&self, _segment: u16, _bus: u8, _device: u8, _function: u8, _offset: u16, _value: u8) {
         unimplemented!()
     }
-    fn write_pci_u16(&self, _segment: u16, _bus: u8, device: u8, _function: u8, _offset: u16, _value: u16) {
+    fn write_pci_u16(&self, _segment: u16, _bus: u8, _device: u8, _function: u8, _offset: u16, _value: u16) {
         unimplemented!()
     }
-    fn write_pci_u32(&self, _segment: u16, _bus: u8, device: u8, _function: u8, _offset: u16, _value: u32) {
+    fn write_pci_u32(&self, _segment: u16, _bus: u8, _device: u8, _function: u8, _offset: u16, _value: u32) {
         unimplemented!()
     }
     fn stall(&self, _microseconds: u64) {
@@ -141,15 +141,8 @@ pub(crate) fn crudely_cmp_values(a: &AmlValue, b: &AmlValue) -> bool {
             AmlValue::String(ref b) => a == b,
             _ => false,
         },
-        AmlValue::OpRegion { region, offset, length, parent_device } => match b {
+        AmlValue::OpRegion(_) => match b {
-            AmlValue::OpRegion {
+            AmlValue::OpRegion(_) => panic!("Can't compare two op-regions"),
-                region: b_region,
-                offset: b_offset,
-                length: b_length,
-                parent_device: b_parent_device,
-            } => {
-                region == b_region && offset == b_offset && length == b_length && parent_device == b_parent_device
-            }
             _ => false,
         },
         AmlValue::Field { region, flags, offset, length } => match b {

aml/src/value.rs

@@ -1,4 +1,4 @@
-use crate::{misc::ArgNum, AmlContext, AmlError, AmlHandle, AmlName};
+use crate::{misc::ArgNum, opregion::OpRegion, AmlContext, AmlError, AmlHandle};
 use alloc::{
     string::{String, ToString},
     sync::Arc,
@@ -8,21 +8,6 @@ use bit_field::BitField;
 use core::{cmp, fmt, fmt::Debug};
 use spinning_top::Spinlock;
 
-#[derive(Clone, Copy, PartialEq, Eq, Debug)]
-pub enum RegionSpace {
-    SystemMemory,
-    SystemIo,
-    PciConfig,
-    EmbeddedControl,
-    SMBus,
-    SystemCmos,
-    PciBarTarget,
-    IPMI,
-    GeneralPurposeIo,
-    GenericSerialBus,
-    OemDefined(u8),
-}
-
 #[derive(Clone, Copy, PartialEq, Eq, Debug)]
 pub enum FieldAccessType {
     Any,
@@ -181,12 +166,7 @@ pub enum AmlValue {
     /// Describes an operation region. Some regions require other objects to be declared under their parent device
     /// (e.g. an `_ADR` object for a `PciConfig` region), in which case an absolute path to the object is stored in
     /// `parent_device`.
-    OpRegion {
+    OpRegion(OpRegion),
-        region: RegionSpace,
-        offset: u64,
-        length: u64,
-        parent_device: Option<AmlName>,
-    },
     /// Describes a field unit within an operation region.
     Field {
         region: AmlHandle,
@@ -280,15 +260,16 @@ impl AmlValue {
         }
     }
 
-    pub fn as_bool(&self) -> Result<bool, AmlError> {
+    pub fn as_bool(&self, context: &mut AmlContext) -> Result<bool, AmlError> {
         match self {
             AmlValue::Boolean(value) => Ok(*value),
             AmlValue::Integer(value) => Ok(*value != 0),
+            AmlValue::Field{ .. } => Ok(self.as_integer(context)? != 0),
             _ => Err(AmlError::IncompatibleValueConversion { current: self.type_of(), target: AmlType::Integer }),
         }
     }
 
-    pub fn as_integer(&self, context: &AmlContext) -> Result<u64, AmlError> {
+    pub fn as_integer(&self, context: &mut AmlContext) -> Result<u64, AmlError> {
         match self {
             AmlValue::Integer(value) => Ok(*value),
             AmlValue::Boolean(value) => Ok(if *value { u64::max_value() } else { 0 }),
@@ -320,7 +301,7 @@ impl AmlValue {
         }
     }
 
-    pub fn as_buffer(&self, context: &AmlContext) -> Result<Arc<Spinlock<Vec<u8>>>, AmlError> {
+    pub fn as_buffer(&self, context: &mut AmlContext) -> Result<Arc<Spinlock<Vec<u8>>>, AmlError> {
         match self {
             AmlValue::Buffer(ref bytes) => Ok(bytes.clone()),
             // TODO: implement conversion of String and Integer to Buffer
@@ -330,7 +311,7 @@ impl AmlValue {
         }
     }
 
-    pub fn as_string(&self, context: &AmlContext) -> Result<String, AmlError> {
+    pub fn as_string(&self, context: &mut AmlContext) -> Result<String, AmlError> {
         match self {
             AmlValue::String(ref string) => Ok(string.clone()),
             // TODO: implement conversion of Buffer to String
@@ -404,7 +385,7 @@ impl AmlValue {
     ///     `Integer` from: `Buffer`, `BufferField`, `DdbHandle`, `FieldUnit`, `String`, `Debug`
     ///     `Package` from: `Debug`
     ///     `String` from: `Integer`, `Buffer`, `Debug`
-    pub fn as_type(&self, desired_type: AmlType, context: &AmlContext) -> Result<AmlValue, AmlError> {
+    pub fn as_type(&self, desired_type: AmlType, context: &mut AmlContext) -> Result<AmlValue, AmlError> {
         // If the value is already of the correct type, just return it as is
         if self.type_of() == desired_type {
             return Ok(self.clone());
@@ -421,99 +402,29 @@ impl AmlValue {
         }
     }
 
-    /// Reads from a field of an opregion, returning either a `AmlValue::Integer` or an `AmlValue::Buffer`,
+    /// Reads from a field of an op-region, returning either a `AmlValue::Integer` or an `AmlValue::Buffer`,
     /// depending on the size of the field.
-    pub fn read_field(&self, context: &AmlContext) -> Result<AmlValue, AmlError> {
+    pub fn read_field(&self, context: &mut AmlContext) -> Result<AmlValue, AmlError> {
         if let AmlValue::Field { region, flags, offset, length } = self {
-            let _maximum_access_size = {
+            if let AmlValue::OpRegion(region) = context.namespace.get(*region)?.clone() {
-                if let AmlValue::OpRegion { region, .. } = context.namespace.get(*region)? {
+                region.read_field(*offset, *length, *flags, context)
-                    match region {
+            } else {
-                        RegionSpace::SystemMemory => 64,
+                Err(AmlError::FieldRegionIsNotOpRegion)
-                        RegionSpace::SystemIo | RegionSpace::PciConfig => 32,
+            }
-                        _ => unimplemented!(),
-                    }
-                } else {
-                    return Err(AmlError::FieldRegionIsNotOpRegion);
-                }
-            };
-            let minimum_access_size = match flags.access_type()? {
-                FieldAccessType::Any => 8,
-                FieldAccessType::Byte => 8,
-                FieldAccessType::Word => 16,
-                FieldAccessType::DWord => 32,
-                FieldAccessType::QWord => 64,
-                FieldAccessType::Buffer => 8, // TODO
-            };
-
-            /*
-             * Find the access size, as either the minimum access size allowed by the region, or the field length
-             * rounded up to the next power-of-2, whichever is larger.
-             */
-            let access_size = u64::max(minimum_access_size, length.next_power_of_two());
-
-            /*
-             * TODO: we need to decide properly how to read from the region itself. Complications:
-             *    - if the region has a minimum access size greater than the desired length, we need to read the
-             *      minimum and mask it (reading a byte from a WordAcc region)
-             *    - if the desired length is larger than we can read, we need to do multiple reads
-             */
-            Ok(AmlValue::Integer(
-                context.read_region(*region, *offset, access_size)?.get_bits(0..(*length as usize)),
-            ))
         } else {
             Err(AmlError::IncompatibleValueConversion { current: self.type_of(), target: AmlType::FieldUnit })
         }
     }
 
+    /// Write to a field of an op-region, from either a `AmlValue::Integer` or `AmlValue::Buffer`
+    /// as necessary.
     pub fn write_field(&mut self, value: AmlValue, context: &mut AmlContext) -> Result<(), AmlError> {
-        /*
-         * If the field's update rule is `Preserve`, we need to read the initial value of the field, so we can
-         * overwrite the correct bits. We destructure the field to do the actual write, so we read from it if
-         * needed here, otherwise the borrow-checker doesn't understand.
-         */
-        let field_update_rule = if let AmlValue::Field { flags, .. } = self {
-            flags.field_update_rule()?
-        } else {
-            return Err(AmlError::IncompatibleValueConversion {
-                current: self.type_of(),
-                target: AmlType::FieldUnit,
-            });
-        };
-        let mut field_value = match field_update_rule {
-            FieldUpdateRule::Preserve => self.read_field(context)?.as_integer(context)?,
-            FieldUpdateRule::WriteAsOnes => 0xffffffff_ffffffff,
-            FieldUpdateRule::WriteAsZeros => 0x0,
-        };
-
         if let AmlValue::Field { region, flags, offset, length } = self {
-            let _maximum_access_size = {
+            if let AmlValue::OpRegion(region) = context.namespace.get(*region)?.clone() {
-                if let AmlValue::OpRegion { region, .. } = context.namespace.get(*region)? {
+                region.write_field(*offset, *length, *flags, value, context)
-                    match region {
+            } else {
-                        RegionSpace::SystemMemory => 64,
+                Err(AmlError::FieldRegionIsNotOpRegion)
-                        RegionSpace::SystemIo | RegionSpace::PciConfig => 32,
+            }
-                        _ => unimplemented!(),
-                    }
-                } else {
-                    return Err(AmlError::FieldRegionIsNotOpRegion);
-                }
-            };
-            let minimum_access_size = match flags.access_type()? {
-                FieldAccessType::Any => 8,
-                FieldAccessType::Byte => 8,
-                FieldAccessType::Word => 16,
-                FieldAccessType::DWord => 32,
-                FieldAccessType::QWord => 64,
-                FieldAccessType::Buffer => 8, // TODO
-            };
-
-            /*
-             * Find the access size, as either the minimum access size allowed by the region, or the field length
-             * rounded up to the next power-of-2, whichever is larger.
-             */
-            let access_size = u64::max(minimum_access_size, length.next_power_of_two());
-
-            field_value.set_bits(0..(*length as usize), value.as_integer(context)?);
-            context.write_region(*region, *offset, access_size, field_value)
         } else {
             Err(AmlError::IncompatibleValueConversion { current: self.type_of(), target: AmlType::FieldUnit })
         }
@@ -642,8 +553,6 @@ impl Args {
     pub const EMPTY: Self = Self([None, None, None, None, None, None, None]);
 
     pub fn from_list(list: Vec<AmlValue>) -> Result<Args, AmlError> {
-        use core::convert::TryInto;
-
         if list.len() > 7 {
             return Err(AmlError::TooManyArgs);
         }

tests/inc.asl

@@ -0,0 +1,9 @@
+DefinitionBlock("inc.aml", "DSDT", 1, "RSACPI", "INCDEC", 1) {
+    Name(X, 0)
+    X++
+    X++
+    X++
+    Name(Y, 0)
+    Y = X
+    X--
+}