RFC: ACPI Service Interface Changes#1474
Conversation
✅ QEMU Validation PassedAll QEMU validation jobs completed successfully.
Workflow run: https://github.com/OpenDevicePartnership/patina/actions/runs/24806567886 Boot Time to EFI Shell
Dependencies
This comment was automatically generated by the Patina QEMU PR Validation Post workflow. |
github-actions
Bot
added
the
impact:non-functional
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Javagedes
requested review from
DanielMaddy,
cfernald,
kouchekiniad,
makubacki and
os-d
| @@ -0,0 +1,554 @@ | |||
| # RFC: `patina_acpi` service interface improvements | |||
There was a problem hiding this comment.
One process question, should this be an amendment to the original patina-acpi RFC instead of a new one?
|
|
||
| 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 |
There was a problem hiding this comment.
| Instead, it will now copy the bytes into new memory and provide that to the user. The EDKII EFIAPI side will need to | |
| Instead, it will now copy the bytes into new memory and provide that to the user. The C FFI interface will need to |
nit: Not scoped to EDK II
|
|
||
| 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<T>`) would directly |
There was a problem hiding this comment.
| and caller of the EDK II protocol. Additionally, the generic methods (such as `get_acpi_table<T>`) would directly | |
| and caller of the UEFI spec protocol. Additionally, the generic methods (such as `get_acpi_table<T>`) would directly |
| /// | ||
| /// 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 { |
There was a problem hiding this comment.
For the sake of avoiding internal panics, should these be TryFromBytes to allow handling malform/unexpected elements without panicking in the parser?
| /// An element that can be used for the variable-length array portion of DST ACPI tables. | ||
| /// | ||
| /// 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. |
There was a problem hiding this comment.
How does ZeroCopy work is element can take different forms? A good example is the MADT where not only are the elements different sizes but also completely different but well-structured contents. This could be a union, but I assume zerocopy may be unhappy if the union size is greater than the bytes provided.
3 participants
Description
Current Status: Draft
Implementation Example: In the works
This RFC proposes interface changes to reduce the two services (
Service<dyn AcpiProvider>andService<AcpiTableManager>) to a singleService<dyn Acpi>service and provide a public Helper struct,Tableformanaging ACPI tables both internal to the service implementation and as a consumer of the service. This
Tablestructure would better support multiple types of ACPI tables, such as statically sized and dynamically sized ACPI
tables.
How This Was Tested
N/A
Integration Instructions
N/A