Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 5:
Patch Set 5:
Sorry for the revert. Please, split out the functional changes (e.g. the cpuid stuff around core/thread count) into their own patch (it's not related to the SMBIOS update, is it?). This change does a lot of things, and I haven't spotted anything yet that is *required* for SMBIOS 3.2.
Nico, From the 3.2 spec, core counts and thread counts don't seem to be optional. The newly added procedure is populating them to comply to one of your previous comment: " please don't add fields without giving them a reasonable value"
Well, core/thread count are not added by in this change, so my comment doesn't apply to them. I was mostly refer- ring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion.
Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit.
I can remove it from this patch if you require it and set the values to 0. Because the count generated by the code before this upgrade was not accurate, at least not on the GKL Sparky platform.
I know the current code doesn't provide accurate values in all cases. But in some cases it might. So why would you want to set it to 0 in all cases?
Also, we are talking about an extension of existing fields. With less than 256 cores, core_count2 must have the same value as core_count. Whether or not the value of the latter is accurate is an orthogonal issue.
Since the comment don't apply to the procedure then I am still confused.
|> Still, you could fix the cpuid code first, then add SMBIOS 3.2 stuff. I don't see why it would have to be part of the same commit. Removing the new core/tread procedure here and keeping everything else, along with the old core count in this commit would achieve the same goal, wouldn't it? If that is your request please confirm and I will do just that.
|> I was mostly referring to the added fields that were left untouched (at 0) where 0 isn't a valid value. Sorry for the confusion. There are 3 added field which are set to 0, and 0 indeed is a valid value for each of them. Please see these lines from the SMBIOS spec: " A value of 0000h indicates the Memory Subsystem Controller Product ID is unknown" " A value of 0000h indicates the Memory Subsystem Controller Manufacturer ID is unknown" " A value of 0000h indicates the Module Product ID is unknown" The FSP code have not yet been upgraded to 3.2 to popolate those memory fields appropriately. Hence their "unknown" 0 values.