-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
3762e79
to
e65ccf4
Compare
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 :) |
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.