[PATCH v4 0/4] hw/smbios: Cleanup topology related variables

[PATCH v4 0/4] hw/smbios: Cleanup topology related variables

[Zhao Liu]

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

Hi all,

This is my v4 patch series based on b111569da9f8 ("Merge tag 'ui-pull-
request' of https://gitlab.com/marcandre.lureau/qemu into staging").

This v4 is a quick refresh to address Igor's comment in v3 [1] about the
location of the new two helpers.

About the 2 newly added helpers, they can also be used in i386 code [2].
I will follow up to do the cleanup after another i386 cluster support
patch series.


Introduction
============

This patchset is split from my previous hybrid topology RFC [3].

This patchset adds 2 new helpers to wrap the threads/cores per sockets
calculation to avoid errors caused by other modules miss topology
changes.

Additionally, there are three places for topology-related cleanup in
smbios:

1. Fix the calculation of the number of sockets.

   Due to the misuse of the smp.cpus variable and the change in the
   meaning of smp.cores, the calculation of socket number in smbios is
   incorrect. This can be fixed by using smp.sockets directly.

2. Fix core count in type4.

   The meaning of smp.cores changed so that the calculation of cores
   per socket became wrong.

   v3 introduces the helper "machine_topo_get_cores_per_socket()" to
   wrap the calculation of cores per socket. This can help other modules
   avoid calculation error caused by missing topology changes.

3. Fix thread count in type4.

   I also found that the definition of thread count in type4 doesn't
   match the spec (smbios 3.0.0) and cleaned it up as well.

   Similar to core count, also use a new helper to fix this.

[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg05402.html
[2]: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg06071.html
[3]: https://mail.gnu.org/archive/html/qemu-devel/2023-02/msg03205.html


Regards,
Zhao
---
Zhao Liu (4):
  machine: Add helpers to get cores/threads per socket
  hw/smbios: Fix smbios_smp_sockets caculation
  hw/smbios: Fix thread count in type4
  hw/smbios: Fix core count in type4

 hw/core/machine-smp.c | 10 ++++++++++
 hw/smbios/smbios.c    | 16 ++++++++++------
 include/hw/boards.h   |  2 ++
 3 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v4 1/4] machine: Add helpers to get cores/threads per socket

[Zhao Liu]

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

The number of cores/threads per socket are needed for smbios, and are
also useful for other modules.

Provide the helpers to wrap the calculation of cores/threads per socket
so that we can avoid calculation errors caused by other modules miss
topology changes.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v4:
 * Put the declarations/definitions after machine_parse_smp_config()
   to avoid missing future topology related changes. (Igor)

v3:
 * The new patch to wrap the calculation of cores/threads per socket.
---
 hw/core/machine-smp.c | 10 ++++++++++
 include/hw/boards.h   |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 89fe0cda4275..0f4d9b6f7a9f 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -197,3 +197,13 @@ void machine_parse_smp_config(MachineState *ms,
         return;
     }
 }
+
+unsigned int machine_topo_get_cores_per_socket(const MachineState *ms)
+{
+    return ms->smp.cores * ms->smp.clusters * ms->smp.dies;
+}
+
+unsigned int machine_topo_get_threads_per_socket(const MachineState *ms)
+{
+    return ms->smp.threads * machine_topo_get_cores_per_socket(ms);
+}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 6b267c21ce7d..12d9e9d17ce9 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -35,6 +35,8 @@ void machine_set_cpu_numa_node(MachineState *machine,
                                Error **errp);
 void machine_parse_smp_config(MachineState *ms,
                               const SMPConfiguration *config, Error **errp);
+unsigned int machine_topo_get_cores_per_socket(const MachineState *ms);
+unsigned int machine_topo_get_threads_per_socket(const MachineState *ms);
 
 /**
  * machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
-- 
2.34.1

[PATCH v4 2/4] hw/smbios: Fix smbios_smp_sockets caculation

[Zhao Liu]

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

smp.sockets is the number of sockets which is configured by "-smp" (
otherwise, the default is 1). Trying to recalculate it here with another
rules leads to errors, such as:

1. 003f230e37d7 ("machine: Tweak the order of topology members in struct
   CpuTopology") changes the meaning of smp.cores but doesn't fix
   original smp.cores uses.

   With the introduction of cluster, now smp.cores means the number of
   cores in one cluster. So smp.cores * smp.threads just means the
   threads in a cluster not in a socket.

2. On the other hand, we shouldn't use smp.cpus here because it
   indicates the initial number of online CPUs at the boot time, and is
   not mathematically related to smp.sockets.

So stop reinventing the another wheel and use the topo values that
has been calculated.

Fixes: 003f230e37d7 ("machine: Tweak the order of topology members in struct CpuTopology")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v4:
 * None.

v3:
 * Reorganized changlog. (Igor)

v2:
 * None.
---
 hw/smbios/smbios.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index d2007e70fb05..d67415d44dd8 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -1088,8 +1088,7 @@ void smbios_get_tables(MachineState *ms,
         smbios_build_type_2_table();
         smbios_build_type_3_table();
 
-        smbios_smp_sockets = DIV_ROUND_UP(ms->smp.cpus,
-                                          ms->smp.cores * ms->smp.threads);
+        smbios_smp_sockets = ms->smp.sockets;
         assert(smbios_smp_sockets >= 1);
 
         for (i = 0; i < smbios_smp_sockets; i++) {
-- 
2.34.1

[PATCH v4 3/4] hw/smbios: Fix thread count in type4

[Zhao Liu]

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

From SMBIOS 3.0 specification, thread count field means:

Thread Count is the total number of threads detected by the BIOS for
this processor socket. It is a processor-wide count, not a
thread-per-core count. [1]

So here we should use threads per socket other than threads per core.

[1] SMBIOS 3.0.0, section 7.5.8, Processor Information - Thread Count

Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v4:
 * None.

v3:
 * Use the wrapped hepler to get threads per socket.

v2:
 * Rename cpus_per_socket to threads_per_socket.
 * Add the comment about smp.max_cpus. Thread count and core count will
   be calculated in 2 ways and will add a sanity check to ensure we
   don't miss any topology level.
---
 hw/smbios/smbios.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index d67415d44dd8..3aae9328c014 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -713,6 +713,7 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
 {
     char sock_str[128];
     size_t tbl_len = SMBIOS_TYPE_4_LEN_V28;
+    unsigned threads_per_socket;
 
     if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
         tbl_len = SMBIOS_TYPE_4_LEN_V30;
@@ -747,17 +748,19 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
     SMBIOS_TABLE_SET_STR(4, asset_tag_number_str, type4.asset);
     SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
 
+    threads_per_socket = machine_topo_get_threads_per_socket(ms);
+
     t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
     t->core_enabled = t->core_count;
 
-    t->thread_count = (ms->smp.threads > 255) ? 0xFF : ms->smp.threads;
+    t->thread_count = (threads_per_socket > 255) ? 0xFF : threads_per_socket;
 
     t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
     t->processor_family2 = cpu_to_le16(0x01); /* Other */
 
     if (tbl_len == SMBIOS_TYPE_4_LEN_V30) {
         t->core_count2 = t->core_enabled2 = cpu_to_le16(ms->smp.cores);
-        t->thread_count2 = cpu_to_le16(ms->smp.threads);
+        t->thread_count2 = cpu_to_le16(threads_per_socket);
     }
 
     SMBIOS_BUILD_TABLE_POST;
-- 
2.34.1

[PATCH v4 4/4] hw/smbios: Fix core count in type4

[Zhao Liu]

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

From SMBIOS 3.0 specification, core count field means:

Core Count is the number of cores detected by the BIOS for this
processor socket. [1]

Before 003f230e37d7 ("machine: Tweak the order of topology members in
struct CpuTopology"), MachineState.smp.cores means "the number of cores
in one package", and it's correct to use smp.cores for core count.

But 003f230e37d7 changes the smp.cores' meaning to "the number of cores
in one die" and doesn't change the original smp.cores' use in smbios as
well, which makes core count in type4 go wrong.

Fix this issue with the correct "cores per socket" caculation.

[1] SMBIOS 3.0.0, section 7.5.6, Processor Information - Core Count

Fixes: 003f230e37d7 ("machine: Tweak the order of topology members in struct CpuTopology")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v4:
 * None.

v3:
 * Use the wrapped helper to get cores per socket.

v2:
 * Calculate cores_per_socket in a different way from
   threads_per_socket.
 * Add the sanity check to ensure consistency of results between these 2
   ways. This can help not miss any future change of cpu topology.
---
 hw/smbios/smbios.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 3aae9328c014..10cd22f610ef 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -714,6 +714,7 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
     char sock_str[128];
     size_t tbl_len = SMBIOS_TYPE_4_LEN_V28;
     unsigned threads_per_socket;
+    unsigned cores_per_socket;
 
     if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
         tbl_len = SMBIOS_TYPE_4_LEN_V30;
@@ -749,8 +750,9 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
     SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
 
     threads_per_socket = machine_topo_get_threads_per_socket(ms);
+    cores_per_socket = machine_topo_get_cores_per_socket(ms);
 
-    t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
+    t->core_count = (cores_per_socket > 255) ? 0xFF : cores_per_socket;
     t->core_enabled = t->core_count;
 
     t->thread_count = (threads_per_socket > 255) ? 0xFF : threads_per_socket;
@@ -759,7 +761,7 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
     t->processor_family2 = cpu_to_le16(0x01); /* Other */
 
     if (tbl_len == SMBIOS_TYPE_4_LEN_V30) {
-        t->core_count2 = t->core_enabled2 = cpu_to_le16(ms->smp.cores);
+        t->core_count2 = t->core_enabled2 = cpu_to_le16(cores_per_socket);
         t->thread_count2 = cpu_to_le16(threads_per_socket);
     }
 
-- 
2.34.1

Re: [PATCH v4 0/4] hw/smbios: Cleanup topology related variables

[Igor Mammedov]

On Wed, 28 Jun 2023 21:54:33 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Hi all,
> 
> This is my v4 patch series based on b111569da9f8 ("Merge tag 'ui-pull-
> request' of https://gitlab.com/marcandre.lureau/qemu into staging").
> 
> This v4 is a quick refresh to address Igor's comment in v3 [1] about the
> location of the new two helpers.
> 
> About the 2 newly added helpers, they can also be used in i386 code [2].
> I will follow up to do the cleanup after another i386 cluster support
> patch series.
> 
> 
> Introduction
> ============
> 
> This patchset is split from my previous hybrid topology RFC [3].
> 
> This patchset adds 2 new helpers to wrap the threads/cores per sockets
> calculation to avoid errors caused by other modules miss topology
> changes.
> 
> Additionally, there are three places for topology-related cleanup in
> smbios:
> 
> 1. Fix the calculation of the number of sockets.
> 
>    Due to the misuse of the smp.cpus variable and the change in the
>    meaning of smp.cores, the calculation of socket number in smbios is
>    incorrect. This can be fixed by using smp.sockets directly.
> 
> 2. Fix core count in type4.
> 
>    The meaning of smp.cores changed so that the calculation of cores
>    per socket became wrong.
> 
>    v3 introduces the helper "machine_topo_get_cores_per_socket()" to
>    wrap the calculation of cores per socket. This can help other modules
>    avoid calculation error caused by missing topology changes.
> 
> 3. Fix thread count in type4.
> 
>    I also found that the definition of thread count in type4 doesn't
>    match the spec (smbios 3.0.0) and cleaned it up as well.
> 
>    Similar to core count, also use a new helper to fix this.
> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg05402.html
> [2]: https://lists.gnu.org/archive/html/qemu-devel/2023-06/msg06071.html
> [3]: https://mail.gnu.org/archive/html/qemu-devel/2023-02/msg03205.html
>

For series:

Acked-by: Igor Mammedov <imammedo@redhat.com>

> 
> Regards,
> Zhao
> ---
> Zhao Liu (4):
>   machine: Add helpers to get cores/threads per socket
>   hw/smbios: Fix smbios_smp_sockets caculation
>   hw/smbios: Fix thread count in type4
>   hw/smbios: Fix core count in type4
> 
>  hw/core/machine-smp.c | 10 ++++++++++
>  hw/smbios/smbios.c    | 16 ++++++++++------
>  include/hw/boards.h   |  2 ++
>  3 files changed, 22 insertions(+), 6 deletions(-)
>