rust-osdev/acpi

AcpiTables<H>::from_rsdp() panics if rsdp address is not 64-bit aligned with nightly compiler "1.70.0-nightly (23ee2af2f 2023-04-07)"

foxcob opened this issue · 5 comments

foxcob commented

qemu-system-x86_64 provides a 32-bit aligned (not 64-bit aligned) address. After updating to the latest nightly (1.70.0 2023-04-07), lib.rs: 191 panics if the address is not aligned.

I don't believe 64-bit alignment is an ACPI requirement, that means explicit handling of address alignment is necessary.

rcerc commented

On legacy BIOS systems, the RSDP is 16-byte aligned, but you are right that ACPI does not appear to make other such guarantees. However, I think the library accounts for this due to the Rsdp struct's packed modifier, which defaults to an alignment of 1. This prohibits referencing fields whose types have alignments greater than 1 (e.g., u16) and forces the compiler to read these fields byte by byte if necessary, avoiding unaligned accesses.

What line is panicking? I checked line 191 in acpi/lib.rs and rsdp/lib.rs, but one is a blank line and the other is a function definition. Could this be another file named lib.rs?

julienfreche commented

I am not certain about the code you have in main compared to the crate I used (4.1.1). But, this line was panicking for me:

https://docs.rs/acpi/4.1.1/src/acpi/lib.rs.html#191

and, I changed u64 to u32 on this line:

https://docs.rs/acpi/4.1.1/src/acpi/lib.rs.html#188

And that fixed things in my case (qemu). I was thinking about submitting a pull request but I just started playing around with Rust so I am not confident the fix is the right thing to do. And since the code in main is different, maybe that's fixed already?

rcerc commented

Ah, I was looking at main. Thank you for catching that. I think that bug was fixed in #131 by using read_unaligned on the pointer. It would be problematic to change the *const u64 to a *const u32 because the XSDT contains 64-bit pointers; doing so would treat the lower half and upper half of each pointer as separate pointers to different tables. As a temporary workaround, read_unaligned could be used:

result.process_sdt(unsafe { tables_base.add(i).read_unaligned() } as usize)?;

Nevertheless, it'd be great if the accumulated bugfixes could all be released (although it would unfortunately be backwards incompatible because of API changes). @IsaacWoods, will this happen soon, or are there other things that must be done first?

julienfreche commented

FWIW, I switched to using main and the problem was indeed solved. (But, yes, I did have to change my code to fit the new API)

  • 👍2
IsaacWoods commented

Nevertheless, it'd be great if the accumulated bugfixes could all be released (although it would unfortunately be backwards incompatible because of API changes). @IsaacWoods, will this happen soon, or are there other things that must be done first?

Yeah, I get that this is less than ideal. The plan was to have a new major version out a few months ago, but I've been very busy outside of open-source stuff and a few big bugs have slipped through the net from trying to get reviews done quickly, so I've been looking for some time to sit down and go through things properly before releasing.

So, as a sort of roadmap:

  1. #177 needs reviewing and merging, as it fixes up the new feature we're shipping
  2. new methods need adding back in (under another feature) that use the default allocator. This will hopefully limit breakage for most users.
  3. The RSDP search code needs a proper review before release - it's had a few changes recently and I'm not entirely convinced its behaviour is correct. #164 is part of this but not the entire fix I don't think.
  4. Part of the problem with that is that my OS is UEFI-only, so I can't reliably test that functionality. I'd like to build a small in-repo kernel that can a) be used as an example kernel (cc #125) for new users and b) can be used to test that the crate works in QEMU on both BIOS and UEFI. I think it'd be best to use the rust-osdev/bootloader project for this to showcase the in-house bootloader project.
  5. #179 needs reviewing and merging

If you'd like to help get things released and have some time, PRs for numbers 2 and 4 are very welcome :)