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

iasl from acpica >= 20190329 miscompiles DSDT table of VirtualBox #462

Closed
yan12125 opened this issue Apr 19, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@yan12125
Copy link

commented Apr 19, 2019

Background

On Arch Linux, virtualbox 6.0.6 breaks lots of Windows 7 virtual machines. They crash with BSOD and the following error message:

The BIOS in this system is not fully ACPI compliant

After investigation, virtualbox building with acpica 20190215 works fine, but with acpica 20190329 or 20190405, built virtualbox is broken.

Relevant discussion threads:

Steps to reproduce the issue

  1. Get VirtualBox sources from https://download.virtualbox.org/virtualbox/6.0.6/VirtualBox-6.0.6.tar.bz2
  2. iasl VirtualBox-6.0.6/src/VBox/Devices/PC/vbox.dsl
  3. iasl -d VirtualBox-6.0.6/src/VBox/Devices/PC/DSDT.aml

In disassembled VirtualBox-6.0.6/src/VBox/Devices/PC/DSDT.dsl, there's a suspicious line

Checksum 0x00 **** Incorrect checksum, should be 0xDD

There are more differences between DSDT tables from old and new acpica:

--- src/VBox/Devices/PC/DSDT-20190215.dsl	2019-04-19 16:25:54.831203690 +0800
+++ src/VBox/Devices/PC/DSDT-20190329.dsl	2019-04-19 16:15:12.088972098 +0800
@@ -1,22 +1,22 @@
 /*
  * Intel ACPI Component Architecture
- * AML/ASL+ Disassembler version 20190215 (64-bit version)
+ * AML/ASL+ Disassembler version 20190329 (64-bit version)
  * Copyright (c) 2000 - 2019 Intel Corporation
  * 
  * Disassembling to symbolic ASL+ operators
  *
- * Disassembly of src/VBox/Devices/PC/DSDT.aml, Fri Apr 19 16:25:54 2019
+ * Disassembly of src/VBox/Devices/PC/DSDT.aml, Fri Apr 19 16:15:12 2019
  *
  * Original Table Header:
  *     Signature        "DSDT"
- *     Length           0x000022EA (8938)
+ *     Length           0x000022F8 (8952)
  *     Revision         0x02
- *     Checksum         0xFF
+ *     Checksum         0x00 **** Incorrect checksum, should be 0xDD
  *     OEM ID           "VBOX  "
  *     OEM Table ID     "VBOXBIOS"
  *     OEM Revision     0x00000002 (2)
  *     Compiler ID      "INTL"
- *     Compiler Version 0x20190215 (538509845)
+ *     Compiler Version 0x20190329 (538510121)
  */
 DefinitionBlock ("", "DSDT", 2, "VBOX  ", "VBOXBIOS", 0x00000002)
 {
@@ -3353,20 +3353,20 @@
                 PIRD = LSRS (Arg0)
             }
         }
-    }
 
-    Name (_S0, Package (0x02)  // _S0_: S0 System State
-    {
-        Zero, 
-        Zero
-    })
-    If ((PWRS & 0x02))
-    {
-        Name (_S1, Package (0x02)  // _S1_: S1 System State
+        Name (_S0, Package (0x02)  // _S0_: S0 System State
         {
-            One, 
-            One
+            Zero, 
+            Zero
         })
+        If ((PWRS & 0x02))
+        {
+            Name (_S1, Package (0x02)  // _S1_: S1 System State
+            {
+                One, 
+                One
+            })
+        }
     }
 
     If ((PWRS & 0x10))
@@ -3388,5 +3388,20 @@
         DBG ("Prepare to sleep: ")
         HEX (Arg0)
     }
+
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
+    Zero
 }

And with acpica 20190405, disassembling fails.

@SchmErik

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Thanks for your report. We are working on a fix

@lwfinger

This comment has been minimized.

Copy link

commented Apr 22, 2019

The problem also happens with openSUSE Tumbleweed, which supplies acpica 20190405.

@SchmErik

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Hi,
This seems to be a bug in the compiler.

Please try the following patch:

diff --git a/source/compiler/aslcodegen.c b/source/compiler/aslcodegen.c
index 88e348242..8ec085275 100644
--- a/source/compiler/aslcodegen.c
+++ b/source/compiler/aslcodegen.c
@@ -690,7 +690,10 @@ CgUpdateHeader (
     {
         if (FlReadFile (ASL_FILE_AML_OUTPUT, &FileByte, 1) != AE_OK)
         {
-            printf ("EOF while reading checksum bytes\n");
+            printf (
+                "EOF while reading checksum bytes, Index 0x%X "
+                "Length 0x%X StartOffset 0x%X SubTreeLen 0x%X\n",
+                i, Length, Op->Asl.FinalAmlOffset, Op->Asl.AmlSubtreeLength);
             return;
         }
 
diff --git a/source/compiler/aslcompiler.h b/source/compiler/aslcompiler.h
index ad0e73f3c..2cf44dd10 100644
--- a/source/compiler/aslcompiler.h
+++ b/source/compiler/aslcompiler.h
@@ -715,7 +715,7 @@ OpnDoPackage (
 void
 OptOptimizeNamePath (
     ACPI_PARSE_OBJECT       *Op,
-    UINT32                  Flags,
+    const ACPI_OPCODE_INFO  *OpInfo,
     ACPI_WALK_STATE         *WalkState,
     char                    *AmlNameString,
     ACPI_NAMESPACE_NODE     *TargetNode);
diff --git a/source/compiler/aslload.c b/source/compiler/aslload.c
index a055bab06..b5d3b1bcb 100644
--- a/source/compiler/aslload.c
+++ b/source/compiler/aslload.c
@@ -493,7 +493,7 @@ LdNamespace1Begin (
     case AML_FIELD_OP:
 
         Status = LdLoadFieldElements (Op, WalkState);
-        break;
+        return (Status);
 
     case AML_INT_CONNECTION_OP:
 
@@ -557,8 +557,7 @@ LdNamespace1Begin (
          * We only want references to named objects:
          *      Store (2, WXYZ) -> Attempt to resolve the name
          */
-        if ((OpInfo->Class == AML_CLASS_NAMED_OBJECT) &&
-            (OpInfo->Type != AML_TYPE_NAMED_FIELD))
+        if (OpInfo->Class == AML_CLASS_NAMED_OBJECT)
         {
             return (AE_OK);
         }
diff --git a/source/compiler/aslopt.c b/source/compiler/aslopt.c
index 6d2045f82..996a9d1c1 100644
--- a/source/compiler/aslopt.c
+++ b/source/compiler/aslopt.c
@@ -641,12 +641,12 @@ OptOptimizeNameDeclaration (
  * FUNCTION:    OptOptimizeNamePath
  *
  * PARAMETERS:  Op                  - Current parser op
- *              Flags               - Opcode info flags
+ *              OpInfo;             - Info for this AML opcode
  *              WalkState           - Current state
  *              AmlNameString       - Unoptimized namepath
  *              TargetNode          - Node to which AmlNameString refers
  *
- * RETURN:      None. If path is optimized, the Op is updated with new path
+ * RETURN:      If path is optimized here, the Op is updated with new path
  *
  * DESCRIPTION: Optimize a Named Declaration or Reference to the minimal length.
  *              Must take into account both the current location in the
@@ -657,7 +657,7 @@ OptOptimizeNameDeclaration (
 void
 OptOptimizeNamePath (
     ACPI_PARSE_OBJECT       *Op,
-    UINT32                  Flags,
+    const ACPI_OPCODE_INFO  *OpInfo,
     ACPI_WALK_STATE         *WalkState,
     char                    *AmlNameString,
     ACPI_NAMESPACE_NODE     *TargetNode)
@@ -696,7 +696,8 @@ OptOptimizeNamePath (
         AcpiPsGetOpcodeName (Op->Common.Parent->Common.AmlOpcode),
         AcpiPsGetOpcodeName (Op->Common.AmlOpcode)));
 
-    if (!(Flags & (AML_NAMED | AML_CREATE)))
+    if (!(OpInfo->Flags & (AML_NAMED | AML_CREATE)))// ||
+        //(OpInfo->Type == AML_TYPE_NAMED_FIELD))
     {
         if (Op->Asl.CompileFlags & OP_IS_NAME_DECLARATION)
         {
@@ -731,7 +732,7 @@ OptOptimizeNamePath (
         CurrentNode = WalkState->ScopeInfo->Scope.Node;
     }
 
-    if (Flags & (AML_NAMED | AML_CREATE))
+    if (OpInfo->Flags & (AML_NAMED | AML_CREATE))
     {
         /* This is the declaration of a new name */
 
@@ -816,7 +817,7 @@ OptOptimizeNamePath (
     /*
      * Attempt an optimization depending on the type of namepath
      */
-    if (Flags & (AML_NAMED | AML_CREATE))
+    if (OpInfo->Flags & (AML_NAMED | AML_CREATE))
     {
         /*
          * This is a named opcode and the namepath is a name declaration, not
@@ -870,7 +871,7 @@ OptOptimizeNamePath (
             " REDUCED BY %2u (TOTAL SAVED %2u)",
             (UINT32) HowMuchShorter, OptTotal));
 
-        if (Flags & AML_NAMED)
+        if (OpInfo->Flags & AML_NAMED)
         {
             if (Op->Asl.AmlOpcode == AML_ALIAS_OP)
             {
@@ -887,7 +888,7 @@ OptOptimizeNamePath (
                 Op->Asl.Child->Asl.AmlLength = strlen (NewPath);
             }
         }
-        else if (Flags & AML_CREATE)
+        else if (OpInfo->Flags & AML_CREATE)
         {
             /* Name must appear as the last parameter */
 
@@ -896,6 +897,7 @@ OptOptimizeNamePath (
             {
                 NextOp = NextOp->Asl.Next;
             }
+
             /* Update the parse node with the new NamePath */
 
             NextOp->Asl.Value.String = NewPath;
diff --git a/source/compiler/aslxref.c b/source/compiler/aslxref.c
index 9d6e5c0bc..23777e6d9 100644
--- a/source/compiler/aslxref.c
+++ b/source/compiler/aslxref.c
@@ -642,15 +642,16 @@ XfNamespaceLocateBegin (
         (OpInfo->Type == AML_TYPE_NAMED_FIELD))
     {
         /*
-         * These are name references, do not push the scope stack
-         * for them.
+         * These names are references to other objects.
+         * Do not push the scope stack for them.
          */
         Flags |= ACPI_NS_DONT_OPEN_SCOPE;
     }
 
     /* Get the NamePath from the appropriate place */
 
-    if (OpInfo->Flags & AML_NAMED)
+    if ((OpInfo->Flags & AML_NAMED) ||
+        (OpInfo->Type == AML_TYPE_NAMED_FIELD))
     {
         /* For nearly all NAMED operators, the name reference is the first child */
 
@@ -676,10 +677,6 @@ XfNamespaceLocateBegin (
 
         Path = NextOp->Asl.Value.String;
     }
-    else if (OpInfo->Type == AML_TYPE_NAMED_FIELD)
-    {
-        Path = Op->Asl.Child->Asl.Value.String;
-    }
     else
     {
         Path = Op->Asl.Value.String;
@@ -749,9 +746,10 @@ XfNamespaceLocateBegin (
                     AslError (ASL_ERROR, ASL_MSG_PREFIX_NOT_EXIST, NextOp,
                         NextOp->Asl.ExternalName);
                 }
-                else if (OpInfo->Flags & AML_NAMED)
+                else if ((OpInfo->Flags & AML_NAMED) ||
+                    (OpInfo->Type == AML_TYPE_NAMED_FIELD))
                 {
-                    /* The new name is the first parameter */
+                    /* The reference object name is the first parameter */
 
                     AslError (ASL_ERROR, ASL_MSG_PREFIX_NOT_EXIST, Op,
                         Op->Asl.ExternalName);
@@ -828,9 +826,15 @@ XfNamespaceLocateBegin (
         Node->Flags |= ANOBJ_IS_REFERENCED;
     }
 
+    if (OpInfo->Type == AML_TYPE_NAMED_FIELD)
+    {
+// TBD: why is this needed? removal cause EOF on large DSDTs
+        Op->Asl.CompileFlags |= OP_IS_NAME_DECLARATION;
+    }
+
     /* Attempt to optimize the NamePath */
 
-    OptOptimizeNamePath (Op, OpInfo->Flags, WalkState, Path, Node);
+    OptOptimizeNamePath (Op, OpInfo, WalkState, Path, Node);
 
     /*
      * 1) Dereference an alias (A name reference that is an alias)
@@ -1068,6 +1072,7 @@ XfNamespaceLocateBegin (
             ((Op->Asl.Parent->Asl.ParseOpcode == PARSEOP_FIELD)     ||
              (Op->Asl.Parent->Asl.ParseOpcode == PARSEOP_BANKFIELD)))
     {
+#if 1
         /*
          * Offset checking for fields. If the parent operation region has a
          * constant length (known at compile time), we can check fields
@@ -1137,8 +1142,18 @@ XfNamespaceLocateBegin (
             case ACPI_ADR_SPACE_CMOS:
             case ACPI_ADR_SPACE_GPIO:
 
+#if 0
                 if ((UINT8) Op->Asl.Parent->Asl.Value.Integer !=
                     AML_FIELD_ACCESS_BYTE)
+#endif
+                /*
+                 * Op is the Field->RegionName
+                 * Op->Asl.Next is the Field->AccessWidth
+                 */
+#if 1
+                if ((UINT8) Op->Asl.Next->Asl.Value.Integer !=
+                    AML_FIELD_ACCESS_BYTE)
+#endif
                 {
                     AslError (ASL_ERROR, ASL_MSG_REGION_BYTE_ACCESS, Op, NULL);
                 }
@@ -1183,6 +1198,7 @@ XfNamespaceLocateBegin (
                     Op->Asl.Child->Asl.ExtraValue);
             }
         }
+#endif
     }
 
     /*
@lwfinger

This comment has been minimized.

Copy link

commented Apr 22, 2019

With that patch, I too can compile and decompile the VirtualBox BIOS. Unfortunately, I have not found a way to build the VB package with that version for testing.

@SchmErik

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

can you build and isntall iASL on your path (using sudo make install) and then go through the vbox build scripts?

This patch produces the same exact AML bytecode as the 20190215 release aside from header information that is non-functional.

@lwfinger

This comment has been minimized.

Copy link

commented Apr 22, 2019

It was a bit more complicated, but I have now built VBox with the patched acpica package. A Windows 7 VM now works!! Thanks.

@yan12125

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

@SchmErik: I can also confirm virtualbox built with acpica 20190405 + your patch works fine on Arch Linux. Thank you very much for the effort!

@yan12125

This comment has been minimized.

Copy link
Author

commented May 10, 2019

virtualbox 6.0.6 built with acpica 20190509 works fine with my Windows 7 virtual machine, and iasl -d does not report incorrect checksums. I think this issue has been resolved and can be closed?

@SchmErik

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Yes, we can close this. Thanks for the report!

@SchmErik SchmErik closed this May 10, 2019

congdanhqx added a commit to congdanhqx/void-packages that referenced this issue May 11, 2019

acpica-utils: update to 20190509.
Also rebuild virtualbox and xen due to this bug:
acpica/acpica#462

congdanhqx added a commit to congdanhqx/void-packages that referenced this issue May 11, 2019

congdanhqx added a commit to congdanhqx/void-packages that referenced this issue May 11, 2019

xen: rebuild after acpica change
[skip ci]
Possibly effected by: acpica/acpica#462

pullmoll added a commit to void-linux/void-packages that referenced this issue May 11, 2019

pullmoll added a commit to void-linux/void-packages that referenced this issue May 11, 2019

xen: rebuild after acpica change
[skip ci]
Possibly effected by: acpica/acpica#462

Closes: #11646 [via git-merge-pr]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.