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: Handle DefExternal (real hardware) #135

Open
rw-vanc opened this issue Jan 26, 2023 · 15 comments
Open

AML: Handle DefExternal (real hardware) #135

rw-vanc opened this issue Jan 26, 2023 · 15 comments

Comments

@rw-vanc
Copy link
Contributor

rw-vanc commented Jan 26, 2023

On HP-dq3xxx, there are references to scopes before they are defined. It's possible some of the problem is due to other parser errors and/or parsing the tables out of order. It's not clear if it's possible to guess what order to the tables should be parsed in to avoid this. Here is my proposed approach:

  1. Assume that the AML we are parsing is correct, and that any errors are ours
  2. Automatically create a scope of type LevelType::Scope whenever a new value or scope is created.
  3. If, during explicit creation of a new scope, if the old type was Scope, update it with the new type. If neither type was Scope and they don't match, maybe throw an error.
@rw-vanc
Copy link
Contributor Author

rw-vanc commented Jan 26, 2023

I added a new function "get_or_add_scope" to transparently/recursively create new scopes during symbol declaration. Let me know if you want me to submit that, and your protocol for submitting.

@IsaacWoods
Copy link
Member

Thanks for filing these issues and working on contributions! I'm very happy for anything to just be opened as PRs and I'll review them when I get the chance :)

Assume that the AML we are parsing is correct, and that any errors are ours

It's a mixed bag. There's a tonne of things we don't handle correctly, but it's also often the case that tables from real firmware don't conform strictly to the spec. Generally, we have no choice but to be as flexible with parsing as we possibly can be to handle real AML.

  1. Automatically create a scope of type LevelType::Scope whenever a new value or scope is created.
  2. If, during explicit creation of a new scope, if the old type was Scope, update it with the new type. If neither type was Scope and they don't match, maybe throw an error.

Hm, this could potentially be reasonable, although I'm not sure we should preemptively create scopes for normal values until they're needed. Could you provide some example ASL that would require this?

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 4, 2023

I'm a little confused by what's going on, it doesn't seem to always break, but this breaks it.

DefinitionBlock ("", "SSDT", 2, "HPQOEM", "88E9    ", 0x00003000)
{
    External (_SB_.PC00, DeviceObj)
    External (_SB_.PC00.GFX0, DeviceObj)
...
    Scope (\_SB.PC00.GFX0)
    {
SSDT @ 0x0000000000000000
    0000: 53 53 44 54 CD 32 00 00 02 2B 48 50 51 4F 45 4D  SSDT.2...+HPQOEM
    0010: 38 38 45 39 20 20 20 20 00 30 00 00 41 43 50 49  88E9    .0..ACPI
    0020: 18 10 19 20 A0 40 26 00 15 5C 2E 5F 53 42 5F 50  ... .@&..\._SB_P
    0030: 43 30 30 06 00 15 5C 2F 03 5F 53 42 5F 50 43 30  C00...\/._SB_PC0
    0040: 30 4D 43 5F 5F 06 00 15 5C 2F 03 5F 53 42 5F 50  0MC__...\/._SB_P
    0050: 43 30 30 47 46 58 30 06 00 15 5C 4E 44 49 44 00  C00GFX0...\NDID.
Testing AML file: "../hp-ron/ssdt4.aml"... [TRACE] --> DefIfElse
[TRACE] <-- DefIfElse
[TRACE] --> DefScope
[TRACE]   Scope name: \_SB_.PC00.GFX0
[ERROR] Failed to parse AML stream. Err = LevelDoesNotExist(AmlName([Root, Segment("_SB_"), Segment("PC00")]))

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 4, 2023

Testing AML file: "../hp-ron/ssdt4.aml"... [TRACE] --> TermObj
[TRACE]   --> NamedObj
[TRACE]     --> StatementOpcode
[TRACE]       --> DefIfElse
[TRACE]         --> TermArg
[TRACE]           --> DataObject
[TRACE]             --> ComputationalData
[TRACE]             <-- ComputationalData
[TRACE]           <-- DataObject
[TRACE]         <-- TermArg
[TRACE]       <-- DefIfElse
[TRACE]     <-- StatementOpcode
[TRACE]   <-- TermObj
[TRACE]   --> TermObj
[TRACE]     --> DefScope
[TRACE]       --> NameString
[TRACE]       <-- NameString
[TRACE]       Scope name: \_SB_.PC00.GFX0
[ERROR] Failed to parse AML stream. Err = LevelDoesNotExist(AmlName([Root, Segment("_SB_"), Segment("PC00")]))

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 4, 2023

The error might actually be down here.

    01E0: 00 15 5C 50 42 43 4C 08 00 15 5C 2F 04 5F 53 42  ..\PBCL...\/._SB
    01F0: 5F 50 43 30 30 47 46 58 30 48 44 4F 53 08 00 15  _PC00GFX0HDOS...
    0200: 5C 45 43 4F 4E 01 00 15 5C 50 4E 48 4D 01 00 15  \ECON...\PNHM...
    0210: 5C 2F 04 5F 53 42 5F 50 43 30 30 47 46 58 30 4F  \/._SB_PC00GFX0O
    0220: 53 59 53 01 00 15 5C 2F 04 5F 53 42 5F 50 43 30  SYS...\/._SB_PC0
    0230: 30 47 46 58 30 43 50 53 43 00 00 15 5C 47 55 41  0GFX0CPSC...\GUA
    0240: 4D 08 01 15 5C 2F 04 5F 53 42 5F 50 43 30 30 47  M...\/._SB_PC00G
    0250: 46 58 30 44 53 45 4E 00 00 15 5C 2F 04 5F 53 42  FX0DSEN...\/._SB
    0260: 5F 50 43 30 30 47 46 58 30 53 30 49 44 00 00 15  _PC00GFX0S0ID...
    0270: 5C 2F 04 5F 53 42 5F 50 43 30 30 47 46 58 30 48  \/._SB_PC00GFX0H
    0280: 4E 4F 54 08 01 10 87 04 03 5C 2F 03 5F 53 42 5F  NOT......\/._SB_
    0290: 50 43 30 30 47 46 58 30 08 44 50 4C 44 12 1A 01  PC00GFX0.DPLD...

@IsaacWoods
Copy link
Member

I think this is probably an issue with how we handle DefExternals? We currently parse them but don't add anything to the namespace - the idea being another table will add those values later on. This doesn't work in that case - I wonder if we need to add scopes, devices, etc to the namespace when they're first defined by a DefExternal, and then gracefully handle when they're "re-defined" in the real table?

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 4, 2023

Do you know where I can get info on how to decode the ObjectType byte? I can't find anything in the spec.
DefExternal = 0x15 NameString ObjectType ArgumentCount

@IsaacWoods
Copy link
Member

Hm yeah it's not obvious where they define that. My guess would be that they use the same constants as for DefObjectType, as it would be strange to define multiple (but I wouldn't put it entirely past ACPI). You can find the values in Table 19.36 of the spec. It would definitely be worth testing that they're correct by passing some ASL through the parser.

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 4, 2023

It looks like the entire list of externals is defined in an If(false) block. Any advice on how to parse the If(false) safely? I will take a shot at it myself, but any input you have is appreciated. I've attached to whole acpidump file, there are many other errors that will come up, so you might find it useful for future reference. I'm working on the first ssdt.
acpidump.log

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 5, 2023

My current solution has the following features. Feedback is appreciated, but I will submit something soon if you want to read through the code first.

  • Because Externals are in an If(0) block, a special case of def_if_else checks for If(predicate == false) with no Else. It doesn't check for If(0) specifically, as the details of the predicate are not available when the check is done. This special-case code calls a new parser function, externals_list, on the then_branch.
  • externals_list takes a list_length, and operates similar to term_list. It accepts any number of Externals, but if it gets something other than External, it just does take_to_end_of_pkglength and discards the result. This should allow it to still handle the case where If(false) occurs in other situations, as it will just skip the content as it did previously. However, the case that Externals are mixed with other statements will not be processed completely. I don't know if this occurs in real-world examples.
  • def_external adds values and levels to the namespace. It considers the ObjectType when deciding if it should add it as a level.
  • ObjectTypes for External declarations are taken from acpica source because it is more accurate than the spec.
  • Levels added are LevelType::External. Values added are AmlType::External.
  • Attempts to convert External to Integer, etc. will fail, as the value is not known. I'm not sure how to order the processing of tables to ensure actual definitions occur before attempts to read Externals. I will modify my own aml_tester for my testing.
  • New function add_external_levels optionally adds all parent levels when defining an External symbol. This is necessary because I have e.g. a table with this as the first declaration: External (_SB_.PR00._CST, UnknownObj)
  • I plan to add the ability to not error when a collision with an External occurs. Instead the LevelType and ValueType will be updated.

@rw-vanc rw-vanc changed the title AML: Scope creation needs to be more liberal (real hardware) AML: Handle DefExternal (real hardware) Feb 5, 2023
@IsaacWoods
Copy link
Member

IsaacWoods commented Feb 5, 2023

Because Externals are in an If(0) block, a special case of def_if_else checks for If(predicate == false) with no Else. It doesn't check for If(0) specifically, as the details of the predicate are not available when the check is done. This special-case code calls a new parser function, externals_list, on the then_branch.

I'm not sure how this could be correct - If(0) blocks seem like they would be used precisely to prevent AML from being run, perhaps set in a template ASL file from the firmware providers or whatever, and so we shouldn't be parsing those Externals. Have I misunderstood this, or does ACPICA or another implementation do something similar to this?

Just checking that you are parsing the DSDT first and then attempting to add SSDTs - this is defined by the spec as far as I can remember?

I have not had time to look at your dump in detail yet - is there any chance you could provide the ASL for the DSDT and SSDTs (you can decompile with iasl)

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 5, 2023

I just found this in "acpica/documents/changes.txt":
12 February 2016. Summary of changes for version 20160212:
...
Completed full support for the ACPI 6.0 External() AML opcode. The
compiler emits an external AML opcode for each ASL External statement.
This opcode is used by the disassembler to assist with the disassembly of
external control methods by specifying the required number of arguments
for the method. AML interpreters do not use this opcode. To ensure that
interpreters do not even see the opcode, a block of one or more external
opcodes is surrounded by an "If(0)" construct. As this feature becomes
commonly deployed in BIOS code, the ability of disassemblers to correctly
disassemble AML code will be greatly improved. David Box.

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 6, 2023

It appears that namespace.add_value adds the value to the object_map regardless of whether the key can be added to level. Is it ok to only add the value if adding the key is successful (i.e. level exists and no name collision)?

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 6, 2023

Here's my asl corresponding to the acpidump.log. I will try to give a mapping from file name to location in the dump file, when I get time.
asl_files.zip

@rw-vanc
Copy link
Contributor Author

rw-vanc commented Feb 6, 2023

Please feel free to comment on the PR, I'm open to suggestions and improvements.
PR 145

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

No branches or pull requests

2 participants