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

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

[Zhao Liu]

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

Hi all,

This patchset is split from my previous hybrid topology RFC [1] for
easier review.

There are three places for topology-related cleanup:

1. Fix the use of smp.cores.

   The meaning of CPU topology members in MachineState.smp has changed
   since 003f230e37d7 ("machine: Tweak the order of topology members in
   struct CpuTopology"), but the use of smp.cores based on the original
   semantics (the number of cores in one package) has not been updated,
   so here is a cleanup.

2. Consolidate the use of MachineState.smp.

   The socket calculation can be simplified by directly using the
   MachineState.smp.sockets.

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.

[1]: https://mail.gnu.org/archive/html/qemu-devel/2023-02/msg03205.html


Regards,
Zhao

---
Zhao Liu (3):
  hw/smbios: Fix smbios_smp_sockets caculation
  hw/smbios: Fix thread count in type4
  hw/smbios: Fix core count in type4

 hw/smbios/smbios.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/3] hw/smbios: Fix smbios_smp_sockets caculation

[Zhao Liu]

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

Here're 2 mistakes:
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. And because of the introduction of cluster,
   now smp.cores means the number of cores in one cluster. So smp.cores
   * smp.threads just means the cpus in a cluster not in a socket.
2. smp.cpus means the number of initial online cpus, not the total
   number of cpus. For such topology calculation, smp.max_cpus
   should be considered.

Since the number of sockets has already been recorded in smp structure,
use smp.sockets directly.

Fixes: 003f230e37d7 ("machine: Tweak the order of topology members in struct CpuTopology")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 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 2/3] 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>
---
 hw/smbios/smbios.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index d67415d44dd8..f80a701cdfc1 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 cpus_per_socket = ms->smp.max_cpus / ms->smp.sockets;
 
     if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
         tbl_len = SMBIOS_TYPE_4_LEN_V30;
@@ -750,14 +751,14 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
     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 = (cpus_per_socket > 255) ? 0xFF : cpus_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(cpus_per_socket);
     }
 
     SMBIOS_BUILD_TABLE_POST;
-- 
2.34.1

Re: [PATCH 2/3] hw/smbios: Fix thread count in type4

[Ani Sinha]

On Tue, 30 May 2023, Zhao Liu wrote:

> 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

I see two patches sent out around this fix. The patch
[PATCH] hw/smbios: fix thead count field in type 4 table

looks correct to me. I NACK this patch.

>
> Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/smbios/smbios.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index d67415d44dd8..f80a701cdfc1 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 cpus_per_socket = ms->smp.max_cpus / ms->smp.sockets;
>

This is confusing. Looking at machine_parse_smp_config(), this is
essentially total number of threads per socket. Maybe a better naming is
threads_per_socket.

Regardless, this patch is confusing.

>      if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
>          tbl_len = SMBIOS_TYPE_4_LEN_V30;
> @@ -750,14 +751,14 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
>      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 = (cpus_per_socket > 255) ? 0xFF : cpus_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(cpus_per_socket);
>      }
>
>      SMBIOS_BUILD_TABLE_POST;
> --
> 2.34.1
>
>

Re: [PATCH 2/3] hw/smbios: Fix thread count in type4

[Zhao Liu]

Hi Ani,

Thanks for your review!

On Wed, May 31, 2023 at 01:28:57PM +0530, Ani Sinha wrote:
> Date: Wed, 31 May 2023 13:28:57 +0530 (IST)
> From: Ani Sinha <anisinha@redhat.com>
> Subject: Re: [PATCH 2/3] hw/smbios: Fix thread count in type4
> 
> 
> 
> On Tue, 30 May 2023, Zhao Liu wrote:
> 
> > 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
> 
> I see two patches sent out around this fix. The patch
> [PATCH] hw/smbios: fix thead count field in type 4 table
> 
> looks correct to me. I NACK this patch.

The deffierence between these 2 fixes is the understanding of "smp.cores".

Strictly speaking, smp.cores only represents the number of cores in one
cluster, not in one socket, so we shouldn't use "smp.cores * smp.threads"
to calculation the number of threads in the sockets (which I also fixed the
similar thing in another patch [1]).

[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07187.html

> 
> >
> > Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  hw/smbios/smbios.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index d67415d44dd8..f80a701cdfc1 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 cpus_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> >
> 
> This is confusing. Looking at machine_parse_smp_config(), this is
> essentially total number of threads per socket.

It's "the maximum number of logical processors on the machine" (I
referred the comment fo CpuTopology in include/hw/boards.h), which means
the total threads of the whole system (sockets * dies * clusters * cores
* threads).

> Maybe a better naming is threads_per_socket.

Thanks, I can send a v2 if the other two patches are OK.

> 
> Regardless, this patch is confusing.

The intent of my cleanup was to eliminate the confusion of these various
topological variables of the smp.

The third patch also tries to fix the incorrect usage of smp.cores.

Regards,
Zhao

> 
> >      if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
> >          tbl_len = SMBIOS_TYPE_4_LEN_V30;
> > @@ -750,14 +751,14 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
> >      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 = (cpus_per_socket > 255) ? 0xFF : cpus_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(cpus_per_socket);
> >      }
> >
> >      SMBIOS_BUILD_TABLE_POST;
> > --
> > 2.34.1
> >
> >
> 

[PATCH 3/3] 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>
---
 hw/smbios/smbios.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index f80a701cdfc1..32e26bffa2df 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 cpus_per_socket = ms->smp.max_cpus / ms->smp.sockets;
+    unsigned cores_per_socket = cpus_per_socket / ms->smp.threads;
 
     if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
         tbl_len = SMBIOS_TYPE_4_LEN_V30;
@@ -748,7 +749,7 @@ 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);
 
-    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 = (cpus_per_socket > 255) ? 0xFF : cpus_per_socket;
@@ -757,7 +758,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(cpus_per_socket);
     }
 
-- 
2.34.1

Re: [PATCH 3/3] hw/smbios: Fix core count in type4

[Igor Mammedov]

On Tue, 30 May 2023 00:43:43 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> 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>
> ---
>  hw/smbios/smbios.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index f80a701cdfc1..32e26bffa2df 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 cpus_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> +    unsigned cores_per_socket = cpus_per_socket / ms->smp.threads;
wouldn't be
   smp.dies * smp.clusters * smp.cores
cleaner and more self-describing

and then you can add sanity check 
      g_assert(cores_per_socket != (cpus_per_socket / ms->smp.threads))
so we won't miss change to CpuTopology in the future?
 
>  
>      if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
>          tbl_len = SMBIOS_TYPE_4_LEN_V30;
> @@ -748,7 +749,7 @@ 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);
>  
> -    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 = (cpus_per_socket > 255) ? 0xFF : cpus_per_socket;
> @@ -757,7 +758,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(cpus_per_socket);
>      }
>  

Re: [PATCH 3/3] hw/smbios: Fix core count in type4

[Zhao Liu]

On Wed, May 31, 2023 at 05:46:48PM +0200, Igor Mammedov wrote:
> Date: Wed, 31 May 2023 17:46:48 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH 3/3] hw/smbios: Fix core count in type4
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> 
> On Tue, 30 May 2023 00:43:43 +0800
> Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> 
> > 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>
> > ---
> >  hw/smbios/smbios.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index f80a701cdfc1..32e26bffa2df 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 cpus_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> > +    unsigned cores_per_socket = cpus_per_socket / ms->smp.threads;
> wouldn't be
>    smp.dies * smp.clusters * smp.cores
> cleaner and more self-describing
> 
> and then you can add sanity check 
>       g_assert(cores_per_socket != (cpus_per_socket / ms->smp.threads))
> so we won't miss change to CpuTopology in the future?

Thanks! Will use this way.

Regards,
Zhao

>  
> >  
> >      if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
> >          tbl_len = SMBIOS_TYPE_4_LEN_V30;
> > @@ -748,7 +749,7 @@ 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);
> >  
> > -    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 = (cpus_per_socket > 255) ? 0xFF : cpus_per_socket;
> > @@ -757,7 +758,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(cpus_per_socket);
> >      }
> >  
>