From 888c490a9b207851609c8a097ed052cb4f8c0264 Mon Sep 17 00:00:00 2001 From: Joey Vagedes Date: Thu, 16 Apr 2026 11:21:34 -0700 Subject: [PATCH 1/4] update --- .../text/0000-acpi-interface-improvements.md | 201 ++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 docs/src/rfc/text/0000-acpi-interface-improvements.md diff --git a/docs/src/rfc/text/0000-acpi-interface-improvements.md b/docs/src/rfc/text/0000-acpi-interface-improvements.md new file mode 100644 index 000000000..d5efeb1ee --- /dev/null +++ b/docs/src/rfc/text/0000-acpi-interface-improvements.md @@ -0,0 +1,201 @@ +# RFC: `patina_acpi` service interface improvements + +This RFC proposes multiple interface changes to the two services (`Service` and +`Service) to support ACPI Tables with unknown sizes (DSTs) and to generally improve user ergonomics +when using the service. + +## Change Log + +- 2026-04-16 + +## Motivation + +The main motivation behind this change is to add support for DSTs (Dynamically Sized Types) in the generics portion of +the service interface (e.g. `Service::add_acpi_table` and +`Service::get_acpi_table`). DST ACPI tables are currently supported via non-generic interfaces, e.g. +`Service::install_acpi_table`, but it puts all unsafety burdons on you, such as (1) allocating to the +correct memory type and (2) ensuring the ACPI table has the correct format. This interface simply accepts a pointer to +a memory address and treats it as a ACPI table. This support would be highly beneficial as most ACPI tables tend to +have variable length - e.g. a header with a runtime variable number of bytes behind it representing N number of +parseable structures. + +Supporting DSTs will require some fundamental changes to how ACPI table data is managed (described in-depth below), +which means we have the opportunity to make some other fundamental changes to improve the code, such as using zerocopy +instead of raw pointer manipulation. + +## Technology Background + +- TODO + +## Goals + +1. Support DSTs through the ACPI service interface. +2. Reduce `unsafe` public interfaces via `AcpiTable` trait. +3. Reduce `unsafe` usage internal to the service via zerocopy. +4. Consolidate the two services provided by `patina_acpi`. +5. Provide updated and more usage documentation. + +## Requirements + +1. DSTs are supported through the ACPI service interface. +2. Documentation is updated and better usage documentation created. + +## Unresolved Questions + +- Do we find the const SIGNATURE in the `AcpiTable` trait defintion useful? Currently we cache the `TypeId` when + adding a table with the service, which is used to validate against if someone attempts to retrieve the table from the + database. This has the downside that if a ACPI table was added via the protocol, we do not have a `TypeId` associated + with the table, and cannot validate it, if we attempt to retrieve it on the rust side. Replacing the TypeId with + signature validation would better validate the `C protocol -> Service` story while also 100% validating the + `Service -> Service` story. + +- As noted in the Technology Background, DSTs must always be a reference to the data, since they are an unknown size. + Examples of this are `&MyStruct`, `Box`, `Rc`, etc. + +## Alternatives + +The alternative is to continue the status quo. As mentioned above, DSTs are currently supported via the non-generic +service interface, but it requires you to (1) Manually construct the `AcpiTable` struct (A wrapper around a raw +pointer), (2) Ensure you've allocated to the correct memory type yourself, and (3) Manually attempt casting a generic +table to a table of your choosing. + +## Prior Art + + + + + +## Rust Code Design + +### AcpiTable Trait Definition + +Below is the expected AcpiTable trait defintion. A derive macro will be provided for types that start with a +`AcpiTableHeader` structure, however it can also be manually implemented if a type does not start with an +`AcpiTableHeader`, but has the required fields as specified in the trait documentation. This trait also requires that +multiple zerocopy traits be implemented on it. + +```rust +/// A trait implemented on a type that represents an ACPI Table. +/// +/// ## Examples +/// +/// ```rust +/// +/// #[derive(FromBytes, IntoBytes, Immutable, KnownLayout)] +/// #[repr(C, packed)] +/// struct MyTable { +/// signature: u32, +/// length: u32, +/// revision: u8, +/// checksum: u8, +/// data_differing_from_header: [u8; 256] +/// } +/// +/// // SAFETY: This table starts with the required fields +/// unsafe impl AcpiTable for MyTable { +/// const SIGNATURE: u32 = signature!('F', 'F', 'A', 'B'); +/// } +/// ``` +/// +/// ## Safety +/// +/// The structure implementing this trait must start with either a [AcpiTableHeader] or the following for +/// fields in this exact order: +/// +/// 1. signature: u32 +/// 2. length: u32 +/// 3. revision: u8 +/// 4. checksum: u8 +unsafe trait AcpiTable: FromBytes + IntoBytes + Immutable + KnownLayout { + /// The signature of the table. Must match the signature field in the table. + const SIGNATURE: u32 +} + +/// A derive macro for the AcpiTable trait for types that start with [AcpiTableHeader] +/// +/// ## Examples +/// +/// ```rust +/// #[derive(AcpiTable, FromBytes, IntoBytes, Immutable, KnownLayout)] +/// #[repr(C, packed)] +/// #[signature('F', 'F', 'A', 'B')] +/// struct MyAcpiTable { +/// header: AcpiTableHeader, +/// data: u32, +/// data2: u8, +/// other: [u32; 8], +/// } +/// +/// #[derive(AcpiTable, FromBytes, IntoBytes, Immutable, KnownLayout)] +/// #[repr(C, packed)] +/// #[signature('F', 'F', 'B', 'B')] +/// struct OtherAcpiTable(AcpiTableHeader, [u8; 256]); +pub use patina_macro::AcpiTable; +``` + +### Acpi service definition + +To support ACPI Tables that are DSTs, the interface consuming and retrieving + +```rust +pub trait AcpiProvider { + /// Installs the ACPI table and returns a key that can be used for future manipulation + /// + /// This method copies the byte slice into the correct memory type. + /// + /// ## Safety + /// + /// Meet the safety requirements of [AcpiTable]. + unsafe fn install_acpi_table(&self, acpi_table: &[u8]) -> Result; + + /// Uninstalls an ACPI table associated with the provided [TableKey]. + /// + /// Returns the underlying bytes representing the table. + fn uninstall_acpi_table(&self, table_key: TableKey) -> Option>; + + /// Retrieves an ACPI table by its table key. + fn get_acpi_table<'a>(&'a self, table_key: TableKey) -> Option<&'a [u8]>; + + /// Returns all currently installed tables in an iterable format + fn tables<'a>(&'a self) -> &'a [&'a [u8]] +} + +pub trait AcpiProviderExt { + fn install_acpi_table(&self, acpi_table: &T) -> Result { + self.install_acpi_table(acpi_table.as_bytes()) + } + + /// Returns the table if it exists, if the table signature matches expectations + /// + /// ## Returns + /// + /// Returns `None` if if the signatures do not match + /// Returns `None` if no key is found for the table + /// Returns `None` if zerocopy fails to convert the bytes + /// + fn get_acpi_table<'a, T: AcpiTable>(&'a self, table_key: TableKey) -> Option<&'a T> { + let table = self.get_acpi_table_unchecked(table_key)?; + + if table.header().signature != T::SIGNATURE { + return None + } + + table + } + + /// Returns the table if it exists without validating it's signature + /// + /// ## Returns + /// + /// Returns `None` if no key is found for the table + /// Returns `None` if zerocopy fails to convert the bytes + /// + fn unsafe get_acpi_table_unchecked<'a, T: AcpiTable>(&'a self, table_key: TableKey) -> Option<&'a T> { + let bytes = ::get_acpi_table(self, table_key)?; + + let len = ::ref_from_prefix(bytes)?.len() + ::ref_from_bytes(bytes[..len])? + } +} +``` + From 7437cc0e1fad824f6a27473718a6fe2a9eb2f103 Mon Sep 17 00:00:00 2001 From: Joey Vagedes Date: Mon, 20 Apr 2026 15:42:24 -0700 Subject: [PATCH 2/4] u --- .../text/0000-acpi-interface-improvements.md | 76 +++++++++++++++---- 1 file changed, 62 insertions(+), 14 deletions(-) diff --git a/docs/src/rfc/text/0000-acpi-interface-improvements.md b/docs/src/rfc/text/0000-acpi-interface-improvements.md index d5efeb1ee..2c4e98dc9 100644 --- a/docs/src/rfc/text/0000-acpi-interface-improvements.md +++ b/docs/src/rfc/text/0000-acpi-interface-improvements.md @@ -21,7 +21,7 @@ parseable structures. Supporting DSTs will require some fundamental changes to how ACPI table data is managed (described in-depth below), which means we have the opportunity to make some other fundamental changes to improve the code, such as using zerocopy -instead of raw pointer manipulation. +instead of raw pointer manipulation. This also allows for tightening the memory safety of the current design. ## Technology Background @@ -42,16 +42,6 @@ instead of raw pointer manipulation. ## Unresolved Questions -- Do we find the const SIGNATURE in the `AcpiTable` trait defintion useful? Currently we cache the `TypeId` when - adding a table with the service, which is used to validate against if someone attempts to retrieve the table from the - database. This has the downside that if a ACPI table was added via the protocol, we do not have a `TypeId` associated - with the table, and cannot validate it, if we attempt to retrieve it on the rust side. Replacing the TypeId with - signature validation would better validate the `C protocol -> Service` story while also 100% validating the - `Service -> Service` story. - -- As noted in the Technology Background, DSTs must always be a reference to the data, since they are an unknown size. - Examples of this are `&MyStruct`, `Box`, `Rc`, etc. - ## Alternatives The alternative is to continue the status quo. As mentioned above, DSTs are currently supported via the non-generic @@ -106,7 +96,7 @@ multiple zerocopy traits be implemented on it. /// 2. length: u32 /// 3. revision: u8 /// 4. checksum: u8 -unsafe trait AcpiTable: FromBytes + IntoBytes + Immutable + KnownLayout { +unsafe trait AcpiTable: FromBytes + IntoBytes + Immutable + KnownLayout + Packed { /// The signature of the table. Must match the signature field in the table. const SIGNATURE: u32 } @@ -133,12 +123,70 @@ unsafe trait AcpiTable: FromBytes + IntoBytes + Immutable + KnownLayout { pub use patina_macro::AcpiTable; ``` -### Acpi service definition +### ACPI Table defintion To support ACPI Tables that are DSTs, the interface consuming and retrieving ```rust -pub trait AcpiProvider { + +struct Table<'a, T: AcpiTable> { + data: Vec, + _phantom: PhantomData, +} + +impl Table<'_, T> { + pub fn as_bytes(&self) -> &[u8] { + &self.data + } + + pub fn as_bytes_mut(&mut self) -> &[u8] { + &mut self.data + } + + /// Clones and appends all elements into the ACPI Table. + /// + /// Useful for DST ACPI tables that are runtime parseable objects who can have table entries appended to. + pub fn extend_from_slice(&mut self, other: &[u8]) { + self.data.extend_from_slice(other) + } +} + +impl Deref for Table<'_, T> { + type Target = T; + + fn deref(&self) -> &Self::Target { + ::ref_from_bytes(&self).unwrap() + } +} + +impl DerefMut for Table<'_, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + ::mut_from_bytes(&self).unwrap() + } +} + +implTryInto for Table<'_, AcpiTableHeader> { + type Error = AcpiError; + + fn try_into(self) -> Result { + if self.header().signature != T::SIGNATURE { + return AcpiError::InvalidTableFormat; + } + + Table { + data: self.data, + _marker: core::marker::PhantomData + } + } + +} + +``` + +### ACPI service definition + +```rust +pub trait Acpi { /// Installs the ACPI table and returns a key that can be used for future manipulation /// /// This method copies the byte slice into the correct memory type. From f8af761e3656713bb156950c446dab3f3ba74b98 Mon Sep 17 00:00:00 2001 From: Joey Vagedes Date: Wed, 22 Apr 2026 15:25:02 -0700 Subject: [PATCH 3/4] update --- .../text/0000-acpi-interface-improvements.md | 593 +++++++++++++----- 1 file changed, 444 insertions(+), 149 deletions(-) diff --git a/docs/src/rfc/text/0000-acpi-interface-improvements.md b/docs/src/rfc/text/0000-acpi-interface-improvements.md index 2c4e98dc9..23de24f23 100644 --- a/docs/src/rfc/text/0000-acpi-interface-improvements.md +++ b/docs/src/rfc/text/0000-acpi-interface-improvements.md @@ -1,31 +1,38 @@ # RFC: `patina_acpi` service interface improvements -This RFC proposes multiple interface changes to the two services (`Service` and -`Service) to support ACPI Tables with unknown sizes (DSTs) and to generally improve user ergonomics -when using the service. +This RFC proposes interface changes to reduce the two services (`Service` and +`Service) to a single `Service` service and provide a public Helper struct,`Table` for +managing ACPI tables both internal to the service implementation and as a consumer of the service. This `Table` +structure would better support multiple types of ACPI tables, such as statically sized and dynamically sized ACPI +tables. ## Change Log -- 2026-04-16 +- 2026-04-22: Initial RFC created. ## Motivation The main motivation behind this change is to add support for DSTs (Dynamically Sized Types) in the generics portion of -the service interface (e.g. `Service::add_acpi_table` and -`Service::get_acpi_table`). DST ACPI tables are currently supported via non-generic interfaces, e.g. -`Service::install_acpi_table`, but it puts all unsafety burdons on you, such as (1) allocating to the -correct memory type and (2) ensuring the ACPI table has the correct format. This interface simply accepts a pointer to -a memory address and treats it as a ACPI table. This support would be highly beneficial as most ACPI tables tend to -have variable length - e.g. a header with a runtime variable number of bytes behind it representing N number of -parseable structures. - -Supporting DSTs will require some fundamental changes to how ACPI table data is managed (described in-depth below), -which means we have the opportunity to make some other fundamental changes to improve the code, such as using zerocopy -instead of raw pointer manipulation. This also allows for tightening the memory safety of the current design. +the service interface. DST ACPI tables are currently supported via usage of the non-generic interfaces ACPI service +methods, as they simply take a slice of bytes that start with an ACPI table header. This puts all safety burdons on the +developer. The proposed interface changes and additional support for DSTs would be highly beneficial as it would +provide a standard interface for interacting with both static and dynamically sized ACPI tables. + +Supporting DSTs will require some fundamental changes to how ACPI table data is managed (described in-depth below). The +high level description is that we will be removing the current generic table structure in favor of `Box<[u8]>`, which +self manages the ACPI page allocation while relying on `zerocopy` to interpret the table byte prefixes as an +`AcpiTableHeader`. Due to this, it will remove a large chunk of `unsafe` blocks throughout the crate. + +The final benefit is that of safety. This change removes many different areas of the code wrapped in `unsafe`, and most +importantly, stops the `get_acpi_table` method from returning a pointer directly to the actual ACPI table in memory. +Instead, it will now copy the bytes into new memory and provide that to the user. The EDKII EFIAPI side will need to +continue to provide the pointer the actual ACPI table in memory. ## Technology Background -- TODO +Please refer to [RFC 5 - ACPI](https://github.com/OpenDevicePartnership/patina/blob/main/docs/src/rfc/text/0005-acpi.md)'s +technology background. While these changes do not directly impact the logic related to ACPI table management, it is +touching the code that does so. ## Goals @@ -42,6 +49,9 @@ instead of raw pointer manipulation. This also allows for tightening the memory ## Unresolved Questions +- Do we want to specify the revision in the `AcpiTable` trait and require that revision matches in addition to + signature? + ## Alternatives The alternative is to continue the status quo. As mentioned above, DSTs are currently supported via the non-generic @@ -51,199 +61,484 @@ table to a table of your choosing. ## Prior Art +The prior implementation generically stored each ACPI table as a pointer to a leaked union representing the ACPI +table. It would then provide direct pointer access to the caller of `get_acpi_table` both for the patina component +and caller of the EDK II protocol. Additionally, the generic methods (such as `get_acpi_table`) would directly +return a `T`, making it difficult to use DST style ACPI tables, but did successfully clone the structure so that +it was not directly referencing the true ACPI table memory. +## Rust Code Design +This change is focused mostly on the service API to provide common logic for table management (creation, updating) +on the consumer side of the service. Only a very minimal set of changes are made to the service implementation to +accompidate the interface changes. These changes will remove unsafe code in the service implementation as table +access will move away from pointers and dereferencing to slices and safe byte interpretation via `zerocopy`. +Starting with changes to the service implementation and data management: A new unsafe trait, `AcpiTable` is created to +house all unsafe requirements that must be upheld to prevent UB from within the service. This is mainly that the table +must start with an `AcpiTableHeader`, but it also enforces your ACPI table implement certain `zerocopy` traits to +ensure the service can re-interpret the prefix bytes as an AcpiTableHeader. A derive macro is provided to automatically +do this validation when implementing the `AcpiTable` trait. Implementation details will be discussed below. -## Rust Code Design +On the consumer side service changes, a new struct, `Table` is used to ensure you can safely work with your ACPI table +without breaking any UB guarantees. While this table supports statically sized tables (allowing for safe interpretation +of the table as the underlying type), it's focus is on simplifying the data management when it comes to dynamically +sized tables. Broadly speaking, it provides an API for inserting, removing, and manipulating elements at the end of +a dynamically sized ACPI table with formats similar to this: + +```rust +struct CustomAcpiTable { + header: AcpiTableHeader, + fields: CustomFields, + elements: [CustomElement] +} +``` -### AcpiTable Trait Definition +It should be noted that this interface allows the elements to be statically sized or dynamically sized. More details, +and the provided interface, will be discussed below. -Below is the expected AcpiTable trait defintion. A derive macro will be provided for types that start with a -`AcpiTableHeader` structure, however it can also be manually implemented if a type does not start with an -`AcpiTableHeader`, but has the required fields as specified in the trait documentation. This trait also requires that -multiple zerocopy traits be implemented on it. +### ACPI Table Trait Definition + +The ACPI table trait (`AcpiTable`) is dual hatted. It provides safety guarantees for the service implementation, and +provides important information to the `Table` struct (as described below) to provide safe table manipulation by +consumers of `patina_acpi`. ```rust -/// A trait implemented on a type that represents an ACPI Table. -/// -/// ## Examples +/// A trait for types that represent valid ACPI tables. /// -/// ```rust +/// This trait should not be implemented manually. Use the derive macro to implement this trait on a type representing +/// an ACPI table. /// -/// #[derive(FromBytes, IntoBytes, Immutable, KnownLayout)] -/// #[repr(C, packed)] -/// struct MyTable { -/// signature: u32, -/// length: u32, -/// revision: u8, -/// checksum: u8, -/// data_differing_from_header: [u8; 256] -/// } +/// # Safety /// -/// // SAFETY: This table starts with the required fields -/// unsafe impl AcpiTable for MyTable { -/// const SIGNATURE: u32 = signature!('F', 'F', 'A', 'B'); -/// } -/// ``` +/// Implementors must ensure that: +/// - The type begins with an `AcpiTableHeader` as its first field /// -/// ## Safety +/// # Examples /// -/// The structure implementing this trait must start with either a [AcpiTableHeader] or the following for -/// fields in this exact order: -/// -/// 1. signature: u32 -/// 2. length: u32 -/// 3. revision: u8 -/// 4. checksum: u8 -unsafe trait AcpiTable: FromBytes + IntoBytes + Immutable + KnownLayout + Packed { - /// The signature of the table. Must match the signature field in the table. - const SIGNATURE: u32 +/// +/// ``` +pub unsafe trait AcpiTable: FromBytes + IntoBytes + KnownLayout + Unaligned + Immutable { + /// The ACPI table signature, used for validation when converting a table to the implementor type + const SIGNATURE: u32; + + /// The single struct that contains all additional fields before the table of optional [Self::Element]s + type Fields: IntoBytes + Immutable = (); + + /// The element type for the variable-length array portion of DST ACPI tables + type Element: Element + ?Sized = (); + + /// The kind of ACPI table this is (Fixed, Dst). Used to gate slice style methods to only DST ACPI tables. + #[allow(private_bounds)] + type Kind: TableKind = Fixed; } -/// A derive macro for the AcpiTable trait for types that start with [AcpiTableHeader] -/// -/// ## Examples +/// An element that can be used for the variable-length array portion of DST ACPI tables. /// -/// ```rust -/// #[derive(AcpiTable, FromBytes, IntoBytes, Immutable, KnownLayout)] -/// #[repr(C, packed)] -/// #[signature('F', 'F', 'A', 'B')] -/// struct MyAcpiTable { -/// header: AcpiTableHeader, -/// data: u32, -/// data2: u8, -/// other: [u32; 8], -/// } +/// This trait has two roles: (1) Ensure that each element can be converted to and from bytes using `zerocopy` and +/// (2) to differentiate between Sized and Unsized elements for indexing optimizations. +pub trait Element: IntoBytes + FromBytes + Immutable + KnownLayout + Unaligned { + /// A non-zero size for types that are `Sized`. + /// + /// Element::SIZE == 0 indicates that this is a DST and that [Self::element_size] must be used to determine size. + const SIZE: usize = 0; + + /// Returns the runtime size (in bytes) of the element. + /// + /// This is used to determine the size (in bytes) + fn element_size(&self) -> usize; +} + +/// A trait to specify the type of ACPI table this is /// -/// #[derive(AcpiTable, FromBytes, IntoBytes, Immutable, KnownLayout)] -/// #[repr(C, packed)] -/// #[signature('F', 'F', 'B', 'B')] -/// struct OtherAcpiTable(AcpiTableHeader, [u8; 256]); -pub use patina_macro::AcpiTable; +/// It is purposefully kept private so only our implementations can be used. +trait TableKind { } + +#[doc(hidden)] +pub struct Fixed; + +impl TableKind for Fixed { } + +#[doc(hidden)] +pub struct Dst; + +impl TableKind for Dst { } ``` -### ACPI Table defintion +### ACPI Table Management Changes -To support ACPI Tables that are DSTs, the interface consuming and retrieving +As mentioned above, there are minimal changes to the table management logic inside the service itself. The core of the +change is that the object that manages the underlying data for each table is being changed from a custom struct that +wraps a leaked pointer, to a `Box<[u8]>`. In the current implementation, a union is used to treat the underlying data as +either a (1) signature, (2) AcpiTableHeader, or (3), the entire table. In the new implementation as seen below, we will +utilize `zerocopy` to safely interpret the prefix bytes as a `AcpiTableHeader`, which is used for all interior table +management. Other then some helper functions to assist with this, no other changes will be made. ```rust - -struct Table<'a, T: AcpiTable> { - data: Vec, - _phantom: PhantomData, +struct StandardAcpiProvider { + /// Platform-installed ACPI tables + acpi_tables: TplMutex>>, + ... } -impl Table<'_, T> { - pub fn as_bytes(&self) -> &[u8] { - &self.data +impl AcpiProvider for StandardAcpiProvider { + /// Installs the bytes as an ACPI table + /// + /// Returns a key usable with [Acpi::get_table]. + /// + /// ## Safety + /// + /// - The table must begin with a valid [AcpiTableHeader] + unsafe fn install_acpi_table(&self, table: &[u8]) -> Result { + let table: Box<[u8], PageFree> = Self::allocate_table(table)?; + let header = AcpiTableHeader::ref_from_bytes(&table).map_err(|_| AcpiError::InvalidLength); + + let table_key = match header.signature { + signature::FACS => { /* Special Handling */ } + signature::FADT => { /* Special Handling */ } + signature::DSDT => { /* Special Handling */ } + _ => self.install_generic_table(table)? + }; + + self.publish_tables()?; + self.notify_acpi_list(table_key)?; + Ok(table_key) } - pub fn as_bytes_mut(&mut self) -> &[u8] { - &mut self.data + /// Uninstalls the ACPI table. + fn uninstall_acpi_table(&self, table_key: usize) -> Result<(), AcpiTableError> { + // Uninstalling a table is slightly simplified as the Box<[u8]> will automatically deallocate the table once + // it is removed form the BTreeMap and goes out of scope. + // + // Note: We are still responsible for updating the XSDT or other special table handling for the DSDT / FACS } - /// Clones and appends all elements into the ACPI Table. - /// - /// Useful for DST ACPI tables that are runtime parseable objects who can have table entries appended to. - pub fn extend_from_slice(&mut self, other: &[u8]) { - self.data.extend_from_slice(other) + /// Returns a copy of the table if it exists. + fn get_table(&self, key: &usize) -> Result, AcpiTableError> { + let copied_bytes: Vec = self.acpi_tables.lock().get(key).ok_or(AcpiEror::NotFound)?.as_ref().clone(); + let table: Table = Table { + data: copied_bytes, + _marker: PhantomData, + }; + + Ok(table) } } -impl Deref for Table<'_, T> { - type Target = T; +``` - fn deref(&self) -> &Self::Target { - ::ref_from_bytes(&self).unwrap() - } -} +### ACPI Service Definition -impl DerefMut for Table<'_, T> { - fn deref_mut(&mut self) -> &mut Self::Target { - ::mut_from_bytes(&self).unwrap() - } -} +Below is the updated ACPI service definition that fully replaces the `AcpiTableManager` and `AcpiProvider` services. + +```rust +pub trait Acpi { + /// Installs the bytes as an ACPI table + /// + /// Returns a key usable with [Acpi::get_table]. + /// + /// ## Safety + /// + /// - The bytes must represent a valid ACPI Table. A valid ACPI Table meets the same safety requirements as + /// required for the [AcpiTable] trait. + unsafe fn install_table(&self, table: &[u8]) -> Result; -implTryInto for Table<'_, AcpiTableHeader> { - type Error = AcpiError; + /// Gets a table by its key. + /// + /// ## Errors + /// + /// Returns [AcpiError::NotFound] if the table is not found. + /// Returns [AcpiError::InvalidLength] if the table is not long enough for an [AcpiTableHeader]. + fn get_table(&self, key: usize) -> Result, AcpiError>; - fn try_into(self) -> Result { - if self.header().signature != T::SIGNATURE { - return AcpiError::InvalidTableFormat; - } + /// Removes a table and returns it to the caller + /// + /// ## Errors + /// + /// Returns [AcpiError::NotFound] if the table is not found. + /// Returns [AcpiError::InvalidLength] if the table is not long enough for an [AcpiTableHeader]. + fn uninstall_table(&self, key: usize) -> Result, AcpiError>; - Table { - data: self.data, - _marker: core::marker::PhantomData - } - } + /// Registers a function that will be called whenever a new ACPI table is installed. + fn register_notify(&self, notify_fn: AcpiNotifyFn); + + /// Unregisters a function that will be called whenever a new ACPI table is installed. + /// + /// ## Errors + /// + /// Returns [AcpiError::InvalidNotifyUnregister] if the notify fn was not found. + fn unregister_notify(&self, notify_fn: AcpiNotifyFn) -> Result<(), AcpiError>; + /// Returns a copy of all registered ACPI tables. + fn collect_tables(&self) -> Vec>; } -``` +/// An extension trait to provide generic methods for convenience +pub trait AcpiExt { + /// Installs the ACPI Table + /// + /// Returns a key usable with [Acpi::get_table]. + /// + fn install_table(&self, table: &T) -> Result; -### ACPI service definition + /// Gets a table by it's key. + /// + /// ## Errors + /// + /// Returns [AcpiError::NotFound] if the table is not found + /// Returns [AcpiError::InvalidSignature] if the table signature does not match [T::SIGNATURE] + /// Returns [AcpiError::BadFormat] if the conversion to `T` failed. + fn get_table(&self, key: u32) -> Result, AcpiError>; -```rust -pub trait Acpi { - /// Installs the ACPI table and returns a key that can be used for future manipulation - /// - /// This method copies the byte slice into the correct memory type. - /// - /// ## Safety + /// A convenience method to uninstall a table, edit it, and re-install the table /// - /// Meet the safety requirements of [AcpiTable]. - unsafe fn install_acpi_table(&self, acpi_table: &[u8]) -> Result; + /// ## Errors + /// + /// Returns [AcpiError::NotFound] if the table is not found + /// Returns [AcpiError::InvalidSignature] if the table signature does not match T::SIGNATURE + /// Returns [AcpiError::BadFormat] if the conversion to `T` failed. + #[must_use] + fn mutate_table(&self, key: u32, f: F) -> Result<(), AcpiError> + where + T: AcpiTable, + F: FnOnce(&mut Table) -> Result<(), AcpiError>; +} + +/// This is a way to have generic methods on a service interface, by implementing the generics on the +/// service itself. +impl AcpiExt on Service { ... } + +``` - /// Uninstalls an ACPI table associated with the provided [TableKey]. +### Consumer ACPI Table Management + +As alluded to, a major portion of this change was to better support ACPI table management generically; particularly +for DST style ACPI tables. This change introduces the `Table` structure, as referenced many times above, as the way to +manage ACPI tables. As seen above, this structure is used by the service implementation to treat all tables generically +as just their ACPI header, but it also provides an interface for consumers of the service to safely generate ACPI +tables that can consumed by the service. The main purpose is to create an easy way to generate and consume dynamically +sized ACPI tables outside of the service. + +The `Table` structure provides a rich interface for creating ACPI Tables and working with DSTs as if they were a vector +of the underlying elements. That is to say a ACPI Table with a layout of `{ AcpiHeader, [MyElement] }` should will +have an interface similar to a `Vec`. Below is the provided interface. The implementations are left out for +brevity's sake + +```rust +pub struct Table { + data: Vec, + _marker: PhantomData, +} + +/// Methods in this impl block are common to all ACPI tables. +impl Table { + /// Creates a new ACPI table with the given header and fields. /// - /// Returns the underlying bytes representing the table. - fn uninstall_acpi_table(&self, table_key: TableKey) -> Option>; + /// If your structure has no fields, consider using [Table::new] instead. + pub fn new_with_fields(header: AcpiTableHeader, fields: T::Fields) -> Self; + + /// Updates the length field of the header to match [Self::data]'s length + fn update_length(&mut self); + + /// Performs a checksum and sets the checksum field to it. + fn checksum(&mut self); - /// Retrieves an ACPI table by its table key. - fn get_acpi_table<'a>(&'a self, table_key: TableKey) -> Option<&'a [u8]>; + /// Returns a reference to the ACPI table header. + fn header(&self) -> &AcpiTableHeader; - /// Returns all currently installed tables in an iterable format - fn tables<'a>(&'a self) -> &'a [&'a [u8]] + /// Returns a mutable reference to the ACPI table header. + fn header_mut(&mut self) -> &mut AcpiTableHeader; + + /// Consumes the table and returns the underlying bytes. + pub fn into_bytes(self) -> Vec; } -pub trait AcpiProviderExt { - fn install_acpi_table(&self, acpi_table: &T) -> Result { - self.install_acpi_table(acpi_table.as_bytes()) - } +/// Methods in this impl block are specific to ACPI tables with no fields. +/// +/// ## Panic +/// +/// This function will panic if there is a mismatch of the expected table signature. +impl > Table { + /// Creates a new ACPI table with no fields. + pub fn new(header: AcpiTableHeader) -> Self; +} + +/// Methods in this impl block are specific to ACPI tables with a dynamically sized list of elements at the end. +impl + ?Sized> Table { + /// Returns the number of elements in the table. + pub fn len(&self) -> usize; + + /// Returns `true` if the table contains no elements. + pub fn is_empty(&self) -> bool; - /// Returns the table if it exists, if the table signature matches expectations + /// Pushes a new [AcpiTable::Element] to the end of the table. /// - /// ## Returns + /// Useful for ACPI tables that have a dynamically sized list of elements at the end. /// - /// Returns `None` if if the signatures do not match - /// Returns `None` if no key is found for the table - /// Returns `None` if zerocopy fails to convert the bytes + /// ## Errors /// - fn get_acpi_table<'a, T: AcpiTable>(&'a self, table_key: TableKey) -> Option<&'a T> { - let table = self.get_acpi_table_unchecked(table_key)?; + /// Returns [AcpiError::AllocationError] if the new table size would exceed [u32::MAX]. + pub fn push(&mut self, element: &T::Element); - if table.header().signature != T::SIGNATURE { - return None - } + /// Pushes multiple [AcpiTable::Element]s to the end of the table. + /// + /// Useful for ACPI tables that have a dynamically sized list of elements at the end. + /// + /// ## Errors + /// + /// Returns [AcpiError::AllocationError] if the new table size would exceed [u32::MAX]. + pub fn extend_from_slice(&mut self, elements: &[&T::Element]) -> Result<(), AcpiError>; - table - } + /// Returns a reference to the element at the given index, if it exists. + pub fn get(&self, index: usize) -> Option<&T::Element>; + + /// Returns a mutable reference to the element at the given index, if it exists. + pub fn get_mut(&mut self, index: usize) -> Option<&mut T::Element>; + + /// Removes the element at the given index from the table and returns it, if it exists. + /// + /// **Note:** Since the element may be a DST, the underlying bytes are returned instead of the type. + pub fn remove(&mut self, index: usize) -> Option>; - /// Returns the table if it exists without validating it's signature + /// Removes the last element from the table and returns it, if it exists. /// - /// ## Returns + /// **Note:** Since the element may be a DST, the underlying bytes are returned instead of the type. + pub fn pop(&mut self) -> Option>; + + /// Returns an iterator over the elements of the table. + pub fn iter(&self) -> impl Iterator; + + /// Returns a mutable iterator over the elements of the table. + pub fn iter_mut(&mut self) -> impl Iterator; +} + +impl Table { + /// Converts a generic ACPI table into a specific table. /// - /// Returns `None` if no key is found for the table - /// Returns `None` if zerocopy fails to convert the bytes + /// ## Errors /// - fn unsafe get_acpi_table_unchecked<'a, T: AcpiTable>(&'a self, table_key: TableKey) -> Option<&'a T> { - let bytes = ::get_acpi_table(self, table_key)?; + /// Returns [AcpiError::InvalidSignature] if the table signature does not match T::SIGNATURE. + /// Returns [AcpiError::InvalidLength] if the table is not long enough for a T. + fn try_into(self) -> Result, AcpiError>; +} + +impl Deref for Table { + type Target = T; +} + +impl DerefMut for Table + +impl + ?Sized> Index for Table { + type Output = T::Element; +} + +impl + ?Sized> IndexMut for Table + +impl <'a, T: AcpiTable + ?Sized> IntoIterator for &'a Table { + type Item = &'a T::Element; + type IntoIter = Iter<'a, T>; +} + +impl <'a, T: AcpiTable + ?Sized> IntoIterator for &'a mut Table { + type Item = &'a mut T::Element; + type IntoIter = IterMut<'a, T>; +} +``` - let len = ::ref_from_prefix(bytes)?.len() - ::ref_from_bytes(bytes[..len])? +### Consumer ACPI Table Management Examples + +This first example is consumer interface for statically sized ACPI tables + +```rust +#[derive(FromBytes, IntoBytes, KnownLayout, Unaligned, Immutable)] +struct Fields { + a: u32, + b: u32, + c: u64, + d: u64 +} + +#[derive(AcpiTable, FromBytes, IntoBytes, KnownLayout, Unaligned, Immutable)] +#[signature('S', 'T', 'B', 'L')] +struct SimpleTable { + header: AcpiTableHeader, + fields: Fields, +} + +struct MyComponent; + +#[component] +impl MyComponent { + fn entry_point(self, acpi: Service) -> Result<()> { + + let header = AcpiTableHeader { ... }; + + // let table = Table::new(header) -> This method is not available because this struct has fields + let mut table: Table = Table::new_with_fields(header, Fields { a: 20, b: 40, c: 60, d: 70}); + + let key = acpi.install_table(table).unwrap(); + + acpi.mutate_table(key, |table: &mut Table| { + table.fields.c = 200; + }).unwrap() + + assert_eq!(acpi.get_table(key).unwrap().fields.c, 200); + + Ok(()) } } ``` +The second example is a DST style table where the element is statically sized + +```rust +#[derive(FromBytes, IntoBytes, KnownLayout, Unaligned, Immutable)] +struct Element { + a: u32, + b: u32, +} + +#[derive(AcpiTable, FromBytes, IntoBytes, KnownLayout, Unaligned, Immutable)] +#[signature('S', 'T', 'B', 'L')] +struct DstTable { + header: AcpiTableHeader, + elements: [Element], +} + +struct MyComponent; + +#[component] +impl MyComponent { + fn entry_point(self, acpi: Service) -> Result<()> { + + let header = AcpiTableHeader { ... }; + + let mut table: Table = Table::new(header); + + table.push(&Element { a: 20, b: 40 }) + table.push(&Element { a: 60, b: 80 }) + table.extend_from_slice(&[&Element { a: 100, b: 120}, &Element { a: 140, b: 160 }]); + + assert_eq!(table.len(), 4); + + let _ table.pop().unwrap(); + + assert_eq!(table.len(), 3); + + let iter = table.iter(); + + let _ = table.get(0).unwrap(); + let _ = table[0]; + + let key = acpi.install_table(table).unwrap(); + + acpi.mutate_table(key, |table: &mut Table| { + for element in &mut table { + element.a *= 2; + } + }).unwrap(); + + Ok(()) + } +} +``` From d22dcfa8a3efcd18f49661978ec6a0c0348402f7 Mon Sep 17 00:00:00 2001 From: Joey Vagedes Date: Wed, 22 Apr 2026 15:31:19 -0700 Subject: [PATCH 4/4] update --- .../text/0000-acpi-interface-improvements.md | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/docs/src/rfc/text/0000-acpi-interface-improvements.md b/docs/src/rfc/text/0000-acpi-interface-improvements.md index 23de24f23..e62ab4834 100644 --- a/docs/src/rfc/text/0000-acpi-interface-improvements.md +++ b/docs/src/rfc/text/0000-acpi-interface-improvements.md @@ -1,7 +1,7 @@ # RFC: `patina_acpi` service interface improvements This RFC proposes interface changes to reduce the two services (`Service` and -`Service) to a single `Service` service and provide a public Helper struct,`Table` for +`Service) to a single `Service` service and provide a public Helper struct, `Table` for managing ACPI tables both internal to the service implementation and as a consumer of the service. This `Table` structure would better support multiple types of ACPI tables, such as statically sized and dynamically sized ACPI tables. @@ -14,12 +14,12 @@ tables. The main motivation behind this change is to add support for DSTs (Dynamically Sized Types) in the generics portion of the service interface. DST ACPI tables are currently supported via usage of the non-generic interfaces ACPI service -methods, as they simply take a slice of bytes that start with an ACPI table header. This puts all safety burdons on the +methods, as they simply take a slice of bytes that start with an ACPI table header. This puts all safety burdens on the developer. The proposed interface changes and additional support for DSTs would be highly beneficial as it would provide a standard interface for interacting with both static and dynamically sized ACPI tables. Supporting DSTs will require some fundamental changes to how ACPI table data is managed (described in-depth below). The -high level description is that we will be removing the current generic table structure in favor of `Box<[u8]>`, which +high-level description is that we will be removing the current generic table structure in favor of `Box<[u8]>`, which self manages the ACPI page allocation while relying on `zerocopy` to interpret the table byte prefixes as an `AcpiTableHeader`. Due to this, it will remove a large chunk of `unsafe` blocks throughout the crate. @@ -71,7 +71,7 @@ it was not directly referencing the true ACPI table memory. This change is focused mostly on the service API to provide common logic for table management (creation, updating) on the consumer side of the service. Only a very minimal set of changes are made to the service implementation to -accompidate the interface changes. These changes will remove unsafe code in the service implementation as table +accommodate the interface changes. These changes will remove unsafe code in the service implementation as table access will move away from pointers and dereferencing to slices and safe byte interpretation via `zerocopy`. Starting with changes to the service implementation and data management: A new unsafe trait, `AcpiTable` is created to @@ -82,7 +82,7 @@ do this validation when implementing the `AcpiTable` trait. Implementation detai On the consumer side service changes, a new struct, `Table` is used to ensure you can safely work with your ACPI table without breaking any UB guarantees. While this table supports statically sized tables (allowing for safe interpretation -of the table as the underlying type), it's focus is on simplifying the data management when it comes to dynamically +of the table as the underlying type), its focus is on simplifying the data management when it comes to dynamically sized tables. Broadly speaking, it provides an API for inserting, removing, and manipulating elements at the end of a dynamically sized ACPI table with formats similar to this: @@ -145,10 +145,20 @@ pub trait Element: IntoBytes + FromBytes + Immutable + KnownLayout + Unaligned { /// Returns the runtime size (in bytes) of the element. /// - /// This is used to determine the size (in bytes) + /// This is used to determine the size (in bytes) of the element. fn element_size(&self) -> usize; } +/// A blanket implementation of `Element` for all sized types +impl Element for E { + const SIZE: usize = core::mem::size_of::(); + + fn element_size(&self) -> usize { + Self::SIZE + } +} + + /// A trait to specify the type of ACPI table this is /// /// It is purposefully kept private so only our implementations can be used. @@ -171,8 +181,8 @@ As mentioned above, there are minimal changes to the table management logic insi change is that the object that manages the underlying data for each table is being changed from a custom struct that wraps a leaked pointer, to a `Box<[u8]>`. In the current implementation, a union is used to treat the underlying data as either a (1) signature, (2) AcpiTableHeader, or (3), the entire table. In the new implementation as seen below, we will -utilize `zerocopy` to safely interpret the prefix bytes as a `AcpiTableHeader`, which is used for all interior table -management. Other then some helper functions to assist with this, no other changes will be made. +utilize `zerocopy` to safely interpret the prefix bytes as an `AcpiTableHeader`, which is used for all interior table +management. Other than some helper functions to assist with this, no other changes will be made. ```rust struct StandardAcpiProvider { @@ -208,14 +218,14 @@ impl AcpiProvider for StandardAcpiProvider { /// Uninstalls the ACPI table. fn uninstall_acpi_table(&self, table_key: usize) -> Result<(), AcpiTableError> { // Uninstalling a table is slightly simplified as the Box<[u8]> will automatically deallocate the table once - // it is removed form the BTreeMap and goes out of scope. + // it is removed from the BTreeMap and goes out of scope. // // Note: We are still responsible for updating the XSDT or other special table handling for the DSDT / FACS } /// Returns a copy of the table if it exists. fn get_table(&self, key: &usize) -> Result, AcpiTableError> { - let copied_bytes: Vec = self.acpi_tables.lock().get(key).ok_or(AcpiEror::NotFound)?.as_ref().clone(); + let copied_bytes: Vec = self.acpi_tables.lock().get(key).ok_or(AcpiError::NotFound)?.as_ref().clone(); let table: Table = Table { data: copied_bytes, _marker: PhantomData, @@ -281,7 +291,7 @@ pub trait AcpiExt { /// fn install_table(&self, table: &T) -> Result; - /// Gets a table by it's key. + /// Gets a table by its key. /// /// ## Errors /// @@ -306,7 +316,7 @@ pub trait AcpiExt { /// This is a way to have generic methods on a service interface, by implementing the generics on the /// service itself. -impl AcpiExt on Service { ... } +impl AcpiExt for Service { ... } ``` @@ -320,7 +330,7 @@ tables that can consumed by the service. The main purpose is to create an easy w sized ACPI tables outside of the service. The `Table` structure provides a rich interface for creating ACPI Tables and working with DSTs as if they were a vector -of the underlying elements. That is to say a ACPI Table with a layout of `{ AcpiHeader, [MyElement] }` should will +of the underlying elements. That is to say, an ACPI Table with a layout of `{ AcpiHeader, [MyElement] }` will have an interface similar to a `Vec`. Below is the provided interface. The implementations are left out for brevity's sake @@ -459,7 +469,7 @@ struct Fields { } #[derive(AcpiTable, FromBytes, IntoBytes, KnownLayout, Unaligned, Immutable)] -#[signature('S', 'T', 'B', 'L')] +#[signature = "STBL"] struct SimpleTable { header: AcpiTableHeader, fields: Fields,