[PATCH v4 0/2] hw/core/machine: Add an unit test for smp_parse

[PATCH v4 0/2] hw/core/machine: Add an unit test for smp_parse

[Philippe Mathieu-Daudé]

Respin of Yanan Wang v3, based on
"hw/core: Restrict qdev-hotplug to sysemu"

Based-on: 20211028150521.1973821-1-philmd@redhat.com
https://lore.kernel.org/qemu-devel/20211028150521.1973821-1-philmd@redhat.com

git-backport-diff:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0001] [FC] 'hw/core/machine: Split out the smp parsing code'
002/2:[----] [--] 'tests/unit: Add an unit test for smp parsing'

Yanan Wang (2):
  hw/core/machine: Split out the smp parsing code
  tests/unit: Add an unit test for smp parsing

 include/hw/boards.h         |   1 +
 hw/core/machine-smp.c       | 181 +++++++++++
 hw/core/machine.c           | 159 ----------
 tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |   2 +
 hw/core/meson.build         |   1 +
 tests/unit/meson.build      |   1 +
 7 files changed, 780 insertions(+), 159 deletions(-)
 create mode 100644 hw/core/machine-smp.c
 create mode 100644 tests/unit/test-smp-parse.c

-- 
2.31.1

[PATCH v4 1/2] hw/core/machine: Split out the smp parsing code

[Philippe Mathieu-Daudé]

From: Yanan Wang <wangyanan55@huawei.com>

We are going to introduce an unit test for the parser smp_parse()
in hw/core/machine.c, but now machine.c is only built in softmmu.

In order to solve the build dependency on the smp parsing code and
avoid building unrelated stuff for the unit tests, move the tested
code from machine.c into a separate file, i.e., machine-smp.c and
build it in common field.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20211026034659.22040-2-wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/boards.h   |   1 +
 hw/core/machine-smp.c | 181 ++++++++++++++++++++++++++++++++++++++++++
 hw/core/machine.c     | 159 -------------------------------------
 MAINTAINERS           |   1 +
 hw/core/meson.build   |   1 +
 5 files changed, 184 insertions(+), 159 deletions(-)
 create mode 100644 hw/core/machine-smp.c

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 5adbcbb99b1..e36fc7d8615 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -34,6 +34,7 @@ HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
 void machine_set_cpu_numa_node(MachineState *machine,
                                const CpuInstanceProperties *props,
                                Error **errp);
+void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp);
 
 /**
  * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
new file mode 100644
index 00000000000..116a0cbbfab
--- /dev/null
+++ b/hw/core/machine-smp.c
@@ -0,0 +1,181 @@
+/*
+ * QEMU Machine core (related to -smp parsing)
+ *
+ * Copyright (c) 2021 Huawei Technologies Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/boards.h"
+#include "qapi/error.h"
+
+/*
+ * Report information of a machine's supported CPU topology hierarchy.
+ * Topology members will be ordered from the largest to the smallest
+ * in the string.
+ */
+static char *cpu_hierarchy_to_string(MachineState *ms)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    GString *s = g_string_new(NULL);
+
+    g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
+
+    if (mc->smp_props.dies_supported) {
+        g_string_append_printf(s, " * dies (%u)", ms->smp.dies);
+    }
+
+    g_string_append_printf(s, " * cores (%u)", ms->smp.cores);
+    g_string_append_printf(s, " * threads (%u)", ms->smp.threads);
+
+    return g_string_free(s, false);
+}
+
+/*
+ * smp_parse - Generic function used to parse the given SMP configuration
+ *
+ * Any missing parameter in "cpus/maxcpus/sockets/cores/threads" will be
+ * automatically computed based on the provided ones.
+ *
+ * In the 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.
+ *
+ * In the calculation of cpus/maxcpus: 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.
+ *
+ * For compatibility, apart from the parameters that will be computed, newly
+ * introduced topology members which are likely to be target specific should
+ * be directly set as 1 if they are omitted (e.g. dies for PC since 4.1).
+ */
+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;
+
+    /*
+     * Specified CPU topology parameters must be greater than zero,
+     * explicit configuration like "cpus=0" is not allowed.
+     */
+    if ((config->has_cpus && config->cpus == 0) ||
+        (config->has_sockets && config->sockets == 0) ||
+        (config->has_dies && config->dies == 0) ||
+        (config->has_cores && config->cores == 0) ||
+        (config->has_threads && config->threads == 0) ||
+        (config->has_maxcpus && config->maxcpus == 0)) {
+        warn_report("Deprecated CPU topology (considered invalid): "
+                    "CPU topology parameters must be greater than zero");
+    }
+
+    /*
+     * If not supported by the machine, a topology parameter must be
+     * omitted or specified equal to 1.
+     */
+    if (!mc->smp_props.dies_supported && dies > 1) {
+        error_setg(errp, "dies not supported by this machine's CPU topology");
+        return;
+    }
+
+    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_props.prefer_sockets) {
+            /* prefer sockets over cores 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 {
+            /* prefer cores over sockets 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);
+            }
+        }
+
+        /* try to calculate omitted threads at last */
+        if (threads == 0) {
+            threads = maxcpus / (sockets * dies * cores);
+        }
+    }
+
+    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * 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 * dies * cores * threads != maxcpus) {
+        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
+        error_setg(errp, "Invalid CPU topology: "
+                   "product of the hierarchy must match maxcpus: "
+                   "%s != maxcpus (%u)",
+                   topo_msg, maxcpus);
+        return;
+    }
+
+    if (maxcpus < cpus) {
+        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
+        error_setg(errp, "Invalid CPU topology: "
+                   "maxcpus must be equal to or greater than smp: "
+                   "%s == maxcpus (%u) < smp_cpus (%u)",
+                   topo_msg, maxcpus, cpus);
+        return;
+    }
+
+    if (ms->smp.cpus < mc->min_cpus) {
+        error_setg(errp, "Invalid SMP CPUs %d. The min CPUs "
+                   "supported by machine '%s' is %d",
+                   ms->smp.cpus,
+                   mc->name, mc->min_cpus);
+        return;
+    }
+
+    if (ms->smp.max_cpus > mc->max_cpus) {
+        error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
+                   "supported by machine '%s' is %d",
+                   ms->smp.max_cpus,
+                   mc->name, mc->max_cpus);
+        return;
+    }
+}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index b8d95eec32d..dc15f5f9e5c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -749,165 +749,6 @@ void machine_set_cpu_numa_node(MachineState *machine,
     }
 }
 
-/*
- * Report information of a machine's supported CPU topology hierarchy.
- * Topology members will be ordered from the largest to the smallest
- * in the string.
- */
-static char *cpu_hierarchy_to_string(MachineState *ms)
-{
-    MachineClass *mc = MACHINE_GET_CLASS(ms);
-    GString *s = g_string_new(NULL);
-
-    g_string_append_printf(s, "sockets (%u)", ms->smp.sockets);
-
-    if (mc->smp_props.dies_supported) {
-        g_string_append_printf(s, " * dies (%u)", ms->smp.dies);
-    }
-
-    g_string_append_printf(s, " * cores (%u)", ms->smp.cores);
-    g_string_append_printf(s, " * threads (%u)", ms->smp.threads);
-
-    return g_string_free(s, false);
-}
-
-/*
- * smp_parse - Generic function used to parse the given SMP configuration
- *
- * Any missing parameter in "cpus/maxcpus/sockets/cores/threads" will be
- * automatically computed based on the provided ones.
- *
- * In the 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.
- *
- * In the calculation of cpus/maxcpus: 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.
- *
- * For compatibility, apart from the parameters that will be computed, newly
- * introduced topology members which are likely to be target specific should
- * be directly set as 1 if they are omitted (e.g. dies for PC since 4.1).
- */
-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;
-
-    /*
-     * Specified CPU topology parameters must be greater than zero,
-     * explicit configuration like "cpus=0" is not allowed.
-     */
-    if ((config->has_cpus && config->cpus == 0) ||
-        (config->has_sockets && config->sockets == 0) ||
-        (config->has_dies && config->dies == 0) ||
-        (config->has_cores && config->cores == 0) ||
-        (config->has_threads && config->threads == 0) ||
-        (config->has_maxcpus && config->maxcpus == 0)) {
-        warn_report("Deprecated CPU topology (considered invalid): "
-                    "CPU topology parameters must be greater than zero");
-    }
-
-    /*
-     * If not supported by the machine, a topology parameter must be
-     * omitted or specified equal to 1.
-     */
-    if (!mc->smp_props.dies_supported && dies > 1) {
-        error_setg(errp, "dies not supported by this machine's CPU topology");
-        return;
-    }
-
-    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_props.prefer_sockets) {
-            /* prefer sockets over cores 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 {
-            /* prefer cores over sockets 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);
-            }
-        }
-
-        /* try to calculate omitted threads at last */
-        if (threads == 0) {
-            threads = maxcpus / (sockets * dies * cores);
-        }
-    }
-
-    maxcpus = maxcpus > 0 ? maxcpus : sockets * dies * 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 * dies * cores * threads != maxcpus) {
-        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
-        error_setg(errp, "Invalid CPU topology: "
-                   "product of the hierarchy must match maxcpus: "
-                   "%s != maxcpus (%u)",
-                   topo_msg, maxcpus);
-        return;
-    }
-
-    if (maxcpus < cpus) {
-        g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
-        error_setg(errp, "Invalid CPU topology: "
-                   "maxcpus must be equal to or greater than smp: "
-                   "%s == maxcpus (%u) < smp_cpus (%u)",
-                   topo_msg, maxcpus, cpus);
-        return;
-    }
-
-    if (ms->smp.cpus < mc->min_cpus) {
-        error_setg(errp, "Invalid SMP CPUs %d. The min CPUs "
-                   "supported by machine '%s' is %d",
-                   ms->smp.cpus,
-                   mc->name, mc->min_cpus);
-        return;
-    }
-
-    if (ms->smp.max_cpus > mc->max_cpus) {
-        error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
-                   "supported by machine '%s' is %d",
-                   ms->smp.max_cpus,
-                   mc->name, mc->max_cpus);
-        return;
-    }
-}
-
 static void machine_get_smp(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
diff --git a/MAINTAINERS b/MAINTAINERS
index 894dc431052..80ec27d76ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1623,6 +1623,7 @@ F: cpu.c
 F: hw/core/cpu.c
 F: hw/core/machine-qmp-cmds.c
 F: hw/core/machine.c
+F: hw/core/machine-smp.c
 F: hw/core/null-machine.c
 F: hw/core/numa.c
 F: hw/cpu/cluster.c
diff --git a/hw/core/meson.build b/hw/core/meson.build
index afc5ed2c906..987bb118c3d 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -23,6 +23,7 @@
 endif
 
 common_ss.add(files('cpu-common.c'))
+common_ss.add(files('machine-smp.c'))
 softmmu_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
 softmmu_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: files('generic-loader.c'))
 softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.c'))
-- 
2.31.1

[PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[Philippe Mathieu-Daudé]

From: Yanan Wang <wangyanan55@huawei.com>

Now that we have a generic parser smp_parse(), let's add an unit
test for the code. All possible valid/invalid SMP configurations
that the user can specify are covered.

Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |   1 +
 tests/unit/meson.build      |   1 +
 3 files changed, 596 insertions(+)
 create mode 100644 tests/unit/test-smp-parse.c

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
new file mode 100644
index 00000000000..e96abe9ba46
--- /dev/null
+++ b/tests/unit/test-smp-parse.c
@@ -0,0 +1,594 @@
+/*
+ * SMP parsing unit-tests
+ *
+ * Copyright (c) 2021 Huawei Technologies Co., Ltd
+ *
+ * Authors:
+ *  Yanan Wang <wangyanan55@huawei.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/object.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+
+#include "hw/boards.h"
+
+#define T true
+#define F false
+
+#define MIN_CPUS 1   /* set the min CPUs supported by the machine as 1 */
+#define MAX_CPUS 512 /* set the max CPUs supported by the machine as 512 */
+
+/*
+ * Used to define the generic 3-level CPU topology hierarchy
+ *  -sockets/cores/threads
+ */
+#define SMP_CONFIG_GENERIC(ha, a, hb, b, hc, c, hd, d, he, e) \
+        {                                                     \
+            .has_cpus    = ha, .cpus    = a,                  \
+            .has_sockets = hb, .sockets = b,                  \
+            .has_cores   = hc, .cores   = c,                  \
+            .has_threads = hd, .threads = d,                  \
+            .has_maxcpus = he, .maxcpus = e,                  \
+        }
+
+#define CPU_TOPOLOGY_GENERIC(a, b, c, d, e)                   \
+        {                                                     \
+            .cpus     = a,                                    \
+            .sockets  = b,                                    \
+            .cores    = c,                                    \
+            .threads  = d,                                    \
+            .max_cpus = e,                                    \
+        }
+
+/*
+ * Currently a 4-level topology hierarchy is supported on PC machines
+ *  -sockets/dies/cores/threads
+ */
+#define SMP_CONFIG_WITH_DIES(ha, a, hb, b, hc, c, hd, d, he, e, hf, f) \
+        {                                                     \
+            .has_cpus    = ha, .cpus    = a,                  \
+            .has_sockets = hb, .sockets = b,                  \
+            .has_dies    = hc, .dies    = c,                  \
+            .has_cores   = hd, .cores   = d,                  \
+            .has_threads = he, .threads = e,                  \
+            .has_maxcpus = hf, .maxcpus = f,                  \
+        }
+
+/**
+ * @config - the given SMP configuration
+ * @expect_prefer_sockets - the expected parsing result for the
+ * valid configuration, when sockets are preferred over cores
+ * @expect_prefer_cores - the expected parsing result for the
+ * valid configuration, when cores are preferred over sockets
+ * @expect_error - the expected error report when the given
+ * configuration is invalid
+ */
+typedef struct SMPTestData {
+    SMPConfiguration config;
+    CpuTopology expect_prefer_sockets;
+    CpuTopology expect_prefer_cores;
+    const char *expect_error;
+} SMPTestData;
+
+/* Type info of the tested machine */
+static const TypeInfo smp_machine_info = {
+    .name = TYPE_MACHINE,
+    .parent = TYPE_OBJECT,
+    .class_size = sizeof(MachineClass),
+    .instance_size = sizeof(MachineState),
+};
+
+/*
+ * List all the possible valid sub-collections of the generic 5
+ * topology parameters (i.e. cpus/maxcpus/sockets/cores/threads),
+ * then test the automatic calculation algorithm of the missing
+ * values in the parser.
+ */
+static struct SMPTestData data_generic_valid[] = {
+    {
+        /* config: no configuration provided
+         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
+    }, {
+        /* config: -smp 8
+         * prefer_sockets: cpus=8,sockets=8,cores=1,threads=1,maxcpus=8
+         * prefer_cores: cpus=8,sockets=1,cores=8,threads=1,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 8, 1, 1, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 8, 1, 8),
+    }, {
+        /* config: -smp sockets=2
+         * expect: cpus=2,sockets=2,cores=1,threads=1,maxcpus=2 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(2, 2, 1, 1, 2),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(2, 2, 1, 1, 2),
+    }, {
+        /* config: -smp cores=4
+         * expect: cpus=4,sockets=1,cores=4,threads=1,maxcpus=4 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(4, 1, 4, 1, 4),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(4, 1, 4, 1, 4),
+    }, {
+        /* config: -smp threads=2
+         * expect: cpus=2,sockets=1,cores=1,threads=2,maxcpus=2 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(2, 1, 1, 2, 2),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(2, 1, 1, 2, 2),
+    }, {
+        /* config: -smp maxcpus=16
+         * prefer_sockets: cpus=16,sockets=16,cores=1,threads=1,maxcpus=16
+         * prefer_cores: cpus=16,sockets=1,cores=16,threads=1,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 16, 1, 1, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 1, 16, 1, 16),
+    }, {
+        /* config: -smp 8,sockets=2
+         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+    }, {
+        /* config: -smp 8,cores=4
+         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+    }, {
+        /* config: -smp 8,threads=2
+         * prefer_sockets: cpus=8,sockets=4,cores=1,threads=2,maxcpus=8
+         * prefer_cores: cpus=8,sockets=1,cores=4,threads=2,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 4, 1, 2, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
+    }, {
+        /* config: -smp 8,maxcpus=16
+         * prefer_sockets: cpus=8,sockets=16,cores=1,threads=1,maxcpus=16
+         * prefer_cores: cpus=8,sockets=1,cores=16,threads=1,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 16, 1, 1, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 16, 1, 16),
+    }, {
+        /* config: -smp sockets=2,cores=4
+         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+    }, {
+        /* config: -smp sockets=2,threads=2
+         * expect: cpus=4,sockets=2,cores=1,threads=2,maxcpus=4 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(4, 2, 1, 2, 4),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(4, 2, 1, 2, 4),
+    }, {
+        /* config: -smp sockets=2,maxcpus=16
+         * expect: cpus=16,sockets=2,cores=8,threads=1,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 8, 1, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 8, 1, 16),
+    }, {
+        /* config: -smp cores=4,threads=2
+         * expect: cpus=8,sockets=1,cores=4,threads=2,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
+    }, {
+        /* config: -smp cores=4,maxcpus=16
+         * expect: cpus=16,sockets=4,cores=4,threads=1,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 4, 4, 1, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 4, 4, 1, 16),
+    }, {
+        /* config: -smp threads=2,maxcpus=16
+         * prefer_sockets: cpus=16,sockets=8,cores=1,threads=2,maxcpus=16
+         * prefer_cores: cpus=16,sockets=1,cores=8,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 8, 1, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 1, 8, 2, 16),
+    }, {
+        /* config: -smp 8,sockets=2,cores=4
+         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, F, 0, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+    }, {
+        /* config: -smp 8,sockets=2,threads=2
+         * expect: cpus=8,sockets=2,cores=2,threads=2,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
+    }, {
+        /* config: -smp 8,sockets=2,maxcpus=16
+         * expect: cpus=8,sockets=2,cores=8,threads=1,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 8, 1, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 8, 1, 16),
+    }, {
+        /* config: -smp 8,cores=4,threads=2
+         * expect: cpus=8,sockets=1,cores=4,threads=2,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
+    }, {
+        /* config: -smp 8,cores=4,maxcpus=16
+         * expect: cpus=8,sockets=4,cores=4,threads=1,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 4, 4, 1, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 4, 4, 1, 16),
+    }, {
+        /* config: -smp 8,threads=2,maxcpus=16
+         * prefer_sockets: cpus=8,sockets=8,cores=1,threads=2,maxcpus=16
+         * prefer_cores: cpus=8,sockets=1,cores=8,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 8, 1, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 8, 2, 16),
+    }, {
+        /* config: -smp sockets=2,cores=4,threads=2
+         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, T, 2, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+    }, {
+        /* config: -smp sockets=2,cores=4,maxcpus=16
+         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+    }, {
+        /* config: -smp sockets=2,threads=2,maxcpus=16
+         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+    }, {
+        /* config: -smp cores=4,threads=2,maxcpus=16
+         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+    }, {
+        /* config: -smp 8,sockets=2,cores=4,threads=1
+         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 1, F, 0),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
+    }, {
+        /* config: -smp 8,sockets=2,cores=4,maxcpus=16
+         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, F, 0, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+    }, {
+        /* config: -smp 8,sockets=2,threads=2,maxcpus=16
+         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+    }, {
+        /* config: -smp 8,cores=4,threads=2,maxcpus=16
+         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+    }, {
+        /* config: -smp sockets=2,cores=4,threads=2,maxcpus=16
+         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
+    }, {
+        /* config: -smp 8,sockets=2,cores=4,threads=2,maxcpus=16
+         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 16),
+        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
+    },
+};
+
+static struct SMPTestData data_generic_invalid[] = {
+    {
+        /* config: -smp 2,dies=2 */
+        .config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
+        .expect_error = "dies not supported by this machine's CPU topology",
+    }, {
+        /* config: -smp 8,sockets=2,cores=4,threads=2,maxcpus=8 */
+        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 8),
+        .expect_error = "Invalid CPU topology: "
+                        "product of the hierarchy must match maxcpus: "
+                        "sockets (2) * cores (4) * threads (2) "
+                        "!= maxcpus (8)",
+    }, {
+        /* config: -smp 18,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_GENERIC(T, 18, T, 2, T, 4, T, 2, T, 16),
+        .expect_error = "Invalid CPU topology: "
+                        "maxcpus must be equal to or greater than smp: "
+                        "sockets (2) * cores (4) * threads (2) "
+                        "== maxcpus (16) < smp_cpus (18)",
+    }, {
+        /* config: -smp 1
+         * should tweak the supported min CPUs to 2 for testing */
+        .config = SMP_CONFIG_GENERIC(T, 1, F, 0, F, 0, F, 0, F, 0),
+        .expect_error = "Invalid SMP CPUs 1. The min CPUs supported "
+                        "by machine '(null)' is 2",
+    }, {
+        /* config: -smp 512
+         * should tweak the supported max CPUs to 511 for testing */
+        .config = SMP_CONFIG_GENERIC(T, 512, F, 0, F, 0, F, 0, F, 0),
+        .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
+                        "by machine '(null)' is 511",
+    },
+};
+
+static struct SMPTestData data_with_dies_invalid[] = {
+    {
+        /* config: -smp 16,sockets=2,dies=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_WITH_DIES(T, 16, T, 2, T, 2, T, 4, T, 2, T, 16),
+        .expect_error = "Invalid CPU topology: "
+                        "product of the hierarchy must match maxcpus: "
+                        "sockets (2) * dies (2) * cores (4) * threads (2) "
+                        "!= maxcpus (16)",
+    }, {
+        /* config: -smp 34,sockets=2,dies=2,cores=4,threads=2,maxcpus=32 */
+        .config = SMP_CONFIG_WITH_DIES(T, 34, T, 2, T, 2, T, 4, T, 2, T, 32),
+        .expect_error = "Invalid CPU topology: "
+                        "maxcpus must be equal to or greater than smp: "
+                        "sockets (2) * dies (2) * cores (4) * threads (2) "
+                        "== maxcpus (32) < smp_cpus (34)",
+    },
+};
+
+static char *smp_config_to_string(SMPConfiguration *config)
+{
+    return g_strdup_printf(
+        "(SMPConfiguration) {\n"
+        "    .has_cpus    = %5s, cpus    = %ld,\n"
+        "    .has_sockets = %5s, sockets = %ld,\n"
+        "    .has_dies    = %5s, dies    = %ld,\n"
+        "    .has_cores   = %5s, cores   = %ld,\n"
+        "    .has_threads = %5s, threads = %ld,\n"
+        "    .has_maxcpus = %5s, maxcpus = %ld,\n"
+        "}",
+        config->has_cpus ? "true" : "false", config->cpus,
+        config->has_sockets ? "true" : "false", config->sockets,
+        config->has_dies ? "true" : "false", config->dies,
+        config->has_cores ? "true" : "false", config->cores,
+        config->has_threads ? "true" : "false", config->threads,
+        config->has_maxcpus ? "true" : "false", config->maxcpus);
+}
+
+static char *cpu_topology_to_string(CpuTopology *topo)
+{
+    return g_strdup_printf(
+        "(CpuTopology) {\n"
+        "    .cpus     = %u,\n"
+        "    .sockets  = %u,\n"
+        "    .dies     = %u,\n"
+        "    .cores    = %u,\n"
+        "    .threads  = %u,\n"
+        "    .max_cpus = %u,\n"
+        "}",
+        topo->cpus, topo->sockets, topo->dies,
+        topo->cores, topo->threads, topo->max_cpus);
+}
+
+static void check_parse(MachineState *ms, SMPConfiguration *config,
+                        CpuTopology *expect_topo, const char *expect_err,
+                        bool is_valid)
+{
+    g_autofree char *config_str = smp_config_to_string(config);
+    g_autofree char *expect_topo_str = cpu_topology_to_string(expect_topo);
+    g_autofree char *output_topo_str = NULL;
+    Error *err = NULL;
+
+    /* call the generic parser smp_parse() */
+    smp_parse(ms, config, &err);
+
+    output_topo_str = cpu_topology_to_string(&ms->smp);
+
+    /* when the configuration is supposed to be valid */
+    if (is_valid) {
+        if ((err == NULL) &&
+            (ms->smp.cpus == expect_topo->cpus) &&
+            (ms->smp.sockets == expect_topo->sockets) &&
+            (ms->smp.dies == expect_topo->dies) &&
+            (ms->smp.cores == expect_topo->cores) &&
+            (ms->smp.threads == expect_topo->threads) &&
+            (ms->smp.max_cpus == expect_topo->max_cpus)) {
+            return;
+        }
+
+        if (err != NULL) {
+            g_printerr("Test smp_parse failed!\n"
+                       "Input configuration: %s\n"
+                       "Should be valid: yes\n"
+                       "Expected topology: %s\n\n"
+                       "Result is valid: no\n"
+                       "Output error report: %s\n",
+                       config_str, expect_topo_str, error_get_pretty(err));
+            goto end;
+        }
+
+        g_printerr("Test smp_parse failed!\n"
+                   "Input configuration: %s\n"
+                   "Should be valid: yes\n"
+                   "Expected topology: %s\n\n"
+                   "Result is valid: yes\n"
+                   "Output topology: %s\n",
+                   config_str, expect_topo_str, output_topo_str);
+        goto end;
+    }
+
+    /* when the configuration is supposed to be invalid */
+    if (err != NULL) {
+        if (expect_err == NULL ||
+            g_str_equal(expect_err, error_get_pretty(err))) {
+            error_free(err);
+            return;
+        }
+
+        g_printerr("Test smp_parse failed!\n"
+                   "Input configuration: %s\n"
+                   "Should be valid: no\n"
+                   "Expected error report: %s\n\n"
+                   "Result is valid: no\n"
+                   "Output error report: %s\n",
+                   config_str, expect_err, error_get_pretty(err));
+        goto end;
+    }
+
+    g_printerr("Test smp_parse failed!\n"
+               "Input configuration: %s\n"
+               "Should be valid: no\n"
+               "Expected error report: %s\n\n"
+               "Result is valid: yes\n"
+               "Output topology: %s\n",
+               config_str, expect_err, output_topo_str);
+
+end:
+    if (err != NULL) {
+        error_free(err);
+    }
+
+    abort();
+}
+
+static void smp_parse_test(MachineState *ms, SMPTestData *data, bool is_valid)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    mc->smp_props.prefer_sockets = true;
+    check_parse(ms, &data->config, &data->expect_prefer_sockets,
+                data->expect_error, is_valid);
+
+    mc->smp_props.prefer_sockets = false;
+    check_parse(ms, &data->config, &data->expect_prefer_cores,
+                data->expect_error, is_valid);
+}
+
+/* The parsed results of the unsupported parameters should be 1 */
+static void unsupported_params_init(MachineClass *mc, SMPTestData *data)
+{
+    if (!mc->smp_props.dies_supported) {
+        data->expect_prefer_sockets.dies = 1;
+        data->expect_prefer_cores.dies = 1;
+    }
+}
+
+/* Reset the related machine properties before each sub-test */
+static void smp_machine_class_init(MachineClass *mc)
+{
+    mc->min_cpus = MIN_CPUS;
+    mc->max_cpus = MAX_CPUS;
+
+    mc->smp_props.prefer_sockets = true;
+    mc->smp_props.dies_supported = false;
+}
+
+static void test_generic(void)
+{
+    Object *obj = object_new(TYPE_MACHINE);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData *data = &(SMPTestData){0};
+    int i;
+
+    smp_machine_class_init(mc);
+
+    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+        *data = data_generic_valid[i];
+        unsupported_params_init(mc, data);
+
+        smp_parse_test(ms, data, true);
+
+        /* Unsupported parameters can be provided with their values as 1 */
+        data->config.has_dies = true;
+        data->config.dies = 1;
+        smp_parse_test(ms, data, true);
+    }
+
+    /* Reset the supported min CPUs and max CPUs */
+    mc->min_cpus = 2;
+    mc->max_cpus = 511;
+
+    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
+        *data = data_generic_invalid[i];
+        unsupported_params_init(mc, data);
+
+        smp_parse_test(ms, data, false);
+    }
+
+    object_unref(obj);
+}
+
+static void test_with_dies(void)
+{
+    Object *obj = object_new(TYPE_MACHINE);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData *data = &(SMPTestData){0};
+    unsigned int num_dies = 2;
+    int i;
+
+    smp_machine_class_init(mc);
+    mc->smp_props.dies_supported = true;
+
+    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+        *data = data_generic_valid[i];
+        unsupported_params_init(mc, data);
+
+        /* when dies parameter is omitted, it will be set as 1 */
+        data->expect_prefer_sockets.dies = 1;
+        data->expect_prefer_cores.dies = 1;
+
+        smp_parse_test(ms, data, true);
+
+        /* when dies parameter is specified */
+        data->config.has_dies = true;
+        data->config.dies = num_dies;
+        if (data->config.has_cpus) {
+            data->config.cpus *= num_dies;
+        }
+        if (data->config.has_maxcpus) {
+            data->config.maxcpus *= num_dies;
+        }
+
+        data->expect_prefer_sockets.dies = num_dies;
+        data->expect_prefer_sockets.cpus *= num_dies;
+        data->expect_prefer_sockets.max_cpus *= num_dies;
+        data->expect_prefer_cores.dies = num_dies;
+        data->expect_prefer_cores.cpus *= num_dies;
+        data->expect_prefer_cores.max_cpus *= num_dies;
+
+        smp_parse_test(ms, data, true);
+    }
+
+    for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
+        *data = data_with_dies_invalid[i];
+        unsupported_params_init(mc, data);
+
+        smp_parse_test(ms, data, false);
+    }
+
+    object_unref(obj);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    module_call_init(MODULE_INIT_QOM);
+    type_register_static(&smp_machine_info);
+
+    g_test_add_func("/test-smp-parse/generic", test_generic);
+    g_test_add_func("/test-smp-parse/with_dies", test_with_dies);
+
+    g_test_run();
+
+    return 0;
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 80ec27d76ae..310a9512ea1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1633,6 +1633,7 @@ F: include/hw/boards.h
 F: include/hw/core/cpu.h
 F: include/hw/cpu/cluster.h
 F: include/sysemu/numa.h
+F: tests/unit/test-smp-parse.c
 T: git https://gitlab.com/ehabkost/qemu.git machine-next
 
 Xtensa Machines
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 7c297d7e5c0..5e81525bef1 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -45,6 +45,7 @@
   'test-uuid': [],
   'ptimer-test': ['ptimer-test-stubs.c', meson.project_source_root() / 'hw/core/ptimer.c'],
   'test-qapi-util': [],
+  'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c'],
 }
 
 if have_system or have_tools
-- 
2.31.1

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[wangyanan (Y)]

Hi Philippe,

I saw that there are some cross-build failures and a clang complain
about this patch in your triggered CI pipeline. I believe the minor
diff below will resolve them. If you are going to resend v2 of the
"qdev-hotplug" patches, I would very much appreciate it if you
can also help to respin v5 of this series with the diff applied.
Sorry for the extra work included. :)

Thanks,
Yanan

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index e96abe9ba4..61985094f5 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -347,12 +347,12 @@ static char *smp_config_to_string(SMPConfiguration 
*config)
  {
      return g_strdup_printf(
          "(SMPConfiguration) {\n"
-        "    .has_cpus    = %5s, cpus    = %ld,\n"
-        "    .has_sockets = %5s, sockets = %ld,\n"
-        "    .has_dies    = %5s, dies    = %ld,\n"
-        "    .has_cores   = %5s, cores   = %ld,\n"
-        "    .has_threads = %5s, threads = %ld,\n"
-        "    .has_maxcpus = %5s, maxcpus = %ld,\n"
+        "    .has_cpus    = %5s, cpus    = %" PRId64 ",\n"
+        "    .has_sockets = %5s, sockets = %" PRId64 ",\n"
+        "    .has_dies    = %5s, dies    = %" PRId64 ",\n"
+        "    .has_cores   = %5s, cores   = %" PRId64 ",\n"
+        "    .has_threads = %5s, threads = %" PRId64 ",\n"
+        "    .has_maxcpus = %5s, maxcpus = %" PRId64 ",\n"
          "}",
          config->has_cpus ? "true" : "false", config->cpus,
          config->has_sockets ? "true" : "false", config->sockets,
@@ -495,7 +495,7 @@ static void test_generic(void)
      Object *obj = object_new(TYPE_MACHINE);
      MachineState *ms = MACHINE(obj);
      MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){0};
+    SMPTestData *data = &(SMPTestData){{0}};
      int i;

      smp_machine_class_init(mc);
@@ -531,7 +531,7 @@ static void test_with_dies(void)
      Object *obj = object_new(TYPE_MACHINE);
      MachineState *ms = MACHINE(obj);
      MachineClass *mc = MACHINE_GET_CLASS(obj);
-    SMPTestData *data = &(SMPTestData){0};
+    SMPTestData *data = &(SMPTestData){{0}};
      unsigned int num_dies = 2;
      int i;

On 2021/10/28 23:09, Philippe Mathieu-Daudé wrote:
> From: Yanan Wang <wangyanan55@huawei.com>
>
> Now that we have a generic parser smp_parse(), let's add an unit
> test for the code. All possible valid/invalid SMP configurations
> that the user can specify are covered.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>   MAINTAINERS                 |   1 +
>   tests/unit/meson.build      |   1 +
>   3 files changed, 596 insertions(+)
>   create mode 100644 tests/unit/test-smp-parse.c
>
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> new file mode 100644
> index 00000000000..e96abe9ba46
> --- /dev/null
> +++ b/tests/unit/test-smp-parse.c
> @@ -0,0 +1,594 @@
> +/*
> + * SMP parsing unit-tests
> + *
> + * Copyright (c) 2021 Huawei Technologies Co., Ltd
> + *
> + * Authors:
> + *  Yanan Wang <wangyanan55@huawei.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qom/object.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +
> +#include "hw/boards.h"
> +
> +#define T true
> +#define F false
> +
> +#define MIN_CPUS 1   /* set the min CPUs supported by the machine as 1 */
> +#define MAX_CPUS 512 /* set the max CPUs supported by the machine as 512 */
> +
> +/*
> + * Used to define the generic 3-level CPU topology hierarchy
> + *  -sockets/cores/threads
> + */
> +#define SMP_CONFIG_GENERIC(ha, a, hb, b, hc, c, hd, d, he, e) \
> +        {                                                     \
> +            .has_cpus    = ha, .cpus    = a,                  \
> +            .has_sockets = hb, .sockets = b,                  \
> +            .has_cores   = hc, .cores   = c,                  \
> +            .has_threads = hd, .threads = d,                  \
> +            .has_maxcpus = he, .maxcpus = e,                  \
> +        }
> +
> +#define CPU_TOPOLOGY_GENERIC(a, b, c, d, e)                   \
> +        {                                                     \
> +            .cpus     = a,                                    \
> +            .sockets  = b,                                    \
> +            .cores    = c,                                    \
> +            .threads  = d,                                    \
> +            .max_cpus = e,                                    \
> +        }
> +
> +/*
> + * Currently a 4-level topology hierarchy is supported on PC machines
> + *  -sockets/dies/cores/threads
> + */
> +#define SMP_CONFIG_WITH_DIES(ha, a, hb, b, hc, c, hd, d, he, e, hf, f) \
> +        {                                                     \
> +            .has_cpus    = ha, .cpus    = a,                  \
> +            .has_sockets = hb, .sockets = b,                  \
> +            .has_dies    = hc, .dies    = c,                  \
> +            .has_cores   = hd, .cores   = d,                  \
> +            .has_threads = he, .threads = e,                  \
> +            .has_maxcpus = hf, .maxcpus = f,                  \
> +        }
> +
> +/**
> + * @config - the given SMP configuration
> + * @expect_prefer_sockets - the expected parsing result for the
> + * valid configuration, when sockets are preferred over cores
> + * @expect_prefer_cores - the expected parsing result for the
> + * valid configuration, when cores are preferred over sockets
> + * @expect_error - the expected error report when the given
> + * configuration is invalid
> + */
> +typedef struct SMPTestData {
> +    SMPConfiguration config;
> +    CpuTopology expect_prefer_sockets;
> +    CpuTopology expect_prefer_cores;
> +    const char *expect_error;
> +} SMPTestData;
> +
> +/* Type info of the tested machine */
> +static const TypeInfo smp_machine_info = {
> +    .name = TYPE_MACHINE,
> +    .parent = TYPE_OBJECT,
> +    .class_size = sizeof(MachineClass),
> +    .instance_size = sizeof(MachineState),
> +};
> +
> +/*
> + * List all the possible valid sub-collections of the generic 5
> + * topology parameters (i.e. cpus/maxcpus/sockets/cores/threads),
> + * then test the automatic calculation algorithm of the missing
> + * values in the parser.
> + */
> +static struct SMPTestData data_generic_valid[] = {
> +    {
> +        /* config: no configuration provided
> +         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> +    }, {
> +        /* config: -smp 8
> +         * prefer_sockets: cpus=8,sockets=8,cores=1,threads=1,maxcpus=8
> +         * prefer_cores: cpus=8,sockets=1,cores=8,threads=1,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 8, 1, 1, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 8, 1, 8),
> +    }, {
> +        /* config: -smp sockets=2
> +         * expect: cpus=2,sockets=2,cores=1,threads=1,maxcpus=2 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(2, 2, 1, 1, 2),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(2, 2, 1, 1, 2),
> +    }, {
> +        /* config: -smp cores=4
> +         * expect: cpus=4,sockets=1,cores=4,threads=1,maxcpus=4 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(4, 1, 4, 1, 4),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(4, 1, 4, 1, 4),
> +    }, {
> +        /* config: -smp threads=2
> +         * expect: cpus=2,sockets=1,cores=1,threads=2,maxcpus=2 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(2, 1, 1, 2, 2),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(2, 1, 1, 2, 2),
> +    }, {
> +        /* config: -smp maxcpus=16
> +         * prefer_sockets: cpus=16,sockets=16,cores=1,threads=1,maxcpus=16
> +         * prefer_cores: cpus=16,sockets=1,cores=16,threads=1,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 16, 1, 1, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 1, 16, 1, 16),
> +    }, {
> +        /* config: -smp 8,sockets=2
> +         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +    }, {
> +        /* config: -smp 8,cores=4
> +         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +    }, {
> +        /* config: -smp 8,threads=2
> +         * prefer_sockets: cpus=8,sockets=4,cores=1,threads=2,maxcpus=8
> +         * prefer_cores: cpus=8,sockets=1,cores=4,threads=2,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 4, 1, 2, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
> +    }, {
> +        /* config: -smp 8,maxcpus=16
> +         * prefer_sockets: cpus=8,sockets=16,cores=1,threads=1,maxcpus=16
> +         * prefer_cores: cpus=8,sockets=1,cores=16,threads=1,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 16, 1, 1, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 16, 1, 16),
> +    }, {
> +        /* config: -smp sockets=2,cores=4
> +         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +    }, {
> +        /* config: -smp sockets=2,threads=2
> +         * expect: cpus=4,sockets=2,cores=1,threads=2,maxcpus=4 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(4, 2, 1, 2, 4),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(4, 2, 1, 2, 4),
> +    }, {
> +        /* config: -smp sockets=2,maxcpus=16
> +         * expect: cpus=16,sockets=2,cores=8,threads=1,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 8, 1, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 8, 1, 16),
> +    }, {
> +        /* config: -smp cores=4,threads=2
> +         * expect: cpus=8,sockets=1,cores=4,threads=2,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
> +    }, {
> +        /* config: -smp cores=4,maxcpus=16
> +         * expect: cpus=16,sockets=4,cores=4,threads=1,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 4, 4, 1, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 4, 4, 1, 16),
> +    }, {
> +        /* config: -smp threads=2,maxcpus=16
> +         * prefer_sockets: cpus=16,sockets=8,cores=1,threads=2,maxcpus=16
> +         * prefer_cores: cpus=16,sockets=1,cores=8,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 8, 1, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 1, 8, 2, 16),
> +    }, {
> +        /* config: -smp 8,sockets=2,cores=4
> +         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +    }, {
> +        /* config: -smp 8,sockets=2,threads=2
> +         * expect: cpus=8,sockets=2,cores=2,threads=2,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
> +    }, {
> +        /* config: -smp 8,sockets=2,maxcpus=16
> +         * expect: cpus=8,sockets=2,cores=8,threads=1,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 8, 1, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 8, 1, 16),
> +    }, {
> +        /* config: -smp 8,cores=4,threads=2
> +         * expect: cpus=8,sockets=1,cores=4,threads=2,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 4, 2, 8),
> +    }, {
> +        /* config: -smp 8,cores=4,maxcpus=16
> +         * expect: cpus=8,sockets=4,cores=4,threads=1,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 4, 4, 1, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 4, 4, 1, 16),
> +    }, {
> +        /* config: -smp 8,threads=2,maxcpus=16
> +         * prefer_sockets: cpus=8,sockets=8,cores=1,threads=2,maxcpus=16
> +         * prefer_cores: cpus=8,sockets=1,cores=8,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, F, 0, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 8, 1, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 1, 8, 2, 16),
> +    }, {
> +        /* config: -smp sockets=2,cores=4,threads=2
> +         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, T, 2, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp sockets=2,cores=4,maxcpus=16
> +         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp sockets=2,threads=2,maxcpus=16
> +         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, F, 0, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp cores=4,threads=2,maxcpus=16
> +         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, T, 4, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp 8,sockets=2,cores=4,threads=1
> +         * expect: cpus=8,sockets=2,cores=4,threads=1,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 1, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 1, 8),
> +    }, {
> +        /* config: -smp 8,sockets=2,cores=4,maxcpus=16
> +         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, F, 0, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp 8,sockets=2,threads=2,maxcpus=16
> +         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, F, 0, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp 8,cores=4,threads=2,maxcpus=16
> +         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, F, 0, T, 4, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp sockets=2,cores=4,threads=2,maxcpus=16
> +         * expect: cpus=16,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(F, 0, T, 2, T, 4, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(16, 2, 4, 2, 16),
> +    }, {
> +        /* config: -smp 8,sockets=2,cores=4,threads=2,maxcpus=16
> +         * expect: cpus=8,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 16),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(8, 2, 4, 2, 16),
> +    },
> +};
> +
> +static struct SMPTestData data_generic_invalid[] = {
> +    {
> +        /* config: -smp 2,dies=2 */
> +        .config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
> +        .expect_error = "dies not supported by this machine's CPU topology",
> +    }, {
> +        /* config: -smp 8,sockets=2,cores=4,threads=2,maxcpus=8 */
> +        .config = SMP_CONFIG_GENERIC(T, 8, T, 2, T, 4, T, 2, T, 8),
> +        .expect_error = "Invalid CPU topology: "
> +                        "product of the hierarchy must match maxcpus: "
> +                        "sockets (2) * cores (4) * threads (2) "
> +                        "!= maxcpus (8)",
> +    }, {
> +        /* config: -smp 18,sockets=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_GENERIC(T, 18, T, 2, T, 4, T, 2, T, 16),
> +        .expect_error = "Invalid CPU topology: "
> +                        "maxcpus must be equal to or greater than smp: "
> +                        "sockets (2) * cores (4) * threads (2) "
> +                        "== maxcpus (16) < smp_cpus (18)",
> +    }, {
> +        /* config: -smp 1
> +         * should tweak the supported min CPUs to 2 for testing */
> +        .config = SMP_CONFIG_GENERIC(T, 1, F, 0, F, 0, F, 0, F, 0),
> +        .expect_error = "Invalid SMP CPUs 1. The min CPUs supported "
> +                        "by machine '(null)' is 2",
> +    }, {
> +        /* config: -smp 512
> +         * should tweak the supported max CPUs to 511 for testing */
> +        .config = SMP_CONFIG_GENERIC(T, 512, F, 0, F, 0, F, 0, F, 0),
> +        .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
> +                        "by machine '(null)' is 511",
> +    },
> +};
> +
> +static struct SMPTestData data_with_dies_invalid[] = {
> +    {
> +        /* config: -smp 16,sockets=2,dies=2,cores=4,threads=2,maxcpus=16 */
> +        .config = SMP_CONFIG_WITH_DIES(T, 16, T, 2, T, 2, T, 4, T, 2, T, 16),
> +        .expect_error = "Invalid CPU topology: "
> +                        "product of the hierarchy must match maxcpus: "
> +                        "sockets (2) * dies (2) * cores (4) * threads (2) "
> +                        "!= maxcpus (16)",
> +    }, {
> +        /* config: -smp 34,sockets=2,dies=2,cores=4,threads=2,maxcpus=32 */
> +        .config = SMP_CONFIG_WITH_DIES(T, 34, T, 2, T, 2, T, 4, T, 2, T, 32),
> +        .expect_error = "Invalid CPU topology: "
> +                        "maxcpus must be equal to or greater than smp: "
> +                        "sockets (2) * dies (2) * cores (4) * threads (2) "
> +                        "== maxcpus (32) < smp_cpus (34)",
> +    },
> +};
> +
> +static char *smp_config_to_string(SMPConfiguration *config)
> +{
> +    return g_strdup_printf(
> +        "(SMPConfiguration) {\n"
> +        "    .has_cpus    = %5s, cpus    = %ld,\n"
> +        "    .has_sockets = %5s, sockets = %ld,\n"
> +        "    .has_dies    = %5s, dies    = %ld,\n"
> +        "    .has_cores   = %5s, cores   = %ld,\n"
> +        "    .has_threads = %5s, threads = %ld,\n"
> +        "    .has_maxcpus = %5s, maxcpus = %ld,\n"
> +        "}",
> +        config->has_cpus ? "true" : "false", config->cpus,
> +        config->has_sockets ? "true" : "false", config->sockets,
> +        config->has_dies ? "true" : "false", config->dies,
> +        config->has_cores ? "true" : "false", config->cores,
> +        config->has_threads ? "true" : "false", config->threads,
> +        config->has_maxcpus ? "true" : "false", config->maxcpus);
> +}
> +
> +static char *cpu_topology_to_string(CpuTopology *topo)
> +{
> +    return g_strdup_printf(
> +        "(CpuTopology) {\n"
> +        "    .cpus     = %u,\n"
> +        "    .sockets  = %u,\n"
> +        "    .dies     = %u,\n"
> +        "    .cores    = %u,\n"
> +        "    .threads  = %u,\n"
> +        "    .max_cpus = %u,\n"
> +        "}",
> +        topo->cpus, topo->sockets, topo->dies,
> +        topo->cores, topo->threads, topo->max_cpus);
> +}
> +
> +static void check_parse(MachineState *ms, SMPConfiguration *config,
> +                        CpuTopology *expect_topo, const char *expect_err,
> +                        bool is_valid)
> +{
> +    g_autofree char *config_str = smp_config_to_string(config);
> +    g_autofree char *expect_topo_str = cpu_topology_to_string(expect_topo);
> +    g_autofree char *output_topo_str = NULL;
> +    Error *err = NULL;
> +
> +    /* call the generic parser smp_parse() */
> +    smp_parse(ms, config, &err);
> +
> +    output_topo_str = cpu_topology_to_string(&ms->smp);
> +
> +    /* when the configuration is supposed to be valid */
> +    if (is_valid) {
> +        if ((err == NULL) &&
> +            (ms->smp.cpus == expect_topo->cpus) &&
> +            (ms->smp.sockets == expect_topo->sockets) &&
> +            (ms->smp.dies == expect_topo->dies) &&
> +            (ms->smp.cores == expect_topo->cores) &&
> +            (ms->smp.threads == expect_topo->threads) &&
> +            (ms->smp.max_cpus == expect_topo->max_cpus)) {
> +            return;
> +        }
> +
> +        if (err != NULL) {
> +            g_printerr("Test smp_parse failed!\n"
> +                       "Input configuration: %s\n"
> +                       "Should be valid: yes\n"
> +                       "Expected topology: %s\n\n"
> +                       "Result is valid: no\n"
> +                       "Output error report: %s\n",
> +                       config_str, expect_topo_str, error_get_pretty(err));
> +            goto end;
> +        }
> +
> +        g_printerr("Test smp_parse failed!\n"
> +                   "Input configuration: %s\n"
> +                   "Should be valid: yes\n"
> +                   "Expected topology: %s\n\n"
> +                   "Result is valid: yes\n"
> +                   "Output topology: %s\n",
> +                   config_str, expect_topo_str, output_topo_str);
> +        goto end;
> +    }
> +
> +    /* when the configuration is supposed to be invalid */
> +    if (err != NULL) {
> +        if (expect_err == NULL ||
> +            g_str_equal(expect_err, error_get_pretty(err))) {
> +            error_free(err);
> +            return;
> +        }
> +
> +        g_printerr("Test smp_parse failed!\n"
> +                   "Input configuration: %s\n"
> +                   "Should be valid: no\n"
> +                   "Expected error report: %s\n\n"
> +                   "Result is valid: no\n"
> +                   "Output error report: %s\n",
> +                   config_str, expect_err, error_get_pretty(err));
> +        goto end;
> +    }
> +
> +    g_printerr("Test smp_parse failed!\n"
> +               "Input configuration: %s\n"
> +               "Should be valid: no\n"
> +               "Expected error report: %s\n\n"
> +               "Result is valid: yes\n"
> +               "Output topology: %s\n",
> +               config_str, expect_err, output_topo_str);
> +
> +end:
> +    if (err != NULL) {
> +        error_free(err);
> +    }
> +
> +    abort();
> +}
> +
> +static void smp_parse_test(MachineState *ms, SMPTestData *data, bool is_valid)
> +{
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> +    mc->smp_props.prefer_sockets = true;
> +    check_parse(ms, &data->config, &data->expect_prefer_sockets,
> +                data->expect_error, is_valid);
> +
> +    mc->smp_props.prefer_sockets = false;
> +    check_parse(ms, &data->config, &data->expect_prefer_cores,
> +                data->expect_error, is_valid);
> +}
> +
> +/* The parsed results of the unsupported parameters should be 1 */
> +static void unsupported_params_init(MachineClass *mc, SMPTestData *data)
> +{
> +    if (!mc->smp_props.dies_supported) {
> +        data->expect_prefer_sockets.dies = 1;
> +        data->expect_prefer_cores.dies = 1;
> +    }
> +}
> +
> +/* Reset the related machine properties before each sub-test */
> +static void smp_machine_class_init(MachineClass *mc)
> +{
> +    mc->min_cpus = MIN_CPUS;
> +    mc->max_cpus = MAX_CPUS;
> +
> +    mc->smp_props.prefer_sockets = true;
> +    mc->smp_props.dies_supported = false;
> +}
> +
> +static void test_generic(void)
> +{
> +    Object *obj = object_new(TYPE_MACHINE);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +    SMPTestData *data = &(SMPTestData){0};
> +    int i;
> +
> +    smp_machine_class_init(mc);
> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +        *data = data_generic_valid[i];
> +        unsupported_params_init(mc, data);
> +
> +        smp_parse_test(ms, data, true);
> +
> +        /* Unsupported parameters can be provided with their values as 1 */
> +        data->config.has_dies = true;
> +        data->config.dies = 1;
> +        smp_parse_test(ms, data, true);
> +    }
> +
> +    /* Reset the supported min CPUs and max CPUs */
> +    mc->min_cpus = 2;
> +    mc->max_cpus = 511;
> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
> +        *data = data_generic_invalid[i];
> +        unsupported_params_init(mc, data);
> +
> +        smp_parse_test(ms, data, false);
> +    }
> +
> +    object_unref(obj);
> +}
> +
> +static void test_with_dies(void)
> +{
> +    Object *obj = object_new(TYPE_MACHINE);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +    SMPTestData *data = &(SMPTestData){0};
> +    unsigned int num_dies = 2;
> +    int i;
> +
> +    smp_machine_class_init(mc);
> +    mc->smp_props.dies_supported = true;
> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +        *data = data_generic_valid[i];
> +        unsupported_params_init(mc, data);
> +
> +        /* when dies parameter is omitted, it will be set as 1 */
> +        data->expect_prefer_sockets.dies = 1;
> +        data->expect_prefer_cores.dies = 1;
> +
> +        smp_parse_test(ms, data, true);
> +
> +        /* when dies parameter is specified */
> +        data->config.has_dies = true;
> +        data->config.dies = num_dies;
> +        if (data->config.has_cpus) {
> +            data->config.cpus *= num_dies;
> +        }
> +        if (data->config.has_maxcpus) {
> +            data->config.maxcpus *= num_dies;
> +        }
> +
> +        data->expect_prefer_sockets.dies = num_dies;
> +        data->expect_prefer_sockets.cpus *= num_dies;
> +        data->expect_prefer_sockets.max_cpus *= num_dies;
> +        data->expect_prefer_cores.dies = num_dies;
> +        data->expect_prefer_cores.cpus *= num_dies;
> +        data->expect_prefer_cores.max_cpus *= num_dies;
> +
> +        smp_parse_test(ms, data, true);
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
> +        *data = data_with_dies_invalid[i];
> +        unsupported_params_init(mc, data);
> +
> +        smp_parse_test(ms, data, false);
> +    }
> +
> +    object_unref(obj);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    module_call_init(MODULE_INIT_QOM);
> +    type_register_static(&smp_machine_info);
> +
> +    g_test_add_func("/test-smp-parse/generic", test_generic);
> +    g_test_add_func("/test-smp-parse/with_dies", test_with_dies);
> +
> +    g_test_run();
> +
> +    return 0;
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 80ec27d76ae..310a9512ea1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1633,6 +1633,7 @@ F: include/hw/boards.h
>   F: include/hw/core/cpu.h
>   F: include/hw/cpu/cluster.h
>   F: include/sysemu/numa.h
> +F: tests/unit/test-smp-parse.c
>   T: git https://gitlab.com/ehabkost/qemu.git machine-next
>   
>   Xtensa Machines
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 7c297d7e5c0..5e81525bef1 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -45,6 +45,7 @@
>     'test-uuid': [],
>     'ptimer-test': ['ptimer-test-stubs.c', meson.project_source_root() / 'hw/core/ptimer.c'],
>     'test-qapi-util': [],
> +  'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c'],
>   }
>   
>   if have_system or have_tools

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[Philippe Mathieu-Daudé]

On 10/31/21 13:02, wangyanan (Y) wrote:
> Hi Philippe,
> 
> I saw that there are some cross-build failures and a clang complain
> about this patch in your triggered CI pipeline. I believe the minor
> diff below will resolve them. If you are going to resend v2 of the
> "qdev-hotplug" patches, I would very much appreciate it if you
> can also help to respin v5 of this series with the diff applied.
> Sorry for the extra work included. :)

Sure, thanks for running the CI pipeline, you saved me time (I'll still
run it again, but with your diff applied).

> 
> Thanks,
> Yanan
> 
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index e96abe9ba4..61985094f5 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -347,12 +347,12 @@ static char *smp_config_to_string(SMPConfiguration
> *config)
>  {
>      return g_strdup_printf(
>          "(SMPConfiguration) {\n"
> -        "    .has_cpus    = %5s, cpus    = %ld,\n"
> -        "    .has_sockets = %5s, sockets = %ld,\n"
> -        "    .has_dies    = %5s, dies    = %ld,\n"
> -        "    .has_cores   = %5s, cores   = %ld,\n"
> -        "    .has_threads = %5s, threads = %ld,\n"
> -        "    .has_maxcpus = %5s, maxcpus = %ld,\n"
> +        "    .has_cpus    = %5s, cpus    = %" PRId64 ",\n"
> +        "    .has_sockets = %5s, sockets = %" PRId64 ",\n"
> +        "    .has_dies    = %5s, dies    = %" PRId64 ",\n"
> +        "    .has_cores   = %5s, cores   = %" PRId64 ",\n"
> +        "    .has_threads = %5s, threads = %" PRId64 ",\n"
> +        "    .has_maxcpus = %5s, maxcpus = %" PRId64 ",\n"
>          "}",
>          config->has_cpus ? "true" : "false", config->cpus,
>          config->has_sockets ? "true" : "false", config->sockets,
> @@ -495,7 +495,7 @@ static void test_generic(void)
>      Object *obj = object_new(TYPE_MACHINE);
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
> -    SMPTestData *data = &(SMPTestData){0};
> +    SMPTestData *data = &(SMPTestData){{0}};
>      int i;
> 
>      smp_machine_class_init(mc);
> @@ -531,7 +531,7 @@ static void test_with_dies(void)
>      Object *obj = object_new(TYPE_MACHINE);
>      MachineState *ms = MACHINE(obj);
>      MachineClass *mc = MACHINE_GET_CLASS(obj);
> -    SMPTestData *data = &(SMPTestData){0};
> +    SMPTestData *data = &(SMPTestData){{0}};
>      unsigned int num_dies = 2;
>      int i;
> 
> On 2021/10/28 23:09, Philippe Mathieu-Daudé wrote:
>> From: Yanan Wang <wangyanan55@huawei.com>
>>
>> Now that we have a generic parser smp_parse(), let's add an unit
>> test for the code. All possible valid/invalid SMP configurations
>> that the user can specify are covered.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>   MAINTAINERS                 |   1 +
>>   tests/unit/meson.build      |   1 +
>>   3 files changed, 596 insertions(+)
>>   create mode 100644 tests/unit/test-smp-parse.c

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[Philippe Mathieu-Daudé]

Hi,

On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
> From: Yanan Wang <wangyanan55@huawei.com>
> 
> Now that we have a generic parser smp_parse(), let's add an unit
> test for the code. All possible valid/invalid SMP configurations
> that the user can specify are covered.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                 |   1 +
>  tests/unit/meson.build      |   1 +
>  3 files changed, 596 insertions(+)
>  create mode 100644 tests/unit/test-smp-parse.c

Could you have a look at this test failure please?
https://cirrus-ci.com/task/5823855357853696

MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
G_TEST_SRCDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/tests/unit
G_TEST_BUILDDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/build/tests/unit
tests/unit/test-smp-parse.exe --tap -k
Test smp_parse failed!
Input configuration: (SMPConfiguration) {
    .has_cpus    =  true, cpus    = 1,
    .has_sockets = false, sockets = 0,
    .has_dies    = false, dies    = 0,
    .has_cores   = false, cores   = 0,
    .has_threads = false, threads = 0,
    .has_maxcpus = false, maxcpus = 0,
}
Should be valid: no
Expected error report: Invalid SMP CPUs 1. The min CPUs supported by
machine '(null)' is 2
Result is valid: no
Output error report: Invalid SMP CPUs 1. The min CPUs supported by
machine '(NULL)' is 2
ERROR test-smp-parse - too few tests run (expected 2, got 0)
make: *** [Makefile.mtest:576: run-test-70] Error 1

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[wangyanan (Y)]

On 2021/11/9 17:36, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>> From: Yanan Wang <wangyanan55@huawei.com>
>>
>> Now that we have a generic parser smp_parse(), let's add an unit
>> test for the code. All possible valid/invalid SMP configurations
>> that the user can specify are covered.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>   MAINTAINERS                 |   1 +
>>   tests/unit/meson.build      |   1 +
>>   3 files changed, 596 insertions(+)
>>   create mode 100644 tests/unit/test-smp-parse.c
> Could you have a look at this test failure please?
> https://cirrus-ci.com/task/5823855357853696
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> G_TEST_SRCDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/tests/unit
> G_TEST_BUILDDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/build/tests/unit
> tests/unit/test-smp-parse.exe --tap -k
> Test smp_parse failed!
> Input configuration: (SMPConfiguration) {
>      .has_cpus    =  true, cpus    = 1,
>      .has_sockets = false, sockets = 0,
>      .has_dies    = false, dies    = 0,
>      .has_cores   = false, cores   = 0,
>      .has_threads = false, threads = 0,
>      .has_maxcpus = false, maxcpus = 0,
> }
> Should be valid: no
> Expected error report: Invalid SMP CPUs 1. The min CPUs supported by
> machine '(null)' is 2
> Result is valid: no
> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
> machine '(NULL)' is 2
> ERROR test-smp-parse - too few tests run (expected 2, got 0)
> make: *** [Makefile.mtest:576: run-test-70] Error 1
>
>
> .

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[wangyanan (Y)]

Hi,

On 2021/11/9 17:36, Philippe Mathieu-Daudé wrote:
> Hi,
>
> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>> From: Yanan Wang <wangyanan55@huawei.com>
>>
>> Now that we have a generic parser smp_parse(), let's add an unit
>> test for the code. All possible valid/invalid SMP configurations
>> that the user can specify are covered.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>   MAINTAINERS                 |   1 +
>>   tests/unit/meson.build      |   1 +
>>   3 files changed, 596 insertions(+)
>>   create mode 100644 tests/unit/test-smp-parse.c
> Could you have a look at this test failure please?
> https://cirrus-ci.com/task/5823855357853696
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> G_TEST_SRCDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/tests/unit
> G_TEST_BUILDDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/build/tests/unit
> tests/unit/test-smp-parse.exe --tap -k
> Test smp_parse failed!
> Input configuration: (SMPConfiguration) {
>      .has_cpus    =  true, cpus    = 1,
>      .has_sockets = false, sockets = 0,
>      .has_dies    = false, dies    = 0,
>      .has_cores   = false, cores   = 0,
>      .has_threads = false, threads = 0,
>      .has_maxcpus = false, maxcpus = 0,
> }
> Should be valid: no
> Expected error report: Invalid SMP CPUs 1. The min CPUs supported by
> machine '(null)' is 2
> Result is valid: no
> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
> machine '(NULL)' is 2
> ERROR test-smp-parse - too few tests run (expected 2, got 0)
> make: *** [Makefile.mtest:576: run-test-70] Error 1
>
>
> .
Obviously, the name string for the tested machine type in cirrus-ci is 
"NULL",
while the expected name string is "null". It was also "null" when running on
my Arm64 platform locally.

Anyway, I shouldn't have hardcoded this expected error message. I can
send a fix patch to make it more flexible then more stable. :)

Thanks,
Yanan

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[Thomas Huth]

On 09/11/2021 13.18, wangyanan (Y) wrote:
> Hi,
> 
> On 2021/11/9 17:36, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>>> From: Yanan Wang <wangyanan55@huawei.com>
>>>
>>> Now that we have a generic parser smp_parse(), let's add an unit
>>> test for the code. All possible valid/invalid SMP configurations
>>> that the user can specify are covered.
>>>
>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>>   MAINTAINERS                 |   1 +
>>>   tests/unit/meson.build      |   1 +
>>>   3 files changed, 596 insertions(+)
>>>   create mode 100644 tests/unit/test-smp-parse.c
>> Could you have a look at this test failure please?
>> https://cirrus-ci.com/task/5823855357853696
>>
>> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
>> G_TEST_SRCDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/tests/unit 
>>
>> G_TEST_BUILDDIR=C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/build/tests/unit 
>>
>> tests/unit/test-smp-parse.exe --tap -k
>> Test smp_parse failed!
>> Input configuration: (SMPConfiguration) {
>>      .has_cpus    =  true, cpus    = 1,
>>      .has_sockets = false, sockets = 0,
>>      .has_dies    = false, dies    = 0,
>>      .has_cores   = false, cores   = 0,
>>      .has_threads = false, threads = 0,
>>      .has_maxcpus = false, maxcpus = 0,
>> }
>> Should be valid: no
>> Expected error report: Invalid SMP CPUs 1. The min CPUs supported by
>> machine '(null)' is 2
>> Result is valid: no
>> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
>> machine '(NULL)' is 2
>> ERROR test-smp-parse - too few tests run (expected 2, got 0)
>> make: *** [Makefile.mtest:576: run-test-70] Error 1
>>
>>
>> .
> Obviously, the name string for the tested machine type in cirrus-ci is "NULL",
> while the expected name string is "null". It was also "null" when running on
> my Arm64 platform locally.
> 
> Anyway, I shouldn't have hardcoded this expected error message. I can
> send a fix patch to make it more flexible then more stable. :)

Yes, please do so. The "(null)"/"(NULL)" obviously depends on the libc that 
is used, so we should not rely on the correct spelling of the string in the 
tests here.

  Thomas

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[Philippe Mathieu-Daudé]

On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
> From: Yanan Wang <wangyanan55@huawei.com>
> 
> Now that we have a generic parser smp_parse(), let's add an unit
> test for the code. All possible valid/invalid SMP configurations
> that the user can specify are covered.
> 
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                 |   1 +
>  tests/unit/meson.build      |   1 +
>  3 files changed, 596 insertions(+)
>  create mode 100644 tests/unit/test-smp-parse.c

> +static struct SMPTestData data_generic_valid[] = {
> +    {
> +        /* config: no configuration provided
> +         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */

[1]

> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
> +    }, {

> +static void test_generic(void)
> +{
> +    Object *obj = object_new(TYPE_MACHINE);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);

Watch out, while you create a machine instance in each
test, there is only one machine class registered (see
type_register_static(&smp_machine_info) below in [2]),
...

> +    SMPTestData *data = &(SMPTestData){0};
> +    int i;
> +
> +    smp_machine_class_init(mc);
> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +        *data = data_generic_valid[i];
> +        unsupported_params_init(mc, data);
> +
> +        smp_parse_test(ms, data, true);
> +
> +        /* Unsupported parameters can be provided with their values as 1 */
> +        data->config.has_dies = true;
> +        data->config.dies = 1;
> +        smp_parse_test(ms, data, true);
> +    }
> +
> +    /* Reset the supported min CPUs and max CPUs */
> +    mc->min_cpus = 2;
> +    mc->max_cpus = 511;

... and here you are modifying the single machine class state, ...

> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
> +        *data = data_generic_invalid[i];
> +        unsupported_params_init(mc, data);
> +
> +        smp_parse_test(ms, data, false);
> +    }
> +
> +    object_unref(obj);
> +}
> +
> +static void test_with_dies(void)
> +{
> +    Object *obj = object_new(TYPE_MACHINE);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);

... so here the machine class state is inconsistent, ...

> +    SMPTestData *data = &(SMPTestData){0};
> +    unsigned int num_dies = 2;
> +    int i;
> +
> +    smp_machine_class_init(mc);
> +    mc->smp_props.dies_supported = true;
> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +        *data = data_generic_valid[i];
> +        unsupported_params_init(mc, data);
> +
> +        /* when dies parameter is omitted, it will be set as 1 */
> +        data->expect_prefer_sockets.dies = 1;
> +        data->expect_prefer_cores.dies = 1;
> +
> +        smp_parse_test(ms, data, true);

... in particular the first test [1] is tested with mc->min_cpus = 2.

I wonder why you are not getting:

Output error report: Invalid SMP CPUs 1. The min CPUs supported by
machine '(null)' is 2

for [1].

> +
> +        /* when dies parameter is specified */
> +        data->config.has_dies = true;
> +        data->config.dies = num_dies;
> +        if (data->config.has_cpus) {
> +            data->config.cpus *= num_dies;
> +        }
> +        if (data->config.has_maxcpus) {
> +            data->config.maxcpus *= num_dies;
> +        }
> +
> +        data->expect_prefer_sockets.dies = num_dies;
> +        data->expect_prefer_sockets.cpus *= num_dies;
> +        data->expect_prefer_sockets.max_cpus *= num_dies;
> +        data->expect_prefer_cores.dies = num_dies;
> +        data->expect_prefer_cores.cpus *= num_dies;
> +        data->expect_prefer_cores.max_cpus *= num_dies;
> +
> +        smp_parse_test(ms, data, true);
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
> +        *data = data_with_dies_invalid[i];
> +        unsupported_params_init(mc, data);
> +
> +        smp_parse_test(ms, data, false);
> +    }
> +
> +    object_unref(obj);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    module_call_init(MODULE_INIT_QOM);
> +    type_register_static(&smp_machine_info);

[2]

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[wangyanan (Y)]

On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote:
> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>> From: Yanan Wang <wangyanan55@huawei.com>
>>
>> Now that we have a generic parser smp_parse(), let's add an unit
>> test for the code. All possible valid/invalid SMP configurations
>> that the user can specify are covered.
>>
>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>   MAINTAINERS                 |   1 +
>>   tests/unit/meson.build      |   1 +
>>   3 files changed, 596 insertions(+)
>>   create mode 100644 tests/unit/test-smp-parse.c
>> +static struct SMPTestData data_generic_valid[] = {
>> +    {
>> +        /* config: no configuration provided
>> +         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
> [1]
>
>> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
>> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>> +    }, {
>> +static void test_generic(void)
>> +{
>> +    Object *obj = object_new(TYPE_MACHINE);
>> +    MachineState *ms = MACHINE(obj);
>> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> Watch out, while you create a machine instance in each
> test, there is only one machine class registered (see
> type_register_static(&smp_machine_info) below in [2]),
> ...
Yes, I noticed this. So on the top of each sub-test function, the properties
of the single machine class is re-initialized by smp_machine_class_init(mc).
See [*] below.
>> +    SMPTestData *data = &(SMPTestData){0};
>> +    int i;
>> +
>> +    smp_machine_class_init(mc);
[*]
>> +
>> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>> +        *data = data_generic_valid[i];
>> +        unsupported_params_init(mc, data);
>> +
>> +        smp_parse_test(ms, data, true);
>> +
>> +        /* Unsupported parameters can be provided with their values as 1 */
>> +        data->config.has_dies = true;
>> +        data->config.dies = 1;
>> +        smp_parse_test(ms, data, true);
>> +    }
>> +
>> +    /* Reset the supported min CPUs and max CPUs */
>> +    mc->min_cpus = 2;
>> +    mc->max_cpus = 511;
> ... and here you are modifying the single machine class state, ...
>
>> +
>> +    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
>> +        *data = data_generic_invalid[i];
>> +        unsupported_params_init(mc, data);
>> +
>> +        smp_parse_test(ms, data, false);
>> +    }
>> +
>> +    object_unref(obj);
>> +}
>> +
>> +static void test_with_dies(void)
>> +{
>> +    Object *obj = object_new(TYPE_MACHINE);
>> +    MachineState *ms = MACHINE(obj);
>> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> ... so here the machine class state is inconsistent, ...
>
>> +    SMPTestData *data = &(SMPTestData){0};
>> +    unsigned int num_dies = 2;
>> +    int i;
>> +
>> +    smp_machine_class_init(mc);
And here [*].
>> +    mc->smp_props.dies_supported = true;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>> +        *data = data_generic_valid[i];
>> +        unsupported_params_init(mc, data);
>> +
>> +        /* when dies parameter is omitted, it will be set as 1 */
>> +        data->expect_prefer_sockets.dies = 1;
>> +        data->expect_prefer_cores.dies = 1;
>> +
>> +        smp_parse_test(ms, data, true);
> ... in particular the first test [1] is tested with mc->min_cpus = 2.
>
> I wonder why you are not getting:
>
> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
> machine '(null)' is 2
>
> for [1].
So as I have explained above, we won't get an output error report like 
this here. :)

Thanks,
Yanan
>> +
>> +        /* when dies parameter is specified */
>> +        data->config.has_dies = true;
>> +        data->config.dies = num_dies;
>> +        if (data->config.has_cpus) {
>> +            data->config.cpus *= num_dies;
>> +        }
>> +        if (data->config.has_maxcpus) {
>> +            data->config.maxcpus *= num_dies;
>> +        }
>> +
>> +        data->expect_prefer_sockets.dies = num_dies;
>> +        data->expect_prefer_sockets.cpus *= num_dies;
>> +        data->expect_prefer_sockets.max_cpus *= num_dies;
>> +        data->expect_prefer_cores.dies = num_dies;
>> +        data->expect_prefer_cores.cpus *= num_dies;
>> +        data->expect_prefer_cores.max_cpus *= num_dies;
>> +
>> +        smp_parse_test(ms, data, true);
>> +    }
>> +
>> +    for (i = 0; i < ARRAY_SIZE(data_with_dies_invalid); i++) {
>> +        *data = data_with_dies_invalid[i];
>> +        unsupported_params_init(mc, data);
>> +
>> +        smp_parse_test(ms, data, false);
>> +    }
>> +
>> +    object_unref(obj);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    module_call_init(MODULE_INIT_QOM);
>> +    type_register_static(&smp_machine_info);
> [2]
>
> .

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[Philippe Mathieu-Daudé]

On 11/11/21 10:31, wangyanan (Y) wrote:
> 
> On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote:
>> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>>> From: Yanan Wang <wangyanan55@huawei.com>
>>>
>>> Now that we have a generic parser smp_parse(), let's add an unit
>>> test for the code. All possible valid/invalid SMP configurations
>>> that the user can specify are covered.
>>>
>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>   tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>>   MAINTAINERS                 |   1 +
>>>   tests/unit/meson.build      |   1 +
>>>   3 files changed, 596 insertions(+)
>>>   create mode 100644 tests/unit/test-smp-parse.c
>>> +static struct SMPTestData data_generic_valid[] = {
>>> +    {
>>> +        /* config: no configuration provided
>>> +         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
>> [1]
>>
>>> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
>>> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>>> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>>> +    }, {
>>> +static void test_generic(void)
>>> +{
>>> +    Object *obj = object_new(TYPE_MACHINE);
>>> +    MachineState *ms = MACHINE(obj);
>>> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
>> Watch out, while you create a machine instance in each
>> test, there is only one machine class registered (see
>> type_register_static(&smp_machine_info) below in [2]),
>> ...
> Yes, I noticed this. So on the top of each sub-test function, the
> properties
> of the single machine class is re-initialized by
> smp_machine_class_init(mc).
> See [*] below.
>>> +    SMPTestData *data = &(SMPTestData){0};
>>> +    int i;
>>> +
>>> +    smp_machine_class_init(mc);
> [*]
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>>> +        *data = data_generic_valid[i];
>>> +        unsupported_params_init(mc, data);
>>> +
>>> +        smp_parse_test(ms, data, true);
>>> +
>>> +        /* Unsupported parameters can be provided with their values
>>> as 1 */
>>> +        data->config.has_dies = true;
>>> +        data->config.dies = 1;
>>> +        smp_parse_test(ms, data, true);
>>> +    }
>>> +
>>> +    /* Reset the supported min CPUs and max CPUs */
>>> +    mc->min_cpus = 2;
>>> +    mc->max_cpus = 511;
>> ... and here you are modifying the single machine class state, ...
>>
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
>>> +        *data = data_generic_invalid[i];
>>> +        unsupported_params_init(mc, data);
>>> +
>>> +        smp_parse_test(ms, data, false);
>>> +    }
>>> +
>>> +    object_unref(obj);
>>> +}
>>> +
>>> +static void test_with_dies(void)
>>> +{
>>> +    Object *obj = object_new(TYPE_MACHINE);
>>> +    MachineState *ms = MACHINE(obj);
>>> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
>> ... so here the machine class state is inconsistent, ...
>>
>>> +    SMPTestData *data = &(SMPTestData){0};
>>> +    unsigned int num_dies = 2;
>>> +    int i;
>>> +
>>> +    smp_machine_class_init(mc);
> And here [*].
>>> +    mc->smp_props.dies_supported = true;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>>> +        *data = data_generic_valid[i];
>>> +        unsupported_params_init(mc, data);
>>> +
>>> +        /* when dies parameter is omitted, it will be set as 1 */
>>> +        data->expect_prefer_sockets.dies = 1;
>>> +        data->expect_prefer_cores.dies = 1;
>>> +
>>> +        smp_parse_test(ms, data, true);
>> ... in particular the first test [1] is tested with mc->min_cpus = 2.
>>
>> I wonder why you are not getting:
>>
>> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
>> machine '(null)' is 2
>>
>> for [1].
> So as I have explained above, we won't get an output error report like
> this here. :)

I see. IMHO this is bad practice example, so I'll send a cleanup patch.

Re: [PATCH v4 2/2] tests/unit: Add an unit test for smp parsing

[wangyanan (Y)]

On 2021/11/11 17:37, Philippe Mathieu-Daudé wrote:
> On 11/11/21 10:31, wangyanan (Y) wrote:
>> On 2021/11/11 17:14, Philippe Mathieu-Daudé wrote:
>>> On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
>>>> From: Yanan Wang <wangyanan55@huawei.com>
>>>>
>>>> Now that we have a generic parser smp_parse(), let's add an unit
>>>> test for the code. All possible valid/invalid SMP configurations
>>>> that the user can specify are covered.
>>>>
>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
>>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Message-Id: <20211026034659.22040-3-wangyanan55@huawei.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>    tests/unit/test-smp-parse.c | 594 ++++++++++++++++++++++++++++++++++++
>>>>    MAINTAINERS                 |   1 +
>>>>    tests/unit/meson.build      |   1 +
>>>>    3 files changed, 596 insertions(+)
>>>>    create mode 100644 tests/unit/test-smp-parse.c
>>>> +static struct SMPTestData data_generic_valid[] = {
>>>> +    {
>>>> +        /* config: no configuration provided
>>>> +         * expect: cpus=1,sockets=1,cores=1,threads=1,maxcpus=1 */
>>> [1]
>>>
>>>> +        .config = SMP_CONFIG_GENERIC(F, 0, F, 0, F, 0, F, 0, F, 0),
>>>> +        .expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>>>> +        .expect_prefer_cores   = CPU_TOPOLOGY_GENERIC(1, 1, 1, 1, 1),
>>>> +    }, {
>>>> +static void test_generic(void)
>>>> +{
>>>> +    Object *obj = object_new(TYPE_MACHINE);
>>>> +    MachineState *ms = MACHINE(obj);
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
>>> Watch out, while you create a machine instance in each
>>> test, there is only one machine class registered (see
>>> type_register_static(&smp_machine_info) below in [2]),
>>> ...
>> Yes, I noticed this. So on the top of each sub-test function, the
>> properties
>> of the single machine class is re-initialized by
>> smp_machine_class_init(mc).
>> See [*] below.
>>>> +    SMPTestData *data = &(SMPTestData){0};
>>>> +    int i;
>>>> +
>>>> +    smp_machine_class_init(mc);
>> [*]
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>>>> +        *data = data_generic_valid[i];
>>>> +        unsupported_params_init(mc, data);
>>>> +
>>>> +        smp_parse_test(ms, data, true);
>>>> +
>>>> +        /* Unsupported parameters can be provided with their values
>>>> as 1 */
>>>> +        data->config.has_dies = true;
>>>> +        data->config.dies = 1;
>>>> +        smp_parse_test(ms, data, true);
>>>> +    }
>>>> +
>>>> +    /* Reset the supported min CPUs and max CPUs */
>>>> +    mc->min_cpus = 2;
>>>> +    mc->max_cpus = 511;
>>> ... and here you are modifying the single machine class state, ...
>>>
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(data_generic_invalid); i++) {
>>>> +        *data = data_generic_invalid[i];
>>>> +        unsupported_params_init(mc, data);
>>>> +
>>>> +        smp_parse_test(ms, data, false);
>>>> +    }
>>>> +
>>>> +    object_unref(obj);
>>>> +}
>>>> +
>>>> +static void test_with_dies(void)
>>>> +{
>>>> +    Object *obj = object_new(TYPE_MACHINE);
>>>> +    MachineState *ms = MACHINE(obj);
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
>>> ... so here the machine class state is inconsistent, ...
>>>
>>>> +    SMPTestData *data = &(SMPTestData){0};
>>>> +    unsigned int num_dies = 2;
>>>> +    int i;
>>>> +
>>>> +    smp_machine_class_init(mc);
>> And here [*].
>>>> +    mc->smp_props.dies_supported = true;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
>>>> +        *data = data_generic_valid[i];
>>>> +        unsupported_params_init(mc, data);
>>>> +
>>>> +        /* when dies parameter is omitted, it will be set as 1 */
>>>> +        data->expect_prefer_sockets.dies = 1;
>>>> +        data->expect_prefer_cores.dies = 1;
>>>> +
>>>> +        smp_parse_test(ms, data, true);
>>> ... in particular the first test [1] is tested with mc->min_cpus = 2.
>>>
>>> I wonder why you are not getting:
>>>
>>> Output error report: Invalid SMP CPUs 1. The min CPUs supported by
>>> machine '(null)' is 2
>>>
>>> for [1].
>> So as I have explained above, we won't get an output error report like
>> this here. :)
> I see. IMHO this is bad practice example, so I'll send a cleanup patch.
>
> .
Sure! I'm very happy that we can have a better solution. Thank you for 
doing that.

Thanks,
Yanan

Re: [PATCH v4 0/2] hw/core/machine: Add an unit test for smp_parse

[Eduardo Habkost]

On Thu, Oct 28, 2021 at 05:09:11PM +0200, Philippe Mathieu-Daudé wrote:
> Respin of Yanan Wang v3, based on
> "hw/core: Restrict qdev-hotplug to sysemu"
> 
> Based-on: 20211028150521.1973821-1-philmd@redhat.com
> https://lore.kernel.org/qemu-devel/20211028150521.1973821-1-philmd@redhat.com
> 
> git-backport-diff:
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/2:[0001] [FC] 'hw/core/machine: Split out the smp parsing code'
> 002/2:[----] [--] 'tests/unit: Add an unit test for smp parsing'

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [PATCH v4 0/2] hw/core/machine: Add an unit test for smp_parse

[Philippe Mathieu-Daudé]

On 10/28/21 17:09, Philippe Mathieu-Daudé wrote:
> Respin of Yanan Wang v3, based on
> "hw/core: Restrict qdev-hotplug to sysemu"
> 
> Based-on: 20211028150521.1973821-1-philmd@redhat.com
> https://lore.kernel.org/qemu-devel/20211028150521.1973821-1-philmd@redhat.com
> 
> git-backport-diff:
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/2:[0001] [FC] 'hw/core/machine: Split out the smp parsing code'
> 002/2:[----] [--] 'tests/unit: Add an unit test for smp parsing'
> 
> Yanan Wang (2):
>   hw/core/machine: Split out the smp parsing code
>   tests/unit: Add an unit test for smp parsing

Series queued to machine-next (including patch #2 fixup).