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: Add DefExternal #145

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

Conversation

rw-vanc
Copy link
Contributor

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

Implement DefExternal, with If(0) guard

  • Externals are in an If(0) block. See acpica/documents/changes.txt, Summary of changes for version 20160212.
  • 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.
  • New function add_external_levels optionally adds all parent levels when defining an External symbol. This is necessary because of real-world AML e.g. a table with this as the first declaration: External (_SB_.PR00._CST, UnknownObj)
  • Name collision handling now checks for the following situations:
    • add_value External when value exists - return the old handle
    • add_value other than External when External exists - replace the value at the old handle and return the old handle
    • add_value other than External when value exists - NameCollision error
    • add_level External or Scope when level exists - no change
    • add_level not External or Scope when level was External or Scope - update level.typ
    • add_level not External or Scope when level was not External or Scope - NameCollision error
  • Added tests for the above

@rw-vanc rw-vanc changed the title Add DefExternal aml: Add DefExternal Feb 19, 2023
@IsaacWoods
Copy link
Member

Sorry it’s taken me so long to get to this, very busy at the moment. Haven’t been able to have a proper look through the other changes, but I have concerns over reading through If(0) guards.

The full quote from the ACPICA docs:

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.

We are an AML interpreter, not a decompiler. This makes me feel that we should not be reading External opcodes at all, past our current functionality of parsing and ignoring it to handle them appearing in the byte stream. If real firmware is failing, I don’t think this is the correct way of fixing it. I’ll be able to have a closer look at this soon - I think we should make sure we’re parsing the tables in the correct order, and we may need to explore deferring parsing properly in some cases.

@IsaacWoods IsaacWoods self-assigned this Mar 27, 2023
@rw-vanc
Copy link
Contributor Author

rw-vanc commented Mar 27, 2023

I am working my way through parsing my HP laptop AML. It's impossible to tell whether we can get by without Externals at this point, because I am still working on getting through the DSDT. The DSDT does have Externals, but at least some of the external references are guarded by CondRefOf. Once I am able to parse a full set of tables, I will try disabling the External code in my version of the parser and see what happens. It could be quite a while, so please leave this open for now.

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