feat(opentmk): add acpi table reader support #2363
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull Request Overview
This PR implements ACPI table parsing in the UEFI environment to dynamically determine the number of logical processors from the MADT (Multiple APIC Description Table), replacing a hardcoded processor count.
- Adds a new
acpi_wrapmodule with functions to parse ACPI tables (RSDP, XSDT, MADT) - Updates
get_vp_count()to use actual APIC count from MADT instead of returning a hardcoded value of 4 - Adds
acpi_specdependency to support ACPI table parsing
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| opentmk/src/uefi/acpi_wrap.rs | New module implementing ACPI table parsing to extract processor count from MADT |
| opentmk/src/uefi/mod.rs | Exposes the new acpi_wrap module and fixes entry attribute syntax |
| opentmk/src/platform/hyperv/arch/x86_64/ctx.rs | Updates get_vp_count() to call ACPI parsing on UEFI, returns error on other platforms |
| opentmk/src/tmkdefs.rs | Adds new AcpiError variant to the error enum |
| opentmk/Cargo.toml | Adds acpi_spec dependency |
| Cargo.lock | Updates lock file with acpi_spec dependency |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| fn get_vp_count(&self) -> TmkResult<u32> { | ||
| // TODO: use ACPI to get the actual count | ||
| Ok(4) | ||
| #[cfg(target_os = "uefi")] |
| let system_table = uefi::table::system_table_raw(); | ||
|
|
||
| if system_table.is_none() { | ||
| return Err(TmkError::AcpiError); |
There was a problem hiding this comment.
These errors should carry more detail so we can tell which of these conditions we've hit.
| return Err(TmkError::AcpiError); | ||
| } | ||
|
|
||
| let mut system_table = system_table.unwrap(); |
There was a problem hiding this comment.
let Ok(system_table) = system_table else { return Err(TmkError::AcpiError) };
| // SAFETY: UEFI guarantees that the configuration table pointer is valid for the number of entries | ||
| let config_slice = unsafe { core::slice::from_raw_parts(config_table_ptr, config_count) }; | ||
|
|
||
| let find = |guid| { |
There was a problem hiding this comment.
just inline this if it's only used once?
| use crate::tmkdefs::TmkError; | ||
| use crate::tmkdefs::TmkResult; | ||
|
|
||
| fn get_rsdp_ptr() -> TmkResult<NonNull<Rsdp>> { |
There was a problem hiding this comment.
Should this function return a &'static Rsdp instead? Is that safe to do? It'd be more useful to its callers that way at least.
There was a problem hiding this comment.
Same applies to other functions in here.
| Ok(NonNull::new(xsdt_address as *mut Header).ok_or(TmkError::AcpiError)?) | ||
| } | ||
|
|
||
| fn get_madt_ptr() -> TmkResult<NonNull<Header>> { |
There was a problem hiding this comment.
Should we have some sort of AcpiContext that gets carried around and saved so that we don't have to repeat this parsing if we call get_apic_count_from_madt multiple times?
There was a problem hiding this comment.
Like we could provide platform contexts with an AcpiContext that finds all the tables ahead of time and holds on to an XsdtParser, MadtParser, RsdpParser, etc, and then we don't have to repeat any of the parsing ever.
| }) | ||
| .map(|u64| { | ||
| // SAFETY: the address is valid as it was found in the ACPI tables, UEFI guarantees their validity | ||
| unsafe { transmute::<*const c_void, &Header>(*u64 as *const c_void) } |
There was a problem hiding this comment.
We should avoid using transmute whenever possible, use the pointer methods for casting and as_ref instead.
| use crate::tmkdefs::TmkResult; | ||
|
|
||
| fn get_rsdp_ptr() -> TmkResult<NonNull<Rsdp>> { | ||
| let system_table = uefi::table::system_table_raw(); |
There was a problem hiding this comment.
Would it be more worthwhile to move some of this code into acpi_spec as a SystemTableParser, RsdpParser, etc, even if they don't expose everything those tables have just yet?
2 participants
adding additional code to read the CPU count from ACPI table.