Jérémy Compostella has submitted this change. ( https://review.coreboot.org/c/coreboot/+/85576?usp=email )
Change subject: cpu/x86/topology: Simplify CPU topology initialization ......................................................................
cpu/x86/topology: Simplify CPU topology initialization
This commit rewrites the CPU topology initialization code to simplify it and make it more maintainable.
The previous code used a complex set of if-else statements to initialize the CPU topology based on the CPUID leaves that were supported. This has been replaced with a simpler and more readable function that follows the Intel Software Developer Manual recommendation by prioritizing CPUID EAX=0x1f over CPUID EAX=0xb if available.
The new code removes the need for separate functions to handle the topology initialization for different CPUID leaves. It uses a static array of bitfield descriptors to store the APIC ID descriptor information for each level of the CPU topology. This simplifies the code and makes it easier to add new levels of topology in the future.
The code populates the node ID based on the package ID, eliminating the need for an extra function call.
Change-Id: Ie9424559f895af69e79c36b919e80af803861148 Signed-off-by: Jeremy Compostella jeremy.compostella@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/85576 Reviewed-by: Jincheng Li jincheng.li@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Shuo Liu shuo.liu@intel.com --- M src/cpu/x86/mp_init.c M src/cpu/x86/topology.c M src/include/cpu/x86/topology.h M src/soc/intel/xeon_sp/spr/cpu.c 4 files changed, 108 insertions(+), 123 deletions(-)
Approvals: build bot (Jenkins): Verified Jincheng Li: Looks good to me, but someone else must approve Shuo Liu: Looks good to me, approved
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index bb51697..47b3a9e 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -204,7 +204,7 @@ dev->path.apic.initial_lapicid = initial_lapicid(); dev->enabled = 1;
- set_cpu_topology_from_leaf_b(dev); + set_cpu_topology(dev);
if (cpu_is_intel()) printk(BIOS_INFO, "AP: slot %u apic_id %x, MCU rev: 0x%08x\n", index, @@ -562,7 +562,7 @@ return CB_ERR; } bsp->path.apic.initial_lapicid = initial_lapicid(); - set_cpu_topology_from_leaf_b(bsp); + set_cpu_topology(bsp);
/* Find the device structure for the boot CPU. */ set_cpu_info(0, bsp); diff --git a/src/cpu/x86/topology.c b/src/cpu/x86/topology.c index baa4a7a..b2fd202 100644 --- a/src/cpu/x86/topology.c +++ b/src/cpu/x86/topology.c @@ -1,38 +1,73 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <console/console.h> #include <cpu/cpu.h> -#include <device/device.h> #include <cpu/x86/topology.h> +#include <device/device.h>
-#define CPUID_EXTENDED_CPU_TOPOLOGY2 0x1f +/* + * The following code defines functions for populating the topology information of a given CPU + * device (struct device *cpu). + * + * Reference: Intel Software Developer Manual Extended Topology Enumeration Leaves (CPUID EAX 0xb + * and 0x1f). + */
-#define CPUID_EXTENDED_CPU_TOPOLOGY 0x0b -#define LEVEL_TYPE_CORE 2 -#define LEVEL_TYPE_SMT 1 +#define CPUID_EXTENDED_CPU_TOPOLOGY 0x0b +#define CPUID_EXTENDED_CPU_TOPOLOGY2 0x1f + +enum { + LEVEL_TYPE_PACKAGE, /* Use unused level zero for package. */ + LEVEL_TYPE_SMT, + LEVEL_TYPE_CORE, + LEVEL_TYPE_MODULE, + LEVEL_TYPE_TILE, + LEVEL_TYPE_DIE, + LEVEL_TYPE_DIEGRP, + LEVEL_TYPE_MAX +};
#define CPUID_CPU_TOPOLOGY(x, val) \ (((val) >> CPUID_CPU_TOPOLOGY_##x##_SHIFT) & CPUID_CPU_TOPOLOGY_##x##_MASK)
-#define CPUID_CPU_TOPOLOGY_LEVEL_TYPE_SHIFT 0x8 -#define CPUID_CPU_TOPOLOGY_LEVEL_TYPE_MASK 0xff -#define CPUID_CPU_TOPOLOGY_LEVEL(res) CPUID_CPU_TOPOLOGY(LEVEL_TYPE, (res).ecx) +#define CPUID_CPU_TOPOLOGY_LEVEL_TYPE_SHIFT 0x8 +#define CPUID_CPU_TOPOLOGY_LEVEL_TYPE_MASK 0xff +#define CPUID_CPU_TOPOLOGY_LEVEL(res) CPUID_CPU_TOPOLOGY(LEVEL_TYPE, (res).ecx)
-#define CPUID_CPU_TOPOLOGY_LEVEL_BITS_SHIFT 0x0 -#define CPUID_CPU_TOPOLOGY_LEVEL_BITS_MASK 0x1f -#define CPUID_CPU_TOPOLOGY_THREAD_BITS(res) CPUID_CPU_TOPOLOGY(LEVEL_BITS, (res).eax) -#define CPUID_CPU_TOPOLOGY_CORE_BITS(res, threadbits) \ - ((CPUID_CPU_TOPOLOGY(LEVEL_BITS, (res).eax)) - threadbits) +#define CPUID_CPU_TOPOLOGY_LEVEL_BITS_SHIFT 0x0 +#define CPUID_CPU_TOPOLOGY_LEVEL_BITS_MASK 0x1f
-/* Return the level shift for the highest supported level (the package) */ -static enum cb_err get_cpu_package_bits(uint32_t *package_bits) +struct bitfield_descriptor { + uint8_t first_bit; + uint8_t num_bits; +}; + +/* + * Fills in the supplied bit field APIC ID descriptor structure array. + * + * Per Intel Software Developer Manual recommendation, it prioritizes CPUID EAX=0x1f over CPUID + * EAX=0xb if available. The function iterates over the hierarchy levels to extract per-domain + * (level) APIC bit field information and populates the provided topology array with the + * bitfield descriptors for each topology level. + * + * Returns CB_ERR if: + * - CPUID EAX=0x1f is not supported. + * - CPUID EAX=0xb is not supported. + * - The first topology level is 0. + * + * Returns CB_SUCCESS otherwise. + */ +static enum cb_err read_topology_structure(struct bitfield_descriptor *topology) { - struct cpuid_result cpuid_regs; - int level_num, cpu_id_op = 0; const uint32_t cpuid_max_func = cpuid_get_max_func(); + struct cpuid_result cpuid_regs; + int level_num = 0, cpu_id_op; + unsigned int level; + uint8_t next_level_first_bit, first_bit = 0;
/* - * Not all CPUs support this, those won't get topology filled in here. - * CPU specific code can do this however. + * Not all CPUs support this, those won't get topology filled in here. CPU specific + * code can do this however. */ if (cpuid_max_func >= CPUID_EXTENDED_CPU_TOPOLOGY2) cpu_id_op = CPUID_EXTENDED_CPU_TOPOLOGY2; @@ -41,124 +76,78 @@ else return CB_ERR;
- *package_bits = level_num = 0; cpuid_regs = cpuid_ext(cpu_id_op, level_num);
/* - * Sub-leaf index 0 enumerates SMT level, some AMD CPUs leave this CPUID leaf - * reserved so bail out. Cpu specific code can fill in the topology later. + * Sub-leaf index 0 enumerates SMT level, some AMD CPUs leave this CPUID leaf reserved + * so bail out. Cpu specific code can fill in the topology later. */ if (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs) != LEVEL_TYPE_SMT) return CB_ERR;
do { - *package_bits = (CPUID_CPU_TOPOLOGY(LEVEL_BITS, (cpuid_regs).eax)); + level = CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs); + next_level_first_bit = CPUID_CPU_TOPOLOGY(LEVEL_BITS, cpuid_regs.eax); + if (level < LEVEL_TYPE_MAX) { + topology[level].num_bits = next_level_first_bit - first_bit; + topology[level].first_bit = first_bit; + } else { + printk(BIOS_ERR, "Unknown processor topology level %d\n", level); + } + first_bit = next_level_first_bit; level_num++; cpuid_regs = cpuid_ext(cpu_id_op, level_num); /* Stop when level type is invalid i.e 0. */ } while (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs));
+ topology[LEVEL_TYPE_PACKAGE].first_bit = next_level_first_bit; return CB_SUCCESS; }
-void set_cpu_node_id_leaf_1f_b(struct device *cpu) +void set_cpu_topology(struct device *cpu) { - static uint32_t package_bits; - static enum cb_err package_bits_ret; - static bool done = false; + const uint32_t apicid = cpu->path.apic.initial_lapicid; + static struct bitfield_descriptor topology[LEVEL_TYPE_MAX]; + static enum cb_err ret; + static bool done; + struct { + unsigned int level; + unsigned int *field; + } apic_fields[] = { + { LEVEL_TYPE_SMT, &cpu->path.apic.thread_id }, + { LEVEL_TYPE_CORE, &cpu->path.apic.core_id }, + { LEVEL_TYPE_PACKAGE, &cpu->path.apic.package_id }, + { LEVEL_TYPE_PACKAGE, &cpu->path.apic.node_id } + };
if (!done) { - package_bits_ret = get_cpu_package_bits(&package_bits); + ret = read_topology_structure(topology); done = true; }
- const uint32_t apicid = cpu->path.apic.initial_lapicid; - /* - * If leaf_1f or leaf_b does not exist don't update the node_id. - */ - if (package_bits_ret == CB_SUCCESS) - cpu->path.apic.node_id = (apicid >> package_bits); -} - -/* Get number of bits for core ID and SMT ID */ -static enum cb_err get_cpu_core_thread_bits(uint32_t *core_bits, uint32_t *thread_bits) -{ - struct cpuid_result cpuid_regs; - int level_num, cpu_id_op = 0; - const uint32_t cpuid_max_func = cpuid_get_max_func(); - - /* - * Not all CPUs support this, those won't get topology filled in here. - * CPU specific code can do this however. - */ - if (cpuid_max_func < CPUID_EXTENDED_CPU_TOPOLOGY) - return CB_ERR; - - cpu_id_op = CPUID_EXTENDED_CPU_TOPOLOGY; - - *core_bits = level_num = 0; - cpuid_regs = cpuid_ext(cpu_id_op, level_num); - - /* - * Sub-leaf index 0 enumerates SMT level, some AMD CPUs leave this CPUID leaf - * reserved so bail out. Cpu specific code can fill in the topology later. - */ - if (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs) != LEVEL_TYPE_SMT) - return CB_ERR; - - *thread_bits = CPUID_CPU_TOPOLOGY_THREAD_BITS(cpuid_regs); - do { - level_num++; - cpuid_regs = cpuid_ext(cpu_id_op, level_num); - if (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs) == LEVEL_TYPE_CORE) { - *core_bits = CPUID_CPU_TOPOLOGY_CORE_BITS(cpuid_regs, *thread_bits); - break; - } - /* Stop when level type is invalid i.e 0 */ - } while (CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs)); - - return CB_SUCCESS; -} - -static void set_cpu_topology(struct device *cpu, unsigned int node, - unsigned int package, unsigned int core, - unsigned int thread) -{ - cpu->path.apic.node_id = node; - cpu->path.apic.package_id = package; - cpu->path.apic.core_id = core; - cpu->path.apic.thread_id = thread; -} - -void set_cpu_topology_from_leaf_b(struct device *cpu) -{ - static uint32_t core_bits, thread_bits; - static enum cb_err core_thread_bits_ret; - static bool done = false; - if (!done) { - core_thread_bits_ret = get_cpu_core_thread_bits(&core_bits, &thread_bits); - done = true; - } - - const uint32_t apicid = cpu->path.apic.initial_lapicid; - uint32_t package_id, core_id, thread_id; - /* - * If leaf_b does not exist set the following best-guess defaults: + * If no APIC descriptor is found, set the following best-guess defaults: * - 1 package - * - no SMP * - core_id = apicid + * - no SMP + * - 1 node * CPU specific code can always update these later on. */ - if (core_thread_bits_ret != CB_SUCCESS) { - package_id = 0; - core_id = apicid; - thread_id = 0; - } else { - package_id = apicid >> (thread_bits + core_bits); - core_id = (apicid >> thread_bits) & ((1 << core_bits) - 1); - thread_id = apicid & ((1 << thread_bits) - 1); + if (ret != CB_SUCCESS) { + cpu->path.apic.package_id = 0; + cpu->path.apic.core_id = apicid; + cpu->path.apic.thread_id = 0; + cpu->path.apic.node_id = 0; + return; }
- set_cpu_topology(cpu, 0, package_id, core_id, thread_id); + for (size_t i = 0; i < ARRAY_SIZE(apic_fields); i++) { + struct bitfield_descriptor *desc = &topology[apic_fields[i].level]; + if (desc->first_bit || desc->num_bits) { + unsigned int value = apicid >> desc->first_bit; + if (desc->num_bits) + value &= (1 << (desc->num_bits)) - 1; + *apic_fields[i].field = value; + } + } } diff --git a/src/include/cpu/x86/topology.h b/src/include/cpu/x86/topology.h index d66f2eb..a40052e 100644 --- a/src/include/cpu/x86/topology.h +++ b/src/include/cpu/x86/topology.h @@ -5,15 +5,13 @@
#include <device/device.h>
-/* Fill in the topology in struct path APIC based CPUID EAX=0xb. - * If leaf 0xb is not supported or is not implemented then no topology - * will be filled in. +/* + * Sets the topology information for the given CPU device using the bitfield descriptors + * obtained from the CPUID leaves. Per Intel Software Developer Manual recommendation, it + * prioritizes CPUID EAX=0x1f over CPUID EAX=0xb if available. + * + * If the topology information cannot be obtained from CPUID, it sets default values. */ -void set_cpu_topology_from_leaf_b(struct device *cpu); +void set_cpu_topology(struct device *cpu);
-/* Fill in the topology node ID in struct path APIC based CPUID EAX=0x1f - * or CPUID EAX=0xb. If those leaves aren't supported then the node ID - * won't be updated. - */ -void set_cpu_node_id_leaf_1f_b(struct device *cpu); #endif diff --git a/src/soc/intel/xeon_sp/spr/cpu.c b/src/soc/intel/xeon_sp/spr/cpu.c index 7af7f9a..febc72b 100644 --- a/src/soc/intel/xeon_sp/spr/cpu.c +++ b/src/soc/intel/xeon_sp/spr/cpu.c @@ -72,8 +72,6 @@ __func__, dev_path(cpu), cpu_index(), cpu->path.apic.apic_id, cpu->path.apic.package_id);
- /* Populate the node ID. It will be used as proximity ID. */ - set_cpu_node_id_leaf_1f_b(cpu); assert (cpu->path.apic.node_id < CONFIG_MAX_SOCKET);
/*
-
Jérémy Compostella (Code Review)