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 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" <mst@redhat.com> 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