diff mbox series

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

Message ID 20230601092952.1114727-4-zhao1.liu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series hw/smbios: Cleanup topology related variables | expand

Commit Message

Zhao Liu June 1, 2023, 9:29 a.m. UTC 
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(-)

Comments

Igor Mammedov June 7, 2023, 2:51 p.m. UTC | #1 
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);
>      }
>
Zhao Liu June 8, 2023, 2:52 a.m. UTC | #2 
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);
> >      }
> >  
>
diff mbox series

Patch

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);
     }