[Qemu-devel] [PATCH v2 0/3] vl: smp_parse sanity checks

[Qemu-devel] [PATCH v2 0/3] vl: smp_parse sanity checks

[Eduardo Habkost]

>From the original series sent by Andrew, only patch 1/3 was kept. Patch 2/3
from v1 was replaced by a safer fix. Patch 3/3 was removed because it generates
a warning even on the most common use-case ("-smp <n>" without any extra
options).

Andrew Jones (1):
  vl: fix max_cpus check

Eduardo Habkost (2):
  vl: Avoid unnecessary 'if' nesting
  vl: Don't silently change topology when all -smp options were set

 vl.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/3] vl: Avoid unnecessary 'if' nesting

[Eduardo Habkost]

Just a coding style change, to make other changes easier to review.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index f4a6e5e..c1bfc9b 100644
--- a/vl.c
+++ b/vl.c
@@ -1284,13 +1284,11 @@ static void smp_parse(QemuOpts *opts)
             if (cpus == 0) {
                 cpus = cores * threads * sockets;
             }
+        } else if (cores == 0) {
+            threads = threads > 0 ? threads : 1;
+            cores = cpus / (sockets * threads);
         } else {
-            if (cores == 0) {
-                threads = threads > 0 ? threads : 1;
-                cores = cpus / (sockets * threads);
-            } else {
-                threads = cpus / (cores * sockets);
-            }
+            threads = cpus / (cores * sockets);
         }
 
         max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/3] vl: fix max_cpus check

[Eduardo Habkost]

From: Andrew Jones <drjones@redhat.com>

We should confirm max_cpus, which is >= smp_cpus, is
<= the machine's true max_cpus, not just smp_cpus.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index c1bfc9b..2ed8b07 100644
--- a/vl.c
+++ b/vl.c
@@ -3901,9 +3901,9 @@ 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) {
+    if (max_cpus > machine_class->max_cpus) {
         fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus "
-                "supported by machine `%s' (%d)\n", smp_cpus,
+                "supported by machine `%s' (%d)\n", max_cpus,
                 machine_class->name, machine_class->max_cpus);
         exit(1);
     }
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/3] vl: Don't silently change topology when all -smp options were set

[Eduardo Habkost]

QEMU tries to change the "threads" option even if it was explicitly set
in the command-line, and it shouldn't do that.

The right thing to do when all options (cpus, sockets, cores, threds)
are explicitly set is to sanity check them and abort in case they don't
make sense (i.e. when sockets*cores*threads < cpus).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 2ed8b07..8880a4e 100644
--- a/vl.c
+++ b/vl.c
@@ -1287,8 +1287,14 @@ static void smp_parse(QemuOpts *opts)
         } else if (cores == 0) {
             threads = threads > 0 ? threads : 1;
             cores = cpus / (sockets * threads);
-        } else {
+        } else if (threads == 0) {
             threads = cpus / (cores * sockets);
+        } else if (sockets * cores * threads < cpus) {
+            fprintf(stderr, "cpu topology: error: "
+                    "sockets (%u) * cores (%u) * threads (%u) < "
+                    "smp_cpus (%u)\n",
+                    sockets, cores, threads, cpus);
+            exit(1);
         }
 
         max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 1/3] vl: Avoid unnecessary 'if' nesting

[Andrew Jones]

On Tue, Nov 11, 2014 at 04:50:54PM -0200, Eduardo Habkost wrote:
> Just a coding style change, to make other changes easier to review.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index f4a6e5e..c1bfc9b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1284,13 +1284,11 @@ static void smp_parse(QemuOpts *opts)
>              if (cpus == 0) {
>                  cpus = cores * threads * sockets;
>              }
> +        } else if (cores == 0) {
> +            threads = threads > 0 ? threads : 1;
> +            cores = cpus / (sockets * threads);
>          } else {
> -            if (cores == 0) {
> -                threads = threads > 0 ? threads : 1;
> -                cores = cpus / (sockets * threads);
> -            } else {
> -                threads = cpus / (cores * sockets);
> -            }
> +            threads = cpus / (cores * sockets);
>          }
>  
>          max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
> -- 
> 1.9.3
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

Re: [Qemu-devel] [PATCH v2 3/3] vl: Don't silently change topology when all -smp options were set

[Andrew Jones]

On Tue, Nov 11, 2014 at 04:50:56PM -0200, Eduardo Habkost wrote:
> QEMU tries to change the "threads" option even if it was explicitly set
> in the command-line, and it shouldn't do that.
> 
> The right thing to do when all options (cpus, sockets, cores, threds)
> are explicitly set is to sanity check them and abort in case they don't
> make sense (i.e. when sockets*cores*threads < cpus).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 2ed8b07..8880a4e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1287,8 +1287,14 @@ static void smp_parse(QemuOpts *opts)
>          } else if (cores == 0) {
>              threads = threads > 0 ? threads : 1;
>              cores = cpus / (sockets * threads);
> -        } else {
> +        } else if (threads == 0) {
>              threads = cpus / (cores * sockets);
> +        } else if (sockets * cores * threads < cpus) {
> +            fprintf(stderr, "cpu topology: error: "
> +                    "sockets (%u) * cores (%u) * threads (%u) < "
> +                    "smp_cpus (%u)\n",
> +                    sockets, cores, threads, cpus);
> +            exit(1);
>          }
>  
>          max_cpus = qemu_opt_get_number(opts, "maxcpus", 0);
> -- 
> 1.9.3
>

Reviewed-by: Andrew Jones <drjones@redhat.com>