Change in coreboot[master]: soc/intel/common/block/acpi: Fix get_cores_per_package

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48691 )

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

soc/intel/common/block/acpi: 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 Prodrive Hermes, the ACPI code is now generated even with
HyperThreading disabled.

Change-Id: Id9b985a07cd3f99a823622f766c80ff240ac1188
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/common/block/acpi/acpi.c
1 file changed, 7 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/48691/1

diff --git a/src/soc/intel/common/block/acpi/acpi.c b/src/soc/intel/common/block/acpi/acpi.c
index d2177294..f25cb1d 100644
--- a/src/soc/intel/common/block/acpi/acpi.c
+++ b/src/soc/intel/common/block/acpi/acpi.c
@@ -12,6 +12,7 @@
 #include <cpu/intel/turbo.h>
 #include <cpu/intel/common/common.h>
 #include <cpu/x86/smm.h>
+#include <cpu/x86/msr.h>
 #include <intelblocks/acpi.h>
 #include <intelblocks/lpc_lib.h>
 #include <intelblocks/msr.h>
@@ -263,20 +264,11 @@
    return power;
 }

-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);
+	/* Note: SDM is wrong, this is correct */
+	return msr.lo & 0xffff;
 }

static void generate_c_state_entries(void)
@@ -429,10 +421,10 @@
    int core_id, cpu_id, pcontrol_blk = ACPI_BASE_ADDRESS;
    int 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",
+	printk(BIOS_DEBUG, "Found %d CPU(s) with %d logical core(s) each.\n",
           numcpus, cores_per_package);

for (cpu_id = 0; cpu_id < numcpus; cpu_id++) {

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48691 )

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/48691/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/48691/1/src/soc/intel/common/block/... 
PS1, Line 424: get_logical_cores_per_package();
can you reuse cpu_read_topology()?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48691 )

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/48691/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48691/1//COMMIT_MSG@12 
PS1, Line 12: .
because `numcpus` ends up being zero due to integer division truncation.

https://review.coreboot.org/c/coreboot/+/48691/1//COMMIT_MSG@21 
PS1, Line 21: HyperThreading disabled.
Maybe mention that this change is based on commit 920d2b77f2

[Patrick Rudolph (Code Review)]

Hello Philipp Deppenwiese, build bot (Jenkins), Shaunak Saha, Marc Jones, Duncan Laurie, Christian Walter, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48691

to look at the new patch set (#2).

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

soc/intel/common/block/acpi: 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() because `numcpus`
ends up being zero due to integer division truncation.

- Use MSR 0x35 instead, which returns the correct number of logical
  processors with and without HT.

- Use cpu_read_topology() to gather the required information

Tested on Prodrive Hermes, the ACPI code is now generated even with
HyperThreading disabled.

Change-Id: Id9b985a07cd3f99a823622f766c80ff240ac1188
Signed-off-by: Patrick Rudolph 
---
M src/soc/intel/common/block/acpi/acpi.c
1 file changed, 13 insertions(+), 26 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/48691/2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48691 )

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/48691/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48691/1//COMMIT_MSG@12 
PS1, Line 12: .
> because `numcpus` ends up being zero due to integer division truncation.

Done

https://review.coreboot.org/c/coreboot/+/48691/1//COMMIT_MSG@21 
PS1, Line 21: HyperThreading disabled.
> Maybe mention that this change is based on commit 920d2b77f2

use different function, marking as obsolete

https://review.coreboot.org/c/coreboot/+/48691/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/48691/1/src/soc/intel/common/block/... 
PS1, Line 424: get_logical_cores_per_package();
> can you reuse cpu_read_topology()?

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48691 )

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

Patch Set 2: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48691 )

Change subject: soc/intel/common/block/acpi: Fix get_cores_per_package
......................................................................

soc/intel/common/block/acpi: 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() because `numcpus`
ends up being zero due to integer division truncation.

- Use MSR 0x35 instead, which returns the correct number of logical
  processors with and without HT.

- Use cpu_read_topology() to gather the required information

Tested on Prodrive Hermes, the ACPI code is now generated even with
HyperThreading disabled.

Change-Id: Id9b985a07cd3f99a823622f766c80ff240ac1188
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48691
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/common/block/acpi/acpi.c
1 file changed, 13 insertions(+), 26 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/common/block/acpi/acpi.c b/src/soc/intel/common/block/acpi/acpi.c
index 934a92f7..e3e72fd 100644
--- a/src/soc/intel/common/block/acpi/acpi.c
+++ b/src/soc/intel/common/block/acpi/acpi.c
@@ -14,7 +14,6 @@
 #include <cpu/x86/smm.h>
 #include <intelblocks/acpi.h>
 #include <intelblocks/lpc_lib.h>
-#include <intelblocks/msr.h>
 #include <intelblocks/pmclib.h>
 #include <intelblocks/uart.h>
 #include <soc/gpio.h>
@@ -284,22 +283,6 @@
    return power;
 }

-static int get_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;
-}
-
 static void generate_c_state_entries(void)
 {
    acpi_cstate_t *c_state_map;
@@ -450,21 +433,25 @@
    int core_id, cpu_id, pcontrol_blk = ACPI_BASE_ADDRESS;
    int plen = 6;
    int totalcores = dev_count_cpu();
-	int cores_per_package = get_cores_per_package();
-	int numcpus = totalcores / cores_per_package;
+	unsigned int num_virt;
+	unsigned int num_phys;

-	printk(BIOS_DEBUG, "Found %d CPU(s) with %d core(s) each.\n",
-	       numcpus, cores_per_package);
+	cpu_read_topology(&num_phys, &num_virt);
+
+	int numcpus = totalcores / num_virt;
+
+	printk(BIOS_DEBUG, "Found %d CPU(s) with %d/%d physical/logical core(s) each.\n",
+	       numcpus, num_phys, num_virt);

for (cpu_id = 0; cpu_id < numcpus; cpu_id++) {
-		for (core_id = 0; core_id < cores_per_package; core_id++) {
+		for (core_id = 0; core_id < num_virt; core_id++) {
    		if (core_id > 0) {
    			pcontrol_blk = 0;
    			plen = 0;
    		}

/* Generate processor _SB.CPUx */
-			acpigen_write_processor((cpu_id) * cores_per_package +
+			acpigen_write_processor((cpu_id) * num_virt +
    					core_id, pcontrol_blk, plen);

/* Generate C-state tables */
@@ -473,17 +460,17 @@
    		generate_cppc_entries(core_id);

/* Soc specific power states generation */
-			soc_power_states_generation(core_id, cores_per_package);
+			soc_power_states_generation(core_id, num_virt);

acpigen_pop_len();
    	}
    }
    /* PPKG is usually used for thermal management
       of the first and only package. */
-	acpigen_write_processor_package("PPKG", 0, cores_per_package);
+	acpigen_write_processor_package("PPKG", 0, num_virt);

/* Add a method to notify processor nodes */
-	acpigen_write_processor_cnot(cores_per_package);
+	acpigen_write_processor_cnot(num_virt);
 }

#if CONFIG(SOC_INTEL_COMMON_ACPI_WAKE_SOURCE)