On Fri, May 30, 2014 at 09:50:33AM +0200, Igor Mammedov wrote: > On Wed, 14 May 2014 16:18:42 -0300 > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > If a given machine have max_cpus set, not just smp_cpus needs to be > > limited, but the total number of CPUs (considering CPU hotplug) for the > > machine. > > > > We also had yet another max_cpus limit check at smp_parse(), ensuring > > that max_cpus < MAX_CPUMASK_BITS. > > > > This patch unifies the machine->max_cpus and MAX_CPUMASK_BITS checks, > > and also moves the (smp_cpus <= max_cpus) outside smp_parse(), to keep > > all checks in the same place. > > > > With those changes, the new code ensures that: > > > > 1 <= smp_cpus <= max_cpus <= machine->max_cpus <= MAX_CPUMASK_BITS > > > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > > Cc: Peter Maydell <peter.mayd...@linaro.org> > > Cc: Andreas Färber <afaer...@suse.de> > > Cc: Igor Mammedov <imamm...@redhat.com> > > Cc: Marcelo Tosatti <mtosa...@redhat.com> > > --- > > Changes v1 -> v2: > > * v1 was: [PATCH] vl.c: Check max_cpus limit instead of smp_cpus > > * Unify machine->max_cpus and MAX_CPUMASK_BITS check > > * Move all checks outside smp_parse() > > * Add assert() lines ensuring the results are consistent > > * s/machine/machine_class/, after rebase to latest qemu.git > > (commit 6b342cc) > > --- > > vl.c | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 709d8cd..749abbd 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1425,15 +1425,6 @@ static void smp_parse(QemuOpts *opts) > > max_cpus = smp_cpus; > > } > > > > - if (max_cpus > MAX_CPUMASK_BITS) { > > - fprintf(stderr, "Unsupported number of maxcpus\n"); > > - exit(1); > > - } > > - if (max_cpus < smp_cpus) { > > - fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > > - exit(1); > > - } > > - > > } > > > > static void configure_realtime(QemuOpts *opts) > > @@ -4054,13 +4045,24 @@ int main(int argc, char **argv, char **envp) > > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > > > machine_class->max_cpus = machine_class->max_cpus ?: 1; /* Default to > > UP */ > > - if (smp_cpus > machine_class->max_cpus) { > > - fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max > > cpus " > > - "supported by machine `%s' (%d)\n", smp_cpus, > > + machine_class->max_cpus = MIN(machine_class->max_cpus, > > MAX_CPUMASK_BITS); > > + > > + if (max_cpus < smp_cpus) { > > + fprintf(stderr, "maxcpus must be equal to or greater than smp\n"); > > + exit(1); > > + } > > + if (max_cpus > machine_class->max_cpus) { > > + fprintf(stderr, "Total number of CPUs (%d), exceeds maximum " > > + "supported by machine `%s' (%d)\n", max_cpus, > > machine_class->name, machine_class->max_cpus); > > exit(1); > > } > > > > + assert(1 <= smp_cpus); > > + assert(smp_cpus <= max_cpus); > > + assert(max_cpus <= machine_class->max_cpus); > > + assert(machine_class->max_cpus <= MAX_CPUMASK_BITS); > It would be better complement some assertions with checks with proper error > reporting.
Absolutely. I was assuming all assert()s above have corresponding error checks/reporting. If they can be triggered in any way, that's a bug. > > also try to test following combos: > -smp 0,maxcpus=0 This gets translated to cpus=0,sockets=0,cores=0,threads=0. sockets,cores,threads become 1 when they are 0. cpus becomes sockets*cores*threads when it is zero. So, this gets translated to smp_cpus=1. That's an existing feature. Arguably confusing (and we may change it), but not a bug. > -smp -1 assert() is triggered, that's a bug. Thanks for finding it. (And that was the whole point of adding the assertions: to detect bugs.) > -smp 0,maxcpus=-1 $ ./install/bin/qemu-system-x86_64 0,maxcpus=0 -vnc :0 -S -monitor stdio -smp 0,maxcpus=-1 maxcpus must be equal to or greater than smp $ That's correct behavior. > -smp 1,maxcpus=0xFFFFFFFF $ ./install/bin/qemu-system-x86_64 0,maxcpus=0 -vnc :0 -S -monitor stdio -smp 1,maxcpus=0xFFFFFFFF maxcpus must be equal to or greater than smp $ The QemuOpts API can't (and never could) differentiate MAX_UINT from -1. Arguably a confusing feature (and we may try to change it), but not a bug. And the only way to change this behavior is by changing the QemuOpts API. > > we need to handle values as unsigned at least and maybe also ban 0 as valid > one. We translate "0" to "use the default". A confusing existing feature we may want to drop, but not a bug. > > The rest seems to work as expected. Thanks! -- Eduardo