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]

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]

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]

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]

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]

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)

[IsaacWoods]

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. Clean up allocator_api feature #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. acpi: fix length of "extended_area_bytes" in RSDP search #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 Kernel Example with ACPI #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. rsdp: Make whole RSDP readable #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 :)