Re: [Qemu-devel] [Qemu-trivial] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Igor Mammedov]

On Mon, 02 Dec 2013 23:09:55 +0100
Andreas Färber <address@hidden> wrote:

> Am 02.12.2013 18:06, schrieb Michael Tokarev:
> > 25.11.2013 07:39, Alexey Kardashevskiy wrote:
> >> Since modern POWER7/POWER8 chips can have more that 256 CPU threads
> >> (>2000 actually), remove this check from smp_parse.
> >>
> >> The CPUs number is still checked against machine->max_cpus and this check
> >> should be enough not to break other archs.
> 
> "should be" is not exactly the highest level of confidence for a
> "trivial" patch... :/
> 
> > []
> >> -    if (max_cpus > 255) {
> >> -        fprintf(stderr, "Unsupported number of maxcpus\n");
> >> -        exit(1);
> >> -    }
> 
> I believe Eduardo touched that code last for NUMA, so let's CC him.
> 
> > I don't know whenever this is actually safe.  Do we have any static arrays
> > of size 255 somewhere, which will be overflowed without this check? :)
> 
> s390 has the ipi_states[] array, but not fixed to that size.
> 
> x86 APIC IDs I think have or had a limitation to 255 rather than 16-bit?
> Igor?
it's still fixed size array:
hw/intc/apic.c: static APICCommonState *local_apics[MAX_APICS + 1];
with: #define MAX_APICS 255

> 
> Alexey, did you actually check that, e.g., x86 machines don't break with
> 256 or 257 CPUs now?
patch shouldn't hurt x86 machines since
there is check later 
vl.c:
    if (smp_cpus > machine->max_cpus) {
           fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus 
"
                "supported by machine `%s' (%d)\n", smp_cpus,  machine->name,
                machine->max_cpus);
        exit(1);
    }
       
and both piix4/q35 machines have machine->max_cpus initialized to 255 or 
lower(isa/xen).

> 
> Adding a qtest would be one way to prove that at least the QEMU code of
> all other architectures doesn't break with the ppc change.
> 
> Regards,
> Andreas
>