From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WSqhW-0000jm-QN for qemu-devel@nongnu.org; Wed, 26 Mar 2014 12:29:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WSqhQ-0002GX-PS for qemu-devel@nongnu.org; Wed, 26 Mar 2014 12:29:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1440) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WSqhQ-0002GM-Hx for qemu-devel@nongnu.org; Wed, 26 Mar 2014 12:29:08 -0400 Date: Wed, 26 Mar 2014 18:29:28 +0200 From: "Michael S. Tsirkin" Message-ID: <20140326162928.GA32190@redhat.com> References: <20140326064510.5518.72436.malonedeb@chaenomeles.canonical.com> <20140326064510.5518.72436.malonedeb@chaenomeles.canonical.com> <20140326103112.GA20219@redhat.com> <5332C7D2.3030901@redhat.com> <20140326125828.GA2192@redhat.com> <20140326144829.52f72b74@nial.usersys.redhat.com> <5332F7D3.7000209@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5332F7D3.7000209@redhat.com> 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: Laszlo Ersek Cc: Igor Mammedov , Bug 1297651 <1297651@bugs.launchpad.net>, qemu-devel@nongnu.org, ehabkost@redhat.com, robert.hu@intel.com On Wed, Mar 26, 2014 at 04:52:51PM +0100, Laszlo Ersek wrote: > On 03/26/14 14:48, Igor Mammedov wrote: > > On Wed, 26 Mar 2014 14:58:28 +0200 > > "Michael S. Tsirkin" wrote: > > >> If we want to change ACPI rev, I think we should do this > >> conditionally when max_cpus > 255. > >> Would be worth it if this fixes some guests. > >> > >> As for reverting, I think it's a problem that we seem to > >> allow max_cpus = 256 but then it doesn't really work. > > > more clean would be to abort if CPON index (i.e. APIC ID) > > is more than 255. That would affect small number of weird > > topologies but sould be fine for most usecases. > > The question is not about a CPON index / APIC ID that *exceeds* 255. > > Eduardo's patches already make sure that the APIC ID *width* is at most > 8 bits, so the highest APIC ID that any VCPU can take is already at most > 255. (IOW the exclusive limit for APIC IDs is already 256.) In other > words, the CPON index already can't exceed 255. > > The question is about the CPON index / APIC ID that is *precisely* 255. > Eduardo's patches allow this (correctly), but the SSDT generator used > *not* to create a CPON array element for the index 255. The generator > limited the CPON element *count* in 255, hence the maximum CPON index > (== APIC ID) that was available for hotplugging used to be 254. > > My patch wanted to bump this CPON size one higher (to 256), so that the > VCPU with APIC ID 255 (== CPON array index 255) becomes hotpluggable. > > However. > > Given that > (a) for PC, we limit the *number* of VCPUs in 255, inclusive (ie. max > vcpu *index* is 254), and > (b) Eduardo's patches (correctly) restrict the accepted topologies so > that any APIC ID fits into 8 bits, > > it turns out that there simply isn't a (VCPU count, topology) pair, > accepted by (a) and (b) simultaneously, that would enable a VCPU APIC ID > of 255. The attached program prints nothing. > > (Note that as soon as you break (a), ie. increase MAX_CPUS to 256 in the > attached program, you immediately get a bunch of topologies where APIC > ID (CPON index) 255 becomes possible, while keeping (b) intact.) > > Hence my patches fix a case that is purely academical (never happens in > practice) as long as (a) and (b) are guaranteed *together*. > > I should have done more research before posting my patches. > > Thanks (and sorry about the churn), > Laszlo More importantly, I should have clarified which command line is fixed by the patch before merging for 2.0. Anyway, I think the way it is ATM is reasonable even if we could in practice drop a bunch of code (or replace with asserts). Any more changes just seem to add risk. We should probably revisit this for 2.1. -- MST