[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Bug 1297651] [NEW] KVM create a win7 guest with Qemu,
From: |
Hu, Robert |
Subject: |
Re: [Qemu-devel] [Bug 1297651] [NEW] KVM create a win7 guest with Qemu, it boots up fail |
Date: |
Thu, 27 Mar 2014 05:15:23 +0000 |
> -----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.
[Prev in Thread] |
Current Thread |
[Next in Thread] |