Re: [Qemu-devel] [PATCH v2 RESEND] vl.c: Unify MAX_CPUMASK_BITS and machine->max_cpus checks

[Igor Mammedov]

On Wed, 14 May 2014 16:18:42 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Cc: Andreas Färber <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: Marcelo Tosatti <address@hidden>
> ---
> 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.

also try to test following combos:
-smp 0,maxcpus=0
-smp -1
-smp 0,maxcpus=-1
-smp 1,maxcpus=0xFFFFFFFF

we need to handle values as unsigned at least and maybe also ban 0 as valid
one.

The rest seems to work as expected.

> +
>      /*
>       * Get the default machine options from the machine if it is not already
>       * specified either by the configuration file or by the command line.
> -- 
> 1.9.0
> 


-- 
Regards,
  Igor