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

[Qemu-devel] [PATCH RESEND 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 RESEND 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>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 vl.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index 113e98e..3b7157d 100644
--- a/vl.c
+++ b/vl.c
@@ -1300,13 +1300,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 RESEND 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 3b7157d..cc4c535 100644
--- a/vl.c
+++ b/vl.c
@@ -3954,9 +3954,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 RESEND 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>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 vl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index cc4c535..8ac38df 100644
--- a/vl.c
+++ b/vl.c
@@ -1303,8 +1303,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 RESEND v2 0/3] vl: smp_parse sanity checks

[Paolo Bonzini]

On 19/12/2014 03:59, Eduardo Habkost wrote:
> 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(-)
> 

Eduardo, I forgot the status of these patches.  Should I just apply
them?  If so, can you take care of documenting the changes in the 2.3
release notes?

Paolo

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

[Eduardo Habkost]

On Thu, Jan 08, 2015 at 11:48:30AM +0100, Paolo Bonzini wrote:
> 
> 
> On 19/12/2014 03:59, Eduardo Habkost wrote:
> > 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(-)
> > 
> 
> Eduardo, I forgot the status of these patches.  Should I just apply
> them?  If so, can you take care of documenting the changes in the 2.3
> release notes?

Yes, please.

I believe 2/3 doesn't need to be in the release notes as it is just a
bug fix which shouldn't break existing setups. 3/3 will make QEMU abort
in (rare?) cases where the command-line was already visibly broken, so I
will document it.

-- 
Eduardo

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

[Paolo Bonzini]

On 08/01/2015 14:22, Eduardo Habkost wrote:
> Yes, please.
> 
> I believe 2/3 doesn't need to be in the release notes as it is just a
> bug fix which shouldn't break existing setups. 3/3 will make QEMU abort
> in (rare?) cases where the command-line was already visibly broken, so I
> will document it.

Ok, applied (locally on top of my pending pull request).

Paolo