iASL and ACPICA aren't able to share the same grammar rules

[mirh]

I utterly see iasl is meant to be an high quality aml compiler fully complaint with latest ACPI spec, but this would help immensely when it comes to fixing bad DSDT tables.
In particular the code I had my hands on at the moment was:

        Device (SATA)
        {
            Name (_ADR, 0x00110000)  // _ADR: Address
            If ((STCL == 0x0101))
            {
                Method (_INI, 0, NotSerialized)  // _INI: Initialize
                {
                }....

Fairly, I'm alerted there's an unexpected PARSEOP_IF.. It's an opcode outside of a method after all.

But for as much as I racked my brain I couldn't come up with a rationale of what ever this had been supposed to serve (nor why none of my storage devices hadn't already mishbehaved). I mean, I can't see a way any person, even drunk, could have put something that blatantly amiss there.

Then I found 80d7951, 66e20c8 and so on (btw, thanks for fixing that pesky AE_NOT_FOUND problem).

For as much that code was now illegal, it may still have had its sense once upon a time (and in Windows..)
And I (with no damn docs or datasheet) wouldn't really be able to imagine a consistent substitute, if even possible at all.

So I hoped some kind of "unwarranted within ACPI specs but valid within ACPI Component Architecture" swtich could be added to the project own compiler.
Thanks for your time.

EDIT: Wtf? Is module-level executable code now legitly allowed?

[acpibob]

Unfortunately, this error is caught by the main parser, which is generated by Bison/Yacc -- and this means that such errors are not (easily) recoverable. Also, it isn't clear how AML interpreters would handle this code.

That being said, we will take another look at the issue.

[acpibob]

"module-level code" is in fact allowed as long as the code is in fact at module level. This was put back into the ACPI spec at some point. Lots of DSDTs use this construct to selectively add things to the namespace based upon BIOS configuration data.

[mirh]

Soo...

• is this still working (despite being wrong) just because kernel has mercy..
• or is this (ideally flawlessly) working because it's indeed 100% legitimate code?

Because I mean, in the first case my request would still stand and all..
while in the second one it would be a real bug in the compiler.

I posted the full dsdt here anyway, if knowing the aforementioned code block is under Device (PCI0) which is "inside" Scope (_SB) can help.

[zetalog]

We've been making this working for interpreter and succeeded in this series:
http://www.spinics.net/lists/linux-acpi/msg63550.html
It is validated that Windows supports such kind of code.

But for compiler, we don't support it, currently we can use Include to generate such kind of table with iasl.

[zetalog]

With iasl, I now generate the table in this way:
In module.asl
If ((STCL == 0x0101))
{
Method (_INI, 0, NotSerialized) // _INI: Initialize
{
}....
In main.asl
Device (SATA)
{
Include "module.asl"

You can try it and test the generated table with Windows.

Thanks and best regards
-Lv

[zetalog]

The module-level execution is supported in the way mentioned by you:
80d7951 , 66e20c8
Because there are many issues related to the combined code path/locking/order unresolved.
I caught most of them, but not sure if what I caught is all of them.
Supporting the execution of such code in fact is very nature for an interpreter, it can just execute the entire table in the same way it executes a control method and hold the references of the created objects until the table is unloaded.
So enabling it is not hard, but running the table loading in this way now could trigger many old issues that originally solved in totally different (and obviously wrong) way.

Thanks and best regards
-Lv

[zetalog]

So I don't think the issue is an ACPICA only issue, ACPICA only plays a part of the ACPI functionality in an operating system. IMO, Both the host (the user) and ACPICA have the responsibility to have this solved by sitting together and dicussing constructively. If the hosts side issues are not solved in the correct way, ACPICA may have to work in a wrong way.

[zetalog]

IMO, it is a real bug.
From the development/design's point of view, it is no good to add such a complexity to an interpreter to behave totally differently against table loading and method execution given the fact both of the actions are to interpret same syntaxed AML.
Why an interpreter implementation need to implement 2 different style interpretion?
The answer is simple.
It is implemented in different way just because it wants to work something around. For examples, lockings, correct state machine, and so on.

Thanks and best regards
-Lv

[acpibob]

Let me clarify things a bit.

    Device (SATA)
    {
        If ((STCL == 0x0101))

1. According to the ACPI specification (and the ASL grammar within), this code is illegal. Executable code underneath a device is not allowable via the language grammar.

2. There was a bug in the iASL compiler that accidentally allowed this construct. It was recently fixed.

3. During the time that this bug in the compiler existed, some BIOS vendors wrote code (in some rare cases) that used this construct.

4. Regardless of whether any AML interpreters will execute this code, it is still technically illegal by virtue of the ASL grammar. I should point out that at this time, there is no existing ASL compiler that will compile this code.

5. We are not certain at this time what changes (if any) will be made to the compiler and/or the ASL grammar, but this will take coordination/approval with Microsoft and the multi-company ACPI working group. Also, the possibility certainly exists that there will be side-effects to allowing this type of code.

6. I should also point out that there is no 100% guarantee that disassembled ASL code will actually recompile. This is at least due to the nature of the AML code where (for example) a guess has to be made concerning external control methods (methods called from one table but defined in another). If the guess is wrong, the ASL code will not compile. Thus, there is no fully deterministic way to disassemble AML code.

7. A workaround would probably be to move this code out from under the Device() definition and up to the top level of the definition block:

    If ((STCL == 0x0101))
    {
    }
    Device (SATA)
    {

[mirh]

Wait.. I guess you guys should first agree on the whether this code is legal or not.. :s

One's link basically mimics that, then another claims it's wrong.
And btw, that last workaround doesn't seem to work because this device is in turn inside another device... and I guess I'd have better to delete the check altogether if any, rather than moving it on irrelevant code.

I decompiled the code with -e switch and its two ssdt tables for the records, there shouldn't have been anything "to guess".

[acpibob]

The key to the workaround is this: "move this code out from under the Device() definition and up to the top level of the definition block:" i.e., move the code all the way up to the top level. Then, the code will always be executed, at table load time.

Even -e doesn't always work because of dynamic declarations of objects and dynamic loading of tables (SSDTs). But it often does work.
Only -fe is the sure way to resolve all external methods correctly.
There are a couple more issues with disassembly, but they are rare.

Yes, the legality of the code is a large issue because it involves the ACPI spec and the grammar of the ASL language.

[mirh]

Then, the code will always be executed, at table load time.

Yes, but wouldn't this result in a lot more stuff depending on the conditional statement?

Yes, the legality of the code is a large issue because it involves the ACPI spec and the grammar of the ASL language.

What about this then?

[acpibob]

I don't have time to read through this today, but:

1. It is known that the current module-level code support has limitations, and Lv is working on it. We will execute the table as one big control method.
2. If you examine the ACPICA code, you will see comments in several places that control methods cannot be executed until the hardware is initialized (GPEs, etc) and default region handlers are installed, etc.
3. We have worked for years to get the initialization sequence correct so that it runs everywhere correctly.
4. One thing that must be considered carefully: ACPICA must be bug-for-bug Windows compatible. It is not a reference implementation of the ACPI specification.

That being said, we will address your concerns, but it may take a bit of time.

[mirh]

Ook, got it.
You'll know better than me how to fix the thing :p
I'll rename the issue to reflect the current "state of messing".
Good luck

[zetalog]

First, non named object definition opcodes directly put at the table level are legal by the spec.

DefinitionBlock ("dsdt.aml", "DSDT", 1, "HP    ", "INSYDE  ", 0xF0000000)
{
    If ((STCL == 0x0101))

The grammar in spec (since 2.0) is:

DefinitionBlockTerm :=
    DefinitionBlock (
        AMLFileName         // String
        TableSignature      // String
        ComplianceRevision, // ByteConst
        OEMID,              // String
        TableID,            // String
        OEMRevision         // DWordConst
    ) {TermList}
       ^^^^^^^^

While this is not supported by ACPICA - this is the TermList bug I'm fixing.

For the following ASL, it is illegal by spec because spec uses ObjectList instead of TermList:

    Scope (\)
    {
        If ((STCL == 0x0101))

And

    Device (SATA)
    {
        If ((STCL == 0x0101))

The grammar in spec is:

ScopeTerm :=
    Scope (
        Location            // NameString
    ) {ObjectList}
       ^^^^^^^^^^

DeviceTerm :=
    Device (
        DeviceName          // NameString
    ) {ObjectList}
       ^^^^^^^^^^

For ObjectList -> TermList grammar change, this is just a natural consequence of the changed behavior of the table parsing. And in fact, the changed behavior doesn't only affect table parsing, but affect method parsing. So IMO, this is a different topic. And since the grammar definition in spec is ObjectList, I think this probably is a spec bug. Because:

• Since TermList is allowed at anonymous root scope, why can't it be allowed under any scope opened by Scope() opcode?
• If TermList is allowed under any scope opened by Scope() opcode, why can't it be allowed under any scope created by Device()/PowerResource()/Processor()/ThermalZone() opcodes? What the difference between the scope creation opcodes (Device, etc.,) and the scope opening opcode (Scope). Forcing such kind of differences can only leave inconvenience to the ASL developers.

We have filed an ECR for ASWG to correct the spec bug as Windows doesn't parse these terms (DeviceTerm/ScopeTerm and etc.,) using ObjectList, instead it parses them using TermList.

So IMO, we are talking about 2 different things:

• ACPICA bug of TermList parsing support at the table level: here ACPICA is not compliant to the spec.
• ACPI spec bug of the ObjectList grammar definition and its usages: here ACPICA is compliant to the spec but the spec is not compliant to the de-facto standard.

Please don't confuse them.

Thanks and best regards
-Lv

[mirh]

Cool!
I just tried iasl 20160831 and I guess #170 did miracles.
Now it's no longer stuck there, but I have another error (the last one hopefully).

                                                       11580:             }
Error    6126 -                             syntax error, unexpected '}' ^

                                                       11600:             }
Error    6126 -                             syntax error, unexpected '}' ^

(Then I'd hope the unexpected $end and premature End-Of-File error has something to do with this other ones messing up with parenthesis)
You can find the dsl here.

Of course, again, I used -e switch (otherwise the resulting dsl table has way more errors with -tc).

[acpibob]

Please send the original binary DSDT, thanks.

From: mirh [mailto:notifications@github.com]
Sent: Thursday, September 1, 2016 1:08 AM
To: acpica/acpica acpica@noreply.github.com
Cc: Moore, Robert robert.moore@intel.com; Comment comment@noreply.github.com
Subject: Re: [acpica/acpica] iASL and ACPICA aren't able to share the same grammar rules (#122)

Cool!
I just tried iasl 20160831 and I guess #170#170 did miracles.
Now it's no longer stuck there, but I have another error (the last one hopefully).

                                                   11580:             }

Error 6126 - syntax error, unexpected '}' ^

                                                   11600:             }

Error 6126 - syntax error, unexpected '}' ^

(Then I'd hope the unexpected $end and premature End-Of-File error has something to do with this other ones messing up with parenthesis)
You can find the dsl herehttp://pastebin.com/0U1JMtQ3.

Of course, again, I used -e switch (otherwise the resulting dsl table has way more errors with -tc).

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com//issues/122#issuecomment-244006863, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA6_rNXyOqk4AGkRWAW1aD88QlSKtsU9ks5qlof0gaJpZM4HiPbF.

[mirh]

Here you are.

[acpibob]

I can't get to this link, please email it to me directly

[mirh]

Sent.

[acpibob]

Thanks, I've been able to reproduce the problem.

There are two of these (invalid) statements that are causing the problem:

            Name (_PLD, ToPLD ()  // _PLD: Physical Location of Device

It is a disassembly problem, I think -- we need to investigate further.
Bob

[acpibob]

It is in fact a problem with the disassembly of the _PLD statements, there is a rather odd case in the AML where the _PLD buffer is defined to be 16 bytes in the AML, but the actual number of bytes in the buffer is only 4.

On another subject, this DSDT has a whole bunch of problems, as below:

c:\0acpi-bugs\pld-disasm>iasl -f -ve dsdt.dsl

Intel ACPI Component Architecture
ASL+ Optimizing Compiler version 20160831-32
Copyright (c) 2000 - 2016 Intel Corporation

Ignoring all errors, forcing AML file generation

dsdt.dsl 1532: 0xFDFC0000, // Length
Error 6049 - ^ Length is larger than Min/Max window

dsdt.dsl 5675: (CTRL & 0x1E)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 8565: Name (_HID, "PnP0C14") // _HID: Hardware ID
Error 6136 - ^ Non-hex letters must be upper case (nP0C14)

dsdt.dsl 9132: ((QDEV (0x02) << 0x02) + Local0)
Error 6114 - Result is not used, operator has no effect ^

dsdt.dsl 9133: ((QDEV (0x04) << 0x04) + Local0)
Error 6114 - Result is not used, operator has no effect ^

dsdt.dsl 9134: ((QDEV (0x08) << 0x06) + Local0)
Error 6114 - Result is not used, operator has no effect ^

dsdt.dsl 10045: (0x02 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10050: (One | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10055: (0x04 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10060: (0x08 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10065: (0x10 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10077: (0x02 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10082: (One | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10087: (0x04 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10092: (0x08 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10097: (0x10 | Local1)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 10959: (0x0F & ODDM)
Error 6114 - ^ Result is not used, operator has no effect

dsdt.dsl 11579: Name (_PLD, Buffer (0x10) // _PLD: Physical Location of Device
Error 6105 - ^ Invalid object type for reserved name (_PLD: found Buffer, Package re
quired)

dsdt.dsl 11602: Name (_PLD, Buffer (0x10) // _PLD: Physical Location of Device
Error 6105 - ^ Invalid object type for reserved name (_PLD: found Buffer, Package re
quired)

ASL Input: dsdt.dsl - 11878 lines, 376850 bytes, 5634 keywords
AML Output: dsdt.aml - 47706 bytes, 1415 named objects, 4219 executable opcodes

Compilation complete. 19 Errors, 25 Warnings, 141 Remarks, 86 Optimizations, 6 Constants Folded

[mirh]

I got no such errors with the dsl table decompiled with -e switch.

[acpibob]

What version of the compiler?

[acpibob]

If you got the 2 syntax errors as above, the compiler has aborted before performing a full analysis.

[mirh]

What version of the compiler?

Last one ofc.

If you got the 2 syntax errors as above, the compiler has aborted before performing a full analysis.

Oh.. Then all the previous errors were just hiding these ones?

[acpibob]

Yes, the 2 previous syntax errors were hiding the other 19 semantic errors.

[acpibob]

You can comment out the 2 copies of this line in the .dsl file, and see what happens when you compile:

            Name (_PLD, ToPLD ()  // _PLD: Physical Location of Device

[mirh]

Nah, I have no hurry.
Take your time to figure out the problems with disassembly. 😃

[mirh]

At least forward references for ML code have yet to land in linux (where I can test them - though they should in a month)
Then I dunno if there are others asymmetries left I should take into account.

[mirh]

Ok, forward references didn't make my day.
I do believe hence, what I'm looking forward to is some of the remaining NameString work of #189

[SchmErik]

Can you describe the issue in more detail?

[mirh]

Well, the whole thing started with me noticing (when trying to recompile my DSDT) that some errors were really too strange, for even a blindfolded monkey to have written them.
But even more strangely, acpi wasn't even spewing a warning when using it.

Now, my original example has been patched with the landing of MLC (and _PLD fix), but after discussing here I was getting the impression a tide of changes was coming (very likely to influence the issue)
As far as at least the errors I was getting though, after a year the exact same ones show.
So.. I guess proper rationale should be calling it a day by now, unless proven wrong in some whatever future.

p.s. 4e78ce6 changed RESULT_NOT_USED in errors but grammar.asl still reports old behavior (possibly of more functions too)

[acpibob]

I'm still not entirely sure what you mean by "iASL and ACPICA aren't able to share the same grammar rules", but I'll take another shot at it.

Yes, the behavior of iASL versus ACPICA core is in fact different. Many, if not most, of these differences can be directly traced back to the fact that the ACPICA runtime (interpreter, etc.) must be compatible with "other ACPI implementations". However, the iASL compiler has no such restrictions, and we try to have iASL be as tight as possible in catching things that are actually incorrect.

The fact is that ACPICA runtime allows AML code to execute that the iASL compiler would flag as an error.

[mirh]

Yes, I know about the iASL and ACPICA different "purposes"
In the case of MLC though, the point (after the first Lv's comments) was shifted to iASL actually being supposed to support it (and indeed I think you even got to modify acpi spec itself to reflect this).

My assumption was that even #189 might have had something to do with this, but I guess you shouldn't/won't need this bug to remember that.

[acpibob]

Yes, the ACPI spec has been updated for the new grammar that supports module-level code underneath things like Devices, etc.

I don't know what the exact status of this whole issue is, so I'll have to let Lv respond.

What is the exact issue you are seeing with "grammar.asl"?

[acpibob]

I do believe hence, what I'm looking forward to is some of the remaining NameString work of #189

This has been completed and released. No changes to the test suite were required.

[mirh]

What is the exact issue you are seeing with "grammar.asl"?

I'm not even sure what those files are meant to represent, but I wanted to report information under Compilation should look like this: is not actual anymore.

This has been completed and released. No changes to the test suite were required.

• Parser: Avoid resolving in-package references

• Namespace: Add support to convert "Zero" into NULL object reference

• Utility: Add support to resolve external object references

I see no trace in current master of this.

[acpibob]

The changes for forward references from package objects has been completed and released.
As far as zero/Null references: I have a feeling that this is not done. The part where references are treated as namestrings will not be implemented internally, and the external interface remains the same.
Likewise, the "Utility" change will not be implemented, it is not required because of the way changes were implemented internally for forward references from packages.

[acpibob]

Here's the info from our release notes:

28 July 2017. Summary of changes for version 20170728:

AML interpreter: Implemented a new feature that allows forward references
from individual named references within package objects that are
contained within blocks of "module-level code". This provides
compatibility with other ACPI implementations and supports existing
firmware that depends on this feature. Example:

Name (ABCD, 1)
If (ABCD)                       /* An If() at module-level */
{
    Name (PKG1, Package()
    {
        INT1                    /* Forward reference to object INT1 

*/
})
Name (INT1, 0x1234)
}

[mirh]

Evaluate \_SB.PCI0.VGA.AF03 1 0 from acpiexec dsdt.aml should pretty much show you what I have been talking about.
Sorry for the delay, and I'm just posting here momentarily, because you seemed so sure it shouldn't be mlc, and I wouldn't want to clutter #963

Tables

[SchmErik]

How does windows evaluate this?

[mirh]

Ehrm.. I fear acpi debugging in windows (given acpica tools are to no avail and I have to use microsoft ones, right?) is a mess that will require me a day to gather the right OS debug builds.

[SchmErik]

We tried invoking the following on windows and it returned an error status.

DefinitionBlock ("", "DSDT", 2, "Intel", "_DSDT_01", 0x00000001)
 {
        Method (TEST){}

        Method (EXEC){
        Local2 = 0 /* \_SB_.PCI0.VGA_.AF03.SCDP */
        If (CondRefOf (test, Local4))
        {
            Local2 &= 0xFFFFFFFFFFFFFFFE
            Local2 |= Local4 //error
        }
    }
}

CondRefOf stores a reference to Local4 and the |= expression results in an error because or operations on references are not allowed. It is likely that this is a mistake and the CondRefOf needs to be fixed to something like this:

        If (Local4 = CondRefOf (test))...

[mirh]

Oh.. I see.
So it's a fault on the part of the linux driver (that makes that call) eventually, and not an acpica problem (which complains about "needed integer/string/buffer, found reference" - but still)

Without further ado then, I guess thanks and see you next time.

[SchmErik]

It's a firmware issue that is revealed by the Linux driver

[startergo]

iasl version 20200110 still has issue with:
"Invalid object type for reserved name (_PLD: found Buffer, Package required)"

 Name (_PLD, ToPLD (
                            PLD_Revision                                 = 0x1,

[SchmErik]

You want to encapsulate the ToPLD macro in a package like so:

Name (_PLD, Package(){ ToPLD(...)}

[startergo]

You want to encapsulate the ToPLD macro in a package like so:

Name (_PLD, Package(){ ToPLD(...)}

Thanks already did it, but I thought it was fixed.

[SchmErik]

Oh, sorry about the previous message. I wasn't reading enough. Are you using the same acpidump/acpi firmware?

[startergo]

Sorry same as what?

[mirh]

As mine.
It wasn't really the smartest move to continue this already long issue.

[startergo]

As mine.

It wasn't really the smartest move to continue this already long issue.

No I was referring to this:

Closing, as this seems to be resolved.

I don't think the bug was resolved that is why I replied

[SchmErik]

No I was referring to this:

Closing, as this seems to be resolved.

I don't think the bug was resolved that is why I replied

I see. Do you have firmware that exhibits this behavior? or is @mirh's the only one that we know of?

[startergo]

No I was referring to this:

Closing, as this seems to be resolved.

I don't think the bug was resolved that is why I replied

I see. Do you have firmware that exhibits this behavior? or is @mirh's the only one that we know of?

Yes I do.

bios_info: Gather BIOS DMI information.
--------------------------------------------------------------------------------
Test 1 of 1: Gather BIOS DMI information
BIOS Vendor       : American Megatrends Inc.
BIOS Version      : V1.10L16
BIOS Release Date : 07/25/2010
Board Name        : CF31L-1
Board Serial #    : None
Board Version     : 1
Board Asset Tag   : No Asset Tag
Chassis Serial #  : None
Chassis Type      : 10
Chassis Vendor    : Panasonic Corporation
Chassis Version   : 001
Chassic Asset Tag : No Asset Tag
Product Name      : CF-31GT2AA1M
Product Version   : 001
System Vendor     : Panasonic Corporation

[startergo]

Archive.zip
Attached are the DSDT/SSDT files

[acpibob]

This indeed fixes the issue:
Name (_PLD, Package() {ToPLD (
PLD_Revision = 0x1,
The reason that ToPLD returns a buffer (and not a package) is that multiple instances of ToPLD (and thus, multiple buffers) can be present in a single _PLD Package. Thus, iASL is compliant with the ACPI specification.