Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31863 )
Change subject: src/arch: An upgrade of SMBIOS to latest version 3.2 ......................................................................
Patch Set 4:
(7 comments)
If you are going to abandon the parent change, you'd mean to squash them, right? Maybe it would make sense to move the CPUID additions into its own change, though, e.g. one change that adds the 3.2 fields with current values, and one that enhances the core/thread_count detection.
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@551 PS4, Line 551: u16 __weak smbios_processor_core_count(void) Generally, non-empty weak implementations are discouraged. OTOH, this has no side-effects, so I would accept it.
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@556 PS4, Line 556: while (1) { please, use a for loop
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@558 PS4, Line 558: leaf_b = cpuid_ext(0xb, ecx); Should we check if leaf 0xb is implemented and return 1 if not?
Are there processors that have leaf 0x1 implemented but not 0xb? would it make sense to fall back to the old behavior in this case?
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@569 PS4, Line 569: u16 __weak smbios_processor_thread_count(void) This is basically the same function as the one above. How about adding a `level_type` parameter instead of repeating the whole implementation?
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@618 PS4, Line 618: & 0x00ff Nit, explicit masking isn't necessary.
https://review.coreboot.org/#/c/31863/4/src/arch/x86/smbios.c@619 PS4, Line 619: t->core_count2 = 0x00ff; AFAIR, the spec says this field always contains the correct count if present.
https://review.coreboot.org/#/c/31863/4/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31863/4/src/include/smbios.h@429 PS4, Line 429: log_type_descriptor descriptor[256]; any plans to use it?