coreboot-gerrit November 2022

coreboot-gerrit@coreboot.org

1 participants
7286 discussions

[M] Change in coreboot[master]: soc/intel/xeon_sp: Don't sort struct device cpus for numa
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Lance Zhao, Arthur Heymans, Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Tim Wawrzynczak, Christian Walter, Angel Pons, Shuming Chu (Shuming), TangYiwei. Hello Lance Zhao, build bot (Jenkins), Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Tim Wawrzynczak, Christian Walter, TangYiwei, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/68912 to look at the new patch set (#7). Change subject: soc/intel/xeon_sp: Don't sort struct device cpus for numa ...................................................................... soc/intel/xeon_sp: Don't sort struct device cpus for numa Currently the xeon_sp code reassigns struct devices apic_id so that srat entries can be added in a certain order. This is not a good idea as it breaks thread local storage which contains a pointer to it's struct device cpu. This moves the sorting of the lapic_ids to the srat table generation and adds the numa node id in each core init entry (now it is done in parallel too as a bonus). Change-Id: I372bcea1932d28e9bf712cc712f19a76fe3199b1 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/acpi/acpi.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/nb_acpi.c M src/soc/intel/xeon_sp/skx/cpu.c M src/soc/intel/xeon_sp/util.c 6 files changed, 60 insertions(+), 121 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/68912/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/68912 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I372bcea1932d28e9bf712cc712f19a76fe3199b1 Gerrit-Change-Number: 68912 Gerrit-PatchSet: 7 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: Lance Zhao Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-Attention: TangYiwei Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: soc/intel/xeon_sp: Don't sort struct device cpus for numa
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Lance Zhao, Arthur Heymans, Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Tim Wawrzynczak, Christian Walter, Angel Pons, Shuming Chu (Shuming), TangYiwei. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68912 ) Change subject: soc/intel/xeon_sp: Don't sort struct device cpus for numa ...................................................................... Patch Set 6: (2 comments) File src/soc/intel/xeon_sp/nb_acpi.c: https://review.coreboot.org/c/coreboot/+/68912/comment/b97fe6ea_1c6f9a88 PS3, Line 29: /* First all regular cpus, then all the threads */ : for (cpu = all_devices; cpu; cpu = cpu->next) { : if (!is_enabled_cpu(cpu)) : continue; : if (num_cpus >= ARRAY_SIZE(apic_ids)) : break; : /* Is this a thread? */ : if (cpu->path.apic.apic_id & 1) : continue; : apic_ids[num_cpus++] = cpu->path.apic.apic_id; : } : const uint32_t num_of_cores = num_cpus; : if (num_cpus > 1) : bubblesort(&apic_ids[0], num_of_cores, NUM_ASCENDING); > > Maybe rewrite this as a loop? […] Done https://review.coreboot.org/c/coreboot/+/68912/comment/83370364_256cd0cd PS3, Line 54: if (num_cpus > 1) > > What's the purpose of this check? Did you mean to check if `num_cpus - num_of_cores > 1` instead? […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/68912 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I372bcea1932d28e9bf712cc712f19a76fe3199b1 Gerrit-Change-Number: 68912 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: Lance Zhao Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-Attention: TangYiwei Gerrit-Comment-Date: Fri, 04 Nov 2022 13:11:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur.heymans(a)9elements.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/intel/xeon_sp: Don't sort struct device cpus for numa
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Lance Zhao, Arthur Heymans, Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Tim Wawrzynczak, Christian Walter, Angel Pons, Shuming Chu (Shuming), TangYiwei. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68912 ) Change subject: soc/intel/xeon_sp: Don't sort struct device cpus for numa ...................................................................... Patch Set 5: (1 comment) File src/soc/intel/xeon_sp/cpx/cpu.c: https://review.coreboot.org/c/coreboot/+/68912/comment/5f67cc68_162857e6 PS3, Line 81: apic_id > Should it be? > cpu->path.apic.package_id = get_cpu_package(cpu->path.apic.apic_id); I moved parsing cpu topology in a common path on mp init and also fixed up madt table to put threads below. -- To view, visit https://review.coreboot.org/c/coreboot/+/68912 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I372bcea1932d28e9bf712cc712f19a76fe3199b1 Gerrit-Change-Number: 68912 Gerrit-PatchSet: 5 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: Lance Zhao Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-Attention: TangYiwei Gerrit-Comment-Date: Fri, 04 Nov 2022 13:10:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur.heymans(a)9elements.com> Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com> Comment-In-Reply-To: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: acpi/acpi.c: Follow spec more closely for MADT
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Lance Zhao, Tim Wawrzynczak. Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69225 ) Change subject: acpi/acpi.c: Follow spec more closely for MADT ...................................................................... acpi/acpi.c: Follow spec more closely for MADT Secondary threads need to be added after the primary threads. Change-Id: I3a98560760b662a7ba7efb46f5f7882fb0f7bb1f Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/acpi/acpi.c 1 file changed, 41 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/69225/1 diff --git a/src/acpi/acpi.c b/src/acpi/acpi.c index e211558..c233274 100644 --- a/src/acpi/acpi.c +++ b/src/acpi/acpi.c @@ -141,20 +141,41 @@ return lapic->length; } +/* Increase if necessary. Currently all x86 CPUs only have 2 SMP threads */ +#define MAX_THREAD_ID 1 +/* + * From ACPI 6.4 spec: + * "The advent of multi-threaded processors yielded multiple logical processors + * executing on common processor hardware. ACPI defines logical processors in + * an identical manner as physical processors. To ensure that non + * multi-threading aware OSPM implementations realize optimal performance on + * platforms containing multi-threaded processors, two guidelines should be + * followed. The first is the same as above, that is, OSPM should initialize + * processors in the order that they appear in the MADT. The second is that + * platform firmware should list the first logical processor of each of the + * individual multi-threaded processors in the MADT before listing any of the + * second logical processors. This approach should be used for all successive + * logical processors." + */ unsigned long acpi_create_madt_lapics(unsigned long current) { struct device *cpu; int index, apic_ids[CONFIG_MAX_CPUS] = {0}, num_cpus = 0; - for (cpu = all_devices; cpu; cpu = cpu->next) { - if (!is_enabled_cpu(cpu)) - continue; - if (num_cpus >= ARRAY_SIZE(apic_ids)) - break; - apic_ids[num_cpus++] = cpu->path.apic.apic_id; + unsigned int sort_start = 0; + for (unsigned int thread_id; thread_id <= MAX_THREAD_ID; thread_id++) { + for (cpu = all_devices; cpu; cpu = cpu->next) { + if (!is_enabled_cpu(cpu)) + continue; + if (num_cpus >= ARRAY_SIZE(apic_ids)) + break; + if (cpu->path.apic.thread_id != thread_id) + continue; + apic_ids[num_cpus++] = cpu->path.apic.apic_id; + } + bubblesort(&apic_ids[sort_start], num_cpus - sort_start, NUM_ASCENDING); + sort_start = num_cpus; } - if (num_cpus > 1) - bubblesort(apic_ids, num_cpus, NUM_ASCENDING); for (index = 0; index < num_cpus; index++) { if (apic_ids[index] < 0xff) current += acpi_create_madt_lapic((acpi_madt_lapic_t *)current, -- To view, visit https://review.coreboot.org/c/coreboot/+/69225 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3a98560760b662a7ba7efb46f5f7882fb0f7bb1f Gerrit-Change-Number: 69225 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Lance Zhao Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: cpu/x86/smm_module_loader: Use struct device instead of cpu index
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69224 ) Change subject: cpu/x86/smm_module_loader: Use struct device instead of cpu index ...................................................................... cpu/x86/smm_module_loader: Use struct device instead of cpu index The initial lapicid is now present inside struct device so simply loop over those to fill smm_stub params Change-Id: Ia1c2a889efe54589cf036cd1c3afb36e48d15720 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/arch/x86/cpu.c M src/cpu/x86/smm/smm_module_loader.c M src/include/cpu/cpu.h 3 files changed, 20 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/69224/1 diff --git a/src/arch/x86/cpu.c b/src/arch/x86/cpu.c index cee29fd..31e55da 100644 --- a/src/arch/x86/cpu.c +++ b/src/arch/x86/cpu.c @@ -210,7 +210,7 @@ cpu->ops = driver ? driver->ops : NULL; } -static struct bus *cpu_cluster(void) +struct bus *cpu_cluster(void) { static struct bus *bus; struct device *dev; @@ -249,22 +249,6 @@ return cpu_get_index(initial_lapicid()); } -/* Returns default APIC id based on logical_cpu number or < 0 on failure. */ -int cpu_get_apic_id(int logical_cpu) -{ - if (logical_cpu >= CONFIG_MAX_CPUS || logical_cpu < 0 || !cpu_cluster()) - return -1; - - struct device *dev = cpu_cluster()->children; - while (dev && logical_cpu != 0) { - dev = dev->sibling; - logical_cpu--; - } - if (!dev) - return -1; - return dev->path.apic.initial_lapicid; -} - void cpu_initialize(unsigned int index) { /* Because we busy wait at the printk spinlock. diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 0425b4c..660a3fe 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -7,6 +7,7 @@ #include <console/console.h> #include <cpu/cpu.h> #include <cpu/x86/smm.h> +#include <device/device.h> #include <rmodule.h> #include <stdio.h> #include <string.h> @@ -268,8 +269,9 @@ stub_params->fxsave_area = (uintptr_t)fxsave_area; stub_params->fxsave_area_size = FXSAVE_SIZE; - for (int i = 0; i < params->num_cpus; i++) - stub_params->apic_id_to_cpu[i] = cpu_get_apic_id(i); + int i = 0; + for (struct device *dev = cpu_cluster()->children; dev; dev = dev->sibling) + stub_params->apic_id_to_cpu[i++] = dev->path.apic.initial_lapicid; printk(BIOS_DEBUG, "%s: stack_top = 0x%x\n", __func__, stub_params->stack_top); printk(BIOS_DEBUG, "%s: per cpu stack_size = 0x%x\n", __func__, diff --git a/src/include/cpu/cpu.h b/src/include/cpu/cpu.h index 4b2d7ea..118ce8d 100644 --- a/src/include/cpu/cpu.h +++ b/src/include/cpu/cpu.h @@ -7,8 +7,8 @@ #include <stdint.h> void cpu_initialize(unsigned int cpu_index); -/* Returns default APIC id based on logical_cpu number or < 0 on failure. */ -int cpu_get_apic_id(int logical_cpu); +/* Return */ +struct bus *cpu_cluster(void); /* Return the CPU logical number based on default APIC id or < 0 on failure. */ int cpu_get_index(const unsigned int lapic_id); uintptr_t cpu_get_lapic_addr(void); -- To view, visit https://review.coreboot.org/c/coreboot/+/69224 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia1c2a889efe54589cf036cd1c3afb36e48d15720 Gerrit-Change-Number: 69224 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: mp_service_ppi: Use struct device to fill in buffer
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Andrey Petrov. Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69223 ) Change subject: mp_service_ppi: Use struct device to fill in buffer ...................................................................... mp_service_ppi: Use struct device to fill in buffer Now the CPU topology is filled in struct device during mp_init. Change-Id: I7322b43f5b95dda5fbe81e7427f5269c9d6f8755 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h 3 files changed, 20 insertions(+), 65 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/69223/1 diff --git a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c index 66bbc2f..07d5f39 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c @@ -28,24 +28,20 @@ efi_return_status_t mp_get_processor_info(efi_uintn_t processor_number, efi_processor_information *processor_info_buffer) { - int apicid; uint8_t package, core, thread; - if (cpu_index() < 0) - return FSP_DEVICE_ERROR; - if (processor_info_buffer == NULL) return FSP_INVALID_PARAMETER; if (processor_number >= get_cpu_count()) return FSP_NOT_FOUND; - apicid = cpu_get_apic_id(processor_number); + /* We can't use cpu_info() as we operate from FSP context here */ + struct device dev = *dev_find_lapic(lapicid()); + if (dev == NULL) + return FSP_NOT_FOUND; - if (apicid < 0) - return FSP_DEVICE_ERROR; - - processor_info_buffer->ProcessorId = apicid; + processor_info_buffer->ProcessorId = dev->path.apic.initial_lapicid; processor_info_buffer->StatusFlag = PROCESSOR_HEALTH_STATUS_BIT | PROCESSOR_ENABLED_BIT; @@ -53,12 +49,9 @@ if (processor_number == BSP_CPU_SLOT) processor_info_buffer->StatusFlag |= PROCESSOR_AS_BSP_BIT; - /* Fill EFI_CPU_PHYSICAL_LOCATION structure information */ - get_cpu_topology_from_apicid(apicid, &package, &core, &thread); - - processor_info_buffer->Location.Package = package; - processor_info_buffer->Location.Core = core; - processor_info_buffer->Location.Thread = thread; + processor_info_buffer->Location.Package = dev->path.apic.package_id; + processor_info_buffer->Location.Core = dev->path.apic.core_id; + processor_info_buffer->Location.Thread = dev->path.apic.thread_id; return FSP_SUCCESS; } diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 4898066..99a1399 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -435,52 +435,6 @@ return valid_size; } -/* Get number of bits for core ID and SMT ID */ -static void 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(); - - /* Assert if extended CPU topology not supported */ - assert(cpuid_max_func >= CPUID_EXTENDED_CPU_TOPOLOGY); - - 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, if not assert */ - assert(CPUID_CPU_TOPOLOGY_LEVEL(cpuid_regs) == LEVEL_TYPE_SMT); - - *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)); -} - -void get_cpu_topology_from_apicid(uint32_t apicid, uint8_t *package, - uint8_t *core, uint8_t *thread) -{ - - uint32_t core_bits, thread_bits; - - get_cpu_core_thread_bits(&core_bits, &thread_bits); - - if (package) - *package = apicid >> (thread_bits + core_bits); - if (core) - *core = (apicid >> thread_bits) & ((1 << core_bits) - 1); - if (thread) - *thread = apicid & ((1 << thread_bits) - 1); -} - static void sync_core_prmrr(void) { static msr_t msr_base, msr_mask; diff --git a/src/soc/intel/common/block/include/intelblocks/cpulib.h b/src/soc/intel/common/block/include/intelblocks/cpulib.h index 7878bf4..6d9b18d 100644 --- a/src/soc/intel/common/block/include/intelblocks/cpulib.h +++ b/src/soc/intel/common/block/include/intelblocks/cpulib.h @@ -183,10 +183,6 @@ */ void enable_pm_timer_emulation(void); -/* Derive core, package and thread information from lapic ID */ -void get_cpu_topology_from_apicid(uint32_t apicid, uint8_t *package, - uint8_t *core, uint8_t *thread); - /* * Initialize core PRMRR * -- To view, visit https://review.coreboot.org/c/coreboot/+/69223 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7322b43f5b95dda5fbe81e7427f5269c9d6f8755 Gerrit-Change-Number: 69223 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: cpu/x86/topology: Add code to fill in topology on struct path
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/69222 ) Change subject: cpu/x86/topology: Add code to fill in topology on struct path ...................................................................... cpu/x86/topology: Add code to fill in topology on struct path This is needed to generate MADT and SRAT where lapicid for threads need to be added last. Change-Id: I2210eb9b663dd90941a64132aa7154440dc7e5a9 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/cpu/x86/Makefile.inc M src/cpu/x86/mp_init.c A src/cpu/x86/topology.c A src/include/cpu/x86/topology.h 4 files changed, 98 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/69222/1 diff --git a/src/cpu/x86/Makefile.inc b/src/cpu/x86/Makefile.inc index 159b069..e1aff49 100644 --- a/src/cpu/x86/Makefile.inc +++ b/src/cpu/x86/Makefile.inc @@ -64,3 +64,5 @@ $(call src-to-obj,$(TARGET_STAGE),$(SIPI_BIN).manual): $(SIPI_BIN) @printf " OBJCOPY $(subst $(obj)/,,$(@))\n" cd $(dir $<); $(OBJCOPY_rmodules_$(ARCH-$(TARGET_STAGE)-y)) -I binary $(notdir $<) $(target-objcopy) $(abspath $@) + +ramstage-y += topology.c diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index ee8a685..e7702a1 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -13,6 +13,7 @@ #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> +#include <cpu/x86/topology.h> #include <cpu/x86/mp.h> #include <delay.h> #include <device/device.h> @@ -198,6 +199,8 @@ dev->path.apic.initial_lapicid = initial_lapicid(); dev->enabled = 1; + set_cpu_topology_from_leaf_b(dev); + if (cpu_is_intel()) printk(BIOS_INFO, "AP: slot %zu apic_id %x, MCU rev: 0x%08x\n", info->index, info->cpu->path.apic.apic_id, get_current_microcode_rev()); diff --git a/src/cpu/x86/topology.c b/src/cpu/x86/topology.c new file mode 100644 index 0000000..c1e417f --- /dev/null +++ b/src/cpu/x86/topology.c @@ -0,0 +1,72 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <assert.h> +#include <arch/cpu.h> +#include <device/device.h> +#include <cpu/x86/topology.h> + +#define CPUID_EXTENDED_CPU_TOPOLOGY 0x0b +#define LEVEL_TYPE_CORE 2 +#define LEVEL_TYPE_SMT 1 + +#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_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) + +/* 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(); + + /* Assert if extended CPU topology not supported */ + 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, if not assert */ + 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; +} + +void set_cpu_topology_from_leaf_b(struct device *cpu) +{ + if (cpu->path.type != DEVICE_PATH_APIC) + return; + + uint32_t core_bits, thread_bits; + if (get_cpu_core_thread_bits(&core_bits, &thread_bits) != CB_SUCCESS) + return; + + const uint32_t apicid = cpu->path.apic.initial_lapicid; + cpu->path.apic.package_id = apicid >> (thread_bits + core_bits); + cpu->path.apic.core_id = (apicid >> thread_bits) & ((1 << core_bits) - 1); + cpu->path.apic.thread_id = apicid & ((1 << thread_bits) - 1); +} diff --git a/src/include/cpu/x86/topology.h b/src/include/cpu/x86/topology.h new file mode 100644 index 0000000..f92005b --- /dev/null +++ b/src/include/cpu/x86/topology.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef CPU_X86_TOPOLOGY_H +#define CPU_X86_TOPOLOGY_H + +void set_cpu_topology_from_leaf_b(struct device *cpu); + +#endif -- To view, visit https://review.coreboot.org/c/coreboot/+/69222 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2210eb9b663dd90941a64132aa7154440dc7e5a9 Gerrit-Change-Number: 69222 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: soc/intel/xeon_sp: Don't sort struct device cpus for numa
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Arthur Heymans, Anjaneya "Reddy" Chagam, Johnny Lin, Christian Walter, Angel Pons, Shuming Chu (Shuming), TangYiwei. Hello build bot (Jenkins), Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Christian Walter, TangYiwei, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/68912 to look at the new patch set (#6). Change subject: soc/intel/xeon_sp: Don't sort struct device cpus for numa ...................................................................... soc/intel/xeon_sp: Don't sort struct device cpus for numa Currently the xeon_sp code reassigns struct devices apic_id so that srat entries can be added in a certain order. This is not a good idea as it breaks thread local storage which contains a pointer to it's struct device cpu. This moves the sorting of the lapic_ids to the srat table generation and adds the numa node id in each core init entry (now it is done in parallel too as a bonus). Change-Id: I372bcea1932d28e9bf712cc712f19a76fe3199b1 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/acpi/acpi.c M src/soc/intel/xeon_sp/cpx/cpu.c M src/soc/intel/xeon_sp/include/soc/util.h M src/soc/intel/xeon_sp/nb_acpi.c M src/soc/intel/xeon_sp/skx/cpu.c M src/soc/intel/xeon_sp/util.c 6 files changed, 60 insertions(+), 121 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/68912/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/68912 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I372bcea1932d28e9bf712cc712f19a76fe3199b1 Gerrit-Change-Number: 68912 Gerrit-PatchSet: 6 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: TangYiwei Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-Attention: TangYiwei Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: cpu/x86/mp_init.c: Keep track of initial lapic ID inside device_path
by Arthur Heymans (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Angel Pons, Arthur Heymans, Kyösti Mälkki. Hello build bot (Jenkins), Angel Pons, Elyes Haouas, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/64342 to look at the new patch set (#11). Change subject: cpu/x86/mp_init.c: Keep track of initial lapic ID inside device_path ...................................................................... cpu/x86/mp_init.c: Keep track of initial lapic ID inside device_path It's quite confusing to keep track of lapic ID inside the device struct and initial lapic ID inside an array. Change-Id: I4d9f8d23c0b0e5c142f6907593428d8509e4e7bb Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/arch/x86/cpu.c M src/arch/x86/include/arch/cpu.h M src/cpu/x86/lapic/lapic_cpu_init.c M src/cpu/x86/mp_init.c M src/device/cpu_device.c M src/include/cpu/cpu.h M src/include/device/path.h 7 files changed, 66 insertions(+), 52 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/64342/11 -- To view, visit https://review.coreboot.org/c/coreboot/+/64342 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4d9f8d23c0b0e5c142f6907593428d8509e4e7bb Gerrit-Change-Number: 64342 Gerrit-PatchSet: 11 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: soc/intel/xeon_sp: Remove unused 'soc_chip.h' file
by Elyes Haouas (Code Review) 04 Nov '22

04 Nov '22

Attention is currently required from: Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Tim Chu. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69207 ) Change subject: soc/intel/xeon_sp: Remove unused 'soc_chip.h' file ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/xeon_sp/include/soc/soc_chip.h: https://review.coreboot.org/c/coreboot/+/69207/comment/01bf20fd_f111e559 PS1, Line 6: ../chip.h src/soc/intel/xeon_sp/cpx/chip.h or src/soc/intel/xeon_sp/skx/chip.h ? -- To view, visit https://review.coreboot.org/c/coreboot/+/69207 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icb968972bf55f4792ae71255aac4b87e36e395b7 Gerrit-Change-Number: 69207 Gerrit-PatchSet: 1 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 04 Nov 2022 12:34:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2022