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

[Laszlo Ersek <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

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

[-- Attachment #2: x.c --]
[-- Type: text/plain, Size: 1766 bytes --]

#define _XOPEN_SOURCE 500

#include <stdio.h>
#include <assert.h>

/* this is an inclusive limit on the number of VCPUs */
#define MAX_CPUS 255

/* @limit is exclusive */
static unsigned
width(unsigned limit)
{
    assert(limit != 0);
    if (limit ==   1) { return 0; }
    if (limit ==   2) { return 1; }
    if (limit <=   4) { return 2; }
    if (limit <=   8) { return 3; }
    if (limit <=  16) { return 4; }
    if (limit <=  32) { return 5; }
    if (limit <=  64) { return 6; }
    if (limit <= 128) { return 7; }
    assert(limit <= 256);
    return 8;
}

int
main(void)
{
    unsigned pkgs, cores, threads;

    for (        pkgs    = 1; pkgs                   <= MAX_CPUS; ++pkgs   ) {
        for (    cores   = 1; pkgs * cores           <= MAX_CPUS; ++cores  ) {
            for (threads = 1; pkgs * cores * threads <= MAX_CPUS; ++threads) {
                /* this is an actual APIC ID, not an exclusive limit */
                unsigned max_apic_id;

                /* we limit the width of APIC IDs in 8 bits */
                if (width(pkgs) + width(cores) + width(threads) > 8) {
                    continue;
                }

                max_apic_id =                                    pkgs    - 1;
                max_apic_id = (max_apic_id << width(cores)  ) | (cores   - 1);
                max_apic_id = (max_apic_id << width(threads)) | (threads - 1);

                assert(max_apic_id < 256);
                if (max_apic_id == 255) {
                    fprintf(stdout, "(%3u, %3u, %3u): "
                            "cpus=%3u max_apic_id=%3u\n",
                            pkgs, cores, threads,
                            pkgs * cores * threads, max_apic_id);
                }
            }
        }
    }
    return 0;
}