On Mon, 02 Dec 2013 23:09:55 +0100 Andreas Färber <afaer...@suse.de> 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 >