| Message ID | 20230529164343.467793-3-zhao1.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/smbios: Cleanup topology related variables | expand |
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 > >
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 > > > > >
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;