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

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

[Zhao Liu]

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

Hi all,

This is my v2 patch series based on c76e7652c786 ("Revert 'python/qmp/
protocol: add open_with_socket()'").

Compared with v1 [1], v2 uses the different ways to calculate
threads_per_socket and cores_per_socket, and add the sanity check to
ensure consistency of results between these 2 ways, which can help us
not miss any future change of cpu topology.


Introduction
------------

This patchset is split from my previous hybrid topology RFC [2] 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://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07228.html
[2]: https://mail.gnu.org/archive/html/qemu-devel/2023-02/msg03205.html


Regards,
Zhao
---
Changelog:

Since v1:
 * Rename cpus_per_socket to threads_per_socket.
 * 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.

---
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 | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 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

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

[Igor Mammedov]

On Thu,  1 Jun 2023 17:29:50 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> 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.
that's probably not relevant to the patch.


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


I'd rephrase commit message to something like this:
---
CPU topology is calculated by ..., and trying to recalculate it here
with another rules leads to an error, such as 

 ... example follows ..

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

> 
> 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++) {

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

[Zhao Liu]

On Wed, Jun 07, 2023 at 04:35:03PM +0200, Igor Mammedov wrote:
> Date: Wed, 7 Jun 2023 16:35:03 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH v2 1/3] hw/smbios: Fix smbios_smp_sockets caculation
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)

Hi Igor,

> 
> On Thu,  1 Jun 2023 17:29:50 +0800
> Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> 
> > 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.
> that's probably not relevant to the patch.
> 

For the 2nd point, I mean the original calculation should use max_cpus
other than cpus to calculate sockets:

- smbios_smp_sockets = DIV_ROUND_UP(ms->smp.cpus,
+ smbios_smp_sockets = DIV_ROUND_UP(ms->smp.max_cpus,
                                    ms->smp.cores * ms->smp.threads);


But since we already have smp.sockets, we can use it directly.

> 
> > 
> > Since the number of sockets has already been recorded in smp structure,
> > use smp.sockets directly.
> 
> 
> I'd rephrase commit message to something like this:
> ---
> CPU topology is calculated by ..., and trying to recalculate it here
> with another rules leads to an error, such as 
> 
>  ... example follows ..
> 
> So stop reinventing the another wheel and use topo values that ... has calculated. 

Looks good for me. Thanks!

Regards,
Zhao

> 
> > 
> > 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++) {
> 

[PATCH v2 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>
---
Changes since v1:
 * 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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index d67415d44dd8..faf82d4ae646 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,20 @@ 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);
 
+    /* smp.max_cpus is the total number of threads for the system. */
+    threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;
+
     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

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

[Igor Mammedov]

On Thu,  1 Jun 2023 17:29:51 +0800
Zhao Liu <zhao1.liu@linux.intel.com> 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
> 
> Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Changes since v1:
>  * 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 | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index d67415d44dd8..faf82d4ae646 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,20 @@ 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);
>  
> +    /* smp.max_cpus is the total number of threads for the system. */
> +    threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;

what I dislike here is introducing topo calculations with its own assumptions
in random places.

I'd suggest to add threads_per_socket (even if it's just a helper field) into
topo structure and calculate it with the rest on topology.
And then use result here.

> +
>      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;

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

[Zhao Liu]

On Wed, Jun 07, 2023 at 04:49:34PM +0200, Igor Mammedov wrote:
> Date: Wed, 7 Jun 2023 16:49:34 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH v2 2/3] hw/smbios: Fix thread count in type4
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> 
> On Thu,  1 Jun 2023 17:29:51 +0800
> Zhao Liu <zhao1.liu@linux.intel.com> 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
> > 
> > Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > Changes since v1:
> >  * 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 | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index d67415d44dd8..faf82d4ae646 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,20 @@ 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);
> >  
> > +    /* smp.max_cpus is the total number of threads for the system. */
> > +    threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> 
> what I dislike here is introducing topo calculations with its own assumptions
> in random places.
> 
> I'd suggest to add threads_per_socket (even if it's just a helper field) into
> topo structure and calculate it with the rest on topology.
> And then use result here.

Thanks, I will try this way.

Zhao

> 
> > +
> >      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;
> 

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

[Igor Mammedov]

On Thu, 8 Jun 2023 10:51:05 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> On Wed, Jun 07, 2023 at 04:49:34PM +0200, Igor Mammedov wrote:
> > Date: Wed, 7 Jun 2023 16:49:34 +0200
> > From: Igor Mammedov <imammedo@redhat.com>
> > Subject: Re: [PATCH v2 2/3] hw/smbios: Fix thread count in type4
> > X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> > 
> > On Thu,  1 Jun 2023 17:29:51 +0800
> > Zhao Liu <zhao1.liu@linux.intel.com> 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
> > > 
> > > Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > ---
> > > Changes since v1:
> > >  * 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 | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > > index d67415d44dd8..faf82d4ae646 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,20 @@ 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);
> > >  
> > > +    /* smp.max_cpus is the total number of threads for the system. */
> > > +    threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;  
> > 
> > what I dislike here is introducing topo calculations with its own assumptions
> > in random places.
> > 
> > I'd suggest to add threads_per_socket (even if it's just a helper field) into
> > topo structure and calculate it with the rest on topology.
> > And then use result here.  
> 
> Thanks, I will try this way.

maybe instead a field, a helper function located close to topo code
would be better/more acceptable

> 
> Zhao
> 
> >   
> > > +
> > >      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;  
> >   
> 

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

[Michael Tokarev]

01.06.2023 12:29, 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
> 
> Fixes: c97294ec1b9e ("SMBIOS: Build aggregate smbios tables and entry point")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

Hi!

This, and other two patches in this area, smells like a -stable material.
Is it not?

196ea60a73 hw/smbios: Fix core count in type4
7298fd7de5 hw/smbios: Fix thread count in type4
d79a284a44 hw/smbios: Fix smbios_smp_sockets caculation

Thanks,

/mjt

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

[Michael Tokarev]

05.08.2023 08:58, Michael Tokarev wrote:

> 196ea60a73 hw/smbios: Fix core count in type4
> 7298fd7de5 hw/smbios: Fix thread count in type4
> d79a284a44 hw/smbios: Fix smbios_smp_sockets caculation

plus this one:

a1d027be95 machine: Add helpers to get cores/threads per socket

/mjt

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

[Igor Mammedov]

On Sat, 5 Aug 2023 09:00:41 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 05.08.2023 08:58, Michael Tokarev wrote:
> 
> > 196ea60a73 hw/smbios: Fix core count in type4
> > 7298fd7de5 hw/smbios: Fix thread count in type4
> > d79a284a44 hw/smbios: Fix smbios_smp_sockets caculation  
> 
> plus this one:
> 
> a1d027be95 machine: Add helpers to get cores/threads per socket
> 
> /mjt
> 

just to note: v4 was what got merged eventually
https://www.mail-archive.com/qemu-devel@nongnu.org/msg972625.html

as for stable, I guess dies/clusters aren't used in production
(based on lack of bug reports/complaints).
It's not worth of back-porting if it's too complex,
but if it's clean cherry-picks it might help folks who use
downstream (it's easier for downstream to pickup fixes from
stable branch) to test this code path.

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

[Michael Tokarev]

07.08.2023 12:56, Igor Mammedov wrote:
> On Sat, 5 Aug 2023 09:00:41 +0300
> Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
>> 05.08.2023 08:58, Michael Tokarev wrote:
>>
>>> 196ea60a73 hw/smbios: Fix core count in type4
>>> 7298fd7de5 hw/smbios: Fix thread count in type4
>>> d79a284a44 hw/smbios: Fix smbios_smp_sockets caculation
>>
>> plus this one:
>>
>> a1d027be95 machine: Add helpers to get cores/threads per socket
> 
> just to note: v4 was what got merged eventually
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg972625.html

Yeah, I replied to the wrong version of the patch. Sure thing, whatever
is picked up for -stable gets picked up from the master branch, always,
not from any other source.  Above, I'm quoting commit-IDs from the master
branch.

> as for stable, I guess dies/clusters aren't used in production
> (based on lack of bug reports/complaints).

Quite often people try something and just give up if it doesn't work,
trying other ways or working around the issue one way or another.

> It's not worth of back-porting if it's too complex,
> but if it's clean cherry-picks it might help folks who use
> downstream (it's easier for downstream to pickup fixes from
> stable branch) to test this code path.

The whole thing - provided the preparational patch a1d027be95
"machine: Add helpers to get cores/threads per socket" is also
picked up - applies cleanly and in a stright-forward way to 8.0
and even to 7.2, and passes the usual qemu testsuite. Sure thing
since the issues weren't noticed before, the testsuite does not
cover this area.  It'd be nice to have some verifier to check if
the whole thing actually works after applying the patchset.

I'll pick this thing up for the next stable, thank you for the
clarification.

The whole -stable thing is exactly in order to centralize fixes.

Thanks,

/mjt

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

[Igor Mammedov]

On Mon, 7 Aug 2023 13:06:47 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 07.08.2023 12:56, Igor Mammedov wrote:
> > On Sat, 5 Aug 2023 09:00:41 +0300
> > Michael Tokarev <mjt@tls.msk.ru> wrote:
[...]
> The whole thing - provided the preparational patch a1d027be95
> "machine: Add helpers to get cores/threads per socket" is also
> picked up - applies cleanly and in a stright-forward way to 8.0
> and even to 7.2, and passes the usual qemu testsuite. Sure thing
> since the issues weren't noticed before, the testsuite does not
> cover this area.  It'd be nice to have some verifier to check if
> the whole thing actually works after applying the patchset.

Zhao Liu,
can you help us out with adding test cases to cover the code
you are touching?

[...]

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

[Zhao Liu]

Hi Igor,

On Mon, Aug 07, 2023 at 12:11:29PM +0200, Igor Mammedov wrote:
> Date: Mon, 7 Aug 2023 12:11:29 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH v2 2/3] hw/smbios: Fix thread count in type4
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> 
> On Mon, 7 Aug 2023 13:06:47 +0300
> Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> > 07.08.2023 12:56, Igor Mammedov wrote:
> > > On Sat, 5 Aug 2023 09:00:41 +0300
> > > Michael Tokarev <mjt@tls.msk.ru> wrote:
> [...]
> > The whole thing - provided the preparational patch a1d027be95
> > "machine: Add helpers to get cores/threads per socket" is also
> > picked up - applies cleanly and in a stright-forward way to 8.0
> > and even to 7.2, and passes the usual qemu testsuite. Sure thing
> > since the issues weren't noticed before, the testsuite does not
> > cover this area.  It'd be nice to have some verifier to check if
> > the whole thing actually works after applying the patchset.
> 
> Zhao Liu,
> can you help us out with adding test cases to cover the code
> you are touching?

Yes, sure.

Just double check, I should add these 2 test cases:
1. in "bios-tables-test.c" to test smbios type4 topology related things, and
2. also in "test-smp-parse.c" to test our new topology helpers.

Do I understand correctly?

-Zhao

> 
> [...]
> 

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

[Igor Mammedov]

On Mon, 7 Aug 2023 22:31:35 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> Hi Igor,
> 
> On Mon, Aug 07, 2023 at 12:11:29PM +0200, Igor Mammedov wrote:
> > Date: Mon, 7 Aug 2023 12:11:29 +0200
> > From: Igor Mammedov <imammedo@redhat.com>
> > Subject: Re: [PATCH v2 2/3] hw/smbios: Fix thread count in type4
> > X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
> > 
> > On Mon, 7 Aug 2023 13:06:47 +0300
> > Michael Tokarev <mjt@tls.msk.ru> wrote:
> >   
> > > 07.08.2023 12:56, Igor Mammedov wrote:  
> > > > On Sat, 5 Aug 2023 09:00:41 +0300
> > > > Michael Tokarev <mjt@tls.msk.ru> wrote:  
> > [...]  
> > > The whole thing - provided the preparational patch a1d027be95
> > > "machine: Add helpers to get cores/threads per socket" is also
> > > picked up - applies cleanly and in a stright-forward way to 8.0
> > > and even to 7.2, and passes the usual qemu testsuite. Sure thing
> > > since the issues weren't noticed before, the testsuite does not
> > > cover this area.  It'd be nice to have some verifier to check if
> > > the whole thing actually works after applying the patchset.  
> > 
> > Zhao Liu,
> > can you help us out with adding test cases to cover the code
> > you are touching?  
> 
> Yes, sure.
> 
> Just double check, I should add these 2 test cases:
> 1. in "bios-tables-test.c" to test smbios type4 topology related things, and
> 2. also in "test-smp-parse.c" to test our new topology helpers.
> 
> Do I understand correctly?

yep, I'd do both.
Also looking at test cases I don't see any test cases that
check topo end-to-end path (at least for x86). I mean
checking related CPUID values.
One possible place to it without writing testcase from scratch
could be test-x86-cpuid-compat.c.
Where I'd add test cases for some CPUID leaves at certain topo
configurations (values to check could be hardcoded magic numbers
as long as they are properly documented/reference specs) .

> 
> -Zhao
> 
> > 
> > [...]
> >   
> 

[PATCH v2 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>
---
Changes since v1:
 * 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 | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index faf82d4ae646..2b46a51dfcad 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;
@@ -750,8 +751,16 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
 
     /* smp.max_cpus is the total number of threads for the system. */
     threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;
+    cores_per_socket = ms->smp.cores * ms->smp.clusters * ms->smp.dies;
 
-    t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
+    /*
+     * Currently, max_cpus = threads * cores * clusters * dies * sockets.
+     * threads_per_socket and cores_per_socket are calculated in 2 ways so
+     * that this sanity check ensures we won't miss any topology level.
+     */
+    g_assert(cores_per_socket == (threads_per_socket / ms->smp.threads));
+
+    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;
@@ -760,7 +769,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 v2 3/3] hw/smbios: Fix core count in type4

[Igor Mammedov]

On Thu,  1 Jun 2023 17:29:52 +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.

see comment on 2/3 patch and do the same for cores.

> 
> [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>
> ---
> Changes since v1:
>  * 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 | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index faf82d4ae646..2b46a51dfcad 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;
> @@ -750,8 +751,16 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
>  
>      /* smp.max_cpus is the total number of threads for the system. */
>      threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> +    cores_per_socket = ms->smp.cores * ms->smp.clusters * ms->smp.dies;
>  
> -    t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
> +    /*
> +     * Currently, max_cpus = threads * cores * clusters * dies * sockets.
> +     * threads_per_socket and cores_per_socket are calculated in 2 ways so
> +     * that this sanity check ensures we won't miss any topology level.
> +     */
> +    g_assert(cores_per_socket == (threads_per_socket / ms->smp.threads));
> +
> +    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;
> @@ -760,7 +769,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);
>      }
>  

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

[Zhao Liu]

On Wed, Jun 07, 2023 at 04:51:07PM +0200, Igor Mammedov wrote:
> Date: Wed, 7 Jun 2023 16:51:07 +0200
> From: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [PATCH v2 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 Thu,  1 Jun 2023 17:29:52 +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.
> 
> see comment on 2/3 patch and do the same for cores.

Ok, thanks.

> 
> > 
> > [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>
> > ---
> > Changes since v1:
> >  * 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 | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index faf82d4ae646..2b46a51dfcad 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;
> > @@ -750,8 +751,16 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
> >  
> >      /* smp.max_cpus is the total number of threads for the system. */
> >      threads_per_socket = ms->smp.max_cpus / ms->smp.sockets;
> > +    cores_per_socket = ms->smp.cores * ms->smp.clusters * ms->smp.dies;
> >  
> > -    t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
> > +    /*
> > +     * Currently, max_cpus = threads * cores * clusters * dies * sockets.
> > +     * threads_per_socket and cores_per_socket are calculated in 2 ways so
> > +     * that this sanity check ensures we won't miss any topology level.
> > +     */
> > +    g_assert(cores_per_socket == (threads_per_socket / ms->smp.threads));
> > +
> > +    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;
> > @@ -760,7 +769,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);
> >      }
> >  
>