Rewrite acpi crate and entire AML interpreter
#246
Conversation
I don't really understand why this had a lifetime instead of constructing the slice for the lifetime of `self`, but this cleans stuff up a fair bit both in the library and for users.
…e`, `DefMethod`, `DefExternal`
Still need to do a bunch of bit fiddling stuff to facilitate actually reading and storing to them.
- Correct a number of bugs in how we handle mapped regions of SDTs - Simplify API on `AcpiTables` - Move all `alloc`-usage into `platform` - Move all SDT types under new `sdt` module - Move from `ManagedSlice` back to just using `Vec` - Improve documentation throughout the library - Remove `AcpiResult` type in preference of normal `Result`
This also makes the logic clearer about whether we're meant to resolve a top-level name or not, depending on what we're interpreting.
|
We have 3 different situations for ACPI:
So, this leads to some questions:
|
|
1 should be possible with a simple mapper that just returns an identity-mapped mapping I guess. 2 would also be plausible as long as you have a way to associate a given physical address (e.g. the start of a given table) with the correct virtual mapping.
There are some structures that you need up-to-date read-write mappings to the actual underlying backing memory. An example is the FACS for controlling the global lock and various other things. I guess we could include a flag when making a mapping saying whether we just need read-only data that could be copied and then the underlying memory reused, or whether we need access to the actual physical memory. Could I ask for the rationale for copying the tables? It's generally not a large amount of data; I was planning to just keep the real memory around for the life of the system in my kernel. AML can make arbitrary accesses to physical memory, port IO, and PCI config+BAR space, so your ACPI driver will need those capabilities either way. Another possibility I've been considering is to allow tables to be overriden from the handler to allow e.g. a particularly broken table to be patched by OSPM. If you managed to mock an
I think the tables are normal system memory, so I guess there is nothing stopping it being written. A
I think if you have an unusual way of accessing tables (e.g. from userspace +/- over IPC), your best bet may be to roll your own |
|
Looks great, and does solve #245, thanks! |
This will allow us to add variants without them being breaking changes, and I don't think you should be exhaustively matching our errors regardless.
…g`, and `ToInteger`
|
I have a real-world AML file that is producing an opcode error. I am AFK, but do you want the whole file or just the opcode that is producing the error? |
|
Whole file please. |
|
Thanks for the reports. Interpreting the files via the test harness, both do produce errors, although not the same one. The HP laptop produces The Dell table produces Reassuringly, the interpreter in its current state seems to munch through quite a lot of those tables, which do include some weird stuff. I'll get these fixed when I can. Please do report any new problems you come across. |
|
I found similar cases as well, but thought it was something on my side. I found |
This (entirely too large) PR represents work I've been doing over the last few months to address a large number of bugs in both the
acpiandamlcrates, and generally improve the quality of the library. While this work is nowhere near complete, it represents the library getting back to a point where I think it is reasonable for projects to use it (in that it is more correct than the current interpreter in all circumstances), and is already too unwieldy to rebase against other PRs that are waiting for review/merging.For contributors awaiting PRs being merged, I apologise for this derailing your contributions, and I am happy to assist in rebasing your changes/creating new patches with you credited as authors. I will review current PRs when I have time.
Summary of changes
Firstly, the
acpicrate has undergone major reworking to simplify logic surrounding table mapping and RSDT enumeration. This should fix #245.Previous work on allowing fallible allocations has been removed, as it added significant complexity for little real-world gain at this time - if the ecosystem around fallible allocation in Rust improves in the future, I'm not against re-adding these capabilities. The split between the portion of the library that can be used without an allocator vs requiring one has been made clearer, with allocating methods being moved under the higher-level
platformmodule.This PR also deprecates the
amlcrate, bringing the interpreter into theacpicrate under a feature-flag. This simplifies the most common use-cases of the library, and was effectively required for ergonomic handling of some AML operations that require us to handle static ACPI tables. Future work with the ACPI event system will likely require closer coupling between the previously-separate parts of the library, as well.I am imagining that these changes will be published as
acpi v6.0.0, with theamlcrate being marked as deprecated and receiving no future updates.Remaining tasks
AcpiHandlervsacpi::aml::Handler- do we want to merge them, makeAcpiHandlera supertrait (perhaps being renamedRegionMapperor similar as this is the only functionality it needs to provide)?GenericAddressto have arbitrary field widths - apparently required by the PCC (see acpi: add support for PCC table #233)DefMatchDefVarPackageDefLoadDefLoadTableDefDataRegionDefToBufferDefToIntegerDefToStringDefToDecimalStringDefToHexStringDefCopyObjectDefNotifyFeedback
I would welcome feedback on the new APIs, crate layout, or changes I should consider before publishing a new major version of
acpi. I particularly welcome feedback + bug-reports from people who have integratedacpiinto a kernel or other project.cc @rw-vanc - I would be interested in any concerns re Redox moving to the new version of the crate, and any further work you'd be interested in.
cc @usadson - this should fix #245. Thank you for your report.