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

[Hu, Robert]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Wednesday, March 26, 2014 6:31 PM
> To: Bug 1297651
> Cc: address@hidden; address@hidden; address@hidden; Hu,
> Robert
> Subject: Re: [Bug 1297651] [NEW] KVM create a win7 guest with Qemu, it boots
> up fail
> 
> 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 <address@hidden>
> 
> 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 <address@hidden>
> >     Reviewed-by: Michael S. Tsirkin <address@hidden>
> >     Signed-off-by: Michael S. Tsirkin <address@hidden>
> >
> > ** 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);
Patch to qemu(839a5547574e57), guest can boot fine.