[PATCH 00/14] Cleanup on SMP and its test

[PATCH 00/14] Cleanup on SMP and its test

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Hi all,

To make review easier, I've merged my previous single SMP patch [1] and
SMP test series [2] into this series as well.

So this series includes:
 * [Patch 1] Remove deprecated "parameter=0" SMP configurations, which
   is marked as deprecated in v6.2.
 * [Patch 2] Deprecate unsupported "parameter=1" SMP configurations.
 * [Patch 3 & 4] Minor code cleanup for machine_parse_smp_config().
 * [Patch 5 ~ 14] Test case enhancements to cover more SMP API changes.

[1]: https://lore.kernel.org/qemu-devel/20240304044510.2305849-1-zhao1.liu@linux.intel.com/
[2]: https://lore.kernel.org/qemu-devel/20240118144857.2124034-1-zhao1.liu@linux.intel.com/

Thanks and Best Regards,
Zhao

---
Zhao Liu (14):
  hw/core/machine-smp: Remove deprecated "parameter=0" SMP
    configurations
  hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP
    configurations
  hw/core/machine-smp: Simplify variables' initialization in
    machine_parse_smp_config()
  hw/core/machine-smp: Calculate total CPUs once in
    machine_parse_smp_config()
  tests/unit/test-smp-parse: Drop the unsupported "dies=1" case
  tests/unit/test-smp-parse: Use CPU number macros in invalid topology
    case
  tests/unit/test-smp-parse: Bump max_cpus to 4096
  tests/unit/test-smp-parse: Make test cases aware of the book/drawer
  tests/unit/test-smp-parse: Test "books" parameter in -smp
  tests/unit/test-smp-parse: Test "drawers" parameter in -smp
  tests/unit/test-smp-parse: Test "drawers" and "books" combination case
  tests/unit/test-smp-parse: Test the full 7-levels topology hierarchy
  tests/unit/test-smp-parse: Test smp_props.has_clusters
  tests/unit/test-smp-parse: Test "parameter=0" SMP configurations

 docs/about/deprecated.rst       |  30 +-
 docs/about/removed-features.rst |  15 +
 hw/core/machine-smp.c           |  94 +++--
 tests/unit/test-smp-parse.c     | 612 ++++++++++++++++++++++++++++++--
 4 files changed, 678 insertions(+), 73 deletions(-)

-- 
2.34.1

[PATCH 01/14] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The "parameter=0" SMP configurations have been marked as deprecated
since v6.2.

For these cases, -smp currently returns the warning and adjusts the
zeroed parameters to 1 by default.

Remove the above compatibility logic in v9.0, and return error directly
if any -smp parameter is set as 0.

Cc: devel@lists.libvirt.org
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
---
 docs/about/deprecated.rst       | 16 ----------------
 docs/about/removed-features.rst | 15 +++++++++++++++
 hw/core/machine-smp.c           |  5 +++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 36bd3e15ef06..872974640252 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -36,22 +36,6 @@ and will cause a warning.
 The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on``
 rather than ``delay=off``.
 
-``-smp`` ("parameter=0" SMP configurations) (since 6.2)
-'''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-Specified CPU topology parameters must be greater than zero.
-
-In the SMP configuration, users should either provide a CPU topology
-parameter with a reasonable value (greater than zero) or just omit it
-and QEMU will compute the missing value.
-
-However, historically it was implicitly allowed for users to provide
-a parameter with zero value, which is meaningless and could also possibly
-cause unexpected results in the -smp parsing. So support for this kind of
-configurations (e.g. -smp 8,sockets=0) is deprecated since 6.2 and will
-be removed in the near future, users have to ensure that all the topology
-members described with -smp are greater than zero.
-
 Plugin argument passing through ``arg=<string>`` (since 6.1)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 417a0e4fa1d9..f9cf874f7b1f 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -489,6 +489,21 @@ The ``-singlestep`` option has been turned into an accelerator property,
 and given a name that better reflects what it actually does.
 Use ``-accel tcg,one-insn-per-tb=on`` instead.
 
+``-smp`` ("parameter=0" SMP configurations) (removed in 9.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Specified CPU topology parameters must be greater than zero.
+
+In the SMP configuration, users should either provide a CPU topology
+parameter with a reasonable value (greater than zero) or just omit it
+and QEMU will compute the missing value.
+
+However, historically it was implicitly allowed for users to provide
+a parameter with zero value, which is meaningless and could also possibly
+cause unexpected results in the -smp parsing. So support for this kind of
+configurations (e.g. -smp 8,sockets=0) is removed since 9.0, users have
+to ensure that all the topology members described with -smp are greater
+than zero.
 
 User-mode emulator command line arguments
 -----------------------------------------
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 25019c91ee36..96533886b14e 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
         (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");
+        error_setg(errp, "Invalid CPU topology: "
+                   "CPU topology parameters must be greater than zero");
+        return;
     }
 
     /*
-- 
2.34.1

[PATCH 02/14] hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Currentlt, it was allowed for users to specify the unsupported
topology parameter as "1". For example, x86 PC machine doesn't
support drawer/book/cluster topology levels, but user could specify
"-smp drawers=1,books=1,clusters=1".

This is meaningless and confusing, so that the support for this kind of
configurations is marked depresated since 9.0. And report warning
message for such case like:

qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
                    Unsupported clusters parameter mustn't be specified as 1
qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
                    Unsupported books parameter mustn't be specified as 1
qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
                    Unsupported drawers parameter mustn't be specified as 1

Users have to ensure that all the topology members described with -smp
are supported by the target machine.

Cc: devel@lists.libvirt.org
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 docs/about/deprecated.rst | 14 +++++++++
 hw/core/machine-smp.c     | 63 +++++++++++++++++++++++++++++----------
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 872974640252..2e782e83e952 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -47,6 +47,20 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
 However, short-form booleans are deprecated and full explicit ``arg_name=on``
 form is preferred.
 
+``-smp`` (Unsopported "parameter=1" SMP configurations) (since 9.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Specified CPU topology parameters must be supported by the machine.
+
+In the SMP configuration, users should provide the CPU topology parameters that
+are supported by the target machine.
+
+However, historically it was allowed for users to specify the unsupported
+topology parameter as "1", which is meaningless. So support for this kind of
+configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
+marked depresated since 9.0, users have to ensure that all the topology members
+described with -smp are supported by the target machine.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 96533886b14e..50a5a40dbc3d 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -112,30 +112,61 @@ void machine_parse_smp_config(MachineState *ms,
 
     /*
      * If not supported by the machine, a topology parameter must be
-     * omitted or specified equal to 1.
+     * omitted.
      */
-    if (!mc->smp_props.dies_supported && dies > 1) {
-        error_setg(errp, "dies not supported by this machine's CPU topology");
-        return;
-    }
-    if (!mc->smp_props.clusters_supported && clusters > 1) {
-        error_setg(errp, "clusters not supported by this machine's CPU topology");
-        return;
+    if (!mc->smp_props.clusters_supported && config->has_clusters) {
+        if (config->clusters > 1) {
+            error_setg(errp, "clusters not supported by this "
+                       "machine's CPU topology");
+            return;
+        } else {
+            /* Here clusters only equals 1 since we've checked zero case. */
+            warn_report("Deprecated CPU topology (considered invalid): "
+                        "Unsupported clusters parameter mustn't be "
+                        "specified as 1");
+        }
     }
+    clusters = clusters > 0 ? clusters : 1;
 
+    if (!mc->smp_props.dies_supported && config->has_dies) {
+        if (config->dies > 1) {
+            error_setg(errp, "dies not supported by this "
+                       "machine's CPU topology");
+            return;
+        } else {
+            /* Here dies only equals 1 since we've checked zero case. */
+            warn_report("Deprecated CPU topology (considered invalid): "
+                        "Unsupported dies parameter mustn't be "
+                        "specified as 1");
+        }
+    }
     dies = dies > 0 ? dies : 1;
-    clusters = clusters > 0 ? clusters : 1;
 
-    if (!mc->smp_props.books_supported && books > 1) {
-        error_setg(errp, "books not supported by this machine's CPU topology");
-        return;
+    if (!mc->smp_props.books_supported && config->has_books) {
+        if (config->books > 1) {
+            error_setg(errp, "books not supported by this "
+                       "machine's CPU topology");
+            return;
+        } else {
+            /* Here books only equals 1 since we've checked zero case. */
+            warn_report("Deprecated CPU topology (considered invalid): "
+                        "Unsupported books parameter mustn't be "
+                        "specified as 1");
+        }
     }
     books = books > 0 ? books : 1;
 
-    if (!mc->smp_props.drawers_supported && drawers > 1) {
-        error_setg(errp,
-                   "drawers not supported by this machine's CPU topology");
-        return;
+    if (!mc->smp_props.drawers_supported && config->has_drawers) {
+        if (config->drawers > 1) {
+            error_setg(errp, "drawers not supported by this "
+                       "machine's CPU topology");
+            return;
+        } else {
+            /* Here drawers only equals 1 since we've checked zero case. */
+            warn_report("Deprecated CPU topology (considered invalid): "
+                        "Unsupported drawers parameter mustn't be "
+                        "specified as 1");
+        }
     }
     drawers = drawers > 0 ? drawers : 1;
 
-- 
2.34.1

[PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

SMPConfiguration initializes its int64_t members as 0 by default.

Therefore, in machine_parse_smp_config(), initialize local topology
variables with SMPConfiguration's members directly.

Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/machine-smp.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 50a5a40dbc3d..3d9799aef039 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -82,15 +82,15 @@ void machine_parse_smp_config(MachineState *ms,
                               const SMPConfiguration *config, Error **errp)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
-    unsigned cpus    = config->has_cpus ? config->cpus : 0;
-    unsigned drawers = config->has_drawers ? config->drawers : 0;
-    unsigned books   = config->has_books ? config->books : 0;
-    unsigned sockets = config->has_sockets ? config->sockets : 0;
-    unsigned dies    = config->has_dies ? config->dies : 0;
-    unsigned clusters = config->has_clusters ? config->clusters : 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;
+    unsigned cpus     = config->cpus;
+    unsigned drawers  = config->drawers;
+    unsigned books    = config->books;
+    unsigned sockets  = config->sockets;
+    unsigned dies     = config->dies;
+    unsigned clusters = config->clusters;
+    unsigned cores    = config->cores;
+    unsigned threads  = config->threads;
+    unsigned maxcpus  = config->maxcpus;
 
     /*
      * Specified CPU topology parameters must be greater than zero,
-- 
2.34.1

[PATCH 04/14] hw/core/machine-smp: Calculate total CPUs once in machine_parse_smp_config()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

In machine_parse_smp_config(), the number of total CPUs is calculated
by:

    drawers * books * sockets * dies * clusters * cores * threads

To avoid missing the future new topology level, use a local variable to
cache the calculation result so that total CPUs are only calculated
once.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/machine-smp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 3d9799aef039..0e398ef02666 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -91,6 +91,7 @@ void machine_parse_smp_config(MachineState *ms,
     unsigned cores    = config->cores;
     unsigned threads  = config->threads;
     unsigned maxcpus  = config->maxcpus;
+    unsigned total_cpus;
 
     /*
      * Specified CPU topology parameters must be greater than zero,
@@ -211,8 +212,8 @@ void machine_parse_smp_config(MachineState *ms,
         }
     }
 
-    maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies *
-                                      clusters * cores * threads;
+    total_cpus = drawers * books * sockets * dies * clusters * cores * threads;
+    maxcpus = maxcpus > 0 ? maxcpus : total_cpus;
     cpus = cpus > 0 ? cpus : maxcpus;
 
     ms->smp.cpus = cpus;
@@ -228,8 +229,7 @@ void machine_parse_smp_config(MachineState *ms,
     mc->smp_props.has_clusters = config->has_clusters;
 
     /* sanity-check of the computed topology */
-    if (drawers * books * sockets * dies * clusters * cores * threads !=
-        maxcpus) {
+    if (total_cpus != maxcpus) {
         g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
         error_setg(errp, "Invalid CPU topology: "
                    "product of the hierarchy must match maxcpus: "
-- 
2.34.1

[PATCH 05/14] tests/unit/test-smp-parse: Drop the unsupported "dies=1" case

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Unsupported "parameter=1" SMP configurations is marked as deprecated,
so drop the related test case.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 tests/unit/test-smp-parse.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 24972666a74d..1874bea08609 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -607,11 +607,6 @@ static void test_generic_valid(const void *opaque)
         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);
     }
 
     object_unref(obj);
-- 
2.34.1

[PATCH 06/14] tests/unit/test-smp-parse: Use CPU number macros in invalid topology case

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Use MAX_CPUS/MIN_CPUS macros in invalid topology case. This gives us the
flexibility to change the maximum and minimum CPU limits.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/unit/test-smp-parse.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 1874bea08609..84e342277452 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -323,15 +323,21 @@ static const struct SMPTestData data_generic_invalid[] = {
                         "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),
+        /*
+         * config: -smp 1
+         * The test machine should tweak the supported min CPUs to
+         * 2 (MIN_CPUS + 1) for testing.
+         */
+        .config = SMP_CONFIG_GENERIC(T, MIN_CPUS, F, 0, F, 0, F, 0, F, 0),
         .expect_error = "Invalid SMP CPUs 1. The min CPUs supported "
                         "by machine '" SMP_MACHINE_NAME "' 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),
+        /*
+         * config: -smp 512
+         * The test machine should tweak the supported max CPUs to
+         * 511 (MAX_CPUS - 1) for testing.
+         */
+        .config = SMP_CONFIG_GENERIC(T, MAX_CPUS, F, 0, F, 0, F, 0, F, 0),
         .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
                         "by machine '" SMP_MACHINE_NAME "' is 511",
     },
@@ -575,8 +581,8 @@ static void machine_generic_invalid_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
 
     /* Force invalid min CPUs and max CPUs */
-    mc->min_cpus = 2;
-    mc->max_cpus = 511;
+    mc->min_cpus = MIN_CPUS + 1;
+    mc->max_cpus = MAX_CPUS - 1;
 }
 
 static void machine_with_dies_class_init(ObjectClass *oc, void *data)
-- 
2.34.1

[PATCH 07/14] tests/unit/test-smp-parse: Bump max_cpus to 4096

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The q35 machine is trying to support up to 4096 vCPUs [1], so it's
necessary to bump max_cpus in test-smp-parse to 4096 to cover the
topological needs of future machines.

[1]: https://lore.kernel.org/qemu-devel/20240228143351.3967-1-anisinha@redhat.com/

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
---
 tests/unit/test-smp-parse.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 84e342277452..2eb9533bc505 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -20,8 +20,8 @@
 #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 */
+#define MIN_CPUS 1    /* set the min CPUs supported by the machine as 1 */
+#define MAX_CPUS 4096 /* set the max CPUs supported by the machine as 4096 */
 
 #define SMP_MACHINE_NAME "TEST-SMP"
 
@@ -333,13 +333,13 @@ static const struct SMPTestData data_generic_invalid[] = {
                         "by machine '" SMP_MACHINE_NAME "' is 2",
     }, {
         /*
-         * config: -smp 512
+         * config: -smp 4096
          * The test machine should tweak the supported max CPUs to
-         * 511 (MAX_CPUS - 1) for testing.
+         * 4095 (MAX_CPUS - 1) for testing.
          */
-        .config = SMP_CONFIG_GENERIC(T, MAX_CPUS, F, 0, F, 0, F, 0, F, 0),
-        .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
-                        "by machine '" SMP_MACHINE_NAME "' is 511",
+        .config = SMP_CONFIG_GENERIC(T, 4096, F, 0, F, 0, F, 0, F, 0),
+        .expect_error = "Invalid SMP CPUs 4096. The max CPUs supported "
+                        "by machine '" SMP_MACHINE_NAME "' is 4095",
     },
 };
 
-- 
2.34.1

[PATCH 08/14] tests/unit/test-smp-parse: Make test cases aware of the book/drawer

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Currently, -smp supports 2 more new levels: book and drawer.

It is necessary to consider the effects of book and drawer in the test
cases to ensure that the calculations are correct. This is also the
preparation to add new book and drawer test cases.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/unit/test-smp-parse.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 2eb9533bc505..f656bbb6da27 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -384,6 +384,8 @@ static char *smp_config_to_string(const SMPConfiguration *config)
     return g_strdup_printf(
         "(SMPConfiguration) {\n"
         "    .has_cpus     = %5s, cpus     = %" PRId64 ",\n"
+        "    .has_drawers  = %5s, drawers  = %" PRId64 ",\n"
+        "    .has_books    = %5s, books    = %" PRId64 ",\n"
         "    .has_sockets  = %5s, sockets  = %" PRId64 ",\n"
         "    .has_dies     = %5s, dies     = %" PRId64 ",\n"
         "    .has_clusters = %5s, clusters = %" PRId64 ",\n"
@@ -392,6 +394,8 @@ static char *smp_config_to_string(const SMPConfiguration *config)
         "    .has_maxcpus  = %5s, maxcpus  = %" PRId64 ",\n"
         "}",
         config->has_cpus ? "true" : "false", config->cpus,
+        config->has_drawers ? "true" : "false", config->drawers,
+        config->has_books ? "true" : "false", config->books,
         config->has_sockets ? "true" : "false", config->sockets,
         config->has_dies ? "true" : "false", config->dies,
         config->has_clusters ? "true" : "false", config->clusters,
@@ -404,10 +408,10 @@ static char *smp_config_to_string(const SMPConfiguration *config)
 static unsigned int cpu_topology_get_threads_per_socket(const CpuTopology *topo)
 {
     /* Check the divisor to avoid invalid topology examples causing SIGFPE. */
-    if (!topo->sockets) {
+    if (!topo->drawers || !topo->books || !topo->sockets) {
         return 0;
     } else {
-        return topo->max_cpus / topo->sockets;
+        return topo->max_cpus / topo->drawers / topo->books / topo->sockets;
     }
 }
 
@@ -429,6 +433,8 @@ static char *cpu_topology_to_string(const CpuTopology *topo,
     return g_strdup_printf(
         "(CpuTopology) {\n"
         "    .cpus               = %u,\n"
+        "    .drawers            = %u,\n"
+        "    .books              = %u,\n"
         "    .sockets            = %u,\n"
         "    .dies               = %u,\n"
         "    .clusters           = %u,\n"
@@ -438,7 +444,8 @@ static char *cpu_topology_to_string(const CpuTopology *topo,
         "    .threads_per_socket = %u,\n"
         "    .cores_per_socket   = %u,\n"
         "}",
-        topo->cpus, topo->sockets, topo->dies, topo->clusters,
+        topo->cpus, topo->drawers, topo->books,
+        topo->sockets, topo->dies, topo->clusters,
         topo->cores, topo->threads, topo->max_cpus,
         threads_per_socket, cores_per_socket);
 }
@@ -473,6 +480,8 @@ static void check_parse(MachineState *ms, const SMPConfiguration *config,
     if (is_valid) {
         if ((err == NULL) &&
             (ms->smp.cpus == expect_topo->cpus) &&
+            (ms->smp.drawers == expect_topo->drawers) &&
+            (ms->smp.books == expect_topo->books) &&
             (ms->smp.sockets == expect_topo->sockets) &&
             (ms->smp.dies == expect_topo->dies) &&
             (ms->smp.clusters == expect_topo->clusters) &&
@@ -564,6 +573,16 @@ static void unsupported_params_init(const MachineClass *mc, SMPTestData *data)
         data->expect_prefer_sockets.clusters = 1;
         data->expect_prefer_cores.clusters = 1;
     }
+
+    if (!mc->smp_props.books_supported) {
+        data->expect_prefer_sockets.books = 1;
+        data->expect_prefer_cores.books = 1;
+    }
+
+    if (!mc->smp_props.drawers_supported) {
+        data->expect_prefer_sockets.drawers = 1;
+        data->expect_prefer_cores.drawers = 1;
+    }
 }
 
 static void machine_base_class_init(ObjectClass *oc, void *data)
-- 
2.34.1

[PATCH 09/14] tests/unit/test-smp-parse: Test "books" parameter in -smp

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Although book was introduced to -smp along with drawer by s390 machine,
as a general topology level in QEMU that may be reused by other arches
in the future, it is desirable to cover this parameter's parsing in a
separate case.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/unit/test-smp-parse.c | 105 ++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index f656bbb6da27..090238ab23f8 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -75,6 +75,22 @@
             .has_maxcpus  = hf, .maxcpus  = f,                \
         }
 
+/*
+ * Currently a 5-level topology hierarchy is supported on s390 ccw machines
+ *  -drawers/books/sockets/cores/threads
+ */
+#define SMP_CONFIG_WITH_BOOKS_DRAWERS(ha, a, hb, b, hc, c, hd, \
+                                      d, he, e, hf, f, hg, g)  \
+        {                                                      \
+            .has_cpus     = ha, .cpus     = a,                 \
+            .has_drawers  = hb, .drawers  = b,                 \
+            .has_books    = hc, .books    = c,                 \
+            .has_sockets  = hd, .sockets  = d,                 \
+            .has_cores    = he, .cores    = e,                 \
+            .has_threads  = hf, .threads  = f,                 \
+            .has_maxcpus  = hg, .maxcpus  = g,                 \
+        }
+
 /**
  * @config - the given SMP configuration
  * @expect_prefer_sockets - the expected parsing result for the
@@ -308,6 +324,11 @@ static const struct SMPTestData data_generic_invalid[] = {
         /* config: -smp 2,clusters=2 */
         .config = SMP_CONFIG_WITH_CLUSTERS(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
         .expect_error = "clusters not supported by this machine's CPU topology",
+    }, {
+        /* config: -smp 2,books=2 */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, F, 0, T, 2, F,
+                                                0, F, 0, F, 0, F, 0),
+        .expect_error = "books 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),
@@ -379,6 +400,26 @@ static const struct SMPTestData data_with_clusters_invalid[] = {
     },
 };
 
+static const struct SMPTestData data_with_books_invalid[] = {
+    {
+        /* config: -smp 16,books=2,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 16, F, 1, T, 2, T,
+                                                2, T, 4, T, 2, T, 16),
+        .expect_error = "Invalid CPU topology: "
+                        "product of the hierarchy must match maxcpus: "
+                        "books (2) * sockets (2) * cores (4) * threads (2) "
+                        "!= maxcpus (16)",
+    }, {
+        /* config: -smp 34,books=2,sockets=2,cores=4,threads=2,maxcpus=32 */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 34, F, 1, T, 2, T,
+                                                2, T, 4, T, 2, T, 32),
+        .expect_error = "Invalid CPU topology: "
+                        "maxcpus must be equal to or greater than smp: "
+                        "books (2) * sockets (2) * cores (4) * threads (2) "
+                        "== maxcpus (32) < smp_cpus (34)",
+    },
+};
+
 static char *smp_config_to_string(const SMPConfiguration *config)
 {
     return g_strdup_printf(
@@ -618,6 +659,13 @@ static void machine_with_clusters_class_init(ObjectClass *oc, void *data)
     mc->smp_props.clusters_supported = true;
 }
 
+static void machine_with_books_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->smp_props.books_supported = true;
+}
+
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
@@ -756,6 +804,56 @@ static void test_with_clusters(const void *opaque)
     object_unref(obj);
 }
 
+static void test_with_books(const void *opaque)
+{
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData data = {};
+    unsigned int num_books = 2;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+        data = data_generic_valid[i];
+        unsupported_params_init(mc, &data);
+
+        /* when books parameter is omitted, it will be set as 1 */
+        data.expect_prefer_sockets.books = 1;
+        data.expect_prefer_cores.books = 1;
+
+        smp_parse_test(ms, &data, true);
+
+        /* when books parameter is specified */
+        data.config.has_books = true;
+        data.config.books = num_books;
+        if (data.config.has_cpus) {
+            data.config.cpus *= num_books;
+        }
+        if (data.config.has_maxcpus) {
+            data.config.maxcpus *= num_books;
+        }
+
+        data.expect_prefer_sockets.books = num_books;
+        data.expect_prefer_sockets.cpus *= num_books;
+        data.expect_prefer_sockets.max_cpus *= num_books;
+        data.expect_prefer_cores.books = num_books;
+        data.expect_prefer_cores.cpus *= num_books;
+        data.expect_prefer_cores.max_cpus *= num_books;
+
+        smp_parse_test(ms, &data, true);
+    }
+
+    for (i = 0; i < ARRAY_SIZE(data_with_books_invalid); i++) {
+        data = data_with_books_invalid[i];
+        unsupported_params_init(mc, &data);
+
+        smp_parse_test(ms, &data, false);
+    }
+
+    object_unref(obj);
+}
+
 /* Type info of the tested machine */
 static const TypeInfo smp_machine_types[] = {
     {
@@ -780,6 +878,10 @@ static const TypeInfo smp_machine_types[] = {
         .name           = MACHINE_TYPE_NAME("smp-with-clusters"),
         .parent         = TYPE_MACHINE,
         .class_init     = machine_with_clusters_class_init,
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-with-books"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_with_books_class_init,
     }
 };
 
@@ -803,6 +905,9 @@ int main(int argc, char *argv[])
     g_test_add_data_func("/test-smp-parse/with_clusters",
                          MACHINE_TYPE_NAME("smp-with-clusters"),
                          test_with_clusters);
+    g_test_add_data_func("/test-smp-parse/with_books",
+                         MACHINE_TYPE_NAME("smp-with-books"),
+                         test_with_books);
 
     g_test_run();
 
-- 
2.34.1

[PATCH 10/14] tests/unit/test-smp-parse: Test "drawers" parameter in -smp

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Although drawer was introduced to -smp along with book by s390 machine,
as a general topology level in QEMU that may be reused by other arches
in the future, it is desirable to cover this parameter's parsing in a
separate case.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 tests/unit/test-smp-parse.c | 89 +++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 090238ab23f8..aea1b2e73a55 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -329,6 +329,11 @@ static const struct SMPTestData data_generic_invalid[] = {
         .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, F, 0, T, 2, F,
                                                 0, F, 0, F, 0, F, 0),
         .expect_error = "books not supported by this machine's CPU topology",
+    }, {
+        /* config: -smp 2,drawers=2 */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 2, T, 2, F, 0, F,
+                                                0, F, 0, F, 0, F, 0),
+        .expect_error = "drawers 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),
@@ -420,6 +425,26 @@ static const struct SMPTestData data_with_books_invalid[] = {
     },
 };
 
+static const struct SMPTestData data_with_drawers_invalid[] = {
+    {
+        /* config: -smp 16,drawers=2,sockets=2,cores=4,threads=2,maxcpus=16 */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 16, T, 2, F, 1, T,
+                                                2, T, 4, T, 2, T, 16),
+        .expect_error = "Invalid CPU topology: "
+                        "product of the hierarchy must match maxcpus: "
+                        "drawers (2) * sockets (2) * cores (4) * threads (2) "
+                        "!= maxcpus (16)",
+    }, {
+        /* config: -smp 34,drawers=2,sockets=2,cores=4,threads=2,maxcpus=32 */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 34, T, 2, F, 1, T,
+                                                2, T, 4, T, 2, T, 32),
+        .expect_error = "Invalid CPU topology: "
+                        "maxcpus must be equal to or greater than smp: "
+                        "drawers (2) * sockets (2) * cores (4) * threads (2) "
+                        "== maxcpus (32) < smp_cpus (34)",
+    },
+};
+
 static char *smp_config_to_string(const SMPConfiguration *config)
 {
     return g_strdup_printf(
@@ -666,6 +691,13 @@ static void machine_with_books_class_init(ObjectClass *oc, void *data)
     mc->smp_props.books_supported = true;
 }
 
+static void machine_with_drawers_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->smp_props.drawers_supported = true;
+}
+
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
@@ -854,6 +886,56 @@ static void test_with_books(const void *opaque)
     object_unref(obj);
 }
 
+static void test_with_drawers(const void *opaque)
+{
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData data = {};
+    unsigned int num_drawers = 2;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+        data = data_generic_valid[i];
+        unsupported_params_init(mc, &data);
+
+        /* when drawers parameter is omitted, it will be set as 1 */
+        data.expect_prefer_sockets.drawers = 1;
+        data.expect_prefer_cores.drawers = 1;
+
+        smp_parse_test(ms, &data, true);
+
+        /* when drawers parameter is specified */
+        data.config.has_drawers = true;
+        data.config.drawers = num_drawers;
+        if (data.config.has_cpus) {
+            data.config.cpus *= num_drawers;
+        }
+        if (data.config.has_maxcpus) {
+            data.config.maxcpus *= num_drawers;
+        }
+
+        data.expect_prefer_sockets.drawers = num_drawers;
+        data.expect_prefer_sockets.cpus *= num_drawers;
+        data.expect_prefer_sockets.max_cpus *= num_drawers;
+        data.expect_prefer_cores.drawers = num_drawers;
+        data.expect_prefer_cores.cpus *= num_drawers;
+        data.expect_prefer_cores.max_cpus *= num_drawers;
+
+        smp_parse_test(ms, &data, true);
+    }
+
+    for (i = 0; i < ARRAY_SIZE(data_with_drawers_invalid); i++) {
+        data = data_with_drawers_invalid[i];
+        unsupported_params_init(mc, &data);
+
+        smp_parse_test(ms, &data, false);
+    }
+
+    object_unref(obj);
+}
+
 /* Type info of the tested machine */
 static const TypeInfo smp_machine_types[] = {
     {
@@ -882,6 +964,10 @@ static const TypeInfo smp_machine_types[] = {
         .name           = MACHINE_TYPE_NAME("smp-with-books"),
         .parent         = TYPE_MACHINE,
         .class_init     = machine_with_books_class_init,
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-with-drawers"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_with_drawers_class_init,
     }
 };
 
@@ -908,6 +994,9 @@ int main(int argc, char *argv[])
     g_test_add_data_func("/test-smp-parse/with_books",
                          MACHINE_TYPE_NAME("smp-with-books"),
                          test_with_books);
+    g_test_add_data_func("/test-smp-parse/with_drawers",
+                         MACHINE_TYPE_NAME("smp-with-drawers"),
+                         test_with_drawers);
 
     g_test_run();
 
-- 
2.34.1

[PATCH 11/14] tests/unit/test-smp-parse: Test "drawers" and "books" combination case

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Since s390 machine supports both "drawers" and "books" in -smp, add the
"drawers" and "books" combination test case to match the actual topology
usage scenario.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
---
 tests/unit/test-smp-parse.c | 103 ++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index aea1b2e73a55..0cf611519865 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -445,6 +445,33 @@ static const struct SMPTestData data_with_drawers_invalid[] = {
     },
 };
 
+static const struct SMPTestData data_with_drawers_books_invalid[] = {
+    {
+        /*
+         * config: -smp 200,drawers=2,books=2,sockets=2,cores=4,\
+         * threads=2,maxcpus=200
+         */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 200, T, 3, T, 5, T,
+                                                2, T, 4, T, 2, T, 200),
+        .expect_error = "Invalid CPU topology: "
+                        "product of the hierarchy must match maxcpus: "
+                        "drawers (3) * books (5) * sockets (2) * "
+                        "cores (4) * threads (2) != maxcpus (200)",
+    }, {
+        /*
+         * config: -smp 242,drawers=2,books=2,sockets=2,cores=4,\
+         * threads=2,maxcpus=240
+         */
+        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 242, T, 3, T, 5, T,
+                                                2, T, 4, T, 2, T, 240),
+        .expect_error = "Invalid CPU topology: "
+                        "maxcpus must be equal to or greater than smp: "
+                        "drawers (3) * books (5) * sockets (2) * "
+                        "cores (4) * threads (2) "
+                        "== maxcpus (240) < smp_cpus (242)",
+    },
+};
+
 static char *smp_config_to_string(const SMPConfiguration *config)
 {
     return g_strdup_printf(
@@ -698,6 +725,14 @@ static void machine_with_drawers_class_init(ObjectClass *oc, void *data)
     mc->smp_props.drawers_supported = true;
 }
 
+static void machine_with_drawers_books_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->smp_props.drawers_supported = true;
+    mc->smp_props.books_supported = true;
+}
+
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
@@ -936,6 +971,67 @@ static void test_with_drawers(const void *opaque)
     object_unref(obj);
 }
 
+static void test_with_drawers_books(const void *opaque)
+{
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData data = {};
+    unsigned int num_drawers = 5, num_books = 3;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+        data = data_generic_valid[i];
+        unsupported_params_init(mc, &data);
+
+        /*
+         * when drawers and books parameters are omitted, they will
+         * be both set as 1.
+         */
+        data.expect_prefer_sockets.drawers = 1;
+        data.expect_prefer_sockets.books = 1;
+        data.expect_prefer_cores.drawers = 1;
+        data.expect_prefer_cores.books = 1;
+
+        smp_parse_test(ms, &data, true);
+
+        /* when drawers and books parameters are both specified */
+        data.config.has_drawers = true;
+        data.config.drawers = num_drawers;
+        data.config.has_books = true;
+        data.config.books = num_books;
+
+        if (data.config.has_cpus) {
+            data.config.cpus *= num_drawers * num_books;
+        }
+        if (data.config.has_maxcpus) {
+            data.config.maxcpus *= num_drawers * num_books;
+        }
+
+        data.expect_prefer_sockets.drawers = num_drawers;
+        data.expect_prefer_sockets.books = num_books;
+        data.expect_prefer_sockets.cpus *= num_drawers * num_books;
+        data.expect_prefer_sockets.max_cpus *= num_drawers * num_books;
+
+        data.expect_prefer_cores.drawers = num_drawers;
+        data.expect_prefer_cores.books = num_books;
+        data.expect_prefer_cores.cpus *= num_drawers * num_books;
+        data.expect_prefer_cores.max_cpus *= num_drawers * num_books;
+
+        smp_parse_test(ms, &data, true);
+    }
+
+    for (i = 0; i < ARRAY_SIZE(data_with_drawers_books_invalid); i++) {
+        data = data_with_drawers_books_invalid[i];
+        unsupported_params_init(mc, &data);
+
+        smp_parse_test(ms, &data, false);
+    }
+
+    object_unref(obj);
+}
+
 /* Type info of the tested machine */
 static const TypeInfo smp_machine_types[] = {
     {
@@ -968,6 +1064,10 @@ static const TypeInfo smp_machine_types[] = {
         .name           = MACHINE_TYPE_NAME("smp-with-drawers"),
         .parent         = TYPE_MACHINE,
         .class_init     = machine_with_drawers_class_init,
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-with-drawers-books"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_with_drawers_books_class_init,
     }
 };
 
@@ -997,6 +1097,9 @@ int main(int argc, char *argv[])
     g_test_add_data_func("/test-smp-parse/with_drawers",
                          MACHINE_TYPE_NAME("smp-with-drawers"),
                          test_with_drawers);
+    g_test_add_data_func("/test-smp-parse/with_drawers_books",
+                         MACHINE_TYPE_NAME("smp-with-drawers-books"),
+                         test_with_drawers_books);
 
     g_test_run();
 
-- 
2.34.1

[PATCH 12/14] tests/unit/test-smp-parse: Test the full 7-levels topology hierarchy

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Currently, -smp supports up to 7-levels topology hierarchy:
  -drawers/books/sockets/dies/clusters/cores/threads.

Though no machine supports all these 7 levels yet, these 7 levels have
the strict containment relationship and together form the generic CPU
topology representation of QEMU.

Also, note that the maxcpus is calculated by multiplying all 7 levels:

  maxcpus = drawers * books * sockets * dies * clusters *
            cores * threads.

To cover this code path, it is necessary to test the full topology case
(with all 7 levels). This also helps to avoid introducing new issues by
further expanding the CPU topology in the future.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 tests/unit/test-smp-parse.c | 143 ++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 0cf611519865..75581691713c 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -91,6 +91,24 @@
             .has_maxcpus  = hg, .maxcpus  = g,                 \
         }
 
+/*
+ * Currently QEMU supports up to a 7-level topology hierarchy, which is the
+ * QEMU's unified abstract representation of CPU topology.
+ *  -drawers/books/sockets/dies/clusters/cores/threads
+ */
+#define SMP_CONFIG_WITH_FULL_TOPO(a, b, c, d, e, f, g, h, i)    \
+        {                                                       \
+            .has_cpus     = true, .cpus     = a,                \
+            .has_drawers  = true, .drawers  = b,                \
+            .has_books    = true, .books    = c,                \
+            .has_sockets  = true, .sockets  = d,                \
+            .has_dies     = true, .dies     = e,                \
+            .has_clusters = true, .clusters = f,                \
+            .has_cores    = true, .cores    = g,                \
+            .has_threads  = true, .threads  = h,                \
+            .has_maxcpus  = true, .maxcpus  = i,                \
+        }
+
 /**
  * @config - the given SMP configuration
  * @expect_prefer_sockets - the expected parsing result for the
@@ -472,6 +490,40 @@ static const struct SMPTestData data_with_drawers_books_invalid[] = {
     },
 };
 
+static const struct SMPTestData data_full_topo_invalid[] = {
+    {
+        /*
+         * config: -smp 200,drawers=3,books=5,sockets=2,dies=4,\
+         *              clusters=2,cores=7,threads=2,maxcpus=200
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(200, 3, 5, 2, 4, 2, 7, 2, 200),
+        .expect_error = "Invalid CPU topology: "
+                        "product of the hierarchy must match maxcpus: "
+                        "drawers (3) * books (5) * sockets (2) * dies (4) * "
+                        "clusters (2) * cores (7) * threads (2) "
+                        "!= maxcpus (200)",
+    }, {
+        /*
+         * config: -smp 3361,drawers=3,books=5,sockets=2,dies=4,\
+         *              clusters=2,cores=7,threads=2,maxcpus=3360
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(3361, 3, 5, 2, 4, 2, 7, 2, 3360),
+        .expect_error = "Invalid CPU topology: "
+                        "maxcpus must be equal to or greater than smp: "
+                        "drawers (3) * books (5) * sockets (2) * dies (4) * "
+                        "clusters (2) * cores (7) * threads (2) "
+                        "== maxcpus (3360) < smp_cpus (3361)",
+    }, {
+        /*
+         * config: -smp 1,drawers=3,books=5,sockets=2,dies=4,\
+         *              clusters=2,cores=7,threads=3,maxcpus=5040
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(3361, 3, 5, 2, 4, 2, 7, 3, 5040),
+        .expect_error = "Invalid SMP CPUs 5040. The max CPUs supported "
+                        "by machine '" SMP_MACHINE_NAME "' is 4096",
+    },
+};
+
 static char *smp_config_to_string(const SMPConfiguration *config)
 {
     return g_strdup_printf(
@@ -733,6 +785,16 @@ static void machine_with_drawers_books_class_init(ObjectClass *oc, void *data)
     mc->smp_props.books_supported = true;
 }
 
+static void machine_full_topo_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->smp_props.drawers_supported = true;
+    mc->smp_props.books_supported = true;
+    mc->smp_props.dies_supported = true;
+    mc->smp_props.clusters_supported = true;
+}
+
 static void test_generic_valid(const void *opaque)
 {
     const char *machine_type = opaque;
@@ -1032,6 +1094,80 @@ static void test_with_drawers_books(const void *opaque)
     object_unref(obj);
 }
 
+static void test_full_topo(const void *opaque)
+{
+    const char *machine_type = opaque;
+    Object *obj = object_new(machine_type);
+    MachineState *ms = MACHINE(obj);
+    MachineClass *mc = MACHINE_GET_CLASS(obj);
+    SMPTestData data = {};
+    unsigned int drawers = 5, books = 3, dies = 2, clusters = 7, multiplier;
+    int i;
+
+    multiplier = drawers * books * dies * clusters;
+    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+        data = data_generic_valid[i];
+        unsupported_params_init(mc, &data);
+
+        /*
+         * when drawers, books, dies and clusters parameters are omitted,
+         * they will be set as 1.
+         */
+        data.expect_prefer_sockets.drawers = 1;
+        data.expect_prefer_sockets.books = 1;
+        data.expect_prefer_sockets.dies = 1;
+        data.expect_prefer_sockets.clusters = 1;
+        data.expect_prefer_cores.drawers = 1;
+        data.expect_prefer_cores.books = 1;
+        data.expect_prefer_cores.dies = 1;
+        data.expect_prefer_cores.clusters = 1;
+
+        smp_parse_test(ms, &data, true);
+
+        /* when drawers, books, dies and clusters parameters are specified. */
+        data.config.has_drawers = true;
+        data.config.drawers = drawers;
+        data.config.has_books = true;
+        data.config.books = books;
+        data.config.has_dies = true;
+        data.config.dies = dies;
+        data.config.has_clusters = true;
+        data.config.clusters = clusters;
+
+        if (data.config.has_cpus) {
+            data.config.cpus *= multiplier;
+        }
+        if (data.config.has_maxcpus) {
+            data.config.maxcpus *= multiplier;
+        }
+
+        data.expect_prefer_sockets.drawers = drawers;
+        data.expect_prefer_sockets.books = books;
+        data.expect_prefer_sockets.dies = dies;
+        data.expect_prefer_sockets.clusters = clusters;
+        data.expect_prefer_sockets.cpus *= multiplier;
+        data.expect_prefer_sockets.max_cpus *= multiplier;
+
+        data.expect_prefer_cores.drawers = drawers;
+        data.expect_prefer_cores.books = books;
+        data.expect_prefer_cores.dies = dies;
+        data.expect_prefer_cores.clusters = clusters;
+        data.expect_prefer_cores.cpus *= multiplier;
+        data.expect_prefer_cores.max_cpus *= multiplier;
+
+        smp_parse_test(ms, &data, true);
+    }
+
+    for (i = 0; i < ARRAY_SIZE(data_full_topo_invalid); i++) {
+        data = data_full_topo_invalid[i];
+        unsupported_params_init(mc, &data);
+
+        smp_parse_test(ms, &data, false);
+    }
+
+    object_unref(obj);
+}
+
 /* Type info of the tested machine */
 static const TypeInfo smp_machine_types[] = {
     {
@@ -1068,6 +1204,10 @@ static const TypeInfo smp_machine_types[] = {
         .name           = MACHINE_TYPE_NAME("smp-with-drawers-books"),
         .parent         = TYPE_MACHINE,
         .class_init     = machine_with_drawers_books_class_init,
+    }, {
+        .name           = MACHINE_TYPE_NAME("smp-full-topo"),
+        .parent         = TYPE_MACHINE,
+        .class_init     = machine_full_topo_class_init,
     }
 };
 
@@ -1100,6 +1240,9 @@ int main(int argc, char *argv[])
     g_test_add_data_func("/test-smp-parse/with_drawers_books",
                          MACHINE_TYPE_NAME("smp-with-drawers-books"),
                          test_with_drawers_books);
+    g_test_add_data_func("/test-smp-parse/full",
+                         MACHINE_TYPE_NAME("smp-full-topo"),
+                         test_full_topo);
 
     g_test_run();
 
-- 
2.34.1

[PATCH 13/14] tests/unit/test-smp-parse: Test smp_props.has_clusters

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The smp_props.has_clusters in MachineClass is not a user configured
field, and it indicates if user specifies "clusters" in -smp.

After -smp parsing, other module could aware if the cluster level
is configured by user. This is used when the machine has only 1 cluster
since there's only 1 cluster by default.

Add the check to cover this field.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Xiaoling Song <xiaoling.song@intel.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 tests/unit/test-smp-parse.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 75581691713c..d39cfdc19bfe 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -573,7 +573,8 @@ static unsigned int cpu_topology_get_cores_per_socket(const CpuTopology *topo)
 
 static char *cpu_topology_to_string(const CpuTopology *topo,
                                     unsigned int threads_per_socket,
-                                    unsigned int cores_per_socket)
+                                    unsigned int cores_per_socket,
+                                    bool has_clusters)
 {
     return g_strdup_printf(
         "(CpuTopology) {\n"
@@ -588,17 +589,20 @@ static char *cpu_topology_to_string(const CpuTopology *topo,
         "    .max_cpus           = %u,\n"
         "    .threads_per_socket = %u,\n"
         "    .cores_per_socket   = %u,\n"
+        "    .has_clusters       = %s,\n"
         "}",
         topo->cpus, topo->drawers, topo->books,
         topo->sockets, topo->dies, topo->clusters,
         topo->cores, topo->threads, topo->max_cpus,
-        threads_per_socket, cores_per_socket);
+        threads_per_socket, cores_per_socket,
+        has_clusters ? "true" : "false");
 }
 
 static void check_parse(MachineState *ms, const SMPConfiguration *config,
                         const CpuTopology *expect_topo, const char *expect_err,
                         bool is_valid)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
     g_autofree char *config_str = smp_config_to_string(config);
     g_autofree char *expect_topo_str = NULL, *output_topo_str = NULL;
     unsigned int expect_threads_per_socket, expect_cores_per_socket;
@@ -611,15 +615,18 @@ static void check_parse(MachineState *ms, const SMPConfiguration *config,
                         cpu_topology_get_cores_per_socket(expect_topo);
     expect_topo_str = cpu_topology_to_string(expect_topo,
                                              expect_threads_per_socket,
-                                             expect_cores_per_socket);
+                                             expect_cores_per_socket,
+                                             config->has_clusters);
 
     /* call the generic parser */
     machine_parse_smp_config(ms, config, &err);
 
     ms_threads_per_socket = machine_topo_get_threads_per_socket(ms);
     ms_cores_per_socket = machine_topo_get_cores_per_socket(ms);
-    output_topo_str = cpu_topology_to_string(&ms->smp, ms_threads_per_socket,
-                                             ms_cores_per_socket);
+    output_topo_str = cpu_topology_to_string(&ms->smp,
+                                             ms_threads_per_socket,
+                                             ms_cores_per_socket,
+                                             mc->smp_props.has_clusters);
 
     /* when the configuration is supposed to be valid */
     if (is_valid) {
@@ -634,7 +641,8 @@ static void check_parse(MachineState *ms, const SMPConfiguration *config,
             (ms->smp.threads == expect_topo->threads) &&
             (ms->smp.max_cpus == expect_topo->max_cpus) &&
             (ms_threads_per_socket == expect_threads_per_socket) &&
-            (ms_cores_per_socket == expect_cores_per_socket)) {
+            (ms_cores_per_socket == expect_cores_per_socket) &&
+            (mc->smp_props.has_clusters == config->has_clusters)) {
             return;
         }
 
-- 
2.34.1

[PATCH 14/14] tests/unit/test-smp-parse: Test "parameter=0" SMP configurations

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

The support for "parameter=0" SMP configurations is removed, and QEMU
returns error for those cases.

So add the related test cases to ensure parameters can't accept 0.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 tests/unit/test-smp-parse.c | 92 +++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index d39cfdc19bfe..8994337e12c7 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -524,6 +524,91 @@ static const struct SMPTestData data_full_topo_invalid[] = {
     },
 };
 
+static const struct SMPTestData data_zero_topo_invalid[] = {
+    {
+        /*
+         * Test "cpus=0".
+         * config: -smp 0,drawers=1,books=1,sockets=1,dies=1,\
+         *              clusters=1,cores=1,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(0, 1, 1, 1, 1, 1, 1, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "drawers=0".
+         * config: -smp 1,drawers=0,books=1,sockets=1,dies=1,\
+         *              clusters=1,cores=1,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 0, 1, 1, 1, 1, 1, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "books=0".
+         * config: -smp 1,drawers=1,books=0,sockets=1,dies=1,\
+         *              clusters=1,cores=1,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 0, 1, 1, 1, 1, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "sockets=0".
+         * config: -smp 1,drawers=1,books=1,sockets=0,dies=1,\
+         *              clusters=1,cores=1,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 1, 0, 1, 1, 1, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "dies=0".
+         * config: -smp 1,drawers=1,books=1,sockets=1,dies=0,\
+         *              clusters=1,cores=1,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 1, 1, 0, 1, 1, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "clusters=0".
+         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
+         *              clusters=0,cores=1,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 1, 1, 1, 0, 1, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "cores=0".
+         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
+         *              clusters=1,cores=0,threads=1,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 1, 1, 1, 1, 0, 1, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "threads=0".
+         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
+         *              clusters=1,cores=1,threads=0,maxcpus=1
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 1, 1, 1, 1, 1, 0, 1),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    }, {
+        /*
+         * Test "maxcpus=0".
+         * config: -smp 1,drawers=1,books=1,sockets=1,dies=1,\
+         *              clusters=1,cores=1,threads=1,maxcpus=0
+         */
+        .config = SMP_CONFIG_WITH_FULL_TOPO(1, 1, 1, 1, 1, 1, 1, 1, 0),
+        .expect_error = "Invalid CPU topology: CPU topology parameters must "
+                        "be greater than zero",
+    },
+};
+
 static char *smp_config_to_string(const SMPConfiguration *config)
 {
     return g_strdup_printf(
@@ -1173,6 +1258,13 @@ static void test_full_topo(const void *opaque)
         smp_parse_test(ms, &data, false);
     }
 
+    for (i = 0; i < ARRAY_SIZE(data_zero_topo_invalid); i++) {
+        data = data_zero_topo_invalid[i];
+        unsupported_params_init(mc, &data);
+
+        smp_parse_test(ms, &data, false);
+    }
+
     object_unref(obj);
 }
 
-- 
2.34.1

Re: [PATCH 04/14] hw/core/machine-smp: Calculate total CPUs once in machine_parse_smp_config()

[Philippe Mathieu-Daudé]

On 6/3/24 10:53, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> In machine_parse_smp_config(), the number of total CPUs is calculated
> by:
> 
>      drawers * books * sockets * dies * clusters * cores * threads
> 
> To avoid missing the future new topology level, use a local variable to
> cache the calculation result so that total CPUs are only calculated
> once.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/core/machine-smp.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 00/14] Cleanup on SMP and its test

[Philippe Mathieu-Daudé]

Hi,

On 6/3/24 10:53, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Hi all,
> 
> To make review easier, I've merged my previous single SMP patch [1] and
> SMP test series [2] into this series as well.
> 
> So this series includes:
>   * [Patch 1] Remove deprecated "parameter=0" SMP configurations, which
>     is marked as deprecated in v6.2.
>   * [Patch 2] Deprecate unsupported "parameter=1" SMP configurations.
>   * [Patch 3 & 4] Minor code cleanup for machine_parse_smp_config().
>   * [Patch 5 ~ 14] Test case enhancements to cover more SMP API changes.
> 
> [1]: https://lore.kernel.org/qemu-devel/20240304044510.2305849-1-zhao1.liu@linux.intel.com/
> [2]: https://lore.kernel.org/qemu-devel/20240118144857.2124034-1-zhao1.liu@linux.intel.com/
> 
> Thanks and Best Regards,
> Zhao

In a previous community call, Zhao asked us how his work will scale
in the heterogeneous context.

My first idea is CPUs must belong to a cluster. For machines without
explicit cluster, we could always create the first one. Then -smp
would become a sugar property of the first cluster. Next -smp could
also be sugar property of the next cluster.

Regards,

Phil.

Re: [PATCH 07/14] tests/unit/test-smp-parse: Bump max_cpus to 4096

[Thomas Huth]

On 06/03/2024 10.54, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The q35 machine is trying to support up to 4096 vCPUs [1], so it's
> necessary to bump max_cpus in test-smp-parse to 4096 to cover the
> topological needs of future machines.
> 
> [1]: https://lore.kernel.org/qemu-devel/20240228143351.3967-1-anisinha@redhat.com/
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Xiaoling Song <xiaoling.song@intel.com>
> ---
>   tests/unit/test-smp-parse.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 84e342277452..2eb9533bc505 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -20,8 +20,8 @@
>   #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 */
> +#define MIN_CPUS 1    /* set the min CPUs supported by the machine as 1 */
> +#define MAX_CPUS 4096 /* set the max CPUs supported by the machine as 4096 */
>   
>   #define SMP_MACHINE_NAME "TEST-SMP"
>   
> @@ -333,13 +333,13 @@ static const struct SMPTestData data_generic_invalid[] = {
>                           "by machine '" SMP_MACHINE_NAME "' is 2",
>       }, {
>           /*
> -         * config: -smp 512
> +         * config: -smp 4096
>            * The test machine should tweak the supported max CPUs to
> -         * 511 (MAX_CPUS - 1) for testing.
> +         * 4095 (MAX_CPUS - 1) for testing.
>            */
> -        .config = SMP_CONFIG_GENERIC(T, MAX_CPUS, F, 0, F, 0, F, 0, F, 0),
> -        .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
> -                        "by machine '" SMP_MACHINE_NAME "' is 511",
> +        .config = SMP_CONFIG_GENERIC(T, 4096, F, 0, F, 0, F, 0, F, 0),
> +        .expect_error = "Invalid SMP CPUs 4096. The max CPUs supported "

You could maybe use stringify(MAX_CPUS) in above line
(but it won't work for the 4095 below, so it's maybe not worth the effort)

> +                        "by machine '" SMP_MACHINE_NAME "' is 4095",
>       },
>   };

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 11/14] tests/unit/test-smp-parse: Test "drawers" and "books" combination case

[Thomas Huth]

On 06/03/2024 10.54, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Since s390 machine supports both "drawers" and "books" in -smp, add the
> "drawers" and "books" combination test case to match the actual topology
> usage scenario.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Xiaoling Song <xiaoling.song@intel.com>
> ---
>   tests/unit/test-smp-parse.c | 103 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 103 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>


> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index aea1b2e73a55..0cf611519865 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -445,6 +445,33 @@ static const struct SMPTestData data_with_drawers_invalid[] = {
>       },
>   };
>   
> +static const struct SMPTestData data_with_drawers_books_invalid[] = {
> +    {
> +        /*
> +         * config: -smp 200,drawers=2,books=2,sockets=2,cores=4,\
> +         * threads=2,maxcpus=200
> +         */
> +        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 200, T, 3, T, 5, T,
> +                                                2, T, 4, T, 2, T, 200),
> +        .expect_error = "Invalid CPU topology: "
> +                        "product of the hierarchy must match maxcpus: "
> +                        "drawers (3) * books (5) * sockets (2) * "
> +                        "cores (4) * threads (2) != maxcpus (200)",
> +    }, {
> +        /*
> +         * config: -smp 242,drawers=2,books=2,sockets=2,cores=4,\
> +         * threads=2,maxcpus=240
> +         */
> +        .config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 242, T, 3, T, 5, T,
> +                                                2, T, 4, T, 2, T, 240),
> +        .expect_error = "Invalid CPU topology: "
> +                        "maxcpus must be equal to or greater than smp: "
> +                        "drawers (3) * books (5) * sockets (2) * "
> +                        "cores (4) * threads (2) "
> +                        "== maxcpus (240) < smp_cpus (242)",
> +    },
> +};
> +
>   static char *smp_config_to_string(const SMPConfiguration *config)
>   {
>       return g_strdup_printf(
> @@ -698,6 +725,14 @@ static void machine_with_drawers_class_init(ObjectClass *oc, void *data)
>       mc->smp_props.drawers_supported = true;
>   }
>   
> +static void machine_with_drawers_books_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
> +    mc->smp_props.drawers_supported = true;
> +    mc->smp_props.books_supported = true;
> +}
> +
>   static void test_generic_valid(const void *opaque)
>   {
>       const char *machine_type = opaque;
> @@ -936,6 +971,67 @@ static void test_with_drawers(const void *opaque)
>       object_unref(obj);
>   }
>   
> +static void test_with_drawers_books(const void *opaque)
> +{
> +    const char *machine_type = opaque;
> +    Object *obj = object_new(machine_type);
> +    MachineState *ms = MACHINE(obj);
> +    MachineClass *mc = MACHINE_GET_CLASS(obj);
> +    SMPTestData data = {};
> +    unsigned int num_drawers = 5, num_books = 3;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
> +        data = data_generic_valid[i];
> +        unsupported_params_init(mc, &data);
> +
> +        /*
> +         * when drawers and books parameters are omitted, they will
> +         * be both set as 1.
> +         */
> +        data.expect_prefer_sockets.drawers = 1;
> +        data.expect_prefer_sockets.books = 1;
> +        data.expect_prefer_cores.drawers = 1;
> +        data.expect_prefer_cores.books = 1;
> +
> +        smp_parse_test(ms, &data, true);
> +
> +        /* when drawers and books parameters are both specified */
> +        data.config.has_drawers = true;
> +        data.config.drawers = num_drawers;
> +        data.config.has_books = true;
> +        data.config.books = num_books;
> +
> +        if (data.config.has_cpus) {
> +            data.config.cpus *= num_drawers * num_books;
> +        }
> +        if (data.config.has_maxcpus) {
> +            data.config.maxcpus *= num_drawers * num_books;
> +        }
> +
> +        data.expect_prefer_sockets.drawers = num_drawers;
> +        data.expect_prefer_sockets.books = num_books;
> +        data.expect_prefer_sockets.cpus *= num_drawers * num_books;
> +        data.expect_prefer_sockets.max_cpus *= num_drawers * num_books;
> +
> +        data.expect_prefer_cores.drawers = num_drawers;
> +        data.expect_prefer_cores.books = num_books;
> +        data.expect_prefer_cores.cpus *= num_drawers * num_books;
> +        data.expect_prefer_cores.max_cpus *= num_drawers * num_books;
> +
> +        smp_parse_test(ms, &data, true);
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(data_with_drawers_books_invalid); i++) {
> +        data = data_with_drawers_books_invalid[i];
> +        unsupported_params_init(mc, &data);
> +
> +        smp_parse_test(ms, &data, false);
> +    }
> +
> +    object_unref(obj);
> +}
> +
>   /* Type info of the tested machine */
>   static const TypeInfo smp_machine_types[] = {
>       {
> @@ -968,6 +1064,10 @@ static const TypeInfo smp_machine_types[] = {
>           .name           = MACHINE_TYPE_NAME("smp-with-drawers"),
>           .parent         = TYPE_MACHINE,
>           .class_init     = machine_with_drawers_class_init,
> +    }, {
> +        .name           = MACHINE_TYPE_NAME("smp-with-drawers-books"),
> +        .parent         = TYPE_MACHINE,
> +        .class_init     = machine_with_drawers_books_class_init,
>       }
>   };
>   
> @@ -997,6 +1097,9 @@ int main(int argc, char *argv[])
>       g_test_add_data_func("/test-smp-parse/with_drawers",
>                            MACHINE_TYPE_NAME("smp-with-drawers"),
>                            test_with_drawers);
> +    g_test_add_data_func("/test-smp-parse/with_drawers_books",
> +                         MACHINE_TYPE_NAME("smp-with-drawers-books"),
> +                         test_with_drawers_books);
>   
>       g_test_run();
>   

Re: [PATCH 14/14] tests/unit/test-smp-parse: Test "parameter=0" SMP configurations

[Thomas Huth]

On 06/03/2024 10.54, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The support for "parameter=0" SMP configurations is removed, and QEMU
> returns error for those cases.
> 
> So add the related test cases to ensure parameters can't accept 0.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   tests/unit/test-smp-parse.c | 92 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 92 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

[Thomas Huth]

On 06/03/2024 10.53, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Currentlt, it was allowed for users to specify the unsupported

s/Currentlt/Currently/

> topology parameter as "1". For example, x86 PC machine doesn't
> support drawer/book/cluster topology levels, but user could specify
> "-smp drawers=1,books=1,clusters=1".
> 
> This is meaningless and confusing, so that the support for this kind of
> configurations is marked depresated since 9.0. And report warning

s/depresated/deprecated/

> message for such case like:
> 
> qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
>                      Unsupported clusters parameter mustn't be specified as 1
> qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
>                      Unsupported books parameter mustn't be specified as 1
> qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
>                      Unsupported drawers parameter mustn't be specified as 1
> 
> Users have to ensure that all the topology members described with -smp
> are supported by the target machine.
> 
> Cc: devel@lists.libvirt.org
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   docs/about/deprecated.rst | 14 +++++++++
>   hw/core/machine-smp.c     | 63 +++++++++++++++++++++++++++++----------
>   2 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 872974640252..2e782e83e952 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -47,6 +47,20 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
>   However, short-form booleans are deprecated and full explicit ``arg_name=on``
>   form is preferred.
>   
> +``-smp`` (Unsopported "parameter=1" SMP configurations) (since 9.0)

s/Unsopported/Unsupported/

> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Specified CPU topology parameters must be supported by the machine.
> +
> +In the SMP configuration, users should provide the CPU topology parameters that
> +are supported by the target machine.
> +
> +However, historically it was allowed for users to specify the unsupported
> +topology parameter as "1", which is meaningless. So support for this kind of
> +configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
> +marked depresated since 9.0, users have to ensure that all the topology members

s/depresated/deprecated/

> +described with -smp are supported by the target machine.
> +
>   QEMU Machine Protocol (QMP) commands
>   ------------------------------------
>   
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 96533886b14e..50a5a40dbc3d 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -112,30 +112,61 @@ void machine_parse_smp_config(MachineState *ms,
>   
>       /*
>        * If not supported by the machine, a topology parameter must be
> -     * omitted or specified equal to 1.
> +     * omitted.
>        */
> -    if (!mc->smp_props.dies_supported && dies > 1) {
> -        error_setg(errp, "dies not supported by this machine's CPU topology");
> -        return;
> -    }
> -    if (!mc->smp_props.clusters_supported && clusters > 1) {
> -        error_setg(errp, "clusters not supported by this machine's CPU topology");
> -        return;
> +    if (!mc->smp_props.clusters_supported && config->has_clusters) {
> +        if (config->clusters > 1) {
> +            error_setg(errp, "clusters not supported by this "
> +                       "machine's CPU topology");
> +            return;
> +        } else {
> +            /* Here clusters only equals 1 since we've checked zero case. */
> +            warn_report("Deprecated CPU topology (considered invalid): "
> +                        "Unsupported clusters parameter mustn't be "
> +                        "specified as 1");
> +        }
>       }
> +    clusters = clusters > 0 ? clusters : 1;
>   
> +    if (!mc->smp_props.dies_supported && config->has_dies) {
> +        if (config->dies > 1) {
> +            error_setg(errp, "dies not supported by this "
> +                       "machine's CPU topology");
> +            return;
> +        } else {
> +            /* Here dies only equals 1 since we've checked zero case. */
> +            warn_report("Deprecated CPU topology (considered invalid): "
> +                        "Unsupported dies parameter mustn't be "
> +                        "specified as 1");
> +        }
> +    }
>       dies = dies > 0 ? dies : 1;
> -    clusters = clusters > 0 ? clusters : 1;
>   
> -    if (!mc->smp_props.books_supported && books > 1) {
> -        error_setg(errp, "books not supported by this machine's CPU topology");
> -        return;
> +    if (!mc->smp_props.books_supported && config->has_books) {
> +        if (config->books > 1) {
> +            error_setg(errp, "books not supported by this "
> +                       "machine's CPU topology");
> +            return;
> +        } else {
> +            /* Here books only equals 1 since we've checked zero case. */
> +            warn_report("Deprecated CPU topology (considered invalid): "
> +                        "Unsupported books parameter mustn't be "
> +                        "specified as 1");
> +        }
>       }
>       books = books > 0 ? books : 1;
>   
> -    if (!mc->smp_props.drawers_supported && drawers > 1) {
> -        error_setg(errp,
> -                   "drawers not supported by this machine's CPU topology");
> -        return;
> +    if (!mc->smp_props.drawers_supported && config->has_drawers) {
> +        if (config->drawers > 1) {
> +            error_setg(errp, "drawers not supported by this "
> +                       "machine's CPU topology");
> +            return;
> +        } else {
> +            /* Here drawers only equals 1 since we've checked zero case. */
> +            warn_report("Deprecated CPU topology (considered invalid): "
> +                        "Unsupported drawers parameter mustn't be "
> +                        "specified as 1");
> +        }
>       }
>       drawers = drawers > 0 ? drawers : 1;

Apart from the typos, patch looks fine. I recommend to run "checkpath.pl" 
with the --codespell parameter, that helps to avoid those.

  Thomas

Re: [PATCH 07/14] tests/unit/test-smp-parse: Bump max_cpus to 4096

[Zhao Liu]

Hi Thomas,

> >           /*
> > -         * config: -smp 512
> > +         * config: -smp 4096
> >            * The test machine should tweak the supported max CPUs to
> > -         * 511 (MAX_CPUS - 1) for testing.
> > +         * 4095 (MAX_CPUS - 1) for testing.
> >            */
> > -        .config = SMP_CONFIG_GENERIC(T, MAX_CPUS, F, 0, F, 0, F, 0, F, 0),
> > -        .expect_error = "Invalid SMP CPUs 512. The max CPUs supported "
> > -                        "by machine '" SMP_MACHINE_NAME "' is 511",
> > +        .config = SMP_CONFIG_GENERIC(T, 4096, F, 0, F, 0, F, 0, F, 0),
> > +        .expect_error = "Invalid SMP CPUs 4096. The max CPUs supported "
> 
> You could maybe use stringify(MAX_CPUS) in above line
> (but it won't work for the 4095 below, so it's maybe not worth the effort)

Yes, it's also because of corner cases like 4095 that I don't do such a
thorough cleanup here.
 
> > +                        "by machine '" SMP_MACHINE_NAME "' is 4095",
> >       },
> >   };
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks!

Zhao

Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported "parameter=1" SMP configurations

[Zhao Liu]

On Thu, Mar 07, 2024 at 07:22:10AM +0100, Thomas Huth wrote:
> Date: Thu, 7 Mar 2024 07:22:10 +0100
> From: Thomas Huth <thuth@redhat.com>
> Subject: Re: [PATCH 02/14] hw/core/machine-smp: Deprecate unsupported
>  "parameter=1" SMP configurations
> 
> On 06/03/2024 10.53, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Currentlt, it was allowed for users to specify the unsupported
> 
> s/Currentlt/Currently/
> 
> > topology parameter as "1". For example, x86 PC machine doesn't
> > support drawer/book/cluster topology levels, but user could specify
> > "-smp drawers=1,books=1,clusters=1".
> > 
> > This is meaningless and confusing, so that the support for this kind of
> > configurations is marked depresated since 9.0. And report warning
> 
> s/depresated/deprecated/
> 
> > message for such case like:
> > 
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >                      Unsupported clusters parameter mustn't be specified as 1
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >                      Unsupported books parameter mustn't be specified as 1
> > qemu-system-x86_64: warning: Deprecated CPU topology (considered invalid):
> >                      Unsupported drawers parameter mustn't be specified as 1
> > 
> > Users have to ensure that all the topology members described with -smp
> > are supported by the target machine.
> > 
> > Cc: devel@lists.libvirt.org
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   docs/about/deprecated.rst | 14 +++++++++
> >   hw/core/machine-smp.c     | 63 +++++++++++++++++++++++++++++----------
> >   2 files changed, 61 insertions(+), 16 deletions(-)
> > 
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 872974640252..2e782e83e952 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -47,6 +47,20 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``.
> >   However, short-form booleans are deprecated and full explicit ``arg_name=on``
> >   form is preferred.
> > +``-smp`` (Unsopported "parameter=1" SMP configurations) (since 9.0)
> 
> s/Unsopported/Unsupported/
> 
> > +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> > +
> > +Specified CPU topology parameters must be supported by the machine.
> > +
> > +In the SMP configuration, users should provide the CPU topology parameters that
> > +are supported by the target machine.
> > +
> > +However, historically it was allowed for users to specify the unsupported
> > +topology parameter as "1", which is meaningless. So support for this kind of
> > +configurations (e.g. -smp drawers=1,books=1,clusters=1 for x86 PC machine) is
> > +marked depresated since 9.0, users have to ensure that all the topology members
> 
> s/depresated/deprecated/
> 
> > +described with -smp are supported by the target machine.
> > +
> >   QEMU Machine Protocol (QMP) commands
> >   ------------------------------------
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 96533886b14e..50a5a40dbc3d 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -112,30 +112,61 @@ void machine_parse_smp_config(MachineState *ms,
> >       /*
> >        * If not supported by the machine, a topology parameter must be
> > -     * omitted or specified equal to 1.
> > +     * omitted.
> >        */
> > -    if (!mc->smp_props.dies_supported && dies > 1) {
> > -        error_setg(errp, "dies not supported by this machine's CPU topology");
> > -        return;
> > -    }
> > -    if (!mc->smp_props.clusters_supported && clusters > 1) {
> > -        error_setg(errp, "clusters not supported by this machine's CPU topology");
> > -        return;
> > +    if (!mc->smp_props.clusters_supported && config->has_clusters) {
> > +        if (config->clusters > 1) {
> > +            error_setg(errp, "clusters not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here clusters only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported clusters parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> >       }
> > +    clusters = clusters > 0 ? clusters : 1;
> > +    if (!mc->smp_props.dies_supported && config->has_dies) {
> > +        if (config->dies > 1) {
> > +            error_setg(errp, "dies not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here dies only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported dies parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> > +    }
> >       dies = dies > 0 ? dies : 1;
> > -    clusters = clusters > 0 ? clusters : 1;
> > -    if (!mc->smp_props.books_supported && books > 1) {
> > -        error_setg(errp, "books not supported by this machine's CPU topology");
> > -        return;
> > +    if (!mc->smp_props.books_supported && config->has_books) {
> > +        if (config->books > 1) {
> > +            error_setg(errp, "books not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here books only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported books parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> >       }
> >       books = books > 0 ? books : 1;
> > -    if (!mc->smp_props.drawers_supported && drawers > 1) {
> > -        error_setg(errp,
> > -                   "drawers not supported by this machine's CPU topology");
> > -        return;
> > +    if (!mc->smp_props.drawers_supported && config->has_drawers) {
> > +        if (config->drawers > 1) {
> > +            error_setg(errp, "drawers not supported by this "
> > +                       "machine's CPU topology");
> > +            return;
> > +        } else {
> > +            /* Here drawers only equals 1 since we've checked zero case. */
> > +            warn_report("Deprecated CPU topology (considered invalid): "
> > +                        "Unsupported drawers parameter mustn't be "
> > +                        "specified as 1");
> > +        }
> >       }
> >       drawers = drawers > 0 ? drawers : 1;
> 
> Apart from the typos, patch looks fine. I recommend to run "checkpath.pl"
> with the --codespell parameter, that helps to avoid those.
>

Oops...I didn't realize there were so many typos.

Maybe I'm relying too much on --codespell ;-), and these typos aren't in
the default dictionary used by --codespell so I didn't check them
before.

I'll refresh a new version (at Friday) for these typos.

Thanks,
Zhao

Re: [PATCH 00/14] Cleanup on SMP and its test

[Zhao Liu]

Hi Philippe,

> In a previous community call, Zhao asked us how his work will scale
> in the heterogeneous context.
> 
> My first idea is CPUs must belong to a cluster.

Thank you for considering this!

At present, cluster is a arch-specific topology level used by ARM.
So maybe we need call this abstraction as another name not "cluster"?

I guess the cluster you mentioned is the cluster device used in TCG,
right? I also tried to eliminate differences between cluster devices
and the cluster level in CPU topology [1].

My previous proposal introduced a abstract topology device [2]. And all
topology specific levels are derived from the underlying topology
device, including CPU.

I feel like this topology device abstraction seems close to your idea,
am I understanding it correctly? ;-)

> For machines without
> explicit cluster, we could always create the first one. Then -smp
> would become a sugar property of the first cluster. Next -smp could
> also be sugar property of the next cluster.

Could you please explain the above ideas more?

It feels we need to split -smp for each cluster. But I'm not sure if
sugar property means defining smp-like properties for each cluster.

Or is there a command line example? ;-)

[1]: https://lore.kernel.org/qemu-devel/20231130144203.2307629-23-zhao1.liu@linux.intel.com/
[2]: https://lore.kernel.org/qemu-devel/20231130144203.2307629-9-zhao1.liu@linux.intel.com/

Thanks,
Zhao

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Thomas Huth]

On 06/03/2024 10.53, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> SMPConfiguration initializes its int64_t members as 0 by default.

Can we always rely on that? ... or is this just by luck due to the current 
implementation? In the latter case, I'd maybe rather drop this patch again.

  Thomas


> Therefore, in machine_parse_smp_config(), initialize local topology
> variables with SMPConfiguration's members directly.
> 
> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/core/machine-smp.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 50a5a40dbc3d..3d9799aef039 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -82,15 +82,15 @@ void machine_parse_smp_config(MachineState *ms,
>                                 const SMPConfiguration *config, Error **errp)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(ms);
> -    unsigned cpus    = config->has_cpus ? config->cpus : 0;
> -    unsigned drawers = config->has_drawers ? config->drawers : 0;
> -    unsigned books   = config->has_books ? config->books : 0;
> -    unsigned sockets = config->has_sockets ? config->sockets : 0;
> -    unsigned dies    = config->has_dies ? config->dies : 0;
> -    unsigned clusters = config->has_clusters ? config->clusters : 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;
> +    unsigned cpus     = config->cpus;
> +    unsigned drawers  = config->drawers;
> +    unsigned books    = config->books;
> +    unsigned sockets  = config->sockets;
> +    unsigned dies     = config->dies;
> +    unsigned clusters = config->clusters;
> +    unsigned cores    = config->cores;
> +    unsigned threads  = config->threads;
> +    unsigned maxcpus  = config->maxcpus;
>   
>       /*
>        * Specified CPU topology parameters must be greater than zero,

Re: [PATCH 05/14] tests/unit/test-smp-parse: Drop the unsupported "dies=1" case

[Thomas Huth]

On 06/03/2024 10.53, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Unsupported "parameter=1" SMP configurations is marked as deprecated,
> so drop the related test case.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   tests/unit/test-smp-parse.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 24972666a74d..1874bea08609 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-smp-parse.c
> @@ -607,11 +607,6 @@ static void test_generic_valid(const void *opaque)
>           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);
>       }
>   
>       object_unref(obj);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Zhao Liu]

Hi Thomas,

On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> Date: Fri, 8 Mar 2024 14:20:45 +0100
> From: Thomas Huth <thuth@redhat.com>
> Subject: Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables'
>  initialization in machine_parse_smp_config()
> 
> On 06/03/2024 10.53, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > SMPConfiguration initializes its int64_t members as 0 by default.
> 
> Can we always rely on that? ... or is this just by luck due to the current
> implementation? In the latter case, I'd maybe rather drop this patch again.
>

Thanks for the correction, I revisited and referenced more similar use
cases, and indeed, only if the flag "has_*" is true, its corresponding
field should be considered reliable.

Keeping explicit checking on has_* and explicit initialization of these
topology variables makes the code more readable.

This patch is over-optimized and I would drop it.

Regards,
Zhao

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Prasad Pandit]

Hi,

On Fri, 8 Mar 2024 at 20:50, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> > Can we always rely on that? ... or is this just by luck due to the current
> > implementation? In the latter case, I'd maybe rather drop this patch again.
>
> Thanks for the correction, I revisited and referenced more similar use
> cases, and indeed, only if the flag "has_*" is true, its corresponding
> field should be considered reliable.

* Is this because 'SMPConfiguration config'  fields are not always
initialized with default values? Is that a bug? Having
'SMPConfiguration' fields initialised to known default values could
help to unify/simplify code which uses those fields.

> Keeping explicit checking on has_* and explicit initialization of these
> topology variables makes the code more readable.
>
> This patch is over-optimized and I would drop it.

* Could we then simplify it in the following if <expression>
===
      if ((config->has_cpus && config->cpus == 0) ||
          ... ||
          (config->has_maxcpus && config->maxcpus == 0))

could be

      if (!cpus || !drawers || ... || !maxcpus) { ... }
===

Thank you.
---
  - Prasad

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Zhao Liu]

Hi Prasad (and also welcome folks correct me),

> On Fri, 8 Mar 2024 at 20:50, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> > On Fri, Mar 08, 2024 at 02:20:45PM +0100, Thomas Huth wrote:
> > > Can we always rely on that? ... or is this just by luck due to the current
> > > implementation? In the latter case, I'd maybe rather drop this patch again.
> >
> > Thanks for the correction, I revisited and referenced more similar use
> > cases, and indeed, only if the flag "has_*" is true, its corresponding
> > field should be considered reliable.
> 
> * Is this because 'SMPConfiguration config'  fields are not always
> initialized with default values?

SMPConfiguration is created and set in machine_set_smp().

Firstly, it is created by g_autoptr(), and then it is initialized by
visit_type_SMPConfiguration().

The visit_type_SMPConfiguration() is generated by
gen_visit_object_members() in scripts/qapi/visit.py.

The g_autoptr() requires user to initialize:

  You must initialise the variable in some way — either by use of an
  initialiser or by ensuring that it is assigned to unconditionally
  before it goes out of scope.

This means if user doesn't initialize some field, the the value should
be considerred as unreliable. And I guess for int, the default
initialization value is the same as if we had declared a regular integer
variable. But anyway, fields that are not explicitly initialized should
not be accessed.

IIUC, in visit_type_SMPConfiguration() (let's have a look at
gen_visit_object_members()), there's no explicit initialization of
SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
gen_visit_object_members()). For int type, has_* means that the field is
set.

Therefore, we shouldn't rely on the uninitialized fields and should
check the has_*.

> Is that a bug?

It's not a bug, and it could be a simplification of the automatic code
generation.

> Having 'SMPConfiguration' fields initialised to known default values could
> help to unify/simplify code which uses those fields.

I realized that keeping the check for has_* here would help improve code
readability; after all, it's more of a pain to go back and check
scripts.

> > Keeping explicit checking on has_* and explicit initialization of these
> > topology variables makes the code more readable.
> >
> > This patch is over-optimized and I would drop it.
> 
> * Could we then simplify it in the following if <expression>
> ===
>       if ((config->has_cpus && config->cpus == 0) ||
>           ... ||
>           (config->has_maxcpus && config->maxcpus == 0))
> 
> could be
> 
>       if (!cpus || !drawers || ... || !maxcpus) { ... }
> ===
>

This doesn't work since the above check is used to identify if user sets
parameters as 0 explicitly, which needs to check both has_* and the
specific field value.

(Communicating with you also helped me to understand the QAPI related
parts.)

Thanks,
Zhao

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Prasad Pandit]

Hello Zhao,

> (Communicating with you also helped me to understand the QAPI related parts.)

* I'm also visiting QAPI code parts for the first time. Thanks to you. :)

On Mon, 11 Mar 2024 at 10:36, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> SMPConfiguration is created and set in machine_set_smp().
> Firstly, it is created by g_autoptr(), and then it is initialized by
> visit_type_SMPConfiguration().
>
> The visit_type_SMPConfiguration() is generated by
> gen_visit_object_members() in scripts/qapi/visit.py.
>
> IIUC, in visit_type_SMPConfiguration() (let's have a look at
> gen_visit_object_members()), there's no explicit initialization of
> SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
> gen_visit_object_members()). For int type, has_* means that the field is
> set.
>

* Thank you for the above details, appreciate it. I added few
fprintf() calls to visit_type_SMPConfiguration() to see what user
values it receives
===
$ ./qemu-system-x86_64 -smp cores=2,maxcpus=2,cpus=1,sockets=2
visit_type_SMPConfiguration: name: smp
        has_cpus: 1:1
 has_drawvers: 0:0
      has_books: 0:0
  has_sockets: 1:2
        has_dies: 0:0
 has_clusters: 0:0
     has_cores: 1:2
  has_threads: 0:0
has_maxcpus: 1:2
qemu-system-x86_64: Invalid CPU topology: product of the hierarchy
must match maxcpus: sockets (2) * dies (1) * cores (2) * threads (0)
!= maxcpus (2)
===
* As seen above, undefined -smp fields (both has_xxxx and xxxx) are
set to zero(0).

===
main
 qemu_init
  qemu_apply_machine_options
   object_set_properties_from_keyval
    object_set_properties_from_qdict
     object_property_set
      machine_set_smp
       visit_type_SMPConfiguration
        visit_start_struct
(gdb) p v->start_struct
$4 = ... 0x5555570248e4 <qobject_input_start_struct>
(gdb)
(gdb)
 qobject_input_start_struct
   if (obj) {
        *obj = g_malloc0(size);
    }
===
* Stepping through function calls in gdb(1) revealed above call
sequence which leads to  'SMPConfiguration **'  objects allocation by
g_malloc0.

> This means if user doesn't initialize some field, the the value should
> be considerred as unreliable. And I guess for int, the default
> initialization value is the same as if we had declared a regular integer
> variable. But anyway, fields that are not explicitly initialized should
> not be accessed.

* g_malloc0() allocating 'SMPConfiguration **' object above assures us
that undefined -smp fields shall always be zero(0).

* 'obj->has_xxxx' field is set only if the user has supplied the
variable value, otherwise it remains set to zero(0).
   visit_type_SMPConfiguration_members
     ->visit_optional
       ->v->optional
        -> qobject_input_optional

* Overall, I think there is scope to simplify the
'machine_parse_smp_config()' function by using SMPConfiguration
field(s) ones.

Thank you.
---
  - Prasad

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Zhao Liu]

On Wed, Mar 13, 2024 at 04:22:30PM +0530, Prasad Pandit wrote:
> Date: Wed, 13 Mar 2024 16:22:30 +0530
> From: Prasad Pandit <ppandit@redhat.com>
> Subject: Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables'
>  initialization in machine_parse_smp_config()
> 
> Hello Zhao,
> 
> > (Communicating with you also helped me to understand the QAPI related parts.)
> 
> * I'm also visiting QAPI code parts for the first time. Thanks to you. :)
> 
> On Mon, 11 Mar 2024 at 10:36, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> > SMPConfiguration is created and set in machine_set_smp().
> > Firstly, it is created by g_autoptr(), and then it is initialized by
> > visit_type_SMPConfiguration().
> >
> > The visit_type_SMPConfiguration() is generated by
> > gen_visit_object_members() in scripts/qapi/visit.py.
> >
> > IIUC, in visit_type_SMPConfiguration() (let's have a look at
> > gen_visit_object_members()), there's no explicit initialization of
> > SMPConfiguration() (i.e., the parameter named "%(c_name)s *obj" in
> > gen_visit_object_members()). For int type, has_* means that the field is
> > set.
> >
> 
> * Thank you for the above details, appreciate it. I added few
> fprintf() calls to visit_type_SMPConfiguration() to see what user
> values it receives
> ===
> $ ./qemu-system-x86_64 -smp cores=2,maxcpus=2,cpus=1,sockets=2
> visit_type_SMPConfiguration: name: smp
>         has_cpus: 1:1
>  has_drawvers: 0:0
>       has_books: 0:0
>   has_sockets: 1:2
>         has_dies: 0:0
>  has_clusters: 0:0
>      has_cores: 1:2
>   has_threads: 0:0
> has_maxcpus: 1:2
> qemu-system-x86_64: Invalid CPU topology: product of the hierarchy
> must match maxcpus: sockets (2) * dies (1) * cores (2) * threads (0)
> != maxcpus (2)
> ===
> * As seen above, undefined -smp fields (both has_xxxx and xxxx) are
> set to zero(0).
> 
> ===
> main
>  qemu_init
>   qemu_apply_machine_options
>    object_set_properties_from_keyval
>     object_set_properties_from_qdict
>      object_property_set
>       machine_set_smp
>        visit_type_SMPConfiguration
>         visit_start_struct
> (gdb) p v->start_struct
> $4 = ... 0x5555570248e4 <qobject_input_start_struct>
> (gdb)
> (gdb)
>  qobject_input_start_struct
>    if (obj) {
>         *obj = g_malloc0(size);
>     }
> ===
> * Stepping through function calls in gdb(1) revealed above call
> sequence which leads to  'SMPConfiguration **'  objects allocation by
> g_malloc0.

Thanks! I misunderstood, it turns out that the initialization is done here.

> 
> > This means if user doesn't initialize some field, the the value should
> > be considerred as unreliable. And I guess for int, the default
> > initialization value is the same as if we had declared a regular integer
> > variable. But anyway, fields that are not explicitly initialized should
> > not be accessed.
> 
> * g_malloc0() allocating 'SMPConfiguration **' object above assures us
> that undefined -smp fields shall always be zero(0).
> 
> * 'obj->has_xxxx' field is set only if the user has supplied the
> variable value, otherwise it remains set to zero(0).
>    visit_type_SMPConfiguration_members
>      ->visit_optional
>        ->v->optional
>         -> qobject_input_optional

Yes, you're right!

> 
> * Overall, I think there is scope to simplify the
> 'machine_parse_smp_config()' function by using SMPConfiguration
> field(s) ones.

Indeed, as you say, these items are initialized to 0 by default.

I think, however, that the initialization is so far away from where the
smp is currently parsed that one can't easily confirm it (thanks for
your confirmation!).

From a code readability view, the fact that we're explicitly
initializing to 0 again here brings little overhead, but makes the code
more readable as well as easier to maintain. I think the small redundancy
here is worth it.

Also, in other use cases people always relies on fields marked by has_*,
and there is no (or less?) precedent for direct access to places where
has_* is not set. I understand that this is also a habit, i.e., fields
with a has_* of False by default are unreliable and avoid going directly
to them.

Regards,
Zhao

Re: [PATCH 03/14] hw/core/machine-smp: Simplify variables' initialization in machine_parse_smp_config()

[Prasad Pandit]

Hello,

On Mon, 18 Mar 2024 at 13:23, Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> Indeed, as you say, these items are initialized to 0 by default.
>
> I think, however, that the initialization is so far away from where the
> smp is currently parsed that one can't easily confirm it (thanks for
> your confirmation!).
>
> From a code readability view, the fact that we're explicitly
> initializing to 0 again here brings little overhead, but makes the code
> more readable as well as easier to maintain. I think the small redundancy
> here is worth it.
>
> Also, in other use cases people always relies on fields marked by has_*,
> and there is no (or less?) precedent for direct access to places where
> has_* is not set. I understand that this is also a habit, i.e., fields
> with a has_* of False by default are unreliable and avoid going directly
> to them.

* Ummn...okay. (I'm not fully convinced, but that's fine, I'm okay to
go with you on this.)

Thank you.
---
  - Prasad