Re: [Qemu-devel] [Bug 1297651] [NEW] KVM create a win7 guest with Qemu, it boots up fail

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Mar 26, 2014 at 06:45:10AM -0000, Robert Hu wrote:
> Public bug reported:
> 
> Environment:
> ------------
> Host OS (ia32/ia32e/IA64):ia32e
> Guest OS (ia32/ia32e/IA64):ia32e
> Guest OS Type (Linux/Windows):Windows
> kvm.git Commit:94b3ffcd41a90d2cb0b32ca23aa58a01111d5dc0
> qemu-kvm Commit:839a5547574e57cce62f49bfc50fe1f04b00589a
> Host Kernel Version:3.14.0-rc3
> Hardware:Romley_EP, Ivytown_EP, HSW_EP
> 
> 
> Bug detailed description:
> --------------------------
> when create a win7 guest, the guest boot up fail.
> 
> note: 
> 1. when create win2000, winxp, win2k3, win2k8, guest, the guest boot up fail.
> 2. when create win8, win8.1, win2012 guest, the guest boot up fine.
> 
> 
> Reproduce steps:
> ----------------
> 1.create guest
> qemu-system-x86_64 -enable-kvm -m 1024 -smp 2 -net none -hda /root/win7.qcow
> 
> 
> Current result:
> ----------------
> win7 guest boot up fail
> 
> Expected result:
> ----------------
> win7 guest boot up fine
> 
> Basic root-causing log:
> ----------------------
> 
> This should be a qemu bug
> kvm      + qemu     =  result
> 94b3ffcd + 839a5547 = bad
> 94b3ffcd + 3a87f8b6 = good
> 
> the first bad commit is:
> commit 9bcc80cd71892df42605e0c097d85c0237ff45d1
> Author: Laszlo Ersek <lersek@redhat.com>

Thanks for the excellent bug report!



> Date:   Mon Mar 17 17:05:16 2014 +0100
> 
>     i386/acpi-build: allow more than 255 elements in CPON
> 
>     The build_ssdt() function builds a number of AML objects that are related
>     to CPU hotplug, and whose IDs form a contiguous sequence of APIC IDs.
>     (APIC IDs are in fact discontiguous, but this is the traditional
>     interface: build a contiguous sequence from zero up that covers all
>     possible APIC IDs.) These objects are:
> 
>     - a Processor() object for each VCPU,
>     - a NTFY method, with one branch for each VCPU,
>     - a CPON package with one element (hotplug status byte) for each VCPU.
> 
>     The build_ssdt() function currently limits the *count* of processor
>     objects, and NTFY branches, and CPON elements, in 0xFF (see the assignment
>     to "acpi_cpus"). This allows for an inclusive APIC ID range of [0..254].
>     This is incorrect, because the highest APIC ID that we otherwise allow a
>     VCPU to take is 255.
> 
>     In order to extend the maximum count to 256, and the traversed APIC ID
>     range correspondingly to [0..255]:
>     - the Processor() objects need no change,
>     - the NTFY method also needs no change,
>     - the CPON package must be updated, because it is defined with a
>       DefPackage, and the number of elements in such a package can be at most
>       255. We pick a DefVarPackage instead.
> 
>     We replace the Op byte, and the encoding of the number of elements.
>     Compare:
> 
>     DefPackage     := PackageOp    PkgLength NumElements    PackageElementList
>     DefVarPackage  := VarPackageOp PkgLength VarNumElements PackageElementList
> 
>     PackageOp      := 0x12
>     VarPackageOp   := 0x13


I think I know what's going on here: the specification says:

The ASL compiler can emit two different AML opcodes for a Package
declaration, either PackageOp or VarPackageOp. For small, fixed-length
packages, the PackageOp is used and this

opcode is compatible with ACPI 1.0. A VarPackageOp will be emitted if
any of the following conditions are true:
•
 The NumElements argument is a TermArg that can only be resolved at
runtime.
•
 At compile time, NumElements resolves to a constant that is larger than
255.
•
 The PackageList contains more than 255 initializer elements.


So we clearly violate this rule.




>     NumElements    := ByteData
>     VarNumElements := TermArg => Integer
> 
>     The build_append_int() function implements precisely the following TermArg
>     encodings (a subset of what the ACPI spec describes):
> 
>       TermArg             := DataObject
>       DataObject          := ComputationalData
>       ComputationalData   := ConstObj | ByteConst | WordConst | DWordConst
>       directly encoded in the function, with build_append_byte():
>         ConstObj          := ZeroOp | OneOp
>           ZeroOp          := 0x00
>           OneOp           := 0x01
> 
>       call to build_append_value(..., 1):
>         ByteConst         := BytePrefix ByteData
>           BytePrefix      := 0x0A
>           ByteData        := 0x00 - 0xFF
> 
>       call to build_append_value(..., 2):
>         WordConst         := WordPrefix WordData
>           WordPrefix      := 0x0B
>           WordData        := ByteData[0:7] ByteData[8:15]
> 
>       call to build_append_value(..., 4):
>         DWordConst        := DWordPrefix DWordData
>           DWordPrefix     := 0x0C
>           DWordData       := WordData[0:15] WordData[16:31]
> 
>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ** Affects: qemu
>      Importance: Undecided
>          Status: New
>

The following seems to fix the issue - still testing. Can you confirm please?
However the question we should ask is whether
it's a good idea to allow hotplug ID values that might
make guests fail to boot.

How about limiting ACPI_CPU_HOTPLUG_ID_LIMIT to 255?

We never allowed > 255 in the past, is it worth the
maintainance headaches?

 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f1054dd..7597517 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1055,9 +1055,21 @@ build_ssdt(GArray *table_data, GArray *linker,
 
         {
             GArray *package = build_alloc_array();
-            uint8_t op = 0x13; /* VarPackageOp */
+            uint8_t op;
+
+            /*
+             * Note: The ability to create variable-sized packages was first introduced in ACPI 2.0. ACPI 1.0 only
+             * allowed fixed-size packages with up to 255 elements.
+             * Windows guests up to win2k8 fail when VarPackageOp is used.
+             */
+            if (acpi_cpus <= 255) {
+                op = 0x12; /* PackageOp */
+                build_append_byte(package, acpi_cpus); /* NumElements */
+            } else {
+                op = 0x13; /* VarPackageOp */
+                build_append_int(package, acpi_cpus); /* VarNumElements */
+            }
 
-            build_append_int(package, acpi_cpus); /* VarNumElements */
             for (i = 0; i < acpi_cpus; i++) {
                 uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
                 build_append_byte(package, b);