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

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
> 

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 :|