HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31420
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/include/smbios.h 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/1
diff --git a/src/include/smbios.h b/src/include/smbios.h index 6498e64..2efd5f0 100644 --- a/src/include/smbios.h +++ b/src/include/smbios.h @@ -374,6 +374,9 @@ u8 thread_count; u16 processor_characteristics; u16 processor_family2; + u16 core_count2; + u16 core_enabled2; + u16 thread_count2; u8 eos[2]; } __packed;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31420/1/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31420/1/src/include/smbios.h@377 PS1, Line 377: u16 core_count2; Should be set to the same value as `core_count`.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31420/1/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31420/1/src/include/smbios.h@377 PS1, Line 377: u16 core_count2;
Should be set to the same value as `core_count`.
yes and no :)
By my understanding it is equal to cores count not to 'core_count'
core_count can go up to 255, so if we have more than 255, core_count will have the value 0xff and core_count2 will have the cores count
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31420/1/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31420/1/src/include/smbios.h@377 PS1, Line 377: u16 core_count2;
yes and no :) […]
Yes and no. You are correct for values > 255, I am correct for values <= 255. Spec says about `core_count2`:
If this field is present, it holds the core count for the processor socket.
I assume for forward compatibility: Software that knows about `core_count2`, doesn't have to check `core_count` if the former is present. So adding but not setting it, i.e. `core_count2 == 0`, is wrong.
Elyes HAOUAS has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 1:
(1 comment)
you are totally right.
Hello Richard Spiegel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31420
to look at the new patch set (#2).
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/2
Hello Richard Spiegel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31420
to look at the new patch set (#3).
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/3
Hello Richard Spiegel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31420
to look at the new patch set (#4).
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/4
Elyes HAOUAS has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31420/4/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/4/src/arch/x86/smbios.c@563 PS4, Line 563: t->processor_upgrade = 0x06; lol
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31420/2/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/2/src/arch/x86/smbios.c@564 PS2, Line 564: t->core_count2 = (res.ebx >> 16) & 0xffff; The field is still only 8 bits. See cpuid leaf 1 docu- mentation in SDM vol. 2.
Not for this change, but for the future: This field doesn't accurately represent the number of cores. It is how many different APIC (core) IDs the processor supports which may be more than the number of actual cores. One solution would be to call into platform specific code to get the actual core count (some- where in MP init it should be already known).
Hello Richard Spiegel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31420
to look at the new patch set (#5).
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/5
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 5:
(2 comments)
Btw. what about the other two new fields?
https://review.coreboot.org/#/c/31420/5/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/5/src/arch/x86/smbios.c@565 PS5, Line 565: if (t->core_count2 == 0) we just set it to 0 in line 553.
https://review.coreboot.org/#/c/31420/5/src/arch/x86/smbios.c@566 PS5, Line 566: t->core_count2 = (u16)(t->core_count); no cast needed
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 5:
(2 comments)
Btw. what about the other two new fields?
same for Family2: "100h to FFFDh, Processor Family has a value of FEh and Processor Family 2 has the enumerated value" Core Enabled 2: "For core enabled counts of 256 or greater, the Core Enabled field is set to FFh and the Core Enabled 2 field is set to the number of enabled cores."
Hello Richard Spiegel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31420
to look at the new patch set (#6).
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/include/smbios.h 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/6
Hello Richard Spiegel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31420
to look at the new patch set (#7).
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
SMBIOS: Update Processor Information structure to v3.2.0
Change-Id: I1861615eff904f13029b8d038ffcf3de2d2bfc2a Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/arch/x86/smbios.c M src/include/smbios.h 2 files changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/31420/7
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@565 PS7, Line 565: t->core_count2 = t->core_count; Shouldn't you add code that once the number of cores are truly know and found to be greater then 255,to update core_count to 0xff and core_count2 with the correct number? Actually, by the time that tables are written (one of the last things before loading OS, number of cores should already be known. I'm not sure where and if this number is stored, but if it's not yet being stored you should add a separate commit to store this number, and than use it here. OTOH, I'm not sure if currently there is any platform using coreboot that has more than 255 cores (usually a server), so maybe this can wait? Nico, any thoughts?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@565 PS7, Line 565: t->core_count2 = t->core_count;
Shouldn't you add code that once the number of cores are truly know and found to be greater then 255 […]
I am fine with any solution that is consistent with the SMBIOS rules and doesn't read completely unrelated CPUID information.
Being accurate about the number of cores is a different matter.
https://review.coreboot.org/#/c/31420/7/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31420/7/src/include/smbios.h@378 PS7, Line 378: u16 core_enabled2; : u16 thread_count2; If you add them, you have to fill them. If you don't want to fill them, don't add them.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31420/7/src/include/smbios.h@378 PS7, Line 378: u16 core_enabled2; : u16 thread_count2;
If you add them, you have to fill them. If you don't want to […]
Please git grep core_enabled. same for thread_ . By my understanding, all of them are set to zero here src/arch/x86/smbios.c at line #553.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/31420/7/src/include/smbios.h@378 PS7, Line 378: u16 core_enabled2; : u16 thread_count2;
Please git grep core_enabled. same for thread_ .
I definitely won't.
By my understanding, all of them are set to zero here src/arch/x86/smbios.c at line #553.
If some things are broken, that's no excuse to add more broken things.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@564 PS7, Line 564: (res.ebx >> 16) & 0xff; This is maximum number of logical processors per package, not the maximum number of cores.
Per "Intel ® Processor Identification and the CPUID Instruction" - Order Number: 241618-039
Note page #37 : "The BIOS will use this function to determine the maximum number of cores implemented in a specific physical processor package. To do this the BIOS must initially set the EAX register to 4 and the ECX register to 0 prior to executing the CPUID instruction. After executing the CPUID instruction, (EAX[31:26] + 1) contains the maximum number of cores."
I have no idea about AMD (and others) ...
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@564 PS7, Line 564: (res.ebx >> 16) & 0xff;
This is maximum number of logical processors per package, not the maximum number of cores. […]
Maximum number is not the same as actual number. The CPU vendor creates a single set of masks, and then probing what is actually working, burn some fuses to make a particular SKU. So you could have CPU ID saying maximum 8 cores, but only 4 are actually active...
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@564 PS7, Line 564: (res.ebx >> 16) & 0xff;
Maximum number is not the same as actual number. […]
by my understanding, that is "core_enabled".
we have - core_count: max number of cores - thread_count: total threads ( not per core) - core_enabled: number of enabled cores by the BIOS
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@564 PS7, Line 564: (res.ebx >> 16) & 0xff;
by my understanding, that is "core_enabled". […]
Elyes, there are two levels of disablement, what the silicon vendor can disable and what the firmware can disable. SMBios spec won't account for the former.
My interpretation:
* x – maximum number of cores that are physically present * n – maximum number of cores that silicon vendor allows us to use * e – number of active cores
What CPUID (leaf 1 as currently used, or 4 as you described*) reports might be `x`. SMBios `core_count` would be `n` and `core_enabled` `e`. Also, to make it more confusing, whenever Intel writes `cores` it could mean logical cores, in SMBios: threads :-/ Especially, when IDs are mentioned, because each logical core has its own ID.
*Both CPUID 1 EBX[23:16] and CPUID 4 EAX[31:26] have the same description in the current SDM "Maximum number of addressable IDs for processor cores in the physical package".
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
IIRC, the generic solution for Intel is to actually count the logical cores, that is: fire them all up, let them increment the same integer each by 1 and see what you get ;) everything else might be platform specific, so in any case we should ask the platform specific MP init code if we want the exact numbers.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Patch Set 7:
(1 comment)
c
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/31420/7/src/arch/x86/smbios.c@564 PS7, Line 564: (res.ebx >> 16) & 0xff;
Elyes, there are two levels of disablement, what the silicon vendor can […]
Thank by giving a better explanation than mine. It's exactly what I was trying to get Elyes attention to.
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31420 )
Change subject: SMBIOS: Update Processor Information structure to v3.2.0 ......................................................................
Abandoned
see https://review.coreboot.org/#/c/coreboot/+/31725/
-
Elyes HAOUAS (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Richard Spiegel (Code Review)