Skip to content

feat(acpi): add RSDP discovery from BIOS memory regions.#2124

Open
donjuanplatinum wants to merge 5 commits into
DragonOS-Community:masterfrom
donjuanplatinum:acpi-fix
Open

feat(acpi): add RSDP discovery from BIOS memory regions.#2124
donjuanplatinum wants to merge 5 commits into
DragonOS-Community:masterfrom
donjuanplatinum:acpi-fix

Conversation

@donjuanplatinum

@donjuanplatinum donjuanplatinum commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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

#1273
#1487

根据Linux内核的源码以及ACPI规范 实现对RSDP的fallback搜索 以当rsdp_addr = 0时 能正常启动DragonOS.

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>
@github-actions github-actions Bot added the enhancement New feature or request label Jul 15, 2026
@fslongjin

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread kernel/src/arch/x86_64/init/pvh/mod.rs Outdated
Comment thread kernel/src/driver/acpi/rsdp.rs Outdated
   impl.
2. when len < 36 || len > available_len, rsdp_valid return false
@fslongjin

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread kernel/src/driver/acpi/rsdp.rs Outdated
Signed-off-by: Donjuanplatinum <donplat@barrensea.org>

@fslongjin fslongjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The RSDP pre-validator does not have the same revision/length contract as Linux or the downstream acpi-rs parser. This can accept a candidate that later causes an out-of-bounds validation read or selects XSDT under the wrong revision semantics.
  2. 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 the EarlyIoRemap/pseudo-mapper contract and the current call site. Please correct it.
  • The unrelated unmap_parents: true -> false change in no_init.rs changes 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 == 0 should 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.

Comment thread kernel/src/driver/acpi/rsdp.rs Outdated
Comment thread kernel/src/driver/acpi/rsdp.rs Outdated
@donjuanplatinum

donjuanplatinum commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

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:

  1. The RSDP pre-validator does not have the same revision/length contract as Linux or the downstream acpi-rs parser. This can accept a candidate that later causes an out-of-bounds validation read or selects XSDT under the wrong revision semantics.
  2. 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 the EarlyIoRemap/pseudo-mapper contract and the current call site. Please correct it.
  • The unrelated unmap_parents: true -> false change in no_init.rs changes 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 == 0 should 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 acpi-rs repo.

  1. In Linux kernel source in drivers/acpi/acpica/tbutils.c line 242-259 shows that: when revision > 1, use XSDT. and revision = 0 or 1 use RSDT.
	/* 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 acpi-rs in acpi/src/lib.rs line 233-248 shows that: only revision ==0 use RSDT, other use XSDT.

 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.
2. the checksum len in acpi 2.0 condition.
in linux source drivers/acpi/acpica/tbxfroot.c line 59-89.

the acpi2.0 checksum len is a constant ACPI_RSDP_XCHECKSUM_LENGTH valued 36 and its defined in include/acpi/acconfig.h in line 173:

/* RSDP checksums */
#define ACPI_RSDP_CHECKSUM_LENGTH       20
#define ACPI_RSDP_XCHECKSUM_LENGTH      36
acpi_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 acpi/src/rsdp.rs line 119-127 it use self.length

   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 RSDP in memory is not 36 and use ACPI2.0, linux will also see it as 36,but acpi-rs not.

and if firmware write the lenght > 36, acpi-rs will read over the range and go into UB.

@fslongjin fslongjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two blocking issues found during the adversarial review:

Comment thread kernel/src/driver/acpi/rsdp.rs
Comment thread kernel/src/mm/no_init.rs Outdated
Signed-off-by: Donjuanplatinum <donplat@barrensea.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants