Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46068 )
Change subject: arch/x86/smbios: Populate SMBIOS type 7 with cache information ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46068/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46068/8//COMMIT_MSG@11 PS8, Line 11: L1 Instruction cache size, and multiply the cache size of L1 and L2 : by the number of cores
I refered to the section of cache information in Intel BWG.
Which BWG in particular? Intel seems to hide the Server ones, can you share a document number?
I was asking because the doesn't look like anything what I'd expect. I've reviewed it now, but it does weird things that I'm not sure if all my comments are correct.
https://review.coreboot.org/c/coreboot/+/46068/9/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/46068/9/src/arch/x86/smbios.c@507 PS9, Line 507: cpu_have_cpuid Do we really have to check this? Does coreboot still support CPUs that don't have it.
Much simpler would be a single check, then you don't have to repeat it later, e.g.
if (!cpu_have_cpuid()) return 1;
https://review.coreboot.org/c/coreboot/+/46068/9/src/arch/x86/smbios.c@513 PS9, Line 513: 1 AIUI, `1` gives the number of cores, `0` would give the number of threads, aka. logical cores.
https://review.coreboot.org/c/coreboot/+/46068/9/src/arch/x86/smbios.c@517 PS9, Line 517: } I don't understand why cpuid(0xb) is queried at all. Is it supposed to be more accurate?
https://review.coreboot.org/c/coreboot/+/46068/9/src/arch/x86/smbios.c@519 PS9, Line 519: level `level` is not the same as the sub-leaf here. For instance L1I and L1D can have separate sub-leafs but are on the same level.
https://review.coreboot.org/c/coreboot/+/46068/9/src/arch/x86/smbios.c@522 PS9, Line 522: if (max_logical_cpus_sharing_cache != max_logical_cpus_per_package) With the distinction of `number_of_logical_cpus` and `max_logical_cpus_per_ package`, this looks very odd. Did you maybe mean to use `number_of_logical_cpus` here?