Angel Pons 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 11: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/46068/11/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/46068/11/src/arch/x86/smbios.c@503 PS11, Line 503: /* Provide sane defaults even for CPU without CPUID */ : res.eax = res.edx = 0; : res.ebx = 0x10000; This isn't necessary anymore
https://review.coreboot.org/c/coreboot/+/46068/11/src/arch/x86/smbios.c@516 PS11, Line 516: ` nit: this is a backtick, but should be a single quote instead: '
https://review.coreboot.org/c/coreboot/+/46068/11/src/arch/x86/smbios.c@519 PS11, Line 519: } else {
else is not generally useful after a break or return
It would be good to handle this
https://review.coreboot.org/c/coreboot/+/46068/11/src/arch/x86/smbios.c@766 PS11, Line 766: t->max_cache_size *= number_of_caches; : t->installed_size *= number_of_caches; : t->max_cache_size2 *= number_of_caches; : t->installed_size2 *= number_of_caches; This will cause problems with the `SMBIOS_CACHE_SIZE_UNIT_64KB` flag set above. I'd instead multiply once in `smbios_write_type7_cache_parameters()`
https://review.coreboot.org/c/coreboot/+/46068/11/src/arch/x86/smbios.c@870 PS11, Line 870: cache_size How about multiplying by `get_number_of_caches()` here instead?