Andrey Petrov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
arch/x86: Correctly determine number of enabled cores
Do not use CONFIG_MAX_CPUs in calculation, just assume we are enabling all the cores.
TEST=tested with dmidecode
Change-Id: Id0935f48e73c037bb7c0e1cf36f94d98a40a499c Signed-off-by: Andrey Petrov anpetrov@fb.com --- M src/arch/x86/smbios.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/36662/1
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 261888f..789f8e1 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -658,8 +658,8 @@ t->processor_family = (res.eax > 0) ? 0x0c : 0x6; t->processor_type = 3; /* System Processor */ t->core_count = (res.ebx >> 16) & 0xff; - /* Assume we enable all the cores always, capped only by MAX_CPUS */ - t->core_enabled = MAX(t->core_count, CONFIG_MAX_CPUS); + /* Assume we enable all the cores always */ + t->core_enabled = t->core_count; t->l1_cache_handle = 0xffff; t->l2_cache_handle = 0xffff; t->l3_cache_handle = 0xffff;
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 1: Code-Review-1
Reading the SMBIOS spec, this field seems explicitly designed to *not* reflect actual core count but rather whatever is configured by the firmware. So actually CONFIG_MAX_CPUS would be a better choice.
OTOH, maybe the CPUID result will reflect this anyway?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
Reading the SMBIOS spec, this field seems explicitly designed to *not* reflect actual core count but rather whatever is configured by the firmware. So actually CONFIG_MAX_CPUS would be a better choice.
OTOH, maybe the CPUID result will reflect this anyway?
The current code looks correct. CONFIG_MAX_CPUS is only used to determine the size of structures used during CPU init, so it is effectively the maximum amount of cores configured. However things often go wrong if the amount of cores exceeds CONFIG_MAX_CPUS.
Hello David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36662
to look at the new patch set (#2).
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
arch/x86: Correctly determine number of enabled cores
Instead of using MAX of (cores_enabled, MAX_CPUS), use MIN which is correct.
TEST=tested with dmidecode
Change-Id: Id0935f48e73c037bb7c0e1cf36f94d98a40a499c Signed-off-by: Andrey Petrov anpetrov@fb.com --- M src/arch/x86/smbios.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/36662/2
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 2:
ok. however current code is not correct and it should be MIN not MAX ;-)
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 2: Code-Review+1
Taking into account what Arthur said, PS2 looks good. There should probably be a sanity check elsewhere to ensure CONFIG_MAX_CPUS isn't exceeded. As far as SMBIOS tables are concerned using the MIN rather than MAX here seems correct.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 2: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 2: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
Patch Set 2: Code-Review+2
Andrey Petrov has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36662 )
Change subject: arch/x86: Correctly determine number of enabled cores ......................................................................
arch/x86: Correctly determine number of enabled cores
Instead of using MAX of (cores_enabled, MAX_CPUS), use MIN which is correct.
TEST=tested with dmidecode
Change-Id: Id0935f48e73c037bb7c0e1cf36f94d98a40a499c Signed-off-by: Andrey Petrov anpetrov@fb.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36662 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: David Hendricks david.hendricks@gmail.com Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/smbios.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, but someone else must approve Nico Huber: Looks good to me, approved Philipp Deppenwiese: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 261888f..725d808 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -659,7 +659,7 @@ t->processor_type = 3; /* System Processor */ t->core_count = (res.ebx >> 16) & 0xff; /* Assume we enable all the cores always, capped only by MAX_CPUS */ - t->core_enabled = MAX(t->core_count, CONFIG_MAX_CPUS); + t->core_enabled = MIN(t->core_count, CONFIG_MAX_CPUS); t->l1_cache_handle = 0xffff; t->l2_cache_handle = 0xffff; t->l3_cache_handle = 0xffff;
-
Andrey Petrov (Code Review) -
Arthur Heymans (Code Review)
-
David Hendricks (Code Review)
-
Nico Huber (Code Review)
-
Philipp Deppenwiese (Code Review)