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.

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

aml: DefIndexField implementation #186

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alnyan
Copy link
Contributor

@alnyan alnyan commented Aug 23, 2023

Hi, I created this pull request (along with the issue #184) to track the implementation of DefIndexField opcode as well as to make sure I'm going the right direction with that.

The PR is still work-in-progress and doesn't have some corner cases covered (TODOs in the comments), but if you think the implementation so far is okay, I'll extend it and finish the PR.

@alnyan alnyan marked this pull request as draft August 23, 2023 09:39
Copy link
Member

@IsaacWoods IsaacWoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start!

The parsing looks good, and from a quick glance the access stuff looks sensible. Will have a closer look at some point in the future.

Re needing a mutable context in as_integer and friends - this has cropped up as a problem before (cc #155, which actually still needs a review oops), and doesn't seem like an easy problem to solve. On the one hand, turning a value into an integer doesn't feel like something that should need that mutable access; on the other, ACPI is ACPI and so the dividers on what should be a simple vs complex operation are non-existent.

One possibility is of course to suck this up (i.e. and make as_* take a mutable context, but it does make user code pretty gross. Another would be to separate it out so that as_integer can only do the intrinsic conversions (buffers to ints and friends), and users are required to do a separate resolve_fields or whatever first. It may turn out we basically have to do that everywhere, so it isn't any better than just sticking it in as_integer. I feel I'll need to sit down and play around with the code when I get the chance to see how it feels.

I guess a final option is to move to having some sort of dynamically-checked borrowing of
the context, or internally, parts of the context (e.g. namespace, handler). However, this feels
like a great way to introduce hard-to-debug deadlocks, so I'm an advocate for holding off
on that for as long as possible.

Re read_field needing mutable context access - that's an inevitability really I think, as #155 shows, as that can involve PCI access, which can involve method invocation, so don't worry about that.

* Reserved fields shouldn't actually be added to the namespace; they seem to show gaps in
* the operation region that aren't used for anything.
*
* NOTE: had to use raw_pkg_length() instead of pkg_length() here because pkg_length() performs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah this is a really good point! Looks like we're using the raw length in the other field code but still parsing with a pkg_length, which I guess could cause errors if the op region was longer than the remaining AML slice. We should probably look at that in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've created a separate issue for that already. The parsing of the field might become a little more complicated, as instead of just pkg_length() we would need to also pass a opreg to the parser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for implemeting as_* functions on IndexFields, I've done that on my separate branch where I just try to make a dirty but fully working implementation for my laptop, but that involves making almost everything (write_field/write_region/etc.) take an immutable ref to an AmlContext instead of a mutable one which doesn't seem quite right.

I've also tried making as_integer take a mutable ref, but then we'd have a problem in some places where pieces of code like:

// in def_increment()/def_decrement() parsers
let value = try_with_context!(context, context.read_target(&addend));
let value = try_with_context!(context, value.as_integer(context));

fails to compile because the first line now borrows context immutably and the second one tries to borrow it mutably. Fixing that in and preserving mutability of the ref might involve either moving the context inside some kind of Spinlock or making wrapper functions that merge the functionality of read_target() and as_* without needing to borrow twice.

@IsaacWoods
Copy link
Member

Hey @alnyan - apologies for the lack of a review for a long while. This should hopefully now be unblocked as of #208 - the rest of the changes look good so it'd be great to get this merged if we can.

Also fine if you'd rather not work on this further - just let me know and I'll get this rebased when I can :)

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.

None yet

2 participants