[RFC PATCH 0/6] machine: smp parsing fixes and improvement

[RFC PATCH 0/6] machine: smp parsing fixes and improvement

[Yanan Wang]

Hello,

Here are some smp parsing fix and improvement, most of which is about
the smp parsing helpers. This series was arranged based on the latest
QEMU code since commit d940d468e29b.

Description:
Patch #1 improves the calculation of maxcpus.
Patch #2 and #3 adds the missing zero-check for values of sockets and dies.
Patch #4 allows us to uniformly use maxcpus for all calculations.
Patch #5 reject the configuration of dies more strictly for non-PC machines.
Patch #6 makes no functional change but tweaks the order of topology parameters.

Regards,
Yanan

Yanan Wang (6):
  machine: Set the value of maxcpus to match cpus if specified as zero
  machine: Perform zero-check for the computed value of sockets
  pc/machine: Perform zero-check for the value of -smp dies
  machine: Uniformly use maxcpus to calculate the missing values
  pc/machine: Disallow any configuration of dies for non-PC machines
  machine: Tweak the order of topology members in struct CpuTopology

 hw/core/machine.c   | 49 +++++++++++++++++++++++++--------------------
 hw/i386/pc.c        | 48 +++++++++++++++++++++++++-------------------
 include/hw/boards.h |  7 ++++---
 qapi/machine.json   |  2 +-
 4 files changed, 59 insertions(+), 47 deletions(-)

-- 
2.19.1

[RFC PATCH 1/6] machine: Set the value of maxcpus to match cpus if specified as zero

[Yanan Wang]

It is currently allowed to explicitly specified the topology parameters
as 0 in the -smp cmdlines, such as -smp cpus=8,maxcpus=0,sockets=0. And
for the values of cpus/sockets/cores/threads, we always determine that
they are ommited if either set to 0 in the cmdline(e.g. sockets=0) or
just not explicitly specified, then we compute the ommited values.

We probably should also treat "maxcpus=0" as ommited and then set the
value to match smp cpus. This makes cmdlines like "-smp 8, maxcpus=0"
start to work as "-smp 8,maxcpus=8,sockets=8,cores=1,threads=1".

Note that this patch won't affect any existing working cmdlines, but
will allow configuration like "-smp cpus=n,maxcpus=0" to be valid.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 14 ++++++++------
 hw/i386/pc.c      | 14 ++++++++------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ffc076ae84..f17bbe3275 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -745,6 +745,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     unsigned sockets = config->has_sockets ? config->sockets : 0;
     unsigned cores   = config->has_cores ? config->cores : 0;
     unsigned threads = config->has_threads ? config->threads : 0;
+    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
 
     if (config->has_dies && config->dies != 0 && config->dies != 1) {
         error_setg(errp, "dies not supported by this machine's CPU topology");
@@ -758,8 +759,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
             sockets = sockets > 0 ? sockets : 1;
             cpus = cores * threads * sockets;
         } else {
-            ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
-            sockets = ms->smp.max_cpus / (cores * threads);
+            maxcpus = maxcpus > 0 ? maxcpus : cpus;
+            sockets = maxcpus / (cores * threads);
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
@@ -776,19 +777,19 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
         return;
     }
 
-    ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-    if (ms->smp.max_cpus < cpus) {
+    if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
     }
 
-    if (sockets * cores * threads != ms->smp.max_cpus) {
+    if (sockets * cores * threads != maxcpus) {
         error_setg(errp, "Invalid CPU topology: "
                    "sockets (%u) * cores (%u) * threads (%u) "
                    "!= maxcpus (%u)",
                    sockets, cores, threads,
-                   ms->smp.max_cpus);
+                   maxcpus);
         return;
     }
 
@@ -796,6 +797,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     ms->smp.cores = cores;
     ms->smp.threads = threads;
     ms->smp.sockets = sockets;
+    ms->smp.max_cpus = maxcpus;
 }
 
 static void machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8e1220db72..a9b22fdc01 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -717,6 +717,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     unsigned dies    = config->has_dies ? config->dies : 1;
     unsigned cores   = config->has_cores ? config->cores : 0;
     unsigned threads = config->has_threads ? config->threads : 0;
+    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
 
     /* compute missing values, prefer sockets over cores over threads */
     if (cpus == 0 || sockets == 0) {
@@ -726,8 +727,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
             sockets = sockets > 0 ? sockets : 1;
             cpus = cores * threads * dies * sockets;
         } else {
-            ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
-            sockets = ms->smp.max_cpus / (cores * threads * dies);
+            maxcpus = maxcpus > 0 ? maxcpus : cpus;
+            sockets = maxcpus / (dies * cores * threads);
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
@@ -744,19 +745,19 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
         return;
     }
 
-    ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-    if (ms->smp.max_cpus < cpus) {
+    if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
     }
 
-    if (sockets * dies * cores * threads != ms->smp.max_cpus) {
+    if (sockets * dies * cores * threads != maxcpus) {
         error_setg(errp, "Invalid CPU topology deprecated: "
                    "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
                    "!= maxcpus (%u)",
                    sockets, dies, cores, threads,
-                   ms->smp.max_cpus);
+                   maxcpus);
         return;
     }
 
@@ -765,6 +766,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     ms->smp.threads = threads;
     ms->smp.sockets = sockets;
     ms->smp.dies = dies;
+    ms->smp.max_cpus = maxcpus;
 }
 
 static
-- 
2.19.1

[RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets

[Yanan Wang]

We currently perform zero-check (default the value to 1 if zeroed)
for the computed values of cores/threads, to make sure they are at
least 1. For consistency, we probably should also default sockets
to 1 if the computed value is zero. Note that this won't affect
any existing working cmdlines but will improve the error reporting
of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 1 +
 hw/i386/pc.c      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f17bbe3275..1e194677cd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
         } else {
             maxcpus = maxcpus > 0 ? maxcpus : cpus;
             sockets = maxcpus / (cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a9b22fdc01..a44511c937 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
         } else {
             maxcpus = maxcpus > 0 ? maxcpus : cpus;
             sockets = maxcpus / (dies * cores * threads);
+            sockets = sockets > 0 ? sockets : 1;
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
-- 
2.19.1

[RFC PATCH 3/6] pc/machine: Perform zero-check for the value of -smp dies

[Yanan Wang]

It's possible that dies parameter is explicitly specified as "dies=0"
in the cmdline, if so we will wrongly calculate the other ommited
parameters such as "sockets = maxcpus / (dies * cores * threads);"
with a zeroed dies value.

So perform zero-check (default the value to 1 if zeroed) for -smp dies
before using it to calculate other parameters.

Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a44511c937..93d1f12a49 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
 {
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
-    unsigned dies    = config->has_dies ? config->dies : 1;
+    unsigned dies    = config->has_dies ? config->dies : 0;
     unsigned cores   = config->has_cores ? config->cores : 0;
     unsigned threads = config->has_threads ? config->threads : 0;
     unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
 
     /* compute missing values, prefer sockets over cores over threads */
+    dies = dies > 0 ? dies : 1;
+
     if (cpus == 0 || sockets == 0) {
         cores = cores > 0 ? cores : 1;
         threads = threads > 0 ? threads : 1;
-- 
2.19.1

[RFC PATCH 4/6] machine: Uniformly use maxcpus to calculate the missing values

[Yanan Wang]

We are currently using maxcpus to calculate value of sockets but using
cpus to calculate value of cores/threads. This makes cmdlines like
"-smp 8,maxcpus=12,cores=4" work while "-smp 8,maxcpus=12,sockets=3"
break the invalid cpu topology check.

This patch allows us to uniformly use maxcpus to calculate the missing
values. Also the if branch of "cpus == 0 || sockets == 0" was splited
into branches of "cpus == 0" and "sockets == 0" so that we can clearly
figure out that we are parsing -smp cmdlines with a preference of cpus
over sockets over cores over threads.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 36 +++++++++++++++++++-----------------
 hw/i386/pc.c      | 37 +++++++++++++++++++------------------
 2 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1e194677cd..58882835be 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -752,34 +752,36 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     }
 
     /* compute missing values, prefer sockets over cores over threads */
-    if (cpus == 0 || sockets == 0) {
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
+    if (cpus == 0) {
+        sockets = sockets > 0 ? sockets : 1;
         cores = cores > 0 ? cores : 1;
         threads = threads > 0 ? threads : 1;
-        if (cpus == 0) {
-            sockets = sockets > 0 ? sockets : 1;
-            cpus = cores * threads * sockets;
-        } else {
-            maxcpus = maxcpus > 0 ? maxcpus : cpus;
-            sockets = maxcpus / (cores * threads);
-            sockets = sockets > 0 ? sockets : 1;
-        }
+        cpus = sockets * cores * threads;
+        maxcpus = maxcpus > 0 ? maxcpus : cpus;
+    } else if (sockets == 0) {
+        cores = cores > 0 ? cores : 1;
+        threads = threads > 0 ? threads : 1;
+        sockets = maxcpus / (cores * threads);
+        sockets = sockets > 0 ? sockets : 1;
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
-        cores = cpus / (sockets * threads);
+        cores = maxcpus / (sockets * threads);
         cores = cores > 0 ? cores : 1;
     } else if (threads == 0) {
-        threads = cpus / (cores * sockets);
+        threads = maxcpus / (sockets * cores);
         threads = threads > 0 ? threads : 1;
-    } else if (sockets * cores * threads < cpus) {
+    }
+
+    if (sockets * cores * threads < cpus) {
         error_setg(errp, "cpu topology: "
-                   "sockets (%u) * cores (%u) * threads (%u) < "
-                   "smp_cpus (%u)",
+                   "sockets (%u) * cores (%u) * threads (%u) "
+                   "< smp_cpus (%u)",
                    sockets, cores, threads, cpus);
         return;
     }
 
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
     if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
@@ -795,9 +797,9 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     }
 
     ms->smp.cpus = cpus;
+    ms->smp.sockets = sockets;
     ms->smp.cores = cores;
     ms->smp.threads = threads;
-    ms->smp.sockets = sockets;
     ms->smp.max_cpus = maxcpus;
 }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 93d1f12a49..1812f33ab1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -721,35 +721,36 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
 
     /* compute missing values, prefer sockets over cores over threads */
     dies = dies > 0 ? dies : 1;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-    if (cpus == 0 || sockets == 0) {
+    if (cpus == 0) {
+        sockets = sockets > 0 ? sockets : 1;
         cores = cores > 0 ? cores : 1;
         threads = threads > 0 ? threads : 1;
-        if (cpus == 0) {
-            sockets = sockets > 0 ? sockets : 1;
-            cpus = cores * threads * dies * sockets;
-        } else {
-            maxcpus = maxcpus > 0 ? maxcpus : cpus;
-            sockets = maxcpus / (dies * cores * threads);
-            sockets = sockets > 0 ? sockets : 1;
-        }
+        cpus = sockets * dies * cores * threads;
+        maxcpus = maxcpus > 0 ? maxcpus : cpus;
+    } else if (sockets == 0) {
+        cores = cores > 0 ? cores : 1;
+        threads = threads > 0 ? threads : 1;
+        sockets = maxcpus / (dies * cores * threads);
+        sockets = sockets > 0 ? sockets : 1;
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
-        cores = cpus / (sockets * dies * threads);
+        cores = maxcpus / (sockets * dies * threads);
         cores = cores > 0 ? cores : 1;
     } else if (threads == 0) {
-        threads = cpus / (cores * dies * sockets);
+        threads = maxcpus / (sockets * dies * cores);
         threads = threads > 0 ? threads : 1;
-    } else if (sockets * dies * cores * threads < cpus) {
+    }
+
+    if (sockets * dies * cores * threads < cpus) {
         error_setg(errp, "cpu topology: "
-                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
-                   "smp_cpus (%u)",
+                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
+                   "< smp_cpus (%u)",
                    sockets, dies, cores, threads, cpus);
         return;
     }
 
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
     if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
@@ -765,10 +766,10 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     }
 
     ms->smp.cpus = cpus;
-    ms->smp.cores = cores;
-    ms->smp.threads = threads;
     ms->smp.sockets = sockets;
     ms->smp.dies = dies;
+    ms->smp.cores = cores;
+    ms->smp.threads = threads;
     ms->smp.max_cpus = maxcpus;
 }
 
-- 
2.19.1

[RFC PATCH 5/6] pc/machine: Disallow any configuration of dies for non-PC machines

[Yanan Wang]

Since a machine type does not support topology parameter of dies,
it's probably more reasonable to reject any explicit specification
to avoid possible confuse, including "dies=0" and "dies=1" although
they won't affect the calculation of non-PC machines.

Also a comment of struct SMPConfiguration is fixed.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 2 +-
 qapi/machine.json | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 58882835be..55785feee2 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -747,7 +747,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     unsigned threads = config->has_threads ? config->threads : 0;
     unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
 
-    if (config->has_dies && config->dies != 0 && config->dies != 1) {
+    if (config->has_dies) {
         error_setg(errp, "dies not supported by this machine's CPU topology");
     }
 
diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb..253f84abf6 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1297,7 +1297,7 @@
 #
 # @dies: number of dies per socket in the CPU topology
 #
-# @cores: number of cores per thread in the CPU topology
+# @cores: number of cores per die in the CPU topology
 #
 # @threads: number of threads per core in the CPU topology
 #
-- 
2.19.1

[RFC PATCH 6/6] machine: Tweak the order of topology members in struct CpuTopology

[Yanan Wang]

Now that all the possible topology parameters are integrated in struct
CpuTopology, tweak the order of topology members to be "cpus/sockets/
dies/cores/threads/maxcpus" for readability and consistency. We also
tweak the comment by adding explanation of dies parameter.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c   | 4 ++--
 include/hw/boards.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 55785feee2..8c538d2ba5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -968,10 +968,10 @@ static void machine_initfn(Object *obj)
     /* default to mc->default_cpus */
     ms->smp.cpus = mc->default_cpus;
     ms->smp.max_cpus = mc->default_cpus;
-    ms->smp.cores = 1;
+    ms->smp.sockets = 1;
     ms->smp.dies = 1;
+    ms->smp.cores = 1;
     ms->smp.threads = 1;
-    ms->smp.sockets = 1;
 }
 
 static void machine_finalize(Object *obj)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 1eae4427e8..3b64757981 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -275,17 +275,18 @@ typedef struct DeviceMemoryState {
 /**
  * CpuTopology:
  * @cpus: the number of present logical processors on the machine
- * @cores: the number of cores in one package
- * @threads: the number of threads in one core
  * @sockets: the number of sockets on the machine
+ * @dies: the number of dies in one socket
+ * @cores: the number of cores in one die
+ * @threads: the number of threads in one core
  * @max_cpus: the maximum number of logical processors on the machine
  */
 typedef struct CpuTopology {
     unsigned int cpus;
+    unsigned int sockets;
     unsigned int dies;
     unsigned int cores;
     unsigned int threads;
-    unsigned int sockets;
     unsigned int max_cpus;
 } CpuTopology;
 
-- 
2.19.1

Re: [RFC PATCH 5/6] pc/machine: Disallow any configuration of dies for non-PC machines

[Daniel P. Berrangé]

On Fri, Jul 02, 2021 at 06:07:38PM +0800, Yanan Wang wrote:
> Since a machine type does not support topology parameter of dies,
> it's probably more reasonable to reject any explicit specification
> to avoid possible confuse, including "dies=0" and "dies=1" although
> they won't affect the calculation of non-PC machines.
> 
> Also a comment of struct SMPConfiguration is fixed.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 2 +-
>  qapi/machine.json | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 58882835be..55785feee2 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -747,7 +747,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      unsigned threads = config->has_threads ? config->threads : 0;
>      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>  
> -    if (config->has_dies && config->dies != 0 && config->dies != 1) {
> +    if (config->has_dies) {
>          error_setg(errp, "dies not supported by this machine's CPU topology");
>      }

This will cause a functional regression. Libvirt always specifies
dies=1 if QEMU supports it.  I don't see a need to reject it,
since conceptually it is reasonable to say that all existing
CPUs have a single die. It simply that 99% of CPUs don't support
more than 1 die.

> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb..253f84abf6 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1297,7 +1297,7 @@
>  #
>  # @dies: number of dies per socket in the CPU topology
>  #
> -# @cores: number of cores per thread in the CPU topology
> +# @cores: number of cores per die in the CPU topology
>  #
>  # @threads: number of threads per core in the CPU topology
>  #

This is a simple docs fix and ok

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 5/6] pc/machine: Disallow any configuration of dies for non-PC machines

[wangyanan (Y)]

On 2021/7/2 18:18, Daniel P. Berrangé wrote:
> On Fri, Jul 02, 2021 at 06:07:38PM +0800, Yanan Wang wrote:
>> Since a machine type does not support topology parameter of dies,
>> it's probably more reasonable to reject any explicit specification
>> to avoid possible confuse, including "dies=0" and "dies=1" although
>> they won't affect the calculation of non-PC machines.
>>
>> Also a comment of struct SMPConfiguration is fixed.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/core/machine.c | 2 +-
>>   qapi/machine.json | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 58882835be..55785feee2 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -747,7 +747,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>       unsigned threads = config->has_threads ? config->threads : 0;
>>       unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>>   
>> -    if (config->has_dies && config->dies != 0 && config->dies != 1) {
>> +    if (config->has_dies) {
>>           error_setg(errp, "dies not supported by this machine's CPU topology");
>>       }
> This will cause a functional regression. Libvirt always specifies
> dies=1 if QEMU supports it.  I don't see a need to reject it,
> since conceptually it is reasonable to say that all existing
> CPUs have a single die. It simply that 99% of CPUs don't support
> more than 1 die.
Ok, I agree. This is a sincere suggestion, but clearly i didn't consider
how Libvirt deals with configuration of dies. I will drop this part and
the single comment fix can be merged into patch #6.
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index c3210ee1fb..253f84abf6 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -1297,7 +1297,7 @@
>>   #
>>   # @dies: number of dies per socket in the CPU topology
>>   #
>> -# @cores: number of cores per thread in the CPU topology
>> +# @cores: number of cores per die in the CPU topology
>>   #
>>   # @threads: number of threads per core in the CPU topology
>>   #
> This is a simple docs fix and ok
>
Thanks,
Yanan
.

Re: [RFC PATCH 1/6] machine: Set the value of maxcpus to match cpus if specified as zero

[Andrew Jones]

On Fri, Jul 02, 2021 at 06:07:34PM +0800, Yanan Wang wrote:
> It is currently allowed to explicitly specified the topology parameters
> as 0 in the -smp cmdlines, such as -smp cpus=8,maxcpus=0,sockets=0. And
> for the values of cpus/sockets/cores/threads, we always determine that
> they are ommited if either set to 0 in the cmdline(e.g. sockets=0) or
> just not explicitly specified, then we compute the ommited values.
> 
> We probably should also treat "maxcpus=0" as ommited and then set the
> value to match smp cpus. This makes cmdlines like "-smp 8, maxcpus=0"
> start to work as "-smp 8,maxcpus=8,sockets=8,cores=1,threads=1".
> 
> Note that this patch won't affect any existing working cmdlines, but
> will allow configuration like "-smp cpus=n,maxcpus=0" to be valid.

Personally, I'd rather see -smp cpus=n,sockets=0 become an error than to
"fix" -smp cpus=n,maxcpus=0.

Thanks,
drew
 
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 14 ++++++++------
>  hw/i386/pc.c      | 14 ++++++++------
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ffc076ae84..f17bbe3275 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -745,6 +745,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      unsigned sockets = config->has_sockets ? config->sockets : 0;
>      unsigned cores   = config->has_cores ? config->cores : 0;
>      unsigned threads = config->has_threads ? config->threads : 0;
> +    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>  
>      if (config->has_dies && config->dies != 0 && config->dies != 1) {
>          error_setg(errp, "dies not supported by this machine's CPU topology");
> @@ -758,8 +759,8 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>              sockets = sockets > 0 ? sockets : 1;
>              cpus = cores * threads * sockets;
>          } else {
> -            ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
> -            sockets = ms->smp.max_cpus / (cores * threads);
> +            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +            sockets = maxcpus / (cores * threads);
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> @@ -776,19 +777,19 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>          return;
>      }
>  
> -    ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -    if (ms->smp.max_cpus < cpus) {
> +    if (maxcpus < cpus) {
>          error_setg(errp, "maxcpus must be equal to or greater than smp");
>          return;
>      }
>  
> -    if (sockets * cores * threads != ms->smp.max_cpus) {
> +    if (sockets * cores * threads != maxcpus) {
>          error_setg(errp, "Invalid CPU topology: "
>                     "sockets (%u) * cores (%u) * threads (%u) "
>                     "!= maxcpus (%u)",
>                     sockets, cores, threads,
> -                   ms->smp.max_cpus);
> +                   maxcpus);
>          return;
>      }
>  
> @@ -796,6 +797,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      ms->smp.cores = cores;
>      ms->smp.threads = threads;
>      ms->smp.sockets = sockets;
> +    ms->smp.max_cpus = maxcpus;
>  }
>  
>  static void machine_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8e1220db72..a9b22fdc01 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -717,6 +717,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      unsigned dies    = config->has_dies ? config->dies : 1;
>      unsigned cores   = config->has_cores ? config->cores : 0;
>      unsigned threads = config->has_threads ? config->threads : 0;
> +    unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>  
>      /* compute missing values, prefer sockets over cores over threads */
>      if (cpus == 0 || sockets == 0) {
> @@ -726,8 +727,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>              sockets = sockets > 0 ? sockets : 1;
>              cpus = cores * threads * dies * sockets;
>          } else {
> -            ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
> -            sockets = ms->smp.max_cpus / (cores * threads * dies);
> +            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +            sockets = maxcpus / (dies * cores * threads);
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> @@ -744,19 +745,19 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>          return;
>      }
>  
> -    ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -    if (ms->smp.max_cpus < cpus) {
> +    if (maxcpus < cpus) {
>          error_setg(errp, "maxcpus must be equal to or greater than smp");
>          return;
>      }
>  
> -    if (sockets * dies * cores * threads != ms->smp.max_cpus) {
> +    if (sockets * dies * cores * threads != maxcpus) {
>          error_setg(errp, "Invalid CPU topology deprecated: "
>                     "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
>                     "!= maxcpus (%u)",
>                     sockets, dies, cores, threads,
> -                   ms->smp.max_cpus);
> +                   maxcpus);
>          return;
>      }
>  
> @@ -765,6 +766,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      ms->smp.threads = threads;
>      ms->smp.sockets = sockets;
>      ms->smp.dies = dies;
> +    ms->smp.max_cpus = maxcpus;
>  }
>  
>  static
> -- 
> 2.19.1
> 

Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets

[Andrew Jones]

On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:
> We currently perform zero-check (default the value to 1 if zeroed)
> for the computed values of cores/threads, to make sure they are at
> least 1. For consistency, we probably should also default sockets
> to 1 if the computed value is zero. Note that this won't affect
> any existing working cmdlines but will improve the error reporting
> of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".

How does this help error checking? If the user input values that compute a
fractional (rounded down to zero with integer division) value, then we'll
catch that with the sockets*cores*threads == maxcpus test now, but we may
not catch that after this patch.

Thanks,
drew

> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 1 +
>  hw/i386/pc.c      | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f17bbe3275..1e194677cd 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>          } else {
>              maxcpus = maxcpus > 0 ? maxcpus : cpus;
>              sockets = maxcpus / (cores * threads);
> +            sockets = sockets > 0 ? sockets : 1;
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a9b22fdc01..a44511c937 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>          } else {
>              maxcpus = maxcpus > 0 ? maxcpus : cpus;
>              sockets = maxcpus / (dies * cores * threads);
> +            sockets = sockets > 0 ? sockets : 1;
>          }
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> -- 
> 2.19.1
> 

Re: [RFC PATCH 3/6] pc/machine: Perform zero-check for the value of -smp dies

[Andrew Jones]

On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
> It's possible that dies parameter is explicitly specified as "dies=0"
> in the cmdline, if so we will wrongly calculate the other ommited
> parameters such as "sockets = maxcpus / (dies * cores * threads);"
> with a zeroed dies value.
> 
> So perform zero-check (default the value to 1 if zeroed) for -smp dies
> before using it to calculate other parameters.

OK, dies=0 may make some sense for a user that doesn't want to describe
dies.

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

> 
> Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/i386/pc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a44511c937..93d1f12a49 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>  {
>      unsigned cpus    = config->has_cpus ? config->cpus : 0;
>      unsigned sockets = config->has_sockets ? config->sockets : 0;
> -    unsigned dies    = config->has_dies ? config->dies : 1;
> +    unsigned dies    = config->has_dies ? config->dies : 0;
>      unsigned cores   = config->has_cores ? config->cores : 0;
>      unsigned threads = config->has_threads ? config->threads : 0;
>      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>  
>      /* compute missing values, prefer sockets over cores over threads */
> +    dies = dies > 0 ? dies : 1;
> +
>      if (cpus == 0 || sockets == 0) {
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -- 
> 2.19.1
> 

Re: [RFC PATCH 3/6] pc/machine: Perform zero-check for the value of -smp dies

[Andrew Jones]

On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
> > It's possible that dies parameter is explicitly specified as "dies=0"
> > in the cmdline, if so we will wrongly calculate the other ommited
> > parameters such as "sockets = maxcpus / (dies * cores * threads);"
> > with a zeroed dies value.
> > 
> > So perform zero-check (default the value to 1 if zeroed) for -smp dies
> > before using it to calculate other parameters.
> 
> OK, dies=0 may make some sense for a user that doesn't want to describe
> dies.
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

But... this is the pc smp parse function, not the general one, so maybe my
comment from patch 1 should apply here as well, which is, that dies=0
should be an error rather than silently changed to dies=1.

> 
> > 
> > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >  hw/i386/pc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index a44511c937..93d1f12a49 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
> >  {
> >      unsigned cpus    = config->has_cpus ? config->cpus : 0;
> >      unsigned sockets = config->has_sockets ? config->sockets : 0;
> > -    unsigned dies    = config->has_dies ? config->dies : 1;
> > +    unsigned dies    = config->has_dies ? config->dies : 0;
> >      unsigned cores   = config->has_cores ? config->cores : 0;
> >      unsigned threads = config->has_threads ? config->threads : 0;
> >      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> >  
> >      /* compute missing values, prefer sockets over cores over threads */
> > +    dies = dies > 0 ? dies : 1;
> > +
> >      if (cpus == 0 || sockets == 0) {
> >          cores = cores > 0 ? cores : 1;
> >          threads = threads > 0 ? threads : 1;
> > -- 
> > 2.19.1
> > 

Re: [RFC PATCH 4/6] machine: Uniformly use maxcpus to calculate the missing values

[Andrew Jones]

On Fri, Jul 02, 2021 at 06:07:37PM +0800, Yanan Wang wrote:
> We are currently using maxcpus to calculate value of sockets but using
> cpus to calculate value of cores/threads. This makes cmdlines like
> "-smp 8,maxcpus=12,cores=4" work while "-smp 8,maxcpus=12,sockets=3"
> break the invalid cpu topology check.
> 
> This patch allows us to uniformly use maxcpus to calculate the missing
> values. Also the if branch of "cpus == 0 || sockets == 0" was splited
> into branches of "cpus == 0" and "sockets == 0" so that we can clearly
> figure out that we are parsing -smp cmdlines with a preference of cpus
> over sockets over cores over threads.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 36 +++++++++++++++++++-----------------
>  hw/i386/pc.c      | 37 +++++++++++++++++++------------------
>  2 files changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1e194677cd..58882835be 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -752,34 +752,36 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      }
>  
>      /* compute missing values, prefer sockets over cores over threads */
> -    if (cpus == 0 || sockets == 0) {
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +
> +    if (cpus == 0) {
> +        sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -        if (cpus == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
> -            cpus = cores * threads * sockets;
> -        } else {
> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -            sockets = maxcpus / (cores * threads);
> -            sockets = sockets > 0 ? sockets : 1;
> -        }
> +        cpus = sockets * cores * threads;
> +        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +    } else if (sockets == 0) {
> +        cores = cores > 0 ? cores : 1;
> +        threads = threads > 0 ? threads : 1;
> +        sockets = maxcpus / (cores * threads);
> +        sockets = sockets > 0 ? sockets : 1;

As stated in the other patch, this rounding up of a fractional sockets
shouldn't be here. maxcpus or (cpus==maxcpus) should always be selected by
the user to be a product of whole number sockets, cores, threads.

>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> -        cores = cpus / (sockets * threads);
> +        cores = maxcpus / (sockets * threads);
>          cores = cores > 0 ? cores : 1;

Now that we're using maxcpus for the calculation, then no rounding for
cores either...

>      } else if (threads == 0) {
> -        threads = cpus / (cores * sockets);
> +        threads = maxcpus / (sockets * cores);
>          threads = threads > 0 ? threads : 1;

...or threads.

> -    } else if (sockets * cores * threads < cpus) {
> +    }
> +
> +    if (sockets * cores * threads < cpus) {
>          error_setg(errp, "cpu topology: "
> -                   "sockets (%u) * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> +                   "sockets (%u) * cores (%u) * threads (%u) "
> +                   "< smp_cpus (%u)",

Why make this change?

>                     sockets, cores, threads, cpus);
>          return;
>      }
>  
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
>      if (maxcpus < cpus) {
>          error_setg(errp, "maxcpus must be equal to or greater than smp");
>          return;
> @@ -795,9 +797,9 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      }
>  
>      ms->smp.cpus = cpus;
> +    ms->smp.sockets = sockets;
>      ms->smp.cores = cores;
>      ms->smp.threads = threads;
> -    ms->smp.sockets = sockets;
>      ms->smp.max_cpus = maxcpus;
>  }
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 93d1f12a49..1812f33ab1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -721,35 +721,36 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>  
>      /* compute missing values, prefer sockets over cores over threads */
>      dies = dies > 0 ? dies : 1;
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -    if (cpus == 0 || sockets == 0) {
> +    if (cpus == 0) {
> +        sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -        if (cpus == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
> -            cpus = cores * threads * dies * sockets;
> -        } else {
> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -            sockets = maxcpus / (dies * cores * threads);
> -            sockets = sockets > 0 ? sockets : 1;
> -        }
> +        cpus = sockets * dies * cores * threads;
> +        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +    } else if (sockets == 0) {
> +        cores = cores > 0 ? cores : 1;
> +        threads = threads > 0 ? threads : 1;
> +        sockets = maxcpus / (dies * cores * threads);
> +        sockets = sockets > 0 ? sockets : 1;
>      } else if (cores == 0) {
>          threads = threads > 0 ? threads : 1;
> -        cores = cpus / (sockets * dies * threads);
> +        cores = maxcpus / (sockets * dies * threads);
>          cores = cores > 0 ? cores : 1;
>      } else if (threads == 0) {
> -        threads = cpus / (cores * dies * sockets);
> +        threads = maxcpus / (sockets * dies * cores);
>          threads = threads > 0 ? threads : 1;
> -    } else if (sockets * dies * cores * threads < cpus) {
> +    }
> +
> +    if (sockets * dies * cores * threads < cpus) {
>          error_setg(errp, "cpu topology: "
> -                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> +                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> +                   "< smp_cpus (%u)",
>                     sockets, dies, cores, threads, cpus);
>          return;
>      }
>  

Same comments as for the general function.

Thanks,
drew

> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
>      if (maxcpus < cpus) {
>          error_setg(errp, "maxcpus must be equal to or greater than smp");
>          return;
> @@ -765,10 +766,10 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      }
>  
>      ms->smp.cpus = cpus;
> -    ms->smp.cores = cores;
> -    ms->smp.threads = threads;
>      ms->smp.sockets = sockets;
>      ms->smp.dies = dies;
> +    ms->smp.cores = cores;
> +    ms->smp.threads = threads;
>      ms->smp.max_cpus = maxcpus;
>  }
>  
> -- 
> 2.19.1
> 

Re: [RFC PATCH 1/6] machine: Set the value of maxcpus to match cpus if specified as zero

[Daniel P. Berrangé]

On Mon, Jul 12, 2021 at 04:57:37PM +0200, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 06:07:34PM +0800, Yanan Wang wrote:
> > It is currently allowed to explicitly specified the topology parameters
> > as 0 in the -smp cmdlines, such as -smp cpus=8,maxcpus=0,sockets=0. And
> > for the values of cpus/sockets/cores/threads, we always determine that
> > they are ommited if either set to 0 in the cmdline(e.g. sockets=0) or
> > just not explicitly specified, then we compute the ommited values.
> > 
> > We probably should also treat "maxcpus=0" as ommited and then set the
> > value to match smp cpus. This makes cmdlines like "-smp 8, maxcpus=0"
> > start to work as "-smp 8,maxcpus=8,sockets=8,cores=1,threads=1".
> > 
> > Note that this patch won't affect any existing working cmdlines, but
> > will allow configuration like "-smp cpus=n,maxcpus=0" to be valid.
> 
> Personally, I'd rather see -smp cpus=n,sockets=0 become an error than to
> "fix" -smp cpus=n,maxcpus=0.

Yes, a value of 0 is not really something a user should ever be explicitly
setting. It is purely an internal default that is used by QEMU to identify
when it needs to auto-fill a value.

A user should give a non-zero value, or omit the parameter entirely


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 3/6] pc/machine: Perform zero-check for the value of -smp dies

[Andrew Jones]

On Mon, Jul 12, 2021 at 05:05:43PM +0200, Andrew Jones wrote:
> On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote:
> > On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
> > > It's possible that dies parameter is explicitly specified as "dies=0"
> > > in the cmdline, if so we will wrongly calculate the other ommited
> > > parameters such as "sockets = maxcpus / (dies * cores * threads);"
> > > with a zeroed dies value.
> > > 
> > > So perform zero-check (default the value to 1 if zeroed) for -smp dies
> > > before using it to calculate other parameters.
> > 
> > OK, dies=0 may make some sense for a user that doesn't want to describe
> > dies.
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> But... this is the pc smp parse function, not the general one, so maybe my
> comment from patch 1 should apply here as well, which is, that dies=0
> should be an error rather than silently changed to dies=1.

Also, after reading Daniel's comment on a later patch, I think anything=0
should just be an error, even for the general parser.

Thanks,
drew

> 
> > 
> > > 
> > > Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
> > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > > ---
> > >  hw/i386/pc.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index a44511c937..93d1f12a49 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
> > >  {
> > >      unsigned cpus    = config->has_cpus ? config->cpus : 0;
> > >      unsigned sockets = config->has_sockets ? config->sockets : 0;
> > > -    unsigned dies    = config->has_dies ? config->dies : 1;
> > > +    unsigned dies    = config->has_dies ? config->dies : 0;
> > >      unsigned cores   = config->has_cores ? config->cores : 0;
> > >      unsigned threads = config->has_threads ? config->threads : 0;
> > >      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
> > >  
> > >      /* compute missing values, prefer sockets over cores over threads */
> > > +    dies = dies > 0 ? dies : 1;
> > > +
> > >      if (cpus == 0 || sockets == 0) {
> > >          cores = cores > 0 ? cores : 1;
> > >          threads = threads > 0 ? threads : 1;
> > > -- 
> > > 2.19.1
> > > 

Re: [RFC PATCH 6/6] machine: Tweak the order of topology members in struct CpuTopology

[Andrew Jones]

On Fri, Jul 02, 2021 at 06:07:39PM +0800, Yanan Wang wrote:
> Now that all the possible topology parameters are integrated in struct
> CpuTopology, tweak the order of topology members to be "cpus/sockets/
> dies/cores/threads/maxcpus" for readability and consistency. We also
> tweak the comment by adding explanation of dies parameter.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c   | 4 ++--
>  include/hw/boards.h | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)

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

Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets

[Daniel P. Berrangé]

On Mon, Jul 12, 2021 at 05:00:48PM +0200, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:
> > We currently perform zero-check (default the value to 1 if zeroed)
> > for the computed values of cores/threads, to make sure they are at
> > least 1. For consistency, we probably should also default sockets
> > to 1 if the computed value is zero. Note that this won't affect
> > any existing working cmdlines but will improve the error reporting
> > of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".
> 
> How does this help error checking? If the user input values that compute a
> fractional (rounded down to zero with integer division) value, then we'll
> catch that with the sockets*cores*threads == maxcpus test now, but we may
> not catch that after this patch.

Since we're having alot of debates about what should be valid scenarios
vs invalid scenarios, I think this points towards introducing a tests
for the smp_parse function, that enumerates both the correct and
incorrect scenarios based on the current implementation.

This series should then update the test cases for scenarios that we
think are currently wrongly handled.

> 
> Thanks,
> drew
> 
> > 
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >  hw/core/machine.c | 1 +
> >  hw/i386/pc.c      | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index f17bbe3275..1e194677cd 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> >          } else {
> >              maxcpus = maxcpus > 0 ? maxcpus : cpus;
> >              sockets = maxcpus / (cores * threads);
> > +            sockets = sockets > 0 ? sockets : 1;
> >          }
> >      } else if (cores == 0) {
> >          threads = threads > 0 ? threads : 1;
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index a9b22fdc01..a44511c937 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
> >          } else {
> >              maxcpus = maxcpus > 0 ? maxcpus : cpus;
> >              sockets = maxcpus / (dies * cores * threads);
> > +            sockets = sockets > 0 ? sockets : 1;
> >          }
> >      } else if (cores == 0) {
> >          threads = threads > 0 ? threads : 1;
> > -- 
> > 2.19.1
> > 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 6/6] machine: Tweak the order of topology members in struct CpuTopology

[Pankaj Gupta]

> Now that all the possible topology parameters are integrated in struct
> CpuTopology, tweak the order of topology members to be "cpus/sockets/
> dies/cores/threads/maxcpus" for readability and consistency. We also
> tweak the comment by adding explanation of dies parameter.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c   | 4 ++--
>  include/hw/boards.h | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 55785feee2..8c538d2ba5 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -968,10 +968,10 @@ static void machine_initfn(Object *obj)
>      /* default to mc->default_cpus */
>      ms->smp.cpus = mc->default_cpus;
>      ms->smp.max_cpus = mc->default_cpus;
> -    ms->smp.cores = 1;
> +    ms->smp.sockets = 1;
>      ms->smp.dies = 1;
> +    ms->smp.cores = 1;
>      ms->smp.threads = 1;
> -    ms->smp.sockets = 1;
>  }
>
>  static void machine_finalize(Object *obj)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 1eae4427e8..3b64757981 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -275,17 +275,18 @@ typedef struct DeviceMemoryState {
>  /**
>   * CpuTopology:
>   * @cpus: the number of present logical processors on the machine
> - * @cores: the number of cores in one package
> - * @threads: the number of threads in one core
>   * @sockets: the number of sockets on the machine
> + * @dies: the number of dies in one socket
> + * @cores: the number of cores in one die
> + * @threads: the number of threads in one core
>   * @max_cpus: the maximum number of logical processors on the machine
>   */
>  typedef struct CpuTopology {
>      unsigned int cpus;
> +    unsigned int sockets;
>      unsigned int dies;
>      unsigned int cores;
>      unsigned int threads;
> -    unsigned int sockets;
>      unsigned int max_cpus;
>  } CpuTopology;

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>

Re: [RFC PATCH 3/6] pc/machine: Perform zero-check for the value of -smp dies

[wangyanan (Y)]

On 2021/7/12 23:27, Andrew Jones wrote:
> On Mon, Jul 12, 2021 at 05:05:43PM +0200, Andrew Jones wrote:
>> On Mon, Jul 12, 2021 at 05:04:04PM +0200, Andrew Jones wrote:
>>> On Fri, Jul 02, 2021 at 06:07:36PM +0800, Yanan Wang wrote:
>>>> It's possible that dies parameter is explicitly specified as "dies=0"
>>>> in the cmdline, if so we will wrongly calculate the other ommited
>>>> parameters such as "sockets = maxcpus / (dies * cores * threads);"
>>>> with a zeroed dies value.
>>>>
>>>> So perform zero-check (default the value to 1 if zeroed) for -smp dies
>>>> before using it to calculate other parameters.
>>> OK, dies=0 may make some sense for a user that doesn't want to describe
>>> dies.
>>>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> But... this is the pc smp parse function, not the general one, so maybe my
>> comment from patch 1 should apply here as well, which is, that dies=0
>> should be an error rather than silently changed to dies=1.
> Also, after reading Daniel's comment on a later patch, I think anything=0
> should just be an error, even for the general parser.
>
Ok, after reading comments in patch #1, I also agree with this solution.
We can reject the "anything = 0" configuration in advance in function
machine_smp_parse(). I will do it in v2.

BTW, I have converted smp_parse() into a generic helper allowing future
extension for new arch-specific parameters, as you have suggested.
So that we don't need to maintain duplicated parsing code any more.

Thanks,
Yanan
.
>
>>>> Fixes: 1b45842203540 (vl.c: Add -smp, dies=* command line support and update doc)
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> ---
>>>>   hw/i386/pc.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index a44511c937..93d1f12a49 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -714,12 +714,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>>>   {
>>>>       unsigned cpus    = config->has_cpus ? config->cpus : 0;
>>>>       unsigned sockets = config->has_sockets ? config->sockets : 0;
>>>> -    unsigned dies    = config->has_dies ? config->dies : 1;
>>>> +    unsigned dies    = config->has_dies ? config->dies : 0;
>>>>       unsigned cores   = config->has_cores ? config->cores : 0;
>>>>       unsigned threads = config->has_threads ? config->threads : 0;
>>>>       unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>>>>   
>>>>       /* compute missing values, prefer sockets over cores over threads */
>>>> +    dies = dies > 0 ? dies : 1;
>>>> +
>>>>       if (cpus == 0 || sockets == 0) {
>>>>           cores = cores > 0 ? cores : 1;
>>>>           threads = threads > 0 ? threads : 1;
>>>> -- 
>>>> 2.19.1
>>>>
> .

Re: [RFC PATCH 2/6] machine: Perform zero-check for the computed value of sockets

[wangyanan (Y)]

On 2021/7/12 23:30, Daniel P. Berrangé wrote:
> On Mon, Jul 12, 2021 at 05:00:48PM +0200, Andrew Jones wrote:
>> On Fri, Jul 02, 2021 at 06:07:35PM +0800, Yanan Wang wrote:
>>> We currently perform zero-check (default the value to 1 if zeroed)
>>> for the computed values of cores/threads, to make sure they are at
>>> least 1. For consistency, we probably should also default sockets
>>> to 1 if the computed value is zero. Note that this won't affect
>>> any existing working cmdlines but will improve the error reporting
>>> of the invalid ones such as "-smp 8,maxcpus=9,cores=10,threads=1".
>> How does this help error checking? If the user input values that compute a
>> fractional (rounded down to zero with integer division) value, then we'll
>> catch that with the sockets*cores*threads == maxcpus test now, but we may
>> not catch that after this patch.
> Since we're having alot of debates about what should be valid scenarios
> vs invalid scenarios, I think this points towards introducing a tests
> for the smp_parse function, that enumerates both the correct and
> incorrect scenarios based on the current implementation.
I think so! Actually I'm already working on a simple QEMU unit test
(test-smp-parse.c) for smp parsing and plan to post it in v2. But it
will directly test the finally improved generic parser.

Thanks,
Yanan
.
> This series should then update the test cases for scenarios that we
> think are currently wrongly handled.
>
>> Thanks,
>> drew
>>
>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>> ---
>>>   hw/core/machine.c | 1 +
>>>   hw/i386/pc.c      | 1 +
>>>   2 files changed, 2 insertions(+)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index f17bbe3275..1e194677cd 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -761,6 +761,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>>           } else {
>>>               maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>>               sockets = maxcpus / (cores * threads);
>>> +            sockets = sockets > 0 ? sockets : 1;
>>>           }
>>>       } else if (cores == 0) {
>>>           threads = threads > 0 ? threads : 1;
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index a9b22fdc01..a44511c937 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -729,6 +729,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>>           } else {
>>>               maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>>               sockets = maxcpus / (dies * cores * threads);
>>> +            sockets = sockets > 0 ? sockets : 1;
>>>           }
>>>       } else if (cores == 0) {
>>>           threads = threads > 0 ? threads : 1;
>>> -- 
>>> 2.19.1
>>>
> Regards,
> Daniel

Re: [RFC PATCH 4/6] machine: Uniformly use maxcpus to calculate the missing values

[wangyanan (Y)]

Hi Drew,

On 2021/7/12 23:25, Andrew Jones wrote:
> On Fri, Jul 02, 2021 at 06:07:37PM +0800, Yanan Wang wrote:
>> We are currently using maxcpus to calculate value of sockets but using
>> cpus to calculate value of cores/threads. This makes cmdlines like
>> "-smp 8,maxcpus=12,cores=4" work while "-smp 8,maxcpus=12,sockets=3"
>> break the invalid cpu topology check.
>>
>> This patch allows us to uniformly use maxcpus to calculate the missing
>> values. Also the if branch of "cpus == 0 || sockets == 0" was splited
>> into branches of "cpus == 0" and "sockets == 0" so that we can clearly
>> figure out that we are parsing -smp cmdlines with a preference of cpus
>> over sockets over cores over threads.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/core/machine.c | 36 +++++++++++++++++++-----------------
>>   hw/i386/pc.c      | 37 +++++++++++++++++++------------------
>>   2 files changed, 38 insertions(+), 35 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 1e194677cd..58882835be 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -752,34 +752,36 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>       }
>>   
>>       /* compute missing values, prefer sockets over cores over threads */
>> -    if (cpus == 0 || sockets == 0) {
>> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +
>> +    if (cpus == 0) {
>> +        sockets = sockets > 0 ? sockets : 1;
>>           cores = cores > 0 ? cores : 1;
>>           threads = threads > 0 ? threads : 1;
>> -        if (cpus == 0) {
>> -            sockets = sockets > 0 ? sockets : 1;
>> -            cpus = cores * threads * sockets;
>> -        } else {
>> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -            sockets = maxcpus / (cores * threads);
>> -            sockets = sockets > 0 ? sockets : 1;
>> -        }
>> +        cpus = sockets * cores * threads;
>> +        maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +    } else if (sockets == 0) {
>> +        cores = cores > 0 ? cores : 1;
>> +        threads = threads > 0 ? threads : 1;
>> +        sockets = maxcpus / (cores * threads);
>> +        sockets = sockets > 0 ? sockets : 1;
> As stated in the other patch, this rounding up of a fractional sockets
> shouldn't be here. maxcpus or (cpus==maxcpus) should always be selected by
> the user to be a product of whole number sockets, cores, threads.
I also default sockets to 1 if computed as zero because we are doing
this for cores/threads and I did't see the difference between them.

Anyway, now that we are using maxcpus for all calculations, so we can
either get rid of all the rounding-ups for sockets/cores/threads or just
keep them all.

With the rouding-up, we get:
-smp cpus=12,cores=16,maxcpus=12 --> -smp 
cpus=12,sockets=0,cores=16,threads=1,maxcpus=12 --> error_report

without the rounding-up we get:
-smp cpus=12,cores=16,maxcpus=12 --> -smp 
cpus=12,sockets=1,cores=16,threads=1,maxcpus=12 --> error_report

We will both get an error report as expected, but I can get rid of the 
rounding if it's preferred.
>>       } else if (cores == 0) {
>>           threads = threads > 0 ? threads : 1;
>> -        cores = cpus / (sockets * threads);
>> +        cores = maxcpus / (sockets * threads);
>>           cores = cores > 0 ? cores : 1;
> Now that we're using maxcpus for the calculation, then no rounding for
> cores either...
>
>>       } else if (threads == 0) {
>> -        threads = cpus / (cores * sockets);
>> +        threads = maxcpus / (sockets * cores);
>>           threads = threads > 0 ? threads : 1;
> ...or threads.
>
>> -    } else if (sockets * cores * threads < cpus) {
>> +    }
>> +
>> +    if (sockets * cores * threads < cpus) {
>>           error_setg(errp, "cpu topology: "
>> -                   "sockets (%u) * cores (%u) * threads (%u) < "
>> -                   "smp_cpus (%u)",
>> +                   "sockets (%u) * cores (%u) * threads (%u) "
>> +                   "< smp_cpus (%u)",
> Why make this change?
No need actually, will change it back. :)
>>                      sockets, cores, threads, cpus);
>>           return;
>>       }
>>   
>> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -
>>       if (maxcpus < cpus) {
>>           error_setg(errp, "maxcpus must be equal to or greater than smp");
>>           return;
>> @@ -795,9 +797,9 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>>       }
>>   
>>       ms->smp.cpus = cpus;
>> +    ms->smp.sockets = sockets;
>>       ms->smp.cores = cores;
>>       ms->smp.threads = threads;
>> -    ms->smp.sockets = sockets;
>>       ms->smp.max_cpus = maxcpus;
>>   }
>>   
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 93d1f12a49..1812f33ab1 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -721,35 +721,36 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>   
>>       /* compute missing values, prefer sockets over cores over threads */
>>       dies = dies > 0 ? dies : 1;
>> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>>   
>> -    if (cpus == 0 || sockets == 0) {
>> +    if (cpus == 0) {
>> +        sockets = sockets > 0 ? sockets : 1;
>>           cores = cores > 0 ? cores : 1;
>>           threads = threads > 0 ? threads : 1;
>> -        if (cpus == 0) {
>> -            sockets = sockets > 0 ? sockets : 1;
>> -            cpus = cores * threads * dies * sockets;
>> -        } else {
>> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -            sockets = maxcpus / (dies * cores * threads);
>> -            sockets = sockets > 0 ? sockets : 1;
>> -        }
>> +        cpus = sockets * dies * cores * threads;
>> +        maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> +    } else if (sockets == 0) {
>> +        cores = cores > 0 ? cores : 1;
>> +        threads = threads > 0 ? threads : 1;
>> +        sockets = maxcpus / (dies * cores * threads);
>> +        sockets = sockets > 0 ? sockets : 1;
>>       } else if (cores == 0) {
>>           threads = threads > 0 ? threads : 1;
>> -        cores = cpus / (sockets * dies * threads);
>> +        cores = maxcpus / (sockets * dies * threads);
>>           cores = cores > 0 ? cores : 1;
>>       } else if (threads == 0) {
>> -        threads = cpus / (cores * dies * sockets);
>> +        threads = maxcpus / (sockets * dies * cores);
>>           threads = threads > 0 ? threads : 1;
>> -    } else if (sockets * dies * cores * threads < cpus) {
>> +    }
>> +
>> +    if (sockets * dies * cores * threads < cpus) {
>>           error_setg(errp, "cpu topology: "
>> -                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
>> -                   "smp_cpus (%u)",
>> +                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
>> +                   "< smp_cpus (%u)",
>>                      sockets, dies, cores, threads, cpus);
>>           return;
>>       }
>>   
> Same comments as for the general function.
>
> Thanks,
> drew
>
>> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -
>>       if (maxcpus < cpus) {
>>           error_setg(errp, "maxcpus must be equal to or greater than smp");
>>           return;
>> @@ -765,10 +766,10 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>       }
>>   
>>       ms->smp.cpus = cpus;
>> -    ms->smp.cores = cores;
>> -    ms->smp.threads = threads;
>>       ms->smp.sockets = sockets;
>>       ms->smp.dies = dies;
>> +    ms->smp.cores = cores;
>> +    ms->smp.threads = threads;
>>       ms->smp.max_cpus = maxcpus;
>>   }
>>   
>> -- 
>> 2.19.1
>>
Thanks,
Yanan
.
> .

Re: [RFC PATCH 4/6] machine: Uniformly use maxcpus to calculate the missing values

[wangyanan (Y)]

On 2021/7/2 18:07, Yanan Wang wrote:
> We are currently using maxcpus to calculate value of sockets but using
> cpus to calculate value of cores/threads. This makes cmdlines like
> "-smp 8,maxcpus=12,cores=4" work while "-smp 8,maxcpus=12,sockets=3"
> break the invalid cpu topology check.
>
> This patch allows us to uniformly use maxcpus to calculate the missing
> values. Also the if branch of "cpus == 0 || sockets == 0" was splited
> into branches of "cpus == 0" and "sockets == 0" so that we can clearly
> figure out that we are parsing -smp cmdlines with a preference of cpus
> over sockets over cores over threads.
BTW, after this patch, configs like:
-smp maxcpus=16 --> -smp 1,sockets=1,cores=1,threads=1,maxcpus=16
-smp sockets=2,maxcpus=16 --> -smp 2,sockets=2,cores=1,threads=1,maxcpus=16
......
still can not work. So I suggest to use the parameters computed by 
maxcpus to calculate
the value of cpus if it's omitted. The diff based on this patch is:

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 93c5227920..924c48fd43 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -757,13 +757,7 @@ static void smp_parse(MachineState *ms, 
SMPConfiguration *config, Error **errp)
      /* compute missing values, prefer sockets over cores over threads */
      maxcpus = maxcpus > 0 ? maxcpus : cpus;

-    if (cpus == 0) {
-        sockets = sockets > 0 ? sockets : 1;
-        cores = cores > 0 ? cores : 1;
-        threads = threads > 0 ? threads : 1;
-        cpus = sockets * cores * threads;
-        maxcpus = maxcpus > 0 ? maxcpus : cpus;
-    } else if (sockets == 0) {
+    if (sockets == 0) {
          cores = cores > 0 ? cores : 1;
          threads = threads > 0 ? threads : 1;
          sockets = maxcpus / (cores * threads);
@@ -777,6 +771,9 @@ static void smp_parse(MachineState *ms, 
SMPConfiguration *config, Error **errp)
          threads = threads > 0 ? threads : 1;
      }

+    cpus = cpus > 0 ? cpus : sockets * cores * threads;
+    maxcpus = maxcpus > 0 ? maxcpus : cpus;
+
      if (sockets * cores * threads < cpus) {
          error_setg(errp, "cpu topology: "
                     "sockets (%u) * cores (%u) * threads (%u) < "


Thanks,
Yanan
.
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>   hw/core/machine.c | 36 +++++++++++++++++++-----------------
>   hw/i386/pc.c      | 37 +++++++++++++++++++------------------
>   2 files changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1e194677cd..58882835be 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -752,34 +752,36 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>       }
>   
>       /* compute missing values, prefer sockets over cores over threads */
> -    if (cpus == 0 || sockets == 0) {
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +
> +    if (cpus == 0) {
> +        sockets = sockets > 0 ? sockets : 1;
>           cores = cores > 0 ? cores : 1;
>           threads = threads > 0 ? threads : 1;
> -        if (cpus == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
> -            cpus = cores * threads * sockets;
> -        } else {
> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -            sockets = maxcpus / (cores * threads);
> -            sockets = sockets > 0 ? sockets : 1;
> -        }
> +        cpus = sockets * cores * threads;
> +        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +    } else if (sockets == 0) {
> +        cores = cores > 0 ? cores : 1;
> +        threads = threads > 0 ? threads : 1;
> +        sockets = maxcpus / (cores * threads);
> +        sockets = sockets > 0 ? sockets : 1;
>       } else if (cores == 0) {
>           threads = threads > 0 ? threads : 1;
> -        cores = cpus / (sockets * threads);
> +        cores = maxcpus / (sockets * threads);
>           cores = cores > 0 ? cores : 1;
>       } else if (threads == 0) {
> -        threads = cpus / (cores * sockets);
> +        threads = maxcpus / (sockets * cores);
>           threads = threads > 0 ? threads : 1;
> -    } else if (sockets * cores * threads < cpus) {
> +    }
> +
> +    if (sockets * cores * threads < cpus) {
>           error_setg(errp, "cpu topology: "
> -                   "sockets (%u) * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> +                   "sockets (%u) * cores (%u) * threads (%u) "
> +                   "< smp_cpus (%u)",
>                      sockets, cores, threads, cpus);
>           return;
>       }
>   
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
>       if (maxcpus < cpus) {
>           error_setg(errp, "maxcpus must be equal to or greater than smp");
>           return;
> @@ -795,9 +797,9 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>       }
>   
>       ms->smp.cpus = cpus;
> +    ms->smp.sockets = sockets;
>       ms->smp.cores = cores;
>       ms->smp.threads = threads;
> -    ms->smp.sockets = sockets;
>       ms->smp.max_cpus = maxcpus;
>   }
>   
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 93d1f12a49..1812f33ab1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -721,35 +721,36 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>   
>       /* compute missing values, prefer sockets over cores over threads */
>       dies = dies > 0 ? dies : 1;
> +    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>   
> -    if (cpus == 0 || sockets == 0) {
> +    if (cpus == 0) {
> +        sockets = sockets > 0 ? sockets : 1;
>           cores = cores > 0 ? cores : 1;
>           threads = threads > 0 ? threads : 1;
> -        if (cpus == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
> -            cpus = cores * threads * dies * sockets;
> -        } else {
> -            maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -            sockets = maxcpus / (dies * cores * threads);
> -            sockets = sockets > 0 ? sockets : 1;
> -        }
> +        cpus = sockets * dies * cores * threads;
> +        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> +    } else if (sockets == 0) {
> +        cores = cores > 0 ? cores : 1;
> +        threads = threads > 0 ? threads : 1;
> +        sockets = maxcpus / (dies * cores * threads);
> +        sockets = sockets > 0 ? sockets : 1;
>       } else if (cores == 0) {
>           threads = threads > 0 ? threads : 1;
> -        cores = cpus / (sockets * dies * threads);
> +        cores = maxcpus / (sockets * dies * threads);
>           cores = cores > 0 ? cores : 1;
>       } else if (threads == 0) {
> -        threads = cpus / (cores * dies * sockets);
> +        threads = maxcpus / (sockets * dies * cores);
>           threads = threads > 0 ? threads : 1;
> -    } else if (sockets * dies * cores * threads < cpus) {
> +    }
> +
> +    if (sockets * dies * cores * threads < cpus) {
>           error_setg(errp, "cpu topology: "
> -                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> +                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> +                   "< smp_cpus (%u)",
>                      sockets, dies, cores, threads, cpus);
>           return;
>       }
>   
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
>       if (maxcpus < cpus) {
>           error_setg(errp, "maxcpus must be equal to or greater than smp");
>           return;
> @@ -765,10 +766,10 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>       }
>   
>       ms->smp.cpus = cpus;
> -    ms->smp.cores = cores;
> -    ms->smp.threads = threads;
>       ms->smp.sockets = sockets;
>       ms->smp.dies = dies;
> +    ms->smp.cores = cores;
> +    ms->smp.threads = threads;
>       ms->smp.max_cpus = maxcpus;
>   }
>