From cc22607dbfbab0c9ce42c071b5b3c4a304845313 Mon Sep 17 00:00:00 2001 From: Arthur Heymans Date: Sat, 12 Nov 2022 18:51:04 +0100 Subject: [PATCH] Revert "src/arch/x86: Use core apic id to get cpu_index()" This reverts commit 095c931cf12924da9011b47aa64f4a6f11d89f13. Previously cpu_info() was implemented with a struct on top of an aligned stack. As FSP changed the stack value cpu_info() could not be used in FSP context (which PPI is). Now cpu_info() uses GDT segments, which FSP does not touch so it can be used. This also exports cpu_infos from cpu.c as it's a convenient way to get the struct device * for a certain index. TESTED on aldrvp: FSP-S works and is able to run code on APs. Change-Id: I3a40156ba275b572d7d1913d8c17c24b4c8f6d78 Signed-off-by: Arthur Heymans Reviewed-on: https://review.coreboot.org/c/coreboot/+/69509 Reviewed-by: Subrata Banik Tested-by: build bot (Jenkins) --- src/arch/x86/cpu.c | 26 +------------------ src/arch/x86/include/arch/cpu.h | 21 +++++---------- src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c | 19 +++++++------- src/soc/intel/xeon_sp/cpx/cpu.c | 2 +- src/soc/intel/xeon_sp/skx/cpu.c | 2 +- 5 files changed, 20 insertions(+), 50 deletions(-) diff --git a/src/arch/x86/cpu.c b/src/arch/x86/cpu.c index 9fc52330dc..face248fc2 100644 --- a/src/arch/x86/cpu.c +++ b/src/arch/x86/cpu.c @@ -318,33 +318,9 @@ void arch_bootstate_coreboot_exit(void) mp_park_aps(); } -/* - * Previously cpu_index() implementation assumes that cpu_index() - * function will always getting called from coreboot context - * (ESP stack pointer will always refer to coreboot). - * - * But with MP_SERVICES_PPI implementation in coreboot this - * assumption might not be true, where FSP context (stack pointer refers - * to FSP) will request to get cpu_index(). - * - * Hence new logic to use cpuid to fetch lapic id and matches with - * cpus_default_apic_id[] variable to return correct cpu_index(). - */ -int cpu_index(void) -{ - int i; - int lapic_id = initial_lapicid(); - - for (i = 0; i < CONFIG_MAX_CPUS; i++) { - if (cpu_get_apic_id(i) == lapic_id) - return i; - } - return -1; -} - /* cpu_info() looks at address 0 at the base of %gs for a pointer to struct cpu_info */ static struct per_cpu_segment_data segment_data[CONFIG_MAX_CPUS]; -static struct cpu_info cpu_infos[CONFIG_MAX_CPUS]; +struct cpu_info cpu_infos[CONFIG_MAX_CPUS]; enum cb_err set_cpu_info(unsigned int index, struct device *cpu) { diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index c7c6b4e074..18bc961828 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -161,6 +161,13 @@ static inline struct cpu_info *cpu_info(void) return ci; } +static inline unsigned long cpu_index(void) +{ + struct cpu_info *ci; + ci = cpu_info(); + return ci->index; +} + struct cpuinfo_x86 { uint8_t x86; /* CPU family */ uint8_t x86_vendor; /* CPU vendor */ @@ -212,20 +219,6 @@ uint32_t cpu_get_feature_flags_ecx(void); */ uint32_t cpu_get_feature_flags_edx(void); -/* - * Previously cpu_index() implementation assumes that cpu_index() - * function will always getting called from coreboot context - * (ESP stack pointer will always refer to coreboot). - * - * But with MP_SERVICES_PPI implementation in coreboot this - * assumption might not be true, where FSP context (stack pointer refers - * to FSP) will request to get cpu_index(). - * - * Hence new logic to use cpuid to fetch lapic id and matches with - * cpus_default_apic_id[] variable to return correct cpu_index(). - */ -int cpu_index(void); - #define DETERMINISTIC_CACHE_PARAMETERS_CPUID_IA 0x04 #define DETERMINISTIC_CACHE_PARAMETERS_CPUID_AMD 0x8000001d 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 66bbc2f085..d286ee325b 100644 --- a/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c +++ b/src/drivers/intel/fsp2_0/ppi/mp_service_ppi.c @@ -31,16 +31,17 @@ efi_return_status_t mp_get_processor_info(efi_uintn_t processor_number, int apicid; uint8_t package, core, thread; - if (cpu_index() < 0) + if (processor_number >= MIN(get_cpu_count(), CONFIG_MAX_CPUS)) + return FSP_NOT_FOUND; + + extern struct cpu_info cpu_infos[]; + struct cpu_info *info = &cpu_infos[processor_number]; + if (!info) 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); + apicid = info->cpu->path.apic.apic_id; if (apicid < 0) return FSP_DEVICE_ERROR; @@ -66,7 +67,7 @@ efi_return_status_t mp_get_processor_info(efi_uintn_t processor_number, efi_return_status_t mp_startup_all_aps(efi_ap_procedure procedure, bool run_serial, efi_uintn_t timeout_usec, void *argument) { - if (cpu_index() < 0) + if (!cpu_info()) return FSP_DEVICE_ERROR; if (procedure == NULL) @@ -84,7 +85,7 @@ efi_return_status_t mp_startup_all_aps(efi_ap_procedure procedure, efi_return_status_t mp_startup_all_cpus(efi_ap_procedure procedure, efi_uintn_t timeout_usec, void *argument) { - if (cpu_index() < 0) + if (!cpu_info()) return FSP_DEVICE_ERROR; if (procedure == NULL) @@ -119,7 +120,7 @@ efi_return_status_t mp_startup_all_cpus(efi_ap_procedure procedure, efi_return_status_t mp_startup_this_ap(efi_ap_procedure procedure, efi_uintn_t processor_number, efi_uintn_t timeout_usec, void *argument) { - if (cpu_index() < 0) + if (!cpu_info()) return FSP_DEVICE_ERROR; if (processor_number > get_cpu_count()) diff --git a/src/soc/intel/xeon_sp/cpx/cpu.c b/src/soc/intel/xeon_sp/cpx/cpu.c index 4ecfad0f89..8a7dd2ab20 100644 --- a/src/soc/intel/xeon_sp/cpx/cpu.c +++ b/src/soc/intel/xeon_sp/cpx/cpu.c @@ -79,7 +79,7 @@ static void each_cpu_init(struct device *cpu) { msr_t msr; - printk(BIOS_SPEW, "%s dev: %s, cpu: %d, apic_id: 0x%x\n", + printk(BIOS_SPEW, "%s dev: %s, cpu: %lu, apic_id: 0x%x\n", __func__, dev_path(cpu), cpu_index(), cpu->path.apic.apic_id); /* diff --git a/src/soc/intel/xeon_sp/skx/cpu.c b/src/soc/intel/xeon_sp/skx/cpu.c index d2e4af1cd9..24984e14e0 100644 --- a/src/soc/intel/xeon_sp/skx/cpu.c +++ b/src/soc/intel/xeon_sp/skx/cpu.c @@ -59,7 +59,7 @@ static void xeon_sp_core_init(struct device *cpu) { msr_t msr; - printk(BIOS_INFO, "%s dev: %s, cpu: %d, apic_id: 0x%x\n", + printk(BIOS_INFO, "%s dev: %s, cpu: %lu, apic_id: 0x%x\n", __func__, dev_path(cpu), cpu_index(), cpu->path.apic.apic_id); assert(chip_config);