From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WSmwJ-0001xg-IR for qemu-devel@nongnu.org; Wed, 26 Mar 2014 08:28:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WSmwD-0002sf-HA for qemu-devel@nongnu.org; Wed, 26 Mar 2014 08:28:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45623) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WSmwD-0002sP-A5 for qemu-devel@nongnu.org; Wed, 26 Mar 2014 08:28:09 -0400 Message-ID: <5332C7D2.3030901@redhat.com> Date: Wed, 26 Mar 2014 13:28:02 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <20140326064510.5518.72436.malonedeb@chaenomeles.canonical.com> <20140326064510.5518.72436.malonedeb@chaenomeles.canonical.com> <20140326103112.GA20219@redhat.com> In-Reply-To: <20140326103112.GA20219@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Bug 1297651] [NEW] KVM create a win7 guest with Qemu, it boots up fail List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Bug 1297651 <1297651@bugs.launchpad.net> Cc: robert.hu@intel.com, qemu-devel@nongnu.org, ehabkost@redhat.com On 03/26/14 11:31, Michael S. Tsirkin wrote: > On Wed, Mar 26, 2014 at 06:45:10AM -0000, Robert Hu wrote: >> 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 I= Ds. >> (APIC IDs are in fact discontiguous, but this is the traditional >> interface: build a contiguous sequence from zero up that covers al= l >> 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 V= CPU. >> >> The build_ssdt() function currently limits the *count* of processo= r >> objects, and NTFY branches, and CPON elements, in 0xFF (see the as= signment >> 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 a= llow a >> VCPU to take is 255. >> >> In order to extend the maximum count to 256, and the traversed API= C 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 :=3D PackageOp PkgLength NumElements PackageE= lementList >> DefVarPackage :=3D VarPackageOp PkgLength VarNumElements PackageE= lementList >> >> PackageOp :=3D 0x12 >> VarPackageOp :=3D 0x13 >=20 >=20 > I think I know what's going on here: the specification says: >=20 > 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 >=20 > opcode is compatible with ACPI 1.0. A VarPackageOp will be emitted if > any of the following conditions are true: > =E2=80=A2 > The NumElements argument is a TermArg that can only be resolved at > runtime. > =E2=80=A2 > At compile time, NumElements resolves to a constant that is larger tha= n > 255. > =E2=80=A2 > The PackageList contains more than 255 initializer elements. >=20 >=20 > So we clearly violate this rule. I did see this passage of the spec, but it is not relevant. It is about w= hat the ASL compiler does. It comes from: 19 ACPI Source Language (ASL)Reference 19.5 ASL Operator Reference 19.5.98 Package (Declare Package Object) We do not have an ASL compiler at hand. The specification nowhere restric= ts VarPackageOp to > 255. However, what I *did* mess up is compatibility with ACPI 1.0. Just below = the quoted part, there's also this: 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. I forgot that the header of the containing table stated the ACPI version: /* Copy header and patch values in the S3_ / S4_ / S5_ packages */ ssdt_ptr =3D acpi_data_push(table_data, sizeof(ssdp_misc_aml)); and DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", = 0x1) ^^^^ ComplianceRevision So my patch generates ACPI 2.0+ contents for an 1.0 table. > The following seems to fix the issue - still testing. Can you confirm p= lease? This patch only restricts the bug to a subset of cases, but it doesn't fi= x it. > 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. >=20 > How about limiting ACPI_CPU_HOTPLUG_ID_LIMIT to 255? I think it's not the package size / APIC ID value per se that breaks the = boot, but the incompatibility between the ACPI revision stated in the SSD= T header, and the construct in the table. >=20 > We never allowed > 255 in the past, is it worth the > maintainance headaches? >=20 > =20 > 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, > =20 > { > GArray *package =3D build_alloc_array(); > - uint8_t op =3D 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 u= sed. > + */ > + if (acpi_cpus <=3D 255) { > + op =3D 0x12; /* PackageOp */ > + build_append_byte(package, acpi_cpus); /* NumElements = */ > + } else { > + op =3D 0x13; /* VarPackageOp */ > + build_append_int(package, acpi_cpus); /* VarNumElement= s */ > + } > =20 > - build_append_int(package, acpi_cpus); /* VarNumElements */ > for (i =3D 0; i < acpi_cpus; i++) { > uint8_t b =3D test_bit(i, cpu->found_cpus) ? 0x01 : 0x= 00; > build_append_byte(package, b); >=20 The patch will mask the problem for most of the cases, but when VarPackag= eOp is used, it will be broken just the same (because the ACPI revision i= n the SSDT header will still mismatch). Here's my proposal: - I can post a patch that changes the SSDT DSL files, *and* the DSDT file= s (q35-acpi-dsdt.dsl acpi-dsdt.dsl), to state an ACPI revision of 2.0. (T= he ACPI revision of the DSDT file determines integer sizes for SSDTs too,= so we can't just bump the SSDTs to 2.0) - Or I suggest to revert my patches for 2.0. You probably won't like bumping the ACPI rev in DSDT/SSDT, for various co= mpatibility reasons, so I think I suggest to revert these two patches of = mine. It's now clear to me that this VCPU hotplug limit cannot be lifted = without much more intrusive (and guest visible) changes. Sorry about miss= ing this fact in my original submission. Thanks, Laszlo