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: use invoke_method on _SEG, _BBN, _ADR #155

Closed
wants to merge 1 commit into from

Conversation

rw-vanc
Copy link
Contributor

@rw-vanc rw-vanc commented Feb 11, 2023

Use invoke_method on special symbols _SEG, _BBN and _ADR as they might be methods. invoke_method will just return their value if they are not methods.
Make read_region use mutable context. Change clients of read_region (as_integer etc.) to use mutable context. Clone AmlValue objects before calling as_integer etc. to avoid borrow conflicts.

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 11, 2023

This has not been thoroughly tested. It works for me. I don't know how to test it better.

@rw-vanc rw-vanc changed the title use invoke_method on _SEG, _BBN, _ADR aml: use invoke_method on _SEG, _BBN, _ADR Feb 19, 2023
@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 4, 2024

I would like to find a way forward on this. Most of the problems are fixable without much work, but DefIncrement and DefDecrement are creating a problem I don't know how to fix.

First, an AmlValue is obtained using the context, then the AmlValue needs to be converted to an integer, which requires a mutable copy of the context due to potential function parsing. This is causing a borrow conflict.

Please let me know what you think the solution might be. Should we modify AmlContext so function parsing does not need to be &mut? Or is there a way to eliminate the borrow conflict in this specific case?

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 5, 2024

I'm working through removing the need for AmlContext to be mut. I'm going to move Namespace to NamespaceInner and have Namespace provide a mutex wrapper. There's a few other things that will need to be wrapped. I will let you know how it goes.
Edit: That's not the right solution. It's going to take some work to get through this. I will post a draft for review when I have something worth looking at.

@IsaacWoods
Copy link
Member

Firstly, apologies are due for the incredibly long time this has taken me to review - sorry for that. I've not had any time or motivation to work on acpi recently, mainly due to my degree and different OSDev interests in the last few months.

Thank you for your interest in contributing despite the lack of reviews. I've put some time aside this evening to try and get this over the line.


I'm working through removing the need for AmlContext to be mut.

I'm still not sure this is the right approach tbh - it may well still be the only solution that works at some point in the future, but this doesn't feel like it's what we should be giving up compile-time checks for. I hope you haven't put too much time into it for this, apologies if so (and keep it around if you find a promising solution, as I've said I might be wrong about enforcing this through borrow checking).


I'm currently working on getting a version of main compiling with the relevant methods made to take a mutable context. It looks promising so far, but will need some more faffing. In particular, DefIncrement/DefDecrement should be able to do something along the lines of:

-                let value = try_with_context!(context, context.read_target(&addend));
+                let value = {
+                    let foo = try_with_context!(context, context.read_target(&addend));
+                    foo.clone()
+                };
                 let value = try_with_context!(context, value.as_integer(context));

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 5, 2024

The one concern with your suggestion for DefIncrement is that the function will be duplicated by the clone of foo. Maybe the method body needs to be Arc'd?

I only put a few hours into it, just to see where the roadblocks were. I have no issue with spending time on this, even if it ends up on the cutting room floor. It's now an urgent priority for Redox to be able to parse the AML for as many real machines as possible.

@IsaacWoods
Copy link
Member

IsaacWoods commented Mar 5, 2024

The one concern with your suggestion for DefIncrement is that the function will be duplicated by the clone of foo. Maybe the method body needs to be Arc'd?

So it turns out that DefIncrement and DefDecrement are strange in a way that I didn't really appreciate - they're supposed to modify values in-place. That means the thing passed to them both needs to resolve to an Integer, but also be capable of having a value stored to them.

This is basically restricted to Local*, Arg*, and named objects (although I'll need to check how this is supposed to interact with references more closely), so the clone really shouldn't be a problem.

I should have a PR up in a little while - it encompasses the changes from this PR with those commits co-authored by you.

It's now an urgent priority for Redox to be able to parse the AML for as many real machines as possible.

Understood. It'll be interesting to see how the library holds up - I've only got a few x86 machines (the DSDTs of which all parse, but not all the SSDTs iirc) so it'll be cool to have more testing capability on your end.

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