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

[wangyanan (Y)]

On 2021/7/20 0:53, Daniel P. Berrangé wrote:

On Mon, Jul 19, 2021 at 11:20:34AM +0800, Yanan Wang wrote:

Currently the only difference between smp_parse and pc_smp_parse
is the support of multi-dies and the related error reporting code.
With an arch compat variable "bool smp_dies_supported", we can
easily 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.

No functional change intended.

Suggested-by: Andrew Jones <drjo...@redhat.com>
Signed-off-by: Yanan Wang <wangyana...@huawei.com>
---
  hw/core/machine.c   | 28 ++++++++++-------
  hw/i386/pc.c        | 76 +--------------------------------------------
  include/hw/boards.h |  1 +
  3 files changed, 19 insertions(+), 86 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index d73daa10f4..ed6712e964 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -743,6 +743,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 dies    = config->has_dies ? config->dies : 1;
@@ -761,7 +762,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
*config, Error **errp)
          return;
      }

- if (dies > 1) {

+    if (!mc->smp_dies_supported && dies > 1) {
          error_setg(errp, "dies not supported by this machine's CPU topology");
          return;
      }
@@ -772,23 +773,25 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
*config, Error **errp)
          threads = threads > 0 ? threads : 1;
          if (cpus == 0) {
              sockets = sockets > 0 ? sockets : 1;
-            cpus = cores * threads * sockets;
+            cpus = sockets * dies * cores * threads;
          } else {
              maxcpus = maxcpus > 0 ? maxcpus : cpus;
-            sockets = maxcpus / (cores * threads);
+            sockets = maxcpus / (dies * cores * threads);
          }
      } else if (cores == 0) {
          threads = threads > 0 ? threads : 1;
-        cores = cpus / (sockets * threads);
+        cores = cpus / (sockets * dies * threads);
          cores = cores > 0 ? cores : 1;
      } else if (threads == 0) {
-        threads = cpus / (cores * sockets);
+        threads = cpus / (sockets * dies * cores);
          threads = threads > 0 ? threads : 1;
-    } else if (sockets * cores * threads < cpus) {
+    } else if (sockets * dies * cores * threads < cpus) {
+        g_autofree char *dies_msg = g_strdup_printf(
+            mc->smp_dies_supported ? " * dies (%u)" : "", dies);
          error_setg(errp, "cpu topology: "
-                   "sockets (%u) * cores (%u) * threads (%u) < "
+                   "sockets (%u)%s * cores (%u) * threads (%u) < "
                     "smp_cpus (%u)",
-                   sockets, cores, threads, cpus);
+                   sockets, dies_msg, cores, threads, cpus);

Since we're allowing dies=1 (but not greater), I'm not convinced we
need the conditionally built error message, and could just include
"* dies" all the time.

If we do want it to be conditionally different though, I'd just
sugest calling error_setg twice. Although this duplicates stuff,
it'll be clearer to read which I think is a net win.

The duplicates may increase quickly if more arch specific members
are introduced, I think the conditional error reporting may still be
necessary, but should be more clearer than current approach. :)

Thanks,
Yanan
.