Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prepare for stabilisation of the acpi crate #70

Merged
merged 7 commits into from
Apr 24, 2020
Merged

Prepare for stabilisation of the acpi crate #70

merged 7 commits into from
Apr 24, 2020

Conversation

IsaacWoods
Copy link
Member

The acpi crate is nearing the point where I'd like to get it stabilised and publish a v1.0.0 version. This PR will make a few breaking changes for either correctness or future-proofing for the stuff that isn't yet implemented (e.g. the x2APIC) - I'd be interested in getting people's feedback on both the idea of stabilisation, and whether they think any other changes should be made to the crate before stabilisation.

Note that we're not stabilising the aml crate at this time.

cc @rust-osdev/acpi @64 @phil-opp

@IsaacWoods
Make AcpiHandler::map_physical_region unsafe
159ed14 
We turn the virtual addresses returned from this function into references
without checking their validity, and so this is unsafe.
@IsaacWoods
Make InterruptModel non_exhaustive
327d7d7 
This makes it not-a-breaking-change to add new interrupt models in the
future, and isn't a big onus on users as they'll likely want a wildcard
pattern anyway, as not all interrupt models will be supported on their
architecture anyway.
@IsaacWoods
Future proof processor UIDs in MADT parsing
a2af8a6 
The later x2APIC extended the processor UID to a 4-byte field. We want to
use the same representation for that, so simply use `u32` and cast for the
old single-byte entries.
@64
Copy link
Contributor

64 commented Apr 17, 2020

Pretty minor point, but I wonder if it's worth using the address types from x86_64? There are some places where usize is used for physical addresses (e.g parse_rsdp, AcpiHandler), other places where u64 is used (e.g PciConfigRegions). The downside being that compile times take a small hit if you're not already using that crate.

@IsaacWoods
Copy link
Member Author

I wonder if it's worth using the address types from x86_64?

While most people seem to be using this crate exclusively on x86_64, there are ARM64 and (I believe) RISC-V systems that either use ACPI or have expressed that they will in the future. It seems like a bad idea to tie acpi into any one architecture. I do get the attraction of the stronger address types though.

There are some places where usize is used for physical addresses (e.g parse_rsdp, AcpiHandler), other places where u64 is used (e.g PciConfigRegions).

I totally concede that the address handling in acpi is a bit messy, but that's mostly because it's also pretty messy in ACPI. The first versions were designed for 32-bit systems, and 64-bit counterparts were added as x86_64 systems were introduced (or maybe PAE, which I'm not familiar with). The MCFG is the exception, as it was added later and so skipped straight to using 64-bit addresses (this information is second-hand, however, because I've never seen the PCIe spec that I assume includes its layout [PCI-SIG wants $4500 for it?!]).

So I don't think we do anything wrong here - we use usize for addresses that might either be 32-bit or 64-bit depending on the platform address width, and u64 when we're always supplied a 64-bit address. I'd be interested if you think we should do something differently here :)

@64
Copy link
Contributor

64 commented Apr 18, 2020

That sounds totally reasonable. I had a quick google for ACPI on other architectures and didn't find anything, but if you're right that it is used outside of x86_64 then that all makes sense.

@IsaacWoods IsaacWoods marked this pull request as ready for review April 24, 2020 20:50
@IsaacWoods
[Closes rust-osdev#25] Make parse_rsdp and parse_rsdt unsafe
8dca2c9 
These take potentially-incorrect physical addresses and dereference them,
so should definitely be unsafe. This creates warnings about nested unsafe
blocks but there are imminent plans to make that accepted so I'm leaving
it to reduce churn.
@IsaacWoods
Copy link
Member Author

Okay, nobody has raised concerns about this so I'm going to go ahead and merge this :)

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 24, 2020

Build succeeded:

@bors bors bot merged commit 5a016ad into rust-osdev:master Apr 24, 2020
@IsaacWoods IsaacWoods deleted the stable branch April 24, 2020 21:43
@IsaacWoods
Copy link
Member Author

Published as acpi v1.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IsaacWoods @64