Hi Heinrich, Sorry for the late reply.
On Mon, Nov 17, 2025 at 12:27 PM Heinrich Schuchardt < [email protected]> wrote: > On 11/17/25 18:13, Raymond Mao wrote: > > Hi Heinrich, > > > > On Mon, Nov 17, 2025 at 11:43 AM Heinrich Schuchardt > > <[email protected] > > <mailto:[email protected]>> wrote: > > > > On 11/17/25 16:48, Raymond Mao wrote: > > > Hi Heinrich, > > > > > > On Sun, Nov 16, 2025 at 5:39 AM Heinrich Schuchardt > > > <[email protected] > > <mailto:[email protected]> > > > <mailto:[email protected] > > <mailto:[email protected]>>> wrote: > > > > > > In the QFW case the SMBIOS specification version is > > controlled by QEMU. > > > Field 'Thread Enable' is not available before SMBIOS v3.6. > > > > > > We should not print the core, core enabled, and thread counts > > twice. > > > Use the appropriate based on the SMBIOS version. > > > Use decimal output which is easier to read for humans. > > > > > > Signed-off-by: Heinrich Schuchardt > > > <[email protected] > > <mailto:[email protected]> > > > <mailto:[email protected] > > <mailto:[email protected]>>> > > > --- > > > cmd/smbios.c | 21 +++++++++++++++------ > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > > > diff --git a/cmd/smbios.c b/cmd/smbios.c > > > index 671c14e05b5..f9a352c107d 100644 > > > --- a/cmd/smbios.c > > > +++ b/cmd/smbios.c > > > @@ -262,6 +262,8 @@ static const struct str_lookup_table > > > md_tech_strings[] = { > > > { SMBIOS_MD_TECH_OPTANE, "Intel Optane > persistent > > > memory" }, > > > }; > > > > > > +static u16 smbios_version; > > > + > > > /** > > > * smbios_get_string() - get SMBIOS string from table > > > * > > > @@ -504,18 +506,23 @@ static void smbios_print_type4(struct > > > smbios_type4 *table) > > > smbios_print_str("Serial Number", table, table- > > >serial_number); > > > smbios_print_str("Asset Tag", table, > table->asset_tag); > > > smbios_print_str("Part Number", table, table- > > >part_number); > > > - printf("\tCore Count: 0x%02x\n", table->core_count); > > > - printf("\tCore Enabled: 0x%02x\n", > table->core_enabled); > > > - printf("\tThread Count: 0x%02x\n", > table->thread_count); > > > + if (smbios_version < 0x0300) { > > > + printf("\tCore Count: %d\n", > table->core_count); > > > + printf("\tCore Enabled: %d\n", table- > > >core_enabled); > > > + printf("\tThread Count: %d\n", table- > > >thread_count); > > > + } else { > > > + printf("\tCore Count: %d\n", > table->core_count2); > > > + printf("\tCore Enabled: %d\n", table- > > >core_enabled2); > > > + printf("\tThread Count: %d\n", table- > > >thread_count2); > > > + } > > > > > > > > > I think for ">= 0x0300", both parts are needed. > > > > > > Below is the description of " Core Count 2" in spec: https:// > > > www.dmtf.org/sites/default/files/standards/documents/ > > DSP0134_3.8.0.pdf <http://www.dmtf.org/sites/default/files/ > > standards/documents/DSP0134_3.8.0.pdf> > > > <https://www.dmtf.org/sites/default/files/standards/documents/ > > <https://www.dmtf.org/sites/default/files/standards/documents/> > > > DSP0134_3.8.0.pdf> > > > Number of Cores per processor socket. Supports core counts >255. > > If this > > > field is present, it holds the core count for the processor > > socket. Core > > > Count will also hold the core count, except for core counts that > > are 256 > > > or greater. In that case, Core Count shall be set to FFh and Core > > Count > > > 2 will hold the count. See 7.5.6. Legal values: 0000h = unknown > > > 0001h-00FFh = core counts 1 to 255. Matches Core Count value. > 0100h- > > > FFFEh = Core counts 256 to 65534, respectively. FFFFh = reserved. > > > > > > Similar for core_enabled2 and thread_count2, they are all > extensions > > > when values above 255. > > > > The descriptions of fields "Core Count 2", "Core Enabled 2", "Thread > > Count 2" all say: "If this field is present, it holds the ... for the > > processor socket." > > > > So there is no need to look into the old fields "Core Count", "Core > > Enabled", "Thread Count" for SMBIOS version >= 3.0 and test for 0xff. > > > > > > But it also says: " Core Count will also hold the core count, except for > > core counts that are 256 or greater. In that case, Core Count shall be > > set to FFh and Core Count 2 will hold the count." > > So the "old" fields might be with the same values as the "new" ones, or > > they might be "FFh". > > That is the reason I believe it is still valuable to print and show > > whether it is aligned to the spec. > > For values below 256 the values will be equal. Why do you want to print > redundant information? > > Actually, I have a better idea to use 'table->hdr.length' for version control instead of introducing the version explicitly. The 'table->hdr.length' is tied to the version and it is already being used for type 1. I sent a patch [1] to extend the usage of 'table->hdr.length' to all types we have and it covers all logics from 2.0 to 3.7+, and it looks neat. Please see if that matches your cases. [1] https://lore.kernel.org/u-boot/[email protected]/T/#u Regards, Raymond > Best regards > > Heinrich > > > > > But anyway, it is not really a big deal to print them or not in cmd - it > > is just for demo/debug purposes, what I wanted to highlight is the logic > > to exclude fields for old versions < 0x0300 should be needed in > lib/smbios. > > > > Regards, > > Raymond > > > > We should avoid printing duplicate information. > > > > Best regards > > > > Heinrich > > > > > > > > So, if for the consideration of the old versions of spec, the > change > > > should be: > > > ``` > > > - printf("\tCore Count 2: 0x%04x\n", table->core_count2); > > > - printf("\tCore Enabled 2: 0x%04x\n", > table->core_enabled2); > > > - printf("\tThread Count 2: 0x%04x\n", > table->thread_count2); > > > + if (smbios_version >= 0x0300) { > > > + printf("\tCore Count 2: %d\n", > table->core_count2); > > > + printf("\tCore Enabled 2: %d\n", table- > > >core_enabled2); > > > + printf("\tThread Count 2: %d\n", table- > > >thread_count2); > > > + } > > > ``` > > > Moreover, I think the same logic should be done when the table is > > being > > > constructed in lib/smbios.c, where the real content of tables is > > located. > > > > > > Regards, > > > Raymond > > > > > > printf("\tProcessor Characteristics: 0x%04x\n", > > > table->processor_characteristics); > > > smbios_print_lookup_str(processor_family_strings, > > > table->processor_family2, > > > > > ARRAY_SIZE(processor_family_strings), > > > "Processor Family 2"); > > > - printf("\tCore Count 2: 0x%04x\n", > table->core_count2); > > > - printf("\tCore Enabled 2: 0x%04x\n", table- > > >core_enabled2); > > > - printf("\tThread Count 2: 0x%04x\n", table- > > >thread_count2); > > > + if (smbios_version < 0x0306) > > > + return; > > > printf("\tThread Enabled: 0x%04x\n", table- > > >thread_enabled); > > > } > > > > > > @@ -719,6 +726,8 @@ static int do_smbios(struct cmd_tbl > > *cmdtp, int > > > flag, int argc, > > > struct smbios3_entry *entry3 = entry; > > > > > > table = (void *)(uintptr_t)entry3- > > > >struct_table_address; > > > + smbios_version = ((u16)entry3->major_ver << > 16) + > A typo of '<< 8' ? - smbios_version is a u16. > > > + (u16)entry3->minor_ver; > > > snprintf(version, sizeof(version), > "%d.%d.%d", > > > entry3->major_ver, > entry3->minor_ver, > > > entry3->doc_rev); > > > table = (void *)(uintptr_t)entry3- > > > >struct_table_address; > > > -- > > > 2.51.0 > > > > > > >
