On Tue, Dec 03, 2013 at 11:44:19AM +0100, Igor Mammedov wrote:
> 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).
This ensures smp_cpus <= machine->max_cpus, but doesn't ensure
max_cpus <= machine->max_cpus. We need to check max_cpus against
machine->max_cpus, too, then we can eliminate the hardcoded
max_cpus > 255 check.
>
> >
> > 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
> >
>
--
Eduardo
