Lay out new architecture for acpi, allowing separation of table discovery and parsing #75

IsaacWoods · 2020-09-17T07:39:41Z

This lays out a first draft for a new architecture for the acpi crate, which may be published as v2.0.0. It is the implementation I've reached after discussions in #74 about how the acpi crate could be more accomodating to OSs that want to separate the discovery and parsing of the ACPI tables between various stages, notably between kernelspace and userspace.

cc @wt are you still interested in these changes? What do you think about this potential design, and do you think it would be compatible with Redox's needs?
cc @rust-osdev/acpi for anyone who is interested in as large a change as this one

I'm afraid the diffing algorithm has not treated this kindly, so the diffs aren't the easiest to read. Feel free to ask questions if anything is unclear / I've missed anything :)

phil-opp · 2020-09-17T09:44:55Z

Separating the parsing of the ACPI tables sounds like a good idea to me.

A couple of notes about the implementation:

The crate documentation still mentions the old parse_rsdp etc. functions. We should update it to the new associated functions.
The AcpiTables::get_sdt method should probably be private, given that it imposes no restrictions on the generic type T.
It would be useful to have a convenience AcpiTables::platform_info method that just calls PlatformInfo::new. This would make it more discoverable what you can do with the AcpiTables instance.
One problem I see with the coerce_type method is that if a panic occurs between the mem::replace and the mem::forget, a double panic would occur in the panic handler. Maybe skipping the unmap_physical_region in the destructor if handler is None would be a better idea? I think we would not even need the mem::forget then.

IsaacWoods · 2020-09-18T23:32:56Z

Thanks very much for the review @phil-opp - great points :)

I've rewritten the documentation to be up to date and hopefully be more helpful than it was before the rewrite. Providing the AcpiTables::platform_info convenience method is a great tip - I was worried about how discoverable the types for actually doing stuff with the tables were.

AcpiTables::get_sdt is actually public deliberately - I think it'll be useful for users to be able to access tables directly if they want to, both if they want to do something different with the raw table, or if they're looking for non-standard tables (probably vendor-specific, but UEFI even provides a protocol to install your own tables [I'm not sure why this would ever be useful, fwiw]). I've hopefully improved safety by adding some notes on paying attention to the choice of T, and restricting it to a type that implements a new trait AcpiTable. This also allows us to access the header through a method on this new trait, which as a nice side-effect gets rid of the need for coerce_type.

I still need to migrate the RSDP search over to the new types, but after that this should be ready for a release candidate to be published. I think letting it bake for a few weeks after that is probably wise to allow any issues to crop up before publishing v2.0.0.

phil-opp · 2020-09-19T11:48:14Z

Sounds great!

Regarding the RSDP search: Have you thought about moving the detection of the physical RSDP address to a separate crate that does not depend on alloc? We need that functionality in the new version of the bootloader crate since we want to pass the RSDP address as part of the boot info. Right now we have an own very basic implementation of the search, but it would be much nicer if we could reuse your implementation.

IsaacWoods · 2020-09-19T15:04:46Z

That's an interesting idea - it would definitely be cool to see acpi used in the bootloader.

I've split the Rsdp and AcpiHandler types into a new crate rsdp_search, which also includes a method to search for it on BIOS systems. All these types are then re-exported by acpi so the API of that doesn't change. Users can then use AcpiTables::search_for_rsdp_bios to search for the RSDP and go all the way to an AcpiTables, but from bootloader you could directly use Rsdp::search_for_on_bios to either get the address of the RSDP or RSDT, whichever you'd like to supply.

If this works for you, I'll get rsdp_search published :)

phil-opp · 2020-09-20T19:22:40Z

Awesome, thanks a lot! I just moved the bootloader prototype to the rsdp_search crate and it works without problems.

IsaacWoods · 2020-09-24T22:42:24Z

Great!

I've published rsdp v1.0.0 (I've renamed rsdp_search to rsdp, as it was free and fitted better, I thought), and acpi v2.0.0-pre1 for ease of testing. I've updated Pebble to the new version and all seems to work, so if nothing shows up for a week or so I'll go ahead and stabilise it.

wt · 2020-09-25T22:49:22Z

FWIW, I am still interested. I have just been very busy with life. Thanks for jumping into this. I haven't really looked, but I think it's great to see action here. :)

This is something we've wanted for a while, as it removes the human error of forgetting to unmap a physical mapping. Having to store the `H` does make some things a bit janky, but we seem to have been able to work around it so far.

This is a pretty rough implementation, but it's strangely difficult to get it to move the handler from the old mapping into the new, since it apparently can't work out that the `H` is going to be the same size in both types. This works.

This is at the heart of the new architecture - we now collect all the ACPI tables without dispatching them ourselves. This makes the library more flexible, at the cost of an extra step if you don't care about which tables you have, but it's turned out much cleaner than I'd hoped overall. Unfortunately the diff algorithm has butchered this, so this is kinda messy.

This is a new (to QEMU) table that isn't detailed in the current version of the ACPI spec. Not sure when Microsoft dreampt it up, but it looks like it was added to QEMU earlier this year.

One of the problems with the new architecture is that to get a nice view of the topology and interrupt model, we need to get a bunch of "unrelated" info out of a couple of the tables. We encapsulate this in `PlatformInfo`, which amalgamates the FADT and MADT into some common info.

* Convenience method `AcpiTables::platform_info` added to aid discoverability * Deny unsafe ops in unsafe fn * Fix errors created by that lint * Make method in MADT private, as it's called as an implementation detail * Remove unneeded Rsdp::oem_id method. We should probably work out some way of exposing OEM details, but so few people probably care about it, it seems a waste to heap-alloc a string for it just in case.

This is just a convenience method that makes it easier to construct an AcpiTables by searching for it on BIOS platforms. It outsources to the `rsdp` crate.

* Note the `rsdp` crate in the documentation of `acpi` * Publish version 2.0.0!

IsaacWoods · 2020-09-29T17:36:55Z

@wt No worries, it would be great to see this integrated into Redox if/when you find the time. Let us know how it goes :)

Published as acpi v2.0.0 🎉

IsaacWoods force-pushed the new branch from 974ced6 to d659c2b Compare September 19, 2020 13:50

michaelmelanson mentioned this pull request Sep 21, 2020

Add more resource types for parsing _CRS packages #76

Merged

IsaacWoods added 18 commits September 27, 2020 22:46

Move FADT methods onto type

f34613f

Add method to change type of PhysicalMapping

0f5c935

This is a pretty rough implementation, but it's strangely difficult to get it to move the handler from the old mapping into the new, since it apparently can't work out that the `H` is going to be the same size in both types. This works.

Note that we've seen WAET tables in the SDT signature list

39ca004

This is a new (to QEMU) table that isn't detailed in the current version of the ACPI spec. Not sure when Microsoft dreampt it up, but it looks like it was added to QEMU earlier this year.

Provide new way to get HPET info

7703f96

Move PCI config region parsing over to new system

f11e229

Store handler in AcpiTables to make API nicer

b272d42

Add AcpiTable and simplify get_sdt, removing need for coerce_type

1607d72

Rewrite main crate documentation

f2ad68a

Divide RSDP searching into rsdp_search crate

93f9e3b

Rename rsdp_search to rsdp, add fields for publishing

3f98873

Add search_for_rsdp_bios constructor on AcpiTables

7138383

This is just a convenience method that makes it easier to construct an AcpiTables by searching for it on BIOS platforms. It outsources to the `rsdp` crate.

Publish rsdp v1.0.0

85152b3

Update README to talk about new rsdp crate

acb680b

Publish acpi v2.0.0-pre1 for testing

7fbced4

IsaacWoods force-pushed the new branch from cf3cfac to 7fbced4 Compare September 27, 2020 21:47

Acpi v2.0.0

5257344

* Note the `rsdp` crate in the documentation of `acpi` * Publish version 2.0.0!

IsaacWoods merged commit a885b21 into rust-osdev:master Sep 29, 2020

IsaacWoods deleted the new branch September 29, 2020 17:38

IsaacWoods mentioned this pull request Sep 29, 2020

working together better with redox #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lay out new architecture for acpi, allowing separation of table discovery and parsing #75

Lay out new architecture for acpi, allowing separation of table discovery and parsing #75

IsaacWoods commented Sep 17, 2020

phil-opp commented Sep 17, 2020

IsaacWoods commented Sep 18, 2020

phil-opp commented Sep 19, 2020

IsaacWoods commented Sep 19, 2020

phil-opp commented Sep 20, 2020

IsaacWoods commented Sep 24, 2020

wt commented Sep 25, 2020

IsaacWoods commented Sep 29, 2020

Lay out new architecture for acpi, allowing separation of table discovery and parsing #75

Lay out new architecture for acpi, allowing separation of table discovery and parsing #75

Conversation

IsaacWoods commented Sep 17, 2020

phil-opp commented Sep 17, 2020

IsaacWoods commented Sep 18, 2020

phil-opp commented Sep 19, 2020

IsaacWoods commented Sep 19, 2020

phil-opp commented Sep 20, 2020

IsaacWoods commented Sep 24, 2020

wt commented Sep 25, 2020

IsaacWoods commented Sep 29, 2020