Merge #65

65: A few assorted changes r=IsaacWoods a=IsaacWoods

This upstreams a few assorted changes that I seem to have accumulated. Specifically:
* It removes the dependency on `typenum` (replacing them with const generics) to hopefully make `acpi` slightly smaller
* It makes accessing extended fields sound (I think it was unsound before, and just never seemed to miscompile), by treating them as uninitialised memory (instead of potentially treating them as initialised `()` types) until the version is checked
* It introduces a `Signature` type to represent SDT signatures
* It moves some AML opcode parsers around to be in the right place

Closes #44 

Co-authored-by: Isaac Woods <isaacwoods.home@gmail.com>

acpi/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "acpi"
-version = "0.5.0"
+version = "0.6.0"
 authors = ["Isaac Woods"]
 repository = "https://github.com/rust-osdev/acpi"
 description = "Library for parsing ACPI tables"
@@ -12,4 +12,3 @@ edition = "2018"
 [dependencies]
 log = "0.4"
 bit_field = "0.10"
-typenum = "1"

acpi/src/fadt.rs

@@ -1,6 +1,12 @@
-use crate::{sdt::SdtHeader, Acpi, AcpiError, AcpiHandler, AmlTable, GenericAddress, PhysicalMapping};
-
-type ExtendedField<T> = crate::sdt::ExtendedField<T, typenum::U2>;
+use crate::{
+    sdt::{ExtendedField, SdtHeader, ACPI_VERSION_2_0},
+    Acpi,
+    AcpiError,
+    AcpiHandler,
+    AmlTable,
+    GenericAddress,
+    PhysicalMapping,
+};
 
 /// Represents the Fixed ACPI Description Table (FADT). This table contains various fixed hardware
 /// details, such as the addresses of the hardware register blocks. It also contains a pointer to
@@ -58,19 +64,19 @@ pub struct Fadt {
     reset_value: u8,
     arm_boot_arch: u16,
     fadt_minor_version: u8,
-    x_firmware_ctrl: ExtendedField<u64>,
-    x_dsdt_address: ExtendedField<u64>,
-    x_pm1a_event_block: ExtendedField<GenericAddress>,
-    x_pm1b_event_block: ExtendedField<GenericAddress>,
-    x_pm1a_control_block: ExtendedField<GenericAddress>,
-    x_pm1b_control_block: ExtendedField<GenericAddress>,
-    x_pm2_control_block: ExtendedField<GenericAddress>,
-    x_pm_timer_block: ExtendedField<GenericAddress>,
-    x_gpe0_block: ExtendedField<GenericAddress>,
-    x_gpe1_block: ExtendedField<GenericAddress>,
-    sleep_control_reg: ExtendedField<GenericAddress>,
-    sleep_status_reg: ExtendedField<GenericAddress>,
-    hypervisor_vendor_id: ExtendedField<u64>,
+    x_firmware_ctrl: ExtendedField<u64, ACPI_VERSION_2_0>,
+    x_dsdt_address: ExtendedField<u64, ACPI_VERSION_2_0>,
+    x_pm1a_event_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_pm1b_event_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_pm1a_control_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_pm1b_control_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_pm2_control_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_pm_timer_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_gpe0_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    x_gpe1_block: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    sleep_control_reg: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    sleep_status_reg: ExtendedField<GenericAddress, ACPI_VERSION_2_0>,
+    hypervisor_vendor_id: ExtendedField<u64, ACPI_VERSION_2_0>,
 }
 
 pub(crate) fn parse_fadt<H>(
@@ -82,12 +88,12 @@ where
     H: AcpiHandler,
 {
     let fadt = &*mapping;
-    fadt.header.validate(b"FACP")?;
+    fadt.header.validate(crate::sdt::Signature::FADT)?;
 
     let dsdt_address = unsafe {
         fadt.x_dsdt_address
-            .get(fadt.header.revision())
-            .filter(|p| *p != 0)
+            .access(fadt.header.revision)
+            .filter(|&p| p != 0)
             .or(Some(fadt.dsdt_address as u64))
             .filter(|p| *p != 0)
             .map(|p| p as usize)
@@ -95,7 +101,7 @@ where
 
     acpi.dsdt = dsdt_address.map(|address| {
         let dsdt_header = crate::sdt::peek_at_sdt_header(handler, address);
-        AmlTable::new(address, dsdt_header.length())
+        AmlTable::new(address, dsdt_header.length)
     });
 
     Ok(())

acpi/src/hpet.rs

@@ -34,7 +34,7 @@ pub(crate) struct HpetTable {
 }
 
 pub(crate) fn parse_hpet(acpi: &mut Acpi, mapping: &PhysicalMapping<HpetTable>) -> Result<(), AcpiError> {
-    (*mapping).header.validate(b"HPET")?;
+    (*mapping).header.validate(crate::sdt::Signature::HPET)?;
     let hpet = &*mapping;
 
     // Make sure the HPET's in system memory

acpi/src/lib.rs

@@ -22,7 +22,7 @@
 //! gathered from the static tables, and can be queried to set up hardware etc.
 
 #![no_std]
-#![feature(nll, exclusive_range_pattern)]
+#![feature(nll, exclusive_range_pattern, const_generics)]
 
 extern crate alloc;
 #[cfg_attr(test, macro_use)]
@@ -48,22 +48,24 @@ pub use crate::{
     rsdp_search::search_for_rsdp_bios,
 };
 
-use crate::{rsdp::Rsdp, sdt::SdtHeader};
+use crate::{
+    rsdp::Rsdp,
+    sdt::{SdtHeader, Signature},
+};
 use alloc::vec::Vec;
 use core::mem;
 
 #[derive(Debug)]
-// TODO: manually implement Debug to print signatures correctly etc.
 pub enum AcpiError {
     RsdpIncorrectSignature,
     RsdpInvalidOemId,
     RsdpInvalidChecksum,
     NoValidRsdp,
 
-    SdtInvalidSignature([u8; 4]),
-    SdtInvalidOemId([u8; 4]),
-    SdtInvalidTableId([u8; 4]),
-    SdtInvalidChecksum([u8; 4]),
+    SdtInvalidSignature(Signature),
+    SdtInvalidOemId(Signature),
+    SdtInvalidTableId(Signature),
+    SdtInvalidChecksum(Signature),
 
     InvalidMadt(MadtError),
 }
@@ -210,15 +212,15 @@ where
     };
 
     let header = sdt::peek_at_sdt_header(handler, physical_address);
-    let mapping = handler.map_physical_region::<SdtHeader>(physical_address, header.length() as usize);
+    let mapping = handler.map_physical_region::<SdtHeader>(physical_address, header.length as usize);
 
     if revision == 0 {
         /*
          * ACPI Version 1.0. It's a RSDT!
          */
-        (*mapping).validate(b"RSDT")?;
+        (*mapping).validate(sdt::Signature::RSDT)?;
 
-        let num_tables = ((*mapping).length() as usize - mem::size_of::<SdtHeader>()) / mem::size_of::<u32>();
+        let num_tables = ((*mapping).length as usize - mem::size_of::<SdtHeader>()) / mem::size_of::<u32>();
         let tables_base = ((mapping.virtual_start.as_ptr() as usize) + mem::size_of::<SdtHeader>()) as *const u32;
 
         for i in 0..num_tables {
@@ -228,9 +230,9 @@ where
         /*
          * ACPI Version 2.0+. It's a XSDT!
          */
-        (*mapping).validate(b"XSDT")?;
+        (*mapping).validate(sdt::Signature::XSDT)?;
 
-        let num_tables = ((*mapping).length() as usize - mem::size_of::<SdtHeader>()) / mem::size_of::<u64>();
+        let num_tables = ((*mapping).length as usize - mem::size_of::<SdtHeader>()) / mem::size_of::<u64>();
         let tables_base = ((mapping.virtual_start.as_ptr() as usize) + mem::size_of::<SdtHeader>()) as *const u64;
 
         for i in 0..num_tables {

acpi/src/madt.rs

@@ -41,7 +41,7 @@ impl Madt {
     fn entries(&self) -> MadtEntryIter {
         MadtEntryIter {
             pointer: unsafe { (self as *const Madt as *const u8).offset(mem::size_of::<Madt>() as isize) },
-            remaining_length: self.header.length() - mem::size_of::<Madt>() as u32,
+            remaining_length: self.header.length - mem::size_of::<Madt>() as u32,
             _phantom: PhantomData,
         }
     }
@@ -337,7 +337,7 @@ pub(crate) fn parse_madt<H>(
 where
     H: AcpiHandler,
 {
-    (*mapping).header.validate(b"APIC")?;
+    (*mapping).header.validate(crate::sdt::Signature::MADT)?;
 
     /*
      * If the MADT doesn't contain another supported interrupt model (either APIC, SAPIC, X2APIC

acpi/src/mcfg.rs

@@ -42,7 +42,7 @@ pub(crate) struct Mcfg {
 
 impl Mcfg {
     fn entries(&self) -> &[McfgEntry] {
-        let length = self.header.length() as usize - mem::size_of::<Mcfg>();
+        let length = self.header.length as usize - mem::size_of::<Mcfg>();
 
         // intentionally round down in case length isn't an exact multiple of McfgEntry size
         let num_entries = length / mem::size_of::<McfgEntry>();
@@ -66,7 +66,7 @@ struct McfgEntry {
 }
 
 pub(crate) fn parse_mcfg(acpi: &mut Acpi, mapping: &PhysicalMapping<Mcfg>) -> Result<(), AcpiError> {
-    (*mapping).header.validate(b"MCFG")?;
+    (*mapping).header.validate(crate::sdt::Signature::MCFG)?;
 
     acpi.pci_config_regions =
         Some(PciConfigRegions { regions: mapping.entries().iter().map(|&entry| entry).collect() });

acpi/src/sdt.rs

@@ -1,29 +1,23 @@
 use crate::{fadt::Fadt, hpet::HpetTable, madt::Madt, mcfg::Mcfg, Acpi, AcpiError, AcpiHandler, AmlTable};
-use core::{marker::PhantomData, mem, str};
+use core::{fmt, mem, mem::MaybeUninit, str};
 use log::{trace, warn};
-use typenum::Unsigned;
 
-/// A union that represents a field that is not necessarily present and is only present in a later
-/// ACPI version (represented by the compile time number and type parameter `R`)
-#[derive(Copy, Clone)]
+pub const ACPI_VERSION_2_0: u8 = 20;
+
+/// Represents a field which may or may not be present within an ACPI structure, depending on the version of ACPI
+/// that a system supports. If the field is not present, it is not safe to treat the data as initialised.
 #[repr(C, packed)]
-pub union ExtendedField<T: Copy, R: Unsigned> {
-    field: T,
-    nothing: (),
-    _phantom: PhantomData<R>,
-}
+pub struct ExtendedField<T: Copy, const MIN_VERSION: u8>(MaybeUninit<T>);
 
-impl<T: Copy, R: Unsigned> ExtendedField<T, R> {
-    /// Checks that the given revision is greater than `R` (typenum revision number) and then
-    /// returns the field, otherwise returning `None`.
-    ///
-    /// # Unsafety
+impl<T: Copy, const MIN_VERSION: u8> ExtendedField<T, MIN_VERSION> {
+    /// Access the field if it's present for the given ACPI version. You should get this version from another ACPI
+    /// structure, such as the RSDT/XSDT.
     ///
-    /// This is unsafe as the given `revision` could be incorrect or fabricated. However, it should
-    /// be safe if it represents the revision of the table this field is present in.
-    pub unsafe fn get(&self, revision: u8) -> Option<T> {
-        if revision >= R::to_u8() {
-            Some(self.field)
+    /// ### Safety
+    /// If a bogus ACPI version is passed, this function may access uninitialised data, which is unsafe.
+    pub unsafe fn access(&self, version: u8) -> Option<T> {
+        if version >= MIN_VERSION {
+            Some(self.0.assume_init())
         } else {
             None
         }
@@ -66,15 +60,15 @@ impl<T: Copy, R: Unsigned> ExtendedField<T, R> {
 #[derive(Clone, Copy)]
 #[repr(C, packed)]
 pub struct SdtHeader {
-    signature: [u8; 4],
-    length: u32,
-    revision: u8,
-    checksum: u8,
-    oem_id: [u8; 6],
-    oem_table_id: [u8; 8],
-    oem_revision: u32,
-    creator_id: u32,
-    creator_revision: u32,
+    pub signature: Signature,
+    pub length: u32,
+    pub revision: u8,
+    pub checksum: u8,
+    pub oem_id: [u8; 6],
+    pub oem_table_id: [u8; 8],
+    pub oem_revision: u32,
+    pub creator_id: u32,
+    pub creator_revision: u32,
 }
 
 impl SdtHeader {
@@ -83,20 +77,20 @@ impl SdtHeader {
     ///     b) The checksum of the SDT
     ///
     /// This assumes that the whole SDT is mapped.
-    pub fn validate(&self, signature: &[u8; 4]) -> Result<(), AcpiError> {
+    pub fn validate(&self, signature: Signature) -> Result<(), AcpiError> {
         // Check the signature
-        if &self.signature != signature {
-            return Err(AcpiError::SdtInvalidSignature(*signature));
+        if self.signature != signature {
+            return Err(AcpiError::SdtInvalidSignature(signature));
         }
 
         // Check the OEM id
         if str::from_utf8(&self.oem_id).is_err() {
-            return Err(AcpiError::SdtInvalidOemId(*signature));
+            return Err(AcpiError::SdtInvalidOemId(signature));
         }
 
         // Check the OEM table id
         if str::from_utf8(&self.oem_table_id).is_err() {
-            return Err(AcpiError::SdtInvalidTableId(*signature));
+            return Err(AcpiError::SdtInvalidTableId(signature));
         }
 
         // Validate the checksum
@@ -107,29 +101,12 @@ impl SdtHeader {
         }
 
         if sum > 0 {
-            return Err(AcpiError::SdtInvalidChecksum(*signature));
+            return Err(AcpiError::SdtInvalidChecksum(signature));
         }
 
         Ok(())
     }
 
-    pub fn raw_signature(&self) -> [u8; 4] {
-        self.signature
-    }
-
-    pub fn signature<'a>(&'a self) -> &'a str {
-        // Safe to unwrap because we check signature is valid UTF8 in `validate`
-        str::from_utf8(&self.signature).unwrap()
-    }
-
-    pub fn length(&self) -> u32 {
-        self.length
-    }
-
-    pub fn revision(&self) -> u8 {
-        self.revision
-    }
-
     pub fn oem_id<'a>(&'a self) -> &'a str {
         // Safe to unwrap because checked in `validate`
         str::from_utf8(&self.oem_id).unwrap()
@@ -141,6 +118,36 @@ impl SdtHeader {
     }
 }
 
+#[derive(Clone, Copy, PartialEq, Eq)]
+#[repr(transparent)]
+pub struct Signature([u8; 4]);
+
+impl Signature {
+    pub const RSDT: Signature = Signature(*b"RSDT");
+    pub const XSDT: Signature = Signature(*b"XSDT");
+    pub const FADT: Signature = Signature(*b"FACP");
+    pub const HPET: Signature = Signature(*b"HPET");
+    pub const MADT: Signature = Signature(*b"APIC");
+    pub const MCFG: Signature = Signature(*b"MCFG");
+    pub const SSDT: Signature = Signature(*b"SSDT");
+
+    pub fn as_str(&self) -> &str {
+        str::from_utf8(&self.0).unwrap()
+    }
+}
+
+impl fmt::Display for Signature {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}", self.as_str())
+    }
+}
+
+impl fmt::Debug for Signature {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "\"{}\"", self.as_str())
+    }
+}
+
 /// Takes the physical address of an SDT, and maps, clones and unmaps its header. Useful for
 /// finding out how big it is to map it correctly later.
 pub(crate) fn peek_at_sdt_header<H>(handler: &mut H, physical_address: usize) -> SdtHeader
@@ -161,39 +168,39 @@ where
     H: AcpiHandler,
 {
     let header = peek_at_sdt_header(handler, physical_address);
-    trace!("Found ACPI table with signature {:?} and length {:?}", header.signature(), header.length());
+    trace!("Found ACPI table with signature {:?} and length {:?}", header.signature, header.length);
 
     /*
      * For a recognised signature, a new physical mapping should be created with the correct type
      * and length, and then the dispatched to the correct function to actually parse the table.
      */
-    match header.signature() {
-        "FACP" => {
+    match header.signature {
+        Signature::FADT => {
             let fadt_mapping = handler.map_physical_region::<Fadt>(physical_address, mem::size_of::<Fadt>());
             crate::fadt::parse_fadt(acpi, handler, &fadt_mapping)?;
             handler.unmap_physical_region(fadt_mapping);
         }
 
-        "HPET" => {
+        Signature::HPET => {
             let hpet_mapping =
                 handler.map_physical_region::<HpetTable>(physical_address, mem::size_of::<HpetTable>());
             crate::hpet::parse_hpet(acpi, &hpet_mapping)?;
             handler.unmap_physical_region(hpet_mapping);
         }
 
-        "APIC" => {
-            let madt_mapping = handler.map_physical_region::<Madt>(physical_address, header.length() as usize);
+        Signature::MADT => {
+            let madt_mapping = handler.map_physical_region::<Madt>(physical_address, header.length as usize);
             crate::madt::parse_madt(acpi, handler, &madt_mapping)?;
             handler.unmap_physical_region(madt_mapping);
         }
 
-        "MCFG" => {
-            let mcfg_mapping = handler.map_physical_region::<Mcfg>(physical_address, header.length() as usize);
+        Signature::MCFG => {
+            let mcfg_mapping = handler.map_physical_region::<Mcfg>(physical_address, header.length as usize);
             crate::mcfg::parse_mcfg(acpi, &mcfg_mapping)?;
             handler.unmap_physical_region(mcfg_mapping);
         }
 
-        "SSDT" => acpi.ssdts.push(AmlTable::new(physical_address, header.length())),
+        Signature::SSDT => acpi.ssdts.push(AmlTable::new(physical_address, header.length)),
 
         signature => {
             /*

aml/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "aml"
-version = "0.5.0"
+version = "0.6.0"
 authors = ["Isaac Woods"]
 repository = "https://github.com/rust-osdev/acpi"
 description = "Library for parsing AML"

aml/src/parser.rs

@@ -168,7 +168,6 @@ where
     }
 }
 
-// TODO: can we use const generics (e.g. [R; N]) to avoid allocating?
 pub fn n_of<'a, 'c, P, R>(parser: P, n: usize) -> impl Parser<'a, 'c, Vec<R>>
 where
     'c: 'a,

aml/src/term_object.rs

@@ -19,12 +19,12 @@ use crate::{
     },
     pkg_length::{pkg_length, PkgLength},
     type1::type1_opcode,
-    type2::type2_opcode,
+    type2::{def_buffer, def_package, type2_opcode},
     value::{AmlValue, FieldFlags, MethodFlags, RegionSpace},
     AmlContext,
     AmlError,
 };
-use alloc::{string::String, vec::Vec};
+use alloc::string::String;
 use core::str;
 
 /// `TermList`s are usually found within explicit-length objects (so they have a `PkgLength`
@@ -449,69 +449,6 @@ where
         .discard_result()
 }
 
-pub fn def_buffer<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
-where
-    'c: 'a,
-{
-    /*
-     * DefBuffer := 0x11 PkgLength BufferSize ByteList
-     * BufferSize := TermArg => Integer
-     *
-     * XXX: The spec says that zero-length buffers (e.g. the PkgLength is 0) are illegal, but
-     * we've encountered them in QEMU-generated tables, so we return an empty buffer in these
-     * cases.
-     */
-    opcode(opcode::DEF_BUFFER_OP)
-        .then(comment_scope(
-            "DefBuffer",
-            pkg_length().then(term_arg()).feed(|(pkg_length, buffer_size)| {
-                take_to_end_of_pkglength(pkg_length)
-                    .map(move |bytes| Ok((bytes.to_vec(), buffer_size.as_integer()?)))
-            }),
-        ))
-        .map(|((), (bytes, buffer_size))| Ok(AmlValue::Buffer { bytes, size: buffer_size }))
-}
-
-pub fn def_package<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
-where
-    'c: 'a,
-{
-    /*
-     * DefPackage := 0x12 PkgLength NumElements PackageElementList
-     * NumElements := ByteData
-     * PackageElementList := Nothing | <PackageElement PackageElementList>
-     * PackageElement := DataRefObject | NameString
-     */
-    opcode(opcode::DEF_PACKAGE_OP)
-        .then(comment_scope(
-            "DefPackage",
-            pkg_length().then(take()).feed(|(pkg_length, num_elements)| {
-                move |mut input, mut context| {
-                    let mut package_contents = Vec::new();
-
-                    while pkg_length.still_parsing(input) {
-                        let (new_input, new_context, value) = package_element().parse(input, context)?;
-                        input = new_input;
-                        context = new_context;
-
-                        package_contents.push(value);
-                    }
-
-                    assert_eq!(package_contents.len(), num_elements as usize);
-                    Ok((input, context, AmlValue::Package(package_contents)))
-                }
-            }),
-        ))
-        .map(|((), package)| Ok(package))
-}
-
-pub fn package_element<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
-where
-    'c: 'a,
-{
-    choice!(data_ref_object(), name_string().map(|string| Ok(AmlValue::String(string.as_string()))))
-}
-
 pub fn term_arg<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
 where
     'c: 'a,

aml/src/type2.rs

@@ -1,12 +1,22 @@
 use crate::{
     name_object::{name_string, super_name, Target},
     opcode::{self, opcode},
-    parser::{choice, comment_scope, comment_scope_verbose, id, try_with_context, Parser},
-    term_object::term_arg,
+    parser::{
+        choice,
+        comment_scope,
+        comment_scope_verbose,
+        id,
+        take,
+        take_to_end_of_pkglength,
+        try_with_context,
+        Parser,
+    },
+    pkg_length::pkg_length,
+    term_object::{data_ref_object, term_arg},
     value::AmlValue,
     AmlError,
 };
-use alloc::boxed::Box;
+use alloc::{boxed::Box, vec::Vec};
 
 /// Type 2 opcodes return a value and so can be used in expressions.
 pub fn type2_opcode<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
@@ -24,7 +34,33 @@ where
      *                DefSubtract | DefTimer | DefToBCD | DefToBuffer | DefToDecimalString |
      *                DefToHexString | DefToInteger | DefToString | DefWait | DefXOr | MethodInvocation
      */
-    comment_scope_verbose("Type2Opcode", choice!(def_l_equal(), def_store(), method_invocation()))
+    comment_scope_verbose(
+        "Type2Opcode",
+        choice!(def_buffer(), def_l_equal(), def_package(), def_store(), method_invocation()),
+    )
+}
+
+pub fn def_buffer<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
+where
+    'c: 'a,
+{
+    /*
+     * DefBuffer := 0x11 PkgLength BufferSize ByteList
+     * BufferSize := TermArg => Integer
+     *
+     * XXX: The spec says that zero-length buffers (e.g. the PkgLength is 0) are illegal, but
+     * we've encountered them in QEMU-generated tables, so we return an empty buffer in these
+     * cases.
+     */
+    opcode(opcode::DEF_BUFFER_OP)
+        .then(comment_scope(
+            "DefBuffer",
+            pkg_length().then(term_arg()).feed(|(pkg_length, buffer_size)| {
+                take_to_end_of_pkglength(pkg_length)
+                    .map(move |bytes| Ok((bytes.to_vec(), buffer_size.as_integer()?)))
+            }),
+        ))
+        .map(|((), (bytes, buffer_size))| Ok(AmlValue::Buffer { bytes, size: buffer_size }))
 }
 
 fn def_l_equal<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
@@ -45,6 +81,46 @@ where
         .map(|((), result)| Ok(result))
 }
 
+pub fn def_package<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
+where
+    'c: 'a,
+{
+    /*
+     * DefPackage := 0x12 PkgLength NumElements PackageElementList
+     * NumElements := ByteData
+     * PackageElementList := Nothing | <PackageElement PackageElementList>
+     * PackageElement := DataRefObject | NameString
+     */
+    opcode(opcode::DEF_PACKAGE_OP)
+        .then(comment_scope(
+            "DefPackage",
+            pkg_length().then(take()).feed(|(pkg_length, num_elements)| {
+                move |mut input, mut context| {
+                    let mut package_contents = Vec::new();
+
+                    while pkg_length.still_parsing(input) {
+                        let (new_input, new_context, value) = package_element().parse(input, context)?;
+                        input = new_input;
+                        context = new_context;
+
+                        package_contents.push(value);
+                    }
+
+                    assert_eq!(package_contents.len(), num_elements as usize);
+                    Ok((input, context, AmlValue::Package(package_contents)))
+                }
+            }),
+        ))
+        .map(|((), package)| Ok(package))
+}
+
+pub fn package_element<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
+where
+    'c: 'a,
+{
+    choice!(data_ref_object(), name_string().map(|string| Ok(AmlValue::String(string.as_string()))))
+}
+
 fn def_store<'a, 'c>() -> impl Parser<'a, 'c, AmlValue>
 where
     'c: 'a,