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

[Eduardo Habkost]

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 <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).

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