Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax: Fix get_cores_per_package ......................................................................
cpu/intel/206ax: Fix get_cores_per_package
Current implementation uses CPUID 0Bh function that returns numbers of logical cores of requested level. The problem with this approach is that this value doesn't change when HyperThreading is disabled (it's in the Intel docs), so it breaks generate_cpu_entries().
Use MSR 0x35 instead, which returns correct number of logical processors with and without HT.
Related to #29669.
Change-Id: Ib32c2d40408cfa42ca43ab42ed661c168e579ada Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/cpu/intel/model_206ax/acpi.c 1 file changed, 5 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42413/1
diff --git a/src/cpu/intel/model_206ax/acpi.c b/src/cpu/intel/model_206ax/acpi.c index 9ff0673..44b3360 100644 --- a/src/cpu/intel/model_206ax/acpi.c +++ b/src/cpu/intel/model_206ax/acpi.c @@ -14,18 +14,13 @@
static int get_cores_per_package(void) { - struct cpuinfo_x86 c; - struct cpuid_result result; - int cores = 1; + msr_t msr; + int logical_cores;
- get_fms(&c, cpuid_eax(1)); - if (c.x86 != 6) - return 1; + msr = rdmsr(MSR_CORE_THREAD_COUNT); + logical_cores = msr.lo & 0xffff;
- result = cpuid_ext(0xb, 1); - cores = result.ebx & 0xff; - - return cores; + return logical_cores; }
static void generate_cstate_entries(acpi_cstate_t *cstates,
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax: Fix get_cores_per_package ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42413/1/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/1/src/cpu/intel/model_206ax/a... PS1, Line 15: get_cores_per_package probably worth renaming this to get_threads_per_package or get_logical_cores_per_package
Hello build bot (Jenkins), Patrick Rudolph, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42413
to look at the new patch set (#2).
Change subject: cpu/intel/206ax: Fix get_cores_per_package ......................................................................
cpu/intel/206ax: Fix get_cores_per_package
Current implementation uses CPUID 0Bh function that returns numbers of logical cores of requested level. The problem with this approach is that this value doesn't change when HyperThreading is disabled (it's in the Intel docs), so it breaks generate_cpu_entries().
- Use MSR 0x35 instead, which returns correct number of logical processors with and without HT.
- Rename the function to get_logical_cores_per_package, which is more accurate.
Related to #29669.
Change-Id: Ib32c2d40408cfa42ca43ab42ed661c168e579ada Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/cpu/intel/model_206ax/acpi.c 1 file changed, 7 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42413/2
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax: Fix get_cores_per_package ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42413/1/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/1/src/cpu/intel/model_206ax/a... PS1, Line 15: get_cores_per_package
probably worth renaming this to get_threads_per_package or get_logical_cores_per_package
Done
Hello build bot (Jenkins), Patrick Rudolph, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42413
to look at the new patch set (#3).
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
cpu/intel/206ax/acpi.c: Fix get_cores_per_package
Current implementation uses CPUID 0Bh function that returns numbers of logical cores of requested level. The problem with this approach is that this value doesn't change when HyperThreading is disabled (it's in the Intel docs), so it breaks generate_cpu_entries().
- Use MSR 0x35 instead, which returns correct number of logical processors with and without HT.
- Rename the function to get_logical_cores_per_package, which is more accurate.
Related to #29669.
Change-Id: Ib32c2d40408cfa42ca43ab42ed661c168e579ada Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/cpu/intel/model_206ax/acpi.c 1 file changed, 7 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42413/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG@20 PS3, Line 20: Related to #29669. Please use the Change-Id as reference, so Gerrit marks it up as a link.
https://review.coreboot.org/c/coreboot/+/42413/3/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/3/src/cpu/intel/model_206ax/a... PS3, Line 21: logical_cores = msr.lo & 0xffff; Excuse my ignorance, but why is the `& 0xffff` needed?
typedef struct msr_struct { unsigned int lo; unsigned int hi; } msr_t;
Hello build bot (Jenkins), Patrick Rudolph, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42413
to look at the new patch set (#4).
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
cpu/intel/206ax/acpi.c: Fix get_cores_per_package
Current implementation uses CPUID 0Bh function that returns numbers of logical cores of requested level. The problem with this approach is that this value doesn't change when HyperThreading is disabled (it's in the Intel docs), so it breaks generate_cpu_entries().
- Use MSR 0x35 instead, which returns correct number of logical processors with and without HT.
- Rename the function to get_logical_cores_per_package, which is more accurate.
Related to I2b73e32ff5af8ea64a47e8aa706e27648aaf0993.
Change-Id: Ib32c2d40408cfa42ca43ab42ed661c168e579ada Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/cpu/intel/model_206ax/acpi.c 1 file changed, 7 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42413/4
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42413/3/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/3/src/cpu/intel/model_206ax/a... PS3, Line 21: logical_cores = msr.lo & 0xffff;
Excuse my ignorance, but why is the `& 0xffff` needed? […]
Well, it's how it returns data. Bits [31:16] are cores count and [15:0] are threads count. We need threads so 0xffff.
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG@20 PS3, Line 20: Related to #29669.
Please use the Change-Id as reference, so Gerrit marks it up as a link.
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 5: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG@20 PS3, Line 20: Related to #29669.
Done
CB:29669 also works, see? 😄
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG@9 PS5, Line 9: numbers the number
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG@14 PS5, Line 14: correct the correct
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG@15 PS5, Line 15: processors with and without HT. Was this tested? If so, I'd mention which hardware this was tested on
https://review.coreboot.org/c/coreboot/+/42413/5/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/5/src/cpu/intel/model_206ax/a... PS5, Line 15: int this should be an unsigned int, but one would have to change several things
https://review.coreboot.org/c/coreboot/+/42413/5/src/cpu/intel/model_206ax/a... PS5, Line 21: logical_cores I would eliminate this variable and `return msr.lo & 0xffff;` directly.
Hello build bot (Jenkins), Patrick Rudolph, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42413
to look at the new patch set (#6).
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
cpu/intel/206ax/acpi.c: Fix get_cores_per_package
Current implementation uses CPUID 0Bh function that returns the number of logical cores of requested level. The problem with this approach is that this value doesn't change when HyperThreading is disabled (it's in the Intel docs), so it breaks generate_cpu_entries().
- Use MSR 0x35 instead, which returns the correct number of logical processors with and without HT.
- Rename the function to get_logical_cores_per_package, which is more accurate.
Tested on ThinkPad X220 with and without HT.
Related to CB:29669.
Change-Id: Ib32c2d40408cfa42ca43ab42ed661c168e579ada Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M src/cpu/intel/model_206ax/acpi.c 1 file changed, 4 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/42413/6
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42413/3//COMMIT_MSG@20 PS3, Line 20: Related to #29669.
CB:29669 also works, see? 😄
Much better.
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG@9 PS5, Line 9: numbers
the number
Done
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG@14 PS5, Line 14: correct
the correct
Done
https://review.coreboot.org/c/coreboot/+/42413/5//COMMIT_MSG@15 PS5, Line 15: processors with and without HT.
Was this tested? If so, I'd mention which hardware this was tested on
Done
https://review.coreboot.org/c/coreboot/+/42413/5/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/5/src/cpu/intel/model_206ax/a... PS5, Line 21: logical_cores
I would eliminate this variable and `return msr.lo & 0xffff;` directly.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 6: Code-Review+1
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42413/3/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/42413/3/src/cpu/intel/model_206ax/a... PS3, Line 21: logical_cores = msr.lo & 0xffff;
Well, it's how it returns data. Bits [31:16] are cores count and [15:0] are threads count. […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42413 )
Change subject: cpu/intel/206ax/acpi.c: Fix get_cores_per_package ......................................................................
cpu/intel/206ax/acpi.c: Fix get_cores_per_package
Current implementation uses CPUID 0Bh function that returns the number of logical cores of requested level. The problem with this approach is that this value doesn't change when HyperThreading is disabled (it's in the Intel docs), so it breaks generate_cpu_entries().
- Use MSR 0x35 instead, which returns the correct number of logical processors with and without HT.
- Rename the function to get_logical_cores_per_package, which is more accurate.
Tested on ThinkPad X220 with and without HT.
Related to CB:29669.
Change-Id: Ib32c2d40408cfa42ca43ab42ed661c168e579ada Signed-off-by: Evgeny Zinoviev me@ch1p.io Reviewed-on: https://review.coreboot.org/c/coreboot/+/42413 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/cpu/intel/model_206ax/acpi.c 1 file changed, 4 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/cpu/intel/model_206ax/acpi.c b/src/cpu/intel/model_206ax/acpi.c index 13ee20a..b24f411 100644 --- a/src/cpu/intel/model_206ax/acpi.c +++ b/src/cpu/intel/model_206ax/acpi.c @@ -13,20 +13,10 @@ #include "model_206ax.h" #include "chip.h"
-static int get_cores_per_package(void) +static int get_logical_cores_per_package(void) { - struct cpuinfo_x86 c; - struct cpuid_result result; - int cores = 1; - - get_fms(&c, cpuid_eax(1)); - if (c.x86 != 6) - return 1; - - result = cpuid_ext(0xb, 1); - cores = result.ebx & 0xff; - - return cores; + msr_t msr = rdmsr(MSR_CORE_THREAD_COUNT); + return msr.lo & 0xffff; }
static void generate_cstate_entries(acpi_cstate_t *cstates, @@ -288,7 +278,7 @@ { int coreID, cpuID, pcontrol_blk = PMB0_BASE, plen = 6; int totalcores = dev_count_cpu(); - int cores_per_package = get_cores_per_package(); + int cores_per_package = get_logical_cores_per_package(); int numcpus = totalcores/cores_per_package;
printk(BIOS_DEBUG, "Found %d CPU(s) with %d core(s) each.\n",
-
Angel Pons (Code Review) -
Evgeny Zinoviev (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)