Skip to content

Make AML interpreter storage allocator-api compatible#306

Open
SnowCheetos wants to merge 8 commits into
rust-osdev:mainfrom
SnowCheetos:main
Open

Make AML interpreter storage allocator-api compatible#306
SnowCheetos wants to merge 8 commits into
rust-osdev:mainfrom
SnowCheetos:main

Conversation

@SnowCheetos

@SnowCheetos SnowCheetos commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

This ports the AML interpreter and related AML data structures to support allocator-aware storage.

The main change is threading an allocator through AML-owned objects and namespace structures so callers can use allocator-backed Vec, Arc, BTreeMap, and string storage instead of relying on global allocation in the interpreter core. It also applies a patch for #305

Changes

  • Make AML object, namespace, operation-region, and interpreter state generic over an allocator.
  • Add allocator-aware AML string storage via AmlString<A>.
  • Add *_in constructors where AML structures need an explicit allocator.
  • Update AML test tooling and handlers to work with allocator-aware interpreter/object types.
  • Restore the full workspace membership so aml_tester, aml_test_tools, and uacpi_test_adapter build together.
  • Clean up temporary migration comments and remove an unused method-context helper.
  • Preserve string conversion behavior for implicit casts, including NUL truncation and lossy UTF-8 handling.
  • acquire_global_lock now reacquires global_lock_mutex before retrying after firmware contention.
  • release_global_lock now releases global_lock_mutex, balancing the acquire path.
  • Added a no-FACS fast path, avoiding unnecessary handler mutex work when there is no firmware global lock.
  • Added tests/global_lock.rs to assert acquire/release balances handler mutex calls.

Notes

This PR is mostly mechanical allocator plumbing, but it intentionally keeps the public AML behavior unchanged where possible. Follow-up work can further reduce remaining Global use at API boundaries that are still tied to Handler.

Version auto-bumped by autocommit, can revert if needed.

@SnowCheetos
fix(platform): fix: Add allocator to registers
ea1e25d 
Fix: Add allocator to registers across 2 files.

### Changes
- [`src/lib.rs`] Support `aml` feature: Enable the `aml` feature in the library
- [`src/platform/mod.rs`] Add allocator to registers: Use the provided `allocator` for `registers` to avoid potential memory issues

### Version Bumps
- [`Cargo.toml`] Rust / Cargo: 6.1.1 -> 6.1.2 (patch); code changes detected; suggest a patch project version bump

### Risk
- Level: low
@SnowCheetos
style(platform): use &AcpiTables
841c15e 
### Changes
- [`src/platform/numa.rs`] Use &AcpiTables: Allow passing `AcpiTables` as a reference to the `new` method

### Risk
- Level: low
@SnowCheetos
feat: use AmlString for string handling and Parse _SI in namespace
5353b19 
This commit introduces `AmlString` for string handling and parses `_SI` within the `namespace` to improve string manipulation capabilities.

### Changes
- [`src/aml/mod.rs`] Use AmlString for string handling: Introduce `AmlString` for safer string handling and improve error reporting
- [`src/aml/namespace.rs`] Parse `_SI` in namespace: Allow parsing of `_SI` in namespaces using `AmlName::parse_in` for improved flexibility
- [`src/aml/mod.rs`] Implement push method in AmlString: Add `push` method to `AmlString` to support allocator-parameterized string manipulation
- [`src/aml/mod.rs`] Use Allocator for generic resource handling: Update `PciRouteType` to use `Allocator` for generic resource handling
- [`src/lib.rs`] Enable allocator features: Enable `btreemap_alloc` and `allocator_api` for better memory management
- [`tests/bank_fields.rs`] Update test infrastructure and files
- [`tools/aml_test_tools/src/handlers/check_cmd_handler.rs`] Use Global allocator for interpreter: Use the global allocator for the interpreter to improve performance and avoid allocation issues

### Version Bumps
- [`Cargo.toml`] Rust / Cargo: 6.1.2 -> 6.2.0 (minor); manifest changed without an explicit project version bump; suggest minor bump
- [`tools/aml_test_tools/Cargo.toml`] Rust / Cargo: 0.1.0 -> 0.2.0 (minor); code changes detected; suggest a minor project version bump

### Risk
- Level: low
- No security risks are introduced by using `AmlString` and parsing `_SI`.
@SnowCheetos
feat: use MethodContext for method arguments
8023fbf 
This commit introduces `MethodContext` to provide method arguments, improving code clarity and maintainability.

### Changes
- [`src/aml/mod.rs`] Use MethodContext for method arguments: Use `MethodContext` to correctly pass method arguments to the interpreter
- [`src/platform/interrupt.rs`] Add `hw_id` to Gic struct
- [`tests/bank_fields.rs`] Update imports: Refactor imports to align with updated dependencies and improve code clarity
- [`tools/aml_test_tools/src/handlers/check_cmd_handler.rs`] Prevent null check when handler is null: Ensure the `check_cmd_handler` function doesn't panic when passed a null handler

### Risk
- Level: low
- No new security risks introduced.
- No data loss or corruption is expected.
@SnowCheetos
chore(aml): update AmlString formatting and add IRQ information to Re…
9d4858a 
…sourceDescriptor

Update formatting of `AmlString` and add IRQ information to `ResourceDescriptor` in `aml` module

### Changes
- [`src/aml/mod.rs`] Update AmlString formatting: Change `AmlString` to use `A: Allocator + Clone` for formatting, improving flexibility and avoiding dynamic context info loss
- [`src/aml/resource.rs`] Add IRQ information to ResourceDescriptor
- [`src/lib.rs`] Remove unused comment: Remove a comment that discusses unused code and potential future changes

### Version Bumps
- [`Cargo.toml`] Rust / Cargo: 6.2.0 -> 6.2.1 (patch); manifest changed without an explicit project version bump; suggest patch bump

### Risk
- Level: low
- No security implications.
- Formatting changes are for code consistency.
- IRQ information is for debugging and monitoring purposes.
@SnowCheetos
Merge remote-tracking branch 'upstream/main'
78b539f 
# Conflicts:
#	src/aml/mod.rs
#	src/aml/object.rs
#	src/aml/resource.rs
#	tools/aml_test_tools/src/lib.rs
@SnowCheetos SnowCheetos marked this pull request as ready for review June 8, 2026 22:07
@martin-hughes

martin-hughes commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Nice one @SnowCheetos - I hope you and @IsaacWoods won't mind me jumping in with a few thoughts?

Lots of words here - I don't mean to detract from your work. It is definitely worthy of more discussion!

Big thoughts:

  • I feel quite strongly that the Global allocator should be the default - much as it would be for Vec and other standard library types. This would reduce the API disruption but still allow users to use the Allocator API version. For example, if I were writing a user-space ACPI & AML handler (rather than having it in-kernel) I probably wouldn't need a custom allocator.
    • So Interpreter::new would still have a place
  • I'm not sure - I could be persuaded otherwise - but I think having to constrain the Allocator to Clone demonstrates that Interpreter isn't quite ready for the Allocator API. I see that the standard library types do not require Clone, although I acknowledge that they are much simpler.
    The risk of requiring clone is that it requires an allocator to implement it correctly - that is, any Cloned instance of an allocator should be able to deallocate from any of the other clones. Otherwise it's unsafe. Simply requiring Clone does not express that safety requirement.
    • Is there a technical reason not to use references to the allocator? The allocator always has to live longer than any objects it creates. Obviously it leads to a whole bunch of lifetime specifiers...
  • The PR is quite big with a few different features - personally I'd prefer to deal with them one at a time, probably allocator API first, but @IsaacWoods might feel he has a larger context window than me 😊
  • AmlError requiring an Allocator feels like a pretty big smell to me - the impl PartialEq block being a good example of why. I think @IsaacWoods was planning a redesign around AmlError but it feels like it might be appropriate now! I haven't looked but I'm sure there'll be some kind of error context crate that would help.

Smaller thoughts:

  • "It also applies a patch for AML global lock acquisition of global_lock_mutex looks wrong #305" - Thanks 😄, I've been too lazy this week! Bonus points because it's tested.
  • "Restore the full workspace membership so aml_tester, aml_test_tools, and uacpi_test_adapter build together." From looking at the Cargo.toml files I wasn't clear what actually changed - or quite what you meant by this?
  • IMO AmlString should be in its own file - each file already deals with too many responsibilities. (@IsaacWoods doesn't necessarily agree with me!)
    • Alternatively, why not use the string_alloc crate to offload some responsibility to them 😉
  • I think the bounds on Allocator of Clone + 'static are not enough to make it Send + Sync - are they?
  • The API changes are not backwards compatible as it stands (new is removed!), so the version should go up to 7.0.0 (https://semver.org/ spec item 8)

Obviously this is Isaac's crate so hopefully none of that is out of line!

@SnowCheetos
chore: use Global allocator for Interpreter
4114bf4 
### Changes
- [`.gitignore`] Add `.DS_Store` and .rs.bk to: Exclude unnecessary files from Git tracking
- [`src/aml/mod.rs`] Use Global allocator for Interpreter: Change `Interpreter`'s default allocator to `Global` for better portability and performance

### Risk
- Level: low
- No immediate risk identified.
- Global allocator usage is generally safe.
- Potential for increased memory usage if not managed carefully.
@SnowCheetos

SnowCheetos commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Nice one @SnowCheetos - I hope you and @IsaacWoods won't mind me jumping in with a few thoughts?

Lots of words here - I don't mean to detract from your work. It is definitely worthy of more discussion!

Big thoughts:

  • I feel quite strongly that the Global allocator should be the default - much as it would be for Vec and other standard library types. This would reduce the API disruption but still allow users to use the Allocator API version. For example, if I were writing a user-space ACPI & AML handler (rather than having it in-kernel) I probably wouldn't need a custom allocator.

    • So Interpreter::new would still have a place

  • I'm not sure - I could be persuaded otherwise - but I think having to constrain the Allocator to Clone demonstrates that Interpreter isn't quite ready for the Allocator API. I see that the standard library types do not require Clone, although I acknowledge that they are much simpler.
    The risk of requiring clone is that it requires an allocator to implement it correctly - that is, any Cloned instance of an allocator should be able to deallocate from any of the other clones. Otherwise it's unsafe. Simply requiring Clone does not express that safety requirement.

    • Is there a technical reason not to use references to the allocator? The allocator always has to live longer than any objects it creates. Obviously it leads to a whole bunch of lifetime specifiers...

  • The PR is quite big with a few different features - personally I'd prefer to deal with them one at a time, probably allocator API first, but @IsaacWoods might feel he has a larger context window than me 😊

  • AmlError requiring an Allocator feels like a pretty big smell to me - the impl PartialEq block being a good example of why. I think @IsaacWoods was planning a redesign around AmlError but it feels like it might be appropriate now! I haven't looked but I'm sure there'll be some kind of error context crate that would help.

Smaller thoughts:

  • "It also applies a patch for AML global lock acquisition of global_lock_mutex looks wrong #305" - Thanks 😄, I've been too lazy this week! Bonus points because it's tested.

  • "Restore the full workspace membership so aml_tester, aml_test_tools, and uacpi_test_adapter build together." From looking at the Cargo.toml files I wasn't clear what actually changed - or quite what you meant by this?

  • IMO AmlString should be in its own file - each file already deals with too many responsibilities. (@IsaacWoods doesn't necessarily agree with me!)

    • Alternatively, why not use the string_alloc crate to offload some responsibility to them 😉

  • I think the bounds on Allocator of Clone + 'static are not enough to make it Send + Sync - are they?

  • The API changes are not backwards compatible as it stands (new is removed!), so the version should go up to 7.0.0 (https://semver.org/ spec item 8)

Obviously this is Isaac's crate so hopefully none of that is out of line!

Thanks for the feedback. Fully agreed on the global allocator default, I made a patch that ensures both new and new_in are present. On Allocator + Clone though, I did consider using refs initially but was deterred by the sheer amount of lifetimes that will need to be threaded through, though largely mechanical, probably would benefit from a larger discussion (Allocator + 'static just introduces another constraint, one which I personally chose to avoid because it's incompatible to the project where this patch is being actively used). Given that allocators are generally cheap in practice, I think it's a reasonable solution for this PR.

My bad on the confusing commit messages, I use autocommit for essentially 100% of my commits (I was notoriously bad at writing commit messages manually and didn't want to waste AI tokens on such a simple task...), sometimes they can be a bit... wrong, to say the least.

Also agreed that this is quite massive of a PR, happy to break it down into smaller pieces to make it easier for reviews if needed.

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

@SnowCheetos @martin-hughes