[PATCH for-6.2 v3 00/11] machine: smp parsing fixes and improvement

[PATCH for-6.2 v3 00/11] machine: smp parsing fixes and improvement

[Yanan Wang]

Hi,

This is new version of the series [1] that I have posted to introduce
some fixes and improvement for SMP parsing.

[1] https://lore.kernel.org/qemu-devel/20210719032043.25416-1-wangyanan55@huawei.com/

Most of this series is about the SMP parsers:
maxcpus is now uniformly used to calculate the omitted topology members,
calculation of omitted maxcpus/cpus is improved, the error reporting is
improved. It's also suggested that we should start to prefer cores over
sockets over threads on the newer machine types, which will make the
computed virtual topology more reflective of the real hardware.

In order to reduce code duplication and ease the code maintenance, smp_parse
in now converted into a parser generic enough for all arches, so that the PC
specific one can be removed. It's also convenient to introduce more topology
members to the generic parser in the future.

---

Changelogs:

v2->v3:
- apply the calculation improvement to smp_parse and pc_smp_parse
  separately and then convert the finally improved parsers into a
  generic one, so that patches can be reviewed separately.
- to ease review, drop the unit test part for a while until we have
  a good enough generic parser.
- send the patch "machine: Disallow specifying topology parameters as zero"
  for 6.1 separately.
- v2: https://lore.kernel.org/qemu-devel/20210719032043.25416-1-wangyanan55@huawei.com/

v1->v2:
- disallow "anything=0" in the smp configuration (Andrew)
- make function smp_parse() a generic helper for all arches
- improve the error reporting in the parser
- start to prefer cores over sockets since 6.2 (Daniel)
- add a unit test for the smp parsing (Daniel)
- v1: https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg00259.html

---

Yanan Wang (11):
  machine: Minor refactor/cleanup for the smp parsers
  machine: Uniformly use maxcpus to calculate the omitted parameters
  machine: Set the value of cpus to match maxcpus if it's omitted
  machine: Improve the error reporting of smp parsing
  hw: Add compat machines for 6.2
  machine: Prefer cores over sockets in smp parsing since 6.2
  machine: Use ms instead of global current_machine in sanity-check
  machine: Tweak the order of topology members in struct CpuTopology
  machine: Make smp_parse generic enough for all arches
  machine: Remove smp_parse callback from MachineClass
  machine: Move smp_prefer_sockets to struct SMPCompatProps

 hw/arm/virt.c              |  10 ++-
 hw/core/machine.c          | 168 ++++++++++++++++++++++++++++---------
 hw/i386/pc.c               |  66 +--------------
 hw/i386/pc_piix.c          |  15 +++-
 hw/i386/pc_q35.c           |  14 +++-
 hw/ppc/spapr.c             |  16 +++-
 hw/s390x/s390-virtio-ccw.c |  15 +++-
 include/hw/boards.h        |  26 ++++--
 include/hw/i386/pc.h       |   3 +
 qemu-options.hx            |  14 +++-
 10 files changed, 228 insertions(+), 119 deletions(-)

--
2.19.1

[PATCH for-6.2 v3 01/11] machine: Minor refactor/cleanup for the smp parsers

[Yanan Wang]

To pave the way for the functional improvement in later patches,
make some refactor/cleanup for the smp parsers, including using
local maxcpus instead of ms->smp.max_cpus in the calculation,
defaulting dies to 0 initially like other members, cleanup the
sanity check for dies.

No functional change intended.

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

diff --git a/hw/core/machine.c b/hw/core/machine.c
index e1533dfc47..ffc0629854 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -747,9 +747,11 @@ 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) {
+    if (config->has_dies && config->dies > 1) {
         error_setg(errp, "dies not supported by this machine's CPU topology");
+        return;
     }
 
     /* compute missing values, prefer sockets over cores over threads */
@@ -760,8 +762,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 / (sockets * cores);
         }
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
@@ -778,26 +780,27 @@ 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;
     }
 
     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;
 }
 
 static void machine_get_smp(Object *obj, Visitor *v, const char *name,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c2b9d62a35..acd31af452 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -716,9 +716,13 @@ 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;
+
+    /* directly default dies to 1 if it's omitted */
+    dies = dies > 0 ? dies : 1;
 
     /* compute missing values, prefer sockets over cores over threads */
     if (cpus == 0 || sockets == 0) {
@@ -728,8 +732,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;
@@ -746,27 +750,28 @@ 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;
     }
 
     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.maxcpus = maxcpus;
 }
 
 static
-- 
2.19.1

Re: [PATCH for-6.2 v3 01/11] machine: Minor refactor/cleanup for the smp parsers

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:38AM +0800, Yanan Wang wrote:
> To pave the way for the functional improvement in later patches,
> make some refactor/cleanup for the smp parsers, including using
> local maxcpus instead of ms->smp.max_cpus in the calculation,
> defaulting dies to 0 initially like other members, cleanup the
> sanity check for dies.
> 
> No functional change intended.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 19 +++++++++++--------
>  hw/i386/pc.c      | 23 ++++++++++++++---------
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e1533dfc47..ffc0629854 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -747,9 +747,11 @@ 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) {
> +    if (config->has_dies && config->dies > 1) {
>          error_setg(errp, "dies not supported by this machine's CPU topology");
> +        return;
>      }
>  
>      /* compute missing values, prefer sockets over cores over threads */
> @@ -760,8 +762,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 / (sockets * cores);

Should be divided by (cores * threads) like before.

Thanks,
drew

Re: [PATCH for-6.2 v3 01/11] machine: Minor refactor/cleanup for the smp parsers

[wangyanan (Y)]

On 2021/7/29 4:16, Andrew Jones wrote:
> On Wed, Jul 28, 2021 at 11:48:38AM +0800, Yanan Wang wrote:
>> To pave the way for the functional improvement in later patches,
>> make some refactor/cleanup for the smp parsers, including using
>> local maxcpus instead of ms->smp.max_cpus in the calculation,
>> defaulting dies to 0 initially like other members, cleanup the
>> sanity check for dies.
>>
>> No functional change intended.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/core/machine.c | 19 +++++++++++--------
>>   hw/i386/pc.c      | 23 ++++++++++++++---------
>>   2 files changed, 25 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index e1533dfc47..ffc0629854 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -747,9 +747,11 @@ 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) {
>> +    if (config->has_dies && config->dies > 1) {
>>           error_setg(errp, "dies not supported by this machine's CPU topology");
>> +        return;
>>       }
>>   
>>       /* compute missing values, prefer sockets over cores over threads */
>> @@ -760,8 +762,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 / (sockets * cores);
> Should be divided by (cores * threads) like before.
Absolutely... Will fix it.

Thanks,
Yanan
> Thanks,
> drew
>
> .

[PATCH for-6.2 v3 02/11] machine: Uniformly use maxcpus to calculate the omitted parameters

[Yanan Wang]

We are currently using maxcpus to calculate the omitted sockets
but using cpus to calculate the omitted cores/threads. This makes
cmdlines like:
  -smp cpus=8,maxcpus=16
  -smp cpus=8,cores=4,maxcpus=16
  -smp cpus=8,threads=2,maxcpus=16
work fine but the ones like:
  -smp cpus=8,sockets=2,maxcpus=16
  -smp cpus=8,sockets=2,cores=4,maxcpus=16
  -smp cpus=8,sockets=2,threads=2,maxcpus=16
break the sanity check.

Since we require for a valid config that the product of "sockets * cores
* threads" should equal to the maxcpus, we should uniformly use maxcpus
to calculate their omitted values.

Also the if-branch of "cpus == 0 || sockets == 0" was split into two
branches of "cpus == 0" and "sockets == 0" so that we can clearly read
that we are parsing the configuration with a preference on cpus over
sockets over cores over threads.

Note: change in this patch won't affect any existing working cmdlines
but improves consistency and allows more incomplete configs to be valid.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 30 +++++++++++++++---------------
 hw/i386/pc.c      | 30 +++++++++++++++---------------
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ffc0629854..69979c93dd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -755,24 +755,26 @@ 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 / (sockets * cores);
-        }
+        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);
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
-        cores = cpus / (sockets * threads);
-        cores = cores > 0 ? cores : 1;
+        cores = maxcpus / (sockets * threads);
     } else if (threads == 0) {
-        threads = cpus / (cores * sockets);
-        threads = threads > 0 ? threads : 1;
-    } else if (sockets * cores * threads < cpus) {
+        threads = maxcpus / (sockets * cores);
+    }
+
+    if (sockets * cores * threads < cpus) {
         error_setg(errp, "cpu topology: "
                    "sockets (%u) * cores (%u) * threads (%u) < "
                    "smp_cpus (%u)",
@@ -780,8 +782,6 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
         return;
     }
 
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
     if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index acd31af452..a9ff9ef52c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -725,24 +725,26 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     dies = dies > 0 ? dies : 1;
 
     /* 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 * dies * sockets;
-        } else {
-            maxcpus = maxcpus > 0 ? maxcpus : cpus;
-            sockets = maxcpus / (dies * cores * threads);
-        }
+        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);
     } else if (cores == 0) {
         threads = threads > 0 ? threads : 1;
-        cores = cpus / (sockets * dies * threads);
-        cores = cores > 0 ? cores : 1;
+        cores = maxcpus / (sockets * dies * threads);
     } else if (threads == 0) {
-        threads = cpus / (cores * dies * sockets);
-        threads = threads > 0 ? threads : 1;
-    } else if (sockets * dies * cores * threads < cpus) {
+        threads = maxcpus / (sockets * dies * cores);
+    }
+
+    if (sockets * dies * cores * threads < cpus) {
         error_setg(errp, "cpu topology: "
                    "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
                    "smp_cpus (%u)",
@@ -750,8 +752,6 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
         return;
     }
 
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
     if (maxcpus < cpus) {
         error_setg(errp, "maxcpus must be equal to or greater than smp");
         return;
-- 
2.19.1

[PATCH for-6.2 v3 03/11] machine: Set the value of cpus to match maxcpus if it's omitted

[Yanan Wang]

Currently we directly calculate the omitted cpus based on the given
incomplete collection of parameters. This makes some cmdlines like:
  -smp maxcpus=16
  -smp sockets=2,maxcpus=16
  -smp sockets=2,dies=2,maxcpus=16
  -smp sockets=2,cores=4,maxcpus=16
not work. We should probably set the value of cpus to match maxcpus
if it's omitted, which will make above configs start to work.

So the calculation logic of cpus/maxcpus after this patch will be:
When both maxcpus and cpus are omitted, maxcpus will be calculated
from the given parameters and cpus will be set equal to maxcpus.
When only one of maxcpus and cpus is given then the omitted one
will be set to its counterpart's value. Both maxcpus and cpus may
be specified, but maxcpus must be equal to or greater than cpus.

Note: change in this patch won't affect any existing working cmdlines
but allows more incomplete configs to be valid.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 29 ++++++++++++++++-------------
 hw/i386/pc.c      | 29 ++++++++++++++++-------------
 qemu-options.hx   | 11 ++++++++---
 3 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 69979c93dd..958e6e7107 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -755,25 +755,28 @@ 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) {
+    if (cpus == 0 && maxcpus == 0) {
         sockets = sockets > 0 ? sockets : 1;
         cores = cores > 0 ? cores : 1;
         threads = threads > 0 ? threads : 1;
-        cpus = sockets * cores * threads;
+    } else {
         maxcpus = maxcpus > 0 ? maxcpus : cpus;
-    } else if (sockets == 0) {
-        cores = cores > 0 ? cores : 1;
-        threads = threads > 0 ? threads : 1;
-        sockets = maxcpus / (cores * threads);
-    } else if (cores == 0) {
-        threads = threads > 0 ? threads : 1;
-        cores = maxcpus / (sockets * threads);
-    } else if (threads == 0) {
-        threads = maxcpus / (sockets * cores);
+
+        if (sockets == 0) {
+            cores = cores > 0 ? cores : 1;
+            threads = threads > 0 ? threads : 1;
+            sockets = maxcpus / (cores * threads);
+        } else if (cores == 0) {
+            threads = threads > 0 ? threads : 1;
+            cores = maxcpus / (sockets * threads);
+        } else if (threads == 0) {
+            threads = maxcpus / (sockets * cores);
+        }
     }
 
+    maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
+    cpus = cpus > 0 ? cpus : maxcpus;
+
     if (sockets * cores * threads < cpus) {
         error_setg(errp, "cpu topology: "
                    "sockets (%u) * cores (%u) * threads (%u) < "
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a9ff9ef52c..9ad7ae5254 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -725,25 +725,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     dies = dies > 0 ? dies : 1;
 
     /* compute missing values, prefer sockets over cores over threads */
-    maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
-    if (cpus == 0) {
+    if (cpus == 0 && maxcpus == 0) {
         sockets = sockets > 0 ? sockets : 1;
         cores = cores > 0 ? cores : 1;
         threads = threads > 0 ? threads : 1;
-        cpus = sockets * dies * cores * threads;
+    } else {
         maxcpus = maxcpus > 0 ? maxcpus : cpus;
-    } else if (sockets == 0) {
-        cores = cores > 0 ? cores : 1;
-        threads = threads > 0 ? threads : 1;
-        sockets = maxcpus / (dies * cores * threads);
-    } else if (cores == 0) {
-        threads = threads > 0 ? threads : 1;
-        cores = maxcpus / (sockets * dies * threads);
-    } else if (threads == 0) {
-        threads = maxcpus / (sockets * dies * cores);
+
+        if (sockets == 0) {
+            cores = cores > 0 ? cores : 1;
+            threads = threads > 0 ? threads : 1;
+            sockets = maxcpus / (dies * cores * threads);
+        } else if (cores == 0) {
+            threads = threads > 0 ? threads : 1;
+            cores = maxcpus / (sockets * dies * threads);
+        } else if (threads == 0) {
+            threads = maxcpus / (sockets * dies * cores);
+        }
     }
 
+    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
+    cpus = cpus > 0 ? cpus : maxcpus;
+
     if (sockets * dies * cores * threads < cpus) {
         error_setg(errp, "cpu topology: "
                    "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
diff --git a/qemu-options.hx b/qemu-options.hx
index e077aa80bf..0912236b4b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -210,9 +210,14 @@ SRST
     Simulate a SMP system with '\ ``n``\ ' CPUs initially present on
     the machine type board. On boards supporting CPU hotplug, the optional
     '\ ``maxcpus``\ ' parameter can be set to enable further CPUs to be
-    added at runtime. If omitted the maximum number of CPUs will be
-    set to match the initial CPU count. Both parameters are subject to
-    an upper limit that is determined by the specific machine type chosen.
+    added at runtime. When both parameters are omitted, the maximum number
+    of CPUs will be calculated from the provided topology members and the
+    initial CPU count will match the maximum number. When only one of them
+    is given then the omitted one will be set to its counterpart's value.
+    Both parameters may be specified, but the maximum number of CPUs must
+    equal to or greater than the initial CPU count. Both parameters are
+    subject to an upper limit that is determined by the specific machine
+    type chosen.
 
     To control reporting of CPU topology information, the number of sockets,
     dies per socket, cores per die, and threads per core can be specified.
-- 
2.19.1

Re: [PATCH for-6.2 v3 03/11] machine: Set the value of cpus to match maxcpus if it's omitted

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:40AM +0800, Yanan Wang wrote:
> Currently we directly calculate the omitted cpus based on the given
> incomplete collection of parameters. This makes some cmdlines like:
>   -smp maxcpus=16
>   -smp sockets=2,maxcpus=16
>   -smp sockets=2,dies=2,maxcpus=16
>   -smp sockets=2,cores=4,maxcpus=16
> not work. We should probably set the value of cpus to match maxcpus
> if it's omitted, which will make above configs start to work.
> 
> So the calculation logic of cpus/maxcpus after this patch will be:
> When both maxcpus and cpus are omitted, maxcpus will be calculated
> from the given parameters and cpus will be set equal to maxcpus.
> When only one of maxcpus and cpus is given then the omitted one
> will be set to its counterpart's value. Both maxcpus and cpus may
> be specified, but maxcpus must be equal to or greater than cpus.
> 
> Note: change in this patch won't affect any existing working cmdlines
> but allows more incomplete configs to be valid.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 29 ++++++++++++++++-------------
>  hw/i386/pc.c      | 29 ++++++++++++++++-------------
>  qemu-options.hx   | 11 ++++++++---
>  3 files changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 69979c93dd..958e6e7107 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -755,25 +755,28 @@ 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) {
> +    if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -        cpus = sockets * cores * threads;
> +    } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -    } else if (sockets == 0) {
> -        cores = cores > 0 ? cores : 1;
> -        threads = threads > 0 ? threads : 1;
> -        sockets = maxcpus / (cores * threads);
> -    } else if (cores == 0) {
> -        threads = threads > 0 ? threads : 1;
> -        cores = maxcpus / (sockets * threads);
> -    } else if (threads == 0) {
> -        threads = maxcpus / (sockets * cores);
> +
> +        if (sockets == 0) {
> +            cores = cores > 0 ? cores : 1;
> +            threads = threads > 0 ? threads : 1;
> +            sockets = maxcpus / (cores * threads);
> +        } else if (cores == 0) {
> +            threads = threads > 0 ? threads : 1;
> +            cores = maxcpus / (sockets * threads);
> +        } else if (threads == 0) {
> +            threads = maxcpus / (sockets * cores);
> +        }
>      }
>  
> +    maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
> +    cpus = cpus > 0 ? cpus : maxcpus;
> +
>      if (sockets * cores * threads < cpus) {
>          error_setg(errp, "cpu topology: "
>                     "sockets (%u) * cores (%u) * threads (%u) < "
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a9ff9ef52c..9ad7ae5254 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -725,25 +725,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      dies = dies > 0 ? dies : 1;
>  
>      /* compute missing values, prefer sockets over cores over threads */
> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
> -    if (cpus == 0) {
> +    if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
>          threads = threads > 0 ? threads : 1;
> -        cpus = sockets * dies * cores * threads;
> +    } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -    } else if (sockets == 0) {
> -        cores = cores > 0 ? cores : 1;
> -        threads = threads > 0 ? threads : 1;
> -        sockets = maxcpus / (dies * cores * threads);
> -    } else if (cores == 0) {
> -        threads = threads > 0 ? threads : 1;
> -        cores = maxcpus / (sockets * dies * threads);
> -    } else if (threads == 0) {
> -        threads = maxcpus / (sockets * dies * cores);
> +
> +        if (sockets == 0) {
> +            cores = cores > 0 ? cores : 1;
> +            threads = threads > 0 ? threads : 1;
> +            sockets = maxcpus / (dies * cores * threads);
> +        } else if (cores == 0) {
> +            threads = threads > 0 ? threads : 1;
> +            cores = maxcpus / (sockets * dies * threads);
> +        } else if (threads == 0) {
> +            threads = maxcpus / (sockets * dies * cores);
> +        }
>      }
>  
> +    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
> +    cpus = cpus > 0 ? cpus : maxcpus;
> +
>      if (sockets * dies * cores * threads < cpus) {
>          error_setg(errp, "cpu topology: "
>                     "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> diff --git a/qemu-options.hx b/qemu-options.hx
> index e077aa80bf..0912236b4b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -210,9 +210,14 @@ SRST
>      Simulate a SMP system with '\ ``n``\ ' CPUs initially present on
>      the machine type board. On boards supporting CPU hotplug, the optional
>      '\ ``maxcpus``\ ' parameter can be set to enable further CPUs to be
> -    added at runtime. If omitted the maximum number of CPUs will be
> -    set to match the initial CPU count. Both parameters are subject to
> -    an upper limit that is determined by the specific machine type chosen.
> +    added at runtime. When both parameters are omitted, the maximum number
> +    of CPUs will be calculated from the provided topology members and the
> +    initial CPU count will match the maximum number. When only one of them
> +    is given then the omitted one will be set to its counterpart's value.
> +    Both parameters may be specified, but the maximum number of CPUs must
> +    equal to or greater than the initial CPU count. Both parameters are

be equal to

> +    subject to an upper limit that is determined by the specific machine
> +    type chosen.
>  
>      To control reporting of CPU topology information, the number of sockets,
>      dies per socket, cores per die, and threads per core can be specified.
> -- 
> 2.19.1
>

Otherwise

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

Re: [PATCH for-6.2 v3 03/11] machine: Set the value of cpus to match maxcpus if it's omitted

[wangyanan (Y)]

On 2021/7/29 4:22, Andrew Jones wrote:
> On Wed, Jul 28, 2021 at 11:48:40AM +0800, Yanan Wang wrote:
>> Currently we directly calculate the omitted cpus based on the given
>> incomplete collection of parameters. This makes some cmdlines like:
>>    -smp maxcpus=16
>>    -smp sockets=2,maxcpus=16
>>    -smp sockets=2,dies=2,maxcpus=16
>>    -smp sockets=2,cores=4,maxcpus=16
>> not work. We should probably set the value of cpus to match maxcpus
>> if it's omitted, which will make above configs start to work.
>>
>> So the calculation logic of cpus/maxcpus after this patch will be:
>> When both maxcpus and cpus are omitted, maxcpus will be calculated
>> from the given parameters and cpus will be set equal to maxcpus.
>> When only one of maxcpus and cpus is given then the omitted one
>> will be set to its counterpart's value. Both maxcpus and cpus may
>> be specified, but maxcpus must be equal to or greater than cpus.
>>
>> Note: change in this patch won't affect any existing working cmdlines
>> but allows more incomplete configs to be valid.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> ---
>>   hw/core/machine.c | 29 ++++++++++++++++-------------
>>   hw/i386/pc.c      | 29 ++++++++++++++++-------------
>>   qemu-options.hx   | 11 ++++++++---
>>   3 files changed, 40 insertions(+), 29 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 69979c93dd..958e6e7107 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -755,25 +755,28 @@ 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) {
>> +    if (cpus == 0 && maxcpus == 0) {
>>           sockets = sockets > 0 ? sockets : 1;
>>           cores = cores > 0 ? cores : 1;
>>           threads = threads > 0 ? threads : 1;
>> -        cpus = sockets * cores * threads;
>> +    } else {
>>           maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -    } else if (sockets == 0) {
>> -        cores = cores > 0 ? cores : 1;
>> -        threads = threads > 0 ? threads : 1;
>> -        sockets = maxcpus / (cores * threads);
>> -    } else if (cores == 0) {
>> -        threads = threads > 0 ? threads : 1;
>> -        cores = maxcpus / (sockets * threads);
>> -    } else if (threads == 0) {
>> -        threads = maxcpus / (sockets * cores);
>> +
>> +        if (sockets == 0) {
>> +            cores = cores > 0 ? cores : 1;
>> +            threads = threads > 0 ? threads : 1;
>> +            sockets = maxcpus / (cores * threads);
>> +        } else if (cores == 0) {
>> +            threads = threads > 0 ? threads : 1;
>> +            cores = maxcpus / (sockets * threads);
>> +        } else if (threads == 0) {
>> +            threads = maxcpus / (sockets * cores);
>> +        }
>>       }
>>   
>> +    maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
>> +    cpus = cpus > 0 ? cpus : maxcpus;
>> +
>>       if (sockets * cores * threads < cpus) {
>>           error_setg(errp, "cpu topology: "
>>                      "sockets (%u) * cores (%u) * threads (%u) < "
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index a9ff9ef52c..9ad7ae5254 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -725,25 +725,28 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>>       dies = dies > 0 ? dies : 1;
>>   
>>       /* compute missing values, prefer sockets over cores over threads */
>> -    maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -
>> -    if (cpus == 0) {
>> +    if (cpus == 0 && maxcpus == 0) {
>>           sockets = sockets > 0 ? sockets : 1;
>>           cores = cores > 0 ? cores : 1;
>>           threads = threads > 0 ? threads : 1;
>> -        cpus = sockets * dies * cores * threads;
>> +    } else {
>>           maxcpus = maxcpus > 0 ? maxcpus : cpus;
>> -    } else if (sockets == 0) {
>> -        cores = cores > 0 ? cores : 1;
>> -        threads = threads > 0 ? threads : 1;
>> -        sockets = maxcpus / (dies * cores * threads);
>> -    } else if (cores == 0) {
>> -        threads = threads > 0 ? threads : 1;
>> -        cores = maxcpus / (sockets * dies * threads);
>> -    } else if (threads == 0) {
>> -        threads = maxcpus / (sockets * dies * cores);
>> +
>> +        if (sockets == 0) {
>> +            cores = cores > 0 ? cores : 1;
>> +            threads = threads > 0 ? threads : 1;
>> +            sockets = maxcpus / (dies * cores * threads);
>> +        } else if (cores == 0) {
>> +            threads = threads > 0 ? threads : 1;
>> +            cores = maxcpus / (sockets * dies * threads);
>> +        } else if (threads == 0) {
>> +            threads = maxcpus / (sockets * dies * cores);
>> +        }
>>       }
>>   
>> +    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
>> +    cpus = cpus > 0 ? cpus : maxcpus;
>> +
>>       if (sockets * dies * cores * threads < cpus) {
>>           error_setg(errp, "cpu topology: "
>>                      "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index e077aa80bf..0912236b4b 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -210,9 +210,14 @@ SRST
>>       Simulate a SMP system with '\ ``n``\ ' CPUs initially present on
>>       the machine type board. On boards supporting CPU hotplug, the optional
>>       '\ ``maxcpus``\ ' parameter can be set to enable further CPUs to be
>> -    added at runtime. If omitted the maximum number of CPUs will be
>> -    set to match the initial CPU count. Both parameters are subject to
>> -    an upper limit that is determined by the specific machine type chosen.
>> +    added at runtime. When both parameters are omitted, the maximum number
>> +    of CPUs will be calculated from the provided topology members and the
>> +    initial CPU count will match the maximum number. When only one of them
>> +    is given then the omitted one will be set to its counterpart's value.
>> +    Both parameters may be specified, but the maximum number of CPUs must
>> +    equal to or greater than the initial CPU count. Both parameters are
> be equal to
Got it, will fix.

Thanks,
Yanan
>> +    subject to an upper limit that is determined by the specific machine
>> +    type chosen.
>>   
>>       To control reporting of CPU topology information, the number of sockets,
>>       dies per socket, cores per die, and threads per core can be specified.
>> -- 
>> 2.19.1
>>
> Otherwise
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
>
> .

[PATCH for-6.2 v3 04/11] machine: Improve the error reporting of smp parsing

[Yanan Wang]

We have two requirements for a valid SMP configuration:
the product of "sockets * cores * threads" must represent all the
possible cpus, i.e., max_cpus, and then must include the initially
present cpus, i.e., smp_cpus.

So we only need to ensure 1) "sockets * cores * threads == maxcpus"
at first and then ensure 2) "maxcpus >= cpus". With a reasonable
order of the sanity check, we can simplify the error reporting code.
When reporting an error message we also report the exact value of
each topology member to make users easily see what's going on.

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

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 958e6e7107..e879163c3b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -777,25 +777,21 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
     cpus = cpus > 0 ? cpus : maxcpus;
 
-    if (sockets * cores * threads < cpus) {
-        error_setg(errp, "cpu topology: "
-                   "sockets (%u) * cores (%u) * threads (%u) < "
-                   "smp_cpus (%u)",
-                   sockets, cores, threads, cpus);
+    if (sockets * cores * threads != maxcpus) {
+        error_setg(errp, "Invalid CPU topology: "
+                   "product of the hierarchy must match maxcpus: "
+                   "sockets (%u) * cores (%u) * threads (%u) "
+                   "!= maxcpus (%u)",
+                   sockets, cores, threads, maxcpus);
         return;
     }
 
     if (maxcpus < cpus) {
-        error_setg(errp, "maxcpus must be equal to or greater than smp");
-        return;
-    }
-
-    if (sockets * cores * threads != maxcpus) {
         error_setg(errp, "Invalid CPU topology: "
+                   "maxcpus must be equal to or greater than smp: "
                    "sockets (%u) * cores (%u) * threads (%u) "
-                   "!= maxcpus (%u)",
-                   sockets, cores, threads,
-                   maxcpus);
+                   "== maxcpus (%u) < smp_cpus (%u)",
+                   sockets, cores, threads, maxcpus, cpus);
         return;
     }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 9ad7ae5254..3e403a7129 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -747,25 +747,21 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
     cpus = cpus > 0 ? cpus : maxcpus;
 
-    if (sockets * dies * cores * threads < cpus) {
-        error_setg(errp, "cpu topology: "
-                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
-                   "smp_cpus (%u)",
-                   sockets, dies, cores, threads, cpus);
+    if (sockets * cores * threads != maxcpus) {
+        error_setg(errp, "Invalid CPU topology: "
+                   "product of the hierarchy must match maxcpus: "
+                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
+                   "!= maxcpus (%u)",
+                   sockets, dies, cores, threads, maxcpus);
         return;
     }
 
     if (maxcpus < cpus) {
-        error_setg(errp, "maxcpus must be equal to or greater than smp");
-        return;
-    }
-
-    if (sockets * dies * cores * threads != maxcpus) {
-        error_setg(errp, "Invalid CPU topology deprecated: "
+        error_setg(errp, "Invalid CPU topology: "
+                   "maxcpus must be equal to or greater than smp: "
                    "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
-                   "!= maxcpus (%u)",
-                   sockets, dies, cores, threads,
-                   maxcpus);
+                   "== maxcpus (%u) < smp_cpus (%u)",
+                   sockets, dies, cores, threads, maxcpus, cpus);
         return;
     }
 
-- 
2.19.1

Re: [PATCH for-6.2 v3 04/11] machine: Improve the error reporting of smp parsing

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:41AM +0800, Yanan Wang wrote:
> We have two requirements for a valid SMP configuration:
> the product of "sockets * cores * threads" must represent all the
> possible cpus, i.e., max_cpus, and then must include the initially
> present cpus, i.e., smp_cpus.
> 
> So we only need to ensure 1) "sockets * cores * threads == maxcpus"
> at first and then ensure 2) "maxcpus >= cpus". With a reasonable
> order of the sanity check, we can simplify the error reporting code.
> When reporting an error message we also report the exact value of
> each topology member to make users easily see what's going on.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 22 +++++++++-------------
>  hw/i386/pc.c      | 24 ++++++++++--------------
>  2 files changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 958e6e7107..e879163c3b 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -777,25 +777,21 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
>      cpus = cpus > 0 ? cpus : maxcpus;
>  
> -    if (sockets * cores * threads < cpus) {
> -        error_setg(errp, "cpu topology: "
> -                   "sockets (%u) * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> -                   sockets, cores, threads, cpus);
> +    if (sockets * cores * threads != maxcpus) {
> +        error_setg(errp, "Invalid CPU topology: "
> +                   "product of the hierarchy must match maxcpus: "
> +                   "sockets (%u) * cores (%u) * threads (%u) "
> +                   "!= maxcpus (%u)",
> +                   sockets, cores, threads, maxcpus);
>          return;
>      }
>  
>      if (maxcpus < cpus) {
> -        error_setg(errp, "maxcpus must be equal to or greater than smp");
> -        return;
> -    }
> -
> -    if (sockets * cores * threads != maxcpus) {
>          error_setg(errp, "Invalid CPU topology: "
> +                   "maxcpus must be equal to or greater than smp: "
>                     "sockets (%u) * cores (%u) * threads (%u) "
> -                   "!= maxcpus (%u)",
> -                   sockets, cores, threads,
> -                   maxcpus);
> +                   "== maxcpus (%u) < smp_cpus (%u)",
> +                   sockets, cores, threads, maxcpus, cpus);
>          return;
>      }
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9ad7ae5254..3e403a7129 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -747,25 +747,21 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
>      cpus = cpus > 0 ? cpus : maxcpus;
>  
> -    if (sockets * dies * cores * threads < cpus) {
> -        error_setg(errp, "cpu topology: "
> -                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
> -                   "smp_cpus (%u)",
> -                   sockets, dies, cores, threads, cpus);
> +    if (sockets * cores * threads != maxcpus) {
> +        error_setg(errp, "Invalid CPU topology: "
> +                   "product of the hierarchy must match maxcpus: "
> +                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> +                   "!= maxcpus (%u)",
> +                   sockets, dies, cores, threads, maxcpus);
>          return;
>      }
>  
>      if (maxcpus < cpus) {
> -        error_setg(errp, "maxcpus must be equal to or greater than smp");
> -        return;
> -    }
> -
> -    if (sockets * dies * cores * threads != maxcpus) {
> -        error_setg(errp, "Invalid CPU topology deprecated: "
> +        error_setg(errp, "Invalid CPU topology: "
> +                   "maxcpus must be equal to or greater than smp: "
>                     "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> -                   "!= maxcpus (%u)",
> -                   sockets, dies, cores, threads,
> -                   maxcpus);
> +                   "== maxcpus (%u) < smp_cpus (%u)",
> +                   sockets, dies, cores, threads, maxcpus, cpus);
>          return;
>      }
>  
> -- 
> 2.19.1
>

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

[PATCH for-6.2 v3 05/11] hw: Add compat machines for 6.2

[Yanan Wang]

Add 6.2 machine types for arm/i440fx/q35/s390x/spapr.

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c              |  9 ++++++++-
 hw/core/machine.c          |  3 +++
 hw/i386/pc.c               |  3 +++
 hw/i386/pc_piix.c          | 14 +++++++++++++-
 hw/i386/pc_q35.c           | 13 ++++++++++++-
 hw/ppc/spapr.c             | 15 +++++++++++++--
 hw/s390x/s390-virtio-ccw.c | 14 +++++++++++++-
 include/hw/boards.h        |  3 +++
 include/hw/i386/pc.h       |  3 +++
 9 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0b..01165f7f53 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2788,10 +2788,17 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
+static void virt_machine_6_2_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(6, 2)
+
 static void virt_machine_6_1_options(MachineClass *mc)
 {
+    virt_machine_6_2_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(6, 1)
+DEFINE_VIRT_MACHINE(6, 1)
 
 static void virt_machine_6_0_options(MachineClass *mc)
 {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index e879163c3b..458d9736e3 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -37,6 +37,9 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
 
+GlobalProperty hw_compat_6_1[] = {};
+const size_t hw_compat_6_1_len = G_N_ELEMENTS(hw_compat_6_1);
+
 GlobalProperty hw_compat_6_0[] = {
     { "gpex-pcihost", "allow-unmapped-accesses", "false" },
     { "i8042", "extended-state", "false"},
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3e403a7129..8c2235ac46 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -94,6 +94,9 @@
 #include "trace.h"
 #include CONFIG_DEVICES
 
+GlobalProperty pc_compat_6_1[] = {};
+const size_t pc_compat_6_1_len = G_N_ELEMENTS(pc_compat_6_1);
+
 GlobalProperty pc_compat_6_0[] = {
     { "qemu64" "-" TYPE_X86_CPU, "family", "6" },
     { "qemu64" "-" TYPE_X86_CPU, "model", "6" },
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 30b8bd6ea9..fd5c2277f2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -413,7 +413,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
 }
 
-static void pc_i440fx_6_1_machine_options(MachineClass *m)
+static void pc_i440fx_6_2_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_machine_options(m);
@@ -422,6 +422,18 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
     pcmc->default_cpu_version = 1;
 }
 
+DEFINE_I440FX_MACHINE(v6_2, "pc-i440fx-6.2", NULL,
+                      pc_i440fx_6_2_machine_options);
+
+static void pc_i440fx_6_1_machine_options(MachineClass *m)
+{
+    pc_i440fx_6_2_machine_options(m);
+    m->alias = NULL;
+    m->is_default = false;
+    compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
+    compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
+}
+
 DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
                       pc_i440fx_6_1_machine_options);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 04b4a4788d..b45903b15e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -355,7 +355,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_6_1_machine_options(MachineClass *m)
+static void pc_q35_6_2_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_machine_options(m);
@@ -363,6 +363,17 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
     pcmc->default_cpu_version = 1;
 }
 
+DEFINE_Q35_MACHINE(v6_2, "pc-q35-6.2", NULL,
+                   pc_q35_6_2_machine_options);
+
+static void pc_q35_6_1_machine_options(MachineClass *m)
+{
+    pc_q35_6_2_machine_options(m);
+    m->alias = NULL;
+    compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
+    compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
+}
+
 DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
                    pc_q35_6_1_machine_options);
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 81699d4f8b..d39fd4e644 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4685,15 +4685,26 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
     }                                                                \
     type_init(spapr_machine_register_##suffix)
 
+/*
+ * pseries-6.2
+ */
+static void spapr_machine_6_2_class_options(MachineClass *mc)
+{
+    /* Defaults for the latest behaviour inherited from the base class */
+}
+
+DEFINE_SPAPR_MACHINE(6_2, "6.2", true);
+
 /*
  * pseries-6.1
  */
 static void spapr_machine_6_1_class_options(MachineClass *mc)
 {
-    /* Defaults for the latest behaviour inherited from the base class */
+    spapr_machine_6_2_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
 }
 
-DEFINE_SPAPR_MACHINE(6_1, "6.1", true);
+DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
 
 /*
  * pseries-6.0
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e4b18aef49..4d25278cf2 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -791,14 +791,26 @@ bool css_migration_enabled(void)
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
+static void ccw_machine_6_2_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_6_2_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(6_2, "6.2", true);
+
 static void ccw_machine_6_1_instance_options(MachineState *machine)
 {
+    ccw_machine_6_2_instance_options(machine);
 }
 
 static void ccw_machine_6_1_class_options(MachineClass *mc)
 {
+    ccw_machine_6_2_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
 }
-DEFINE_CCW_MACHINE(6_1, "6.1", true);
+DEFINE_CCW_MACHINE(6_1, "6.1", false);
 
 static void ccw_machine_6_0_instance_options(MachineState *machine)
 {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index accd6eff35..463a5514f9 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -353,6 +353,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_6_1[];
+extern const size_t hw_compat_6_1_len;
+
 extern GlobalProperty hw_compat_6_0[];
 extern const size_t hw_compat_6_0_len;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 88dffe7517..97b4ab79b5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -196,6 +196,9 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
 
+extern GlobalProperty pc_compat_6_1[];
+extern const size_t pc_compat_6_1_len;
+
 extern GlobalProperty pc_compat_6_0[];
 extern const size_t pc_compat_6_0_len;
 
-- 
2.19.1

[PATCH for-6.2 v3 06/11] machine: Prefer cores over sockets in smp parsing since 6.2

[Yanan Wang]

In the real SMP hardware topology world, it's much more likely that
we have high cores-per-socket counts and few sockets totally. While
the current preference of sockets over cores in smp parsing results
in a virtual cpu topology with low cores-per-sockets counts and a
large number of sockets, which is just contrary to the real world.

Given that it is better to make the virtual cpu topology be more
reflective of the real world and also for the sake of compatibility,
we start to prefer cores over sockets over threads in smp parsing
since machine type 6.2 for different arches.

In this patch, a boolean "smp_prefer_sockets" is added, and we only
enable the old preference on older machines and enable the new one
since type 6.2 for all arches by using the machine compat mechanism.

Acked-by: David Gibson <david@gibson.dropbear.id.au>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c              |  1 +
 hw/core/machine.c          | 36 ++++++++++++++++++++++++++----------
 hw/i386/pc.c               | 36 ++++++++++++++++++++++++++----------
 hw/i386/pc_piix.c          |  1 +
 hw/i386/pc_q35.c           |  1 +
 hw/ppc/spapr.c             |  1 +
 hw/s390x/s390-virtio-ccw.c |  1 +
 include/hw/boards.h        |  1 +
 qemu-options.hx            |  3 ++-
 9 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 01165f7f53..7babea40dc 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2797,6 +2797,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
 {
     virt_machine_6_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
+    mc->smp_prefer_sockets = true;
 }
 DEFINE_VIRT_MACHINE(6, 1)
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 458d9736e3..a8173a0f45 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -746,6 +746,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
 
 static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
     unsigned cores   = config->has_cores ? config->cores : 0;
@@ -757,7 +758,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
         return;
     }
 
-    /* compute missing values, prefer sockets over cores over threads */
+    /* compute missing values based on the provided ones */
     if (cpus == 0 && maxcpus == 0) {
         sockets = sockets > 0 ? sockets : 1;
         cores = cores > 0 ? cores : 1;
@@ -765,15 +766,30 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     } else {
         maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-        if (sockets == 0) {
-            cores = cores > 0 ? cores : 1;
-            threads = threads > 0 ? threads : 1;
-            sockets = maxcpus / (cores * threads);
-        } else if (cores == 0) {
-            threads = threads > 0 ? threads : 1;
-            cores = maxcpus / (sockets * threads);
-        } else if (threads == 0) {
-            threads = maxcpus / (sockets * cores);
+        if (mc->smp_prefer_sockets) {
+            /* prefer sockets over cores over threads before 6.2 */
+            if (sockets == 0) {
+                cores = cores > 0 ? cores : 1;
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (cores * threads);
+            } else if (cores == 0) {
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * threads);
+            } else if (threads == 0) {
+                threads = maxcpus / (sockets * cores);
+            }
+        } else {
+            /* prefer cores over sockets over threads since 6.2 */
+            if (cores == 0) {
+                sockets = sockets > 0 ? sockets : 1;
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * threads);
+            } else if (sockets == 0) {
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (cores * threads);
+            } else if (threads == 0) {
+                threads = maxcpus / (sockets * cores);
+            }
         }
     }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8c2235ac46..77ab764c5d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -717,6 +717,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
  */
 static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
     unsigned dies    = config->has_dies ? config->dies : 0;
@@ -727,7 +728,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     /* directly default dies to 1 if it's omitted */
     dies = dies > 0 ? dies : 1;
 
-    /* compute missing values, prefer sockets over cores over threads */
+    /* compute missing values based on the provided ones */
     if (cpus == 0 && maxcpus == 0) {
         sockets = sockets > 0 ? sockets : 1;
         cores = cores > 0 ? cores : 1;
@@ -735,15 +736,30 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
     } else {
         maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-        if (sockets == 0) {
-            cores = cores > 0 ? cores : 1;
-            threads = threads > 0 ? threads : 1;
-            sockets = maxcpus / (dies * cores * threads);
-        } else if (cores == 0) {
-            threads = threads > 0 ? threads : 1;
-            cores = maxcpus / (sockets * dies * threads);
-        } else if (threads == 0) {
-            threads = maxcpus / (sockets * dies * cores);
+        if (mc->smp_prefer_sockets) {
+            /* prefer sockets over cores over threads before 6.2 */
+            if (sockets == 0) {
+                cores = cores > 0 ? cores : 1;
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (dies * cores * threads);
+            } else if (cores == 0) {
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * dies * threads);
+            } else if (threads == 0) {
+                threads = maxcpus / (sockets * dies * cores);
+            }
+        } else {
+            /* prefer cores over sockets over threads since 6.2 */
+            if (cores == 0) {
+                sockets = sockets > 0 ? sockets : 1;
+                threads = threads > 0 ? threads : 1;
+                cores = maxcpus / (sockets * dies * threads);
+            } else if (sockets == 0) {
+                threads = threads > 0 ? threads : 1;
+                sockets = maxcpus / (dies * cores * threads);
+            } else if (threads == 0) {
+                threads = maxcpus / (sockets * dies * cores);
+            }
         }
     }
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fd5c2277f2..9b811fc6ca 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -432,6 +432,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
     m->is_default = false;
     compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
     compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
+    m->smp_prefer_sockets = true;
 }
 
 DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b45903b15e..88efb7fde4 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -372,6 +372,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
     m->alias = NULL;
     compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
     compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
+    m->smp_prefer_sockets = true;
 }
 
 DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d39fd4e644..a481fade51 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4702,6 +4702,7 @@ static void spapr_machine_6_1_class_options(MachineClass *mc)
 {
     spapr_machine_6_2_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
+    mc->smp_prefer_sockets = true;
 }
 
 DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4d25278cf2..b40e647883 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -809,6 +809,7 @@ static void ccw_machine_6_1_class_options(MachineClass *mc)
 {
     ccw_machine_6_2_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
+    mc->smp_prefer_sockets = true;
 }
 DEFINE_CCW_MACHINE(6_1, "6.1", false);
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 463a5514f9..2ae039b74f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -247,6 +247,7 @@ struct MachineClass {
     bool nvdimm_supported;
     bool numa_mem_supported;
     bool auto_enable_numa;
+    bool smp_prefer_sockets;
     const char *default_ram_id;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
diff --git a/qemu-options.hx b/qemu-options.hx
index 0912236b4b..450f511ca7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -234,7 +234,8 @@ SRST
     Historically preference was given to the coarsest topology parameters
     when computing missing values (ie sockets preferred over cores, which
     were preferred over threads), however, this behaviour is considered
-    liable to change.
+    liable to change. Prior to 6.2 the preference was sockets over cores
+    over threads. Since 6.2 the preference is cores over sockets over threads.
 ERST
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,
-- 
2.19.1

Re: [PATCH for-6.2 v3 06/11] machine: Prefer cores over sockets in smp parsing since 6.2

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:43AM +0800, Yanan Wang wrote:
> In the real SMP hardware topology world, it's much more likely that
> we have high cores-per-socket counts and few sockets totally. While
> the current preference of sockets over cores in smp parsing results
> in a virtual cpu topology with low cores-per-sockets counts and a
> large number of sockets, which is just contrary to the real world.
> 
> Given that it is better to make the virtual cpu topology be more
> reflective of the real world and also for the sake of compatibility,
> we start to prefer cores over sockets over threads in smp parsing
> since machine type 6.2 for different arches.
> 
> In this patch, a boolean "smp_prefer_sockets" is added, and we only
> enable the old preference on older machines and enable the new one
> since type 6.2 for all arches by using the machine compat mechanism.
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c              |  1 +
>  hw/core/machine.c          | 36 ++++++++++++++++++++++++++----------
>  hw/i386/pc.c               | 36 ++++++++++++++++++++++++++----------
>  hw/i386/pc_piix.c          |  1 +
>  hw/i386/pc_q35.c           |  1 +
>  hw/ppc/spapr.c             |  1 +
>  hw/s390x/s390-virtio-ccw.c |  1 +
>  include/hw/boards.h        |  1 +
>  qemu-options.hx            |  3 ++-
>  9 files changed, 60 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 01165f7f53..7babea40dc 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2797,6 +2797,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
>  {
>      virt_machine_6_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> +    mc->smp_prefer_sockets = true;
>  }
>  DEFINE_VIRT_MACHINE(6, 1)
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 458d9736e3..a8173a0f45 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -746,6 +746,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>  
>  static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>  {
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>      unsigned cpus    = config->has_cpus ? config->cpus : 0;
>      unsigned sockets = config->has_sockets ? config->sockets : 0;
>      unsigned cores   = config->has_cores ? config->cores : 0;
> @@ -757,7 +758,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>          return;
>      }
>  
> -    /* compute missing values, prefer sockets over cores over threads */
> +    /* compute missing values based on the provided ones */
>      if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
> @@ -765,15 +766,30 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -        if (sockets == 0) {
> -            cores = cores > 0 ? cores : 1;
> -            threads = threads > 0 ? threads : 1;
> -            sockets = maxcpus / (cores * threads);
> -        } else if (cores == 0) {
> -            threads = threads > 0 ? threads : 1;
> -            cores = maxcpus / (sockets * threads);
> -        } else if (threads == 0) {
> -            threads = maxcpus / (sockets * cores);
> +        if (mc->smp_prefer_sockets) {
> +            /* prefer sockets over cores over threads before 6.2 */
> +            if (sockets == 0) {
> +                cores = cores > 0 ? cores : 1;
> +                threads = threads > 0 ? threads : 1;
> +                sockets = maxcpus / (cores * threads);
> +            } else if (cores == 0) {
> +                threads = threads > 0 ? threads : 1;
> +                cores = maxcpus / (sockets * threads);
> +            } else if (threads == 0) {
> +                threads = maxcpus / (sockets * cores);
> +            }
> +        } else {
> +            /* prefer cores over sockets over threads since 6.2 */
> +            if (cores == 0) {
> +                sockets = sockets > 0 ? sockets : 1;
> +                threads = threads > 0 ? threads : 1;
> +                cores = maxcpus / (sockets * threads);
> +            } else if (sockets == 0) {
> +                threads = threads > 0 ? threads : 1;
> +                sockets = maxcpus / (cores * threads);
> +            } else if (threads == 0) {
> +                threads = maxcpus / (sockets * cores);
> +            }
>          }
>      }
>  
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8c2235ac46..77ab764c5d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -717,6 +717,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>   */
>  static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>  {
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>      unsigned cpus    = config->has_cpus ? config->cpus : 0;
>      unsigned sockets = config->has_sockets ? config->sockets : 0;
>      unsigned dies    = config->has_dies ? config->dies : 0;
> @@ -727,7 +728,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      /* directly default dies to 1 if it's omitted */
>      dies = dies > 0 ? dies : 1;
>  
> -    /* compute missing values, prefer sockets over cores over threads */
> +    /* compute missing values based on the provided ones */
>      if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
>          cores = cores > 0 ? cores : 1;
> @@ -735,15 +736,30 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>      } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -        if (sockets == 0) {
> -            cores = cores > 0 ? cores : 1;
> -            threads = threads > 0 ? threads : 1;
> -            sockets = maxcpus / (dies * cores * threads);
> -        } else if (cores == 0) {
> -            threads = threads > 0 ? threads : 1;
> -            cores = maxcpus / (sockets * dies * threads);
> -        } else if (threads == 0) {
> -            threads = maxcpus / (sockets * dies * cores);
> +        if (mc->smp_prefer_sockets) {
> +            /* prefer sockets over cores over threads before 6.2 */
> +            if (sockets == 0) {
> +                cores = cores > 0 ? cores : 1;
> +                threads = threads > 0 ? threads : 1;
> +                sockets = maxcpus / (dies * cores * threads);
> +            } else if (cores == 0) {
> +                threads = threads > 0 ? threads : 1;
> +                cores = maxcpus / (sockets * dies * threads);
> +            } else if (threads == 0) {
> +                threads = maxcpus / (sockets * dies * cores);
> +            }
> +        } else {
> +            /* prefer cores over sockets over threads since 6.2 */
> +            if (cores == 0) {
> +                sockets = sockets > 0 ? sockets : 1;
> +                threads = threads > 0 ? threads : 1;
> +                cores = maxcpus / (sockets * dies * threads);
> +            } else if (sockets == 0) {
> +                threads = threads > 0 ? threads : 1;
> +                sockets = maxcpus / (dies * cores * threads);
> +            } else if (threads == 0) {
> +                threads = maxcpus / (sockets * dies * cores);
> +            }
>          }
>      }
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index fd5c2277f2..9b811fc6ca 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -432,6 +432,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
>      m->is_default = false;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> +    m->smp_prefer_sockets = true;
>  }
>  
>  DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b45903b15e..88efb7fde4 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -372,6 +372,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
>      m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> +    m->smp_prefer_sockets = true;
>  }
>  
>  DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d39fd4e644..a481fade51 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4702,6 +4702,7 @@ static void spapr_machine_6_1_class_options(MachineClass *mc)
>  {
>      spapr_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> +    mc->smp_prefer_sockets = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4d25278cf2..b40e647883 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -809,6 +809,7 @@ static void ccw_machine_6_1_class_options(MachineClass *mc)
>  {
>      ccw_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> +    mc->smp_prefer_sockets = true;
>  }
>  DEFINE_CCW_MACHINE(6_1, "6.1", false);
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 463a5514f9..2ae039b74f 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -247,6 +247,7 @@ struct MachineClass {
>      bool nvdimm_supported;
>      bool numa_mem_supported;
>      bool auto_enable_numa;
> +    bool smp_prefer_sockets;
>      const char *default_ram_id;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0912236b4b..450f511ca7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -234,7 +234,8 @@ SRST
>      Historically preference was given to the coarsest topology parameters
>      when computing missing values (ie sockets preferred over cores, which
>      were preferred over threads), however, this behaviour is considered
> -    liable to change.
> +    liable to change. Prior to 6.2 the preference was sockets over cores
> +    over threads. Since 6.2 the preference is cores over sockets over threads.
>  ERST
>  
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> -- 
> 2.19.1
>

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

Re: [PATCH for-6.2 v3 06/11] machine: Prefer cores over sockets in smp parsing since 6.2

[Cornelia Huck]

On Wed, Jul 28 2021, Yanan Wang <wangyanan55@huawei.com> wrote:

> In the real SMP hardware topology world, it's much more likely that
> we have high cores-per-socket counts and few sockets totally. While
> the current preference of sockets over cores in smp parsing results
> in a virtual cpu topology with low cores-per-sockets counts and a
> large number of sockets, which is just contrary to the real world.
>
> Given that it is better to make the virtual cpu topology be more
> reflective of the real world and also for the sake of compatibility,
> we start to prefer cores over sockets over threads in smp parsing
> since machine type 6.2 for different arches.
>
> In this patch, a boolean "smp_prefer_sockets" is added, and we only
> enable the old preference on older machines and enable the new one
> since type 6.2 for all arches by using the machine compat mechanism.
>
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c              |  1 +
>  hw/core/machine.c          | 36 ++++++++++++++++++++++++++----------
>  hw/i386/pc.c               | 36 ++++++++++++++++++++++++++----------
>  hw/i386/pc_piix.c          |  1 +
>  hw/i386/pc_q35.c           |  1 +
>  hw/ppc/spapr.c             |  1 +
>  hw/s390x/s390-virtio-ccw.c |  1 +
>  include/hw/boards.h        |  1 +
>  qemu-options.hx            |  3 ++-
>  9 files changed, 60 insertions(+), 21 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

[PATCH for-6.2 v3 07/11] machine: Use ms instead of global current_machine in sanity-check

[Yanan Wang]

In the sanity-check of smp_cpus and max_cpus against mc in function
machine_set_smp(), we are now using ms->smp.max_cpus for the check
but using current_machine->smp.max_cpus in the error message.
Tweak this by uniformly using the local ms.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a8173a0f45..e13a8f2f34 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -878,7 +878,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
     } else if (ms->smp.max_cpus > mc->max_cpus) {
         error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
                    "supported by machine '%s' is %d",
-                   current_machine->smp.max_cpus,
+                   ms->smp.max_cpus,
                    mc->name, mc->max_cpus);
     }
 
-- 
2.19.1

Re: [PATCH for-6.2 v3 07/11] machine: Use ms instead of global current_machine in sanity-check

[Pankaj Gupta]

> In the sanity-check of smp_cpus and max_cpus against mc in function
> machine_set_smp(), we are now using ms->smp.max_cpus for the check
> but using current_machine->smp.max_cpus in the error message.
> Tweak this by uniformly using the local ms.
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a8173a0f45..e13a8f2f34 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -878,7 +878,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
>      } else if (ms->smp.max_cpus > mc->max_cpus) {
>          error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
>                     "supported by machine '%s' is %d",
> -                   current_machine->smp.max_cpus,
> +                   ms->smp.max_cpus,
>                     mc->name, mc->max_cpus);
>      }
>

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

Re: [PATCH for-6.2 v3 07/11] machine: Use ms instead of global current_machine in sanity-check

[Cornelia Huck]

On Wed, Jul 28 2021, Yanan Wang <wangyanan55@huawei.com> wrote:

> In the sanity-check of smp_cpus and max_cpus against mc in function
> machine_set_smp(), we are now using ms->smp.max_cpus for the check
> but using current_machine->smp.max_cpus in the error message.
> Tweak this by uniformly using the local ms.
>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH for-6.2 v3 08/11] 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.

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
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 e13a8f2f34..9223ece3ea 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1057,10 +1057,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 2ae039b74f..2a1bba86c0 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

[PATCH for-6.2 v3 09/11] machine: Make smp_parse generic enough for all arches

[Yanan Wang]

Currently the only difference between smp_parse and pc_smp_parse
is the support of dies parameter and the related error reporting.
With some arch compat variables like "bool dies_supported", we can
make smp_parse generic enough for all arches and the PC specific
one can be removed.

Making smp_parse() generic enough can reduce code duplication and
ease the code maintenance, and also allows extending the topology
with more arch specific members (e.g., clusters) in the future.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/core/machine.c   | 96 +++++++++++++++++++++++++++++++++++++++------
 hw/i386/pc.c        | 83 +--------------------------------------
 include/hw/boards.h |  9 +++++
 3 files changed, 93 insertions(+), 95 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9223ece3ea..76b6c3bc64 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -15,6 +15,7 @@
 #include "qapi/qmp/qerror.h"
 #include "sysemu/replay.h"
 #include "qemu/units.h"
+#include "qemu/cutils.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "qapi/error.h"
@@ -744,20 +745,87 @@ void machine_set_cpu_numa_node(MachineState *machine,
     }
 }
 
+static char *cpu_topology_hierarchy(MachineState *ms)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    SMPCompatProps *smp_props = &mc->smp_props;
+    char topo_msg[256] = "";
+
+    /*
+     * Topology members should be ordered from the largest to the smallest.
+     * Concept of sockets/cores/threads is supported by default and will be
+     * reported in the hierarchy. Unsupported members will not be reported.
+     */
+    g_autofree char *sockets_msg = g_strdup_printf(
+            " * sockets (%u)", ms->smp.sockets);
+    pstrcat(topo_msg, sizeof(topo_msg), sockets_msg);
+
+    if (smp_props->dies_supported) {
+        g_autofree char *dies_msg = g_strdup_printf(
+                " * dies (%u)", ms->smp.dies);
+        pstrcat(topo_msg, sizeof(topo_msg), dies_msg);
+    }
+
+    g_autofree char *cores_msg = g_strdup_printf(
+            " * cores (%u)", ms->smp.cores);
+    pstrcat(topo_msg, sizeof(topo_msg), cores_msg);
+
+    g_autofree char *threads_msg = g_strdup_printf(
+            " * threads (%u)", ms->smp.threads);
+    pstrcat(topo_msg, sizeof(topo_msg), threads_msg);
+
+    return g_strdup_printf("%s", topo_msg + 3);
+}
+
+/*
+ * smp_parse - Generic function used to parse the given SMP configuration
+ *
+ * A topology parameter must be omitted or specified equal to 1 if it's
+ * not supported by the machine. Concept of sockets/cores/threads is
+ * supported by default. Unsupported members will not be reported in
+ * the cpu topology hierarchy message.
+ *
+ * For compatibility, if omitted the arch-specific members (e.g. dies)
+ * will not be computed, but will directly default to 1 instead. This
+ * logic should also apply to future introduced ones.
+ *
+ * Omitted arch-neutral members, i.e., cpus/sockets/cores/threads/maxcpus
+ * will be computed based on the provided ones. When both maxcpus and cpus
+ * are omitted, maxcpus will be computed from the given parameters and cpus
+ * will be set equal to maxcpus. When only one of maxcpus and cpus is given
+ * then the omitted one will be set to its given counterpart's value.
+ * Both maxcpus and cpus may be specified, but maxcpus must be equal to or
+ * greater than cpus.
+ *
+ * In calculation of omitted sockets/cores/threads, we prefer sockets over
+ * cores over threads before 6.2, while preferring cores over sockets over
+ * threads since 6.2.
+ */
 static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     unsigned cpus    = config->has_cpus ? config->cpus : 0;
     unsigned sockets = config->has_sockets ? config->sockets : 0;
+    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;
 
-    if (config->has_dies && config->dies > 1) {
+    /*
+     * A topology parameter must be omitted or specified equal to 1,
+     * if the machine's CPU topology doesn't support it.
+     */
+    if (!mc->smp_props.dies_supported && dies > 1) {
         error_setg(errp, "dies not supported by this machine's CPU topology");
         return;
     }
 
+    /*
+     * If omitted the arch-specific members will not be computed,
+     * but will directly default to 1 instead.
+     */
+    dies = dies > 0 ? dies : 1;
+
     /* compute missing values based on the provided ones */
     if (cpus == 0 && maxcpus == 0) {
         sockets = sockets > 0 ? sockets : 1;
@@ -796,29 +864,31 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
     cpus = cpus > 0 ? cpus : maxcpus;
 
+    ms->smp.cpus = cpus;
+    ms->smp.sockets = sockets;
+    ms->smp.dies = dies;
+    ms->smp.cores = cores;
+    ms->smp.threads = threads;
+    ms->smp.max_cpus = maxcpus;
+
+    /* sanity-check of the computed topology */
     if (sockets * cores * threads != maxcpus) {
+        g_autofree char *topo_msg = cpu_topology_hierarchy(ms);
         error_setg(errp, "Invalid CPU topology: "
                    "product of the hierarchy must match maxcpus: "
-                   "sockets (%u) * cores (%u) * threads (%u) "
-                   "!= maxcpus (%u)",
-                   sockets, cores, threads, maxcpus);
+                   "%s != maxcpus (%u)",
+                   topo_msg, maxcpus);
         return;
     }
 
     if (maxcpus < cpus) {
+        g_autofree char *topo_msg = cpu_topology_hierarchy(ms);
         error_setg(errp, "Invalid CPU topology: "
                    "maxcpus must be equal to or greater than smp: "
-                   "sockets (%u) * cores (%u) * threads (%u) "
-                   "== maxcpus (%u) < smp_cpus (%u)",
-                   sockets, cores, threads, maxcpus, cpus);
+                   "%s == maxcpus (%u) < smp_cpus (%u)",
+                   topo_msg, maxcpus, cpus);
         return;
     }
-
-    ms->smp.cpus = cpus;
-    ms->smp.sockets = sockets;
-    ms->smp.cores = cores;
-    ms->smp.threads = threads;
-    ms->smp.max_cpus = maxcpus;
 }
 
 static void machine_get_smp(Object *obj, Visitor *v, const char *name,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 77ab764c5d..ce493a9294 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -711,87 +711,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-/*
- * This function is very similar to smp_parse()
- * in hw/core/machine.c but includes CPU die support.
- */
-static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
-{
-    MachineClass *mc = MACHINE_GET_CLASS(ms);
-    unsigned cpus    = config->has_cpus ? config->cpus : 0;
-    unsigned sockets = config->has_sockets ? config->sockets : 0;
-    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;
-
-    /* directly default dies to 1 if it's omitted */
-    dies = dies > 0 ? dies : 1;
-
-    /* compute missing values based on the provided ones */
-    if (cpus == 0 && maxcpus == 0) {
-        sockets = sockets > 0 ? sockets : 1;
-        cores = cores > 0 ? cores : 1;
-        threads = threads > 0 ? threads : 1;
-    } else {
-        maxcpus = maxcpus > 0 ? maxcpus : cpus;
-
-        if (mc->smp_prefer_sockets) {
-            /* prefer sockets over cores over threads before 6.2 */
-            if (sockets == 0) {
-                cores = cores > 0 ? cores : 1;
-                threads = threads > 0 ? threads : 1;
-                sockets = maxcpus / (dies * cores * threads);
-            } else if (cores == 0) {
-                threads = threads > 0 ? threads : 1;
-                cores = maxcpus / (sockets * dies * threads);
-            } else if (threads == 0) {
-                threads = maxcpus / (sockets * dies * cores);
-            }
-        } else {
-            /* prefer cores over sockets over threads since 6.2 */
-            if (cores == 0) {
-                sockets = sockets > 0 ? sockets : 1;
-                threads = threads > 0 ? threads : 1;
-                cores = maxcpus / (sockets * dies * threads);
-            } else if (sockets == 0) {
-                threads = threads > 0 ? threads : 1;
-                sockets = maxcpus / (dies * cores * threads);
-            } else if (threads == 0) {
-                threads = maxcpus / (sockets * dies * cores);
-            }
-        }
-    }
-
-    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
-    cpus = cpus > 0 ? cpus : maxcpus;
-
-    if (sockets * cores * threads != maxcpus) {
-        error_setg(errp, "Invalid CPU topology: "
-                   "product of the hierarchy must match maxcpus: "
-                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
-                   "!= maxcpus (%u)",
-                   sockets, dies, cores, threads, maxcpus);
-        return;
-    }
-
-    if (maxcpus < cpus) {
-        error_setg(errp, "Invalid CPU topology: "
-                   "maxcpus must be equal to or greater than smp: "
-                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
-                   "== maxcpus (%u) < smp_cpus (%u)",
-                   sockets, dies, cores, threads, maxcpus, cpus);
-        return;
-    }
-
-    ms->smp.cpus = cpus;
-    ms->smp.sockets = sockets;
-    ms->smp.dies = dies;
-    ms->smp.cores = cores;
-    ms->smp.threads = threads;
-    ms->smp.maxcpus = maxcpus;
-}
-
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
@@ -1745,7 +1664,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->auto_enable_numa_with_memdev = true;
     mc->has_hotpluggable_cpus = true;
     mc->default_boot_order = "cad";
-    mc->smp_parse = pc_smp_parse;
     mc->block_default_type = IF_IDE;
     mc->max_cpus = 255;
     mc->reset = pc_machine_reset;
@@ -1756,6 +1674,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = pc_machine_device_unplug_cb;
     mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
     mc->nvdimm_supported = true;
+    mc->smp_props.dies_supported = true;
     mc->default_ram_id = "pc.ram";
 
     object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2a1bba86c0..0631900c08 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -108,6 +108,14 @@ typedef struct {
     CPUArchId cpus[];
 } CPUArchIdList;
 
+/**
+ * SMPCompatProps:
+ * @dies_supported - whether dies is supported by the machine
+ */
+typedef struct {
+    bool dies_supported;
+} SMPCompatProps;
+
 /**
  * MachineClass:
  * @deprecation_reason: If set, the machine is marked as deprecated. The
@@ -248,6 +256,7 @@ struct MachineClass {
     bool numa_mem_supported;
     bool auto_enable_numa;
     bool smp_prefer_sockets;
+    SMPCompatProps smp_props;
     const char *default_ram_id;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
-- 
2.19.1

Re: [PATCH for-6.2 v3 09/11] machine: Make smp_parse generic enough for all arches

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:46AM +0800, Yanan Wang wrote:
> Currently the only difference between smp_parse and pc_smp_parse
> is the support of dies parameter and the related error reporting.
> With some arch compat variables like "bool dies_supported", we can
> make smp_parse generic enough for all arches and the PC specific
> one can be removed.
> 
> Making smp_parse() generic enough can reduce code duplication and
> ease the code maintenance, and also allows extending the topology
> with more arch specific members (e.g., clusters) in the future.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c   | 96 +++++++++++++++++++++++++++++++++++++++------
>  hw/i386/pc.c        | 83 +--------------------------------------
>  include/hw/boards.h |  9 +++++
>  3 files changed, 93 insertions(+), 95 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9223ece3ea..76b6c3bc64 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -15,6 +15,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "sysemu/replay.h"
>  #include "qemu/units.h"
> +#include "qemu/cutils.h"
>  #include "hw/boards.h"
>  #include "hw/loader.h"
>  #include "qapi/error.h"
> @@ -744,20 +745,87 @@ void machine_set_cpu_numa_node(MachineState *machine,
>      }
>  }
>  
> +static char *cpu_topology_hierarchy(MachineState *ms)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    SMPCompatProps *smp_props = &mc->smp_props;
> +    char topo_msg[256] = "";
> +
> +    /*
> +     * Topology members should be ordered from the largest to the smallest.
> +     * Concept of sockets/cores/threads is supported by default and will be
> +     * reported in the hierarchy. Unsupported members will not be reported.
> +     */
> +    g_autofree char *sockets_msg = g_strdup_printf(
> +            " * sockets (%u)", ms->smp.sockets);
> +    pstrcat(topo_msg, sizeof(topo_msg), sockets_msg);
> +
> +    if (smp_props->dies_supported) {
> +        g_autofree char *dies_msg = g_strdup_printf(
> +                " * dies (%u)", ms->smp.dies);
> +        pstrcat(topo_msg, sizeof(topo_msg), dies_msg);
> +    }
> +
> +    g_autofree char *cores_msg = g_strdup_printf(
> +            " * cores (%u)", ms->smp.cores);
> +    pstrcat(topo_msg, sizeof(topo_msg), cores_msg);
> +
> +    g_autofree char *threads_msg = g_strdup_printf(
> +            " * threads (%u)", ms->smp.threads);
> +    pstrcat(topo_msg, sizeof(topo_msg), threads_msg);
> +
> +    return g_strdup_printf("%s", topo_msg + 3);
> +}
> +
> +/*
> + * smp_parse - Generic function used to parse the given SMP configuration
> + *
> + * A topology parameter must be omitted or specified equal to 1 if it's
> + * not supported by the machine. Concept of sockets/cores/threads is
> + * supported by default. Unsupported members will not be reported in
> + * the cpu topology hierarchy message.
> + *
> + * For compatibility, if omitted the arch-specific members (e.g. dies)
> + * will not be computed, but will directly default to 1 instead. This
> + * logic should also apply to future introduced ones.
> + *
> + * Omitted arch-neutral members, i.e., cpus/sockets/cores/threads/maxcpus
> + * will be computed based on the provided ones. When both maxcpus and cpus
> + * are omitted, maxcpus will be computed from the given parameters and cpus
> + * will be set equal to maxcpus. When only one of maxcpus and cpus is given
> + * then the omitted one will be set to its given counterpart's value.
> + * Both maxcpus and cpus may be specified, but maxcpus must be equal to or
> + * greater than cpus.
> + *
> + * In calculation of omitted sockets/cores/threads, we prefer sockets over
> + * cores over threads before 6.2, while preferring cores over sockets over
> + * threads since 6.2.
> + */
>  static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
>      unsigned cpus    = config->has_cpus ? config->cpus : 0;
>      unsigned sockets = config->has_sockets ? config->sockets : 0;
> +    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;
>  
> -    if (config->has_dies && config->dies > 1) {
> +    /*
> +     * A topology parameter must be omitted or specified equal to 1,
> +     * if the machine's CPU topology doesn't support it.
> +     */
> +    if (!mc->smp_props.dies_supported && dies > 1) {
>          error_setg(errp, "dies not supported by this machine's CPU topology");
>          return;
>      }
>  
> +    /*
> +     * If omitted the arch-specific members will not be computed,
> +     * but will directly default to 1 instead.
> +     */
> +    dies = dies > 0 ? dies : 1;
> +
>      /* compute missing values based on the provided ones */
>      if (cpus == 0 && maxcpus == 0) {
>          sockets = sockets > 0 ? sockets : 1;
> @@ -796,29 +864,31 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      maxcpus = maxcpus > 0 ? maxcpus : sockets * cores * threads;
>      cpus = cpus > 0 ? cpus : maxcpus;
>  
> +    ms->smp.cpus = cpus;
> +    ms->smp.sockets = sockets;
> +    ms->smp.dies = dies;
> +    ms->smp.cores = cores;
> +    ms->smp.threads = threads;
> +    ms->smp.max_cpus = maxcpus;
> +
> +    /* sanity-check of the computed topology */
>      if (sockets * cores * threads != maxcpus) {
> +        g_autofree char *topo_msg = cpu_topology_hierarchy(ms);
>          error_setg(errp, "Invalid CPU topology: "
>                     "product of the hierarchy must match maxcpus: "
> -                   "sockets (%u) * cores (%u) * threads (%u) "
> -                   "!= maxcpus (%u)",
> -                   sockets, cores, threads, maxcpus);
> +                   "%s != maxcpus (%u)",
> +                   topo_msg, maxcpus);
>          return;
>      }
>  
>      if (maxcpus < cpus) {
> +        g_autofree char *topo_msg = cpu_topology_hierarchy(ms);
>          error_setg(errp, "Invalid CPU topology: "
>                     "maxcpus must be equal to or greater than smp: "
> -                   "sockets (%u) * cores (%u) * threads (%u) "
> -                   "== maxcpus (%u) < smp_cpus (%u)",
> -                   sockets, cores, threads, maxcpus, cpus);
> +                   "%s == maxcpus (%u) < smp_cpus (%u)",
> +                   topo_msg, maxcpus, cpus);
>          return;
>      }
> -
> -    ms->smp.cpus = cpus;
> -    ms->smp.sockets = sockets;
> -    ms->smp.cores = cores;
> -    ms->smp.threads = threads;
> -    ms->smp.max_cpus = maxcpus;
>  }
>  
>  static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 77ab764c5d..ce493a9294 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -711,87 +711,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> -/*
> - * This function is very similar to smp_parse()
> - * in hw/core/machine.c but includes CPU die support.
> - */
> -static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> -{
> -    MachineClass *mc = MACHINE_GET_CLASS(ms);
> -    unsigned cpus    = config->has_cpus ? config->cpus : 0;
> -    unsigned sockets = config->has_sockets ? config->sockets : 0;
> -    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;
> -
> -    /* directly default dies to 1 if it's omitted */
> -    dies = dies > 0 ? dies : 1;
> -
> -    /* compute missing values based on the provided ones */
> -    if (cpus == 0 && maxcpus == 0) {
> -        sockets = sockets > 0 ? sockets : 1;
> -        cores = cores > 0 ? cores : 1;
> -        threads = threads > 0 ? threads : 1;
> -    } else {
> -        maxcpus = maxcpus > 0 ? maxcpus : cpus;
> -
> -        if (mc->smp_prefer_sockets) {
> -            /* prefer sockets over cores over threads before 6.2 */
> -            if (sockets == 0) {
> -                cores = cores > 0 ? cores : 1;
> -                threads = threads > 0 ? threads : 1;
> -                sockets = maxcpus / (dies * cores * threads);
> -            } else if (cores == 0) {
> -                threads = threads > 0 ? threads : 1;
> -                cores = maxcpus / (sockets * dies * threads);
> -            } else if (threads == 0) {
> -                threads = maxcpus / (sockets * dies * cores);
> -            }
> -        } else {
> -            /* prefer cores over sockets over threads since 6.2 */
> -            if (cores == 0) {
> -                sockets = sockets > 0 ? sockets : 1;
> -                threads = threads > 0 ? threads : 1;
> -                cores = maxcpus / (sockets * dies * threads);
> -            } else if (sockets == 0) {
> -                threads = threads > 0 ? threads : 1;
> -                sockets = maxcpus / (dies * cores * threads);
> -            } else if (threads == 0) {
> -                threads = maxcpus / (sockets * dies * cores);
> -            }
> -        }
> -    }
> -
> -    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * cores * threads;
> -    cpus = cpus > 0 ? cpus : maxcpus;
> -
> -    if (sockets * cores * threads != maxcpus) {
> -        error_setg(errp, "Invalid CPU topology: "
> -                   "product of the hierarchy must match maxcpus: "
> -                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> -                   "!= maxcpus (%u)",
> -                   sockets, dies, cores, threads, maxcpus);
> -        return;
> -    }
> -
> -    if (maxcpus < cpus) {
> -        error_setg(errp, "Invalid CPU topology: "
> -                   "maxcpus must be equal to or greater than smp: "
> -                   "sockets (%u) * dies (%u) * cores (%u) * threads (%u) "
> -                   "== maxcpus (%u) < smp_cpus (%u)",
> -                   sockets, dies, cores, threads, maxcpus, cpus);
> -        return;
> -    }
> -
> -    ms->smp.cpus = cpus;
> -    ms->smp.sockets = sockets;
> -    ms->smp.dies = dies;
> -    ms->smp.cores = cores;
> -    ms->smp.threads = threads;
> -    ms->smp.maxcpus = maxcpus;
> -}
> -
>  static
>  void pc_machine_done(Notifier *notifier, void *data)
>  {
> @@ -1745,7 +1664,6 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      mc->auto_enable_numa_with_memdev = true;
>      mc->has_hotpluggable_cpus = true;
>      mc->default_boot_order = "cad";
> -    mc->smp_parse = pc_smp_parse;
>      mc->block_default_type = IF_IDE;
>      mc->max_cpus = 255;
>      mc->reset = pc_machine_reset;
> @@ -1756,6 +1674,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      hc->unplug = pc_machine_device_unplug_cb;
>      mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>      mc->nvdimm_supported = true;
> +    mc->smp_props.dies_supported = true;
>      mc->default_ram_id = "pc.ram";
>  
>      object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size",
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2a1bba86c0..0631900c08 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -108,6 +108,14 @@ typedef struct {
>      CPUArchId cpus[];
>  } CPUArchIdList;
>  
> +/**
> + * SMPCompatProps:
> + * @dies_supported - whether dies is supported by the machine
> + */
> +typedef struct {
> +    bool dies_supported;
> +} SMPCompatProps;
> +
>  /**
>   * MachineClass:
>   * @deprecation_reason: If set, the machine is marked as deprecated. The
> @@ -248,6 +256,7 @@ struct MachineClass {
>      bool numa_mem_supported;
>      bool auto_enable_numa;
>      bool smp_prefer_sockets;
> +    SMPCompatProps smp_props;
>      const char *default_ram_id;
>  
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> -- 
> 2.19.1
>

What about putting smp_prefer_sockets in SMPCompatProps too (as
prefer_sockets)?

Otherwise

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

Re: [PATCH for-6.2 v3 09/11] machine: Make smp_parse generic enough for all arches

[Andrew Jones]

On Wed, Jul 28, 2021 at 10:38:57PM +0200, Andrew Jones wrote:
> On Wed, Jul 28, 2021 at 11:48:46AM +0800, Yanan Wang wrote:
> > @@ -248,6 +256,7 @@ struct MachineClass {
> >      bool numa_mem_supported;
> >      bool auto_enable_numa;
> >      bool smp_prefer_sockets;
> > +    SMPCompatProps smp_props;
> >      const char *default_ram_id;
> >  
> >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > -- 
> > 2.19.1
> >
> 
> What about putting smp_prefer_sockets in SMPCompatProps too (as
> prefer_sockets)?

Ah, I see patch 11/11 does that.

Thanks,
drew

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

[PATCH for-6.2 v3 10/11] machine: Remove smp_parse callback from MachineClass

[Yanan Wang]

Now we have a generic smp parser for all arches, and there will
not be any other arch specific ones, so let's remove the callback
from MachineClass and call the parser directly.

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

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 76b6c3bc64..8f84e38e2e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -934,7 +934,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
         goto out_free;
     }
 
-    mc->smp_parse(ms, config, errp);
+    smp_parse(ms, config, errp);
     if (errp) {
         goto out_free;
     }
@@ -963,7 +963,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
     /* Default 128 MB as guest ram size */
     mc->default_ram_size = 128 * MiB;
     mc->rom_file_has_mr = true;
-    mc->smp_parse = smp_parse;
 
     /* numa node memory size aligned on 8MB by default.
      * On Linux, each node's border has to be 8MB aligned
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0631900c08..72123f594d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -177,10 +177,6 @@ typedef struct {
  *    kvm-type may be NULL if it is not needed.
  * @numa_mem_supported:
  *    true if '--numa node.mem' option is supported and false otherwise
- * @smp_parse:
- *    The function pointer to hook different machine specific functions for
- *    parsing "smp-opts" from QemuOpts to MachineState::CpuTopology and more
- *    machine specific topology fields, such as smp_dies for PCMachine.
  * @hotplug_allowed:
  *    If the hook is provided, then it'll be called for each device
  *    hotplug to check whether the device hotplug is allowed.  Return
@@ -217,7 +213,6 @@ struct MachineClass {
     void (*reset)(MachineState *state);
     void (*wakeup)(MachineState *state);
     int (*kvm_type)(MachineState *machine, const char *arg);
-    void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
 
     BlockInterfaceType block_default_type;
     int units_per_default_bus;
-- 
2.19.1

Re: [PATCH for-6.2 v3 10/11] machine: Remove smp_parse callback from MachineClass

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:47AM +0800, Yanan Wang wrote:
> Now we have a generic smp parser for all arches, and there will
> not be any other arch specific ones, so let's remove the callback
> from MachineClass and call the parser directly.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c   | 3 +--
>  include/hw/boards.h | 5 -----
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 76b6c3bc64..8f84e38e2e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -934,7 +934,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
>          goto out_free;
>      }
>  
> -    mc->smp_parse(ms, config, errp);
> +    smp_parse(ms, config, errp);
>      if (errp) {
>          goto out_free;
>      }
> @@ -963,7 +963,6 @@ static void machine_class_init(ObjectClass *oc, void *data)
>      /* Default 128 MB as guest ram size */
>      mc->default_ram_size = 128 * MiB;
>      mc->rom_file_has_mr = true;
> -    mc->smp_parse = smp_parse;
>  
>      /* numa node memory size aligned on 8MB by default.
>       * On Linux, each node's border has to be 8MB aligned
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 0631900c08..72123f594d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -177,10 +177,6 @@ typedef struct {
>   *    kvm-type may be NULL if it is not needed.
>   * @numa_mem_supported:
>   *    true if '--numa node.mem' option is supported and false otherwise
> - * @smp_parse:
> - *    The function pointer to hook different machine specific functions for
> - *    parsing "smp-opts" from QemuOpts to MachineState::CpuTopology and more
> - *    machine specific topology fields, such as smp_dies for PCMachine.
>   * @hotplug_allowed:
>   *    If the hook is provided, then it'll be called for each device
>   *    hotplug to check whether the device hotplug is allowed.  Return
> @@ -217,7 +213,6 @@ struct MachineClass {
>      void (*reset)(MachineState *state);
>      void (*wakeup)(MachineState *state);
>      int (*kvm_type)(MachineState *machine, const char *arg);
> -    void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
>  
>      BlockInterfaceType block_default_type;
>      int units_per_default_bus;
> -- 
> 2.19.1
>

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

[PATCH for-6.2 v3 11/11] machine: Move smp_prefer_sockets to struct SMPCompatProps

[Yanan Wang]

Now we have a common structure SMPCompatProps used to store information
about SMP compatibility stuff, so we can also move smp_prefer_sockets
there for cleaner code.

No functional change intended.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
---
 hw/arm/virt.c              | 2 +-
 hw/core/machine.c          | 2 +-
 hw/i386/pc_piix.c          | 2 +-
 hw/i386/pc_q35.c           | 2 +-
 hw/ppc/spapr.c             | 2 +-
 hw/s390x/s390-virtio-ccw.c | 2 +-
 include/hw/boards.h        | 3 ++-
 7 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7babea40dc..ae029680da 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2797,7 +2797,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
 {
     virt_machine_6_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
-    mc->smp_prefer_sockets = true;
+    mc->smp_props.prefer_sockets = true;
 }
 DEFINE_VIRT_MACHINE(6, 1)
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8f84e38e2e..61d1f643f4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -834,7 +834,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
     } else {
         maxcpus = maxcpus > 0 ? maxcpus : cpus;
 
-        if (mc->smp_prefer_sockets) {
+        if (mc->smp_props.prefer_sockets) {
             /* prefer sockets over cores over threads before 6.2 */
             if (sockets == 0) {
                 cores = cores > 0 ? cores : 1;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9b811fc6ca..a60ebfc2c1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -432,7 +432,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
     m->is_default = false;
     compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
     compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
-    m->smp_prefer_sockets = true;
+    m->smp_props.prefer_sockets = true;
 }
 
 DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 88efb7fde4..4b622ffb82 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -372,7 +372,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
     m->alias = NULL;
     compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
     compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
-    m->smp_prefer_sockets = true;
+    m->smp_props.prefer_sockets = true;
 }
 
 DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a481fade51..efdea43c0d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4702,7 +4702,7 @@ static void spapr_machine_6_1_class_options(MachineClass *mc)
 {
     spapr_machine_6_2_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
-    mc->smp_prefer_sockets = true;
+    mc->smp_props.prefer_sockets = true;
 }
 
 DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b40e647883..5bdef9b4d7 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -809,7 +809,7 @@ static void ccw_machine_6_1_class_options(MachineClass *mc)
 {
     ccw_machine_6_2_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
-    mc->smp_prefer_sockets = true;
+    mc->smp_props.prefer_sockets = true;
 }
 DEFINE_CCW_MACHINE(6_1, "6.1", false);
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 72123f594d..23671a0f8f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -110,9 +110,11 @@ typedef struct {
 
 /**
  * SMPCompatProps:
+ * @prefer_sockets - whether sockets is preferred over cores in smp parsing
  * @dies_supported - whether dies is supported by the machine
  */
 typedef struct {
+    bool prefer_sockets;
     bool dies_supported;
 } SMPCompatProps;
 
@@ -250,7 +252,6 @@ struct MachineClass {
     bool nvdimm_supported;
     bool numa_mem_supported;
     bool auto_enable_numa;
-    bool smp_prefer_sockets;
     SMPCompatProps smp_props;
     const char *default_ram_id;
 
-- 
2.19.1

Re: [PATCH for-6.2 v3 11/11] machine: Move smp_prefer_sockets to struct SMPCompatProps

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4815 bytes --]

On Wed, Jul 28, 2021 at 11:48:48AM +0800, Yanan Wang wrote:
> Now we have a common structure SMPCompatProps used to store information
> about SMP compatibility stuff, so we can also move smp_prefer_sockets
> there for cleaner code.
> 
> No functional change intended.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/arm/virt.c              | 2 +-
>  hw/core/machine.c          | 2 +-
>  hw/i386/pc_piix.c          | 2 +-
>  hw/i386/pc_q35.c           | 2 +-
>  hw/ppc/spapr.c             | 2 +-
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  include/hw/boards.h        | 3 ++-
>  7 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7babea40dc..ae029680da 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2797,7 +2797,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
>  {
>      virt_machine_6_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> -    mc->smp_prefer_sockets = true;
> +    mc->smp_props.prefer_sockets = true;
>  }
>  DEFINE_VIRT_MACHINE(6, 1)
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8f84e38e2e..61d1f643f4 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -834,7 +834,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -        if (mc->smp_prefer_sockets) {
> +        if (mc->smp_props.prefer_sockets) {
>              /* prefer sockets over cores over threads before 6.2 */
>              if (sockets == 0) {
>                  cores = cores > 0 ? cores : 1;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 9b811fc6ca..a60ebfc2c1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -432,7 +432,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
>      m->is_default = false;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> -    m->smp_prefer_sockets = true;
> +    m->smp_props.prefer_sockets = true;
>  }
>  
>  DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 88efb7fde4..4b622ffb82 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -372,7 +372,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
>      m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> -    m->smp_prefer_sockets = true;
> +    m->smp_props.prefer_sockets = true;
>  }
>  
>  DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a481fade51..efdea43c0d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4702,7 +4702,7 @@ static void spapr_machine_6_1_class_options(MachineClass *mc)
>  {
>      spapr_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> -    mc->smp_prefer_sockets = true;
> +    mc->smp_props.prefer_sockets = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b40e647883..5bdef9b4d7 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -809,7 +809,7 @@ static void ccw_machine_6_1_class_options(MachineClass *mc)
>  {
>      ccw_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> -    mc->smp_prefer_sockets = true;
> +    mc->smp_props.prefer_sockets = true;
>  }
>  DEFINE_CCW_MACHINE(6_1, "6.1", false);
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 72123f594d..23671a0f8f 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -110,9 +110,11 @@ typedef struct {
>  
>  /**
>   * SMPCompatProps:
> + * @prefer_sockets - whether sockets is preferred over cores in smp parsing
>   * @dies_supported - whether dies is supported by the machine
>   */
>  typedef struct {
> +    bool prefer_sockets;
>      bool dies_supported;
>  } SMPCompatProps;
>  
> @@ -250,7 +252,6 @@ struct MachineClass {
>      bool nvdimm_supported;
>      bool numa_mem_supported;
>      bool auto_enable_numa;
> -    bool smp_prefer_sockets;
>      SMPCompatProps smp_props;
>      const char *default_ram_id;
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-6.2 v3 11/11] machine: Move smp_prefer_sockets to struct SMPCompatProps

[Andrew Jones]

On Wed, Jul 28, 2021 at 11:48:48AM +0800, Yanan Wang wrote:
> Now we have a common structure SMPCompatProps used to store information
> about SMP compatibility stuff, so we can also move smp_prefer_sockets
> there for cleaner code.
> 
> No functional change intended.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/arm/virt.c              | 2 +-
>  hw/core/machine.c          | 2 +-
>  hw/i386/pc_piix.c          | 2 +-
>  hw/i386/pc_q35.c           | 2 +-
>  hw/ppc/spapr.c             | 2 +-
>  hw/s390x/s390-virtio-ccw.c | 2 +-
>  include/hw/boards.h        | 3 ++-
>  7 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7babea40dc..ae029680da 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2797,7 +2797,7 @@ static void virt_machine_6_1_options(MachineClass *mc)
>  {
>      virt_machine_6_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> -    mc->smp_prefer_sockets = true;
> +    mc->smp_props.prefer_sockets = true;
>  }
>  DEFINE_VIRT_MACHINE(6, 1)
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8f84e38e2e..61d1f643f4 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -834,7 +834,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>      } else {
>          maxcpus = maxcpus > 0 ? maxcpus : cpus;
>  
> -        if (mc->smp_prefer_sockets) {
> +        if (mc->smp_props.prefer_sockets) {
>              /* prefer sockets over cores over threads before 6.2 */
>              if (sockets == 0) {
>                  cores = cores > 0 ? cores : 1;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 9b811fc6ca..a60ebfc2c1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -432,7 +432,7 @@ static void pc_i440fx_6_1_machine_options(MachineClass *m)
>      m->is_default = false;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> -    m->smp_prefer_sockets = true;
> +    m->smp_props.prefer_sockets = true;
>  }
>  
>  DEFINE_I440FX_MACHINE(v6_1, "pc-i440fx-6.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 88efb7fde4..4b622ffb82 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -372,7 +372,7 @@ static void pc_q35_6_1_machine_options(MachineClass *m)
>      m->alias = NULL;
>      compat_props_add(m->compat_props, hw_compat_6_1, hw_compat_6_1_len);
>      compat_props_add(m->compat_props, pc_compat_6_1, pc_compat_6_1_len);
> -    m->smp_prefer_sockets = true;
> +    m->smp_props.prefer_sockets = true;
>  }
>  
>  DEFINE_Q35_MACHINE(v6_1, "pc-q35-6.1", NULL,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a481fade51..efdea43c0d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4702,7 +4702,7 @@ static void spapr_machine_6_1_class_options(MachineClass *mc)
>  {
>      spapr_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> -    mc->smp_prefer_sockets = true;
> +    mc->smp_props.prefer_sockets = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(6_1, "6.1", false);
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b40e647883..5bdef9b4d7 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -809,7 +809,7 @@ static void ccw_machine_6_1_class_options(MachineClass *mc)
>  {
>      ccw_machine_6_2_class_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_6_1, hw_compat_6_1_len);
> -    mc->smp_prefer_sockets = true;
> +    mc->smp_props.prefer_sockets = true;
>  }
>  DEFINE_CCW_MACHINE(6_1, "6.1", false);
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 72123f594d..23671a0f8f 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -110,9 +110,11 @@ typedef struct {
>  
>  /**
>   * SMPCompatProps:
> + * @prefer_sockets - whether sockets is preferred over cores in smp parsing
>   * @dies_supported - whether dies is supported by the machine
>   */
>  typedef struct {
> +    bool prefer_sockets;
>      bool dies_supported;
>  } SMPCompatProps;
>  
> @@ -250,7 +252,6 @@ struct MachineClass {
>      bool nvdimm_supported;
>      bool numa_mem_supported;
>      bool auto_enable_numa;
> -    bool smp_prefer_sockets;
>      SMPCompatProps smp_props;
>      const char *default_ram_id;
>  
> -- 
> 2.19.1
>

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