feat(acpi): add RSDP discovery from BIOS memory regions.#2124
feat(acpi): add RSDP discovery from BIOS memory regions.#2124donjuanplatinum wants to merge 5 commits into
Conversation
impl ACPI RSDP discovery for legacy BIOS platforms by scanning the standard locations defined by the ACPI specification. The impl follow the linux kernel drivers/acpi/acpica/tbxfroot.c checking: - EBDA region - BIOS reserved memory range 0xE0000-0xFFFFF Signed-off-by: DonjuanPlatinum <donplat@barrensea.org>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ddbf7fe02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
impl. 2. when len < 36 || len > available_len, rsdp_valid return false
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bc222838f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Donjuanplatinum <donplat@barrensea.org>
fslongjin
left a comment
There was a problem hiding this comment.
Thanks for working through the earlier feedback. Moving the BIOS scan before mm_init() and clamping the EBDA window are both good corrections.
I am requesting changes because two correctness/safety issues remain:
- The RSDP pre-validator does not have the same revision/length contract as Linux or the downstream
acpi-rsparser. This can accept a candidate that later causes an out-of-bounds validation read or selects XSDT under the wrong revision semantics. - The scan loop skips the final 16-byte-aligned candidate start in each ACPI search window.
Additional follow-ups:
- Please distinguish “RSDP not found” from EarlyIoRemap map/unmap failures, preferably with
Result<Option<PhysAddr>, SystemError>, and propagate cleanup failures. - The comment saying this function is safe after
mm_init()contradicts theEarlyIoRemap/pseudo-mapper contract and the current call site. Please correct it. - The unrelated
unmap_parents: true -> falsechange inno_init.rschanges the release semantics for every EarlyIoRemap user. Please split it out and provide an invariant/rationale and tests, or revert it if it is not required for this feature. - Add table-driven coverage for revision 0/1/2+, fixed 20/36-byte checksums, malformed lengths, multiple false candidates, and candidates at the first and last 16-byte-aligned positions. A PVH test with
rsdp_paddr == 0should cover the integration path.
For the next revision, I strongly recommend downloading the Linux 6.6.139 source tree into your local workspace and asking your coding agent to review the changes against the actual Linux code, rather than relying on recollection or a single snippet. In particular, have the agent cross-check drivers/acpi/acpica/tbxfroot.c, drivers/acpi/acpica/tbutils.c, drivers/acpi/osl.c, and arch/x86/boot/compressed/acpi.c, together with DragonOS's locked acpi-rs implementation and its unsafe mapping/validation contracts. After making changes, ask the agent to perform the same comparison again to catch semantic or lifetime regressions.
Please re-request review after the two blocking issues and the parser tests are addressed.
There are some issues we need to ensure with upstream
/* Use XSDT if present and not overridden. Otherwise, use RSDT */
if ((rsdp->revision > 1) &&
rsdp->xsdt_physical_address && !acpi_gbl_do_not_use_xsdt) {
/*
* RSDP contains an XSDT (64-bit physical addresses). We must use
* the XSDT if the revision is > 1 and the XSDT pointer is present,
* as per the ACPI specification.
*/
address = (acpi_physical_address)rsdp->xsdt_physical_address;
table_entry_size = ACPI_XSDT_ENTRY_SIZE;
} else {
/* Root table is an RSDT (32-bit physical addresses) */
address = (acpi_physical_address)rsdp->rsdt_physical_address;
table_entry_size = ACPI_RSDT_ENTRY_SIZE;
}in let revision = rsdp_mapping.revision();
let root_table_mapping = if revision == 0 {
/*
* We're running on ACPI Version 1.0. We should use the 32-bit RSDT address.
*/
read_root_table!(RSDT, rsdt_address)
} else {
/*
* We're running on ACPI Version 2.0+. We should use the 64-bit XSDT address, truncated
* to 32 bits on x86.
*/
read_root_table!(XSDT, xsdt_address)
};
Ok(Self { mapping: root_table_mapping, revision, handler })
}so , when revision = 1, linux will regard it as RSDT, but acpi-rs see it as a XSDT. the acpi2.0 checksum len is a constant /* RSDP checksums */
#define ACPI_RSDP_CHECKSUM_LENGTH 20
#define ACPI_RSDP_XCHECKSUM_LENGTH 36acpi_status acpi_tb_validate_rsdp(struct acpi_table_rsdp *rsdp)
{
/*
* The signature and checksum must both be correct
*
* Note: Sometimes there exists more than one RSDP in memory; the valid
* RSDP has a valid checksum, all others have an invalid checksum.
*/
if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
/* Nope, BAD Signature */
return (AE_BAD_SIGNATURE);
}
/* Check the standard checksum */
if (acpi_ut_checksum((u8 *)rsdp, ACPI_RSDP_CHECKSUM_LENGTH) != 0) {
return (AE_BAD_CHECKSUM);
}
/* Check extended checksum if table version >= 2 */
if ((rsdp->revision >= 2) &&
(acpi_ut_checksum((u8 *)rsdp, ACPI_RSDP_XCHECKSUM_LENGTH) != 0)) {
return (AE_BAD_CHECKSUM);
}
return (AE_OK);
}but in acpi-rs in let length = if self.revision > 0 {
// For Version 2.0+, include the number of bytes specified by `length`
self.length as usize
} else {
RSDP_V1_LENGTH
};
let bytes = unsafe { slice::from_raw_parts(self as *const Rsdp as *const u8, length) };
let sum = bytes.iter().fold(0u8, |sum, &byte| sum.wrapping_add(byte));so when the length of and if firmware write the |
685081b to
5b31659
Compare
fslongjin
left a comment
There was a problem hiding this comment.
Two blocking issues found during the adversarial review:
5b31659 to
8daaa3d
Compare
Signed-off-by: Donjuanplatinum <donplat@barrensea.org>
8daaa3d to
f93ecce
Compare
impl ACPI RSDP discovery for legacy BIOS platforms by scanning the standard locations defined by the ACPI specification.
The impl follow the linux kernel drivers/acpi/acpica/tbxfroot.c
checking:
#1273
#1487
根据Linux内核的源码以及ACPI规范 实现对RSDP的fallback搜索 以当rsdp_addr = 0时 能正常启动DragonOS.