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

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.

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
### Changes
- [`src/platform/numa.rs`] Use &AcpiTables: Allow passing `AcpiTables` as a reference to the `new` method

### Risk
- Level: low
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`.
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.
…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.
# 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

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!

### 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

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.

@IsaacWoods

IsaacWoods commented Jun 17, 2026

Copy link
Copy Markdown
Member

Hey @SnowCheetos - thanks for the PR!

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!

You're always very welcome to review @martin-hughes - thanks for your thoughts here and I largely echo them.

  • In principal, I'm in favour of allowing a different allocator to be used with Interpreter, and this will likely require parameterising many types with an A. I do agree that the Clone bound is unfortunate but I understand lifetimes here would likely worsen code comprehensibility further (it's possible more could take references to avoid cloning, maybe enough to avoid the bound in some/all places? Then the lifetime of &A would be that of the interpreter)
  • I do think this patch decreases readability quite a bit, and has some pretty janky edges that I feel it's unfortunate there is not better support upstream for (I've had a quick look across rust-lang and don't really understand why String cannot take an allocator when types like Vec can). To go forward, I would like to address quite a few individual bits of the patch re style and readability (and that could go alongside some higher-level changes to e.g. error types as noted by Martin).

However, unfortunately I feel I cannot review this PR properly as it stands. The changeset is huge and far too varied, and the use of this autocommit utility has rendered individual commits largely useless. I am not entirely against LLM use (although I choose not to use it myself) but contributions proposed by a person are their responsibility regardless of tool use. For example, 841c15e is marked as a style change, but includes breaking changes to public API (it's a good change, fwiw, it was an unfortunate oversight it was not like that before). I don't understand the commit message in 8023fbf at all - MethodContext already existed and handles arguments, and with the style churn I cannot work out what the commit is actually doing. I also do not want a utility changing version numbers multiple times within a single PR, and I echo Martin's thoughts that it doesn't seem to understand semantic versioning at all. I don't understand the value of the comments re risk in the messages either.

If you would like to take these changes further, please:

  • Split this into a few PRs at least
    • The changes in the first and second commits are very welcome - thanks for finding those! They could be bundled into a PR
    • The changes to facilitate an allocator bound on Interpreter. Broad strokes here are that I don't really feel I want a home-grown AmlString type (string_alloc looks good if needed), error bits likely need more work, churn to macros and op methods seem unfortunate
  • Formatting changes make this difficult to review. We do have a rustfmt.toml file that should negate most of this so I'm wondering if your editor is honouring it?
  • The history blocks merging this as is, and is not really amenable to rebasing. Splitting into multiple PRs to address different features will help but the allocator changes will need proper commit messages that explain rationale behind decisions (I much prefer this to comments in code explaining author intention). I'm not dictating that narrative must be entirely human-written or anything, but you as the author are the one that can provide insight into what you were thinking with certain changes.
  • Rebase PR branches instead of merging

I hope this comes off as constructive as intended - it may be that my very limited time and brain power is what precludes me from being able to review a PR of this size. Supporting allocator_api in aml is something I'm definitely interested in supporting, and I appreciate you doing some exploration in this space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants